linux-kernel - Re: [PATCH v4 03/10] PCI: Introduce pci_walk_bus_reverse(), for_each_pci_dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250807232636.GA68733@bhelgaas>
Date: Thu, 7 Aug 2025 18:26:36 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: dan.j.williams@...el.com
Cc: linux-coco@...ts.linux.dev, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org, bhelgaas@...gle.com, aik@....com,
	lukas@...ner.de
Subject: Re: [PATCH v4 03/10] PCI: Introduce pci_walk_bus_reverse(),
 for_each_pci_dev_reverse()

On Thu, Aug 07, 2025 at 04:17:54PM -0700, dan.j.williams@...el.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