Patchwork [1/3] PCI: use pci_is_root_bus() to check whether it is a root bus

login
register
mail settings
Submitter Wei Yang
Date Sept. 6, 2013, 1:45 a.m.
Message ID <1378431958-7874-1-git-send-email-weiyang@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/273018/
State Accepted
Headers show

Comments

Wei Yang - Sept. 6, 2013, 1:45 a.m.
In __pci_bus_size_bridges() we check whether a pci bus is a root
bus by testing bus->self. As indicated by commit 79af72d7
("PCI: pci_is_root_bus helper"), bus->self == NULL is not a proper
way to check the pci root bus.

This patch changes it to pci_is_root_bus() to check whether it is
a root bus.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
---
 drivers/pci/setup-bus.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Bjorn Helgaas - Sept. 6, 2013, 11:09 p.m.
[+cc Kenji, Alex]

On Fri, Sep 06, 2013 at 09:45:56AM +0800, Wei Yang wrote:
> In __pci_bus_size_bridges() we check whether a pci bus is a root
> bus by testing bus->self. As indicated by commit 79af72d7
> ("PCI: pci_is_root_bus helper"), bus->self == NULL is not a proper
> way to check the pci root bus.
> 
> This patch changes it to pci_is_root_bus() to check whether it is
> a root bus.

I think this is a good change, even if only on the grounds of
consistency.

Did you trip over a case where a root bus has bus->self != NULL?
I'd like to know more details about the case where:

  (bus->parent == NULL) && (bus->self != NULL)

I'm sure that situation exists, or Kenji and Alex would not have
made the change in 79af72d7, but I don't know the details.

I'd like to know the details so I can recognize similar problems
elsewhere.

> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> ---
>  drivers/pci/setup-bus.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 520210f..989de3c 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -1134,7 +1134,7 @@ void __ref __pci_bus_size_bridges(struct pci_bus *bus,
>  	}
>  
>  	/* The root bus? */
> -	if (!bus->self)
> +	if (pci_is_root_bus(bus))
>  		return;
>  
>  	switch (bus->self->class >> 8) {
> -- 
> 1.7.1
> 
--
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
Wei Yang - Sept. 9, 2013, 2:14 a.m.
On Fri, Sep 06, 2013 at 05:09:41PM -0600, Bjorn Helgaas wrote:
>[+cc Kenji, Alex]
>
>On Fri, Sep 06, 2013 at 09:45:56AM +0800, Wei Yang wrote:
>> In __pci_bus_size_bridges() we check whether a pci bus is a root
>> bus by testing bus->self. As indicated by commit 79af72d7
>> ("PCI: pci_is_root_bus helper"), bus->self == NULL is not a proper
>> way to check the pci root bus.
>> 
>> This patch changes it to pci_is_root_bus() to check whether it is
>> a root bus.
>
>I think this is a good change, even if only on the grounds of
>consistency.
>
>Did you trip over a case where a root bus has bus->self != NULL?
>I'd like to know more details about the case where:
>
>  (bus->parent == NULL) && (bus->self != NULL)

Currently no. I believe the two platforms I have met, x86 and powerpc, don't
have this status.

>
>I'm sure that situation exists, or Kenji and Alex would not have
>made the change in 79af72d7, but I don't know the details.

Agree, willing to hear about some backgroups about the original patch.

>
>I'd like to know the details so I can recognize similar problems
>elsewhere.
>
>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>> ---
>>  drivers/pci/setup-bus.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
>> index 520210f..989de3c 100644
>> --- a/drivers/pci/setup-bus.c
>> +++ b/drivers/pci/setup-bus.c
>> @@ -1134,7 +1134,7 @@ void __ref __pci_bus_size_bridges(struct pci_bus *bus,
>>  	}
>>  
>>  	/* The root bus? */
>> -	if (!bus->self)
>> +	if (pci_is_root_bus(bus))
>>  		return;
>>  
>>  	switch (bus->self->class >> 8) {
>> -- 
>> 1.7.1
>>
Wei Yang - Sept. 9, 2013, 7 a.m.
On Fri, Sep 06, 2013 at 05:09:41PM -0600, Bjorn Helgaas wrote:
>[+cc Kenji, Alex]
>
>On Fri, Sep 06, 2013 at 09:45:56AM +0800, Wei Yang wrote:
>> In __pci_bus_size_bridges() we check whether a pci bus is a root
>> bus by testing bus->self. As indicated by commit 79af72d7
>> ("PCI: pci_is_root_bus helper"), bus->self == NULL is not a proper
>> way to check the pci root bus.
>> 
>> This patch changes it to pci_is_root_bus() to check whether it is
>> a root bus.
>
>I think this is a good change, even if only on the grounds of
>consistency.
>
>Did you trip over a case where a root bus has bus->self != NULL?
>I'd like to know more details about the case where:
>
>  (bus->parent == NULL) && (bus->self != NULL)

I found one case that (bus->self == NULL) and this bus is not root bus.

Not sure, this case will meet your requirement?
Alex Chiang - Sept. 9, 2013, 4:10 p.m.
Resending because the first mail got rejected by vger.

On Fri, Sep 6, 2013 at 4:09 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>
> [+cc Kenji, Alex]
>
> On Fri, Sep 06, 2013 at 09:45:56AM +0800, Wei Yang wrote:
> > In __pci_bus_size_bridges() we check whether a pci bus is a root
> > bus by testing bus->self. As indicated by commit 79af72d7
> > ("PCI: pci_is_root_bus helper"), bus->self == NULL is not a proper
> > way to check the pci root bus.
> >
> > This patch changes it to pci_is_root_bus() to check whether it is
> > a root bus.
>
> I think this is a good change, even if only on the grounds of
> consistency.
>
> Did you trip over a case where a root bus has bus->self != NULL?
> I'd like to know more details about the case where:
>
>   (bus->parent == NULL) && (bus->self != NULL)
>
> I'm sure that situation exists, or Kenji and Alex would not have
> made the change in 79af72d7, but I don't know the details.
>
> I'd like to know the details so I can recognize similar problems
> elsewhere.

It has been quite a long time, and so I had to google around in the
archives to try and remind myself why we made this change.
Unfortunately, I couldn't find anything public...

Reaching back into my unreliable human memory though, I truly seem to
recall that this interface was created out of paranoia/future-proofing
and not because we had tripped over any actual issues we had found in
the wild at that time.

Sorry for not being more helpful.

/ac
--
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
Bjorn Helgaas - Sept. 9, 2013, 4:15 p.m.
On Mon, Sep 9, 2013 at 1:00 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> On Fri, Sep 06, 2013 at 05:09:41PM -0600, Bjorn Helgaas wrote:
>>[+cc Kenji, Alex]
>>
>>On Fri, Sep 06, 2013 at 09:45:56AM +0800, Wei Yang wrote:
>>> In __pci_bus_size_bridges() we check whether a pci bus is a root
>>> bus by testing bus->self. As indicated by commit 79af72d7
>>> ("PCI: pci_is_root_bus helper"), bus->self == NULL is not a proper
>>> way to check the pci root bus.
>>>
>>> This patch changes it to pci_is_root_bus() to check whether it is
>>> a root bus.
>>
>>I think this is a good change, even if only on the grounds of
>>consistency.
>>
>>Did you trip over a case where a root bus has bus->self != NULL?
>>I'd like to know more details about the case where:
>>
>>  (bus->parent == NULL) && (bus->self != NULL)
>
> I found one case that (bus->self == NULL) and this bus is not root bus.
>
> Not sure, this case will meet your requirement?

I'm definitely interested in that case as well.  Can you include a
complete "lspci -vv" output for the case where this happens?

I suspect this happens with SR-IOV when we create "virtual" buses,
i.e., in virtfn_add_bus().  I think maybe I'll update the comment at
pci_is_root_bus() with a note about why we want to use it instead of
testing for "bus->self == NULL".

I think we still have other tree traversal issues related to those
virtual buses, e.g., the ones I mentioned here [1].  But those are a
separate problem.

Bjorn

[1] http://lkml.kernel.org/r/CAErSpo5sFfr=O-Pp=PyaxGauaEajaTr2aK-EQ_rTVUk1zyz8cA@mail.gmail.com
--
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
Wei Yang - Sept. 10, 2013, 7:46 a.m.
On Mon, Sep 09, 2013 at 10:15:07AM -0600, Bjorn Helgaas wrote:
>On Mon, Sep 9, 2013 at 1:00 AM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>> On Fri, Sep 06, 2013 at 05:09:41PM -0600, Bjorn Helgaas wrote:
>>>[+cc Kenji, Alex]
>>>
>>>Did you trip over a case where a root bus has bus->self != NULL?
>>>I'd like to know more details about the case where:
>>>
>>>  (bus->parent == NULL) && (bus->self != NULL)
>>
>> I found one case that (bus->self == NULL) and this bus is not root bus.
>>
>> Not sure, this case will meet your requirement?
>
>I'm definitely interested in that case as well.  Can you include a
>complete "lspci -vv" output for the case where this happens?

I don't *see* it. I notice this during the code reading.

So yes, this happens when a virtual bus is created for SR-IOV.

Sounds this is the *only* case for (bus->self == NULl)?

>I suspect this happens with SR-IOV when we create "virtual" buses,
>i.e., in virtfn_add_bus().  I think maybe I'll update the comment at
>pci_is_root_bus() with a note about why we want to use it instead of
>testing for "bus->self == NULL".
>
>I think we still have other tree traversal issues related to those
>virtual buses, e.g., the ones I mentioned here [1].  But those are a
>separate problem.
>
>Bjorn
>
>[1] http://lkml.kernel.org/r/CAErSpo5sFfr=O-Pp=PyaxGauaEajaTr2aK-EQ_rTVUk1zyz8cA@mail.gmail.com

Here comes another problem I met recently.

On some platforms, like powernv, each pci-dev has a dev-tree node to represent
it. The dev-tree node is set by pci_set_of_node() during pci_scan_device().

In pci_set_of_node(), it searchs for the node with the same devfn in its
parent. Here comes the question, who should be the VF's parent. Would be a
conflict for the devfn between PF and VF?
Benjamin Herrenschmidt - Sept. 10, 2013, 10:24 a.m.
On Tue, 2013-09-10 at 15:46 +0800, Wei Yang wrote:
> On some platforms, like powernv, each pci-dev has a dev-tree node to represent
> it. The dev-tree node is set by pci_set_of_node() during pci_scan_device().
> 
> In pci_set_of_node(), it searchs for the node with the same devfn in its
> parent. Here comes the question, who should be the VF's parent. Would be a
> conflict for the devfn between PF and VF?

I think it makes no sense to create OFW nodes for VFs under powernv anyway ...

(which is another reason why we need urgently to disconnect the EEH data
from the device node ...)

Cheers,
Ben.


--
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
Wei Yang - Sept. 11, 2013, 12:45 a.m.
On Tue, Sep 10, 2013 at 08:24:56PM +1000, Benjamin Herrenschmidt wrote:
>On Tue, 2013-09-10 at 15:46 +0800, Wei Yang wrote:
>> On some platforms, like powernv, each pci-dev has a dev-tree node to represent
>> it. The dev-tree node is set by pci_set_of_node() during pci_scan_device().
>> 
>> In pci_set_of_node(), it searchs for the node with the same devfn in its
>> parent. Here comes the question, who should be the VF's parent. Would be a
>> conflict for the devfn between PF and VF?
>
>I think it makes no sense to create OFW nodes for VFs under powernv anyway ...
>
>(which is another reason why we need urgently to disconnect the EEH data
>from the device node ...)

Ok, got it.

>
>Cheers,
>Ben.
>

Patch

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 520210f..989de3c 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1134,7 +1134,7 @@  void __ref __pci_bus_size_bridges(struct pci_bus *bus,
 	}
 
 	/* The root bus? */
-	if (!bus->self)
+	if (pci_is_root_bus(bus))
 		return;
 
 	switch (bus->self->class >> 8) {