Re: [PATCH net-next 1/2] net: dsa: untag the bridge pvid from rx skbs

[Vladimir Oltean]

On Tue, Sep 22, 2020 at 08:11:54PM -0700, Florian Fainelli wrote:
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index d16057c5987a..b539241a7533 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -301,6 +301,14 @@ struct dsa_switch {
>        */
>       bool                    configure_vlan_while_not_filtering;
>  
> +     /* If the switch driver always programs the CPU port as egress tagged
> +      * despite the VLAN configuration indicating otherwise, then setting
> +      * @untag_bridge_pvid will force the DSA receive path to pop the 
> bridge's
> +      * default_pvid VLAN tagged frames to offer a consistent behavior
> +      * between a vlan_filtering=0 and vlan_filtering=1 bridge device.
> +      */
> +     bool                    untag_bridge_pvid;
> +
>       /* In case vlan_filtering_is_global is set, the VLAN awareness state
>        * should be retrieved from here and not from the per-port settings.
>        */
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 5c18c0214aac..dec4ab59b7c4 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -225,6 +225,15 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct 
> net_device *dev,
>       skb->pkt_type = PACKET_HOST;
>       skb->protocol = eth_type_trans(skb, skb->dev);
>  
> +     if (unlikely(cpu_dp->ds->untag_bridge_pvid)) {
> +             nskb = dsa_untag_bridge_pvid(skb);
> +             if (!nskb) {
> +                     kfree_skb(skb);
> +                     return 0;
> +             }
> +             skb = nskb;
> +     }
> +
>       s = this_cpu_ptr(p->stats64);
>       u64_stats_update_begin(&s->syncp);
>       s->rx_packets++;

I was thinking a lot simpler. Maybe you could just tail-call
dsa_untag_bridge_pvid(skb) at the end of your .rcv function instead of
putting it in the common receive path. I specifically wrote it to look
at hdr->h_vlan_proto instead of skb->protocol, so it wouldn't depend on
eth_type_trans().

Something like:

diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index 9c6c30649d13..118d253af5a7 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -192,7 +192,7 @@ static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, 
struct net_device *dev,
                nskb->data - ETH_HLEN - BRCM_TAG_LEN,
                2 * ETH_ALEN);
 
-       return nskb;
+       return dsa_untag_bridge_pvid(nskb);
 }
 
 static const struct dsa_device_ops brcm_netdev_ops = {
@@ -220,8 +220,14 @@ static struct sk_buff *brcm_tag_rcv_prepend(struct sk_buff 
*skb,
                                            struct net_device *dev,
                                            struct packet_type *pt)
 {
+       struct sk_buff *nskb;
+
        /* tag is prepended to the packet */
-       return brcm_tag_rcv_ll(skb, dev, pt, ETH_HLEN);
+       nskb = brcm_tag_rcv_ll(skb, dev, pt, ETH_HLEN);
+       if (!nskb)
+               return nskb;
+
+       return dsa_untag_bridge_pvid(nskb);
 }
 
 static const struct dsa_device_ops brcm_prepend_netdev_ops = {

Thanks,
-Vladimir