Re: [net-next PATCH 06/10] net: dsa: realtek: rtl8365mb: add VLAN support

[Yury Norov <ynorov@nvidia.com>]

On Wed, Apr 01, 2026 at 11:45:18PM -0300, Luiz Angelo Daros de Luca wrote:
> > > +     vlan4k->vid = vid;
> > > +     vlan4k->member =
> > > +             FIELD_GET(RTL8365MB_CVLAN_ENTRY_D0_MBR_MASK, data[0]) |
> > > +             (FIELD_GET(RTL8365MB_CVLAN_ENTRY_D2_MBR_EXT_MASK, data[2])
> > > +              << FIELD_WIDTH(RTL8365MB_CVLAN_ENTRY_D0_MBR_MASK));
> >
> > This FIELD_GET() << FIELD_WIDTH() resembles FIELD_PREP(), except that
> > you skip some checks. Is that intentional?
> 
> While it resembles FIELD_PREP(), the usage of FIELD_GET() <<
> FIELD_WIDTH() is intentional and semantically distinct from
> FIELD_PREP(). While FIELD_PREP() prepares a raw value to be written up
> into a register mask, we are performing a read/unpack operation here:
> extracting bits from hardware registers to populate the driver's
> internal logical structure.
> 
> Using FIELD_PREP() for this would require defining a 'fake' software
> mask (e.g., GENMASK(10, 8)) that does not correspond to any physical
> register, which would be more confusing to future maintainers than the
> current explicit reassembly.

Not sure. You can add a nice comment on top of those GENMASKS(),
explaining what actually is happening there. Long term it always
better to avoid opencoded shifts. It's proven by decades of coding.

All that FIELD() machinery is aimed to avoid shifts and other
opencoded bit manipulations, because they are proven to be a constant
source of errors. And now you use that FIELD_WIDTH() exactly for that.

The alternative would look like:

 lo = FIELD_GET(RTL8365MB_CVLAN_ENTRY_D0_MBR_MASK, data[0]);
 hi = FIELD_GET(RTL8365MB_CVLAN_ENTRY_D2_MBR_EXT_MASK, data[2]);
 vlan4k->member = FIELD_PREP(RTL8365MB_CVLAN_ENTRY_MBR_MASK_LO, lo) |
                  FIELD_PREP(RTL8365MB_CVLAN_ENTRY_MBR_EXT_MASK_HI, hi);

I'm not going to teach you how to wright your driver, but you see -
it's 4 lines vs 4 lines, and no opencoded bit ops. And I'm pretty sure
you'll find plenty examples in the kernel where people construct
registers from pieces this way.

Thanks,
Yury

> The 11-bit member field is split across two non-contiguous 16-bit
> words in the CVLAN entry:
> - data[0] (D0): Contains the primary bits [7:0] via
> RTL8365MB_CVLAN_ENTRY_D0_MBR_MASK.
> - data[2] (D2): Contains the extension bits [10:8] via
> RTL8365MB_CVLAN_ENTRY_D2_MBR_EXT_MASK.
> 
> This split layout suggests the hardware was originally designed for 8
> ports and later expanded to 11, placing the extra bits in the
> previously unused space in data[2].
> Even if we concatenate them into a single 48-bit value, a single
> bitfield macro would not handle the non-contiguous mask.