[PATCH net] PCI: fix the return value for the pci_find_pcie_root_port()

[PATCH net] PCI: fix the return value for the pci_find_pcie_root_port()

[Ding Tianhong]

The pci_find_pcie_root_port() would return NULL if the given
dev is already a Root Port, it looks like unfriendly to the
PCIe Root Port device, Thierry and Bjorn suggest to let this
function return the given dev under this circumstances.

Fixes: 0e405232871d6 ("PCI: fix oops when try to find Root Port for a PCI device")
Suggested-by: Thierry Reding <thierry.reding@gmail.com>
Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
---
 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7e2022f..352bb53 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -514,7 +514,7 @@ struct resource *pci_find_resource(struct pci_dev *dev, struct resource *res)
  */
 struct pci_dev *pci_find_pcie_root_port(struct pci_dev *dev)
 {
-	struct pci_dev *bridge, *highest_pcie_bridge = NULL;
+	struct pci_dev *bridge, *highest_pcie_bridge = dev;
 
 	bridge = pci_upstream_bridge(dev);
 	while (bridge && pci_is_pcie(bridge)) {
-- 
1.8.3.1

Re: [PATCH net] PCI: fix the return value for the pci_find_pcie_root_port()

[David Miller]

From: Ding Tianhong <dingtianhong@huawei.com>
Date: Thu, 17 Aug 2017 10:25:30 +0800

> The pci_find_pcie_root_port() would return NULL if the given
> dev is already a Root Port, it looks like unfriendly to the
> PCIe Root Port device, Thierry and Bjorn suggest to let this
> function return the given dev under this circumstances.
> 
> Fixes: 0e405232871d6 ("PCI: fix oops when try to find Root Port for a PCI device")
> Suggested-by: Thierry Reding <thierry.reding@gmail.com>
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>

Bjorn, please review and ACK so Linus doesn't rip my head off again
:-)

Thanks.

Re: [PATCH net] PCI: fix the return value for the pci_find_pcie_root_port()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1589 bytes --]

On Thu, Aug 17, 2017 at 10:25:30AM +0800, Ding Tianhong wrote:
> The pci_find_pcie_root_port() would return NULL if the given
> dev is already a Root Port, it looks like unfriendly to the
> PCIe Root Port device, Thierry and Bjorn suggest to let this
> function return the given dev under this circumstances.
> 
> Fixes: 0e405232871d6 ("PCI: fix oops when try to find Root Port for a PCI device")
> Suggested-by: Thierry Reding <thierry.reding@gmail.com>
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> ---
>  drivers/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7e2022f..352bb53 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -514,7 +514,7 @@ struct resource *pci_find_resource(struct pci_dev *dev, struct resource *res)
>   */
>  struct pci_dev *pci_find_pcie_root_port(struct pci_dev *dev)
>  {
> -	struct pci_dev *bridge, *highest_pcie_bridge = NULL;
> +	struct pci_dev *bridge, *highest_pcie_bridge = dev;
>  
>  	bridge = pci_upstream_bridge(dev);
>  	while (bridge && pci_is_pcie(bridge)) {

I think this should actually be this change on top of a revert of commit
0e405232871d6 ("PCI: fix oops when try to find Root Port for a PCI
device"). After the above change, the previous fix will have a redundant
check because highest_pcie_bridge will never be NULL.

Let me send out that version to clarify what I mean.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net] PCI: fix the return value for the pci_find_pcie_root_port()

[Ding Tianhong]

On 2017/8/17 18:51, Thierry Reding wrote:
> On Thu, Aug 17, 2017 at 10:25:30AM +0800, Ding Tianhong wrote:
>> The pci_find_pcie_root_port() would return NULL if the given
>> dev is already a Root Port, it looks like unfriendly to the
>> PCIe Root Port device, Thierry and Bjorn suggest to let this
>> function return the given dev under this circumstances.
>>
>> Fixes: 0e405232871d6 ("PCI: fix oops when try to find Root Port for a PCI device")
>> Suggested-by: Thierry Reding <thierry.reding@gmail.com>
>> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
>> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>> ---
>>  drivers/pci/pci.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 7e2022f..352bb53 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -514,7 +514,7 @@ struct resource *pci_find_resource(struct pci_dev *dev, struct resource *res)
>>   */
>>  struct pci_dev *pci_find_pcie_root_port(struct pci_dev *dev)
>>  {
>> -	struct pci_dev *bridge, *highest_pcie_bridge = NULL;
>> +	struct pci_dev *bridge, *highest_pcie_bridge = dev;
>>  
>>  	bridge = pci_upstream_bridge(dev);
>>  	while (bridge && pci_is_pcie(bridge)) {
> 
> I think this should actually be this change on top of a revert of commit
> 0e405232871d6 ("PCI: fix oops when try to find Root Port for a PCI
> device"). After the above change, the previous fix will have a redundant
> check because highest_pcie_bridge will never be NULL.
> 
> Let me send out that version to clarify what I mean.
> 

Hi Thierry:

The patch ("PCI: fix oops when try to find Root Port for a PCI device")
has been merge to the linus mainline tree before you found this deficiencies....

Regards
Tianhong

> Thierry
> 

Re: [PATCH net] PCI: fix the return value for the pci_find_pcie_root_port()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2188 bytes --]

On Thu, Aug 17, 2017 at 08:40:16PM +0800, Ding Tianhong wrote:
> 
> 
> On 2017/8/17 18:51, Thierry Reding wrote:
> > On Thu, Aug 17, 2017 at 10:25:30AM +0800, Ding Tianhong wrote:
> >> The pci_find_pcie_root_port() would return NULL if the given
> >> dev is already a Root Port, it looks like unfriendly to the
> >> PCIe Root Port device, Thierry and Bjorn suggest to let this
> >> function return the given dev under this circumstances.
> >>
> >> Fixes: 0e405232871d6 ("PCI: fix oops when try to find Root Port for a PCI device")
> >> Suggested-by: Thierry Reding <thierry.reding@gmail.com>
> >> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> >> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> >> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> >> ---
> >>  drivers/pci/pci.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >> index 7e2022f..352bb53 100644
> >> --- a/drivers/pci/pci.c
> >> +++ b/drivers/pci/pci.c
> >> @@ -514,7 +514,7 @@ struct resource *pci_find_resource(struct pci_dev *dev, struct resource *res)
> >>   */
> >>  struct pci_dev *pci_find_pcie_root_port(struct pci_dev *dev)
> >>  {
> >> -	struct pci_dev *bridge, *highest_pcie_bridge = NULL;
> >> +	struct pci_dev *bridge, *highest_pcie_bridge = dev;
> >>  
> >>  	bridge = pci_upstream_bridge(dev);
> >>  	while (bridge && pci_is_pcie(bridge)) {
> > 
> > I think this should actually be this change on top of a revert of commit
> > 0e405232871d6 ("PCI: fix oops when try to find Root Port for a PCI
> > device"). After the above change, the previous fix will have a redundant
> > check because highest_pcie_bridge will never be NULL.
> > 
> > Let me send out that version to clarify what I mean.
> > 
> 
> Hi Thierry:
> 
> The patch ("PCI: fix oops when try to find Root Port for a PCI device")
> has been merge to the linus mainline tree before you found this deficiencies....

I understand that. I'm just saying that there's no point keeping that
change around because it no longer makes sense after we initialize the
highest_pcie_bridge variable to dev.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH net] PCI: fix the return value for the pci_find_pcie_root_port()

[Ding Tianhong]

On 2017/8/17 21:30, Thierry Reding wrote:
> On Thu, Aug 17, 2017 at 08:40:16PM +0800, Ding Tianhong wrote:
>>
>>
>> On 2017/8/17 18:51, Thierry Reding wrote:
>>> On Thu, Aug 17, 2017 at 10:25:30AM +0800, Ding Tianhong wrote:
>>>> The pci_find_pcie_root_port() would return NULL if the given
>>>> dev is already a Root Port, it looks like unfriendly to the
>>>> PCIe Root Port device, Thierry and Bjorn suggest to let this
>>>> function return the given dev under this circumstances.
>>>>
>>>> Fixes: 0e405232871d6 ("PCI: fix oops when try to find Root Port for a PCI device")
>>>> Suggested-by: Thierry Reding <thierry.reding@gmail.com>
>>>> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
>>>> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
>>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>>>> ---
>>>>  drivers/pci/pci.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index 7e2022f..352bb53 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -514,7 +514,7 @@ struct resource *pci_find_resource(struct pci_dev *dev, struct resource *res)
>>>>   */
>>>>  struct pci_dev *pci_find_pcie_root_port(struct pci_dev *dev)
>>>>  {
>>>> -	struct pci_dev *bridge, *highest_pcie_bridge = NULL;
>>>> +	struct pci_dev *bridge, *highest_pcie_bridge = dev;
>>>>  
>>>>  	bridge = pci_upstream_bridge(dev);
>>>>  	while (bridge && pci_is_pcie(bridge)) {
>>>
>>> I think this should actually be this change on top of a revert of commit
>>> 0e405232871d6 ("PCI: fix oops when try to find Root Port for a PCI
>>> device"). After the above change, the previous fix will have a redundant
>>> check because highest_pcie_bridge will never be NULL.
>>>
>>> Let me send out that version to clarify what I mean.
>>>
>>
>> Hi Thierry:
>>
>> The patch ("PCI: fix oops when try to find Root Port for a PCI device")
>> has been merge to the linus mainline tree before you found this deficiencies....
> 
> I understand that. I'm just saying that there's no point keeping that
> change around because it no longer makes sense after we initialize the
> highest_pcie_bridge variable to dev.
> 

Ok, NO problem.:)

> Thierry
> 

[PATCH] PCI: Allow PCI express root ports to find themselves

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

If the pci_find_pcie_root_port() function is called on a root port
itself, return the root port rather than NULL.

This effectively reverts commit 0e405232871d6 ("PCI: fix oops when
try to find Root Port for a PCI device") which added an extra check
that would now be redundant.

Fixes: a99b646afa8a ("PCI: Disable PCIe Relaxed Ordering if unsupported")
Fixes: c56d4450eb68 ("PCI: Turn off Request Attributes to avoid Chelsio T5 Completion erratum")
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
This applies on top of and was tested on next-20170817.

Michael, it'd be great if you could test this one again to clarify
whether or not the fix that's already in Linus' tree is still needed, or
whether it's indeed obsoleted by this patch.

 drivers/pci/pci.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b05c587e335a..dd56c1c05614 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -514,7 +514,7 @@ EXPORT_SYMBOL(pci_find_resource);
  */
 struct pci_dev *pci_find_pcie_root_port(struct pci_dev *dev)
 {
-	struct pci_dev *bridge, *highest_pcie_bridge = NULL;
+	struct pci_dev *bridge, *highest_pcie_bridge = dev;
 
 	bridge = pci_upstream_bridge(dev);
 	while (bridge && pci_is_pcie(bridge)) {
@@ -522,11 +522,10 @@ struct pci_dev *pci_find_pcie_root_port(struct pci_dev *dev)
 		bridge = pci_upstream_bridge(bridge);
 	}
 
-	if (highest_pcie_bridge &&
-	    pci_pcie_type(highest_pcie_bridge) == PCI_EXP_TYPE_ROOT_PORT)
-		return highest_pcie_bridge;
+	if (pci_pcie_type(highest_pcie_bridge) != PCI_EXP_TYPE_ROOT_PORT)
+		return NULL;
 
-	return NULL;
+	return highest_pcie_bridge;
 }
 EXPORT_SYMBOL(pci_find_pcie_root_port);
 
-- 
2.13.3

Re: [PATCH] PCI: Allow PCI express root ports to find themselves

[Bjorn Helgaas]

On Thu, Aug 17, 2017 at 01:06:14PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> If the pci_find_pcie_root_port() function is called on a root port
> itself, return the root port rather than NULL.
> 
> This effectively reverts commit 0e405232871d6 ("PCI: fix oops when
> try to find Root Port for a PCI device") which added an extra check
> that would now be redundant.
> 
> Fixes: a99b646afa8a ("PCI: Disable PCIe Relaxed Ordering if unsupported")
> Fixes: c56d4450eb68 ("PCI: Turn off Request Attributes to avoid Chelsio T5 Completion erratum")
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

I *think* this should work for everybody, but I can't test it personally.

> ---
> This applies on top of and was tested on next-20170817.
> 
> Michael, it'd be great if you could test this one again to clarify
> whether or not the fix that's already in Linus' tree is still needed, or
> whether it's indeed obsoleted by this patch.
> 
>  drivers/pci/pci.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b05c587e335a..dd56c1c05614 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -514,7 +514,7 @@ EXPORT_SYMBOL(pci_find_resource);
>   */
>  struct pci_dev *pci_find_pcie_root_port(struct pci_dev *dev)
>  {
> -	struct pci_dev *bridge, *highest_pcie_bridge = NULL;
> +	struct pci_dev *bridge, *highest_pcie_bridge = dev;
>  
>  	bridge = pci_upstream_bridge(dev);
>  	while (bridge && pci_is_pcie(bridge)) {
> @@ -522,11 +522,10 @@ struct pci_dev *pci_find_pcie_root_port(struct pci_dev *dev)
>  		bridge = pci_upstream_bridge(bridge);
>  	}
>  
> -	if (highest_pcie_bridge &&
> -	    pci_pcie_type(highest_pcie_bridge) == PCI_EXP_TYPE_ROOT_PORT)
> -		return highest_pcie_bridge;
> +	if (pci_pcie_type(highest_pcie_bridge) != PCI_EXP_TYPE_ROOT_PORT)
> +		return NULL;
>  
> -	return NULL;
> +	return highest_pcie_bridge;
>  }
>  EXPORT_SYMBOL(pci_find_pcie_root_port);
>  
> -- 
> 2.13.3
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: Allow PCI express root ports to find themselves

[Shawn Lin]

Hi

On 2017/8/17 19:06, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> If the pci_find_pcie_root_port() function is called on a root port
> itself, return the root port rather than NULL.
> 
> This effectively reverts commit 0e405232871d6 ("PCI: fix oops when
> try to find Root Port for a PCI device") which added an extra check
> that would now be redundant.
> 
> Fixes: a99b646afa8a ("PCI: Disable PCIe Relaxed Ordering if unsupported")
> Fixes: c56d4450eb68 ("PCI: Turn off Request Attributes to avoid Chelsio T5 Completion erratum")
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Tested-by: Shawn Lin <shawn.lin@rock-chips.com>

> ---
> This applies on top of and was tested on next-20170817.
> 
> Michael, it'd be great if you could test this one again to clarify
> whether or not the fix that's already in Linus' tree is still needed, or
> whether it's indeed obsoleted by this patch.
> 
>   drivers/pci/pci.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b05c587e335a..dd56c1c05614 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -514,7 +514,7 @@ EXPORT_SYMBOL(pci_find_resource);
>    */
>   struct pci_dev *pci_find_pcie_root_port(struct pci_dev *dev)
>   {
> -	struct pci_dev *bridge, *highest_pcie_bridge = NULL;
> +	struct pci_dev *bridge, *highest_pcie_bridge = dev;
>   
>   	bridge = pci_upstream_bridge(dev);
>   	while (bridge && pci_is_pcie(bridge)) {
> @@ -522,11 +522,10 @@ struct pci_dev *pci_find_pcie_root_port(struct pci_dev *dev)
>   		bridge = pci_upstream_bridge(bridge);
>   	}
>   
> -	if (highest_pcie_bridge &&
> -	    pci_pcie_type(highest_pcie_bridge) == PCI_EXP_TYPE_ROOT_PORT)
> -		return highest_pcie_bridge;
> +	if (pci_pcie_type(highest_pcie_bridge) != PCI_EXP_TYPE_ROOT_PORT)
> +		return NULL;
>   
> -	return NULL;
> +	return highest_pcie_bridge;
>   }
>   EXPORT_SYMBOL(pci_find_pcie_root_port);
>   
> 

Re: [PATCH] PCI: Allow PCI express root ports to find themselves

[Michael Ellerman]

Thierry Reding <thierry.reding@gmail.com> writes:

> From: Thierry Reding <treding@nvidia.com>
>
> If the pci_find_pcie_root_port() function is called on a root port
> itself, return the root port rather than NULL.
>
> This effectively reverts commit 0e405232871d6 ("PCI: fix oops when
> try to find Root Port for a PCI device") which added an extra check
> that would now be redundant.
>
> Fixes: a99b646afa8a ("PCI: Disable PCIe Relaxed Ordering if unsupported")
> Fixes: c56d4450eb68 ("PCI: Turn off Request Attributes to avoid Chelsio T5 Completion erratum")
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> This applies on top of and was tested on next-20170817.
>
> Michael, it'd be great if you could test this one again to clarify
> whether or not the fix that's already in Linus' tree is still needed, or
> whether it's indeed obsoleted by this patch.

This works fine for me, applied on top of Linus' tree (d33a2a914319).

cheers

Re: [PATCH] PCI: Allow PCI express root ports to find themselves

[David Miller]

From: Thierry Reding <thierry.reding@gmail.com>
Date: Thu, 17 Aug 2017 13:06:14 +0200

> From: Thierry Reding <treding@nvidia.com>
> 
> If the pci_find_pcie_root_port() function is called on a root port
> itself, return the root port rather than NULL.
> 
> This effectively reverts commit 0e405232871d6 ("PCI: fix oops when
> try to find Root Port for a PCI device") which added an extra check
> that would now be redundant.
> 
> Fixes: a99b646afa8a ("PCI: Disable PCIe Relaxed Ordering if unsupported")
> Fixes: c56d4450eb68 ("PCI: Turn off Request Attributes to avoid Chelsio T5 Completion erratum")
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> This applies on top of and was tested on next-20170817.
> 
> Michael, it'd be great if you could test this one again to clarify
> whether or not the fix that's already in Linus' tree is still needed, or
> whether it's indeed obsoleted by this patch.

Applied, thanks everyone.