[PATCH v2 0/4] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes

[PATCH v2 0/4] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes

[Guenter Roeck]

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
error code gets lost. In cros_ec_cmd_xfer_status(), convert all EC errors
to Linux error codes to report a more meaningful error to the caller to aid
debugging.

To prepare for this change, handle error codes other than -EPROTO for all
callers of cros_ec_cmd_xfer_status(). Specifically, no longer assume that
-EPROTO reflects an error from the EC and all other error codes reflect a
transfer error.

v2: Add patches 1/4 to 3/4 to handle callers of cros_ec_cmd_xfer_status()

----------------------------------------------------------------
Guenter Roeck (4):
      iio: cros_ec: Accept -EOPNOTSUPP as 'not supported' error code
      cros_ec_lightbar: Accept more error codes from cros_ec_cmd_xfer_status
      platform/chrome: cros_ec_sysfs: Report range of error codes from EC
      platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes

 .../iio/common/cros_ec_sensors/cros_ec_sensors.c   |  2 +-
 drivers/platform/chrome/cros_ec_lightbar.c         | 10 +++---
 drivers/platform/chrome/cros_ec_proto.c            | 37 +++++++++++++++++-----
 drivers/platform/chrome/cros_ec_sysfs.c            | 24 ++++++--------
 4 files changed, 43 insertions(+), 30 deletions(-)

[PATCH v2 1/4] iio: cros_ec: Accept -EOPNOTSUPP as 'not supported' error code

[Guenter Roeck]

A follow-up patch will extend the number of errors reported by
cros_ec_cmd_xfer_status(). Specifically, the function will return
-EOPNOTSUPP if a command is not supported by the EC. Prepare for it.

Cc: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Cc: Prashant Malani <pmalani@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added patch

 drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
index a66941fdb385..e3aff95493dd 100644
--- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
+++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
@@ -73,7 +73,7 @@ static int cros_ec_sensors_read(struct iio_dev *indio_dev,
 		st->core.param.sensor_offset.flags = 0;
 
 		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
-		if (ret == -EPROTO) {
+		if (ret == -EPROTO || ret == -EOPNOTSUPP) {
 			/* Reading calibscale is not supported on older EC. */
 			*val = 1;
 			*val2 = 0;
-- 
2.17.1

Re: [PATCH v2 1/4] iio: cros_ec: Accept -EOPNOTSUPP as 'not supported' error code

[Jonathan Cameron]

On Mon, 20 Jul 2020 13:22:40 -0700
Guenter Roeck <linux@roeck-us.net> wrote:

> A follow-up patch will extend the number of errors reported by
> cros_ec_cmd_xfer_status(). Specifically, the function will return
> -EOPNOTSUPP if a command is not supported by the EC. Prepare for it.
> 
> Cc: Yu-Hsuan Hsu <yuhsuan@chromium.org>
> Cc: Prashant Malani <pmalani@chromium.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> v2: Added patch
> 
>  drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> index a66941fdb385..e3aff95493dd 100644
> --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> @@ -73,7 +73,7 @@ static int cros_ec_sensors_read(struct iio_dev *indio_dev,
>  		st->core.param.sensor_offset.flags = 0;
>  
>  		ret = cros_ec_motion_send_host_cmd(&st->core, 0);
> -		if (ret == -EPROTO) {
> +		if (ret == -EPROTO || ret == -EOPNOTSUPP) {
>  			/* Reading calibscale is not supported on older EC. */
>  			*val = 1;
>  			*val2 = 0;

[PATCH v2 2/4] cros_ec_lightbar: Accept more error codes from cros_ec_cmd_xfer_status

[Guenter Roeck]

Since commit c5cd2b47b203 ("platform/chrome: cros_ec_proto: Report command
not supported") we can no longer assume that cros_ec_cmd_xfer_status()
reports -EPROTO for all errors returned by the EC itself. A follow-up
patch will change cros_ec_cmd_xfer_status() to report additional errors
reported by the EC as distinguished Linux error codes.

Handle this change by no longer assuming that -EPROTO is used to report
all errors returned by the EC itself. Since errors reported by the EC are
already reported in text form through sysfs attributes, extend this form
of error reporting to all errors reported by cros_ec_cmd_xfer_status().

Cc: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Cc: Prashant Malani <pmalani@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added patch

 drivers/platform/chrome/cros_ec_lightbar.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
index b59180bff5a3..8445cda57927 100644
--- a/drivers/platform/chrome/cros_ec_lightbar.c
+++ b/drivers/platform/chrome/cros_ec_lightbar.c
@@ -117,7 +117,7 @@ static int get_lightbar_version(struct cros_ec_dev *ec,
 	param = (struct ec_params_lightbar *)msg->data;
 	param->cmd = LIGHTBAR_CMD_VERSION;
 	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
-	if (ret < 0) {
+	if (ret < 0 && ret != -EINVAL) {
 		ret = 0;
 		goto exit;
 	}
@@ -298,11 +298,9 @@ static ssize_t sequence_show(struct device *dev,
 		goto exit;
 
 	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
-	if (ret == -EPROTO) {
-		ret = scnprintf(buf, PAGE_SIZE,
-				"ERROR: EC returned %d\n", msg->result);
-		goto exit;
-	} else if (ret < 0) {
+	if (ret < 0) {
+		ret = scnprintf(buf, PAGE_SIZE, "XFER / EC ERROR %d / %d\n",
+				ret, msg->result);
 		goto exit;
 	}
 
-- 
2.17.1

[PATCH v2 3/4] platform/chrome: cros_ec_sysfs: Report range of error codes from EC

[Guenter Roeck]

Since commit c5cd2b47b203 ("platform/chrome: cros_ec_proto: Report command
not supported") we can no longer assume that cros_ec_cmd_xfer_status()
reports -EPROTO for all errors returned by the EC itself. A follow-up
patch will change cros_ec_cmd_xfer_status() to report additional errors
reported by the EC as distinguished Linux error codes.

Prepare for this change by always reporting both the Linux error code
and the EC error code in sysfs attributes.

Cc: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Cc: Prashant Malani <pmalani@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added patch

 drivers/platform/chrome/cros_ec_sysfs.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index d45ea5d5bfa4..9c1e0998a721 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -150,12 +150,10 @@ static ssize_t version_show(struct device *dev,
 	msg->command = EC_CMD_GET_BUILD_INFO + ec->cmd_offset;
 	msg->insize = EC_HOST_PARAM_SIZE;
 	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
-	if (ret == -EPROTO) {
-		count += scnprintf(buf + count, PAGE_SIZE - count,
-				   "Build info:    EC error %d\n", msg->result);
-	} else if (ret < 0) {
+	if (ret < 0) {
 		count += scnprintf(buf + count, PAGE_SIZE - count,
-				   "Build info:    XFER ERROR %d\n", ret);
+				   "Build info:    XFER / EC ERROR %d / %d\n",
+				   ret, msg->result);
 	} else {
 		msg->data[EC_HOST_PARAM_SIZE - 1] = '\0';
 		count += scnprintf(buf + count, PAGE_SIZE - count,
@@ -166,12 +164,10 @@ static ssize_t version_show(struct device *dev,
 	msg->command = EC_CMD_GET_CHIP_INFO + ec->cmd_offset;
 	msg->insize = sizeof(*r_chip);
 	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
-	if (ret == -EPROTO) {
-		count += scnprintf(buf + count, PAGE_SIZE - count,
-				   "Chip info:     EC error %d\n", msg->result);
-	} else if (ret < 0) {
+	if (ret < 0) {
 		count += scnprintf(buf + count, PAGE_SIZE - count,
-				   "Chip info:     XFER ERROR %d\n", ret);
+				   "Chip info:     XFER / EC ERROR %d / %d\n",
+				   ret, msg->result);
 	} else {
 		r_chip = (struct ec_response_get_chip_info *)msg->data;
 
@@ -190,12 +186,10 @@ static ssize_t version_show(struct device *dev,
 	msg->command = EC_CMD_GET_BOARD_VERSION + ec->cmd_offset;
 	msg->insize = sizeof(*r_board);
 	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
-	if (ret == -EPROTO) {
-		count += scnprintf(buf + count, PAGE_SIZE - count,
-				   "Board version: EC error %d\n", msg->result);
-	} else if (ret < 0) {
+	if (ret < 0) {
 		count += scnprintf(buf + count, PAGE_SIZE - count,
-				   "Board version: XFER ERROR %d\n", ret);
+				   "Board version: XFER / EC ERROR %d / %d\n",
+				   ret, msg->result);
 	} else {
 		r_board = (struct ec_response_board_version *)msg->data;
 
-- 
2.17.1

[PATCH v2 4/4] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes

[Guenter Roeck]

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
error code gets lost. In cros_ec_cmd_xfer_status(), convert all EC errors
to Linux error codes to report a more meaningful error to the caller to aid
debugging.

Cc: Yu-Hsuan Hsu <yuhsuan@chromium.org>
Cc: Prashant Malani <pmalani@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: No change

Notes:
    I would welcome feedback on the error code translations.
    Can we do better ?

    -ENOTSUPP is not a recommended error code, and checkpatch complains
    about it. It is used in existing code, so I did not change it, but it
    might be worthwhile exploring if we can find a better error code to
    report "version not supported". Possible candidates might be EPROTOTYPE,
    ENOPROTOOPT, EPROTONOSUPPORT, EPFNOSUPPORT, or EAFNOSUPPORT. I don't
    see a direct match, but NFS reports -EPROTONOSUPPORT for unsupported
    protocol versions.

 drivers/platform/chrome/cros_ec_proto.c | 37 +++++++++++++++++++------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 3e745e0fe092..10aa9e483d35 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -543,6 +543,29 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
 }
 EXPORT_SYMBOL(cros_ec_cmd_xfer);
 
+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] = -ENOTSUPP,
+	[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,
+};
+
 /**
  * cros_ec_cmd_xfer_status() - Send a command to the ChromeOS EC.
  * @ec_dev: EC device.
@@ -555,8 +578,7 @@ EXPORT_SYMBOL(cros_ec_cmd_xfer);
  *
  * Return:
  * >=0 - The number of bytes transferred
- * -ENOTSUPP - 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,13 +588,12 @@ 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 -ENOTSUPP;
 	} else if (msg->result != EC_RES_SUCCESS) {
-		dev_dbg(ec_dev->dev, "Command result (err: %d)\n", msg->result);
-		return -EPROTO;
+		if (msg->result < ARRAY_SIZE(cros_ec_error_map) && cros_ec_error_map[msg->result])
+			ret = cros_ec_error_map[msg->result];
+		else
+			ret = -EPROTO;
+		dev_dbg(ec_dev->dev, "Command result (err: %d [%d])\n", msg->result, ret);
 	}
 
 	return ret;
-- 
2.17.1

Re: [PATCH v2 4/4] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes

[Enric Balletbo i Serra]

Hi Guenter,

Thank you for work on this. Cc'ing Gwendal as he has a deep knowledge of the EC
and their errors.

On 20/7/20 22:22, Guenter Roeck wrote:
> 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
> error code gets lost. In cros_ec_cmd_xfer_status(), convert all EC errors
> to Linux error codes to report a more meaningful error to the caller to aid
> debugging.
> 
> Cc: Yu-Hsuan Hsu <yuhsuan@chromium.org>
> Cc: Prashant Malani <pmalani@chromium.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: No change
> 
> Notes:
>     I would welcome feedback on the error code translations.
>     Can we do better ?
> 
>     -ENOTSUPP is not a recommended error code, and checkpatch complains
>     about it. It is used in existing code, so I did not change it, but it
>     might be worthwhile exploring if we can find a better error code to
>     report "version not supported". Possible candidates might be EPROTOTYPE,
>     ENOPROTOOPT, EPROTONOSUPPORT, EPFNOSUPPORT, or EAFNOSUPPORT. I don't
>     see a direct match, but NFS reports -EPROTONOSUPPORT for unsupported
>     protocol versions.
> 
>  drivers/platform/chrome/cros_ec_proto.c | 37 +++++++++++++++++++------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index 3e745e0fe092..10aa9e483d35 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -543,6 +543,29 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
>  }
>  EXPORT_SYMBOL(cros_ec_cmd_xfer);
>  
> +static const int cros_ec_error_map[] = {
> +	[EC_RES_INVALID_COMMAND] = -EOPNOTSUPP,
> +	[EC_RES_ERROR] = -EIO,

nit -EREMOTEIO? To make explicit that the error is "remote" from the host.
Although -EIO seems to be more generic.

> +	[EC_RES_INVALID_PARAM] = -EINVAL,
> +	[EC_RES_ACCESS_DENIED] = -EACCES,
> +	[EC_RES_INVALID_RESPONSE] = -EPROTO,
> +	[EC_RES_INVALID_VERSION] = -ENOTSUPP,

+1 for EPROTONOSUPPORT to match with EC_RES_INVALID_VERSION

> +	[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,

Although the name matches, I'm wondering if we should use -EAGAIN instead as per
EC documentation:

  EC_RES_BUSY - Up but too busy. Should retry.

hmm, however, for the audio codec, for example, this seems to have a slightly
different meaning and retry is not what we want, so let's do direct translation
and stay with -EBUSY.

> +	[EC_RES_INVALID_HEADER_VERSION] = -EBADMSG,
> +	[EC_RES_INVALID_HEADER_CRC] = -EBADMSG,
> +	[EC_RES_INVALID_DATA_CRC] = -EBADMSG,
> +	[EC_RES_DUP_UNAVAILABLE] = -ENODATA,
> +};
> +
>  /**
>   * cros_ec_cmd_xfer_status() - Send a command to the ChromeOS EC.
>   * @ec_dev: EC device.
> @@ -555,8 +578,7 @@ EXPORT_SYMBOL(cros_ec_cmd_xfer);
>   *
>   * Return:
>   * >=0 - The number of bytes transferred
> - * -ENOTSUPP - 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,13 +588,12 @@ 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 -ENOTSUPP;
>  	} else if (msg->result != EC_RES_SUCCESS) {
> -		dev_dbg(ec_dev->dev, "Command result (err: %d)\n", msg->result);
> -		return -EPROTO;
> +		if (msg->result < ARRAY_SIZE(cros_ec_error_map) && cros_ec_error_map[msg->result])
> +			ret = cros_ec_error_map[msg->result];
> +		else
> +			ret = -EPROTO;
> +		dev_dbg(ec_dev->dev, "Command result (err: %d [%d])\n", msg->result, ret);
>  	}
>  
>  	return ret;
> 

Re: [PATCH v2 4/4] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes

[Guenter Roeck]

Hi Enric,

On Tue, Jul 21, 2020 at 01:29:01PM +0200, Enric Balletbo i Serra wrote:
> Hi Guenter,
> 
> Thank you for work on this. Cc'ing Gwendal as he has a deep knowledge of the EC
> and their errors.

The series is now also available in Gerrit at
	https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2306731/3
for integration/testing into ChromeOS.

Thanks,
Guenter

> 
> On 20/7/20 22:22, Guenter Roeck wrote:
> > 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
> > error code gets lost. In cros_ec_cmd_xfer_status(), convert all EC errors
> > to Linux error codes to report a more meaningful error to the caller to aid
> > debugging.
> > 
> > Cc: Yu-Hsuan Hsu <yuhsuan@chromium.org>
> > Cc: Prashant Malani <pmalani@chromium.org>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > v2: No change
> > 
> > Notes:
> >     I would welcome feedback on the error code translations.
> >     Can we do better ?
> > 
> >     -ENOTSUPP is not a recommended error code, and checkpatch complains
> >     about it. It is used in existing code, so I did not change it, but it
> >     might be worthwhile exploring if we can find a better error code to
> >     report "version not supported". Possible candidates might be EPROTOTYPE,
> >     ENOPROTOOPT, EPROTONOSUPPORT, EPFNOSUPPORT, or EAFNOSUPPORT. I don't
> >     see a direct match, but NFS reports -EPROTONOSUPPORT for unsupported
> >     protocol versions.
> > 
> >  drivers/platform/chrome/cros_ec_proto.c | 37 +++++++++++++++++++------
> >  1 file changed, 29 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> > index 3e745e0fe092..10aa9e483d35 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -543,6 +543,29 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
> >  }
> >  EXPORT_SYMBOL(cros_ec_cmd_xfer);
> >  
> > +static const int cros_ec_error_map[] = {
> > +	[EC_RES_INVALID_COMMAND] = -EOPNOTSUPP,
> > +	[EC_RES_ERROR] = -EIO,
> 
> nit -EREMOTEIO? To make explicit that the error is "remote" from the host.
> Although -EIO seems to be more generic.
> 
> > +	[EC_RES_INVALID_PARAM] = -EINVAL,
> > +	[EC_RES_ACCESS_DENIED] = -EACCES,
> > +	[EC_RES_INVALID_RESPONSE] = -EPROTO,
> > +	[EC_RES_INVALID_VERSION] = -ENOTSUPP,
> 
> +1 for EPROTONOSUPPORT to match with EC_RES_INVALID_VERSION
> 
> > +	[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,
> 
> Although the name matches, I'm wondering if we should use -EAGAIN instead as per
> EC documentation:
> 
>   EC_RES_BUSY - Up but too busy. Should retry.
> 
> hmm, however, for the audio codec, for example, this seems to have a slightly
> different meaning and retry is not what we want, so let's do direct translation
> and stay with -EBUSY.
> 
> > +	[EC_RES_INVALID_HEADER_VERSION] = -EBADMSG,
> > +	[EC_RES_INVALID_HEADER_CRC] = -EBADMSG,
> > +	[EC_RES_INVALID_DATA_CRC] = -EBADMSG,
> > +	[EC_RES_DUP_UNAVAILABLE] = -ENODATA,
> > +};
> > +
> >  /**
> >   * cros_ec_cmd_xfer_status() - Send a command to the ChromeOS EC.
> >   * @ec_dev: EC device.
> > @@ -555,8 +578,7 @@ EXPORT_SYMBOL(cros_ec_cmd_xfer);
> >   *
> >   * Return:
> >   * >=0 - The number of bytes transferred
> > - * -ENOTSUPP - 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,13 +588,12 @@ 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 -ENOTSUPP;
> >  	} else if (msg->result != EC_RES_SUCCESS) {
> > -		dev_dbg(ec_dev->dev, "Command result (err: %d)\n", msg->result);
> > -		return -EPROTO;
> > +		if (msg->result < ARRAY_SIZE(cros_ec_error_map) && cros_ec_error_map[msg->result])
> > +			ret = cros_ec_error_map[msg->result];
> > +		else
> > +			ret = -EPROTO;
> > +		dev_dbg(ec_dev->dev, "Command result (err: %d [%d])\n", msg->result, ret);
> >  	}
> >  
> >  	return ret;
> > 

Re: [PATCH v2 4/4] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes

[Guenter Roeck]

On Tue, Jul 21, 2020 at 01:29:01PM +0200, Enric Balletbo i Serra wrote:
> Hi Guenter,
> 
> Thank you for work on this. Cc'ing Gwendal as he has a deep knowledge of the EC
> and their errors.
> 
> On 20/7/20 22:22, Guenter Roeck wrote:
> > 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
> > error code gets lost. In cros_ec_cmd_xfer_status(), convert all EC errors
> > to Linux error codes to report a more meaningful error to the caller to aid
> > debugging.
> > 
> > Cc: Yu-Hsuan Hsu <yuhsuan@chromium.org>
> > Cc: Prashant Malani <pmalani@chromium.org>
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > v2: No change
> > 
> > Notes:
> >     I would welcome feedback on the error code translations.
> >     Can we do better ?
> > 
> >     -ENOTSUPP is not a recommended error code, and checkpatch complains
> >     about it. It is used in existing code, so I did not change it, but it
> >     might be worthwhile exploring if we can find a better error code to
> >     report "version not supported". Possible candidates might be EPROTOTYPE,
> >     ENOPROTOOPT, EPROTONOSUPPORT, EPFNOSUPPORT, or EAFNOSUPPORT. I don't
> >     see a direct match, but NFS reports -EPROTONOSUPPORT for unsupported
> >     protocol versions.
> > 
> >  drivers/platform/chrome/cros_ec_proto.c | 37 +++++++++++++++++++------
> >  1 file changed, 29 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> > index 3e745e0fe092..10aa9e483d35 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -543,6 +543,29 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
> >  }
> >  EXPORT_SYMBOL(cros_ec_cmd_xfer);
> >  
> > +static const int cros_ec_error_map[] = {
> > +	[EC_RES_INVALID_COMMAND] = -EOPNOTSUPP,
> > +	[EC_RES_ERROR] = -EIO,
> 
> nit -EREMOTEIO? To make explicit that the error is "remote" from the host.
> Although -EIO seems to be more generic.
> 
I think using -EREMOTEIO is a good idea; some SPI/I2C controller error
might return -EIO.

> > +	[EC_RES_INVALID_PARAM] = -EINVAL,
> > +	[EC_RES_ACCESS_DENIED] = -EACCES,
> > +	[EC_RES_INVALID_RESPONSE] = -EPROTO,
> > +	[EC_RES_INVALID_VERSION] = -ENOTSUPP,
> 
> +1 for EPROTONOSUPPORT to match with EC_RES_INVALID_VERSION
> 
Ok. I'll do that with a separate patch.

> > +	[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,
> 
> Although the name matches, I'm wondering if we should use -EAGAIN instead as per
> EC documentation:
> 
>   EC_RES_BUSY - Up but too busy. Should retry.
> 
> hmm, however, for the audio codec, for example, this seems to have a slightly
> different meaning and retry is not what we want, so let's do direct translation
> and stay with -EBUSY.
> 
> > +	[EC_RES_INVALID_HEADER_VERSION] = -EBADMSG,

Any idea for EC_RES_INVALID_HEADER_VERSION ? I am not entirely happy
with -EBADMSG: the error is distinctly different to CRC errors.
EPROTONOSUPPORT as well, maybe, or something else ?

Thanks,
Guenter

> > +	[EC_RES_INVALID_HEADER_CRC] = -EBADMSG,
> > +	[EC_RES_INVALID_DATA_CRC] = -EBADMSG,
> > +	[EC_RES_DUP_UNAVAILABLE] = -ENODATA,
> > +};
> > +
> >  /**
> >   * cros_ec_cmd_xfer_status() - Send a command to the ChromeOS EC.
> >   * @ec_dev: EC device.
> > @@ -555,8 +578,7 @@ EXPORT_SYMBOL(cros_ec_cmd_xfer);
> >   *
> >   * Return:
> >   * >=0 - The number of bytes transferred
> > - * -ENOTSUPP - 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,13 +588,12 @@ 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 -ENOTSUPP;
> >  	} else if (msg->result != EC_RES_SUCCESS) {
> > -		dev_dbg(ec_dev->dev, "Command result (err: %d)\n", msg->result);
> > -		return -EPROTO;
> > +		if (msg->result < ARRAY_SIZE(cros_ec_error_map) && cros_ec_error_map[msg->result])
> > +			ret = cros_ec_error_map[msg->result];
> > +		else
> > +			ret = -EPROTO;
> > +		dev_dbg(ec_dev->dev, "Command result (err: %d [%d])\n", msg->result, ret);
> >  	}
> >  
> >  	return ret;
> > 

Re: [PATCH v2 4/4] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes

[Brian Norris]

+ drinkcat, aseda

On Tue, Jul 21, 2020 at 07:23:20AM -0700, Guenter Roeck wrote:
> On Tue, Jul 21, 2020 at 01:29:01PM +0200, Enric Balletbo i Serra wrote:
> > On 20/7/20 22:22, Guenter Roeck wrote:
> > > +	[EC_RES_INVALID_HEADER_VERSION] = -EBADMSG,
> 
> Any idea for EC_RES_INVALID_HEADER_VERSION ? I am not entirely happy
> with -EBADMSG: the error is distinctly different to CRC errors.
> EPROTONOSUPPORT as well, maybe, or something else ?

FWIW, these (INVALID_HEADER_VERSION, INVALID_HEADER_CRC,
INVALID_DATA_CRC) aren't actually used on any firmware yet. This has
been open forever:
https://crbug.com/787159
Added here:
https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/780452/

Unfortunately, the linked design doc (still in draft) is not public.

My understanding is that while they're not all exactly the same (CRC is
different than the others), they are all still supposed to represent
"corrupt request [from the Application Processor]". EBADMSG seems good
enough to me.

Brian

P.S. for those added late -- you can grab the whole thread from here:
https://lore.kernel.org/lkml/20200720202243.180230-1-linux@roeck-us.net/
or in mbox form:
https://lore.kernel.org/lkml/20200720202243.180230-1-linux@roeck-us.net/t.mbox.gz

Re: [PATCH v2 4/4] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes

[Guenter Roeck]

On 7/22/20 2:52 PM, Brian Norris wrote:
> + drinkcat, aseda
> 
> On Tue, Jul 21, 2020 at 07:23:20AM -0700, Guenter Roeck wrote:
>> On Tue, Jul 21, 2020 at 01:29:01PM +0200, Enric Balletbo i Serra wrote:
>>> On 20/7/20 22:22, Guenter Roeck wrote:
>>>> +	[EC_RES_INVALID_HEADER_VERSION] = -EBADMSG,
>>
>> Any idea for EC_RES_INVALID_HEADER_VERSION ? I am not entirely happy
>> with -EBADMSG: the error is distinctly different to CRC errors.
>> EPROTONOSUPPORT as well, maybe, or something else ?
> 
> FWIW, these (INVALID_HEADER_VERSION, INVALID_HEADER_CRC,
> INVALID_DATA_CRC) aren't actually used on any firmware yet. This has
> been open forever:
> https://crbug.com/787159
> Added here:
> https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/780452/
> 
> Unfortunately, the linked design doc (still in draft) is not public.
> 
> My understanding is that while they're not all exactly the same (CRC is
> different than the others), they are all still supposed to represent
> "corrupt request [from the Application Processor]". EBADMSG seems good
> enough to me.
> 

SGTM, and make sense.

Thanks,
Guenter

> Brian
> 
> P.S. for those added late -- you can grab the whole thread from here:
> https://lore.kernel.org/lkml/20200720202243.180230-1-linux@roeck-us.net/
> or in mbox form:
> https://lore.kernel.org/lkml/20200720202243.180230-1-linux@roeck-us.net/t.mbox.gz
> 

Re: [PATCH v2 0/4] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes

[Brian Norris]

Hi Guenter,

On Mon, Jul 20, 2020 at 01:22:39PM -0700, Guenter Roeck wrote:
> 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
> error code gets lost. In cros_ec_cmd_xfer_status(), convert all EC errors
> to Linux error codes to report a more meaningful error to the caller to aid
> debugging.
> 
> To prepare for this change, handle error codes other than -EPROTO for all
> callers of cros_ec_cmd_xfer_status(). Specifically, no longer assume that
> -EPROTO reflects an error from the EC and all other error codes reflect a
> transfer error.
> 
> v2: Add patches 1/4 to 3/4 to handle callers of cros_ec_cmd_xfer_status()

I did a rough grep to see what you might be missing:

  git grep -n EPROTO | grep -e cros -e '-ec'

I think cros-ec-pwm / cros_ec_num_pwms() might need fixing too? Boy, I
wrote that, but it sure ain't easy to read...(*checks watch*)...4 years
later.

Apart from the notes already made, I think the series looks good:

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

Feel free to CC me on v3, if you want another look

Brian

Re: [PATCH v2 0/4] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes

[Guenter Roeck]

Hi Brian,

On 7/22/20 3:03 PM, Brian Norris wrote:
> Hi Guenter,
> 
> On Mon, Jul 20, 2020 at 01:22:39PM -0700, Guenter Roeck wrote:
>> 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
>> error code gets lost. In cros_ec_cmd_xfer_status(), convert all EC errors
>> to Linux error codes to report a more meaningful error to the caller to aid
>> debugging.
>>
>> To prepare for this change, handle error codes other than -EPROTO for all
>> callers of cros_ec_cmd_xfer_status(). Specifically, no longer assume that
>> -EPROTO reflects an error from the EC and all other error codes reflect a
>> transfer error.
>>
>> v2: Add patches 1/4 to 3/4 to handle callers of cros_ec_cmd_xfer_status()
> 
> I did a rough grep to see what you might be missing:
> 
>   git grep -n EPROTO | grep -e cros -e '-ec'
> 
> I think cros-ec-pwm / cros_ec_num_pwms() might need fixing too? Boy, I

Yes, you are correct. And there was me thinking I got them all. Thanks a lot !

> wrote that, but it sure ain't easy to read...(*checks watch*)...4 years
> later.
> 
Usually I am quite embarrassed by code I wrote more than a couple of years
ago :-)

> Apart from the notes already made, I think the series looks good:
> 
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> > Feel free to CC me on v3, if you want another look
> 
I'll be happy to do that. I did make a couple of changes, so I'll add
your Reviewed-by: only to unchanged patches.

Thanks,
Guenter