| Message ID | 20130218221306.GA21493@pd.tnic (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 linux-pm" 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 linux-pm" 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.
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) {