Re: [RFC PATCH 2/2] treewide, bits: use ffs_val() where it is open-coded

[David Laight]

On Sat, 10 Jan 2026 10:36:07 +0000 (GMT)
"Maciej W. Rozycki" <macro@xxxxxxxxxxx> wrote:

> On Fri, 9 Jan 2026, Yury Norov wrote:
> 
> > > diff --git a/arch/alpha/kernel/smp.c b/arch/alpha/kernel/smp.c
> > > index ed06367ece574..2f3809c015b99 100644
> > > --- a/arch/alpha/kernel/smp.c
> > > +++ b/arch/alpha/kernel/smp.c
> > > @@ -525,7 +525,7 @@ handle_ipi(struct pt_regs *regs)
> > >  	  do {
> > >  		unsigned long which;
> > >  
> > > -		which = ops & -ops;
> > > +		which = ffs_val(ops);
> > >  		ops &= ~which;
> > >  		which = __ffs(which);  
> > 
> > I guess, this should be:
> > 
> >         which = __ffs(ops);
> >         ops &= ops - 1;  
> 
>  I think it's the wrong operation order.

If you are going to do the __ffs() do it first.
		which = __ffs(ops);
		ops &= ~(1u << which);

OTOH what follows is:
		switch (which) {
		case IPI_RESCHEDULE:
so changing to 'case 1u << IPI_RESCHEDULE: would save the ffs().
But the entire loop is pointless, three tests, the first being:
	if (op & (1u << IPI_RESCHEDULE))
		scheduler_ipi();
would be far less code.

But probably pointless churn - no one cares about alpha any more :-)

> 
> > > diff --git a/arch/mips/dec/ecc-berr.c b/arch/mips/dec/ecc-berr.c
> > > index 1eb356fdd8323..8934b8b1cf375 100644
> > > --- a/arch/mips/dec/ecc-berr.c
> > > +++ b/arch/mips/dec/ecc-berr.c
> > > @@ -153,7 +153,7 @@ static int dec_ecc_be_backend(struct pt_regs *regs, int is_fixup, int invoker)
> > >  			/* Ack now, now we've rewritten (or not). */
> > >  			dec_ecc_be_ack();
> > >  
> > > -			if (syn && syn == (syn & -syn)) {
> > > +			if (syn && syn == ffs_val(syn)) {  
> > 
> > It opencodes is_power_of_2().

Badly...

	David

> 
>  Correct and it also predates that helper's existence by ~4 years.  I'll 
> be happy to see this converted as the intent will be obviously clearer.
> 
>   Maciej
>