| Message ID | 1378431958-7874-1-git-send-email-weiyang@linux.vnet.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
[+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
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 >>
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?
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
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
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?
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
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. >
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) {
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(-)