Re: [PATCH] EDAC: core EDAC support code

Greg KH
2006-03-10T00:02:42Z

On Thu, Mar 09, 2006 at 03:51:25PM -0800, Dave Peterson wrote:
> On Tuesday 07 March 2006 11:03, Arjan van de Ven wrote:
> > afaics it is a list of pci devices. these should just be symlinks to the
> > sysfs resource of these pci devices instead, not a flat table file.
>
> Ok, I'm looking at the EDAC sysfs interface. I see the following
> issues concerning the "one value per file" rule:
>
> 1. /sys/devices/system/edac/mc/mc0/module_name contains two
> values, a module name and a version:
>
> # cat /sys/devices/system/edac/mc/mc0/module_name
> k8_edac Ver: 2.0.1.devel Mar 8 2006

Woah. That's what /sys/modules/ is for right? Don't add new stuff
please.

> 2. /sys/devices/system/edac/mc/mc0/supported_mem_type contains
> the following on the machine I am looking at:
>
> # cat /sys/devices/system/edac/mc/mc0/supported_mem_type
> Unbuffered-DDR Registered-DDR
> #
>
> Here we have a whitespace-delimited list of values. Likewise,
> the following files contain whitespace-delimited lists:
>
> /sys/devices/system/edac/mc/mc0/edac_capability
> /sys/devices/system/edac/mc/mc0/edac_current_capability

What exactly do they look like?

> 3. The following files contain comma-delimited lists of
> (vendor ID, device ID) tuples:
>
> /sys/devices/system/edac/pci/pci_parity_blacklist
> /sys/devices/system/edac/pci/pci_parity_whitelist

What exactly do they look like?

> I assume this is what Arjan is referring to.
> Documentation/drivers/edac/edac.txt gives the following
> description of how the whitelist functions:
>
> This control file allows for an explicit list of PCI
> devices to be scanned for parity errors. Only devices
> found on this list will be examined. The list is a line
> of hexadecimel VENDOR and DEVICE ID tuples:
>
> 1022:7450,1434:16a6
>
> One or more can be inserted, seperated by a comma.
> To write the above list doing the following as one
> command line:
>
> echo "1022:7450,1434:16a6"
> > /sys/devices/system/edac/pci/pci_parity_whitelist
>
> To display what the whitelist is, simply 'cat' the same
> file.
>
> Looking at the current EDAC implementation, these are all of the
> "one value per file" issues I see. If anyone sees any others I
> missed, please let me know. Here are my thoughts on each:
>
> Issue #1
> --------
> Fixing this is easy. /sys/devices/system/edac/mc/mc0/module_name
> can be replaced by two separate files, one providing the name and
> the other providing the version:
>
> /sys/devices/system/edac/mc/mc0/module_name
> /sys/devices/system/edac/mc/mc0/module_version

No, these should just be deleted. Use the proper MODULE_* macros for
these if you really want to display them to users.

> Issue #2
> --------
> To fix this, /sys/devices/system/edac/mc/mc0/supported_mem_type
> can be made into a directory containing a file representing each
> supported memory type. Thus we might have the following:
>
> /sys/devices/system/edac/mc/mc0/supported_mem_type
> /sys/devices/system/edac/mc/mc0/supported_mem_type/Unbuffered-DDR
> /sys/devices/system/edac/mc/mc0/supported_mem_type/Registered-DDR
>
> In the above example, the files Unbuffered-DDR and Registered-DDR
> would each be empty in content. The presence of each file would
> indicate that the memory type it represents is supported.

I don't think the original file is really a big problem.

> Issue #3
> --------
> I am unclear about what to do here. If the list contents were
> read-only, it would be relatively easy to make
> /sys/devices/system/edac/pci/pci_parity_whitelist into a directory
> containing symlinks, one for each device. However, the user is
> supposed to be able to modify the list contents. This would imply
> that the user creates and destroys symlinks. Does sysfs currently
> support this sort of behavior? If not, what is the preferred
> means for implementing a user-modifiable set of values?

No it doesn't. How big can this list get?

thanks,

greg k-h