[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240620122816.qho2hq7i4eakpnbc@skbuf>
Date: Thu, 20 Jun 2024 15:28:16 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Pawel Dembicki <paweldembicki@...il.com>
Cc: netdev@...r.kernel.org, Paolo Abeni <pabeni@...hat.com>,
Vladimir Oltean <vladimir.oltean@....com>,
Andrew Lunn <andrew@...n.ch>,
Florian Fainelli <f.fainelli@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Claudiu Manoil <claudiu.manoil@....com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
UNGLinuxDriver@...rochip.com, Russell King <linux@...linux.org.uk>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 04/12] net: dsa: tag_sja1105: absorb entire
sja1105_vlan_rcv() into dsa_8021q_rcv()
On Wed, Jun 19, 2024 at 10:52:10PM +0200, Pawel Dembicki wrote:
> From: Vladimir Oltean <vladimir.oltean@....com>
>
> tag_sja1105 has a wrapper over dsa_8021q_rcv(): sja1105_vlan_rcv(),
> which determines whether the packet came from a bridge with
> vlan_filtering=1 (the case resolved via
> dsa_find_designated_bridge_port_by_vid()), or if it contains a tag_8021q
> header.
>
> Looking at a new tagger implementation for vsc73xx, based also on
> tag_8021q, it is becoming clear that the logic is needed there as well.
> So instead of forcing each tagger to wrap around dsa_8021q_rcv(), let's
> merge the logic into the core.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> Signed-off-by: Pawel Dembicki <paweldembicki@...il.com>
> ---
> v2,v1:
> - resend only
> ---
> Before patch series split:
> https://patchwork.kernel.org/project/netdevbpf/list/?series=841034&state=%2A&archive=both
> v8, v7, v6:
> - resend only
> v5:
> - add missing SoB
> v4:
> - introduced patch
> ---
> net/dsa/tag_8021q.c | 34 ++++++++++++++++++++++++++++------
> net/dsa/tag_8021q.h | 2 +-
> net/dsa/tag_ocelot_8021q.c | 2 +-
> net/dsa/tag_sja1105.c | 32 ++++----------------------------
> 4 files changed, 34 insertions(+), 36 deletions(-)
>
> diff --git a/net/dsa/tag_8021q.c b/net/dsa/tag_8021q.c
> index 3cb0293793a5..332b0ae02645 100644
> --- a/net/dsa/tag_8021q.c
> +++ b/net/dsa/tag_8021q.c
> @@ -507,27 +507,39 @@ EXPORT_SYMBOL_GPL(dsa_tag_8021q_find_port_by_vbid);
> * @vbid: pointer to storage for imprecise bridge ID. Must be pre-initialized
> * with -1. If a positive value is returned, the source_port and switch_id
> * are invalid.
> + * @vid: pointer to storage for original VID, in case tag_8021q decoding failed.
> + *
> + * If the packet has a tag_8021q header, decode it and set @source_port,
> + * @switch_id and @vbid, and strip the header. Otherwise set @vid and keep the
> + * header in the hwaccel area of the packet.
> */
> void dsa_8021q_rcv(struct sk_buff *skb, int *source_port, int *switch_id,
> - int *vbid)
> + int *vbid, int *vid)
> {
> int tmp_source_port, tmp_switch_id, tmp_vbid;
> - u16 vid, tci;
> + __be16 vlan_proto;
> + u16 tmp_vid, tci;
>
> if (skb_vlan_tag_present(skb)) {
> + vlan_proto = skb->vlan_proto;
> tci = skb_vlan_tag_get(skb);
> __vlan_hwaccel_clear_tag(skb);
> } else {
> + struct vlan_ethhdr *hdr = vlan_eth_hdr(skb);
> +
> + vlan_proto = hdr->h_vlan_proto;
> skb_push_rcsum(skb, ETH_HLEN);
> __skb_vlan_pop(skb, &tci);
> skb_pull_rcsum(skb, ETH_HLEN);
> }
>
> - vid = tci & VLAN_VID_MASK;
> + tmp_vid = tci & VLAN_VID_MASK;
> + if (!vid_is_dsa_8021q(tmp_vid))
> + goto not_tag_8021q;
I think this may be more clearly expressed linearly, without a goto.
if (!vid_is_dsa_8021q(tmp_vid)) {
/* Not a tag_8021q frame, so return the VID to the
* caller for further processing, and put the tag back
*/
if (vid)
*vid = tmp_vid;
__vlan_hwaccel_put_tag(skb, vlan_proto, tci);
return;
}
>
> - tmp_source_port = dsa_8021q_rx_source_port(vid);
> - tmp_switch_id = dsa_8021q_rx_switch_id(vid);
> - tmp_vbid = dsa_tag_8021q_rx_vbid(vid);
> + tmp_source_port = dsa_8021q_rx_source_port(tmp_vid);
> + tmp_switch_id = dsa_8021q_rx_switch_id(tmp_vid);
> + tmp_vbid = dsa_tag_8021q_rx_vbid(tmp_vid);
>
> /* Precise source port information is unknown when receiving from a
> * VLAN-unaware bridging domain, and tmp_source_port and tmp_switch_id
> @@ -546,5 +558,15 @@ void dsa_8021q_rcv(struct sk_buff *skb, int *source_port, int *switch_id,
> *vbid = tmp_vbid;
>
> skb->priority = (tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT;
> + return;
> +
> +not_tag_8021q:
> + if (vid)
> + *vid = tmp_vid;
> + if (vbid)
> + *vbid = -1;
Thinking more about it, I don't think this is needed (hence it is
missing in my rewritten snippet above). I mean, *vbid is already
initialized with -1 (it's a requirement also specified in the
kernel-doc) and we haven't changed it.
> +
> + /* Put the tag back */
> + __vlan_hwaccel_put_tag(skb, vlan_proto, tci);
> }
> EXPORT_SYMBOL_GPL(dsa_8021q_rcv);
> diff --git a/net/dsa/tag_8021q.h b/net/dsa/tag_8021q.h
> index 41f7167ac520..0c6671d7c1c2 100644
> --- a/net/dsa/tag_8021q.h
> +++ b/net/dsa/tag_8021q.h
> @@ -14,7 +14,7 @@ struct sk_buff *dsa_8021q_xmit(struct sk_buff *skb, struct net_device *netdev,
> u16 tpid, u16 tci);
>
> void dsa_8021q_rcv(struct sk_buff *skb, int *source_port, int *switch_id,
> - int *vbid);
> + int *vbid, int *vid);
>
> struct net_device *dsa_tag_8021q_find_port_by_vbid(struct net_device *conduit,
> int vbid);
Powered by blists - more mailing lists