[PATCH 1/2] pseries: device node status can be "ok" or "okay"

[PATCH 1/2] pseries: device node status can be "ok" or "okay"

[Linas Vepstas]

It seems that some versions of firmware will report a device
node status as the string "okay". As we are not expecting this
string, the device node will be ignored by the EEH subsystem.
Which means EEH will not be enabled. 

When EEH is not enabled, PCI errors will be converted into 
Machine Check exceptions, and we'll have a very unhappy system. 

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>

----

Paul,

This is a bug with serious reprecussions ... but appearently 
affects only a few systems, so far.  I've never seen it before. 
Your pick as to whether to jam this into 2.6.23 or wait until 
later.

--linas

 arch/powerpc/platforms/pseries/eeh.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.22-git2/arch/powerpc/platforms/pseries/eeh.c
===================================================================
--- linux-2.6.22-git2.orig/arch/powerpc/platforms/pseries/eeh.c	2007-08-09 18:00:09.000000000 -0500
+++ linux-2.6.22-git2/arch/powerpc/platforms/pseries/eeh.c	2007-08-09 18:03:14.000000000 -0500
@@ -955,7 +955,7 @@ static void *early_enable_eeh(struct dev
 	pdn->eeh_freeze_count = 0;
 	pdn->eeh_false_positives = 0;
 
-	if (status && strcmp(status, "ok") != 0)
+	if (status && strncmp(status, "ok", 2) != 0)
 		return NULL;	/* ignore devices with bad status */
 
 	/* Ignore bad nodes. */

[PATCH 2/2] pseries: remove dead eeh video code

[Linas Vepstas]

Remove dead code, and a misleading comment about EEH checking
for video devices.  The removed code is a left-over from the
olden days where there was concern over how video devices 
worked in Linux. We are never going to go that way again, 
so kill this.

Signed-off-by: Linas Vepstas <linas@austin.ibm.com>

----
 arch/powerpc/platforms/pseries/eeh.c |   17 -----------------
 1 file changed, 17 deletions(-)

Index: linux-2.6.22-git2/arch/powerpc/platforms/pseries/eeh.c
===================================================================
--- linux-2.6.22-git2.orig/arch/powerpc/platforms/pseries/eeh.c	2007-08-09 18:03:14.000000000 -0500
+++ linux-2.6.22-git2/arch/powerpc/platforms/pseries/eeh.c	2007-08-09 18:06:45.000000000 -0500
@@ -969,23 +969,6 @@ static void *early_enable_eeh(struct dev
 	}
 	pdn->class_code = *class_code;
 
-	/*
-	 * Now decide if we are going to "Disable" EEH checking
-	 * for this device.  We still run with the EEH hardware active,
-	 * but we won't be checking for ff's.  This means a driver
-	 * could return bad data (very bad!), an interrupt handler could
-	 * hang waiting on status bits that won't change, etc.
-	 * But there are a few cases like display devices that make sense.
-	 */
-	enable = 1;	/* i.e. we will do checking */
-#if 0
-	if ((*class_code >> 16) == PCI_BASE_CLASS_DISPLAY)
-		enable = 0;
-#endif
-
-	if (!enable)
-		pdn->eeh_mode |= EEH_MODE_NOCHECK;
-
 	/* Ok... see if this device supports EEH.  Some do, some don't,
 	 * and the only way to find out is to check each and every one. */
 	regs = of_get_property(dn, "reg", NULL);

Re: [PATCH 1/2] pseries: device node status can be "ok" or "okay"

[Segher Boessenkool]

> It seems that some versions of firmware will report a device
> node status as the string "okay". As we are not expecting this
> string, the device node will be ignored by the EEH subsystem.
> Which means EEH will not be enabled.
>
> When EEH is not enabled, PCI errors will be converted into
> Machine Check exceptions, and we'll have a very unhappy system.

> -	if (status && strcmp(status, "ok") != 0)
> +	if (status && strncmp(status, "ok", 2) != 0)
>  		return NULL;	/* ignore devices with bad status */

Shouldn't you check for the two literal strings, instead of
only matching the common prefix?  Seems safer.


Segher

Re: [PATCH 1/2] pseries: device node status can be "ok" or "okay"

[Matt Sealey]

Segher Boessenkool wrote:
>> It seems that some versions of firmware will report a device
>> node status as the string "okay". As we are not expecting this
>> string, the device node will be ignored by the EEH subsystem.
>> Which means EEH will not be enabled.
>>
>> When EEH is not enabled, PCI errors will be converted into
>> Machine Check exceptions, and we'll have a very unhappy system.
> 
>> -	if (status && strcmp(status, "ok") != 0)
>> +	if (status && strncmp(status, "ok", 2) != 0)
>>  		return NULL;	/* ignore devices with bad status */
> 
> Shouldn't you check for the two literal strings, instead of
> only matching the common prefix?  Seems safer.

What are the chances that the status string is "oklahoma" or
"okeechobee" or "okinawa"?

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Re: [PATCH 1/2] pseries: device node status can be "ok" or "okay"

[Segher Boessenkool]

>>> It seems that some versions of firmware will report a device
>>> node status as the string "okay". As we are not expecting this
>>> string, the device node will be ignored by the EEH subsystem.
>>> Which means EEH will not be enabled.
>>>
>>> When EEH is not enabled, PCI errors will be converted into
>>> Machine Check exceptions, and we'll have a very unhappy system.
>>> -	if (status && strcmp(status, "ok") != 0)
>>> +	if (status && strncmp(status, "ok", 2) != 0)
>>>  		return NULL;	/* ignore devices with bad status */
>> Shouldn't you check for the two literal strings, instead of
>> only matching the common prefix?  Seems safer.
>
> What are the chances that the status string is "oklahoma" or
> "okeechobee" or "okinawa"?

What does it matter what the chances are?  The code is either
correct or not ;-)

Besides, that's only part of the problem -- what if the
property is empty, or contains an empty string only, or
a single non-zero byte?


Segher