SYSFS "errors"

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:

--

--

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.

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.