Re: [PATCH] EDAC: core EDAC support code

[Dave Peterson <dsp@llnl.gov>]

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
            #

    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

    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

        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

    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.

    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?