Re: [PATCH] PCI: Use pci_is_root_bus() to check for root bus

[Bjorn Helgaas <bhelgaas@google.com>]

On Wed, Nov 06, 2013 at 11:56:13AM -0800, Yinghai Lu wrote:
> On Wed, Nov 6, 2013 at 10:15 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> 
> > commit dfb66fee4715c747a94abd45c20fbe302b10e49c
> > Author: Bjorn Helgaas <bhelgaas@google.com>
> > Date:   Wed Nov 6 10:11:48 2013 -0700
> >
> >     PCI: Add pci_upstream_bridge()
> >
> >     This adds a pci_upstream_bridge() interface to find the PCI-to-PCI bridge
> >     upstream from a device.  If the device is on a root bus, i.e., the upstream
> >     bridge is a host bridge instead of a PCI bridge, this returns NULL.  If the
> >     device is a VF, this returns the bridge upstream from the PF corresponding
> >     to the VF.  This is important for VFs on virtual buses, where
> >     "dev->bus->self == NULL".
> >
> >     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index d3a888a..e09d19a 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -480,6 +480,18 @@ static inline bool pci_is_root_bus(struct pci_bus *pbus)
> >         return !(pbus->parent);
> >  }
> >
> > +static inline struct pci_dev *pci_upstream_bridge(struct pci_dev *dev)
> > +{
> > +       if (pci_is_root_bus(dev->bus))
> > +               return NULL;
> > +
> > +       dev = pci_physfn(dev);
> > +       if (pci_is_root_bus(dev->bus))
> > +               return NULL;
> 
> Maybe we can drop the first pci_is_root_bus() checking.
> for physfn, pci_physfn will return it's self.

Yep, that makes sense, thanks.  I incorporated that change.

> > +
> > +       return dev->bus->self;
> > +}
> > +
> >  #ifdef CONFIG_PCI_MSI
> >  static inline bool pci_dev_msi_enabled(struct pci_dev *pci_dev)
> >  {
> > commit 4e5415f02e32c85e902cd9692eab18200e14b347
> > Author: Bjorn Helgaas <bhelgaas@google.com>
> > Date:   Wed Nov 6 10:00:51 2013 -0700
> >
> >     PCI: Enable upstream bridges even for VFs on virtual buses
> >
> >     Previously we enabled the upstream PCI-to-PCI bridge only when
> >     "dev->bus->self != NULL".  In the case of a VF on a virtual bus, where
> >     "bus->self == NULL", we didn't enable the upstream bridge.
> >
> >     This fixes that by enabling the upstream bridge of the PF corresponding to
> >     the VF.
> >
> >     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index ac40f90..744dc26 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1150,10 +1150,8 @@ static void pci_enable_bridge(struct pci_dev *dev)
> >  {
> >         int retval;
> >
> > -       if (!dev)
> > -               return;
> > -
> > -       pci_enable_bridge(dev->bus->self);
> > +       if (!pci_is_root_bus(dev->bus))
> > +               pci_enable_bridge(pci_upstream_bridge(dev));
> >
> >         if (pci_is_enabled(dev)) {
> >                 if (!dev->is_busmaster)
> > @@ -1188,7 +1186,8 @@ static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
> >         if (atomic_inc_return(&dev->enable_cnt) > 1)
> >                 return 0;               /* already enabled */
> >
> > -       pci_enable_bridge(dev->bus->self);
> > +       if (!pci_is_root_bus(dev->bus))
> > +               pci_enable_bridge(pci_upstream_bridge(dev));
> >
> >         /* only skip sriov related */
> >         for (i = 0; i <= PCI_ROM_RESOURCE; i++)
> 
> still have problem.
> 
> pci_upstream_bridge() could return NULL. so later pci_enable_bridge()
> referring dev->bus could panic, as you remove !dev checking.

pci_upstream_bridge(dev) can only return NULL if dev is on a root bus,
and we never call pci_enable_bridge() in that case.  So I don't see
the problem.

Bjorn