| Message ID | 1503936303-18274-1-git-send-email-bryantly@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc/kernel: Change retrieval of pci_dn | expand |
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)
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)
"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
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
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!
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); }
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(-)