netdev - RE: [PATCH net-next v4 4/7] net: dsa: microchip: Use different registers for KSZ8463

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 for Android: free password hash cracker in your pocket

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

Message-ID:
 <DM3PR11MB873641FBBF2A79E787F13877EC5FA@DM3PR11MB8736.namprd11.prod.outlook.com>
Date: Wed, 23 Jul 2025 02:25:27 +0000
From: <Tristram.Ha@...rochip.com>
To: <horms@...nel.org>
CC: <Woojung.Huh@...rochip.com>, <andrew@...n.ch>, <olteanv@...il.com>,
	<kuba@...nel.org>, <robh@...nel.org>, <krzk+dt@...nel.org>,
	<conor+dt@...nel.org>, <maxime.chevallier@...tlin.com>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <pabeni@...hat.com>,
	<marex@...x.de>, <UNGLinuxDriver@...rochip.com>,
	<devicetree@...r.kernel.org>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH net-next v4 4/7] net: dsa: microchip: Use different
 registers for KSZ8463

> On Sun, Jul 20, 2025 at 11:17:03AM +0100, Simon Horman wrote:
> > On Fri, Jul 18, 2025 at 06:21:03PM -0700, Tristram.Ha@...rochip.com wrote:
> > > From: Tristram Ha <tristram.ha@...rochip.com>
> > >
> > > KSZ8463 does not use same set of registers as KSZ8863 so it is necessary
> > > to change some registers when using KSZ8463.
> > >
> > > Signed-off-by: Tristram Ha <tristram.ha@...rochip.com>
> > > ---
> > > v3
> > > - Replace cpu_to_be16() with swab16() to avoid compiler warning
> >
> > ...
> >
> > > diff --git a/drivers/net/dsa/microchip/ksz_common.c
> b/drivers/net/dsa/microchip/ksz_common.c
> >
> > ...
> >
> > > @@ -2980,10 +2981,15 @@ static int ksz_setup(struct dsa_switch *ds)
> > >     }
> > >
> > >     /* set broadcast storm protection 10% rate */
> > > -   regmap_update_bits(ksz_regmap_16(dev), regs[S_BROADCAST_CTRL],
> > > -                      BROADCAST_STORM_RATE,
> > > -                      (BROADCAST_STORM_VALUE *
> > > -                      BROADCAST_STORM_PROT_RATE) / 100);
> > > +   storm_mask = BROADCAST_STORM_RATE;
> > > +   storm_rate = (BROADCAST_STORM_VALUE *
> BROADCAST_STORM_PROT_RATE) / 100;
> > > +   if (ksz_is_ksz8463(dev)) {
> > > +           storm_mask = swab16(storm_mask);
> > > +           storm_rate = swab16(storm_rate);
> > > +   }
> > > +   regmap_update_bits(ksz_regmap_16(dev),
> > > +                      reg16(dev, regs[S_BROADCAST_CTRL]),
> > > +                      storm_mask, storm_rate);
> >
> > Hi Tristram,
> >
> > I am confused by the use of swab16() here.
> >
> > Let us say that we are running on a little endian host (likely).
> > Then the effect of this is to pass big endian values to regmap_update_bits().
> >
> > But if we are running on a big endian host, the opposite will be true:
> > little endian values will be passed to regmap_update_bits().
> >
> >
> > Looking at KSZ_REGMAP_ENTRY() I see:
> >
> > #define KSZ_REGMAP_ENTRY(width, swp, regbits, regpad, regalign)         \
> >         {                                                               \
> >               ...
> >                 .reg_format_endian = REGMAP_ENDIAN_BIG,                 \
> >                 .val_format_endian = REGMAP_ENDIAN_BIG                  \
> >         }
> 
> Update; I now see this in another patch of the series:
> 
> +#define KSZ8463_REGMAP_ENTRY(width, swp, regbits, regpad, regalign)    \
> +       {                                                               \
>                 ...
> +               .reg_format_endian = REGMAP_ENDIAN_BIG,                 \
> +               .val_format_endian = REGMAP_ENDIAN_LITTLE               \
> +       }
> 
> Which I understand to mean that the hardware is expecting little endian
> values. But still, my concerns raised in my previous email of this
> thread remain.
> 
> And I have a question: does this chip use little endian register values
> whereas other chips used big endian register values?
> 
> >
> > Which based on a skimming the regmap code implies to me that
> > regmap_update_bits() should be passed host byte order values
> > which regmap will convert to big endian when writing out
> > these values.
> >
> > It is unclear to me why changing the byte order of storm_mask
> > and storm_rate is needed here. But it does seem clear that
> > it will lead to inconsistent results on big endian and little
> > endian hosts.

The broadcast storm value 0x7ff is stored in registers 6 and 7 in KSZ8863
where register 6 holds the 0x7 part while register 7 holds the 0xff part.
In KSZ8463 register 6 is defined as 16-bit where the 0x7 part is held in
lower byte and the 0xff part is held in higher byte.  It is necessary to
swap the bytes when the value is passed to the 16-bit write function.

All other KSZ switches use 8-bit access with automatic address increase
so a write to register 0 with value 0x12345678 means 0=0x12, 1=0x34,
2=0x56, and 3=0x78.