netdev - Re: [PATCH net-next 02/15] net: dsa: tag_brcm: use the dsa_xmit_port

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251127134212.wh7d7djiv23lyv2e@skbuf>
Date: Thu, 27 Nov 2025 15:42:12 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Jonas Gorski <jonas.gorski@...il.com>
Cc: netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Florian Fainelli <florian.fainelli@...adcom.com>
Subject: Re: [PATCH net-next 02/15] net: dsa: tag_brcm: use the
 dsa_xmit_port_mask() helper

On Thu, Nov 27, 2025 at 02:29:27PM +0100, Jonas Gorski wrote:
> > @@ -119,10 +120,9 @@ static struct sk_buff *brcm_tag_xmit_ll(struct sk_buff *skb,
> >         brcm_tag[0] = (1 << BRCM_OPCODE_SHIFT) |
> >                        ((queue & BRCM_IG_TC_MASK) << BRCM_IG_TC_SHIFT);
> >         brcm_tag[1] = 0;
> > -       brcm_tag[2] = 0;
> > -       if (dp->index == 8)
> > -               brcm_tag[2] = BRCM_IG_DSTMAP2_MASK;
> > -       brcm_tag[3] = (1 << dp->index) & BRCM_IG_DSTMAP1_MASK;
> > +       port_mask = dsa_xmit_port_mask(skb, dev);
> > +       brcm_tag[2] = (port_mask >> 8) & BRCM_IG_DSTMAP2_MASK;
> > +       brcm_tag[3] = port_mask & BRCM_IG_DSTMAP1_MASK;
> 
> Since this is a consecutive bitmask (actually [22:0]), I wonder if doing
> 
> put_unaligned_be16(port_mask, &brcm_tag[2]);
> 
> would be a bit more readable.
> 
> Or even more correct put_unaligned_be24(port_mask, &brcm_tag[1]), but
> we don't support any switches with that many ports.

I don't think there's a technical requirement for unaligned access here.
We have to use unaligned access for tail tags, but this here is handling
EtherType and/or prepended headers, which are both aligned to a boundary
of 2 due to NET_IP_ALIGN AFAIK.

Anyway, with the replacement of u8 by u8 -> u16 by u16 access I do agree
it would be a good idea, as long as u16 is used throughout (not just in
order not to split the bit mask). Alternatively if there are more than
16 ports, there's pack() which tolerates bit fields crossing any
alignment boundary (but still performs u8 by u8 access).

But I don't feel like changing the access pattern of the tagger is in
the scope of this change set.