Re: [RFC PATCH v2 net-next 10/15] net: dsa: Pass ndo_setup_tc slave callback to drivers

[Kurt Kanzenbach]

Hi,

On Fri, Aug 30, 2019 at 03:46:30AM +0300, Vladimir Oltean wrote:
> DSA currently handles shared block filters (for the classifier-action
> qdisc) in the core due to what I believe are simply pragmatic reasons -
> hiding the complexity from drivers and offerring a simple API for port
> mirroring.
>
> Extend the dsa_slave_setup_tc function by passing all other qdisc
> offloads to the driver layer, where the driver may choose what it
> implements and how. DSA is simply a pass-through in this case.

I'm having the same problem on how to pass the taprio schedule down to
the DSA driver. I didn't perform a pass-through to keep it in sync with
the already implemented offload. See my approach below.

>
> There is an open question related to the drivers potentially needing to
> do work in process context, but .ndo_setup_tc is called in atomic
> context. At the moment the drivers are left to handle this on their own.
> The risk is that once accepting the offload callback right away in the
> DSA core, then the driver would have no way to signal an error back. So
> right now the driver has to do as much error checking as possible in the
> atomic context and only defer (probably) the actual configuring of the
> offload.
>
> Signed-off-by: Vladimir Oltean <olte...@gmail.com>
> ---
>  include/net/dsa.h |  3 +++
>  net/dsa/slave.c   | 12 ++++++++----
>  2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 96acb14ec1a8..232b5d36815d 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -154,6 +154,7 @@ struct dsa_mall_tc_entry {
>       };
>  };
>
> +struct tc_taprio_qopt_offload;

Is this needed? The rest looks good to me.

My approach:

diff --git a/include/net/dsa.h b/include/net/dsa.h
index ba6dfff98196..a60bd55f27f2 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -20,6 +20,7 @@
 #include <linux/platform_data/dsa.h>
 #include <net/devlink.h>
 #include <net/switchdev.h>
+#include <net/pkt_sched.h>

 struct tc_action;
 struct phy_device;
@@ -539,6 +540,13 @@ struct dsa_switch_ops {
         */
        netdev_tx_t (*port_deferred_xmit)(struct dsa_switch *ds, int port,
                                          struct sk_buff *skb);
+
+       /*
+        * Scheduled traffic functionality
+        */
+       int (*port_set_schedule)(struct dsa_switch *ds, int port,
+                                const struct tc_taprio_qopt_offload *taprio);
+       int (*port_del_schedule)(struct dsa_switch *ds, int port);
 };

 struct dsa_switch_driver {
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 8157be7e162d..6290d55e6011 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -15,6 +15,7 @@
 #include <linux/mdio.h>
 #include <net/rtnetlink.h>
 #include <net/pkt_cls.h>
+#include <net/pkt_sched.h>
 #include <net/tc_act/tc_mirred.h>
 #include <linux/if_bridge.h>
 #include <linux/netpoll.h>
@@ -953,12 +954,33 @@ static int dsa_slave_setup_tc_block(struct net_device 
*dev,
        }
 }

+static int dsa_slave_setup_tc_taprio(struct net_device *dev,
+                                    const struct tc_taprio_qopt_offload 
*taprio)
+{
+       struct dsa_port *dp = dsa_slave_to_port(dev);
+       struct dsa_switch *ds = dp->ds;
+
+       if (taprio->enable) {
+               if (!ds->ops->port_set_schedule)
+                       return -EOPNOTSUPP;
+
+               return ds->ops->port_set_schedule(ds, dp->index, taprio);
+       }
+
+       if (!ds->ops->port_del_schedule)
+               return -EOPNOTSUPP;
+
+       return ds->ops->port_del_schedule(ds, dp->index);
+}
+
 static int dsa_slave_setup_tc(struct net_device *dev, enum tc_setup_type type,
                              void *type_data)
 {
        switch (type) {
        case TC_SETUP_BLOCK:
                return dsa_slave_setup_tc_block(dev, type_data);
+       case TC_SETUP_QDISC_TAPRIO:
+               return dsa_slave_setup_tc_taprio(dev, type_data);
        default:
                return -EOPNOTSUPP;
        }

Thanks,
Kurt

signature.asc
Description: PGP signature