| 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
| ||
|
Message-ID:
<DM3PR11MB87369E36CA76C1BB7C78CEB7EC5EA@DM3PR11MB8736.namprd11.prod.outlook.com>
Date: Thu, 24 Jul 2025 02:28:56 +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 Wed, Jul 23, 2025 at 02:25:27AM +0000, Tristram.Ha@...rochip.com wrote:
> > > 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.
>
> Perhaps naively, I would have expected
>
> .val_format_endian = REGMAP_ENDIAN_LITTLE
>
> to handle writing the 16-bit value 0x7ff such that 0x7 is in
> the lower byte, while 0xff is in the upper byte. Is that not the case?
>
> If not, do you get the desired result by removing the swab16() calls
> and using
>
> .val_format_endian = REGMAP_ENDIAN_BIG
>
> But perhaps I misunderstand how .val_format_endian works.
>
> >
> > 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.
It is not about big-endian or little-endian. It is just the presentation
of this register is different between KSZ8863 and KSZ8463. KSZ8863 uses
big-endian for register value as the access is 8-bit and the address is
automatically increased by 1. Writing a value 0x03ff to register 6 means
6=0x03 and 7=0xff. The actual SPI transfer commands are "02 06 03 ff."
KSZ8463 uses little-endian for register value as the access is fixed at
8-bit, 16-bit, or 32-bit. Writing 0x03ff results in the actual SPI
transfer commands "80 70 ff 03" where the correct commands are
"80 70 03 ff."
Powered by blists - more mailing lists