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

Felix Fietkau
2022-11-12T00:05:27Z

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@nbd.name>
>> 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