From: Cédric Le Goater <hidden> Date: 2015-03-25 17:50:45
Currently, when a sensor value is read, the kernel calls OPAL, which in
turn builds a message for the FSP, and waits for a message back.
The new device tree for OPAL sensors [1] adds new sensors that can be
read synchronously (core temperatures for instance) and that don't need
to wait for a response.
This patch modifies the opal call to accept an OPAL_SUCCESS return value
and cover the case above.
[1] https://lists.ozlabs.org/pipermail/skiboot/2015-March/000639.html
Signed-off-by: Cédric Le Goater <redacted>
---
We still uselessly reserve a token (for the response) and take a
lock, which might raise the need of a new 'opal_sensor_read_sync'
call.
arch/powerpc/platforms/powernv/opal-sensor.c | 29 +++++++++++++++++---------
1 file changed, 19 insertions(+), 10 deletions(-)
@@ -46,18 +46,27 @@ int opal_get_sensor_data(u32 sensor_hndl, u32 *sensor_data)mutex_lock(&opal_sensor_mutex);ret=opal_sensor_read(sensor_hndl,token,&data);-if(ret!=OPAL_ASYNC_COMPLETION)-gotoout_token;+switch(ret){+caseOPAL_ASYNC_COMPLETION:+ret=opal_async_wait_response(token,&msg);+if(ret){+pr_err("%s: Failed to wait for the async response, %d\n",+__func__,ret);+gotoout_token;+}-ret=opal_async_wait_response(token,&msg);-if(ret){-pr_err("%s: Failed to wait for the async response, %d\n",-__func__,ret);-gotoout_token;-}+ret=be64_to_cpu(msg.params[1]);++*sensor_data=be32_to_cpu(data);+break;-*sensor_data=be32_to_cpu(data);-ret=be64_to_cpu(msg.params[1]);+caseOPAL_SUCCESS:+*sensor_data=be32_to_cpu(data);+break;++default:+break;+}out_token:mutex_unlock(&opal_sensor_mutex);
From: Stewart Smith <hidden> Date: 2015-03-25 23:07:20
C=C3=A9dric Le Goater [off-list ref] writes:
Currently, when a sensor value is read, the kernel calls OPAL, which in
turn builds a message for the FSP, and waits for a message back.=20
The new device tree for OPAL sensors [1] adds new sensors that can be=20
read synchronously (core temperatures for instance) and that don't need=20
to wait for a response.
This patch modifies the opal call to accept an OPAL_SUCCESS return value
and cover the case above.
[1] https://lists.ozlabs.org/pipermail/skiboot/2015-March/000639.html
Signed-off-by: C=C3=A9dric Le Goater <redacted>
---
We still uselessly reserve a token (for the response) and take a
lock, which might raise the need of a new 'opal_sensor_read_sync'=20
call.
Actually.... why do we take a lock around the OPAL calls at all?
From: Cedric Le Goater <hidden> Date: 2015-03-26 09:45:09
On 03/26/2015 12:07 AM, Stewart Smith wrote:
Cédric Le Goater [off-list ref] writes:
quoted
Currently, when a sensor value is read, the kernel calls OPAL, which in
turn builds a message for the FSP, and waits for a message back.
The new device tree for OPAL sensors [1] adds new sensors that can be
read synchronously (core temperatures for instance) and that don't need
to wait for a response.
This patch modifies the opal call to accept an OPAL_SUCCESS return value
and cover the case above.
[1] https://lists.ozlabs.org/pipermail/skiboot/2015-March/000639.html
Signed-off-by: Cédric Le Goater <redacted>
---
We still uselessly reserve a token (for the response) and take a
lock, which might raise the need of a new 'opal_sensor_read_sync'
call.
Actually.... why do we take a lock around the OPAL calls at all?
The sensor service in OPAL only handles one FSP request at a time and
returns OPAL_BUSY if one is already in progress. The lock covers this case
but we could also remove it return EBUSY to the driver or even retry the
call. That might be dangerous though.
Changing OPAL to handle simultaneously multiple requests does not seem really
necessary, it won't speed up the communication with the FSP and that is the
main bottleneck.
C.
From: Cedric Le Goater <hidden> Date: 2015-03-26 12:58:48
On 03/26/2015 10:44 AM, Cedric Le Goater wrote:
On 03/26/2015 12:07 AM, Stewart Smith wrote:
quoted
Cédric Le Goater [off-list ref] writes:
quoted
Currently, when a sensor value is read, the kernel calls OPAL, which in
turn builds a message for the FSP, and waits for a message back.
The new device tree for OPAL sensors [1] adds new sensors that can be
read synchronously (core temperatures for instance) and that don't need
to wait for a response.
This patch modifies the opal call to accept an OPAL_SUCCESS return value
and cover the case above.
[1] https://lists.ozlabs.org/pipermail/skiboot/2015-March/000639.html
Signed-off-by: Cédric Le Goater <redacted>
---
We still uselessly reserve a token (for the response) and take a
lock, which might raise the need of a new 'opal_sensor_read_sync'
call.
Actually.... why do we take a lock around the OPAL calls at all?
The sensor service in OPAL only handles one FSP request at a time and
returns OPAL_BUSY if one is already in progress. The lock covers this case
but we could also remove it return EBUSY to the driver or even retry the
call. That might be dangerous though.
Changing OPAL to handle simultaneously multiple requests does not seem really
necessary, it won't speed up the communication with the FSP and that is the
main bottleneck.
opal_get_sensor_data() is mixing OPAL return codes and errnos. I will send
a v2 addressing this problem first.
C.
From: Cédric Le Goater <hidden> Date: 2015-03-26 16:04:57
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 <redacted>
---
arch/powerpc/platforms/powernv/opal-sensor.c | 5 -----
1 file changed, 5 deletions(-)
Index: linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
===================================================================
From: Cédric Le Goater <hidden> Date: 2015-03-26 16:04:57
Currently, when a sensor value is read, the kernel calls OPAL, which in
turn builds a message for the FSP, and waits for a message back.
The new device tree for OPAL sensors [1] adds new sensors that can be
read synchronously (core temperatures for instance) and that don't need
to wait for a response.
This patch modifies the opal call to accept an OPAL_SUCCESS return value
and cover the case above.
[1] https://lists.ozlabs.org/pipermail/skiboot/2015-March/000639.html
Signed-off-by: Cédric Le Goater <redacted>
---
We still uselessly reserve a token (for the response) and take a
lock, which might raise the need of a new 'opal_sensor_read_sync'
call.
arch/powerpc/platforms/powernv/opal-sensor.c | 31 ++++++++++++++++-----------
1 file changed, 19 insertions(+), 12 deletions(-)
Index: linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
===================================================================
@@ -78,21 +78,28 @@ int opal_get_sensor_data(u32 sensor_hndlmutex_lock(&opal_sensor_mutex);ret=opal_sensor_read(sensor_hndl,token,&data);-if(ret!=OPAL_ASYNC_COMPLETION){+switch(ret){+caseOPAL_ASYNC_COMPLETION:+ret=opal_async_wait_response(token,&msg);+if(ret){+pr_err("%s: Failed to wait for the async response, %d\n",+__func__,ret);+gotoout_token;+}++*sensor_data=be32_to_cpu(data);+ret=be64_to_cpu(msg.params[1]);ret=convert_opal_code(ret);-gotoout_token;-}+break;-ret=opal_async_wait_response(token,&msg);-if(ret){-pr_err("%s: Failed to wait for the async response, %d\n",-__func__,ret);-gotoout_token;-}+caseOPAL_SUCCESS:+*sensor_data=be32_to_cpu(data);+break;-*sensor_data=be32_to_cpu(data);-ret=be64_to_cpu(msg.params[1]);-ret=convert_opal_code(ret);+default:+ret=convert_opal_code(ret);+break;+}out_token:mutex_unlock(&opal_sensor_mutex);
From: Cédric Le Goater <hidden> Date: 2015-03-26 16:04:57
OPAL has its own list of return codes. The patch provides a translation
of such codes in errnos for the opal_sensor_read call.
Signed-off-by: Cédric Le Goater <redacted>
---
arch/powerpc/platforms/powernv/opal-sensor.c | 37 ++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
Index: linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
===================================================================
From: Stewart Smith <hidden> Date: 2015-03-27 06:05:54
Cedric Le Goater [off-list ref] writes:
The sensor service in OPAL only handles one FSP request at a time and
returns OPAL_BUSY if one is already in progress. The lock covers this case
but we could also remove it return EBUSY to the driver or even retry the
call. That might be dangerous though.
Retrying the call should be okay.
Just because FSP wants to do things serially doesn't mean non-FSP does :)
Changing OPAL to handle simultaneously multiple requests does not seem really
necessary, it won't speed up the communication with the FSP and that is the
main bottleneck.
Only on FSP systems though, and all of the OpenPower machines don't have
FSPs :)
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2015-03-27 09:59:36
On Thu, 2015-26-03 at 16:04:45 UTC, =?utf-8?q?C=C3=A9dric_Le_Goater?= wrote:
quoted hunk
OPAL has its own list of return codes. The patch provides a translation
of such codes in errnos for the opal_sensor_read call.
Signed-off-by: C��dric Le Goater <redacted>
---
arch/powerpc/platforms/powernv/opal-sensor.c | 37 ++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
Index: linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
===================================================================
+static int convert_opal_code(int ret)
+{
+ switch (ret) {
+ case OPAL_SUCCESS: return 0;
+ case OPAL_PARAMETER: return -EINVAL;
+ case OPAL_UNSUPPORTED: return -ENOSYS;
+ case OPAL_ASYNC_COMPLETION: return -EAGAIN;
+ case OPAL_BUSY_EVENT: return -EBUSY;
+ case OPAL_NO_MEM: return -ENOMEM;
+ case OPAL_HARDWARE: return -ENOENT;
+ case OPAL_INTERNAL_ERROR: return -EIO;
+ default: return -EIO;
+ }
+}
That looks a bit familiar :)
static int rtas_error_rc(int rtas_rc)
{
int rc;
switch (rtas_rc) {
case -1: /* Hardware Error */
rc = -EIO;
break;
case -3: /* Bad indicator/domain/etc */
rc = -EINVAL;
break;
case -9000: /* Isolation error */
rc = -EFAULT;
break;
case -9001: /* Outstanding TCE/PTE */
rc = -EEXIST;
break;
case -9002: /* No usable slot */
rc = -ENODEV;
break;
default:
printk(KERN_ERR "%s: unexpected RTAS error %d\n",
__func__, rtas_rc);
rc = -ERANGE;
break;
}
return rc;
}
But I guess we still should have it.
Can you put it in opal.h and give it a better name, maybe opal_error_code() ?
quoted hunk
/*
* 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
@@ -46,8 +78,10 @@ int opal_get_sensor_data(u32 sensor_hndl mutex_lock(&opal_sensor_mutex); ret = opal_sensor_read(sensor_hndl, token, &data);- if (ret != OPAL_ASYNC_COMPLETION)+ if (ret != OPAL_ASYNC_COMPLETION) {+ ret = convert_opal_code(ret); goto out_token;+ } ret = opal_async_wait_response(token, &msg); if (ret) {
@@ -58,6 +92,7 @@ int opal_get_sensor_data(u32 sensor_hndl *sensor_data = be32_to_cpu(data); ret = be64_to_cpu(msg.params[1]);+ ret = convert_opal_code(ret);
I'd do:
ret = convert_opal_code(be64_to_cpu(msg.params[1]));
cheers
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2015-03-27 10:36:27
On Fri, 2015-03-27 at 20:59 +1100, Michael Ellerman wrote:
Can you put it in opal.h and give it a better name, maybe
opal_error_code() ?
Do we want it to be inlined all the time ? Feels more like something we
should have in opal.c
Also we only want to call it when we "forward" the error code up the
food chain, there are a number of cases where we look for specific OPAL
error codes.
Cheers,
Ben.
From: Cedric Le Goater <hidden> Date: 2015-03-27 10:39:27
On 03/27/2015 11:36 AM, Benjamin Herrenschmidt wrote:
On Fri, 2015-03-27 at 20:59 +1100, Michael Ellerman wrote:
quoted
Can you put it in opal.h and give it a better name, maybe
opal_error_code() ?
Do we want it to be inlined all the time ? Feels more like something we
should have in opal.c
Also we only want to call it when we "forward" the error code up the
food chain, there are a number of cases where we look for specific OPAL
error codes.
yes. the forward is not systematic. opal.c looks like a better place.
-ERANGE looks also better when the return code is unexpected.
C.
From: Cedric Le Goater <hidden> Date: 2015-03-27 10:45:09
On 03/27/2015 10:59 AM, Michael Ellerman wrote:
On Thu, 2015-26-03 at 16:04:45 UTC, =?utf-8?q?C=C3=A9dric_Le_Goater?= wrote:
quoted
OPAL has its own list of return codes. The patch provides a translation
of such codes in errnos for the opal_sensor_read call.
Signed-off-by: Cédric Le Goater <redacted>
---
arch/powerpc/platforms/powernv/opal-sensor.c | 37 ++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
Index: linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
===================================================================
+static int convert_opal_code(int ret)
+{
+ switch (ret) {
+ case OPAL_SUCCESS: return 0;
+ case OPAL_PARAMETER: return -EINVAL;
+ case OPAL_UNSUPPORTED: return -ENOSYS;
+ case OPAL_ASYNC_COMPLETION: return -EAGAIN;
+ case OPAL_BUSY_EVENT: return -EBUSY;
+ case OPAL_NO_MEM: return -ENOMEM;
+ case OPAL_HARDWARE: return -ENOENT;
+ case OPAL_INTERNAL_ERROR: return -EIO;
+ default: return -EIO;
+ }
+}
That looks a bit familiar :)
Ah ! I only looked in opal ...
static int rtas_error_rc(int rtas_rc)
{
int rc;
switch (rtas_rc) {
case -1: /* Hardware Error */
rc = -EIO;
break;
case -3: /* Bad indicator/domain/etc */
rc = -EINVAL;
break;
case -9000: /* Isolation error */
rc = -EFAULT;
break;
case -9001: /* Outstanding TCE/PTE */
rc = -EEXIST;
break;
case -9002: /* No usable slot */
rc = -ENODEV;
break;
default:
printk(KERN_ERR "%s: unexpected RTAS error %d\n",
__func__, rtas_rc);
rc = -ERANGE;
this a better code default value.
break;
}
return rc;
}
But I guess we still should have it.
Can you put it in opal.h and give it a better name, maybe opal_error_code() ?
Sure. I will change the name but opal.c looks better, knowing that opal.h is
shared with skiboot.
quoted
/*
* 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
@@ -46,8 +78,10 @@ int opal_get_sensor_data(u32 sensor_hndl mutex_lock(&opal_sensor_mutex); ret = opal_sensor_read(sensor_hndl, token, &data);- if (ret != OPAL_ASYNC_COMPLETION)+ if (ret != OPAL_ASYNC_COMPLETION) {+ ret = convert_opal_code(ret); goto out_token;+ } ret = opal_async_wait_response(token, &msg); if (ret) {
@@ -58,6 +92,7 @@ int opal_get_sensor_data(u32 sensor_hndl *sensor_data = be32_to_cpu(data); ret = be64_to_cpu(msg.params[1]);+ ret = convert_opal_code(ret);
I'd do:
ret = convert_opal_code(be64_to_cpu(msg.params[1]));
From: Cédric Le Goater <hidden> Date: 2015-03-27 16:39:36
OPAL has its own list of return codes. The patch provides a translation
of such codes in errnos for the opal_sensor_read call, and possibly
others if needed.
Signed-off-by: Cédric Le Goater <redacted>
---
Changes since v2 :
- renamed and moved the routine to opal.[ch]
- changed default value to ERANGE like rtas
arch/powerpc/include/asm/opal.h | 2 ++
arch/powerpc/platforms/powernv/opal-sensor.c | 6 ++++--
arch/powerpc/platforms/powernv/opal.c | 17 +++++++++++++++++
3 files changed, 23 insertions(+), 2 deletions(-)
Index: linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
===================================================================
From: Cédric Le Goater <hidden> Date: 2015-03-27 16:39:54
Currently, when a sensor value is read, the kernel calls OPAL, which in
turn builds a message for the FSP, and waits for a message back.
The new device tree for OPAL sensors [1] adds new sensors that can be
read synchronously (core temperatures for instance) and that don't need
to wait for a response.
This patch modifies the opal call to accept an OPAL_SUCCESS return value
and cover the case above.
[1] https://lists.ozlabs.org/pipermail/skiboot/2015-March/000639.html
Signed-off-by: Cédric Le Goater <redacted>
---
We still uselessly reserve a token (for the response) and take a
lock, which might raise the need of a new 'opal_sensor_read_sync'
call.
Changes since v2 :
- merged the return code assignments in one call
arch/powerpc/platforms/powernv/opal-sensor.c | 32 ++++++++++++++++-----------
1 file changed, 20 insertions(+), 12 deletions(-)
Index: linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
===================================================================
@@ -46,20 +46,28 @@ int opal_get_sensor_data(u32 sensor_hndlmutex_lock(&opal_sensor_mutex);ret=opal_sensor_read(sensor_hndl,token,&data);-if(ret!=OPAL_ASYNC_COMPLETION){-ret=opal_error_code(ret);-gotoout_token;-}+switch(ret){+caseOPAL_ASYNC_COMPLETION:+ret=opal_async_wait_response(token,&msg);+if(ret){+pr_err("%s: Failed to wait for the async response, %d\n",+__func__,ret);+gotoout_token;+}-ret=opal_async_wait_response(token,&msg);-if(ret){-pr_err("%s: Failed to wait for the async response, %d\n",-__func__,ret);-gotoout_token;-}+ret=opal_error_code(be64_to_cpu(msg.params[1]));+*sensor_data=be32_to_cpu(data);+break;-*sensor_data=be32_to_cpu(data);-ret=opal_error_code(be64_to_cpu(msg.params[1]));+caseOPAL_SUCCESS:+ret=0;+*sensor_data=be32_to_cpu(data);+break;++default:+ret=opal_error_code(ret);+break;+}out_token:mutex_unlock(&opal_sensor_mutex);
From: Cédric Le Goater <hidden> Date: 2015-03-27 16:40:06
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 <redacted>
---
Changes since v2 :
- removed a goto label
arch/powerpc/platforms/powernv/opal-sensor.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
Index: linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
===================================================================
@@ -40,11 +37,9 @@ int opal_get_sensor_data(u32 sensor_hndltoken=opal_async_get_token_interruptible();if(token<0){pr_err("%s: Couldn't get the token, returning\n",__func__);-ret=token;-gotoout;+returntoken;}-mutex_lock(&opal_sensor_mutex);ret=opal_sensor_read(sensor_hndl,token,&data);switch(ret){caseOPAL_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:returnret;}EXPORT_SYMBOL_GPL(opal_get_sensor_data);
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2015-03-30 02:05:12
On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote:
quoted hunk
OPAL has its own list of return codes. The patch provides a translation
of such codes in errnos for the opal_sensor_read call, and possibly
others if needed.
Index: linux.git/arch/powerpc/platforms/powernv/opal.c
===================================================================
You shouldn't use ENOSYS here, that should only ever mean "no such syscall",
otherwise you get very confusing results like read() returning ENOSYS.
+ case OPAL_ASYNC_COMPLETION: return -EAGAIN;
EAGAIN means "try what you did again", I don't think that's what
ASYNC_COMPLETION means, is it? It looks like it means, "don't try again, but
you need to wait for the result to be ready".
I'm not sure it maps well to any of the Linux codes, maybe EINPROGRESS ?
+ case OPAL_BUSY_EVENT: return -EBUSY;
Yep.
+ case OPAL_NO_MEM: return -ENOMEM;
Yep.
+ case OPAL_HARDWARE: return -ENOENT;
This is another one which I think you shouldn't use as it can lead to confusing
results at user level. eg:
$ cat /sysfs/some/file
Error: No such file or directory
Huh?
Looking at the skiboot code this looks like EIO is a good match.
I'm not sure about this one honestly, it means "Math result not representable".
I suspect the reason RTAS chose it was just that it's not EINVAL.
This should probably also just be EIO.
cheers
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2015-03-30 02:09:58
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
From: Cedric Le Goater <hidden> Date: 2015-03-30 06:37:21
On 03/30/2015 04:05 AM, Michael Ellerman wrote:
On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote:
quoted
OPAL has its own list of return codes. The patch provides a translation
of such codes in errnos for the opal_sensor_read call, and possibly
others if needed.
Index: linux.git/arch/powerpc/platforms/powernv/opal.c
===================================================================
He. Initially, I didn't put a case for SUCCESS, but we have code doing :
ret = be64_to_cpu(msg.params[1]);
quoted
+ case OPAL_PARAMETER: return -EINVAL;
Yep.
quoted
+ case OPAL_UNSUPPORTED: return -ENOSYS;
You shouldn't use ENOSYS here, that should only ever mean "no such syscall",
otherwise you get very confusing results like read() returning ENOSYS.
Indeed. How about ENODEV then ?
quoted
+ case OPAL_ASYNC_COMPLETION: return -EAGAIN;
EAGAIN means "try what you did again", I don't think that's what
ASYNC_COMPLETION means, is it? It looks like it means, "don't try again, but
you need to wait for the result to be ready".
I'm not sure it maps well to any of the Linux codes, maybe EINPROGRESS ?
Yes. This is better.
quoted
+ case OPAL_BUSY_EVENT: return -EBUSY;
Yep.
quoted
+ case OPAL_NO_MEM: return -ENOMEM;
Yep.
quoted
+ case OPAL_HARDWARE: return -ENOENT;
This is another one which I think you shouldn't use as it can lead to confusing
results at user level. eg:
$ cat /sysfs/some/file
Error: No such file or directory
Huh?
Looking at the skiboot code this looks like EIO is a good match.
I'm not sure about this one honestly, it means "Math result not representable".
I suspect the reason RTAS chose it was just that it's not EINVAL.
This should probably also just be EIO.
From: Cedric Le Goater <hidden> Date: 2015-03-30 06:51:14
On 03/30/2015 04:09 AM, Michael Ellerman wrote:
On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote:
quoted
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.
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2015-03-30 06:54:16
On Mon, 2015-03-30 at 08:37 +0200, Cedric Le Goater wrote:
On 03/30/2015 04:05 AM, Michael Ellerman wrote:
quoted
On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote:
quoted
OPAL has its own list of return codes. The patch provides a translation
of such codes in errnos for the opal_sensor_read call, and possibly
others if needed.
+ case OPAL_UNSUPPORTED: return -ENOSYS;
You shouldn't use ENOSYS here, that should only ever mean "no such syscall",
otherwise you get very confusing results like read() returning ENOSYS.
Indeed. How about ENODEV then ?
That can also be confusing from userspace.
I think it's probably best just to use EIO, as far as userspace is concerned if
the kernel lets it call an unsupported OPAL routine that is more or less a
kernel bug.
cheers
From: Cedric Le Goater <hidden> Date: 2015-03-30 06:56:29
On 03/30/2015 08:54 AM, Michael Ellerman wrote:
On Mon, 2015-03-30 at 08:37 +0200, Cedric Le Goater wrote:
quoted
On 03/30/2015 04:05 AM, Michael Ellerman wrote:
quoted
On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote:
quoted
OPAL has its own list of return codes. The patch provides a translation
of such codes in errnos for the opal_sensor_read call, and possibly
others if needed.
+ case OPAL_UNSUPPORTED: return -ENOSYS;
You shouldn't use ENOSYS here, that should only ever mean "no such syscall",
otherwise you get very confusing results like read() returning ENOSYS.
Indeed. How about ENODEV then ?
That can also be confusing from userspace.
I think it's probably best just to use EIO, as far as userspace is concerned if
the kernel lets it call an unsupported OPAL routine that is more or less a
kernel bug.
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2015-03-30 06:59:37
On Mon, 2015-03-30 at 08:51 +0200, Cedric Le Goater wrote:
On 03/30/2015 04:09 AM, Michael Ellerman wrote:
quoted
On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote:
quoted
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
From: Cedric Le Goater <hidden> Date: 2015-03-30 10:06:08
On 03/30/2015 08:59 AM, Michael Ellerman wrote:
On Mon, 2015-03-30 at 08:51 +0200, Cedric Le Goater wrote:
quoted
On 03/30/2015 04:09 AM, Michael Ellerman wrote:
quoted
On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote:
quoted
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.
quoted
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.
From: Cédric Le Goater <hidden> Date: 2015-03-30 10:06:54
OPAL has its own list of return codes. The patch provides a translation
of such codes in errnos for the opal_sensor_read call, and possibly
others if needed.
Signed-off-by: Cédric Le Goater <redacted>
---
Changes since v3 :
- reworked the return codes a little as suggested by Michael Ellerman
Changes since v2 :
- renamed and moved the routine to opal.[ch]
- changed default value to ERANGE like rtas
arch/powerpc/include/asm/opal.h | 2 ++
arch/powerpc/platforms/powernv/opal-sensor.c | 6 ++++--
arch/powerpc/platforms/powernv/opal.c | 19 +++++++++++++++++++
3 files changed, 25 insertions(+), 2 deletions(-)
Index: linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
===================================================================
From: Cédric Le Goater <hidden> Date: 2015-03-30 10:06:54
Currently, when a sensor value is read, the kernel calls OPAL, which in
turn builds a message for the FSP, and waits for a message back.
The new device tree for OPAL sensors [1] adds new sensors that can be
read synchronously (core temperatures for instance) and that don't need
to wait for a response.
This patch modifies the opal call to accept an OPAL_SUCCESS return value
and cover the case above.
[1] https://lists.ozlabs.org/pipermail/skiboot/2015-March/000639.html
Signed-off-by: Cédric Le Goater <redacted>
---
We still uselessly reserve a token (for the response) and take a
lock, which might raise the need of a new 'opal_sensor_read_sync'
call.
Changes since v2 :
- merged the return code assignments in one call
arch/powerpc/platforms/powernv/opal-sensor.c | 32 ++++++++++++++++-----------
1 file changed, 20 insertions(+), 12 deletions(-)
Index: linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
===================================================================
@@ -46,20 +46,28 @@ int opal_get_sensor_data(u32 sensor_hndlmutex_lock(&opal_sensor_mutex);ret=opal_sensor_read(sensor_hndl,token,&data);-if(ret!=OPAL_ASYNC_COMPLETION){-ret=opal_error_code(ret);-gotoout_token;-}+switch(ret){+caseOPAL_ASYNC_COMPLETION:+ret=opal_async_wait_response(token,&msg);+if(ret){+pr_err("%s: Failed to wait for the async response, %d\n",+__func__,ret);+gotoout_token;+}-ret=opal_async_wait_response(token,&msg);-if(ret){-pr_err("%s: Failed to wait for the async response, %d\n",-__func__,ret);-gotoout_token;-}+ret=opal_error_code(be64_to_cpu(msg.params[1]));+*sensor_data=be32_to_cpu(data);+break;-*sensor_data=be32_to_cpu(data);-ret=opal_error_code(be64_to_cpu(msg.params[1]));+caseOPAL_SUCCESS:+ret=0;+*sensor_data=be32_to_cpu(data);+break;++default:+ret=opal_error_code(ret);+break;+}out_token:mutex_unlock(&opal_sensor_mutex);