| Message ID | 1427474362-3903-3-git-send-email-clg@fr.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Michael Ellerman |
| Headers | show |
On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote: > The opal sensor mutex protects the opal_sensor_read call which > can return a OPAL_BUSY code on IBM Power systems if a previous > request is in progress. > > This can be handled at user level with a retry. It can, but how does it actually look in practice? It looks like the only use of opal_get_sensor_data() is show_sensor() in drivers/hwmon/ibmpowernv.c. Because that's a sysfs attribute folks will be generally just dumping that with cat, or reading it in a shell script, neither of which will cope nicely with EBUSY I think? cheers
On 03/30/2015 04:09 AM, Michael Ellerman wrote: > On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote: >> The opal sensor mutex protects the opal_sensor_read call which >> can return a OPAL_BUSY code on IBM Power systems if a previous >> request is in progress. >> >> This can be handled at user level with a retry. > > It can, but how does it actually look in practice? > > It looks like the only use of opal_get_sensor_data() is show_sensor() in > drivers/hwmon/ibmpowernv.c. > > Because that's a sysfs attribute folks will be generally just dumping > that with cat, or reading it in a shell script, neither of which will > cope nicely with EBUSY I think? It won't, I agree but it should only happen when running concurrent cat commands on the hwmon sysfs files. The event should be rare enough. Anyhow, this is not a big issue. We can drop that patch. The real "issue" is the time it takes to get some values back from the FSP. This is what user space has been most surprised about. Thanks, C.
On Mon, 2015-03-30 at 08:51 +0200, Cedric Le Goater wrote: > On 03/30/2015 04:09 AM, Michael Ellerman wrote: > > On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote: > >> The opal sensor mutex protects the opal_sensor_read call which > >> can return a OPAL_BUSY code on IBM Power systems if a previous > >> request is in progress. > >> > >> This can be handled at user level with a retry. > > > > It can, but how does it actually look in practice? > > > > It looks like the only use of opal_get_sensor_data() is show_sensor() in > > drivers/hwmon/ibmpowernv.c. > > > > Because that's a sysfs attribute folks will be generally just dumping > > that with cat, or reading it in a shell script, neither of which will > > cope nicely with EBUSY I think? > > It won't, I agree but it should only happen when running concurrent cat > commands on the hwmon sysfs files. The event should be rare enough. Rare enough maybe, but a real pain in the .. to cope with in a shell script if you're trying to automate something. > Anyhow, this is not a big issue. We can drop that patch. The real "issue" > is the time it takes to get some values back from the FSP. This is what > user space has been most surprised about. OK. The other option would be to move the mutex into the sysfs show routine, so only that is synchronous. That would give you nice behaviour from cat, ie. it would sleep on contention but still be killable with ctrl-c. cheers
On 03/30/2015 08:59 AM, Michael Ellerman wrote: > On Mon, 2015-03-30 at 08:51 +0200, Cedric Le Goater wrote: >> On 03/30/2015 04:09 AM, Michael Ellerman wrote: >>> On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote: >>>> The opal sensor mutex protects the opal_sensor_read call which >>>> can return a OPAL_BUSY code on IBM Power systems if a previous >>>> request is in progress. >>>> >>>> This can be handled at user level with a retry. >>> >>> It can, but how does it actually look in practice? >>> >>> It looks like the only use of opal_get_sensor_data() is show_sensor() in >>> drivers/hwmon/ibmpowernv.c. >>> >>> Because that's a sysfs attribute folks will be generally just dumping >>> that with cat, or reading it in a shell script, neither of which will >>> cope nicely with EBUSY I think? >> >> It won't, I agree but it should only happen when running concurrent cat >> commands on the hwmon sysfs files. The event should be rare enough. > > Rare enough maybe, but a real pain in the .. to cope with in a shell script if > you're trying to automate something. > >> Anyhow, this is not a big issue. We can drop that patch. The real "issue" >> is the time it takes to get some values back from the FSP. This is what >> user space has been most surprised about. > > OK. The other option would be to move the mutex into the sysfs show routine, so > only that is synchronous. That would give you nice behaviour from cat, ie. it > would sleep on contention but still be killable with ctrl-c. Let's keep it how it is and see if it is possible to the improve OPAL side first. I will send you an updated patchset shortly. Thanks for the review. C.
Index: linux.git/arch/powerpc/platforms/powernv/opal-sensor.c =================================================================== --- linux.git.orig/arch/powerpc/platforms/powernv/opal-sensor.c +++ linux.git/arch/powerpc/platforms/powernv/opal-sensor.c @@ -19,13 +19,10 @@ */ #include <linux/delay.h> -#include <linux/mutex.h> #include <linux/of_platform.h> #include <asm/opal.h> #include <asm/machdep.h> -static DEFINE_MUTEX(opal_sensor_mutex); - /* * This will return sensor information to driver based on the requested sensor * handle. A handle is an opaque id for the powernv, read by the driver from the @@ -40,11 +37,9 @@ int opal_get_sensor_data(u32 sensor_hndl token = opal_async_get_token_interruptible(); if (token < 0) { pr_err("%s: Couldn't get the token, returning\n", __func__); - ret = token; - goto out; + return token; } - mutex_lock(&opal_sensor_mutex); ret = opal_sensor_read(sensor_hndl, token, &data); switch (ret) { case OPAL_ASYNC_COMPLETION: @@ -70,9 +65,7 @@ int opal_get_sensor_data(u32 sensor_hndl } out_token: - mutex_unlock(&opal_sensor_mutex); opal_async_release_token(token); -out: return ret; } EXPORT_SYMBOL_GPL(opal_get_sensor_data);
The opal sensor mutex protects the opal_sensor_read call which can return a OPAL_BUSY code on IBM Power systems if a previous request is in progress. This can be handled at user level with a retry. Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> --- Changes since v2 : - removed a goto label arch/powerpc/platforms/powernv/opal-sensor.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)