[PATCH] PCI: Fix device node for virtual bus

[PATCH] PCI: Fix device node for virtual bus

[Gavin Shan]

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

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

Re: [PATCH] PCI: Fix device node for virtual bus

[Bjorn Helgaas]

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
>

Re: [PATCH] PCI: Fix device node for virtual bus

[Gavin Shan]

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

Re: [PATCH] PCI: Fix device node for virtual bus

[Bjorn Helgaas]

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

Re: [PATCH] PCI: Fix device node for virtual bus

[Gavin Shan]

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

Re: [PATCH] PCI: Fix device node for virtual bus

[Bjorn Helgaas]

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

Re: [PATCH] PCI: Fix device node for virtual bus

[Gavin Shan]

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

Re: [PATCH] PCI: Fix device node for virtual bus

[Bjorn Helgaas]

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
>