Re: [PATCH net-next v2 3/4] net: dsa: yt921x: Add port police support

[David Yang]

On Fri, Apr 3, 2026 at 8:41 AM Andrew Lunn <andrew@xxxxxxx> wrote:
>
> > +static void update_ctrls_unaligned(u32 *ctrls, u64 mask, u64 val)
> > +{
> > +     ctrls[0] &= ~((u32)mask);
> > +     ctrls[1] &= ~((u32)(mask >> 32));
> > +     ctrls[0] |= (u32)val;
> > +     ctrls[1] |= (u32)(val >> 32);
>
> Please use the macros upper_32_bits() and lower_32_bits().
>
> > +     update_ctrls_unaligned(&ctrls[0], YT921X_METER_CTRLab_EBS_M,
> > +                            YT921X_METER_CTRLab_EBS(meter.ebs));
> > +     update_ctrls_unaligned(&ctrls[1], YT921X_METER_CTRLbc_CBS_M,
> > +                            YT921X_METER_CTRLbc_CBS(meter.cbs));
>
> That looks odd. The first call to update_ctrls_unaligned() writes to
> ctrls[0] and ctrl[1]. The second call ORs into ctrls[1] and writes
> to ctrls[2]?
>
> When you look at the masks:
>
> > +#define  YT921X_METER_CTRLbc_CBS_M           GENMASK_ULL(35, 20)
> > +#define  YT921X_METER_CTRLab_EBS_M           GENMASK_ULL(33, 18)
>
> They cross the 32 bit boundary. I think it would be cleaner to deal
> with these using GENMASK_U128 and construct the value in a u128. Pass
> the u128 to a reg96_write() and let it discard the upper 32 bits?
>
>     Andrew

Does it unnecessarily depend on __int128? Otherwise it's good.