Date:   Mon, 2 Sep 2019 11:49:30 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Kurt Kanzenbach <kurt.kanzenbach@...utronix.de>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        vedang.patel@...el.com, Richard Cochran <richardcochran@...il.com>,
        weifeng.voon@...el.com, jiri@...lanox.com, m-karicheri2@...com,
        Jose.Abreu@...opsys.com,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        xiyou.wangcong@...il.com, netdev <netdev@...r.kernel.org>
Subject: Re: [RFC PATCH v2 net-next 10/15] net: dsa: Pass ndo_setup_tc slave
 callback to drivers

Hi Kurt,

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

Powered by blists - more mailing lists