Re: [PATCH 01/44] x86/asm/bitops: Change the return type of variable__ffs() to unsigned int

[David Laight <david.laight.linux@gmail.com>]

On Mon, 24 Nov 2025 09:58:51 -0500
Yury Norov <yury.norov@gmail.com> wrote:

> On Thu, Nov 20, 2025 at 09:18:10PM +0000, David Laight wrote:
> > On Thu, 20 Nov 2025 13:33:11 -0500
> > Yury Norov <yury.norov@gmail.com> wrote:
> >   
> > > On Thu, Nov 20, 2025 at 06:29:09PM +0000, David Laight wrote:  
> > > > On Thu, 20 Nov 2025 10:54:01 -0500
> > > > Yury Norov <yury.norov@gmail.com> wrote:
> > > >     
> > > > > On Wed, Nov 19, 2025 at 10:40:57PM +0000, david.laight.linux@gmail.com wrote:    
> > > > > > From: David Laight <david.laight.linux@gmail.com>
> > > > > > 
> > > > > > The return type of variable__ffs() is currently 'unsigned long'.
> > > > > > This makes the x86 __ffs() be 'unsigned long' whereas the generic
> > > > > > version is 'unsigned int'.
> > > > > > 
> > > > > > Similarly change variable_ffz() and ffz().
> > > > > > 
> > > > > > This may save some REX prefix on 64bit.
> > > > > > 
> > > > > > Detected by some extra checks added to min_t() to detect possible
> > > > > > truncation of large values.
> > > > > > 
> > > > > > Signed-off-by: David Laight <david.laight.linux@gmail.com>
> > > > > > ---
> > > > > >  arch/x86/include/asm/bitops.h | 18 +++++++-----------
> > > > > >  1 file changed, 7 insertions(+), 11 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/x86/include/asm/bitops.h b/arch/x86/include/asm/bitops.h
> > > > > > index c2ce213f2b9b..2e8a954d2e2d 100644
> > > > > > --- a/arch/x86/include/asm/bitops.h
> > > > > > +++ b/arch/x86/include/asm/bitops.h
> > > > > > @@ -240,7 +240,7 @@ arch_test_bit_acquire(unsigned long nr, const volatile unsigned long *addr)
> > > > > >  					  variable_test_bit(nr, addr);
> > > > > >  }
> > > > > >  
> > > > > > -static __always_inline __attribute_const__ unsigned long variable__ffs(unsigned long word)
> > > > > > +static __always_inline __attribute_const__ unsigned int variable__ffs(unsigned long word)      
> > > > > 
> > > > > There's a mismatch with the generic ffs() in asm-generic/bitops/ffs.h.
> > > > > 
> > > > > The generic_ffs() returns int. There is another variable__ffs() defined
> > > > > in arch/risk, also returning int.
> > > > > 
> > > > > So I believe, the correct fix would be to switch x86 to int as well.
> > > > > And anyways, I believe this deserves a separate series.    
> > > > 
> > > > It is a single patch, do you want me to resend the patch on its own?
> > > > (With a different commit message)    
> > > 
> > > The patch looks wrong to me. All the flavors of ffs(), ffz() and
> > > others should have the same signature.  
> > 
> > I only changed 'long' to 'int', all the generic (and builtin) ones are 'int'.
> > So I'm not sure which difference you are talking about.
> > IIRC the __attribute_const__ just lets the compiler do CSE.  
> 
> All the versions of ffs(), ffz() and others should in the better world
> have the same signatures. This is unfortunately not what we have now.
> 
> for __ffs() we've got signatures:
> 
> RISCV:
> static __always_inline __attribute_const__ unsigned long variable__ffs(unsigned long word)
> 
> x86:
> static __always_inline __attribute_const__ unsigned long variable__ffs(unsigned long word)
> 
> Generic:
> static __always_inline __attribute_const__ unsigned int generic___ffs(unsigned long word)
> 
> You're changing x86 implementation to match the generic one, but don't
> change RISCV, and this is wrong.

More like 'incomplete'.
It looks like chunks of the RISCV file are a straight C&P of the x86 one.

> Can you make them all matching, and check the same for ffz() please?
> This would require a small separate series, I guess.

I can do a patch for RISCV - but I'm not setup to compile it.
But it should be just changing the function return types.

> 
> Moving forward, it would be nice to make all that helpers looking more
> unified across different arches and flavors. Although, out of scope of
> your series.

I tripped up this one as part of my min_t() checks.
Basically if you do any maths with the result of the x86 'ffs' functions
the other operand (which is likely to be 32bits due to the domain of the
value) has to be promoted to 64bits.
This is likely to happen more often than the opposite.

	David

> 
> Thanks,
> Yury