[PATCH v3] PCI hotplug: rpaphp: Error out on busy status from get-sensor-state

[PATCH v3] PCI hotplug: rpaphp: Error out on busy status from get-sensor-state

[Mahesh Salgaonkar]

When certain PHB HW failure causes phyp to recover PHB, it marks the PE
state as temporarily unavailable until recovery is complete. This also
triggers an EEH handler in Linux which needs to notify drivers, and perform
recovery. But before notifying the driver about the pci error it uses
get_adapter_state()->get-sesnor-state() operation of the hotplug_slot to
determine if the slot contains a device or not. if the slot is empty, the
recovery is skipped entirely.

However on certain PHB failures, the rtas call get-sesnor-state() returns
extended busy error (9902) until PHB is recovered by phyp. Once PHB is
recovered, the get-sensor-state() returns success with correct presence
status. The rtas call interface rtas_get_sensor() loops over the rtas call
on extended delay return code (9902) until the return value is either
success (0) or error (-1). This causes the EEH handler to get stuck for ~6
seconds before it could notify that the pci error has been detected and
stop any active operations. Hence with running I/O traffic, during this 6
seconds, the network driver continues its operation and hits a timeout
(netdev watchdog). On timeouts, network driver go into ffdc capture mode
and reset path assuming the PCI device is in fatal condition. This
sometimes causes EEH recovery to fail. This impacts the ssh connection and
leads to the system being inaccessible.

------------
[52732.244731] DEBUG: ibm_read_slot_reset_state2()
[52732.244762] DEBUG: ret = 0, rets[0]=5, rets[1]=1, rets[2]=4000, rets[3]=>
[52732.244798] DEBUG: in eeh_slot_presence_check
[52732.244804] DEBUG: error state check
[52732.244807] DEBUG: Is slot hotpluggable
[52732.244810] DEBUG: hotpluggable ops ?
[52732.244953] DEBUG: Calling ops->get_adapter_status
[52732.244958] DEBUG: calling rpaphp_get_sensor_state
[52736.564262] ------------[ cut here ]------------
[52736.564299] NETDEV WATCHDOG: enP64p1s0f3 (tg3): transmit queue 0 timed o>
[52736.564324] WARNING: CPU: 1442 PID: 0 at net/sched/sch_generic.c:478 dev>
[...]
[52736.564505] NIP [c000000000c32368] dev_watchdog+0x438/0x440
[52736.564513] LR [c000000000c32364] dev_watchdog+0x434/0x440
------------

To avoid this issue, fix the pci hotplug driver (rpaphp) to return an error
if the slot presence state can not be detected immediately. Current
implementation uses rtas_get_sensor() API which blocks the slot check state
until rtas call returns success. Change rpaphp_get_sensor_state() to invoke
rtas_call(get-sensor-state) directly and take actions based on rtas return
status. This patch now errors out immediately on busy return status from
rtas_call.

Please note that, only on certain PHB failures, the slot presence check
returns BUSY condition. In normal cases it returns immediately with a
correct presence state value. Hence this change has no impact on normal pci
dlpar operations.

Signed-off-by: Mahesh Salgaonkar <mahesh@linux.ibm.com>
---
Change in V3:
- Invoke rtas_call(get-sensor-state) directly from
  rpaphp_get_sensor_state() directly and do special handling.
- See v2 at
  https://lists.ozlabs.org/pipermail/linuxppc-dev/2021-November/237336.html

Change in V2:
- Alternate approach to fix the EEH issue instead of delaying slot presence
  check proposed at
  https://lists.ozlabs.org/pipermail/linuxppc-dev/2021-November/236956.html

Also refer:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2021-November/237027.html
---
 drivers/pci/hotplug/rpaphp_pci.c |   64 ++++++++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/hotplug/rpaphp_pci.c b/drivers/pci/hotplug/rpaphp_pci.c
index c380bdacd1466..57ec1a72b24aa 100644
--- a/drivers/pci/hotplug/rpaphp_pci.c
+++ b/drivers/pci/hotplug/rpaphp_pci.c
@@ -18,15 +18,65 @@
 #include "../pci.h"		/* for pci_add_new_bus */
 #include "rpaphp.h"
 
+/*
+ * RTAS call get-sensor-state(DR_ENTITY_SENSE) return values as per PAPR:
+ *    -1: Hardware Error
+ *    -2: RTAS_BUSY
+ *    -3: Invalid sensor. RTAS Parameter Error.
+ * -9000: Need DR entity to be powered up and unisolated before RTAS call
+ * -9001: Need DR entity to be powered up, but not unisolated, before RTAS call
+ * -9002: DR entity unusable
+ *  990x: Extended delay - where x is a number in the range of 0-5
+ */
+#define RTAS_HARDWARE_ERROR	-1
+#define RTAS_INVALID_SENSOR	-3
+#define SLOT_UNISOLATED		-9000
+#define SLOT_NOT_UNISOLATED	-9001
+#define SLOT_NOT_USABLE		-9002
+
+static int rtas_to_errno(int rtas_rc)
+{
+	int rc;
+
+	switch (rtas_rc) {
+	case RTAS_HARDWARE_ERROR:
+		rc = -EIO;
+		break;
+	case RTAS_INVALID_SENSOR:
+		rc = -EINVAL;
+		break;
+	case SLOT_UNISOLATED:
+	case SLOT_NOT_UNISOLATED:
+		rc = -EFAULT;
+		break;
+	case SLOT_NOT_USABLE:
+		rc = -ENODEV;
+		break;
+	case RTAS_BUSY:
+	case RTAS_EXTENDED_DELAY_MIN...RTAS_EXTENDED_DELAY_MAX:
+		rc = -EBUSY;
+		break;
+	default:
+		err("%s: unexpected RTAS error %d\n", __func__, rtas_rc);
+		rc = -ERANGE;
+		break;
+	}
+	return rc;
+}
+
 int rpaphp_get_sensor_state(struct slot *slot, int *state)
 {
+	int token = rtas_token("get-sensor-state");
 	int rc;
 	int setlevel;
 
-	rc = rtas_get_sensor(DR_ENTITY_SENSE, slot->index, state);
+	if (token == RTAS_UNKNOWN_SERVICE)
+		return -ENOENT;
+
+	rc = rtas_call(token, 2, 2, state, DR_ENTITY_SENSE, slot->index);
 
 	if (rc < 0) {
-		if (rc == -EFAULT || rc == -EEXIST) {
+		if (rc == SLOT_UNISOLATED  || rc == SLOT_NOT_UNISOLATED) {
 			dbg("%s: slot must be power up to get sensor-state\n",
 			    __func__);
 
@@ -38,15 +88,17 @@ int rpaphp_get_sensor_state(struct slot *slot, int *state)
 			if (rc < 0) {
 				dbg("%s: power on slot[%s] failed rc=%d.\n",
 				    __func__, slot->name, rc);
-			} else {
-				rc = rtas_get_sensor(DR_ENTITY_SENSE,
-						     slot->index, state);
+				return rc;
 			}
-		} else if (rc == -ENODEV)
+			rc = rtas_call(token, 2, 2, state, DR_ENTITY_SENSE,
+				       slot->index);
+		} else if (rc == SLOT_NOT_USABLE)
 			info("%s: slot is unusable\n", __func__);
 		else
 			err("%s failed to get sensor state\n", __func__);
 	}
+	if (rc)
+		rc = rtas_to_errno(rc);
 	return rc;
 }
 

Re: [PATCH v3] PCI hotplug: rpaphp: Error out on busy status from get-sensor-state

[Nathan Lynch]

Mahesh Salgaonkar <mahesh@linux.ibm.com> writes:
> To avoid this issue, fix the pci hotplug driver (rpaphp) to return an error
> if the slot presence state can not be detected immediately. Current
> implementation uses rtas_get_sensor() API which blocks the slot check state
> until rtas call returns success. Change rpaphp_get_sensor_state() to invoke
> rtas_call(get-sensor-state) directly and take actions based on rtas return
> status. This patch now errors out immediately on busy return status from
> rtas_call.
>
> Please note that, only on certain PHB failures, the slot presence check
> returns BUSY condition. In normal cases it returns immediately with a
> correct presence state value. Hence this change has no impact on normal pci
> dlpar operations.

I was wondering about this. This seems to be saying -2/990x cannot
happen in other cases. I couldn't find this specified in the
architecture. It seems a bit risky to me to *always* error out on
-2/990x - won't we have intermittent slot enable failures?

> +/*
> + * RTAS call get-sensor-state(DR_ENTITY_SENSE) return values as per PAPR:
> + *    -1: Hardware Error
> + *    -2: RTAS_BUSY
> + *    -3: Invalid sensor. RTAS Parameter Error.
> + * -9000: Need DR entity to be powered up and unisolated before RTAS call
> + * -9001: Need DR entity to be powered up, but not unisolated, before RTAS call
> + * -9002: DR entity unusable
> + *  990x: Extended delay - where x is a number in the range of 0-5
> + */
> +#define RTAS_HARDWARE_ERROR	-1
> +#define RTAS_INVALID_SENSOR	-3
> +#define SLOT_UNISOLATED		-9000
> +#define SLOT_NOT_UNISOLATED	-9001
> +#define SLOT_NOT_USABLE		-9002
> +
> +static int rtas_to_errno(int rtas_rc)
> +{
> +	int rc;
> +
> +	switch (rtas_rc) {
> +	case RTAS_HARDWARE_ERROR:
> +		rc = -EIO;
> +		break;
> +	case RTAS_INVALID_SENSOR:
> +		rc = -EINVAL;
> +		break;
> +	case SLOT_UNISOLATED:
> +	case SLOT_NOT_UNISOLATED:
> +		rc = -EFAULT;
> +		break;
> +	case SLOT_NOT_USABLE:
> +		rc = -ENODEV;
> +		break;
> +	case RTAS_BUSY:
> +	case RTAS_EXTENDED_DELAY_MIN...RTAS_EXTENDED_DELAY_MAX:
> +		rc = -EBUSY;
> +		break;
> +	default:
> +		err("%s: unexpected RTAS error %d\n", __func__, rtas_rc);
> +		rc = -ERANGE;
> +		break;
> +	}
> +	return rc;
> +}

These conversions look OK to me.

Re: [PATCH v3] PCI hotplug: rpaphp: Error out on busy status from get-sensor-state

[Mahesh J Salgaonkar]

On 2021-12-09 09:02:51 Thu, Nathan Lynch wrote:
> Mahesh Salgaonkar <mahesh@linux.ibm.com> writes:
> > To avoid this issue, fix the pci hotplug driver (rpaphp) to return an error
> > if the slot presence state can not be detected immediately. Current
> > implementation uses rtas_get_sensor() API which blocks the slot check state
> > until rtas call returns success. Change rpaphp_get_sensor_state() to invoke
> > rtas_call(get-sensor-state) directly and take actions based on rtas return
> > status. This patch now errors out immediately on busy return status from
> > rtas_call.
> >
> > Please note that, only on certain PHB failures, the slot presence check
> > returns BUSY condition. In normal cases it returns immediately with a
> > correct presence state value. Hence this change has no impact on normal pci
> > dlpar operations.
> 
> I was wondering about this. This seems to be saying -2/990x cannot
> happen in other cases. I couldn't find this specified in the
> architecture. It seems a bit risky to me to *always* error out on
> -2/990x - won't we have intermittent slot enable failures?

Sorry for the late response. So instead of always returning error out
how about we error out only if pe is going through EEH recovery ? During
get_adapter_status I can check if pe->state is set to EEH_PE_RECOVERING
and only then return error on busy else fallback to existing method of
rtas_get_sensor. Let me send out another version with this approach.

Thanks,
-Mahesh.

> 
> > +/*
> > + * RTAS call get-sensor-state(DR_ENTITY_SENSE) return values as per PAPR:
> > + *    -1: Hardware Error
> > + *    -2: RTAS_BUSY
> > + *    -3: Invalid sensor. RTAS Parameter Error.
> > + * -9000: Need DR entity to be powered up and unisolated before RTAS call
> > + * -9001: Need DR entity to be powered up, but not unisolated, before RTAS call
> > + * -9002: DR entity unusable
> > + *  990x: Extended delay - where x is a number in the range of 0-5
> > + */
> > +#define RTAS_HARDWARE_ERROR	-1
> > +#define RTAS_INVALID_SENSOR	-3
> > +#define SLOT_UNISOLATED		-9000
> > +#define SLOT_NOT_UNISOLATED	-9001
> > +#define SLOT_NOT_USABLE		-9002
> > +
> > +static int rtas_to_errno(int rtas_rc)
> > +{
> > +	int rc;
> > +
> > +	switch (rtas_rc) {
> > +	case RTAS_HARDWARE_ERROR:
> > +		rc = -EIO;
> > +		break;
> > +	case RTAS_INVALID_SENSOR:
> > +		rc = -EINVAL;
> > +		break;
> > +	case SLOT_UNISOLATED:
> > +	case SLOT_NOT_UNISOLATED:
> > +		rc = -EFAULT;
> > +		break;
> > +	case SLOT_NOT_USABLE:
> > +		rc = -ENODEV;
> > +		break;
> > +	case RTAS_BUSY:
> > +	case RTAS_EXTENDED_DELAY_MIN...RTAS_EXTENDED_DELAY_MAX:
> > +		rc = -EBUSY;
> > +		break;
> > +	default:
> > +		err("%s: unexpected RTAS error %d\n", __func__, rtas_rc);
> > +		rc = -ERANGE;
> > +		break;
> > +	}
> > +	return rc;
> > +}
> 
> These conversions look OK to me.

-- 
Mahesh J Salgaonkar