Message-ID: <20251128192558.76f7ca56@pumpkin>
Date: Fri, 28 Nov 2025 19:25:58 +0000
From: david laight <david.laight@...box.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: David Yang <mmyangfl@...il.com>, netdev@...r.kernel.org, Andrew Lunn
 <andrew@...n.ch>, Vladimir Oltean <olteanv@...il.com>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman
 <horms@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v3 1/4] net: dsa: yt921x: Use *_ULL bitfield
 macros for VLAN_CTRL

On Fri, 28 Nov 2025 11:43:17 +0000
"Russell King (Oracle)" <linux@...linux.org.uk> wrote:

> On Fri, Nov 28, 2025 at 10:51:41AM +0000, david laight wrote:
> > On Wed, 26 Nov 2025 17:32:34 +0800
> > David Yang <mmyangfl@...il.com> wrote:
> >   
> > > VLAN_CTRL should be treated as a 64-bit register. GENMASK and BIT
> > > macros use unsigned long as the underlying type, which will result in a
> > > build error on architectures where sizeof(long) == 4.  
> > 
> > I suspect GENMASK() should generate u32 or u64 depending on the value
> > of a constant 'high bit'.  
> 
> I suggest checking before making such statements to save embarrasment.
> The above is incorrect.

Is was a suggestion about what it would be a good idea for it to do,
not a statement about what it actually does.
I know GENMASK() generates 'unsigned long'.
The problem is that (as here) if the value is larger than u32 you get
a build error on 32bit,
Then you get the opposite problem that any portable code has to handle
the fact that the result type is (effectively) u64 on 64bit so
you get unwanted (or just unnecessary) integer promotions where the
result is used.
Given that pretty much all GENMASK() are used for hardware bit-patterns
not having a fixed size result type seems wrong.

The newly added GENMASK_U8() and GENMASK_U16() are also pointless.
Integer promotion converts the value to 'signed int' before it
is used.

>... 
> > I found code elsewhere that doesn't really want FIELD_PREP() to
> > generate a 64bit value.
> > 
> > There are actually a lot of dubious uses of 'long' throughout
> > the kernel that break on 32bit.
> > (Actually pretty much all of them!)  
> 
> If you're referring to the use of GENMASK() with bitfields larger
> than 32-bits, then as can be seen from the above, the code wouldn't
> even compile and our CI systems would be screaming about it. They
> aren't, so I think your statement here is also wrong.

No, I'm talking about a 'normal' FIELD_PREP(GENMASK(7, 5), val) having
a 64bit type on 64bit.
It tripped up my min_t(int, ) 'cast truncation test' a few times :-)

The problem with with 'long' is more pervasive.
It gets used for 'quite big' values that are clearly independent of whether
a 32bit or 64bit kernel is being built.
You're going to want to see an example, I think this overflows badly on 32bit:
static long pll1443x_calc_kdiv(int mdiv, int pdiv, int sdiv,
		unsigned long rate, unsigned long prate)
{
	long kdiv;

	/* calc kdiv = round(rate * pdiv * 65536 * 2^sdiv / prate) - (mdiv * 65536) */
	kdiv = ((rate * ((pdiv * 65536) << sdiv) + prate / 2) / prate) - (mdiv * 65536);

	return clamp_t(short, kdiv, KDIV_MIN, KDIV_MAX);
}
https://elixir.bootlin.com/linux/v6.18-rc6/source/drivers/clk/imx/clk-pll14xx.c#L120
(Spot the non-functional clamp_t() - which is why I found this code.)

I'm sure I've seen fs code comparing u64 and ulong that both hold block numbers.
Such code should really only use fixed size types.
Either a value fits in 32bits, so int or u32 is fine; or it doesn't and you
need a u64. 'long' doesn't enter into it.

	David

Powered by blists - more mailing lists