[PATCH] powerpc/kernel: Change retrieval of pci_dn

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

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

Reviewed-by: Sam Bobroff <redacted>

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.

Hi Bryant,

Thanks for the patch, a few comments/questions.

How have you tested this?

"Bryant G. Ly" [off-list ref] 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.

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

"Bryant G. Ly" [off-list ref] 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 <redacted>

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

cheers

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

"Bryant G. Ly" [off-list ref] writes:

quoted

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

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

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

On Mon, Aug 28, 2017 at 11:05:03AM -0500, Bryant G. Ly wrote:

quoted

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

Reviewed-by: Sam Bobroff <redacted>

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!