Re: [PATCH] Bugfix: powerpc/eeh: Wrong place to call pci_get_slot()

[Gavin Shan <gwshan@linux.vnet.ibm.com>]

On Mon, Jul 14, 2014 at 09:35:18PM +0800, Mike Qiu wrote:
>On 07/14/2014 09:01 PM, Gavin Shan wrote:
>>On Mon, Jul 14, 2014 at 04:19:23AM -0400, Mike Qiu wrote:

.../...

>>>Signed-off-by: Mike Qiu <qiudayu@linux.vnet.ibm.com>
>>>---
>>>arch/powerpc/kernel/eeh_pe.c | 29 ++++++++++++++++++++++++++---
>>>1 file changed, 26 insertions(+), 3 deletions(-)
>>>
>>>diff --git a/arch/powerpc/kernel/eeh_pe.c b/arch/powerpc/kernel/eeh_pe.c
>>>index fbd01eb..6f4bfee 100644
>>>--- a/arch/powerpc/kernel/eeh_pe.c
>>>+++ b/arch/powerpc/kernel/eeh_pe.c
>>>@@ -792,6 +792,28 @@ void eeh_pe_restore_bars(struct eeh_pe *pe)
>>>}
>>>
>>>/**
>>>+ * __dn_get_pdev - Retrieve the pci_dev from device_node by bus/devfn
>>>+ * @dn: device_node of the pci_dev
>>>+ * @data: the pci device's bus/devfn
>>>+ *
>>>+ * Retrieve the pci_dev using the given device_node and bus/devfn.
>>>+ */
>>>+void *__dn_get_pdev(struct device_node *dn, void *data)
>>>+{
>>The function isn't necessarily public. "static" is enough, I think.
>>I don't think we need this actually. Please refer to more comments
>>below.
>>
>>>+	struct pci_dn *pdn = PCI_DN(dn);
>>>+	int busno = *((int *)data) >> 8;
>>>+	int devfn = *((int *)data) & 0xff;
>>>+
>>>+	if (!pdn)
>>>+		return NULL;
>>>+
>>>+	if (pdn->busno == busno && pdn->devfn == devfn)
>>>+		return pdn->pcidev;
>>>+
>>>+	return NULL;
>>>+}
>>>+
>>>+/**
>>>  * eeh_pe_loc_get - Retrieve location code binding to the given PE
>>>  * @pe: EEH PE
>>>  *
>>>@@ -807,6 +829,7 @@ const char *eeh_pe_loc_get(struct eeh_pe *pe)
>>>	struct pci_dev *pdev;
>>>	struct device_node *dn;
>>>	const char *loc;
>>>+	int bdevfn;
>>>
>>>	if (!bus)
>>>		return "N/A";
>>>@@ -823,7 +846,9 @@ const char *eeh_pe_loc_get(struct eeh_pe *pe)
>>>		if (loc)
>>>			return loc;
>>>
>>>-		pdev = pci_get_slot(bus, 0x0);
>>>+		/* Get the root port */
>>>+		bdevfn = (bus->number) << 8 || 0x0;
>>>+		pdev = traverse_pci_devices(hose->dn, __dn_get_pdev, &bdevfn);
>>We needn't search pdev from device-tree and then translate it to
>>device-node. Root port is the only child hooked to root bus's
>>device node (it's also PHB's device-node). So I guess you can
>
>So here you mean, child hooked device node(root port) can be
>hose->dn->child?
>

Yes, I think so.

>>just have something:
>>
>>		/* Check PHB's device node */
>>		dn = pci_bus_to_OF_node(bus);
>>		if (unlikely(!dn)) {
>>			loc = "N/A";
>>			goto out;
>>		}
>>		loc = of_get_property(hose->dn,
>>  				      "ibm,loc-code", NULL);
>>		if (loc)
>>			return loc;
>>		loc = of_get_property(hose->dn,
>>				      "ibm,io-base-loc-code", NULL);
>>		if (loc)
>>			return loc;
>>
>>		/* Check root port */
>>		dn = dn->child;
>>>	} else {
>>>		pdev = bus->self;
>>Here, we needn't grab the bridge as well:
>>
>>		dn = pci_bus_to_OF_node(bus);
>>>	}
>>Then check the device-node of bridge (or root port):
>>
>>	if (unlikely(!dn)) {
>>		loc = "N/A";
>>		goto out;
>>	}
>>
>>         loc = of_get_property(dn, "ibm,loc-code", NULL);
>>         if (!loc)
>>                 loc = of_get_property(dn, "ibm,slot-location-code", NULL);
>>         if (!loc)
>>                 loc = "N/A";
>>
>>>@@ -846,8 +871,6 @@ const char *eeh_pe_loc_get(struct eeh_pe *pe)
>>>		loc = "N/A";
>>>
>>>out:
>>>-	if (pci_is_root_bus(bus) && pdev)
>>>-		pci_dev_put(pdev);
>>>	return loc;
>>>}
>
>I will make a new patch, after tested, I will resend it out
>

Thanks, Mike.

Thanks,
Gavin