Re: SYSFS "errors"

Re: SYSFS "errors"

[Felipe Balbi]

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

Hi, On Mon, Feb 18, 2013 at 09:49:16AM -0800, Greg KH wrote:
> > Input/output error - /sys/devices/cpu/power/autosuspend_delay_ms
> 
> The issue with this file is, if the power.use_autosuspend flag is not
> set for the device, then it can't be read or written to.  This flag
> changes dynamically with the system state
> (__pm_runtime_use_autosuspend() can change it), so we can't just not
> show the file if the flag is not set properly, sorry.
> 
> So the "error" is correct here, as is the 0644 file value.

hmm... we could create the file at pm_runtime_enable() time and remove
it on pm_runtime_disable() time, no ? Addin Rafael to Cc

> > No such device or address - /sys/devices/pci0000:00/0000:00:1d.7/usb2/2-5/2-5.1/2-5.1.1/2-5.1.1:1.0/host100/target100:0:0/100:0:0:0/block/sdc/trace/pid
> > No such device or address - /sys/devices/pci0000:00/0000:00:1d.7/usb2/2-5/2-5.1/2-5.1.1/2-5.1.1:1.0/host100/target100:0:0/100:0:0:0/block/sdc/trace/end_lba
> > No such device or address - /sys/devices/pci0000:00/0000:00:1d.7/usb2/2-5/2-5.1/2-5.1.1/2-5.1.1:1.0/host100/target100:0:0/100:0:0:0/block/sdc/trace/enable
> > No such device or address - /sys/devices/pci0000:00/0000:00:1d.7/usb2/2-5/2-5.1/2-5.1.1/2-5.1.1:1.0/host100/target100:0:0/100:0:0:0/block/sdc/trace/act_mask
> > No such device or address - /sys/devices/pci0000:00/0000:00:1d.7/usb2/2-5/2-5.1/2-5.1.1/2-5.1.1:1.0/host100/target100:0:0/100:0:0:0/block/sdc/trace/start_lba
> > Invalid argument - /sys/devices/pci0000:00/0000:00:1f.2/ata2/host1/scsi_host/host1/em_buffer
> > Invalid argument - /sys/devices/pci0000:00/0000:00:1f.2/ata2/host1/target1:0:0/1:0:0:0/sw_activity
> > Operation not supported - /sys/devices/pci0000:00/0000:00:1f.2/ata2/host1/target1:0:0/1:0:0:0/unload_heads
> 
> SCSI fun, go poke the SCSI developers about these files, I know nothing
> about them, nor do I really want to :)
> 
> > No such device - /sys/devices/system/edac/mc/mc0/sdram_scrub_rate
> 
> Odd, go ask the edac developers

will do ;-)

> > Input/output error - /sys/devices/system/memory/soft_offline_page
> > Input/output error - /sys/devices/system/memory/hard_offline_page
> 
> These both should be mode 0400, not 0644, they have no 'show' attribute,
> that's a bug, care to make up a patch that I can take?  Or I can, let me
> know.

sure, will send in a bit.

> > Invalid argument - /sys/devices/virtual/net/lo/speed
> > Invalid argument - /sys/devices/virtual/net/lo/duplex
> 
> Ask the network developers, odds are, the loopback device doesn't really
> have a speed :)

right ;-)

> > No such device or address - /sys/devices/virtual/block/loop0/trace/pid
> > No such device or address - /sys/devices/virtual/block/loop0/trace/end_lba
> > No such device or address - /sys/devices/virtual/block/loop0/trace/enable
> > No such device or address - /sys/devices/virtual/block/loop0/trace/act_mask
> > No such device or address - /sys/devices/virtual/block/loop0/trace/start_lba
> 
> Hm, these show up for virtual block devices as well, maybe they aren't
> scsi specific, but rather block specific, go poke the block developers
> then.

yeah, I was looking into these a while ago. They're created by
blk_trace_init_sysfs() which is called by blk_register_queue() but I
wonder if we couldn't defer the creation of those so that:

sysfs_blk_trace_attr_show():

1639         bdev = bdget(part_devt(p));
1640         if (bdev == NULL)
1641                 goto out;
1642 
1643         q = blk_trace_get_queue(bdev);
1644         if (q == NULL)
1645                 goto out_bdput;

never fail (meaning they could be removed), adding some block folks to
the loop.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: SYSFS "errors"

[Mauro Carvalho Chehab]

Em Mon, 18 Feb 2013 20:46:33 +0200
Felipe Balbi <balbi@ti.com> escreveu:

> Hi, On Mon, Feb 18, 2013 at 09:49:16AM -0800, Greg KH wrote:
> > > Input/output error - /sys/devices/cpu/power/autosuspend_delay_ms
> > 
> > The issue with this file is, if the power.use_autosuspend flag is not
> > set for the device, then it can't be read or written to.  This flag
> > changes dynamically with the system state
> > (__pm_runtime_use_autosuspend() can change it), so we can't just not
> > show the file if the flag is not set properly, sorry.
> > 
> > So the "error" is correct here, as is the 0644 file value.
> 
> hmm... we could create the file at pm_runtime_enable() time and remove
> it on pm_runtime_disable() time, no ? Addin Rafael to Cc

...


> > 
> > > No such device - /sys/devices/system/edac/mc/mc0/sdram_scrub_rate
> > 
> > Odd, go ask the edac developers
> 
> will do ;-)

Well, the question is missing ;) /me assumes that you want to talk about
suspending/resume and EDAC, right?

In general, memory controllers don't supports suspend, as far as I can tell. 
Still, I've seen a few ones that support, but the current drivers and/or the
EDAC core currently doesn't offer any support to it, as such setup is done
by the BIOS, when it detects the used DIMM banks.

I suspect that, when the OS puts the machine on a suspend state, the BIOS may
also suspend also the memory controller or put it into a low power consumption
mode, but it does it without any help from the EDAC drivers.

For most of what's there at EDAC, I don't think it is worth to add any PM support
inside it. There are, however, two cases were we may need to add some support:

1) if user changed the SDRAM scrub rate before suspending, it makes sense to
restore it after resume, on the memory controller drivers that support such
feature (not all supports it);

2) hot-pluggable DIMMs. EDAC currently doesn't support. This could be needed
on some future. In this case, the core may need to re-scan the memory controller,
changing the memory properties. I've no idea if is there any real case needing
it, nor what event would trigger the memory-controller re-scan. Resume is likely
one of the candidates for it, on machines that support hot-pluggable memories.

Regards,
Mauro

Re: SYSFS "errors"

[Felipe Balbi]

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

Hi,

On Mon, Feb 18, 2013 at 04:46:38PM -0300, Mauro Carvalho Chehab wrote:
> > > > No such device - /sys/devices/system/edac/mc/mc0/sdram_scrub_rate
> > > 
> > > Odd, go ask the edac developers
> > 
> > will do ;-)
> 
> Well, the question is missing ;) /me assumes that you want to talk about
> suspending/resume and EDAC, right?

oops, my bad. The thing is that file has read permission but reading it
isn't allowed ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: SYSFS "errors"

[Mauro Carvalho Chehab]

Em Mon, 18 Feb 2013 22:05:42 +0200
Felipe Balbi <balbi@ti.com> escreveu:

> Hi,
> 
> On Mon, Feb 18, 2013 at 04:46:38PM -0300, Mauro Carvalho Chehab wrote:
> > > > > No such device - /sys/devices/system/edac/mc/mc0/sdram_scrub_rate
> > > > 
> > > > Odd, go ask the edac developers
> > > 
> > > will do ;-)
> > 
> > Well, the question is missing ;) /me assumes that you want to talk about
> > suspending/resume and EDAC, right?
> 
> oops, my bad. The thing is that file has read permission but reading it
> isn't allowed ;-)

That depends on the driver. See drivers/edac/edac_mc_sysfs.c:

static ssize_t mci_sdram_scrub_rate_show(struct device *dev,
					 struct device_attribute *mattr,
					 char *data)
{
	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");
		return bandwidth;
	}

	return sprintf(data, "%d\n", bandwidth);
}

/* memory scrubber attribute file */
DEVICE_ATTR(sdram_scrub_rate, S_IRUGO | S_IWUSR, mci_sdram_scrub_rate_show,
       	mci_sdram_scrub_rate_store);

static struct attribute *mci_attrs[] = {
        &dev_attr_reset_counters.attr,
       	&dev_attr_mc_name.attr,
        &dev_attr_size_mb.attr,
        &dev_attr_seconds_since_reset.attr,
        &dev_attr_ue_noinfo_count.attr,
        &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
};

The capability of get and/or set the scrub rate is edac-driver specific.
Drivers that support it need to fill those callbacks:
	mci->get_sdram_scrub_rate
	mci->set_sdram_scrub_rate

In any case, the sysfs attribute is created even if the device doesn't
have support for read/set the scrub rate.

On devices where this is not supported, reading (and/or writing) to this
sysfs node will return -ENODEV.

In the past, the sysfs node creation was done using the raw sysfs support.
Doing dynamic creation with the old code were much more complex. I guess
that's the reason why the code was written this way. Now that the code 
uses struct device, it shouldn't be hard to change the code to only 
create this attribute if the device has support for scrub rate setting.

Yet, that would change the userspace API. I'm not sure what
applications would rely on the current behavior. So, changing it
would require some investigation in order to avoid regressions.

Regards,
Mauro

Re: SYSFS "errors"

[Greg KH]

On Mon, Feb 18, 2013 at 06:47:42PM -0300, Mauro Carvalho Chehab wrote:
> In the past, the sysfs node creation was done using the raw sysfs support.
> Doing dynamic creation with the old code were much more complex. I guess
> that's the reason why the code was written this way. Now that the code 
> uses struct device, it shouldn't be hard to change the code to only 
> create this attribute if the device has support for scrub rate setting.
> 
> Yet, that would change the userspace API. I'm not sure what
> applications would rely on the current behavior. So, changing it
> would require some investigation in order to avoid regressions.

Because sysfs is "one value per file" the lack of a file showing up
shouldn't cause any userspace tools any problems, that is why we did
things this way.

But, of course, userspace programmers do know how to mess things up...

greg k-h

Re: SYSFS "errors"

[Borislav Petkov]

On Mon, Feb 18, 2013 at 01:54:34PM -0800, Greg KH wrote:
> Because sysfs is "one value per file" the lack of a file showing up
> shouldn't cause any userspace tools any problems, that is why we did
> things this way.
>
> But, of course, userspace programmers do know how to mess things up...

How about what I proposed earlier to Felipe:

--
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 0ca1ca71157f..d2eef76d1d46 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -704,7 +704,7 @@ static ssize_t mci_sdram_scrub_rate_show(struct device *dev,
 	int bandwidth = 0;
 
 	if (!mci->get_sdram_scrub_rate)
-		return -ENODEV;
+		return sprintf(data, "N/A");
 
 	bandwidth = mci->get_sdram_scrub_rate(mci);
 	if (bandwidth < 0) {

--

Would that hurt the sysfs policy? In this case we *can* read the file
and it correctly tells us that scrub rate reading is not supported
instead of having a number there.

Hmm.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: SYSFS "errors"

[Greg KH]

On Mon, Feb 18, 2013 at 11:13:06PM +0100, Borislav Petkov wrote:
> On Mon, Feb 18, 2013 at 01:54:34PM -0800, Greg KH wrote:
> > Because sysfs is "one value per file" the lack of a file showing up
> > shouldn't cause any userspace tools any problems, that is why we did
> > things this way.
> >
> > But, of course, userspace programmers do know how to mess things up...
> 
> How about what I proposed earlier to Felipe:
> 
> --
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index 0ca1ca71157f..d2eef76d1d46 100644
> --- a/drivers/edac/edac_mc_sysfs.c
> +++ b/drivers/edac/edac_mc_sysfs.c
> @@ -704,7 +704,7 @@ static ssize_t mci_sdram_scrub_rate_show(struct device *dev,
>  	int bandwidth = 0;
>  
>  	if (!mci->get_sdram_scrub_rate)
> -		return -ENODEV;
> +		return sprintf(data, "N/A");
>  
>  	bandwidth = mci->get_sdram_scrub_rate(mci);
>  	if (bandwidth < 0) {
> 
> --
> 
> Would that hurt the sysfs policy? In this case we *can* read the file
> and it correctly tells us that scrub rate reading is not supported
> instead of having a number there.
> 
> Hmm.

I don't know, it depends on if userspace can handle this properly or
not.  What tools rely on this sysfs file?  WHat happens when they get a
non-number in the file?

thanks,

greg k-h

Re: SYSFS "errors"

[Borislav Petkov]

On Mon, Feb 18, 2013 at 02:26:18PM -0800, Greg KH wrote:
> I don't know, it depends on if userspace can handle this properly or
> not. What tools rely on this sysfs file? WHat happens when they get a
> non-number in the file?

I'm not aware of any, frankly speaking.

If there are any, those tools should be able to handle the -ENODEV
they get. Now, if this gets changed this way, the read would succeed
but they'll have to parse the returned value and see that it is not an
integer.

So I don't know either.

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.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: SYSFS "errors"

[Mauro Carvalho Chehab]

Em Mon, 18 Feb 2013 23:44:05 +0100
Borislav Petkov <bp@alien8.de> escreveu:

> On Mon, Feb 18, 2013 at 02:26:18PM -0800, Greg KH wrote:
> > I don't know, it depends on if userspace can handle this properly or
> > not. What tools rely on this sysfs file? WHat happens when they get a
> > non-number in the file?

The thing with "sdram_scrub_rate" is that this is not supported by any
userspace application I know. I suspect that this is used by userspace
scripts. So, we'll never know in advance what behavior those scripts would
expect.

> 
> I'm not aware of any, frankly speaking.
> 
> If there are any, those tools should be able to handle the -ENODEV
> they get. Now, if this gets changed this way, the read would succeed
> but they'll have to parse the returned value and see that it is not an
> integer.
> 
> So I don't know either.
> 
> 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.

Regards,
Mauro

Re: SYSFS "errors"

[Felipe Balbi]

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

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.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: SYSFS "errors"

[Mauro Carvalho Chehab]

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

Em Tue, 19 Feb 2013 12:11:21 +0200
Felipe Balbi <balbi@ti.com> 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 <balbi@ti.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

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);


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: SYSFS "errors"

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: SYSFS "errors"

[Mauro Carvalho Chehab]

Em Tue, 19 Feb 2013 12:43:46 +0100
Borislav Petkov <bp@alien8.de> escreveu:

> 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.

If are there any script like that, then yes.

> [ … ]
> 
> > @@ -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.

True. Patch below fixes it.

> 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.

I'm not sure if is there a way to pass fs permissions to something similar
to device_create_file().

On both cases, an error will happen at open:
	- if file doesn't exist (this approach), it will return -ENOENT;
	- if file is opened with wrong permissions, open will return -EPERM.

However, if the file is not created, readdir() won't show the file.
So, if userspace first seeks for the sdram_scrub_rate, it won't fail.
That makes the logic below a little safer, IMO.


[PATCH EDAC] 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.

If only read or only write is supported, it will keep returning -ENODEV
just like before.

Reported-by: Felipe Balbi <balbi@ti.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 9c58da6..29b66a2 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
  *
  */
@@ -882,7 +882,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 +1016,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 +1068,7 @@ fail:
 			continue;
 		device_unregister(&dimm->dev);
 	}
+fail2:
 	device_unregister(&mci->dev);
 	bus_unregister(&mci->bus);
 	kfree(mci->bus.name);

Re: SYSFS "errors"

[Borislav Petkov]

On Tue, Feb 19, 2013 at 09:16:10AM -0300, Mauro Carvalho Chehab wrote:
> I'm not sure if is there a way to pass fs permissions to something similar
> to device_create_file().

struct device_attribute.attr.mode? I.e., second arg.

> On both cases, an error will happen at open:
> 	- if file doesn't exist (this approach), it will return -ENOENT;
> 	- if file is opened with wrong permissions, open will return -EPERM.
> 
> However, if the file is not created, readdir() won't show the file.

Right, and in that case userspace which *assumes* it is always created -
like it is now - will fail when accessing it.

If simply you adjust the attributes accordingly but *always* create the
file and it has the correct permissions, everyone is happy. Right?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: SYSFS "errors"

[Mauro Carvalho Chehab]

Em Tue, 19 Feb 2013 13:35:02 +0100
Borislav Petkov <bp@alien8.de> escreveu:

> On Tue, Feb 19, 2013 at 09:16:10AM -0300, Mauro Carvalho Chehab wrote:
> > I'm not sure if is there a way to pass fs permissions to something similar
> > to device_create_file().
> 
> struct device_attribute.attr.mode? I.e., second arg.

Ah, now I see what you're meaning. That would require to dynamically
create a per-mci DEVICE_ATTR().

> > On both cases, an error will happen at open:
> > 	- if file doesn't exist (this approach), it will return -ENOENT;
> > 	- if file is opened with wrong permissions, open will return -EPERM.
> > 
> > However, if the file is not created, readdir() won't show the file.
> 
> Right, and in that case userspace which *assumes* it is always created -
> like it is now - will fail when accessing it.
> 
> If simply you adjust the attributes accordingly but *always* create the
> file and it has the correct permissions, everyone is happy. Right?

No, on both cases, open() will return an error (-ENOENT against -EPERM). 

If userspace doesn't check if open() failed, I can't see why
changing the open return error code would help.

-- 

Cheers,
Mauro

Re: SYSFS "errors"

[Borislav Petkov]

On Tue, Feb 19, 2013 at 09:46:40AM -0300, Mauro Carvalho Chehab wrote:
> Ah, now I see what you're meaning. That would require to dynamically
> create a per-mci DEVICE_ATTR().

Dude, look at the code, we do that already. And you're using it with
dev_attr_sdram_scrub_rate.

Simply change the permissions of the attribute before calling
device_create_file.

pseudo:

	umode_t mode = 0;

	if (mci->set...)
		mode |= S_IWUSR;

	if (mci->get...)
		mode |= S_IRUGO;


	dev_attr_sdram_scrub_rate.attr.mode = mode;

	device_create_file(&mci->dev, &dev_attr_sdram_scrub_rate);

> No, on both cases, open() will return an error (-ENOENT against -EPERM).

What if it is a shell script doing:

cat /sys/devices/system/edac/mc/mc0/sdram_scrub_rate

or similar?

Simply fixing the permissions fixes *all* possible cases.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: SYSFS "errors"

[Felipe Balbi]

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

On Tue, Feb 19, 2013 at 02:06:26PM +0100, Borislav Petkov wrote:
> On Tue, Feb 19, 2013 at 09:46:40AM -0300, Mauro Carvalho Chehab wrote:
> > Ah, now I see what you're meaning. That would require to dynamically
> > create a per-mci DEVICE_ATTR().
> 
> Dude, look at the code, we do that already. And you're using it with
> dev_attr_sdram_scrub_rate.
> 
> Simply change the permissions of the attribute before calling
> device_create_file.
> 
> pseudo:
> 
> 	umode_t mode = 0;
> 
> 	if (mci->set...)
> 		mode |= S_IWUSR;
> 
> 	if (mci->get...)
> 		mode |= S_IRUGO;
> 
> 
> 	dev_attr_sdram_scrub_rate.attr.mode = mode;
> 
> 	device_create_file(&mci->dev, &dev_attr_sdram_scrub_rate);
> 
> > No, on both cases, open() will return an error (-ENOENT against -EPERM).
> 
> What if it is a shell script doing:
> 
> cat /sys/devices/system/edac/mc/mc0/sdram_scrub_rate

what's the problem with that ?

$ cat /path/to/file/that/doesnt/exist.txt
cat: /path/to/file/that/doesnt/exist.txt: No such file or directory

Didn't see any gates to hell opening here...

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: SYSFS "errors"

[Borislav Petkov]

On Tue, Feb 19, 2013 at 03:15:00PM +0200, Felipe Balbi wrote:
> what's the problem with that ?

Not a problem - simply annoying.

$ ./test.sh
cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
Setting scrubrate to:

I'm sure someone would ask themselves why all of a sudden the file is
gone.

> $ cat /path/to/file/that/doesnt/exist.txt
> cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
> 
> Didn't see any gates to hell opening here...

And I don't see why we have to debate such a trivial thing when we can
fix it *properly* without *absolutely* *not* upsetting userspace.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: SYSFS "errors"

[Felipe Balbi]

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

Hi,

On Tue, Feb 19, 2013 at 02:28:54PM +0100, Borislav Petkov wrote:
> On Tue, Feb 19, 2013 at 03:15:00PM +0200, Felipe Balbi wrote:
> > what's the problem with that ?
> 
> Not a problem - simply annoying.
> 
> $ ./test.sh
> cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
> Setting scrubrate to:
> 
> I'm sure someone would ask themselves why all of a sudden the file is
> gone.
> 
> > $ cat /path/to/file/that/doesnt/exist.txt
> > cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
> > 
> > Didn't see any gates to hell opening here...
> 
> And I don't see why we have to debate such a trivial thing when we can
> fix it *properly* without *absolutely* *not* upsetting userspace.

because changing the permission will cause the same issue:

$ cat temporary.txt 
cat: temporary.txt: Permission denied
$ ls -l temporary.txt 
---------- 1 balbi balbi 0 Feb 19 15:37 temporary.txt
$ cat temporary.txt 
cat: temporary.txt: Permission denied
$ cat non-existent.txt
cat: non-existent.txt: No such file or directory

just a different error code is returned...

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: SYSFS "errors"

[Mauro Carvalho Chehab]

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

Em Tue, 19 Feb 2013 15:38:09 +0200
Felipe Balbi <balbi@ti.com> escreveu:

> Hi,
> 
> On Tue, Feb 19, 2013 at 02:28:54PM +0100, Borislav Petkov wrote:
> > On Tue, Feb 19, 2013 at 03:15:00PM +0200, Felipe Balbi wrote:
> > > what's the problem with that ?
> > 
> > Not a problem - simply annoying.
> > 
> > $ ./test.sh
> > cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
> > Setting scrubrate to:
> > 
> > I'm sure someone would ask themselves why all of a sudden the file is
> > gone.
> > 
> > > $ cat /path/to/file/that/doesnt/exist.txt
> > > cat: /path/to/file/that/doesnt/exist.txt: No such file or directory
> > > 
> > > Didn't see any gates to hell opening here...
> > 
> > And I don't see why we have to debate such a trivial thing when we can
> > fix it *properly* without *absolutely* *not* upsetting userspace.
> 
> because changing the permission will cause the same issue:

It is actually worse, as if someone is using a C code to open the device
with
	fp = open ("/sys/devices/system/edac/mc/mc0/sdram_scrub_rate", O_RDWR);

It will now start to fail if the device doesn't have both permissions.

Regards,
Mauro

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: SYSFS "errors"

[Borislav Petkov]

On Tue, Feb 19, 2013 at 10:50:48AM -0300, Mauro Carvalho Chehab wrote:
> It is actually worse, as if someone is using a C code to open the device
> with
> 	fp = open ("/sys/devices/system/edac/mc/mc0/sdram_scrub_rate", O_RDWR);
> 
> It will now start to fail if the device doesn't have both permissions.

That's fine - you're supposed to handle open()'s retval properly anyway. See my
other mail.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: SYSFS "errors"

[Borislav Petkov]

On Tue, Feb 19, 2013 at 03:38:09PM +0200, Felipe Balbi wrote:
> because changing the permission will cause the same issue:

Actually, I take that back. Mauro's patch will already create the file
anyway:

	if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate)

Adjusting the permissions is simply the last missing piece to this patch
to make the interface to userspace 100% coherent.

--
From: Mauro Carvalho Chehab <mchehab@redhat.com>
Date: Tue, 19 Feb 2013 09:16:10 -0300
Subject: [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.

If only read or only write is supported, it will keep returning -ENODEV
just like before.

Reported-by: Felipe Balbi <balbi@ti.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
 drivers/edac/edac_mc_sysfs.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 0ca1ca71157f..5a788e60ff67 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
  *
  */
@@ -878,7 +878,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
 };
@@ -1012,6 +1011,23 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
 		return err;
 	}
 
+	if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate) {
+		umode_t mode = 0;
+
+		if (mci->get_sdram_scrub_rate)
+			mode = S_IRUGO;
+
+		if (mci->set_sdram_scrub_rate)
+			mode |= S_IWUSR;
+
+		dev_attr_sdram_scrub_rate.attr.mode = mode;
+
+		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
 	 */
@@ -1056,6 +1072,7 @@ fail:
 			continue;
 		device_unregister(&dimm->dev);
 	}
+fail2:
 	device_unregister(&mci->dev);
 	bus_unregister(&mci->bus);
 	kfree(mci->bus.name);
-- 
1.8.1.3.535.ga923c31


-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: SYSFS "errors"

[Hannes Reinecke]

On 02/19/2013 02:50 PM, Borislav Petkov wrote:
> On Tue, Feb 19, 2013 at 03:38:09PM +0200, Felipe Balbi wrote:
>> because changing the permission will cause the same issue:
>
> Actually, I take that back. Mauro's patch will already create the file
> anyway:
>
> 	if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate)
>
> Adjusting the permissions is simply the last missing piece to this patch
> to make the interface to userspace 100% coherent.
>
> --
> From: Mauro Carvalho Chehab <mchehab@redhat.com>
> Date: Tue, 19 Feb 2013 09:16:10 -0300
> Subject: [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.
>
> If only read or only write is supported, it will keep returning -ENODEV
> just like before.
>
> Reported-by: Felipe Balbi <balbi@ti.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> ---
>   drivers/edac/edac_mc_sysfs.c | 21 +++++++++++++++++++--
>   1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> index 0ca1ca71157f..5a788e60ff67 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
>    *
>    */
> @@ -878,7 +878,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
>   };
> @@ -1012,6 +1011,23 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
>   		return err;
>   	}
>
> +	if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate) {
> +		umode_t mode = 0;
> +
> +		if (mci->get_sdram_scrub_rate)
> +			mode = S_IRUGO;
> +
> +		if (mci->set_sdram_scrub_rate)
> +			mode |= S_IWUSR;
> +
> +		dev_attr_sdram_scrub_rate.attr.mode = mode;
> +
> +		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
>   	 */
> @@ -1056,6 +1072,7 @@ fail:
>   			continue;
>   		device_unregister(&dimm->dev);
>   	}
> +fail2:
>   	device_unregister(&mci->dev);
>   	bus_unregister(&mci->bus);
>   	kfree(mci->bus.name);
>
And of course you all know that creating sysfs attributes via 
'device_create_file' opens all sort of funny race conditions, 
especially when checking these attributes from udev ...

Please consider adding a default attribute which return '-EINVAL' or 
somesuch when the function pointers are not set.
But _not_ adding it via device_create_file(). That's evil.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Re: SYSFS "errors"

[Borislav Petkov]

On Tue, Feb 19, 2013 at 02:58:07PM +0100, Hannes Reinecke wrote:
> Please consider adding a default attribute which return '-EINVAL' or
> somesuch when the function pointers are not set. But _not_ adding it
> via device_create_file(). That's evil.

This is what we do now. We probably could add the permissions fiddling
out in the the code but remain using DEVICE_ATTR.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: SYSFS "errors"

[Mauro Carvalho Chehab]

Em Tue, 19 Feb 2013 14:58:07 +0100
Hannes Reinecke <hare@suse.de> escreveu:

> On 02/19/2013 02:50 PM, Borislav Petkov wrote:
> > On Tue, Feb 19, 2013 at 03:38:09PM +0200, Felipe Balbi wrote:
> >> because changing the permission will cause the same issue:
> >
> > Actually, I take that back. Mauro's patch will already create the file
> > anyway:
> >
> > 	if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate)
> >
> > Adjusting the permissions is simply the last missing piece to this patch
> > to make the interface to userspace 100% coherent.
> >
> > --
> > From: Mauro Carvalho Chehab <mchehab@redhat.com>
> > Date: Tue, 19 Feb 2013 09:16:10 -0300
> > Subject: [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.
> >
> > If only read or only write is supported, it will keep returning -ENODEV
> > just like before.
> >
> > Reported-by: Felipe Balbi <balbi@ti.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
> > ---
> >   drivers/edac/edac_mc_sysfs.c | 21 +++++++++++++++++++--
> >   1 file changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
> > index 0ca1ca71157f..5a788e60ff67 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
> >    *
> >    */
> > @@ -878,7 +878,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
> >   };
> > @@ -1012,6 +1011,23 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
> >   		return err;
> >   	}
> >
> > +	if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate) {
> > +		umode_t mode = 0;
> > +
> > +		if (mci->get_sdram_scrub_rate)
> > +			mode = S_IRUGO;
> > +
> > +		if (mci->set_sdram_scrub_rate)
> > +			mode |= S_IWUSR;
> > +
> > +		dev_attr_sdram_scrub_rate.attr.mode = mode;
> > +
> > +		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
> >   	 */
> > @@ -1056,6 +1072,7 @@ fail:
> >   			continue;
> >   		device_unregister(&dimm->dev);
> >   	}
> > +fail2:
> >   	device_unregister(&mci->dev);
> >   	bus_unregister(&mci->bus);
> >   	kfree(mci->bus.name);
> >
> And of course you all know that creating sysfs attributes via 
> 'device_create_file' opens all sort of funny race conditions, 
> especially when checking these attributes from udev ...

Yes, we know that, but this subsystem has already lots of other
attributes created via device_create_file(). It used to be a
lot worse than that, as, on a very recent past (before Kernel 3.5),
those attributes were created via sysfs_create_file().

There's not much that can be done to avoid it on this subsystem.

> 
> Please consider adding a default attribute which return '-EINVAL' or 
> somesuch when the function pointers are not set.
> But _not_ adding it via device_create_file(). That's evil.

This thread started with Felipe's complain about it returning -ENOSYS ;)
when this feature is not supported.

Regards,
Mauro

Re: SYSFS "errors"

[Felipe Balbi]

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

Hi,

On Tue, Feb 19, 2013 at 11:14:40AM -0300, Mauro Carvalho Chehab wrote:
> > Please consider adding a default attribute which return '-EINVAL' or 
> > somesuch when the function pointers are not set.
> > But _not_ adding it via device_create_file(). That's evil.
> 
> This thread started with Felipe's complain about it returning -ENOSYS ;)
> when this feature is not supported.

I was complaining about a file with read permission not being really
readable which is a fair bug IMHO.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: SYSFS "errors"

[Mauro Carvalho Chehab]

Em Tue, 19 Feb 2013 14:50:56 +0100
Borislav Petkov <bp@alien8.de> escreveu:

> On Tue, Feb 19, 2013 at 03:38:09PM +0200, Felipe Balbi wrote:
> > because changing the permission will cause the same issue:
> 
> Actually, I take that back. Mauro's patch will already create the file
> anyway:
> 
> 	if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate)
> 
> Adjusting the permissions is simply the last missing piece to this patch
> to make the interface to userspace 100% coherent.

That's covers everything but Hannes arguments. I don't think that
adding just one more device_create_file() on a driver that creates
dozens (or hundreds) of sys nodes, depending on the number of DIMMS
on the system would make it any worse.

Regards,
Mauro

[PATCH EDAC] 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 <balbi@ti.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 9c58da6..4f4b613 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");
@@ -870,8 +864,7 @@ DEVICE_ATTR(ce_count, S_IRUGO, mci_ce_count_show, NULL);
 DEVICE_ATTR(max_location, S_IRUGO, mci_max_location_show, NULL);
 
 /* memory scrubber attribute file */
-DEVICE_ATTR(sdram_scrub_rate, S_IRUGO | S_IWUSR, mci_sdram_scrub_rate_show,
-	mci_sdram_scrub_rate_store);
+DEVICE_ATTR(sdram_scrub_rate, 0, NULL, NULL);
 
 static struct attribute *mci_attrs[] = {
 	&dev_attr_reset_counters.attr,
@@ -882,7 +875,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 +1009,22 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
 		return err;
 	}
 
+	if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate) {
+		if (mci->get_sdram_scrub_rate) {
+			dev_attr_sdram_scrub_rate.attr.mode |= S_IRUGO;
+			dev_attr_sdram_scrub_rate.show = &mci_sdram_scrub_rate_show;
+		}
+		if (mci->set_sdram_scrub_rate) {
+			dev_attr_sdram_scrub_rate.attr.mode |= S_IWUSR;
+			dev_attr_sdram_scrub_rate.store = &mci_sdram_scrub_rate_store;
+		}
+		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 +1069,7 @@ fail:
 			continue;
 		device_unregister(&dimm->dev);
 	}
+fail2:
 	device_unregister(&mci->dev);
 	bus_unregister(&mci->bus);
 	kfree(mci->bus.name);

-- 

Cheers,
Mauro

Re: SYSFS "errors"

[Borislav Petkov]

On Tue, Feb 19, 2013 at 11:35:21AM -0300, Mauro Carvalho Chehab wrote:
> That's covers everything but Hannes arguments. I don't think that
> adding just one more device_create_file() on a driver that creates
> dozens (or hundreds) of sys nodes, depending on the number of DIMMS
> on the system would make it any worse.

Right, those attributes are created once during EDAC core init and we
haven't seen any issues so far so let's not address this just yet but
make a mental note about the race condition possibility.

> [PATCH EDAC] 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 <balbi@ti.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

Acked-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: SYSFS "errors"

[Felipe Balbi]

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

On Tue, Feb 19, 2013 at 11:35:21AM -0300, Mauro Carvalho Chehab wrote:
> Em Tue, 19 Feb 2013 14:50:56 +0100
> Borislav Petkov <bp@alien8.de> escreveu:
> 
> > On Tue, Feb 19, 2013 at 03:38:09PM +0200, Felipe Balbi wrote:
> > > because changing the permission will cause the same issue:
> > 
> > Actually, I take that back. Mauro's patch will already create the file
> > anyway:
> > 
> > 	if (mci->set_sdram_scrub_rate || mci->get_sdram_scrub_rate)
> > 
> > Adjusting the permissions is simply the last missing piece to this patch
> > to make the interface to userspace 100% coherent.
> 
> That's covers everything but Hannes arguments. I don't think that
> adding just one more device_create_file() on a driver that creates
> dozens (or hundreds) of sys nodes, depending on the number of DIMMS
> on the system would make it any worse.
> 
> Regards,
> Mauro
> 
> [PATCH EDAC] 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 <balbi@ti.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

looks alright to me:

Reviewed-by: Felipe Balbi <balbi@ti.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: SYSFS "errors"

[Mauro Carvalho Chehab]

Em Tue, 19 Feb 2013 14:06:26 +0100
Borislav Petkov <bp@alien8.de> escreveu:

> > No, on both cases, open() will return an error (-ENOENT against -EPERM).
> 
> What if it is a shell script doing:
> 
> cat /sys/devices/system/edac/mc/mc0/sdram_scrub_rate
> 
> or similar?

Well, cat will return "1" if an error is found, no matter what error happened. 

With an existing file (-ENOSYS):

	$ cat /sys/devices/system/edac/mc/mc0/sdram_scrub_rate; echo $?

	cat: /sys/devices/system/edac/mc/mc0/sdram_scrub_rate: No such device
	1

When the file doesn't exist (-ENOENT):
	$ cat /sys/devices/system/edac/mc/mc0/sdram_scrub_rate_not_exist; echo $?
	cat: /sys/devices/system/edac/mc/mc0/sdram_scrub_rate_not_exist: No such file or directory
	1

When permission doesn't match (-EPERM):
	$ cat /sys/devices/system/cpu/probe; echo $?
	cat: /sys/devices/system/cpu/probe: Permission denied
	1

When everything is ok, it will return 0
	$ cat /sys/devices/system/edac/mc/mc0/ce_count; echo $? >/dev/null
	0

A script ready to handle -ENOSYS would be doing, instead:

	RATE=$(cat /sys/devices/system/edac/mc/mc0/ce_count 2>/dev/null)
	if [ "$?" == "0" ]; then echo "Scrub rate: $RATE"; fi

So, a bash script won't notice any difference.

The only difference will be noticed if the script is written on some other 
language and the script is dumb enough to assume success if the errno is
different than -ENOSYS.

-- 

Cheers,
Mauro

Re: SYSFS "errors"

[Alan Stern]

On Mon, 18 Feb 2013, Felipe Balbi wrote:

> Hi, On Mon, Feb 18, 2013 at 09:49:16AM -0800, Greg KH wrote:
> > > Input/output error - /sys/devices/cpu/power/autosuspend_delay_ms
> > 
> > The issue with this file is, if the power.use_autosuspend flag is not
> > set for the device, then it can't be read or written to.  This flag
> > changes dynamically with the system state
> > (__pm_runtime_use_autosuspend() can change it), so we can't just not
> > show the file if the flag is not set properly, sorry.
> > 
> > So the "error" is correct here, as is the 0644 file value.
> 
> hmm... we could create the file at pm_runtime_enable() time and remove
> it on pm_runtime_disable() time, no ? Addin Rafael to Cc

In theory this could be done, although the times would be when runtime 
autosuspend is turned on or off, not when pm_runtime_enable is called.

Alan Stern

Re: SYSFS "errors"

[Felipe Balbi]

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

On Mon, Feb 18, 2013 at 04:48:06PM -0500, Alan Stern wrote:
> On Mon, 18 Feb 2013, Felipe Balbi wrote:
> 
> > Hi, On Mon, Feb 18, 2013 at 09:49:16AM -0800, Greg KH wrote:
> > > > Input/output error - /sys/devices/cpu/power/autosuspend_delay_ms
> > > 
> > > The issue with this file is, if the power.use_autosuspend flag is not
> > > set for the device, then it can't be read or written to.  This flag
> > > changes dynamically with the system state
> > > (__pm_runtime_use_autosuspend() can change it), so we can't just not
> > > show the file if the flag is not set properly, sorry.
> > > 
> > > So the "error" is correct here, as is the 0644 file value.
> > 
> > hmm... we could create the file at pm_runtime_enable() time and remove
> > it on pm_runtime_disable() time, no ? Addin Rafael to Cc
> 
> In theory this could be done, although the times would be when runtime 
> autosuspend is turned on or off, not when pm_runtime_enable is called.

Right, I got that after replying. Here's a patch I plan to test
tomorrow:

8<------------------------ cut here ----------------------------

From 2cdeeb483383490946cc98727a167843c40d7d27 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <balbi@ti.com>
Date: Mon, 18 Feb 2013 21:20:28 +0200
Subject: [PATCH] base: power: create autosuspend_delay_ms from
 pm_runtime_use_autosuspend()

this patch moves the creation of autosuspend_delay_ms
sysfs file to pm_runtime_use_autosuspend() and, conversely,
its deletion to pm_runtime_dont_use_autosuspend.

The idea behind this patch is such that the file in
question is only readable after pm_runtime_use_autosuspend()
has been called for a particular device, so we might
as well move the creation of the file to that place.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/base/power/power.h   |  4 ++++
 drivers/base/power/runtime.c |  6 ++++++
 drivers/base/power/sysfs.c   | 25 ++++++++++++++++++++++++-
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
index b16686a..3822b94 100644
--- a/drivers/base/power/power.h
+++ b/drivers/base/power/power.h
@@ -90,6 +90,8 @@ static inline void device_pm_init(struct device *dev)
 
 extern int dpm_sysfs_add(struct device *dev);
 extern void dpm_sysfs_remove(struct device *dev);
+extern int dpm_autosuspend_sysfs_add(struct device *dev);
+extern void dpm_autosuspend_sysfs_remove(struct device *dev);
 extern void rpm_sysfs_remove(struct device *dev);
 extern int wakeup_sysfs_add(struct device *dev);
 extern void wakeup_sysfs_remove(struct device *dev);
@@ -102,6 +104,8 @@ extern void pm_qos_sysfs_remove_flags(struct device *dev);
 
 static inline int dpm_sysfs_add(struct device *dev) { return 0; }
 static inline void dpm_sysfs_remove(struct device *dev) {}
+static inline int dpm_autosuspend_sysfs_add(struct device *dev) { return 0; }
+static inline void dpm_autosuspend_sysfs_remove(struct device *dev) {}
 static inline void rpm_sysfs_remove(struct device *dev) {}
 static inline int wakeup_sysfs_add(struct device *dev) { return 0; }
 static inline void wakeup_sysfs_remove(struct device *dev) {}
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 3148b10..a162ba5 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1266,6 +1266,12 @@ void __pm_runtime_use_autosuspend(struct device *dev, bool use)
 	old_use = dev->power.use_autosuspend;
 	dev->power.use_autosuspend = use;
 	update_autosuspend(dev, old_delay, old_use);
+
+	if (use)
+		dpm_autosuspend_sysfs_add(dev);
+	else
+		dpm_autosuspend_sysfs_remove(dev);
+
 	spin_unlock_irq(&dev->power.lock);
 }
 EXPORT_SYMBOL_GPL(__pm_runtime_use_autosuspend);
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index 50d16e3..ef2a41c 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -609,15 +609,26 @@ static struct attribute *runtime_attrs[] = {
 	&dev_attr_control.attr,
 	&dev_attr_runtime_suspended_time.attr,
 	&dev_attr_runtime_active_time.attr,
-	&dev_attr_autosuspend_delay_ms.attr,
 #endif /* CONFIG_PM_RUNTIME */
 	NULL,
 };
+
 static struct attribute_group pm_runtime_attr_group = {
 	.name	= power_group_name,
 	.attrs	= runtime_attrs,
 };
 
+static struct attribute *runtime_autosuspend_attrs[] = {
+#ifdef CONFIG_PM_RUNTIME
+	&dev_attr_autosuspend_delay_ms.attr,
+#endif /* CONFIG_PM_RUNTIME */
+};
+
+static struct attribute_group pm_runtime_autosuspend_attr_group = {
+	.name	= power_group_name,
+	.attrs	= runtime_autosuspend_attrs,
+};
+
 static struct attribute *pm_qos_latency_attrs[] = {
 #ifdef CONFIG_PM_RUNTIME
 	&dev_attr_pm_qos_resume_latency_us.attr,
@@ -671,6 +682,12 @@ int dpm_sysfs_add(struct device *dev)
 	return rc;
 }
 
+int dpm_autosuspend_sysfs_add(struct device *dev)
+{
+	return sysfs_merge_group(&dev->kobj,
+			&pm_runtime_autosuspend_attr_group);
+}
+
 int wakeup_sysfs_add(struct device *dev)
 {
 	return sysfs_merge_group(&dev->kobj, &pm_wakeup_attr_group);
@@ -712,3 +729,9 @@ void dpm_sysfs_remove(struct device *dev)
 	sysfs_unmerge_group(&dev->kobj, &pm_wakeup_attr_group);
 	sysfs_remove_group(&dev->kobj, &pm_attr_group);
 }
+
+void dpm_autosuspend_sysfs_remove(struct device *dev)
+{
+	sysfs_unmerge_group(&dev->kobj,
+			&pm_runtime_autosuspend_attr_group);
+}
-- 
1.8.1.rc1.5.g7e0651a

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: SYSFS "errors"

[Felipe Balbi]

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

+netdev

On Tue, Feb 19, 2013 at 08:41:47AM +0100, Alexander Stein wrote:
> On Monday 18 February 2013 09:49:16, Greg KH wrote:
> > > Invalid argument - /sys/devices/virtual/net/lo/speed
> > > Invalid argument - /sys/devices/virtual/net/lo/duplex
> 
> This is even true for ethernet devices which do not have a link currently or non-ethernet devices (e.g. CAN).
> > LANG=C cat /sys/class/net/enp1s8/speed
> > cat: /sys/class/net/enp1s8/speed: Invalid argument
> > ethtool enp1s8 | grep -A1 Speed
> >         Speed: Unknown!
> >         Duplex: Unknown! (255)
> > LANG=C cat /sys/class/net/can0/speed
> > cat: /sys/class/net/can0/speed: Invalid argument
> 
> Regards,
> Alexander
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]