Re: [PATCH] edac: Add a sysfs node to test the EDAC error report facility

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Em 13-03-2012 20:31, Greg KH escreveu:
> On Wed, Mar 07, 2012 at 09:08:21AM -0300, Mauro Carvalho Chehab wrote:
>> Even on memory controllers that have memory injection, such functionality
>> can be disabled by BIOS during bootstrap, and it may not be possible to
>> enable it via BIOS setup.
>>
>> So, as not all hardware supports error injection, add a mechanism to
>> allow testing the edac driver and the core.
>>
>> This feature is only enabled when EDAC_DEBUG is equal to Y, so there's no
>> extra code for the production Kernels.
> 
> Then please move it to debugfs, where debugging stuff belongs.

There are other edac error injection tools at sysfs. That's why I opted to keep
it there - plus, I was too lazy[1] to learn about the debugfs bits at the time I
wrote it, and adding a single node to sysfs were very trivial ;)

[1] In a matter of fact, testing this patchset is too time-consuming,
as it required me to get access to several systems, including some old ones.
This patch in particular were written in order to help me with the tests.
While I intend to improve it, this has lower priority than the tests themselves,
the discussions about the patches/ABI and the media maintainer stuff.

> 
> At the least, you need to document all sysfs files in Documentation/ABI/

Yes, this is on my TODO list. I'll document it at the Documentation on the
final patchset.

Btw, the current EDAC sysfs API is not documented there at ABI/, but,
instead, at Documentation/edac.txt. What is there is basically the sysfs node
description, but it is more verbose than what is there at Documentation/ABI.

I'm not sure what would be the better to do there: to move everything into
Documentation/ABI, to keep edac.txt as is (of course, adding there the new
nodes) or to keep a simplified edac.txt file and add a more concise
description at Documentation/ABI.

While there's current no plans to remove the existing EDAC sysfs API, when
writing the documentation patch, it is probably a good idea to put the
nodes that provide duplicated information at Documentation/ABI/obsolete.

> 
>>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>> ---
>>
>> NOTE: This patch should be applied after the series that add proper support for
>> FB-DIMM, due to context changes.
>>
>>  drivers/edac/edac_mc_sysfs.c |   62 ++++++++++++++++++++++++++++++++++++++++-
>>  include/linux/edac.h         |    4 +++
>>  2 files changed, 64 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
>> index 870ccb0..f538f9e 100644
>> --- a/drivers/edac/edac_mc_sysfs.c
>> +++ b/drivers/edac/edac_mc_sysfs.c
>> @@ -744,6 +744,42 @@ static ssize_t mci_size_mb_show(struct mem_ctl_info *mci, char *data,
>>  	return sprintf(data, "%u\n", PAGES_TO_MiB(total_pages));
>>  }
>>  
>> +#ifdef CONFIG_EDAC_DEBUG
>> +static ssize_t edac_fake_inject_show(struct mem_ctl_info *mci,
>> +				     char *data, void *priv)
>> +{
>> +	return sprintf(data,
>> +		       "EDAC fake test engine. Writing to this node a value in the form of :\n"
>> +		       "\t0:1:0\n"
>> +		       "will call the EDAC core routine to produce a memory error for the given memory location (0, 1, 0).\n"
>> +		       "The driver's error parsing logic won't be tested. This tool is useful only\n"
>> +		       "if you're testing the EDAC core tracing facility, or if you're needing to test\n"
>> +		       "some userspace application.\n");
>> +}
> 
> Ick, no NEVER do this for a sysfs file, even for "debugging", that's
> what debugfs is for, please move this file there, this isn't ok.
> 
> Again, sysfs is "one value per file".

Ok. I won't add this patch on my linux-next tree after finishing the tests. I'll keep
it on my todo list and re-write it later, when I find some time to convert it into
debugfs.

> 
> thanks,
> 
> greg k-h