[PATCH] powerpc/powernv: handle OPAL_SUCCESS return in opal_sensor_read

[PATCH] powerpc/powernv: handle OPAL_SUCCESS return in opal_sensor_read

[Cédric Le Goater]

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 <clg@fr.ibm.com>
---

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

diff --git a/arch/powerpc/platforms/powernv/opal-sensor.c b/arch/powerpc/platforms/powernv/opal-sensor.c
index 4ab67ef7abc9..99d6d9a371ab 100644
--- a/arch/powerpc/platforms/powernv/opal-sensor.c
+++ b/arch/powerpc/platforms/powernv/opal-sensor.c
@@ -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)
-		goto out_token;
+	switch (ret) {
+	case OPAL_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);
+			goto out_token;
+		}
 
-	ret = opal_async_wait_response(token, &msg);
-	if (ret) {
-		pr_err("%s: Failed to wait for the async response, %d\n",
-				__func__, ret);
-		goto out_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]);
+	case OPAL_SUCCESS:
+		*sensor_data = be32_to_cpu(data);
+		break;
+
+	default:
+		break;
+	}
 
 out_token:
 	mutex_unlock(&opal_sensor_mutex);
-- 
1.7.10.4

Re: [PATCH] powerpc/powernv: handle OPAL_SUCCESS return in opal_sensor_read

[Stewart Smith]

C=C3=A9dric Le Goater <clg@fr.ibm.com> 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 <clg@fr.ibm.com>
> ---
>
>  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?

Re: [PATCH] powerpc/powernv: handle OPAL_SUCCESS return in opal_sensor_read

[Cedric Le Goater]

On 03/26/2015 12:07 AM, Stewart Smith wrote:
> Cédric Le Goater <clg@fr.ibm.com> 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. 
>>
>> 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 <clg@fr.ibm.com>
>> ---
>>
>>  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.


 

Re: [PATCH] powerpc/powernv: handle OPAL_SUCCESS return in opal_sensor_read

[Cedric Le Goater]

On 03/26/2015 10:44 AM, Cedric Le Goater wrote:
> On 03/26/2015 12:07 AM, Stewart Smith wrote:
>> Cédric Le Goater <clg@fr.ibm.com> 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. 
>>>
>>> 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 <clg@fr.ibm.com>
>>> ---
>>>
>>>  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. 

[PATCH v2 1/3] powerpc/powernv: convert codes returned by OPAL calls

[Cédric Le Goater]

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 <clg@fr.ibm.com>
---
 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
===================================================================
--- linux.git.orig/arch/powerpc/platforms/powernv/opal-sensor.c
+++ linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
@@ -26,6 +26,38 @@
 
 static DEFINE_MUTEX(opal_sensor_mutex);
 
+
+/*
+ * opal_sensor_read() return codes
+ *
+ * OPAL_PARAMETER - invalid sensor handler
+ * OPAL_UNSUPPORTED - plateform does not support reading sensors.
+ *
+ *     in case of communication with the FSP on IBM systems
+ *
+ * OPAL_ASYNC_COMPLETION - a request was sent and an async completion will
+ *	be triggered with the @token argument
+ * OPAL_PARTIAL - the request completed but the data returned is invalid
+ * OPAL_BUSY_EVENT - a previous request is still pending
+ * OPAL_NO_MEM - allocation failed
+ * OPAL_INTERNAL_ERROR - communication failure with the FSP
+ * OPAL_HARDWARE - FSP is not available
+ */
+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;
+	}
+}
+
 /*
  * 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);
 
 out_token:
 	mutex_unlock(&opal_sensor_mutex);

[PATCH v2 2/3] powerpc/powernv: handle OPAL_SUCCESS return in opal_sensor_read

[Cédric Le Goater]

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 <clg@fr.ibm.com>
---

 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
===================================================================
--- linux.git.orig/arch/powerpc/platforms/powernv/opal-sensor.c
+++ linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
@@ -78,21 +78,28 @@ 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) {
+	switch (ret) {
+	case OPAL_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);
+			goto out_token;
+		}
+
+		*sensor_data = be32_to_cpu(data);
+		ret = be64_to_cpu(msg.params[1]);
 		ret = convert_opal_code(ret);
-		goto out_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);
-		goto out_token;
-	}
+	case OPAL_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);

[PATCH v2 3/3] powerpc/powernv: remove opal_sensor_mutex

[Cédric Le Goater]

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>
---
 arch/powerpc/platforms/powernv/opal-sensor.c |    5 -----
 1 file changed, 5 deletions(-)

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
@@ -24,9 +24,6 @@
 #include <asm/opal.h>
 #include <asm/machdep.h>
 
-static DEFINE_MUTEX(opal_sensor_mutex);
-
-
 /*
  * opal_sensor_read() return codes
  *
@@ -76,7 +73,6 @@ int opal_get_sensor_data(u32 sensor_hndl
 		goto out;
 	}
 
-	mutex_lock(&opal_sensor_mutex);
 	ret = opal_sensor_read(sensor_hndl, token, &data);
 	switch (ret) {
 	case OPAL_ASYNC_COMPLETION:
@@ -102,7 +98,6 @@ int opal_get_sensor_data(u32 sensor_hndl
 	}
 
 out_token:
-	mutex_unlock(&opal_sensor_mutex);
 	opal_async_release_token(token);
 out:
 	return ret;

Re: [v2,1/3] powerpc/powernv: convert codes returned by OPAL calls

[Michael Ellerman]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2587 bytes --]

On Thu, 2015-26-03 at 16:04:45 UTC, =?utf-8?q?C=C3=A9dric_Le_Goater?= wrote:
> 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 <clg@fr.ibm.com>
> ---
>  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
> ===================================================================
> --- linux.git.orig/arch/powerpc/platforms/powernv/opal-sensor.c
> +++ linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
> @@ -26,6 +26,38 @@
  
> +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() ?

>  /*
>   * 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

Re: [Skiboot] [v2, 1/3] powerpc/powernv: convert codes returned by OPAL calls

[Benjamin Herrenschmidt]

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.

Re: [Skiboot] [v2, 1/3] powerpc/powernv: convert codes returned by OPAL calls

[Cedric Le Goater]

On 03/27/2015 11:36 AM, Benjamin Herrenschmidt wrote:
> 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.

yes. the forward is not systematic. opal.c looks like a better place.

-ERANGE looks also better when the return code is unexpected. 

C.

Re: [v2,1/3] powerpc/powernv: convert codes returned by OPAL calls

[Cedric Le Goater]

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:
>> 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 <clg@fr.ibm.com>
>> ---
>>  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
>> ===================================================================
>> --- linux.git.orig/arch/powerpc/platforms/powernv/opal-sensor.c
>> +++ linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
>> @@ -26,6 +26,38 @@
>   
>> +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.

> 
>>  /*
>>   * 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]));

Yes. the double 'ret =' is ugly.

Thanks,

C. 

[PATCH v3 1/3] powerpc/powernv: convert codes returned by OPAL calls

[Cédric Le Goater]

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 <clg@fr.ibm.com>
---

 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
===================================================================
--- linux.git.orig/arch/powerpc/platforms/powernv/opal-sensor.c
+++ linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
@@ -46,8 +46,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 = opal_error_code(ret);
 		goto out_token;
+	}
 
 	ret = opal_async_wait_response(token, &msg);
 	if (ret) {
@@ -57,7 +59,7 @@ int opal_get_sensor_data(u32 sensor_hndl
 	}
 
 	*sensor_data = be32_to_cpu(data);
-	ret = be64_to_cpu(msg.params[1]);
+	ret = opal_error_code(be64_to_cpu(msg.params[1]));
 
 out_token:
 	mutex_unlock(&opal_sensor_mutex);
Index: linux.git/arch/powerpc/include/asm/opal.h
===================================================================
--- linux.git.orig/arch/powerpc/include/asm/opal.h
+++ linux.git/arch/powerpc/include/asm/opal.h
@@ -983,6 +983,8 @@ struct opal_sg_list *opal_vmalloc_to_sg_
 					     unsigned long vmalloc_size);
 void opal_free_sg_list(struct opal_sg_list *sg);
 
+extern int opal_error_code(int rc);
+
 /*
  * Dump region ID range usable by the OS
  */
Index: linux.git/arch/powerpc/platforms/powernv/opal.c
===================================================================
--- linux.git.orig/arch/powerpc/platforms/powernv/opal.c
+++ linux.git/arch/powerpc/platforms/powernv/opal.c
@@ -894,6 +894,23 @@ void opal_free_sg_list(struct opal_sg_li
 	}
 }
 
+int opal_error_code(int rc)
+{
+	switch (rc) {
+	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:
+		pr_err("%s: unexpected OPAL error %d\n", __func__, rc);
+		return -ERANGE;
+	}
+}
+
 EXPORT_SYMBOL_GPL(opal_poll_events);
 EXPORT_SYMBOL_GPL(opal_rtc_read);
 EXPORT_SYMBOL_GPL(opal_rtc_write);

Re: [PATCH v3 1/3] powerpc/powernv: convert codes returned by OPAL calls

[Michael Ellerman]

On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote:
> 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
> ===================================================================
> --- linux.git.orig/arch/powerpc/platforms/powernv/opal.c
> +++ linux.git/arch/powerpc/platforms/powernv/opal.c
> @@ -894,6 +894,23 @@ void opal_free_sg_list(struct opal_sg_li
>  	}
>  }
>  
> +int opal_error_code(int rc)
> +{
> +	switch (rc) {
> +	case OPAL_SUCCESS:		return 0;

Obviously correct.

> +	case OPAL_PARAMETER:		return -EINVAL;

Yep.

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

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

> +	case OPAL_INTERNAL_ERROR:	return -EIO;

Yeah as good as anything I guess.

> +	default:
> +		pr_err("%s: unexpected OPAL error %d\n", __func__, rc);
> +		return -ERANGE;

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

Re: [PATCH v3 1/3] powerpc/powernv: convert codes returned by OPAL calls

[Cedric Le Goater]

On 03/30/2015 04:05 AM, Michael Ellerman wrote:
> On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote:
>> 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
>> ===================================================================
>> --- linux.git.orig/arch/powerpc/platforms/powernv/opal.c
>> +++ linux.git/arch/powerpc/platforms/powernv/opal.c
>> @@ -894,6 +894,23 @@ void opal_free_sg_list(struct opal_sg_li
>>  	}
>>  }
>>  
>> +int opal_error_code(int rc)
>> +{
>> +	switch (rc) {
>> +	case OPAL_SUCCESS:		return 0;
> 
> Obviously correct.

He. Initially, I didn't put a case for SUCCESS, but we have code doing :

	ret = be64_to_cpu(msg.params[1]);


>> +	case OPAL_PARAMETER:		return -EINVAL;
> 
> Yep.
> 
>> +	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 ? 

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

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

ok. 

>> +	case OPAL_INTERNAL_ERROR:	return -EIO;
> 
> Yeah as good as anything I guess.
> 
>> +	default:
>> +		pr_err("%s: unexpected OPAL error %d\n", __func__, rc);
>> +		return -ERANGE;
> 
> 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.

ok. I will change it. 

Thanks,

C.

Re: [PATCH v3 1/3] powerpc/powernv: convert codes returned by OPAL calls

[Michael Ellerman]

On Mon, 2015-03-30 at 08:37 +0200, Cedric Le Goater wrote:
> On 03/30/2015 04:05 AM, Michael Ellerman wrote:
> > On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote:
> >> 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

Re: [PATCH v3 1/3] powerpc/powernv: convert codes returned by OPAL calls

[Cedric Le Goater]

On 03/30/2015 08:54 AM, Michael Ellerman wrote:
> On Mon, 2015-03-30 at 08:37 +0200, Cedric Le Goater wrote:
>> On 03/30/2015 04:05 AM, Michael Ellerman wrote:
>>> On Fri, 2015-03-27 at 17:39 +0100, Cédric Le Goater wrote:
>>>> 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.

OK. Will do.

Thanks,

C.

[PATCH v4 1/2] powerpc/powernv: convert codes returned by OPAL calls

[Cédric Le Goater]

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 <clg@fr.ibm.com>
---

 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
===================================================================
--- linux.git.orig/arch/powerpc/platforms/powernv/opal-sensor.c
+++ linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
@@ -46,8 +46,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 = opal_error_code(ret);
 		goto out_token;
+	}
 
 	ret = opal_async_wait_response(token, &msg);
 	if (ret) {
@@ -57,7 +59,7 @@ int opal_get_sensor_data(u32 sensor_hndl
 	}
 
 	*sensor_data = be32_to_cpu(data);
-	ret = be64_to_cpu(msg.params[1]);
+	ret = opal_error_code(be64_to_cpu(msg.params[1]));
 
 out_token:
 	mutex_unlock(&opal_sensor_mutex);
Index: linux.git/arch/powerpc/include/asm/opal.h
===================================================================
--- linux.git.orig/arch/powerpc/include/asm/opal.h
+++ linux.git/arch/powerpc/include/asm/opal.h
@@ -983,6 +983,8 @@ struct opal_sg_list *opal_vmalloc_to_sg_
 					     unsigned long vmalloc_size);
 void opal_free_sg_list(struct opal_sg_list *sg);
 
+extern int opal_error_code(int rc);
+
 /*
  * Dump region ID range usable by the OS
  */
Index: linux.git/arch/powerpc/platforms/powernv/opal.c
===================================================================
--- linux.git.orig/arch/powerpc/platforms/powernv/opal.c
+++ linux.git/arch/powerpc/platforms/powernv/opal.c
@@ -894,6 +894,25 @@ void opal_free_sg_list(struct opal_sg_li
 	}
 }
 
+int opal_error_code(int rc)
+{
+	switch (rc) {
+	case OPAL_SUCCESS:		return 0;
+
+	case OPAL_PARAMETER:		return -EINVAL;
+	case OPAL_ASYNC_COMPLETION:	return -EINPROGRESS;
+	case OPAL_BUSY_EVENT:		return -EBUSY;
+	case OPAL_NO_MEM:		return -ENOMEM;
+
+	case OPAL_UNSUPPORTED:		return -EIO;
+	case OPAL_HARDWARE:		return -EIO;
+	case OPAL_INTERNAL_ERROR:	return -EIO;
+	default:
+		pr_err("%s: unexpected OPAL error %d\n", __func__, rc);
+		return -EIO;
+	}
+}
+
 EXPORT_SYMBOL_GPL(opal_poll_events);
 EXPORT_SYMBOL_GPL(opal_rtc_read);
 EXPORT_SYMBOL_GPL(opal_rtc_write);

[PATCH v4 2/2] powerpc/powernv: handle OPAL_SUCCESS return in opal_sensor_read

[Cédric Le Goater]

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 <clg@fr.ibm.com>
---

 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
===================================================================
--- linux.git.orig/arch/powerpc/platforms/powernv/opal-sensor.c
+++ linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
@@ -46,20 +46,28 @@ 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) {
-		ret = opal_error_code(ret);
-		goto out_token;
-	}
+	switch (ret) {
+	case OPAL_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);
+			goto out_token;
+		}
 
-	ret = opal_async_wait_response(token, &msg);
-	if (ret) {
-		pr_err("%s: Failed to wait for the async response, %d\n",
-				__func__, ret);
-		goto out_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]));
+	case OPAL_SUCCESS:
+		ret = 0;
+		*sensor_data = be32_to_cpu(data);
+		break;
+
+	default:
+		ret = opal_error_code(ret);
+		break;
+	}
 
 out_token:
 	mutex_unlock(&opal_sensor_mutex);

[PATCH v3 2/3] powerpc/powernv: handle OPAL_SUCCESS return in opal_sensor_read

[Cédric Le Goater]

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 <clg@fr.ibm.com>
---

 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
===================================================================
--- linux.git.orig/arch/powerpc/platforms/powernv/opal-sensor.c
+++ linux.git/arch/powerpc/platforms/powernv/opal-sensor.c
@@ -46,20 +46,28 @@ 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) {
-		ret = opal_error_code(ret);
-		goto out_token;
-	}
+	switch (ret) {
+	case OPAL_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);
+			goto out_token;
+		}
 
-	ret = opal_async_wait_response(token, &msg);
-	if (ret) {
-		pr_err("%s: Failed to wait for the async response, %d\n",
-				__func__, ret);
-		goto out_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]));
+	case OPAL_SUCCESS:
+		ret = 0;
+		*sensor_data = be32_to_cpu(data);
+		break;
+
+	default:
+		ret = opal_error_code(ret);
+		break;
+	}
 
 out_token:
 	mutex_unlock(&opal_sensor_mutex);

[PATCH v3 3/3] powerpc/powernv: remove opal_sensor_mutex

[Cédric Le Goater]

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

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

Re: [PATCH v3 3/3] powerpc/powernv: remove opal_sensor_mutex

[Michael Ellerman]

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

Re: [PATCH v3 3/3] powerpc/powernv: remove opal_sensor_mutex

[Cedric Le Goater]

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.

Re: [PATCH v3 3/3] powerpc/powernv: remove opal_sensor_mutex

[Michael Ellerman]

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

Re: [PATCH v3 3/3] powerpc/powernv: remove opal_sensor_mutex

[Cedric Le Goater]

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.

Re: [PATCH] powerpc/powernv: handle OPAL_SUCCESS return in opal_sensor_read

[Stewart Smith]

Cedric Le Goater <clg@fr.ibm.com> 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 :)