Re: [PATCH net-next v3 1/4] net: dsa: add support for DSA rx offloading via metadata dst

[Felix Fietkau]

On 12.11.22 00:37, Vladimir Oltean wrote:
> On Thu, Nov 10, 2022 at 10:22:08PM +0100, Felix Fietkau wrote:
> > If a metadata dst is present with the type METADATA_HW_PORT_MUX on a dsa cpu
> > port netdev, assume that it carries the port number and that there is no DSA
> > tag present in the skb data.
> >
> > Signed-off-by: Felix Fietkau <nbd@xxxxxxxx>
> >  				else
> > diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> > index 64b14f655b23..0b67622cf905 100644
> > --- a/net/dsa/dsa.c
> > +++ b/net/dsa/dsa.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/netdevice.h>
> >  #include <linux/sysfs.h>
> >  #include <linux/ptp_classify.h>
> > +#include <net/dst_metadata.h>
> >  
> >  #include "dsa_priv.h"
> >  
> > @@ -216,6 +217,7 @@ static bool dsa_skb_defer_rx_timestamp(struct dsa_slave_priv *p,
> >  static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
> >  			  struct packet_type *pt, struct net_device *unused)
> >  {
> > +	struct metadata_dst *md_dst = skb_metadata_dst(skb);
> >  	struct dsa_port *cpu_dp = dev->dsa_ptr;
> >  	struct sk_buff *nskb = NULL;
> >  	struct dsa_slave_priv *p;
> > @@ -229,7 +231,22 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
> >  	if (!skb)
> >  		return 0;
> >  
> > -	nskb = cpu_dp->rcv(skb, dev);
> > +	if (md_dst && md_dst->type == METADATA_HW_PORT_MUX) {
> > +		unsigned int port = md_dst->u.port_info.port_id;
> > +
> > +		skb_dst_set(skb, NULL);
>
> If you insist on not using the refcounting feature and free your
> metadata_dst in the master's remove() function, that's going to
> invalidate absolutely any point I'm trying to make. Normally I'd leave
> you alone, however I really don't like that this is also forcing DSA to
> not use the refcount, and therefore, that it's forcing any other driver
> to do the same as mtk_eth_soc. Not sure how that's gonna scale in the
> hypothetical future when there will be a DSA master which can offload
> RX DSA tags, *and* the switch can change tagging protocols dynamically
> (which will force the master to allocate/free its metadata dst's at
> runtime too). I guess that will be for me to figure out, which I don't
> like.
>
> Jakub, what do you think? Refcounting or no refcounting?
If think that refcounting for the metadata dst is useful in this case, I 
can replace the skb_dst_set call with skb_dst_drop() in v4, so it would 
work for both cases.

- Felix