Re: [PATCH v6 1/4] PCI: Introduce pci_dev_for_each_resource()

[Bjorn Helgaas <helgaas@xxxxxxxxxx>]

On Thu, Mar 23, 2023 at 04:30:01PM +0200, Andy Shevchenko wrote:
> On Wed, Mar 22, 2023 at 02:28:04PM -0500, Bjorn Helgaas wrote:
> > On Mon, Mar 20, 2023 at 03:16:30PM +0200, Andy Shevchenko wrote:
> ...
> 
> > > + pci_dev_for_each_resource_p(dev, r) {
> > >           /* zap the 2nd function of the winbond chip */
> > > -         if (dev->resource[i].flags & IORESOURCE_IO
> > > -             && dev->bus->number == 0 && dev->devfn == 0x81)
> > > -                 dev->resource[i].flags &= ~IORESOURCE_IO;
> > > -         if (dev->resource[i].start == 0 && dev->resource[i].end) {
> > > -                 dev->resource[i].flags = 0;
> > > -                 dev->resource[i].end = 0;
> > > +         if (dev->bus->number == 0 && dev->devfn == 0x81 &&
> > > +             r->flags & IORESOURCE_IO)
> > 
> > This is a nice literal conversion, but it's kind of lame to test
> > bus->number and devfn *inside* the loop here, since they can't change
> > inside the loop.
> 
> Hmm... why are you asking me, even if I may agree on that? It's
> in the original code and out of scope of this series.

Yeah, I don't think it would be *unreasonable* to clean this up at the
same time so the maintainers can look at both at the same time (this
is arch/powerpc/platforms/pseries/pci.c, so Michael, et al), but no
need for you to do anything, certainly.  I can post a follow-up patch.

> > but
> > since we're converging on the "(dev, res)" style, I think we should
> > reverse the names so we have something like:
> > 
> >   pci_dev_for_each_resource(dev, res)
> >   pci_dev_for_each_resource_idx(dev, res, i)
> 
> Wouldn't it be more churn, including pci_bus_for_each_resource() correction?

Yes, it definitely is a little more churn because we already have
pci_bus_for_each_resource() that would have to be changed.

I poked around looking for similar patterns elsewhere with:

  git grep "#define.*for_each_.*_p("
  git grep "#define.*for_each_.*_idx("

I didn't find any other "_p" iterators and just a few "_idx" ones, so
my hope is to follow what little precedent there is, as well as
converge on the basic "*_for_each_resource()" iterators and remove the
"_idx()" versions over time by doing things like the
pci_claim_resource() change.

What do you think?  If it seems like excessive churn, we can do it
as-is and still try to reduce the use of the index variable over time.

Bjorn