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

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

[Wei Yang]

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

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

[PATCH 2/3] PCI: Use spec name for the comment of PCIe capability field

[Wei Yang]

According to the PCIe specification, bit 7:4 of PCI Express Capabilities
Register in a PCI Express Capability Structure is used to identify the
Device/Port type. If this field equals to 0x7, this PCIe device is a PCI
Express to PCI/PCI-X Bridge. While the comment of this value in the code
does not comply with the specification.

This patch changes the comment to "PCIE to PCI/PCI-X Bridge" instead of
"PCI/PCI-X Bridge", which comply with the specification.

Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
---
 include/uapi/linux/pci_regs.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index c3cc01d..b82b2ff 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -421,7 +421,7 @@
 #define  PCI_EXP_TYPE_ROOT_PORT 0x4	/* Root Port */
 #define  PCI_EXP_TYPE_UPSTREAM	0x5	/* Upstream Port */
 #define  PCI_EXP_TYPE_DOWNSTREAM 0x6	/* Downstream Port */
-#define  PCI_EXP_TYPE_PCI_BRIDGE 0x7	/* PCI/PCI-X Bridge */
+#define  PCI_EXP_TYPE_PCI_BRIDGE 0x7	/* PCIE to PCI/PCI-X Bridge */
 #define  PCI_EXP_TYPE_PCIE_BRIDGE 0x8	/* PCI/PCI-X to PCIE Bridge */
 #define  PCI_EXP_TYPE_RC_END	0x9	/* Root Complex Integrated Endpoint */
 #define  PCI_EXP_TYPE_RC_EC	0xa	/* Root Complex Event Collector */
-- 
1.7.1

Re: [PATCH 2/3] PCI: Use spec name for the comment of PCIe capability field

[Bjorn Helgaas]

On Fri, Sep 06, 2013 at 09:45:57AM +0800, Wei Yang wrote:
> According to the PCIe specification, bit 7:4 of PCI Express Capabilities
> Register in a PCI Express Capability Structure is used to identify the
> Device/Port type. If this field equals to 0x7, this PCIe device is a PCI
> Express to PCI/PCI-X Bridge. While the comment of this value in the code
> does not comply with the specification.
> 
> This patch changes the comment to "PCIE to PCI/PCI-X Bridge" instead of
> "PCI/PCI-X Bridge", which comply with the specification.
> 
> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
> ---
>  include/uapi/linux/pci_regs.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index c3cc01d..b82b2ff 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -421,7 +421,7 @@
>  #define  PCI_EXP_TYPE_ROOT_PORT 0x4	/* Root Port */
>  #define  PCI_EXP_TYPE_UPSTREAM	0x5	/* Upstream Port */
>  #define  PCI_EXP_TYPE_DOWNSTREAM 0x6	/* Downstream Port */
> -#define  PCI_EXP_TYPE_PCI_BRIDGE 0x7	/* PCI/PCI-X Bridge */
> +#define  PCI_EXP_TYPE_PCI_BRIDGE 0x7	/* PCIE to PCI/PCI-X Bridge */
>  #define  PCI_EXP_TYPE_PCIE_BRIDGE 0x8	/* PCI/PCI-X to PCIE Bridge */
>  #define  PCI_EXP_TYPE_RC_END	0x9	/* Root Complex Integrated Endpoint */
>  #define  PCI_EXP_TYPE_RC_EC	0xa	/* Root Complex Event Collector */
> -- 
> 1.7.1
> 

A similar change is already in Linus' tree:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/pci_regs.h?id=fbf501c347b2eea8451a615bd823b6b91a1a8eed

I guess several of us found that comment confusing :)

Re: [PATCH 2/3] PCI: Use spec name for the comment of PCIe capability field

[Wei Yang]

On Fri, Sep 06, 2013 at 05:12:43PM -0600, Bjorn Helgaas wrote:
>On Fri, Sep 06, 2013 at 09:45:57AM +0800, Wei Yang wrote:
>> According to the PCIe specification, bit 7:4 of PCI Express Capabilities
>> Register in a PCI Express Capability Structure is used to identify the
>> Device/Port type. If this field equals to 0x7, this PCIe device is a PCI
>> Express to PCI/PCI-X Bridge. While the comment of this value in the code
>> does not comply with the specification.
>> 
>> This patch changes the comment to "PCIE to PCI/PCI-X Bridge" instead of
>> "PCI/PCI-X Bridge", which comply with the specification.
>> 
>> Signed-off-by: Wei Yang <weiyang@linux.vnet.ibm.com>
>> ---
>>  include/uapi/linux/pci_regs.h |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>> 
>> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
>> index c3cc01d..b82b2ff 100644
>> --- a/include/uapi/linux/pci_regs.h
>> +++ b/include/uapi/linux/pci_regs.h
>> @@ -421,7 +421,7 @@
>>  #define  PCI_EXP_TYPE_ROOT_PORT 0x4	/* Root Port */
>>  #define  PCI_EXP_TYPE_UPSTREAM	0x5	/* Upstream Port */
>>  #define  PCI_EXP_TYPE_DOWNSTREAM 0x6	/* Downstream Port */
>> -#define  PCI_EXP_TYPE_PCI_BRIDGE 0x7	/* PCI/PCI-X Bridge */
>> +#define  PCI_EXP_TYPE_PCI_BRIDGE 0x7	/* PCIE to PCI/PCI-X Bridge */
>>  #define  PCI_EXP_TYPE_PCIE_BRIDGE 0x8	/* PCI/PCI-X to PCIE Bridge */
>>  #define  PCI_EXP_TYPE_RC_END	0x9	/* Root Complex Integrated Endpoint */
>>  #define  PCI_EXP_TYPE_RC_EC	0xa	/* Root Complex Event Collector */
>> -- 
>> 1.7.1
>> 
>
>A similar change is already in Linus' tree:
>
>http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/pci_regs.h?id=fbf501c347b2eea8451a615bd823b6b91a1a8eed
>
>I guess several of us found that comment confusing :)

Thanks, I didn't notice this :-)

-- 
Richard Yang
Help you, Help me

[PATCH 3/3] PCI: Pass full info for window alignment

[Wei Yang]

When calculating the window_alignment(), type information like IORESOURCE_MEM
and IORESOURCE_PREFETCH is not enough for some platform.

As on powernv platform, one prefetchable window could be IORESOURCE_MEM_64 or
not. The platform will calculate the alignment based on this information.

This patch passes the full info for window alignment.

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 989de3c..8781eb1 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -980,7 +980,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
 	}
 
 	min_align = calculate_mem_align(aligns, max_order);
-	min_align = max(min_align, window_alignment(bus, b_res->flags & mask));
+	min_align = max(min_align, window_alignment(bus, b_res->flags));
 	size0 = calculate_memsize(size, min_size, 0, resource_size(b_res), min_align);
 	if (children_add_size > add_size)
 		add_size = children_add_size;
-- 
1.7.1

Re: [PATCH 3/3] PCI: Pass full info for window alignment

[Bjorn Helgaas]

[+cc Gavin]

On Fri, Sep 06, 2013 at 09:45:58AM +0800, Wei Yang wrote:
> When calculating the window_alignment(), type information like IORESOURCE_MEM
> and IORESOURCE_PREFETCH is not enough for some platform.
> 
> As on powernv platform, one prefetchable window could be IORESOURCE_MEM_64 or
> not. The platform will calculate the alignment based on this information.
> 
> This patch passes the full info for window alignment.
> 
> 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 989de3c..8781eb1 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -980,7 +980,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask,
>  	}
>  
>  	min_align = calculate_mem_align(aligns, max_order);
> -	min_align = max(min_align, window_alignment(bus, b_res->flags & mask));
> +	min_align = max(min_align, window_alignment(bus, b_res->flags));

This makes sense to me, but since Gavin added this window_alignment()
call recently with the mask, I'd like him to ack this before I apply it.

>  	size0 = calculate_memsize(size, min_size, 0, resource_size(b_res), min_align);
>  	if (children_add_size > add_size)
>  		add_size = children_add_size;
> -- 
> 1.7.1
> 

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

[Bjorn Helgaas]

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

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

[Wei Yang]

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

-- 
Richard Yang
Help you, Help me

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

[Wei Yang]

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?

-- 
Richard Yang
Help you, Help me

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

[Bjorn Helgaas]

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

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

[Wei Yang]

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?

-- 
Richard Yang
Help you, Help me

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

[Benjamin Herrenschmidt]

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.

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

[Wei Yang]

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

-- 
Richard Yang
Help you, Help me

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

[Alex Chiang]

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