Re: [PATCH] powerpc/eeh: Fix slot locations on NPU and legacy platforms

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

On Thu, Aug 04, 2016 at 03:01:15PM +1000, Russell Currey wrote:
>The slot location code as part of EEH has never functioned perfectly on
>every powerpc system.  The device node properties "ibm,slot-loc",
>"ibm,slot-location-code" and "ibm,io-base-loc-code" have all been
>presented in different cases, and in some situations, there are legacy
>platforms not conforming to the conventions of populating root buses with
>"ibm,io-base-loc-code" and child nodes with "ibm,slot-location-code".
>
>Specifically, some legacy platforms use "ibm,loc-code" instead, which
>stopped working with 7e56f627768.  In addition, EEH PEs for NPU devices
>have slot locations specified on the devices instead of buses due to their
>architecture, and these were not printed.  This has been fixed by looking
>at the top device of a PE for a slot location before checking its bus.
>
>Fixes: 7e56f627768 "powerpc/eeh: Fix PE location code"

Please describe what the patch is going to do if possible. I didn't completely
git it from the changelog.

Fixes: 7e56f627768 ("powerpc/eeh: Fix PE location code")

>Cc: <stable@vger.kernel.org> #4.4+
>Signed-off-by: Russell Currey <ruscur@russell.cc>
>---
> arch/powerpc/kernel/eeh_pe.c | 31 ++++++++++++++++++++++++++-----
> 1 file changed, 26 insertions(+), 5 deletions(-)
>
>diff --git a/arch/powerpc/kernel/eeh_pe.c b/arch/powerpc/kernel/eeh_pe.c
>index f0520da..034538c 100644
>--- a/arch/powerpc/kernel/eeh_pe.c
>+++ b/arch/powerpc/kernel/eeh_pe.c
>@@ -881,17 +881,34 @@ void eeh_pe_restore_bars(struct eeh_pe *pe)
>  * eeh_pe_loc_get - Retrieve location code binding to the given PE
>  * @pe: EEH PE
>  *
>- * Retrieve the location code of the given PE. If the primary PE bus
>- * is root bus, we will grab location code from PHB device tree node
>- * or root port. Otherwise, the upstream bridge's device tree node
>- * of the primary PE bus will be checked for the location code.
>+ * Retrieve the location code of the given PE. The first device associated
>+ * with the PE is checked for a slot location.  If missing, the bus of the
>+ * device is checked instead.  If this is a root bus, the location code is
>+ * taken from the PHB device tree node or root port.  If not, the upstream
>+ * bridge's device tree node of the primary PE bus will be checked instead.
>+ * If a slot location isn't found on the bus, walk through parent buses
>+ * until a location is found.
>  */
> const char *eeh_pe_loc_get(struct eeh_pe *pe)
> {
>-	struct pci_bus *bus = eeh_pe_bus_get(pe);
>+	struct pci_bus *bus;
> 	struct device_node *dn;
> 	const char *loc = NULL;
>
>+	/* Check the slot location of the first (top) PCI device */
>+	struct eeh_dev *edev =
>+		list_first_entry_or_null(&pe->edevs, struct eeh_dev, list);
>+
>+	if (edev) {
>+		loc = of_get_property(edev->pdn->node,
>+				      "ibm,slot-location-code", NULL);
>+		if (loc)
>+			return loc;
>+	}

I don't think it's correct as it's to return the child PE's location code.
The @edev's "ibm,loc-code" should be used in this case. On PowerNV, we
needn't check "ibm,loc-code" as it's created based on "ibm,slot-location-code"
of its bus's node, which is covered by original code. However, "ibm,loc-code"
should be checked on pSeries as we don't have "ibm,slot-location-code" and
"ibm,slot-label" there.

Please use eeh_dev_to_pdn() when getting pdn from a EEH device.

>+
>+	/* If there's nothing on the device, look at the bus */
>+	bus = eeh_pe_bus_get(pe);
>+
> 	while (bus) {
> 		dn = pci_bus_to_OF_node(bus);
> 		if (!dn) {
>@@ -905,6 +922,10 @@ const char *eeh_pe_loc_get(struct eeh_pe *pe)
> 			loc = of_get_property(dn, "ibm,slot-location-code",
> 					      NULL);
>
>+		/* Fall back to ibm,loc-code if nothing else is found */
>+		if (!loc)
>+			loc = of_get_property(dn, "ibm,loc-code", NULL);
>+

We actually need consistent order as above: "ibm,loc-code" has high
priority than "ibm,slot-location-code" and "ibm,io-base-loc-code".

Thanks,
Gavin

> 		if (loc)
> 			return loc;
>
>-- 
>2.9.2
>