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

[Kurt Kanzenbach <kurt.kanzenbach@linutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3694 bytes --]

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 <olteanv@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]