Messages in this thread | | | From | Prashant Malani <> | Date | Mon, 6 Jul 2020 18:02:30 -0700 | Subject | Re: [PATCH] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes |
| |
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
| |