[PATCH] Fix EDAC sdram_scrub_rate read failure

[PATCH] Fix EDAC sdram_scrub_rate read failure

[Han Pingtian]

If sdram scrubbing rate ins't implemented on current system, read it
will cause an error:

cat: /sys/devices/system/edac/mc//mc0/sdram_scrub_rate: Invalid argument

The eba042a81edd6baaff44831b2d719b14a6d21e58 let it returning -EINVAL,
but according to the document, it should show -1:

dram memory scrubbing rate:

        'sdram_scrub_rate'

        Read/Write attribute file that controls memory scrubbing. The scrubbing
        rate is set by writing a minimum bandwidth in bytes/sec to the attribute
        file. The rate will be translated to an internal value that gives at
        least the specified rate.

        Reading the file will return the actual scrubbing rate employed.

        If configuration fails or memory scrubbing is not implemented, the value
        of the attribute file will be -1.

Signed-off-by: Han Pingtian <phan@redhat.com>
---
 drivers/edac/edac_mc_sysfs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 29ffa35..602fdcf 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -475,7 +475,7 @@ static ssize_t mci_sdram_scrub_rate_show(struct mem_ctl_info *mci, char *data)
 	int bandwidth = 0;
 
 	if (!mci->get_sdram_scrub_rate)
-		return -EINVAL;
+		return sprintf(date, "%d\n", -1);
 
 	bandwidth = mci->get_sdram_scrub_rate(mci);
 	if (bandwidth < 0) {
-- 
1.7.1

Re: [PATCH] Fix EDAC sdram_scrub_rate read failure

[Borislav Petkov]

On Fri, Sep 16, 2011 at 06:58:56PM +0800, Han Pingtian wrote:
> If sdram scrubbing rate ins't implemented on current system, read it
> will cause an error:
> 
> cat: /sys/devices/system/edac/mc//mc0/sdram_scrub_rate: Invalid argument
> 
> The eba042a81edd6baaff44831b2d719b14a6d21e58 let it returning -EINVAL,

Can you please add the patch author to CC next time, out of courtesy?
Thanks.

> but according to the document, it should show -1:
> 
> dram memory scrubbing rate:
> 
>         'sdram_scrub_rate'
> 
>         Read/Write attribute file that controls memory scrubbing. The scrubbing
>         rate is set by writing a minimum bandwidth in bytes/sec to the attribute
>         file. The rate will be translated to an internal value that gives at
>         least the specified rate.
> 
>         Reading the file will return the actual scrubbing rate employed.
> 
>         If configuration fails or memory scrubbing is not implemented, the value
>         of the attribute file will be -1.
> 
> Signed-off-by: Han Pingtian <phan@redhat.com>
> ---
>  drivers/edac/edac_mc_sysfs.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index 29ffa35..602fdcf 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -475,7 +475,7 @@ static ssize_t mci_sdram_scrub_rate_show(struct mem_ctl_info *mci, char *data)
>  	int bandwidth = 0;
>  
>  	if (!mci->get_sdram_scrub_rate)
> -		return -EINVAL;
> +		return sprintf(date, "%d\n", -1);

Have you even build-tested your patch:

drivers/edac/edac_mc_sysfs.c: In function ‘mci_sdram_scrub_rate_show’:
drivers/edac/edac_mc_sysfs.c:478: error: ‘date’ undeclared (first use in this function)
drivers/edac/edac_mc_sysfs.c:478: error: (Each undeclared identifier is reported only once
drivers/edac/edac_mc_sysfs.c:478: error: for each function it appears in.)
make[2]: *** [drivers/edac/edac_mc_sysfs.o] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [drivers/edac] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [drivers] Error 2
make: *** Waiting for unfinished jobs....

>  
>  	bandwidth = mci->get_sdram_scrub_rate(mci);
>  	if (bandwidth < 0) {

Anyway, the EDAC document says that sdram_scrub_rate is
read/write and when writing you have to write -1 too. Care to fix
mci_sdram_scrub_rate_store too, test your patch properly and resend?

Thanks.

-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

Re: [PATCH v2] Fix EDAC sdram_scrub_rate read/write failure

[Han Pingtian]

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

On Fri, Sep 16, 2011 at 02:53:27PM +0200, Borislav Petkov wrote:
> On Fri, Sep 16, 2011 at 06:58:56PM +0800, Han Pingtian wrote:
> > If sdram scrubbing rate ins't implemented on current system, read it
> > will cause an error:
> > 
> > cat: /sys/devices/system/edac/mc//mc0/sdram_scrub_rate: Invalid argument
> > 
> > The eba042a81edd6baaff44831b2d719b14a6d21e58 let it returning -EINVAL,
> 
> Can you please add the patch author to CC next time, out of courtesy?
Will do next time. Thanks.
> Thanks.
> 
> > but according to the document, it should show -1:
> > 
> > dram memory scrubbing rate:
> > 
> >         'sdram_scrub_rate'
> > 
> >         Read/Write attribute file that controls memory scrubbing. The scrubbing
> >         rate is set by writing a minimum bandwidth in bytes/sec to the attribute
> >         file. The rate will be translated to an internal value that gives at
> >         least the specified rate.
> > 
> >         Reading the file will return the actual scrubbing rate employed.
> > 
> >         If configuration fails or memory scrubbing is not implemented, the value
> >         of the attribute file will be -1.
> > 
> > Signed-off-by: Han Pingtian <phan@redhat.com>
> > ---
> >  drivers/edac/edac_mc_sysfs.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> > index 29ffa35..602fdcf 100644
> > --- a/drivers/edac/edac_mc_sysfs.c
> > +++ b/drivers/edac/edac_mc_sysfs.c
> > @@ -475,7 +475,7 @@ static ssize_t mci_sdram_scrub_rate_show(struct mem_ctl_info *mci, char *data)
> >  	int bandwidth = 0;
> >  
> >  	if (!mci->get_sdram_scrub_rate)
> > -		return -EINVAL;
> > +		return sprintf(date, "%d\n", -1);
> 
> Have you even build-tested your patch:
> 
> drivers/edac/edac_mc_sysfs.c: In function ‘mci_sdram_scrub_rate_show’:
> drivers/edac/edac_mc_sysfs.c:478: error: ‘date’ undeclared (first use in this function)
> drivers/edac/edac_mc_sysfs.c:478: error: (Each undeclared identifier is reported only once
> drivers/edac/edac_mc_sysfs.c:478: error: for each function it appears in.)
> make[2]: *** [drivers/edac/edac_mc_sysfs.o] Error 1
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [drivers/edac] Error 2
> make[1]: *** Waiting for unfinished jobs....
> make: *** [drivers] Error 2
> make: *** Waiting for unfinished jobs....
> 
> >  
> >  	bandwidth = mci->get_sdram_scrub_rate(mci);
> >  	if (bandwidth < 0) {
> 
> Anyway, the EDAC document says that sdram_scrub_rate is
> read/write and when writing you have to write -1 too. Care to fix
Cannot figure out how to write -1 here. Since the document just says the
value will be -1 when configuration fails or memory scrubbing is not
implemented, I think we can do nothing when setting the vaule if
scrubbing not implemented and just make sure showing will return -1.
> mci_sdram_scrub_rate_store too, test your patch properly and resend?

Thanks your review. I have tested the new patch attached. Please have a
look. 

Thanks.
-- 
Han Pingtian
Quality Engineer
hpt @ #kernel-qe
Red Hat, Inc
Freedom ... courage ... Commitment ... ACCOUNTABILITY

[-- Attachment #2: 0001-Fix-EDAC-sdram_scrub_rate-read-write-failure.patch --]
[-- Type: text/plain, Size: 2397 bytes --]

>From a77c8db0797f8c6b3e1ac30f8da0b7f2d9c10dbe Mon Sep 17 00:00:00 2001
From: Han Pingtian <phan@redhat.com>
Date: Fri, 16 Sep 2011 18:43:20 +0800
Subject: [PATCH] Fix EDAC sdram_scrub_rate read/write failure

If sdram scrubbing rate isn't implemented on current system, read/write it
will cause an error:

]# cat /sys/devices/system/edac/mc/mc0/sdram_scrub_rate
cat: /sys/devices/system/edac/mc/mc0/sdram_scrub_rate: Invalid argument

]# echo 1 > /sys/devices/system/edac/mc/mc0/sdram_scrub_rate
-bash: echo: write error: Invalid argument

The eba042a81edd6baaff44831b2d719b14a6d21e58 let it returning -EINVAL,
but according to the document, the value should be -1, so there
shouldn't be a failure.

Sdram memory scrubbing rate:

        'sdram_scrub_rate'

        Read/Write attribute file that controls memory scrubbing. The scrubbing
        rate is set by writing a minimum bandwidth in bytes/sec to the attribute
        file. The rate will be translated to an internal value that gives at
        least the specified rate.

        Reading the file will return the actual scrubbing rate employed.

        If configuration fails or memory scrubbing is not implemented, the value
        of the attribute file will be -1.

Signed-off-by: Han Pingtian <phan@redhat.com>
---
 drivers/edac/edac_mc_sysfs.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 29ffa35..dae14cf 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -451,8 +451,11 @@ static ssize_t mci_sdram_scrub_rate_store(struct mem_ctl_info *mci,
 	unsigned long bandwidth = 0;
 	int new_bw = 0;
 
-	if (!mci->set_sdram_scrub_rate)
-		return -EINVAL;
+	if (!mci->set_sdram_scrub_rate) {
+		edac_printk(KERN_WARNING, EDAC_MC,
+			"Memory scrub rate writing is not implemented\n");
+		return count;
+	}
 
 	if (strict_strtoul(data, 10, &bandwidth) < 0)
 		return -EINVAL;
@@ -474,8 +477,11 @@ static ssize_t mci_sdram_scrub_rate_show(struct mem_ctl_info *mci, char *data)
 {
 	int bandwidth = 0;
 
-	if (!mci->get_sdram_scrub_rate)
-		return -EINVAL;
+	if (!mci->get_sdram_scrub_rate) {
+		edac_printk(KERN_WARNING, EDAC_MC,
+			"Memory scrub rate reading not implemented\n");
+		return sprintf(data, "%d\n", -1);
+	}
 
 	bandwidth = mci->get_sdram_scrub_rate(mci);
 	if (bandwidth < 0) {
-- 
1.7.1

Re: [PATCH v2] Fix EDAC sdram_scrub_rate read/write failure

[Borislav Petkov]

On Mon, Sep 19, 2011 at 03:37:22AM -0400, Han Pingtian wrote:
> > Anyway, the EDAC document says that sdram_scrub_rate is
> > read/write and when writing you have to write -1 too. Care to fix
> Cannot figure out how to write -1 here. Since the document just says the
> value will be -1 when configuration fails or memory scrubbing is not
> implemented, I think we can do nothing when setting the vaule if
> scrubbing not implemented and just make sure showing will return -1.

This is a good indication of a bug in the API - I can't imagine why
would it make sense to write a negative value to a sysfs file just to
show that scrubbing interface support is not present. More likely the
OS should fail the write with -ENODEV or something saying that the
functionality is not there.

Which is much easier to interpret from userspace: if the write failed
you know something is wrong. If it succeeded OTOH, you still have to go
and

cat /sys/devices/system/edac/mc/mc<N>/sdram_scrub_rate

to make sure it really succeeded. Makes no sense.

So, I'm actually inclined to change the API here instead of adjusting
the code to adhere to it.

+ Tony.

@Tony, do you think the above makes sense? Anything against changing
that part of the EDAC API to say:

Sdram memory scrubbing rate:

        'sdram_scrub_rate'

        Read/Write attribute file that controls memory scrubbing. The scrubbing
        rate is set by writing a minimum bandwidth in bytes/sec to the attribute
        file. The rate will be translated to an internal value that gives at
        least the specified rate.

        Reading the file will return the actual scrubbing rate employed.

        If configuration fails or memory scrubbing is not implemented,
	the access to that attribute from userspace will fail.

?

Leaving the below for more background info.

Thanks.

> > mci_sdram_scrub_rate_store too, test your patch properly and resend?
> 
> Thanks your review. I have tested the new patch attached. Please have a
> look. 
> 
> Thanks.
> -- 
> Han Pingtian
> Quality Engineer
> hpt @ #kernel-qe
> Red Hat, Inc
> Freedom ... courage ... Commitment ... ACCOUNTABILITY

> From a77c8db0797f8c6b3e1ac30f8da0b7f2d9c10dbe Mon Sep 17 00:00:00 2001
> From: Han Pingtian <phan@redhat.com>
> Date: Fri, 16 Sep 2011 18:43:20 +0800
> Subject: [PATCH] Fix EDAC sdram_scrub_rate read/write failure
> 
> If sdram scrubbing rate isn't implemented on current system, read/write it
> will cause an error:
> 
> ]# cat /sys/devices/system/edac/mc/mc0/sdram_scrub_rate
> cat: /sys/devices/system/edac/mc/mc0/sdram_scrub_rate: Invalid argument
> 
> ]# echo 1 > /sys/devices/system/edac/mc/mc0/sdram_scrub_rate
> -bash: echo: write error: Invalid argument
> 
> The eba042a81edd6baaff44831b2d719b14a6d21e58 let it returning -EINVAL,
> but according to the document, the value should be -1, so there
> shouldn't be a failure.
> 
> Sdram memory scrubbing rate:
> 
>         'sdram_scrub_rate'
> 
>         Read/Write attribute file that controls memory scrubbing. The scrubbing
>         rate is set by writing a minimum bandwidth in bytes/sec to the attribute
>         file. The rate will be translated to an internal value that gives at
>         least the specified rate.
> 
>         Reading the file will return the actual scrubbing rate employed.
> 
>         If configuration fails or memory scrubbing is not implemented, the value
>         of the attribute file will be -1.
> 
> Signed-off-by: Han Pingtian <phan@redhat.com>
> ---
>  drivers/edac/edac_mc_sysfs.c |   14 ++++++++++----
>  1 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index 29ffa35..dae14cf 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -451,8 +451,11 @@ static ssize_t mci_sdram_scrub_rate_store(struct mem_ctl_info *mci,
>  	unsigned long bandwidth = 0;
>  	int new_bw = 0;
>  
> -	if (!mci->set_sdram_scrub_rate)
> -		return -EINVAL;
> +	if (!mci->set_sdram_scrub_rate) {
> +		edac_printk(KERN_WARNING, EDAC_MC,
> +			"Memory scrub rate writing is not implemented\n");
> +		return count;
> +	}
>  
>  	if (strict_strtoul(data, 10, &bandwidth) < 0)
>  		return -EINVAL;
> @@ -474,8 +477,11 @@ static ssize_t mci_sdram_scrub_rate_show(struct mem_ctl_info *mci, char *data)
>  {
>  	int bandwidth = 0;
>  
> -	if (!mci->get_sdram_scrub_rate)
> -		return -EINVAL;
> +	if (!mci->get_sdram_scrub_rate) {
> +		edac_printk(KERN_WARNING, EDAC_MC,
> +			"Memory scrub rate reading not implemented\n");
> +		return sprintf(data, "%d\n", -1);
> +	}
>  
>  	bandwidth = mci->get_sdram_scrub_rate(mci);
>  	if (bandwidth < 0) {
> -- 
> 1.7.1
> 


-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

RE: [PATCH v2] Fix EDAC sdram_scrub_rate read/write failure

[Luck, Tony]

> @Tony, do you think the above makes sense? Anything against changing
> that part of the EDAC API to say:

> Sdram memory scrubbing rate:

>     If configuration fails or memory scrubbing is not implemented,
>	the access to that attribute from userspace will fail.

Ideally we'd not have the file there at all if memory scrubbing isn't
implemented.

I like having the write fail if the driver cannot set the chosen
scrub rate ... though there seems to be some wiggle room ... scrub
rates aren't exact, the user can try to set one and then read back
to find out what the rate was actually rounded to. If you have to
read back anyway, then reading back to find out if you failed doesn't
look quite so odd (still odd, but with some sort of logic).

-Tony

Re: [PATCH v2] Fix EDAC sdram_scrub_rate read/write failure

[Borislav Petkov]

Adding some more EDAC people.

On Mon, Sep 19, 2011 at 04:23:35PM -0400, Luck, Tony wrote:
> > @Tony, do you think the above makes sense? Anything against changing
> > that part of the EDAC API to say:
> 
> > Sdram memory scrubbing rate:
> 
> >     If configuration fails or memory scrubbing is not implemented,
> >	the access to that attribute from userspace will fail.
> 
> Ideally we'd not have the file there at all if memory scrubbing isn't
> implemented.

Haa, it'll be great if we could do that. But it doesn't look simple
- the whole sysfs thing comes up along the edac_mc_alloc path and
the ->[gs]et_sdram_scrub_rate func ptrs get assigned after the sysfs
hierarchy is up.

Which means, in order to fix that, we have to change it to support
dynamic sysfs assignment and change a bunch of edac drivers (probably
all, at least the couple I looked at will have to). Which is not a
very big deal but testing them would be a much more formidable task
especially if one doesn't have the hardware to test it on.

> I like having the write fail if the driver cannot set the chosen
> scrub rate ... though there seems to be some wiggle room ... scrub
> rates aren't exact, the user can try to set one and then read back
> to find out what the rate was actually rounded to. If you have to
> read back anyway, then reading back to find out if you failed doesn't
> look quite so odd (still odd, but with some sort of logic).

Agreed, I still think "fail the write" is much more intuitive than what
we have now. And I think we should change it if we're not breaking a
bunch of userspace (which I seriously doubt). Originally I thought
edac-utils might use the scrub rate thing but grepping through v0.16
sources doesn't say anything about scrub rate.

So how about the following - people should scream now if they see
problem with it and if we're breaking their userspace stuff (they should
fix them anyway though :-)

If scrub rate setting is not implemented, we say "No such device." and
otherwise on error we say "Invalid argument" which should be clear,
IMHO.

--
From: Borislav Petkov <borislav.petkov@amd.com>
Date: Wed, 21 Sep 2011 14:10:43 +0200
Subject: [PATCH] EDAC: Correct scrub rate API

The original scrub rate API definition states that if scrub rate
accessors are not implemented, a negative value (-1) should be written
to the sysfs file (/sys/devices/system/edac/mc/mc<N>/sdram_scrub_rate,
where N is the memory controller number on the system). This is
counter-intuitive and awkward at the very least because, when setting
the scrub rate, userspace has to write to sysfs and then read it back to
check error status of the operation.

As Tony notes, best it would be to not have the sdram_scrub_rate in
sysfs if scrub rate support is not implemented. It is too late about
that and a bunch of drivers on a bunch of arches would need to be
changed and tested which is not a trivial task ATM.

Instead, settle for the next best thing of returning -ENODEV when
implementation is missing and -EINVAL when there was an error
encountered while setting the scrub rate.

Reported-by: Han Pingtian <phan@redhat.com>
Cc: Tony Luck <tony.luck@intel.com>
Link: http://lkml.kernel.org/r/20110916105856.GA13253@hpt.nay.redhat.com
Signed-off-by: Borislav Petkov <borislav.petkov@amd.com>
---
 Documentation/edac.txt       |    4 ++--
 drivers/edac/edac_mc_sysfs.c |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/edac.txt b/Documentation/edac.txt
index 249822c..fdcc49f 100644
--- a/Documentation/edac.txt
+++ b/Documentation/edac.txt
@@ -334,8 +334,8 @@ Sdram memory scrubbing rate:
 
 	Reading the file will return the actual scrubbing rate employed.
 
-	If configuration fails or memory scrubbing is not implemented, the value
-	of the attribute file will be -1.
+	If configuration fails or memory scrubbing is not implemented, accessing
+	that attribute will fail.
 
 
 
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 29ffa35..deb91fd 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -452,7 +452,7 @@ static ssize_t mci_sdram_scrub_rate_store(struct mem_ctl_info *mci,
 	int new_bw = 0;
 
 	if (!mci->set_sdram_scrub_rate)
-		return -EINVAL;
+		return -ENODEV;
 
 	if (strict_strtoul(data, 10, &bandwidth) < 0)
 		return -EINVAL;
@@ -475,7 +475,7 @@ static ssize_t mci_sdram_scrub_rate_show(struct mem_ctl_info *mci, char *data)
 	int bandwidth = 0;
 
 	if (!mci->get_sdram_scrub_rate)
-		return -EINVAL;
+		return -ENODEV;
 
 	bandwidth = mci->get_sdram_scrub_rate(mci);
 	if (bandwidth < 0) {
-- 
1.7.4.rc2



-- 
Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551

RE: [PATCH v2] Fix EDAC sdram_scrub_rate read/write failure

[Luck, Tony]

> If scrub rate setting is not implemented, we say "No such device." and
> otherwise on error we say "Invalid argument" which should be clear,
> IMHO.

That sounds clear to me too.

-Tony