[v3,6/6] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes
diff mbox series

Message ID 20200726220101.29059-7-linux@roeck-us.net
State New
Headers show
Series
  • platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes
Related show

Commit Message

Guenter Roeck July 26, 2020, 10:01 p.m. UTC
The EC reports a variety of error codes. Most of those, with the exception
of EC_RES_INVALID_VERSION, are converted to -EPROTO. As result, the actual
EC error code gets lost. Introduce cros_ec_map_error() to map EC error
codes to Linux error codes, and use it in cros_ec_cmd_xfer_status() to
report more meaningful errors to the caller. With this change, callers of
cros_ec_cmd_xfer_status() can implement a more distinguished action without
having to rely on the EC error code. At the same time, debugging is improved
in situations where the Linux error code is reported to userspace and/or in
the kernel log.

Cc: Gwendal Grignou <gwendal@chromium.org>
Cc: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Cc: Prashant Malani <pmalani@chromium.org>
Cc: Brian Norris <briannorris@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v3: Use -ENOPROTOOPT for EC_RES_INVALID_VERSION
    Implement function to convert error codes
v2: No change

 drivers/platform/chrome/cros_ec_proto.c | 52 ++++++++++++++++++++-----
 1 file changed, 42 insertions(+), 10 deletions(-)

Comments

Brian Norris July 29, 2020, 10:21 p.m. UTC | #1
Hi Guenter,

On Sun, Jul 26, 2020 at 03:01:01PM -0700, Guenter Roeck wrote:
> v3: Use -ENOPROTOOPT for EC_RES_INVALID_VERSION
>     Implement function to convert error codes
> v2: No change
> 
>  drivers/platform/chrome/cros_ec_proto.c | 52 ++++++++++++++++++++-----
>  1 file changed, 42 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index e5bbec979a2a..a081b8245682 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -15,6 +15,43 @@
>  
>  #define EC_COMMAND_RETRIES	50
>  
> +static const int cros_ec_error_map[] = {
> +	[EC_RES_INVALID_COMMAND] = -EOPNOTSUPP,
> +	[EC_RES_ERROR] = -EIO,
> +	[EC_RES_INVALID_PARAM] = -EINVAL,
> +	[EC_RES_ACCESS_DENIED] = -EACCES,
> +	[EC_RES_INVALID_RESPONSE] = -EPROTO,
> +	[EC_RES_INVALID_VERSION] = -ENOPROTOOPT,
> +	[EC_RES_INVALID_CHECKSUM] = -EBADMSG,
> +	[EC_RES_IN_PROGRESS] = -EINPROGRESS,
> +	[EC_RES_UNAVAILABLE] = -ENODATA,
> +	[EC_RES_TIMEOUT] = -ETIMEDOUT,
> +	[EC_RES_OVERFLOW] = -EOVERFLOW,
> +	[EC_RES_INVALID_HEADER] = -EBADR,
> +	[EC_RES_REQUEST_TRUNCATED] = -EBADR,
> +	[EC_RES_RESPONSE_TOO_BIG] = -EFBIG,
> +	[EC_RES_BUS_ERROR] = -EFAULT,
> +	[EC_RES_BUSY] = -EBUSY,
> +	[EC_RES_INVALID_HEADER_VERSION] = -EBADMSG,
> +	[EC_RES_INVALID_HEADER_CRC] = -EBADMSG,
> +	[EC_RES_INVALID_DATA_CRC] = -EBADMSG,
> +	[EC_RES_DUP_UNAVAILABLE] = -ENODATA,
> +};

Sorry I didn't pay attention to this earlier, but is there any
programmatic way to ensure that we don't have unexpected holes here? If
we do (e.g., we add new error codes, but they aren't contiguous for
whatever reasons), then those will get treated as "success" with your
current patch.

I say "unexpected" hole, because EC_RES_SUCCESS (0) is an expected hole.

> +
> +static int cros_ec_map_error(uint32_t result)
> +{
> +	int ret = 0;
> +
> +	if (result != EC_RES_SUCCESS) {
> +		if (result < ARRAY_SIZE(cros_ec_error_map) && cros_ec_error_map[result])
> +			ret = cros_ec_error_map[result];

^^ Maybe we want to double check 'ret != 0'? Or maybe

			ret = cros_ec_error_map[result];
			if (!ret) {
				ret = -EPROTO;
				dev_err(..., "Unexpected EC result code %d\n", result);
			}

? Could even be WARN_ON(), since this would be an actionable programming
error, not exactly an external factor. Or maybe I'm being paranoid, and
future programmers are perfect.

Otherwise:

Reviewed-by: Brian Norris <briannorris@chromium.org>

> +		else
> +			ret = -EPROTO;
> +	}
> +
> +	return ret;
> +}
> +
>  static int prepare_packet(struct cros_ec_device *ec_dev,
>  			  struct cros_ec_command *msg)
>  {
Guenter Roeck July 29, 2020, 11:22 p.m. UTC | #2
On 7/29/20 3:21 PM, Brian Norris wrote:
> Hi Guenter,
> 
> On Sun, Jul 26, 2020 at 03:01:01PM -0700, Guenter Roeck wrote:
>> v3: Use -ENOPROTOOPT for EC_RES_INVALID_VERSION
>>     Implement function to convert error codes
>> v2: No change
>>
>>  drivers/platform/chrome/cros_ec_proto.c | 52 ++++++++++++++++++++-----
>>  1 file changed, 42 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
>> index e5bbec979a2a..a081b8245682 100644
>> --- a/drivers/platform/chrome/cros_ec_proto.c
>> +++ b/drivers/platform/chrome/cros_ec_proto.c
>> @@ -15,6 +15,43 @@
>>  
>>  #define EC_COMMAND_RETRIES	50
>>  
>> +static const int cros_ec_error_map[] = {
>> +	[EC_RES_INVALID_COMMAND] = -EOPNOTSUPP,
>> +	[EC_RES_ERROR] = -EIO,
>> +	[EC_RES_INVALID_PARAM] = -EINVAL,
>> +	[EC_RES_ACCESS_DENIED] = -EACCES,
>> +	[EC_RES_INVALID_RESPONSE] = -EPROTO,
>> +	[EC_RES_INVALID_VERSION] = -ENOPROTOOPT,
>> +	[EC_RES_INVALID_CHECKSUM] = -EBADMSG,
>> +	[EC_RES_IN_PROGRESS] = -EINPROGRESS,
>> +	[EC_RES_UNAVAILABLE] = -ENODATA,
>> +	[EC_RES_TIMEOUT] = -ETIMEDOUT,
>> +	[EC_RES_OVERFLOW] = -EOVERFLOW,
>> +	[EC_RES_INVALID_HEADER] = -EBADR,
>> +	[EC_RES_REQUEST_TRUNCATED] = -EBADR,
>> +	[EC_RES_RESPONSE_TOO_BIG] = -EFBIG,
>> +	[EC_RES_BUS_ERROR] = -EFAULT,
>> +	[EC_RES_BUSY] = -EBUSY,
>> +	[EC_RES_INVALID_HEADER_VERSION] = -EBADMSG,
>> +	[EC_RES_INVALID_HEADER_CRC] = -EBADMSG,
>> +	[EC_RES_INVALID_DATA_CRC] = -EBADMSG,
>> +	[EC_RES_DUP_UNAVAILABLE] = -ENODATA,
>> +};
> 
> Sorry I didn't pay attention to this earlier, but is there any
> programmatic way to ensure that we don't have unexpected holes here? If
> we do (e.g., we add new error codes, but they aren't contiguous for
> whatever reasons), then those will get treated as "success" with your
> current patch.
> 
> I say "unexpected" hole, because EC_RES_SUCCESS (0) is an expected hole.
> 
>> +
>> +static int cros_ec_map_error(uint32_t result)
>> +{
>> +	int ret = 0;
>> +
>> +	if (result != EC_RES_SUCCESS) {
>> +		if (result < ARRAY_SIZE(cros_ec_error_map) && cros_ec_error_map[result])
>> +			ret = cros_ec_error_map[result];
> 
> ^^ Maybe we want to double check 'ret != 0'? Or maybe
> 
> 			ret = cros_ec_error_map[result];
> 			if (!ret) {

'ret' won't ever be 0 here. Above:
							&& cros_ec_error_map[result]

and below:

		else
			ret = -EPROTO;

> 				ret = -EPROTO;
> 				dev_err(..., "Unexpected EC result code %d\n", result);
> 			}
> 
> ? Could even be WARN_ON(), since this would be an actionable programming
> error, not exactly an external factor. Or maybe I'm being paranoid, and
> future programmers are perfect.
> 
I think, if anything, we might consider adding the message below (result >=
ARRAY_SIZE(cros_ec_error_map) is just as bad). Not sure myself. I am
open to adding it if people think it would be useful/desirable.

Thanks,
Guenter

> Otherwise:
> 
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> 
>> +		else
>> +			ret = -EPROTO;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>  static int prepare_packet(struct cros_ec_device *ec_dev,
>>  			  struct cros_ec_command *msg)
>>  {
Brian Norris July 29, 2020, 11:29 p.m. UTC | #3
On Wed, Jul 29, 2020 at 4:22 PM Guenter Roeck <linux@roeck-us.net> wrote:
> On 7/29/20 3:21 PM, Brian Norris wrote:
> > On Sun, Jul 26, 2020 at 03:01:01PM -0700, Guenter Roeck wrote:
> >> --- a/drivers/platform/chrome/cros_ec_proto.c
> >> +++ b/drivers/platform/chrome/cros_ec_proto.c

> > ^^ Maybe we want to double check 'ret != 0'? Or maybe
> >
> >                       ret = cros_ec_error_map[result];
> >                       if (!ret) {
>
> 'ret' won't ever be 0 here. Above:
>                                                         && cros_ec_error_map[result]
>
> and below:
>
>                 else
>                         ret = -EPROTO;

Ah, I'm reading too quickly. You're correct, sorry.

> >                               ret = -EPROTO;
> >                               dev_err(..., "Unexpected EC result code %d\n", result);
> >                       }
> >
> > ? Could even be WARN_ON(), since this would be an actionable programming
> > error, not exactly an external factor. Or maybe I'm being paranoid, and
> > future programmers are perfect.
> >
> I think, if anything, we might consider adding the message below (result >=
> ARRAY_SIZE(cros_ec_error_map) is just as bad). Not sure myself. I am
> open to adding it if people think it would be useful/desirable.

No, my primary motivation was that I thought the logic left room for
error if there were holes. I was mistaken on that point. Secondarily,
it was also potentially useful to point out when we fell into those
holes. I'm not sure logging the warning is that important. Generally,
we only care about a handful of result codes, and as long as the rest
don't end up as "success", I think we're in OK shape.

Sorry for the noise. Here's my tag (which given my misreading so far,
should probably have a heavy discount on its value):

Reviewed-by: Brian Norris <briannorris@chromium.org>

Patch
diff mbox series

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index e5bbec979a2a..a081b8245682 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -15,6 +15,43 @@ 
 
 #define EC_COMMAND_RETRIES	50
 
+static const int cros_ec_error_map[] = {
+	[EC_RES_INVALID_COMMAND] = -EOPNOTSUPP,
+	[EC_RES_ERROR] = -EIO,
+	[EC_RES_INVALID_PARAM] = -EINVAL,
+	[EC_RES_ACCESS_DENIED] = -EACCES,
+	[EC_RES_INVALID_RESPONSE] = -EPROTO,
+	[EC_RES_INVALID_VERSION] = -ENOPROTOOPT,
+	[EC_RES_INVALID_CHECKSUM] = -EBADMSG,
+	[EC_RES_IN_PROGRESS] = -EINPROGRESS,
+	[EC_RES_UNAVAILABLE] = -ENODATA,
+	[EC_RES_TIMEOUT] = -ETIMEDOUT,
+	[EC_RES_OVERFLOW] = -EOVERFLOW,
+	[EC_RES_INVALID_HEADER] = -EBADR,
+	[EC_RES_REQUEST_TRUNCATED] = -EBADR,
+	[EC_RES_RESPONSE_TOO_BIG] = -EFBIG,
+	[EC_RES_BUS_ERROR] = -EFAULT,
+	[EC_RES_BUSY] = -EBUSY,
+	[EC_RES_INVALID_HEADER_VERSION] = -EBADMSG,
+	[EC_RES_INVALID_HEADER_CRC] = -EBADMSG,
+	[EC_RES_INVALID_DATA_CRC] = -EBADMSG,
+	[EC_RES_DUP_UNAVAILABLE] = -ENODATA,
+};
+
+static int cros_ec_map_error(uint32_t result)
+{
+	int ret = 0;
+
+	if (result != EC_RES_SUCCESS) {
+		if (result < ARRAY_SIZE(cros_ec_error_map) && cros_ec_error_map[result])
+			ret = cros_ec_error_map[result];
+		else
+			ret = -EPROTO;
+	}
+
+	return ret;
+}
+
 static int prepare_packet(struct cros_ec_device *ec_dev,
 			  struct cros_ec_command *msg)
 {
@@ -555,8 +592,7 @@  EXPORT_SYMBOL(cros_ec_cmd_xfer);
  *
  * Return:
  * >=0 - The number of bytes transferred
- * -ENOPROTOOPT - Operation not supported
- * -EPROTO - Protocol error
+ * <0 - Linux error code
  */
 int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
 			    struct cros_ec_command *msg)
@@ -566,15 +602,11 @@  int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
 	ret = cros_ec_cmd_xfer(ec_dev, msg);
 	if (ret < 0) {
 		dev_err(ec_dev->dev, "Command xfer error (err:%d)\n", ret);
-	} else if (msg->result == EC_RES_INVALID_VERSION) {
-		dev_dbg(ec_dev->dev, "Command invalid version (err:%d)\n",
-			msg->result);
-		return -ENOPROTOOPT;
-	} else if (msg->result != EC_RES_SUCCESS) {
-		dev_dbg(ec_dev->dev, "Command result (err: %d)\n", msg->result);
-		return -EPROTO;
+	} else {
+		ret = cros_ec_map_error(msg->result);
+		if (ret)
+			dev_dbg(ec_dev->dev, "Command result (err: %d [%d])\n", msg->result, ret);
 	}
-
 	return ret;
 }
 EXPORT_SYMBOL(cros_ec_cmd_xfer_status);