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