| Message ID | 20190830004635.24863-11-olteanv@gmail.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | tc-taprio offload for SJA1105 DSA | expand |
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
Hi Kurt, On Mon, 2 Sep 2019 at 10:52, Kurt Kanzenbach <kurt.kanzenbach@linutronix.de> wrote: > > 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. > No, this isn't needed. It is a remnant from v1. > 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; > } > I did something similar in v1 with a .port_setup_taprio in "[RFC PATCH net-next 3/6] net: dsa: Pass tc-taprio offload to drivers". Would this address Ilias's comment about DSA not really needing to have this level of awareness into the qdisc offload type? Rightfully I can agree that the added-value of making a .port_set_schedule and .port_del_schedule in DSA compared to simply passing the ndo_setup_tc is not that great. By the way, thanks for the iproute2 patch for parsing 64-bit base time on ARM 32, saved me a bit of debugging time :) > Thanks, > Kurt Regards, -Vladimir
Hi Vladimir, On Mon, Sep 02, 2019 at 11:49:30AM +0300, Vladimir Oltean wrote: > I did something similar in v1 with a .port_setup_taprio in "[RFC PATCH > net-next 3/6] net: dsa: Pass tc-taprio offload to drivers". Okay, didn't see that one. > Would this address Ilias's comment about DSA not really needing to > have this level of awareness into the qdisc offload type? Rightfully I > can agree that the added-value of making a .port_set_schedule and > .port_del_schedule in DSA compared to simply passing the ndo_setup_tc > is not that great. I wanted to avoid that drivers have to the same kind of work, and it put it therefore into the core part. However, I agree that the added-value is not that high for TAPRIO. > > By the way, thanks for the iproute2 patch for parsing 64-bit base time > on ARM 32, saved me a bit of debugging time :) No problem :). That cost me a bit of time. > Regards, > -Vladimir Thanks, Kurt
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; struct dsa_port { /* A CPU port is physically connected to a master device. @@ -515,6 +516,8 @@ struct dsa_switch_ops { bool ingress); void (*port_mirror_del)(struct dsa_switch *ds, int port, struct dsa_mall_mirror_tc_entry *mirror); + int (*port_setup_tc)(struct dsa_switch *ds, int port, + enum tc_setup_type type, void *type_data); /* * Cross-chip operations diff --git a/net/dsa/slave.c b/net/dsa/slave.c index 9a88035517a6..75d58229a4bd 100644 --- a/net/dsa/slave.c +++ b/net/dsa/slave.c @@ -1035,12 +1035,16 @@ static int dsa_slave_setup_tc_block(struct net_device *dev, static int dsa_slave_setup_tc(struct net_device *dev, enum tc_setup_type type, void *type_data) { - switch (type) { - case TC_SETUP_BLOCK: + struct dsa_port *dp = dsa_slave_to_port(dev); + struct dsa_switch *ds = dp->ds; + + if (type == TC_SETUP_BLOCK) return dsa_slave_setup_tc_block(dev, type_data); - default: + + if (!ds->ops->port_setup_tc) return -EOPNOTSUPP; - } + + return ds->ops->port_setup_tc(ds, dp->index, type, type_data); } static void dsa_slave_get_stats64(struct net_device *dev,
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. 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(-)