RE: Problems with EDAC coexisting with BIOS

Gross, Mark
2006-04-24T15:58:04Z

[-- Attachment #1: Type: text/plain, Size: 3595 bytes --]

>-----Original Message-----
>From: Alan Cox [mailto:alan@lxorguk.ukuu.org.uk]
>Sent: Monday, April 24, 2006 6:19 AM
>To: Gross, Mark
>Cc: bluesmoke-devel@lists.sourceforge.net; LKML; Carbonari, Steven;
Ong,
>Soo Keong; Wang, Zhenyu Z
>Subject: Re: Problems with EDAC coexisting with BIOS
>
>On Gwe, 2006-04-21 at 09:01 -0700, Gross, Mark wrote:
>> 1) The default AMI BIOS behavior on SMI is to check the chipset error
>> registers (Dev0:Fun1) and re-hide them.
>
>The words "bad design" come to mind (followed by a large number of more
>accurate phrases that are inappropriate for a public list)
>

Yes, EDAC should not have existed without first working more closely
with the BIOS folks. It's too bad we (Intel) didn't catch this. The
BIOS folks where not in the loop with the driver folks making
contributions to EDAC previously.

>> Basically if device 0 : function 1 is hidden by the platform at boot
>> time un-hiding and using the device and function is a risky thing to
do,
>
>Intel provided patches that do exactly this for some of the chip
>workarounds. Are you saying the Intel chip work around also needs
>fixing ?
>

I think what I'm saying is pretty clear and I don't think it is related
to whatever workarounds where done earlier.

>> The driver should never get loaded by default or automatically. If
the
>> user knows enough about there BIOS to trust that the SMI behavior
will
>> coexist with the driver then its OK to load otherwise using this
driver
>> is not a safe thing to do.
>
>So Intel and/or the BIOS vendors also forgot to put in any kind of
>indicator ? How do they expect end users to know this, or OS vendors ?
>Is there a technote that covers this mess ?
>

I don't know of any technote. It took me working with Soo Keong for a
few weeks to chase this issue down to the level I have. The short
answer is that the BIOS assumes the payload OS would not be fighting it
for hidden device access and the EDAC driver violates this assumption.

>> I think the best thing to do is to have the driver error out in its
init
>> or probe code if the dev0:fun1 is hidden at boot time.
>>
>> Comments?
>
>Why did Intel bother implementing this functionality and then screwing
>it up so that OS vendors can't use it ? It seems so bogus.
>

It was just a screw up not to have identified this issue sooner.

>At the very least we should print a warning advising the user that the
>BIOS is incompatible and to ask the BIOS vendor for an update so that
>they can enable error detection and management support.

I would place the warning in the probe or init code.

Attached is a test patch I'm testing now. I don't like it, but it seems
to be working so far. It basically fails the probe call leaving the
driver loaded. I'm going to move the test to e752x_int so the driver
fails at init this am at restart my tests.

>
>Is only the AMI BIOS this braindamaged, should we just blacklist AMI
>bioses in EDAC or is this shared Intel supplied code that may be found
>in other vendors systems.
>

Unknown. Also the BOIS teams for various platforms can modify the base
AMI functionality. I know that at least one Intel e7520 based system
with AMI based bios seems to not expose this issue. The point is that
without working out a handshake between the OS and the platform / BIOS
for this type of thing, loading EDAC without a patch like mine is
equivalent to playing Russian roulette. You can't know which platform /
bios will blow up on you if you load the thing.

--mgross

[-- Attachment #2: edac_2_6_16.patch --]
[-- Type: application/octet-stream, Size: 1065 bytes --]

? scripts/kconfig/lxdialog/lxdialog
Index: drivers/edac/e752x_edac.c
===================================================================
RCS file: /opt/CVS/linux/drivers/edac/e752x_edac.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 e752x_edac.c
--- drivers/edac/e752x_edac.c 21 Apr 2006 02:58:32 -0000 1.1.1.1
+++ drivers/edac/e752x_edac.c 23 Apr 2006 23:42:19 -0000
@@ -755,10 +755,13 @@
debugf0("MC: " __FILE__ ": %s(): mci\n", __func__);
debugf0("Starting Probe1\n");

- /* enable device 0 function 1 */
+ /* check to see if device 0 function 1 is enbaled if it isn't we assume
+ * the BIOS has reserved it for a reason like, it has SMI's setup
+ * expecting exclusive access, and we sould take care to not violate
+ * that assumption and fail the probe. */
pci_read_config_byte(pdev, E752X_DEVPRES1, &stat8);
- stat8 |= (1 << 5);
- pci_write_config_byte(pdev, E752X_DEVPRES1, stat8);
+ if (!(stat8 & (1 << 5))
+ goto fail;

/* need to find out the number of channels */
pci_read_config_dword(pdev, E752X_DRC, &drc);