Re: [PATCH net-next 02/15] net: dsa: tag_brcm: use the dsa_xmit_port_mask() helper

[Vladimir Oltean <vladimir.oltean@nxp.com>]

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.