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@xxxxxxxxxx> 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@xxxxxxxxxx>
>> > ---
>> > 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++)
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.
--
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
