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

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

[Bjorn Helgaas]

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;
-
-	pci_enable_bridge(dev->bus->self);
+	if (!pci_is_root_bus(dev->bus))
+		pci_enable_bridge(dev->bus->self);
 
 	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(dev->bus->self);
 
 	/* only skip sriov related */
 	for (i = 0; i <= PCI_ROM_RESOURCE; i++)

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

[Yinghai Lu]

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.

> -       pci_enable_bridge(dev->bus->self);
> +       if (!pci_is_root_bus(dev->bus))
> +               pci_enable_bridge(dev->bus->self);
>
>         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(dev->bus->self);
>
>         /* only skip sriov related */
>         for (i = 0; i <= PCI_ROM_RESOURCE; i++)
>

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

[Bjorn Helgaas]

[+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?

> > -       pci_enable_bridge(dev->bus->self);
> > +       if (!pci_is_root_bus(dev->bus))
> > +               pci_enable_bridge(dev->bus->self);
> >
> >         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(dev->bus->self);
> >
> >         /* only skip sriov related */
> >         for (i = 0; i <= PCI_ROM_RESOURCE; i++)
> >


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;
+
+	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++)

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

[Yinghai Lu]

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.

> +
> +       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.

Maybe you can cache return from pci_upstream_bridge and check that before
calling pci_enable_bridge.

Thanks

Yinghai

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

[Bjorn Helgaas]

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

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

[Yinghai Lu]

>> > 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.

how about it is VF and it is on it's own virtual bus ?
and it will pass !pci_is_root_bus(dev->bus) checking and get into
pci_enable_bridge(pci_upstream_bridge(dev));


Thanks

Yinghai

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

[Bjorn Helgaas]

On Wed, Nov 06, 2013 at 12:33:31PM -0800, Yinghai Lu wrote:
> >> > 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.
> 
> how about it is VF and it is on it's own virtual bus ?
> and it will pass !pci_is_root_bus(dev->bus) checking and get into
> pci_enable_bridge(pci_upstream_bridge(dev));

Oh, I see.  The problem is when VF is on a virtual bus and the
corresponding PF is on a root bus.  Then we have:

  pci_is_root_bus(VF) == false
  pci_upstream_bridge(VF) == pci_upstream_bridge(PF)
  pci_uptream_bridge(PF) == NULL

I guess that's what your suggestion about "caching the return from
pci_upstream_bridge()" was about.  Like this:


commit 21ea57bf1311f7a8d1b755d355322a1f077fccb7
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.  This is typically just "dev->bus->self", but in
    the case of a VF on a virtual bus, we have to start from the corresponding
    PF.  Returns NULL if there is no upstream PCI bridge, i.e., if the device
    is on a root bus.
    
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

diff --git a/include/linux/pci.h b/include/linux/pci.h
index d3a888a..835ec7b 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -480,6 +480,15 @@ 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)
+{
+	dev = pci_physfn(dev);
+	if (pci_is_root_bus(dev->bus))
+		return NULL;
+
+	return dev->bus->self;
+}
+
 #ifdef CONFIG_PCI_MSI
 static inline bool pci_dev_msi_enabled(struct pci_dev *pci_dev)
 {
commit bca690ab580d5c0c4a0c7201f3c42057288add8b
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..d3ed931 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1148,12 +1148,12 @@ int pci_reenable_device(struct pci_dev *dev)
 
 static void pci_enable_bridge(struct pci_dev *dev)
 {
+	struct pci_dev *bridge;
 	int retval;
 
-	if (!dev)
-		return;
-
-	pci_enable_bridge(dev->bus->self);
+	bridge = pci_upstream_bridge(dev);
+	if (bridge)
+		pci_enable_bridge(bridge);
 
 	if (pci_is_enabled(dev)) {
 		if (!dev->is_busmaster)
@@ -1170,6 +1170,7 @@ static void pci_enable_bridge(struct pci_dev *dev)
 
 static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
 {
+	struct pci_dev *bridge;
 	int err;
 	int i, bars = 0;
 
@@ -1188,7 +1189,9 @@ 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);
+	bridge = pci_upstream_bridge(dev);
+	if (bridge)
+		pci_enable_bridge(bridge);
 
 	/* only skip sriov related */
 	for (i = 0; i <= PCI_ROM_RESOURCE; i++)

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

[Yinghai Lu]

On Wed, Nov 6, 2013 at 12:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:

> Oh, I see.  The problem is when VF is on a virtual bus and the
> corresponding PF is on a root bus.  Then we have:
>
>   pci_is_root_bus(VF) == false
>   pci_upstream_bridge(VF) == pci_upstream_bridge(PF)
>   pci_uptream_bridge(PF) == NULL
>
> I guess that's what your suggestion about "caching the return from
> pci_upstream_bridge()" was about.  Like this:

Yes.

For the two patches,

Acked-by: Yinghai Lu <yinghai@kernel.org>

>
>
> commit 21ea57bf1311f7a8d1b755d355322a1f077fccb7
> 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.  This is typically just "dev->bus->self", but in
>     the case of a VF on a virtual bus, we have to start from the corresponding
>     PF.  Returns NULL if there is no upstream PCI bridge, i.e., if the device
>     is on a root bus.
>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d3a888a..835ec7b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -480,6 +480,15 @@ 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)
> +{
> +       dev = pci_physfn(dev);
> +       if (pci_is_root_bus(dev->bus))
> +               return NULL;
> +
> +       return dev->bus->self;
> +}
> +
>  #ifdef CONFIG_PCI_MSI
>  static inline bool pci_dev_msi_enabled(struct pci_dev *pci_dev)
>  {
> commit bca690ab580d5c0c4a0c7201f3c42057288add8b
> 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..d3ed931 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1148,12 +1148,12 @@ int pci_reenable_device(struct pci_dev *dev)
>
>  static void pci_enable_bridge(struct pci_dev *dev)
>  {
> +       struct pci_dev *bridge;
>         int retval;
>
> -       if (!dev)
> -               return;
> -
> -       pci_enable_bridge(dev->bus->self);
> +       bridge = pci_upstream_bridge(dev);
> +       if (bridge)
> +               pci_enable_bridge(bridge);
>
>         if (pci_is_enabled(dev)) {
>                 if (!dev->is_busmaster)
> @@ -1170,6 +1170,7 @@ static void pci_enable_bridge(struct pci_dev *dev)
>
>  static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
>  {
> +       struct pci_dev *bridge;
>         int err;
>         int i, bars = 0;
>
> @@ -1188,7 +1189,9 @@ 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);
> +       bridge = pci_upstream_bridge(dev);
> +       if (bridge)
> +               pci_enable_bridge(bridge);
>
>         /* only skip sriov related */
>         for (i = 0; i <= PCI_ROM_RESOURCE; i++)

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

[Bjorn Helgaas]

On Wed, Nov 06, 2013 at 04:28:48PM -0800, Yinghai Lu wrote:
> On Wed, Nov 6, 2013 at 12:46 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> 
> > Oh, I see.  The problem is when VF is on a virtual bus and the
> > corresponding PF is on a root bus.  Then we have:
> >
> >   pci_is_root_bus(VF) == false
> >   pci_upstream_bridge(VF) == pci_upstream_bridge(PF)
> >   pci_uptream_bridge(PF) == NULL
> >
> > I guess that's what your suggestion about "caching the return from
> > pci_upstream_bridge()" was about.  Like this:
> 
> Yes.
> 
> For the two patches,
> 
> Acked-by: Yinghai Lu <yinghai@kernel.org>

Thanks, I applied these two with your acks to my pci/misc branch for
v3.13.

Bjorn

> >
> > commit 21ea57bf1311f7a8d1b755d355322a1f077fccb7
> > 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.  This is typically just "dev->bus->self", but in
> >     the case of a VF on a virtual bus, we have to start from the corresponding
> >     PF.  Returns NULL if there is no upstream PCI bridge, i.e., if the device
> >     is on a root bus.
> >
> >     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> >
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index d3a888a..835ec7b 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -480,6 +480,15 @@ 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)
> > +{
> > +       dev = pci_physfn(dev);
> > +       if (pci_is_root_bus(dev->bus))
> > +               return NULL;
> > +
> > +       return dev->bus->self;
> > +}
> > +
> >  #ifdef CONFIG_PCI_MSI
> >  static inline bool pci_dev_msi_enabled(struct pci_dev *pci_dev)
> >  {
> > commit bca690ab580d5c0c4a0c7201f3c42057288add8b
> > 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..d3ed931 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1148,12 +1148,12 @@ int pci_reenable_device(struct pci_dev *dev)
> >
> >  static void pci_enable_bridge(struct pci_dev *dev)
> >  {
> > +       struct pci_dev *bridge;
> >         int retval;
> >
> > -       if (!dev)
> > -               return;
> > -
> > -       pci_enable_bridge(dev->bus->self);
> > +       bridge = pci_upstream_bridge(dev);
> > +       if (bridge)
> > +               pci_enable_bridge(bridge);
> >
> >         if (pci_is_enabled(dev)) {
> >                 if (!dev->is_busmaster)
> > @@ -1170,6 +1170,7 @@ static void pci_enable_bridge(struct pci_dev *dev)
> >
> >  static int pci_enable_device_flags(struct pci_dev *dev, unsigned long flags)
> >  {
> > +       struct pci_dev *bridge;
> >         int err;
> >         int i, bars = 0;
> >
> > @@ -1188,7 +1189,9 @@ 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);
> > +       bridge = pci_upstream_bridge(dev);
> > +       if (bridge)
> > +               pci_enable_bridge(bridge);
> >
> >         /* only skip sriov related */
> >         for (i = 0; i <= PCI_ROM_RESOURCE; i++)

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

[Wei Yang]

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++)


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.

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

[Bjorn Helgaas]

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

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

[Wei Yang]

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

-- 
Richard Yang
Help you, Help me