| Message ID | 20131107030054.GA11245@weiyang.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Thu, Nov 07, 2013 at 11:00:54AM +0800, Wei Yang wrote: > On Wed, Nov 06, 2013 at 11:15:58AM -0700, Bjorn Helgaas wrote: > >[+cc Nishank] > > > >On Tue, Nov 05, 2013 at 07:39:10PM -0800, Yinghai Lu wrote: > >> On Tue, Nov 5, 2013 at 3:29 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > >> > pci_enable_device_flags() and pci_enable_bridge() assume that > >> > "bus->self == NULL" means that "bus" is a root bus. That assumption is > >> > false; see 2ba29e270e97 ("PCI: Use pci_is_root_bus() to check for root > >> > bus") for details. > >> > > >> > This patch changes them to use pci_is_root_bus() instead. > >> > > >> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > >> > --- > >> > drivers/pci/pci.c | 9 ++++----- > >> > 1 file changed, 4 insertions(+), 5 deletions(-) > >> > > >> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > >> > index ac40f90..de65bf7 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; > >> > - > >> > >> May need to keep this checking. > >> > >> virtual bus (for sriov virtual function) could have bus->self == NULL. > > > >Ah, you're right, thanks! When "dev" is a VF, "dev->bus->self" may be > >NULL. If we call pci_enable_device() on a VF, "dev->bus" is not a root > >bus, so we'll call pci_enable_bridge(dev->bus->self) when > >"dev->bus->self" is NULL, so we'll dereference a NULL pointer. > > > >But currently we just return when "dev == NULL", and I think that masks > >a deeper problem: what if we enable IOV but never call > >pci_enable_device(PF)? In that case, the upstream bridge may not be > >enabled, and when we call pci_enable_device(VF), we'll do nothing, so > >the upstream bridge remains disabled. > > > >I didn't see anywhere the core requires the PF to be enabled before IOV > >is enabled. I checked all the current callers of pci_enable_sriov(), > >and they all call pci_enable_device(PF) first. But I don't think > >anything *prevents* a driver from calling pci_enable_sriov(PF) without > >doing pci_enable_device(PF). > > > >Or the PCI core could enable VFs even with no driver attached, e.g., if > >we called pci_enable_sriov() from sriov_numvfs_store() (for the sysfs > >"sriov_numvfs" file). We talked about that a bit at the PCI miniconf in > >New Orleans. IIRC, there are some Cisco boxes where the firmware > >enables IOV, so the VFs are enabled before Linux even sees the PF, and a > >driver could bind to a VF and do pci_enable_device(VF) even if there's > >no PF driver at all. If that VF is on a virtual bus, we won't enable > >the upstream bridge, and the VF may not work. > > > >What do you think of the patches below? > > > > Thanks Bjorn, this is really a potential problme. And your patches fix this > problem. > > While I did a small change on the seconde one like this. Hope you like it :-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index bdd64b1..8d0ce48 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -1153,7 +1153,7 @@ static void pci_enable_bridge(struct pci_dev *dev) > if (!dev) > return; > > - pci_enable_bridge(dev->bus->self); > + pci_enable_bridge(pci_upstream_bridge(dev)); > > if (pci_is_enabled(dev)) { > if (!dev->is_busmaster) { > @@ -1190,7 +1190,7 @@ 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); > + pci_enable_bridge(pci_upstream_bridge(dev)); > > /* only skip sriov related */ > for (i = 0; i <= PCI_ROM_RESOURCE; i++) Thanks for looking at these. I think the latest version (the ones acked by Yinghai) do basically what you're suggesting. > BTW, pci_enable_bridge() is only called in pci_enable_device_flags(). After > change in these two patches, we pass a 'upstream bridge' to > pci_enable_bridge(). I am not sure whether this 'upstream bridge' could be a > VF? I took a look at the SPEC again, but not find clear clause. > > In case the 'upstream bridge' is always a PF, maybe we could simplize the > logic in pci_enable_bridge(). While current logic is reasonable and clear. I doubt it's possible for a VF to be a bridge, but I don't think there's really any reason to build that assumption into the code here. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Nov 07, 2013 at 02:59:18PM -0700, Bjorn Helgaas wrote: >On Thu, Nov 07, 2013 at 11:00:54AM +0800, Wei Yang wrote: >> Thanks Bjorn, this is really a potential problme. And your patches fix this >> problem. >> >> While I did a small change on the seconde one like this. Hope you like it :-) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index bdd64b1..8d0ce48 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -1153,7 +1153,7 @@ static void pci_enable_bridge(struct pci_dev *dev) >> if (!dev) >> return; >> >> - pci_enable_bridge(dev->bus->self); >> + pci_enable_bridge(pci_upstream_bridge(dev)); >> >> if (pci_is_enabled(dev)) { >> if (!dev->is_busmaster) { >> @@ -1190,7 +1190,7 @@ 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); >> + pci_enable_bridge(pci_upstream_bridge(dev)); >> >> /* only skip sriov related */ >> for (i = 0; i <= PCI_ROM_RESOURCE; i++) > >Thanks for looking at these. I think the latest version (the ones >acked by Yinghai) do basically what you're suggesting. Agree :-) > >> BTW, pci_enable_bridge() is only called in pci_enable_device_flags(). After >> change in these two patches, we pass a 'upstream bridge' to >> pci_enable_bridge(). I am not sure whether this 'upstream bridge' could be a >> VF? I took a look at the SPEC again, but not find clear clause. >> >> In case the 'upstream bridge' is always a PF, maybe we could simplize the >> logic in pci_enable_bridge(). While current logic is reasonable and clear. > >I doubt it's possible for a VF to be a bridge, but I don't think >there's really any reason to build that assumption into the code >here. Yep, the latest version is more general. > >Bjorn
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index bdd64b1..8d0ce48 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1153,7 +1153,7 @@ static void pci_enable_bridge(struct pci_dev *dev) if (!dev) return; - pci_enable_bridge(dev->bus->self); + pci_enable_bridge(pci_upstream_bridge(dev)); if (pci_is_enabled(dev)) { if (!dev->is_busmaster) { @@ -1190,7 +1190,7 @@ 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); + pci_enable_bridge(pci_upstream_bridge(dev)); /* only skip sriov related */ for (i = 0; i <= PCI_ROM_RESOURCE; i++)