| Message ID | 1415760163-12517-1-git-send-email-gwshan@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Tue, Nov 11, 2014 at 7:42 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: > pci_set_bus_of_node() sets virtual PCI bus's device node to PHB's > device node wrongly. The patch fixes the issue. This needs more detail. How is this bug visible to users? Is there a bug report? Is this a regression? Should it be marked for stable? We use "virtual bus" to refer to buses created for SR-IOV VFs that are not on the same bus as the PF. These virtual buses have no bridge device leading to them. But I think you're talking about something totally different. Or maybe that *is* what you're talking about, since this patch resembles 2ba29e270e97 ("PCI: Use pci_is_root_bus() to check for root bus"). Bjorn > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > --- > drivers/pci/of.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > index f092993..7b2256b 100644 > --- a/drivers/pci/of.c > +++ b/drivers/pci/of.c > @@ -31,9 +31,9 @@ void pci_release_of_node(struct pci_dev *dev) > > void pci_set_bus_of_node(struct pci_bus *bus) > { > - if (bus->self == NULL) > + if (pci_is_root_bus(bus)) > bus->dev.of_node = pcibios_get_phb_of_node(bus); > - else > + else if (bus->self) > bus->dev.of_node = of_node_get(bus->self->dev.of_node); > } > > -- > 1.8.3.2 > -- 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, Nov 11, 2014 at 07:55:19PM -0700, Bjorn Helgaas wrote: >On Tue, Nov 11, 2014 at 7:42 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: >> pci_set_bus_of_node() sets virtual PCI bus's device node to PHB's >> device node wrongly. The patch fixes the issue. > >This needs more detail. How is this bug visible to users? Is there a >bug report? Is this a regression? Should it be marked for stable? > I don't have opened bug for it. I just found the problem (maybe not a problem) when reading the code: The original implementation binds PHB device-tree node with "virtual bus", which doesn't make sense. Yes, I guess it should be marked for stable if you don't object. >We use "virtual bus" to refer to buses created for SR-IOV VFs that are >not on the same bus as the PF. These virtual buses have no bridge >device leading to them. But I think you're talking about something >totally different. Or maybe that *is* what you're talking about, >since this patch resembles 2ba29e270e97 ("PCI: Use pci_is_root_bus() >to check for root bus"). > "virtual bus" here is the buses created for SR-IOV VFs. Yes, the code changes resembles commit 2ba29e270e97. Thanks, Gavin >Bjorn > >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >> --- >> drivers/pci/of.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pci/of.c b/drivers/pci/of.c >> index f092993..7b2256b 100644 >> --- a/drivers/pci/of.c >> +++ b/drivers/pci/of.c >> @@ -31,9 +31,9 @@ void pci_release_of_node(struct pci_dev *dev) >> >> void pci_set_bus_of_node(struct pci_bus *bus) >> { >> - if (bus->self == NULL) >> + if (pci_is_root_bus(bus)) >> bus->dev.of_node = pcibios_get_phb_of_node(bus); >> - else >> + else if (bus->self) >> bus->dev.of_node = of_node_get(bus->self->dev.of_node); >> } >> >> -- >> 1.8.3.2 >> > -- 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, Nov 11, 2014 at 8:11 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: > On Tue, Nov 11, 2014 at 07:55:19PM -0700, Bjorn Helgaas wrote: >>On Tue, Nov 11, 2014 at 7:42 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: >>> pci_set_bus_of_node() sets virtual PCI bus's device node to PHB's >>> device node wrongly. The patch fixes the issue. >> >>This needs more detail. How is this bug visible to users? Is there a >>bug report? Is this a regression? Should it be marked for stable? >> > > I don't have opened bug for it. I just found the problem (maybe not > a problem) when reading the code: The original implementation binds > PHB device-tree node with "virtual bus", which doesn't make sense. Does this result in something being wrong in sysfs? How is this bug visible to users? > Yes, I guess it should be marked for stable if you don't object. > >>We use "virtual bus" to refer to buses created for SR-IOV VFs that are >>not on the same bus as the PF. These virtual buses have no bridge >>device leading to them. But I think you're talking about something >>totally different. Or maybe that *is* what you're talking about, >>since this patch resembles 2ba29e270e97 ("PCI: Use pci_is_root_bus() >>to check for root bus"). >> > > "virtual bus" here is the buses created for SR-IOV VFs. Yes, the > code changes resembles commit 2ba29e270e97. > > Thanks, > Gavin > >>Bjorn >> >>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >>> --- >>> drivers/pci/of.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c >>> index f092993..7b2256b 100644 >>> --- a/drivers/pci/of.c >>> +++ b/drivers/pci/of.c >>> @@ -31,9 +31,9 @@ void pci_release_of_node(struct pci_dev *dev) >>> >>> void pci_set_bus_of_node(struct pci_bus *bus) >>> { >>> - if (bus->self == NULL) >>> + if (pci_is_root_bus(bus)) >>> bus->dev.of_node = pcibios_get_phb_of_node(bus); >>> - else >>> + else if (bus->self) >>> bus->dev.of_node = of_node_get(bus->self->dev.of_node); >>> } >>> >>> -- >>> 1.8.3.2 >>> >> > -- 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, Nov 11, 2014 at 09:08:23PM -0700, Bjorn Helgaas wrote: >On Tue, Nov 11, 2014 at 8:11 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: >> On Tue, Nov 11, 2014 at 07:55:19PM -0700, Bjorn Helgaas wrote: >>>On Tue, Nov 11, 2014 at 7:42 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: >>>> pci_set_bus_of_node() sets virtual PCI bus's device node to PHB's >>>> device node wrongly. The patch fixes the issue. >>> >>>This needs more detail. How is this bug visible to users? Is there a >>>bug report? Is this a regression? Should it be marked for stable? >>> >> >> I don't have opened bug for it. I just found the problem (maybe not >> a problem) when reading the code: The original implementation binds >> PHB device-tree node with "virtual bus", which doesn't make sense. > >Does this result in something being wrong in sysfs? How is this bug >visible to users? > No, I don't think it caused anything wrong in sysfs. So I'm not sure it's a real problem and need the fix. Please judge. Thanks, Gavin >> Yes, I guess it should be marked for stable if you don't object. >> >>>We use "virtual bus" to refer to buses created for SR-IOV VFs that are >>>not on the same bus as the PF. These virtual buses have no bridge >>>device leading to them. But I think you're talking about something >>>totally different. Or maybe that *is* what you're talking about, >>>since this patch resembles 2ba29e270e97 ("PCI: Use pci_is_root_bus() >>>to check for root bus"). >>> >> >> "virtual bus" here is the buses created for SR-IOV VFs. Yes, the >> code changes resembles commit 2ba29e270e97. >> >> Thanks, >> Gavin >> >>>Bjorn >>> >>>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >>>> --- >>>> drivers/pci/of.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/pci/of.c b/drivers/pci/of.c >>>> index f092993..7b2256b 100644 >>>> --- a/drivers/pci/of.c >>>> +++ b/drivers/pci/of.c >>>> @@ -31,9 +31,9 @@ void pci_release_of_node(struct pci_dev *dev) >>>> >>>> void pci_set_bus_of_node(struct pci_bus *bus) >>>> { >>>> - if (bus->self == NULL) >>>> + if (pci_is_root_bus(bus)) >>>> bus->dev.of_node = pcibios_get_phb_of_node(bus); >>>> - else >>>> + else if (bus->self) >>>> bus->dev.of_node = of_node_get(bus->self->dev.of_node); >>>> } >>>> >>>> -- >>>> 1.8.3.2 >>>> >>> >> > -- 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, Nov 11, 2014 at 10:24 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: > On Tue, Nov 11, 2014 at 09:08:23PM -0700, Bjorn Helgaas wrote: >>On Tue, Nov 11, 2014 at 8:11 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: >>> On Tue, Nov 11, 2014 at 07:55:19PM -0700, Bjorn Helgaas wrote: >>>>On Tue, Nov 11, 2014 at 7:42 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: >>>>> pci_set_bus_of_node() sets virtual PCI bus's device node to PHB's >>>>> device node wrongly. The patch fixes the issue. >>>> >>>>This needs more detail. How is this bug visible to users? Is there a >>>>bug report? Is this a regression? Should it be marked for stable? >>>> >>> >>> I don't have opened bug for it. I just found the problem (maybe not >>> a problem) when reading the code: The original implementation binds >>> PHB device-tree node with "virtual bus", which doesn't make sense. >> >>Does this result in something being wrong in sysfs? How is this bug >>visible to users? >> > > No, I don't think it caused anything wrong in sysfs. So I'm not sure > it's a real problem and need the fix. Please judge. Well, that's not really the way I work. If somebody proposes a patch, I expect the changelog to be an argument for why the patch is needed. I'm not disagreeing with your patch; I'm just trying to get you to provide the justification for it. I'm not an OF expert, so it's not obvious to me. I probably could spend some time researching it and convince myself, but I would rather have *you* convince me :) >>> Yes, I guess it should be marked for stable if you don't object. Once we know what the user-visible effect of this change is, it will probably be obvious whether it should be marked for stable. If it doesn't really fix anything, it probably should not go to stable. 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 Wed, Nov 12, 2014 at 12:02:08PM -0700, Bjorn Helgaas wrote: >On Tue, Nov 11, 2014 at 10:24 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: >> On Tue, Nov 11, 2014 at 09:08:23PM -0700, Bjorn Helgaas wrote: >>>On Tue, Nov 11, 2014 at 8:11 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: >>>> On Tue, Nov 11, 2014 at 07:55:19PM -0700, Bjorn Helgaas wrote: >>>>>On Tue, Nov 11, 2014 at 7:42 PM, Gavin Shan <gwshan@linux.vnet.ibm.com> wrote: >>>>>> pci_set_bus_of_node() sets virtual PCI bus's device node to PHB's >>>>>> device node wrongly. The patch fixes the issue. >>>>> >>>>>This needs more detail. How is this bug visible to users? Is there a >>>>>bug report? Is this a regression? Should it be marked for stable? >>>>> >>>> >>>> I don't have opened bug for it. I just found the problem (maybe not >>>> a problem) when reading the code: The original implementation binds >>>> PHB device-tree node with "virtual bus", which doesn't make sense. >>> >>>Does this result in something being wrong in sysfs? How is this bug >>>visible to users? >>> >> >> No, I don't think it caused anything wrong in sysfs. So I'm not sure >> it's a real problem and need the fix. Please judge. > >Well, that's not really the way I work. If somebody proposes a patch, >I expect the changelog to be an argument for why the patch is needed. >I'm not disagreeing with your patch; I'm just trying to get you to >provide the justification for it. I'm not an OF expert, so it's not >obvious to me. I probably could spend some time researching it and >convince myself, but I would rather have *you* convince me :) > Thank you for the detailed explaining. I'm trying to convince you. Please drop it in case I fail :) Basically, we have 3 types of buses with SR-IOV case inclusive: (A) root bus leading from PHB; (B) normal bus leading from PCI bridge; (C) virtual bus to hold SR-IOV VFs. A's device node is set to PHB's device node and B's device node is equal to parent PCI bridge's device node. So the device node of PCI bus is set that one of parent device (either PHB or PCI bridge for A/B). For (C), the bus's device node would be NULL, which is consistent with that its parent device is NULL. Kernel code can convert PCI bus to its corresponding device node with function pci_bus_to_OF_node(). The code is usually unware what type (PHB, PCI bridge or device) the device node is. With current code, we poke the PHB device node for SR-IOV virtual buses. and potentially wrong properties retrieved. I personally don't think it's appropriate and it's worthy to have NULL device nodes for those virtual buses. >>>> Yes, I guess it should be marked for stable if you don't object. > >Once we know what the user-visible effect of this change is, it will >probably be obvious whether it should be marked for stable. If it >doesn't really fix anything, it probably should not go to stable. > Yep, it's not good candidate for stable. Thanks, Gavin -- 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 Wed, Nov 12, 2014 at 01:42:43PM +1100, Gavin Shan wrote: > pci_set_bus_of_node() sets virtual PCI bus's device node to PHB's > device node wrongly. The patch fixes the issue. > > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> Applied to pci/misc for v3.19, thanks! Thanks for explaining this to me. It seems obvious now that I look at the code. > --- > drivers/pci/of.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/of.c b/drivers/pci/of.c > index f092993..7b2256b 100644 > --- a/drivers/pci/of.c > +++ b/drivers/pci/of.c > @@ -31,9 +31,9 @@ void pci_release_of_node(struct pci_dev *dev) > > void pci_set_bus_of_node(struct pci_bus *bus) > { > - if (bus->self == NULL) > + if (pci_is_root_bus(bus)) > bus->dev.of_node = pcibios_get_phb_of_node(bus); > - else > + else if (bus->self) > bus->dev.of_node = of_node_get(bus->self->dev.of_node); > } > > -- > 1.8.3.2 > -- 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
diff --git a/drivers/pci/of.c b/drivers/pci/of.c index f092993..7b2256b 100644 --- a/drivers/pci/of.c +++ b/drivers/pci/of.c @@ -31,9 +31,9 @@ void pci_release_of_node(struct pci_dev *dev) void pci_set_bus_of_node(struct pci_bus *bus) { - if (bus->self == NULL) + if (pci_is_root_bus(bus)) bus->dev.of_node = pcibios_get_phb_of_node(bus); - else + else if (bus->self) bus->dev.of_node = of_node_get(bus->self->dev.of_node); }
pci_set_bus_of_node() sets virtual PCI bus's device node to PHB's device node wrongly. The patch fixes the issue. Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> --- drivers/pci/of.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)