Re: SYSFS "errors"

Em Tue, 19 Feb 2013 12:11:21 +0200
Felipe Balbi [off-list ref] escreveu:

Hi,

On Tue, Feb 19, 2013 at 07:03:10AM -0300, Mauro Carvalho Chehab wrote:

quoted

quoted

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 <redacted>
Signed-off-by: Mauro Carvalho Chehab <redacted>

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 <norsk5@xmission.com> www.softwarebitmaker.com
  *
- * (c) 2012 - Mauro Carvalho Chehab <mchehab@redhat.com>
+ * (c) 2012-2013 - Mauro Carvalho Chehab <mchehab@redhat.com>
  *	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);