Hi Kurt,
On Mon, 2 Sep 2019 at 10:52, Kurt Kanzenbach
<kurt.kanzenb...@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 <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.
>
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
