Re: [PATCH v1 1/2] PCI/AER: Use for_each_set_bit()

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Sep 27, 2019 at 07:39:13AM -0500, Bjorn Helgaas wrote:
> On Tue, Aug 27, 2019 at 06:18:22PM +0300, Andy Shevchenko wrote:
> > This simplifies and standardizes slot manipulation code
> > by using for_each_set_bit() library function.

> > +	unsigned long status = info->status & ~info->mask;
> > +	int i, max = -1;

> > -	for (i = 0; i < max; i++)
> > -		if (status & (1 << i))
> > -			counter[i]++;
> > +	for_each_set_bit(i, &status, max)
> 
> I applied this,

Thank you!

> but I confess to being a little ambivalent.  It's
> arguably a little easier to read,

I have another opinion here. Instead of parsing body of for-loop, the name of
the function tells you exactly what it's done. Besides the fact that reading
and parsing two lines, with zero conditionals, is faster.

> but it's not nearly as efficient
> (not a great concern here)

David, do you know why for_each_set_bit() has no optimization for the cases
when nbits <= BITS_PER_LONG? (Actually find_*bit() family of functions)

> and more importantly much harder to verify
> that it's correct because you have to chase through
> for_each_set_bit(), find_first_bit(), _ffs(), etc, etc.

If for_each_set_bit() or any other fundamental bit operation helper is broken,
PCI subsystem is a little concern here.

> No doubt it's
> great for bitmaps of arbitrary size, but for a simple 32-bit register
> I'm a little hesitant.  But I applied it anyway.
> 
> > +		counter[i]++;

-- 
With Best Regards,
Andy Shevchenko