On Fri, Mar 06, 2009 at 01:08:10PM -0700, Matthew Wilcox wrote:
> On Fri, Feb 20, 2009 at 02:54:42PM +0800, Yu Zhao wrote:
> > + list_for_each_entry(pdev, &dev->bus->devices, bus_list)
> > + if (pdev->sriov)
> > + break;
> > + if (list_empty(&dev->bus->devices) || !pdev->sriov)
> > + pdev = NULL;
> > + ctrl = 0;
> > + if (!pdev && pci_ari_enabled(dev->bus))
> > + ctrl |= PCI_SRIOV_CTRL_ARI;
> > +
>
> I don't like this loop. At the end of a list_for_each_entry() loop,
> pdev will not be pointing at a pci_device, it'll be pointing to some
> offset from &dev->bus->devices. So checking pdev->sriov at this point
> is really, really bad. I would prefer to see something like this:
>
> ctrl = 0;
> list_for_each_entry(pdev, &dev->bus->devices, bus_list) {
> if (pdev->sriov)
> goto ari_enabled;
> }
>
> if (pci_ari_enabled(dev->bus))
> ctrl = PCI_SRIOV_CTRL_ARI;
> ari_enabled:
> pci_write_config_word(dev, pos + PCI_SRIOV_CTRL, ctrl);
No, please use bus_for_each_dev() instead, or bus_find_device(), don't
walk the bus list by hand. I'm kind of surprised that even builds. Hm,
in looking at the 2.6.29-rc kernels, I notice it will not even build at
all, you are now forced to use those functions, which is good.
Has anyone even tried to build this patch recently?
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
