* [PATCH] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes @ 2020-07-04 14:26 Guenter Roeck 2020-07-06 18:52 ` Prashant Malani 2020-07-07 10:10 ` Enric Balletbo i Serra 0 siblings, 2 replies; 8+ messages in thread From: Guenter Roeck @ 2020-07-04 14:26 UTC (permalink / raw) To: Enric Balletbo i Serra Cc: Benson Leung, linux-kernel, Guenter Roeck, Yu-Hsuan Hsu, Prashant Malani 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. 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> --- 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes 2020-07-04 14:26 [PATCH] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes Guenter Roeck @ 2020-07-06 18:52 ` Prashant Malani 2020-07-06 19:41 ` Guenter Roeck 2020-07-07 10:10 ` Enric Balletbo i Serra 1 sibling, 1 reply; 8+ messages in thread From: Prashant Malani @ 2020-07-06 18:52 UTC (permalink / raw) To: Guenter Roeck Cc: Enric Balletbo i Serra, Benson Leung, linux-kernel, Yu-Hsuan Hsu Hi Guenter, On Sat, Jul 04, 2020 at 07:26:07AM -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. 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> > --- > 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]) Do we expect a case where cros_ec_error_map[msg->result] == 0? > + 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes 2020-07-06 18:52 ` Prashant Malani @ 2020-07-06 19:41 ` Guenter Roeck 2020-07-06 20:07 ` Prashant Malani 0 siblings, 1 reply; 8+ messages in thread From: Guenter Roeck @ 2020-07-06 19:41 UTC (permalink / raw) To: Prashant Malani Cc: Enric Balletbo i Serra, Benson Leung, linux-kernel, Yu-Hsuan Hsu On Mon, Jul 06, 2020 at 11:52:30AM -0700, Prashant Malani wrote: > Hi Guenter, > > On Sat, Jul 04, 2020 at 07:26:07AM -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. 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> > > --- > > 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]) > > Do we expect a case where cros_ec_error_map[msg->result] == 0? > It seemed to be prudent to assume that this code is not going to be updated whenever a new EC error code is added. Doing nothing would risk returning 0, and addding WARN_ON or dev_warn seemed excessive. Having said that, I don't really have a strong opinion one way or another, and I'll be happy to change the code to whatever people think is appropriate. Thanks, Guenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes 2020-07-06 19:41 ` Guenter Roeck @ 2020-07-06 20:07 ` Prashant Malani 2020-07-06 21:37 ` Guenter Roeck 0 siblings, 1 reply; 8+ messages in thread From: Prashant Malani @ 2020-07-06 20:07 UTC (permalink / raw) To: Guenter Roeck Cc: Enric Balletbo i Serra, Benson Leung, Linux Kernel Mailing List, Yu-Hsuan Hsu On Mon, Jul 6, 2020 at 12:41 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On Mon, Jul 06, 2020 at 11:52:30AM -0700, Prashant Malani wrote: > > Hi Guenter, > > > > On Sat, Jul 04, 2020 at 07:26:07AM -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. 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> > > > --- > > > 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]) > > > > Do we expect a case where cros_ec_error_map[msg->result] == 0? > > > > It seemed to be prudent to assume that this code is not going to be > updated whenever a new EC error code is added. Doing nothing would > risk returning 0, and addding WARN_ON or dev_warn seemed excessive. > Having said that, I don't really have a strong opinion one way > or another, and I'll be happy to change the code to whatever people > think is appropriate. Thanks for providing the rationale. I think if a new EC error code is added (and this array isn't updated), msg->result < ARRAY_SIZE(cros_ec_error_map) would return false, and the code block would return -EPROTO. I'll defer to the maintainer's opinion(s), but I think we can remove the condition after '&&'. Best regards, > > Thanks, > Guenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes 2020-07-06 20:07 ` Prashant Malani @ 2020-07-06 21:37 ` Guenter Roeck 2020-07-07 1:02 ` Prashant Malani 0 siblings, 1 reply; 8+ messages in thread From: Guenter Roeck @ 2020-07-06 21:37 UTC (permalink / raw) To: Prashant Malani Cc: Enric Balletbo i Serra, Benson Leung, Linux Kernel Mailing List, Yu-Hsuan Hsu On 7/6/20 1:07 PM, Prashant Malani wrote: > On Mon, Jul 6, 2020 at 12:41 PM Guenter Roeck <linux@roeck-us.net> wrote: >> >> On Mon, Jul 06, 2020 at 11:52:30AM -0700, Prashant Malani wrote: >>> Hi Guenter, >>> >>> On Sat, Jul 04, 2020 at 07:26:07AM -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. 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> >>>> --- >>>> 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]) >>> >>> Do we expect a case where cros_ec_error_map[msg->result] == 0? >>> >> >> It seemed to be prudent to assume that this code is not going to be >> updated whenever a new EC error code is added. Doing nothing would >> risk returning 0, and addding WARN_ON or dev_warn seemed excessive. >> Having said that, I don't really have a strong opinion one way >> or another, and I'll be happy to change the code to whatever people >> think is appropriate. > > Thanks for providing the rationale. I think if a new EC error code is > added (and this array isn't updated), > msg->result < ARRAY_SIZE(cros_ec_error_map) would return false, and > the code block would return -EPROTO. > Some scenarios: Developer 1 adds EC_RES_SOME_ERROR, and does not update the array. Developer 2 adds EC_RES_SOME_OTHER_ERROR and updates the array, but does not realize that EC_RES_SOME_ERROR is missing as well, and does not add it. Developer 3 adds two (or more) error codes, and does not update the array. Someone else later finds a -EPROTO return code and adds the necessary translation to the array. That translation happens to be for the last error code. The developer doing that does not realize that other error codes are missing as well, or does not realize the impact, and does not add translations for the other missing error codes. Overall there are too many situations where this can go wrong for me to trust that it never will. > I'll defer to the maintainer's opinion(s), but I think we can remove > the condition after '&&'. > I thought about it, but I find that I don't feel comfortable with doing that. If that is what is asked for, would you mind providing a separate patch which doesn't have my name on it ? Thanks, Guenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes 2020-07-06 21:37 ` Guenter Roeck @ 2020-07-07 1:02 ` Prashant Malani 0 siblings, 0 replies; 8+ messages in thread From: Prashant Malani @ 2020-07-07 1:02 UTC (permalink / raw) To: Guenter Roeck Cc: Enric Balletbo i Serra, Benson Leung, Linux Kernel Mailing List, Yu-Hsuan Hsu On Mon, Jul 6, 2020 at 2:38 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On 7/6/20 1:07 PM, Prashant Malani wrote: > > On Mon, Jul 6, 2020 at 12:41 PM Guenter Roeck <linux@roeck-us.net> wrote: > >> > >> On Mon, Jul 06, 2020 at 11:52:30AM -0700, Prashant Malani wrote: > >>> Hi Guenter, > >>> > >>> On Sat, Jul 04, 2020 at 07:26:07AM -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. 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> > >>>> --- > >>>> 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]) > >>> > >>> Do we expect a case where cros_ec_error_map[msg->result] == 0? > >>> > >> > >> It seemed to be prudent to assume that this code is not going to be > >> updated whenever a new EC error code is added. Doing nothing would > >> risk returning 0, and addding WARN_ON or dev_warn seemed excessive. > >> Having said that, I don't really have a strong opinion one way > >> or another, and I'll be happy to change the code to whatever people > >> think is appropriate. > > > > Thanks for providing the rationale. I think if a new EC error code is > > added (and this array isn't updated), > > msg->result < ARRAY_SIZE(cros_ec_error_map) would return false, and > > the code block would return -EPROTO. > > > > Some scenarios: > > Developer 1 adds EC_RES_SOME_ERROR, and does not update the array. > Developer 2 adds EC_RES_SOME_OTHER_ERROR and updates the array, but > does not realize that EC_RES_SOME_ERROR is missing as well, and does > not add it. > Developer 3 adds two (or more) error codes, and does not update the > array. Someone else later finds a -EPROTO return code and adds the > necessary translation to the array. That translation happens to be > for the last error code. The developer doing that does not realize > that other error codes are missing as well, or does not realize > the impact, and does not add translations for the other missing > error codes. > > Overall there are too many situations where this can go wrong for me > to trust that it never will. Fair enough. > > > I'll defer to the maintainer's opinion(s), but I think we can remove > > the condition after '&&'. > > > > I thought about it, but I find that I don't feel comfortable with > doing that. If that is what is asked for, would you mind providing > a separate patch which doesn't have my name on it ? > Thanks again for your explanation. As someone Cc-ed, I find it important (for me) to seek clarification behind the code. Thank you for providing that. As I alluded to earlier, over to the maintainer(s) now :) > Thanks, > Guenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes 2020-07-04 14:26 [PATCH] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes Guenter Roeck 2020-07-06 18:52 ` Prashant Malani @ 2020-07-07 10:10 ` Enric Balletbo i Serra 2020-07-07 11:43 ` Guenter Roeck 1 sibling, 1 reply; 8+ messages in thread From: Enric Balletbo i Serra @ 2020-07-07 10:10 UTC (permalink / raw) To: Guenter Roeck; +Cc: Benson Leung, linux-kernel, Yu-Hsuan Hsu, Prashant Malani Hi Prashant and Guenter, Thank you to spend your time on look at this. On 4/7/20 16:26, 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. 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> > --- For me the patch looks good as is. Said that, I'd like to discuss a bit more (but not too much) about it :-). My "concerns" are: - It is not clear to me if some EC errors match directly to a linux kernel error codes, the importance of them, and which action should be taken in that case or if just reporting is enough. - The EC result can be obtained, either, enabling more debug traces or using the linux kernel tracing tools. So, IMO mapping _all_ the errors has very little value. Right now, the policy I followed is return -EPROTO for all EC errors and introduce/match a new error when someone (another kernel driver or userspace needs to know about it, i.e [1], where some EC drivers do different actions if -ENOTSUPP is returned). We introduced that error because we had a use case for it. The future, I'd like to maintain this policy if it makes sense to you. And introduce a new error when we have a use case for it. I.e if at some point a kernel driver needs to know when the EC is busy (-EBUSY) because the driver waits and retries again, I'll be fine to introduce this new error/match code. Otherwise, I feel that has no really value. Said that, if you still feel, that this will help you for debugging purposes, I am completely fine to pick the patch. Thoughts? Thanks, Enric [1] commit c5cd2b47b203f63682778c2a1783198e6b644294 Author: Enric Balletbo i Serra <enric.balletbo@collabora.com> Date: Thu Feb 20 16:58:52 2020 +0100 platform/chrome: cros_ec_proto: Report command not supported In practice most drivers that use the EC protocol what really care is if the result was successful or not, hence, we introduced a cros_ec_cmd_xfer_status() function that converts EC errors to standard Linux error codes. On some few cases, though, we are interested on know if the command is supported or not, and in such cases, just ignore the error. To achieve this, return a -ENOTSUPP error when the command is not supported. > 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; > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes 2020-07-07 10:10 ` Enric Balletbo i Serra @ 2020-07-07 11:43 ` Guenter Roeck 0 siblings, 0 replies; 8+ messages in thread From: Guenter Roeck @ 2020-07-07 11:43 UTC (permalink / raw) To: Enric Balletbo i Serra Cc: Benson Leung, linux-kernel, Yu-Hsuan Hsu, Prashant Malani On 7/7/20 3:10 AM, Enric Balletbo i Serra wrote: > Hi Prashant and Guenter, > > Thank you to spend your time on look at this. > > On 4/7/20 16:26, 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. 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> >> --- > > For me the patch looks good as is. Said that, I'd like to discuss a bit more > (but not too much) about it :-). My "concerns" are: > > - It is not clear to me if some EC errors match directly to a linux kernel error > codes, the importance of them, and which action should be taken in that case or > if just reporting is enough. > > - The EC result can be obtained, either, enabling more debug traces or using the > linux kernel tracing tools. So, IMO mapping _all_ the errors has very little value. > > Right now, the policy I followed is return -EPROTO for all EC errors and > introduce/match a new error when someone (another kernel driver or userspace > needs to know about it, i.e [1], where some EC drivers do different actions if > -ENOTSUPP is returned). We introduced that error because we had a use case for it. > > The future, I'd like to maintain this policy if it makes sense to you. And > introduce a new error when we have a use case for it. I.e if at some point a > kernel driver needs to know when the EC is busy (-EBUSY) because the driver > waits and retries again, I'll be fine to introduce this new error/match code. > Otherwise, I feel that has no really value. > > Said that, if you still feel, that this will help you for debugging purposes, I > am completely fine to pick the patch. > > Thoughts? > Yes, I did feel it was useful, error codes are never perfect, it is rarely possible to enable tracing in situations in which the error that was observed, and I think it would have been and would be useful (having it would have saved hours of analysis and debugging time), but not for the cost of spending hours of argument about it. Please drop. Guenter ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-07-07 11:43 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-04 14:26 [PATCH] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes Guenter Roeck 2020-07-06 18:52 ` Prashant Malani 2020-07-06 19:41 ` Guenter Roeck 2020-07-06 20:07 ` Prashant Malani 2020-07-06 21:37 ` Guenter Roeck 2020-07-07 1:02 ` Prashant Malani 2020-07-07 10:10 ` Enric Balletbo i Serra 2020-07-07 11:43 ` Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).