[PATCH] powerpc/kernel: Change retrieval of pci_dn

[PATCH] powerpc/kernel: Change retrieval of pci_dn

[Bryant G. Ly]

For a PCI device it's pci_dn can be retrieved from
pdev->dev.archdata.firmware_data, PCI_DN(devnode), or parent's list.
Thus, we should just use the generic function pci_get_pdn_by_devfn
to get the pci_dn.

Signed-off-by: Bryant G. Ly <bgly@us.ibm.com>
---
 arch/powerpc/kernel/rtas_pci.c | 30 ++----------------------------
 1 file changed, 2 insertions(+), 28 deletions(-)

diff --git a/arch/powerpc/kernel/rtas_pci.c b/arch/powerpc/kernel/rtas_pci.c
index 73f1934..c21e6c5 100644
--- a/arch/powerpc/kernel/rtas_pci.c
+++ b/arch/powerpc/kernel/rtas_pci.c
@@ -91,25 +91,13 @@ static int rtas_pci_read_config(struct pci_bus *bus,
 				unsigned int devfn,
 				int where, int size, u32 *val)
 {
-	struct device_node *busdn, *dn;
 	struct pci_dn *pdn;
-	bool found = false;
 	int ret;
 
 	/* Search only direct children of the bus */
 	*val = 0xFFFFFFFF;
-	busdn = pci_bus_to_OF_node(bus);
-	for (dn = busdn->child; dn; dn = dn->sibling) {
-		pdn = PCI_DN(dn);
-		if (pdn && pdn->devfn == devfn
-		    && of_device_is_available(dn)) {
-			found = true;
-			break;
-		}
-	}
 
-	if (!found)
-		return PCIBIOS_DEVICE_NOT_FOUND;
+	pdn = pci_get_pdn_by_devfn(bus, devfn);
 
 	ret = rtas_read_config(pdn, where, size, val);
 	if (*val == EEH_IO_ERROR_VALUE(size) &&
@@ -153,23 +141,9 @@ static int rtas_pci_write_config(struct pci_bus *bus,
 				 unsigned int devfn,
 				 int where, int size, u32 val)
 {
-	struct device_node *busdn, *dn;
 	struct pci_dn *pdn;
-	bool found = false;
-
-	/* Search only direct children of the bus */
-	busdn = pci_bus_to_OF_node(bus);
-	for (dn = busdn->child; dn; dn = dn->sibling) {
-		pdn = PCI_DN(dn);
-		if (pdn && pdn->devfn == devfn
-		    && of_device_is_available(dn)) {
-			found = true;
-			break;
-		}
-	}
 
-	if (!found)
-		return PCIBIOS_DEVICE_NOT_FOUND;
+	pdn = pci_get_pdn_by_devfn(bus, devfn);
 
 	return rtas_write_config(pdn, where, size, val);
 }
-- 
2.5.4 (Apple Git-61)

Re: [PATCH] powerpc/kernel: Change retrieval of pci_dn

[Sam Bobroff]

On Mon, Aug 28, 2017 at 11:05:03AM -0500, Bryant G. Ly wrote:
> For a PCI device it's pci_dn can be retrieved from
> pdev->dev.archdata.firmware_data, PCI_DN(devnode), or parent's list.
> Thus, we should just use the generic function pci_get_pdn_by_devfn
> to get the pci_dn.
> 
> Signed-off-by: Bryant G. Ly <bgly@us.ibm.com>
Reviewed-by: Sam Bobroff <sam.bobroff@au1.ibm.com>

I don't know this area but I tested it using a patched kernel with the
old and new code together. My test kernel booted fine (in QEMU+KVM) and
I saw 26 reads and 4 writes, all of which got the same value with either
code block.

I also checked that the error result in the "not found" case is the same
as well, which it is, because rtas_{read,write}_config() will return
PCIBIOS_DEVICE_NOT_FOUND if given a NULL pdn.

So, looks good to me.

Cheers,
Sam.

> ---
>  arch/powerpc/kernel/rtas_pci.c | 30 ++----------------------------
>  1 file changed, 2 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/rtas_pci.c b/arch/powerpc/kernel/rtas_pci.c
> index 73f1934..c21e6c5 100644
> --- a/arch/powerpc/kernel/rtas_pci.c
> +++ b/arch/powerpc/kernel/rtas_pci.c
> @@ -91,25 +91,13 @@ static int rtas_pci_read_config(struct pci_bus *bus,
>  				unsigned int devfn,
>  				int where, int size, u32 *val)
>  {
> -	struct device_node *busdn, *dn;
>  	struct pci_dn *pdn;
> -	bool found = false;
>  	int ret;
>  
>  	/* Search only direct children of the bus */
>  	*val = 0xFFFFFFFF;
> -	busdn = pci_bus_to_OF_node(bus);
> -	for (dn = busdn->child; dn; dn = dn->sibling) {
> -		pdn = PCI_DN(dn);
> -		if (pdn && pdn->devfn == devfn
> -		    && of_device_is_available(dn)) {
> -			found = true;
> -			break;
> -		}
> -	}
>  
> -	if (!found)
> -		return PCIBIOS_DEVICE_NOT_FOUND;
> +	pdn = pci_get_pdn_by_devfn(bus, devfn);
>  
>  	ret = rtas_read_config(pdn, where, size, val);
>  	if (*val == EEH_IO_ERROR_VALUE(size) &&
> @@ -153,23 +141,9 @@ static int rtas_pci_write_config(struct pci_bus *bus,
>  				 unsigned int devfn,
>  				 int where, int size, u32 val)
>  {
> -	struct device_node *busdn, *dn;
>  	struct pci_dn *pdn;
> -	bool found = false;
> -
> -	/* Search only direct children of the bus */
> -	busdn = pci_bus_to_OF_node(bus);
> -	for (dn = busdn->child; dn; dn = dn->sibling) {
> -		pdn = PCI_DN(dn);
> -		if (pdn && pdn->devfn == devfn
> -		    && of_device_is_available(dn)) {
> -			found = true;
> -			break;
> -		}
> -	}
>  
> -	if (!found)
> -		return PCIBIOS_DEVICE_NOT_FOUND;
> +	pdn = pci_get_pdn_by_devfn(bus, devfn);
>  
>  	return rtas_write_config(pdn, where, size, val);
>  }
> -- 
> 2.5.4 (Apple Git-61)

Re: [PATCH] powerpc/kernel: Change retrieval of pci_dn

[Michael Ellerman]

Hi Bryant,

Thanks for the patch, a few comments/questions.

How have you tested this?

"Bryant G. Ly" <bryantly@linux.vnet.ibm.com> writes:
> For a PCI device it's pci_dn can be retrieved from
> pdev->dev.archdata.firmware_data, PCI_DN(devnode), or parent's list.
> Thus, we should just use the generic function pci_get_pdn_by_devfn
> to get the pci_dn.

"generic" means shared between architectures, which is not true for
pci_get_pdn_by_devfn(). "existing" would be more appropriate.

> diff --git a/arch/powerpc/kernel/rtas_pci.c b/arch/powerpc/kernel/rtas_pci.c
> index 73f1934..c21e6c5 100644
> --- a/arch/powerpc/kernel/rtas_pci.c
> +++ b/arch/powerpc/kernel/rtas_pci.c
> @@ -91,25 +91,13 @@ static int rtas_pci_read_config(struct pci_bus *bus,
>  				unsigned int devfn,
>  				int where, int size, u32 *val)
>  {
> -	struct device_node *busdn, *dn;
>  	struct pci_dn *pdn;
> -	bool found = false;
>  	int ret;
>  
>  	/* Search only direct children of the bus */

You kept the comment, but is it still true, and what does it apply to
now?

Also you removed the comment below in write, I'd expect it to either
stay in both or be removed in both?

>  	*val = 0xFFFFFFFF;
> -	busdn = pci_bus_to_OF_node(bus);
> -	for (dn = busdn->child; dn; dn = dn->sibling) {
> -		pdn = PCI_DN(dn);
> -		if (pdn && pdn->devfn == devfn
> -		    && of_device_is_available(dn)) {
> -			found = true;
> -			break;
> -		}
> -	}
>  
> -	if (!found)
> -		return PCIBIOS_DEVICE_NOT_FOUND;
> +	pdn = pci_get_pdn_by_devfn(bus, devfn);

This appears to not handle the pdn == NULL case.

>  	ret = rtas_read_config(pdn, where, size, val);

But actually is handled in there ^
That would have been worth mentioning in the change log.

cheers

>  	if (*val == EEH_IO_ERROR_VALUE(size) &&
> @@ -153,23 +141,9 @@ static int rtas_pci_write_config(struct pci_bus *bus,
>  				 unsigned int devfn,
>  				 int where, int size, u32 val)
>  {
> -	struct device_node *busdn, *dn;
>  	struct pci_dn *pdn;
> -	bool found = false;
> -
> -	/* Search only direct children of the bus */
> -	busdn = pci_bus_to_OF_node(bus);
> -	for (dn = busdn->child; dn; dn = dn->sibling) {
> -		pdn = PCI_DN(dn);
> -		if (pdn && pdn->devfn == devfn
> -		    && of_device_is_available(dn)) {
> -			found = true;
> -			break;
> -		}
> -	}
>  
> -	if (!found)
> -		return PCIBIOS_DEVICE_NOT_FOUND;
> +	pdn = pci_get_pdn_by_devfn(bus, devfn);
>  
>  	return rtas_write_config(pdn, where, size, val);
>  }
> -- 
> 2.5.4 (Apple Git-61)

Re: [PATCH] powerpc/kernel: Change retrieval of pci_dn

[Michael Ellerman]

"Bryant G. Ly" <bryantly@linux.vnet.ibm.com> writes:

> For a PCI device it's pci_dn can be retrieved from
> pdev->dev.archdata.firmware_data, PCI_DN(devnode), or parent's list.
> Thus, we should just use the generic function pci_get_pdn_by_devfn
> to get the pci_dn.
>
> Signed-off-by: Bryant G. Ly <bgly@us.ibm.com>

Minor issue, it's preferable if the email in your Signed-off-by matches
the email you send patches from.

cheers

[PATCH] powerpc/kernel: Change retrieval of pci_dn

[Bryant G. Ly]

For a PCI device it's pci_dn can be retrieved from
pdev->dev.archdata.firmware_data, PCI_DN(devnode), or parent's list.
Thus, we should just use the existing function pci_get_pdn_by_devfn
to get the pci_dn.

Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
Reviewed-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---
 arch/powerpc/kernel/rtas_pci.c | 33 ++++-----------------------------
 1 file changed, 4 insertions(+), 29 deletions(-)

diff --git a/arch/powerpc/kernel/rtas_pci.c b/arch/powerpc/kernel/rtas_pci.c
index 73f1934..c2b148b 100644
--- a/arch/powerpc/kernel/rtas_pci.c
+++ b/arch/powerpc/kernel/rtas_pci.c
@@ -91,26 +91,14 @@ static int rtas_pci_read_config(struct pci_bus *bus,
 				unsigned int devfn,
 				int where, int size, u32 *val)
 {
-	struct device_node *busdn, *dn;
 	struct pci_dn *pdn;
-	bool found = false;
 	int ret;
 
-	/* Search only direct children of the bus */
 	*val = 0xFFFFFFFF;
-	busdn = pci_bus_to_OF_node(bus);
-	for (dn = busdn->child; dn; dn = dn->sibling) {
-		pdn = PCI_DN(dn);
-		if (pdn && pdn->devfn == devfn
-		    && of_device_is_available(dn)) {
-			found = true;
-			break;
-		}
-	}
 
-	if (!found)
-		return PCIBIOS_DEVICE_NOT_FOUND;
+	pdn = pci_get_pdn_by_devfn(bus, devfn);
 
+	/* Validity of pdn is checked in here */
 	ret = rtas_read_config(pdn, where, size, val);
 	if (*val == EEH_IO_ERROR_VALUE(size) &&
 	    eeh_dev_check_failure(pdn_to_eeh_dev(pdn)))
@@ -153,24 +141,11 @@ static int rtas_pci_write_config(struct pci_bus *bus,
 				 unsigned int devfn,
 				 int where, int size, u32 val)
 {
-	struct device_node *busdn, *dn;
 	struct pci_dn *pdn;
-	bool found = false;
-
-	/* Search only direct children of the bus */
-	busdn = pci_bus_to_OF_node(bus);
-	for (dn = busdn->child; dn; dn = dn->sibling) {
-		pdn = PCI_DN(dn);
-		if (pdn && pdn->devfn == devfn
-		    && of_device_is_available(dn)) {
-			found = true;
-			break;
-		}
-	}
 
-	if (!found)
-		return PCIBIOS_DEVICE_NOT_FOUND;
+	pdn = pci_get_pdn_by_devfn(bus, devfn);
 
+	/* Validity of pdn is checked in here. */
 	return rtas_write_config(pdn, where, size, val);
 }
 
-- 
2.5.4 (Apple Git-61)

Re: [PATCH] powerpc/kernel: Change retrieval of pci_dn

[Bryant G. Ly]

On 8/29/17 1:33 AM, Michael Ellerman wrote:

> "Bryant G. Ly" <bryantly@linux.vnet.ibm.com> writes:
>
>> For a PCI device it's pci_dn can be retrieved from
>> pdev->dev.archdata.firmware_data, PCI_DN(devnode), or parent's list.
>> Thus, we should just use the generic function pci_get_pdn_by_devfn
>> to get the pci_dn.
>>
>> Signed-off-by: Bryant G. Ly <bgly@us.ibm.com>
> Minor issue, it's preferable if the email in your Signed-off-by matches
> the email you send patches from.
>
> cheers
>
Hi Michael,

Thanks for the review. I apologize for the email's not matching, I switch between the two frequently
throughout the day for internal gerrit commits and Linux patches. I have addressed all your comments
in the new patch that I had just put up. Also, I have tested it with mellanox cx4 cards on P8 systems.

I'd also like to let you know that I am working on patches to enable SRIOV on power and would like
your feedback on design, which I will send in a private email.

-Bryant

Re: [PATCH] powerpc/kernel: Change retrieval of pci_dn

[Bryant G. Ly]

On 8/29/17 1:20 AM, Sam Bobroff wrote:

> On Mon, Aug 28, 2017 at 11:05:03AM -0500, Bryant G. Ly wrote:
>> For a PCI device it's pci_dn can be retrieved from
>> pdev->dev.archdata.firmware_data, PCI_DN(devnode), or parent's list.
>> Thus, we should just use the generic function pci_get_pdn_by_devfn
>> to get the pci_dn.
>>
>> Signed-off-by: Bryant G. Ly <bgly@us.ibm.com>
> Reviewed-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
>
> I don't know this area but I tested it using a patched kernel with the
> old and new code together. My test kernel booted fine (in QEMU+KVM) and
> I saw 26 reads and 4 writes, all of which got the same value with either
> code block.
>
> I also checked that the error result in the "not found" case is the same
> as well, which it is, because rtas_{read,write}_config() will return
> PCIBIOS_DEVICE_NOT_FOUND if given a NULL pdn.
>
> So, looks good to me.
>
> Cheers,
> Sam.
>
Thanks for the review Sam!

Re: powerpc/kernel: Change retrieval of pci_dn

[Michael Ellerman]

On Tue, 2017-08-29 at 13:11:51 UTC, "Bryant G. Ly" wrote:
> For a PCI device it's pci_dn can be retrieved from
> pdev->dev.archdata.firmware_data, PCI_DN(devnode), or parent's list.
> Thus, we should just use the existing function pci_get_pdn_by_devfn
> to get the pci_dn.
> 
> Signed-off-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
> Reviewed-by: Sam Bobroff <sam.bobroff@au1.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/f9df74dfce542ad2aea2e601ef1a12

cheers