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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20250720102224.GR2459@horms.kernel.org>
Date: Sun, 20 Jul 2025 11:22:24 +0100
From: Simon Horman <horms@...nel.org>
To: Tristram.Ha@...rochip.com
Cc: Woojung Huh <woojung.huh@...rochip.com>, Andrew Lunn <andrew@...n.ch>,
	Vladimir Oltean <olteanv@...il.com>,
	Jakub Kicinski <kuba@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Marek Vasut <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.
> 
> ...
>