Re: [PATCH v4 03/10] PCI: Introduce pci_walk_bus_reverse(), for_each_pci_dev_reverse()

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Aug 07, 2025 at 04:17:54PM -0700, dan.j.williams@intel.com wrote:
> Bjorn Helgaas wrote:
> > On Thu, Jul 17, 2025 at 11:33:51AM -0700, Dan Williams wrote:

> > > +++ b/drivers/base/bus.c
> > > +static struct device *prev_device(struct klist_iter *i)
> > > +{
> > > +	struct klist_node *n = klist_prev(i);
> > > +	struct device *dev = NULL;
> > > +	struct device_private *dev_prv;
> > > +
> > > +	if (n) {
> > > +		dev_prv = to_device_private_bus(n);
> > > +		dev = dev_prv->device;
> > > +	}
> > > +	return dev;
> > 
> > I think this would be simpler as:
> > 
> >   if (!n)
> >     return NULL;
> > 
> >   dev_prv = to_device_private_bus(n);
> >   return dev_prv->device;
> 
> Agree, in isolation, but next to next_device() the style looks odd. So,
> go back and style-fix code from 2008, or make 2025 code look like 2008
> code is the choice.

Good point, I didn't look around at that code.  Following the existing
style seems right to me.

> > > +++ b/drivers/pci/bus.c
> > > +static int __pci_walk_bus_reverse(struct pci_bus *top,
> > > +				  int (*cb)(struct pci_dev *, void *),
> > > +				  void *userdata)
> > > +{
> > > +	struct pci_dev *dev;
> > > +	int ret = 0;
> > > +
> > > +	list_for_each_entry_reverse(dev, &top->devices, bus_list) {
> > > +		if (dev->subordinate) {
> > > +			ret = __pci_walk_bus_reverse(dev->subordinate, cb,
> > > +						     userdata);
> > > +			if (ret)
> > > +				break;
> > > +		}
> > > +		ret = cb(dev, userdata);
> > > +		if (ret)
> > > +			break;
> > > +	}
> > > +	return ret;
> > 
> > Why not:
> > 
> >   list_for_each_entry_reverse(...) {
> >     ...
> >     if (ret)
> >       return ret;
> >   }
> >   return 0;
> 
> Again, for conformance to existing style of __pci_walk_bus(). Want a
> lead-in cleanup for that?

Don't bother.  Maybe some janitor will show up and do it eventually.

Bjorn