[PATCH 3/3] EDAC Add e752x parameter for sysbus_parity selection

[PATCH 3/3] EDAC Add e752x parameter for sysbus_parity selection

[dougthompson]

From:	Peter Tyser <ptyser@xes-inc.com>

This patch adds a module parameter "sysbus_parity" to allow forcing system bus parity error
 checking on or off.  It also adds support to automatically disable system bus parity errors for
 processors which do not support it.

If the sysbus_parity parameter is specified, sysbus parity detection will be forced on or off.  If
 it is not specified, the driver will attempt to look at the CPU identifier string and determine
 if the CPU supports system bus parity.  A blacklist was used instead of a whitelist so that system
 bus parity would be enabled by default and to minimize the chances of breaking things for those
 people already using the driver which for some reason have a processor that does not have a valid
 CPU identifier string.

CC:		Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by:	Peter Tyser <ptyser@xes-inc.com>
Signed-off-by:	Doug Thompson <dougthompson@xmission.com>
---
 e752x_edac.c |   40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

Index: linux-2.6.25-rc5/drivers/edac/e752x_edac.c
===================================================================
--- linux-2.6.25-rc5.orig/drivers/edac/e752x_edac.c
+++ linux-2.6.25-rc5/drivers/edac/e752x_edac.c
@@ -29,6 +29,7 @@
 #define EDAC_MOD_STR	"e752x_edac"
 
 static int force_function_unhide;
+static int sysbus_parity = -1;
 
 static struct edac_pci_ctl_info *e752x_pci;
 
@@ -1049,6 +1050,37 @@ fail:
 	return 1;
 }
 
+/* Setup system bus parity mask register.
+ * Sysbus parity supported on:
+ *   e7320/e7520/e7525 + Xeon
+ *   i3100 + Xeon/Celeron
+ * Sysbus parity not supported on:
+ *   i3100 + Pentium M/Celeron M/Core Duo/Core2 Duo
+ */
+static void e752x_init_sysbus_parity_mask(struct e752x_pvt *pvt)
+{
+	char *cpu_id = cpu_data(0).x86_model_id;
+	struct pci_dev *dev = pvt->dev_d0f1;
+	int enable = 1;
+
+	/* Allow module paramter override, else see if CPU supports parity */
+	if (sysbus_parity != -1) {
+		enable = sysbus_parity;
+	} else if (cpu_id[0] &&
+		   ((strstr(cpu_id, "Pentium") && strstr(cpu_id, " M ")) ||
+		    (strstr(cpu_id, "Celeron") && strstr(cpu_id, " M ")) ||
+		    (strstr(cpu_id, "Core") && strstr(cpu_id, "Duo")))) {
+		e752x_printk(KERN_INFO, "System Bus Parity not "
+			     "supported by CPU, disabling\n");
+		enable = 0;
+	}
+
+	if (enable)
+		pci_write_config_word(dev, E752X_SYSBUS_ERRMASK, 0x0000);
+	else
+		pci_write_config_word(dev, E752X_SYSBUS_ERRMASK, 0x0309);
+}
+
 static void e752x_init_error_reporting_regs(struct e752x_pvt *pvt)
 {
 	struct pci_dev *dev;
@@ -1062,7 +1094,9 @@ static void e752x_init_error_reporting_r
 		pci_write_config_byte(dev, E752X_HI_ERRMASK, 0x00);
 		pci_write_config_byte(dev, E752X_HI_SMICMD, 0x00);
 	}
-	pci_write_config_word(dev, E752X_SYSBUS_ERRMASK, 0x00);
+
+	e752x_init_sysbus_parity_mask(pvt);
+
 	pci_write_config_word(dev, E752X_SYSBUS_SMICMD, 0x00);
 	pci_write_config_byte(dev, E752X_BUF_ERRMASK, 0x00);
 	pci_write_config_byte(dev, E752X_BUF_SMICMD, 0x00);
@@ -1286,3 +1320,7 @@ MODULE_PARM_DESC(force_function_unhide, 
 
 module_param(edac_op_state, int, 0444);
 MODULE_PARM_DESC(edac_op_state, "EDAC Error Reporting state: 0=Poll,1=NMI");
+
+module_param(sysbus_parity, int, 0444);
+MODULE_PARM_DESC(sysbus_parity, "0=disable system bus parity checking,"
+                " 1=enable system bus parity checking, default=auto-detect");

Re: [PATCH 3/3] EDAC Add e752x parameter for sysbus_parity selection

[Andrew Morton]

On Wed, 19 Mar 2008 14:55:59 -0600
dougthompson@xmission.com wrote:

> +/* Setup system bus parity mask register.
> + * Sysbus parity supported on:
> + *   e7320/e7520/e7525 + Xeon
> + *   i3100 + Xeon/Celeron
> + * Sysbus parity not supported on:
> + *   i3100 + Pentium M/Celeron M/Core Duo/Core2 Duo
> + */
> +static void e752x_init_sysbus_parity_mask(struct e752x_pvt *pvt)
> +{
> +	char *cpu_id = cpu_data(0).x86_model_id;
> +	struct pci_dev *dev = pvt->dev_d0f1;
> +	int enable = 1;
> +
> +	/* Allow module paramter override, else see if CPU supports parity */
> +	if (sysbus_parity != -1) {
> +		enable = sysbus_parity;
> +	} else if (cpu_id[0] &&
> +		   ((strstr(cpu_id, "Pentium") && strstr(cpu_id, " M ")) ||
> +		    (strstr(cpu_id, "Celeron") && strstr(cpu_id, " M ")) ||
> +		    (strstr(cpu_id, "Core") && strstr(cpu_id, "Duo")))) {
> +		e752x_printk(KERN_INFO, "System Bus Parity not "
> +			     "supported by CPU, disabling\n");
> +		enable = 0;
> +	}
> +
> +	if (enable)
> +		pci_write_config_word(dev, E752X_SYSBUS_ERRMASK, 0x0000);
> +	else
> +		pci_write_config_word(dev, E752X_SYSBUS_ERRMASK, 0x0309);
> +}

Is that the best way of working out whether the CPU supports system bus
parity?  We do have cpu capability infrastructure in x86 core and I'd have
though it would be better for x86 core to work this out, set the suitable
flag and have clients (ie: EDAC) test that flag?

Re: [PATCH 3/3] EDAC Add e752x parameter for sysbus_parity selection

[Doug Thompson]

Peter Tyser,

can you look into this, as it is your requirement, for a possible better solution with those on
the CC list? 

Thanks

Thanks Andrew for the notice.

doug t



--- Andrew Morton <akpm@linux-foundation.org> wrote:

> On Wed, 19 Mar 2008 14:55:59 -0600
> dougthompson@xmission.com wrote:
> 
> > +/* Setup system bus parity mask register.
> > + * Sysbus parity supported on:
> > + *   e7320/e7520/e7525 + Xeon
> > + *   i3100 + Xeon/Celeron
> > + * Sysbus parity not supported on:
> > + *   i3100 + Pentium M/Celeron M/Core Duo/Core2 Duo
> > + */
> > +static void e752x_init_sysbus_parity_mask(struct e752x_pvt *pvt)
> > +{
> > +	char *cpu_id = cpu_data(0).x86_model_id;
> > +	struct pci_dev *dev = pvt->dev_d0f1;
> > +	int enable = 1;
> > +
> > +	/* Allow module paramter override, else see if CPU supports parity */
> > +	if (sysbus_parity != -1) {
> > +		enable = sysbus_parity;
> > +	} else if (cpu_id[0] &&
> > +		   ((strstr(cpu_id, "Pentium") && strstr(cpu_id, " M ")) ||
> > +		    (strstr(cpu_id, "Celeron") && strstr(cpu_id, " M ")) ||
> > +		    (strstr(cpu_id, "Core") && strstr(cpu_id, "Duo")))) {
> > +		e752x_printk(KERN_INFO, "System Bus Parity not "
> > +			     "supported by CPU, disabling\n");
> > +		enable = 0;
> > +	}
> > +
> > +	if (enable)
> > +		pci_write_config_word(dev, E752X_SYSBUS_ERRMASK, 0x0000);
> > +	else
> > +		pci_write_config_word(dev, E752X_SYSBUS_ERRMASK, 0x0309);
> > +}
> 
> Is that the best way of working out whether the CPU supports system bus
> parity?  We do have cpu capability infrastructure in x86 core and I'd have
> thought it would be better for x86 core to work this out, set the suitable
> flag and have clients (ie: EDAC) test that flag?
> 
> 


W1DUG

Re: [PATCH 3/3] EDAC Add e752x parameter for sysbus_parity selection

[Peter Tyser]

> > > +
> > > +	/* Allow module paramter override, else see if CPU supports parity */
> > > +	if (sysbus_parity != -1) {
> > > +		enable = sysbus_parity;
> > > +	} else if (cpu_id[0] &&
> > > +		   ((strstr(cpu_id, "Pentium") && strstr(cpu_id, " M ")) ||
> > > +		    (strstr(cpu_id, "Celeron") && strstr(cpu_id, " M ")) ||
> > > +		    (strstr(cpu_id, "Core") && strstr(cpu_id, "Duo")))) {
> > > +		e752x_printk(KERN_INFO, "System Bus Parity not "
> > > +			     "supported by CPU, disabling\n");
> > > +		enable = 0;
> > > +	}
> > 
> > Is that the best way of working out whether the CPU supports system bus
> > parity?  We do have cpu capability infrastructure in x86 core and I'd have
> > thought it would be better for x86 core to work this out, set the suitable
> > flag and have clients (ie: EDAC) test that flag?
> > 

The above implementation was the only way I could think of to
dynamically determine if hardware supported system bus parity at
runtime.  I agree that adding a new x86 cpu capability flag for system
bus parity support would be an ideal way to support this feature for use
by subsystems such as EDAC.  However, there are a whole lot of possible
x86 CPU/Northbridge combinations and I could think of no clean way to
easily determine which of those combinations do or don't support system
bus parity.  The logic needed to compare the incredibly large number of
CPU and Northbridge combinations seems rather daunting...

The current patch was manageable in that only a few CPU/Northbridge
combinations needed to be accounted for and a module parameter would
allow for a manual override if necessary.

If others have suggestions as far as a more generic or generally cleaner
approach, I'd be happy to implement and test.