SYSFS "errors"

Commit Message

Mauro Carvalho Chehab Feb. 19, 2013, 11:11 a.m. UTC

Em Tue, 19 Feb 2013 12:11:21 +0200
Felipe Balbi <[e-mail]> escreveu:

> Hi,
> 
> On Tue, Feb 19, 2013 at 07:03:10AM -0300, Mauro Carvalho Chehab wrote:
> > > But my gut feeling says to stay concervative and not touch this code -
> > > we don't know what uses it and how much we would break by "fixing" it.
> > > The current situation is not that big of a deal IMVHO and I'd be willing
> > > to accept the small inconcistency versus possibly breaking userspace.
> > 
> > I remember I saw some discussions about it in the past at bluesmoke ML,
> > saying that -ENODEV is the expected behavior when this is not supported.
> > 
> > Changing from -ENODEV to "N/A" will break anything that would be relying
> > on the previous behavior. So, I think that such change will for sure break
> > userspace.
> > 
> > If we're willing to change it, not creating the "sdram_scrub_rate" sysfs 
> > node is less likely to affect userspace.
> 
> yeah, I agree with this. Guess we shouldn't be creating files which
> aren't supported by the underlying HW and having a read() return -ENODEV
> is quite weird IMO since that's actually 'breaking' read() interface
> although that's up to interpretations.

The enclosed (untested) patch will likely do the trick. It needs to be
tested with one of the drivers that support scrub setting (amd64_edac.c,
cpc925_edac.c, e752x_edac.c, i5100_edac.c or i7core_edac.c).

Regards,
Mauro

-

[PATCH] edac: only create sdram_scrub_rate where supported

Currently, sdram_scrub_rate sysfs node is created even if the device
doesn't support get/set the scub rate. Change the logic to only
create this device node when the operation is supported.

Reported-by: Felipe Balbi <[e-mail]>
Signed-off-by: Mauro Carvalho Chehab <[e-mail]>

Comments

Borislav Petkov Feb. 19, 2013, 11:43 a.m. UTC | #1

On Tue, Feb 19, 2013 at 08:11:49AM -0300, Mauro Carvalho Chehab wrote:
> > > I remember I saw some discussions about it in the past at bluesmoke ML,
> > > saying that -ENODEV is the expected behavior when this is not supported.
> > > 
> > > Changing from -ENODEV to "N/A" will break anything that would be relying
> > > on the previous behavior. So, I think that such change will for sure break
> > > userspace.
> > > 
> > > If we're willing to change it, not creating the "sdram_scrub_rate" sysfs 
> > > node is less likely to affect userspace.

This will break scripts which assume this file's presence implicitly.

[ … ]

> @@ -1017,6 +1010,14 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
>  		return err;
>  	}
>  
> +	if (mci->set_sdram_scrub_rate && mci->get_sdram_scrub_rate) {

This will break cpc925_edac.c because it defines a
->get_sdram_scrub_rate but not a ->set_sdram_scrub_rate.

I think a maybe better fix would be to figure out the sysfs file
permissions based on the presence of the two functions and *then* add
the attribute.

This way, the only visible change to userspace is the corrected sysfs
file permissions.

Patch

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 9c58da6..937975b 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -7,7 +7,7 @@ 
  *
  * Written Doug Thompson <[e-mail]> www.softwarebitmaker.com
  *
- * (c) 2012 - Mauro Carvalho Chehab <[e-mail]>
+ * (c) 2012-2013 - Mauro Carvalho Chehab <[e-mail]>
  *	The entire API were re-written, and ported to use struct device
  *
  */
@@ -681,9 +681,6 @@  static ssize_t mci_sdram_scrub_rate_store(struct device *dev,
 	unsigned long bandwidth = 0;
 	int new_bw = 0;
 
-	if (!mci->set_sdram_scrub_rate)
-		return -ENODEV;
-
 	if (strict_strtoul(data, 10, &bandwidth) < 0)
 		return -EINVAL;
 
@@ -707,9 +704,6 @@  static ssize_t mci_sdram_scrub_rate_show(struct device *dev,
 	struct mem_ctl_info *mci = to_mci(dev);
 	int bandwidth = 0;
 
-	if (!mci->get_sdram_scrub_rate)
-		return -ENODEV;
-
 	bandwidth = mci->get_sdram_scrub_rate(mci);
 	if (bandwidth < 0) {
 		edac_printk(KERN_DEBUG, EDAC_MC, "Error reading scrub rate\n");
@@ -882,7 +876,6 @@  static struct attribute *mci_attrs[] = {
 	&dev_attr_ce_noinfo_count.attr,
 	&dev_attr_ue_count.attr,
 	&dev_attr_ce_count.attr,
-	&dev_attr_sdram_scrub_rate.attr,
 	&dev_attr_max_location.attr,
 	NULL
 };
@@ -1017,6 +1010,14 @@  int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
 		return err;
 	}
 
+	if (mci->set_sdram_scrub_rate && mci->get_sdram_scrub_rate) {
+		err = device_create_file(&mci->dev,
+					 &dev_attr_sdram_scrub_rate);
+		if (err) {
+			edac_dbg(1, "failure: create sdram_scrub_rate\n");
+			goto fail2;
+		}
+	}
 	/*
 	 * Create the dimm/rank devices
 	 */
@@ -1061,6 +1062,7 @@  fail:
 			continue;
 		device_unregister(&dimm->dev);
 	}
+fail2:
 	device_unregister(&mci->dev);
 	bus_unregister(&mci->bus);
 	kfree(mci->bus.name);