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 <[email protected]>
Cc: Prashant Malani <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
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
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 <[email protected]>
> Cc: Prashant Malani <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> 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
>
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 <[email protected]>
> > Cc: Prashant Malani <[email protected]>
> > Signed-off-by: Guenter Roeck <[email protected]>
> > ---
> > 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
On Mon, Jul 6, 2020 at 12:41 PM Guenter Roeck <[email protected]> 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 <[email protected]>
> > > Cc: Prashant Malani <[email protected]>
> > > Signed-off-by: Guenter Roeck <[email protected]>
> > > ---
> > > 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
On 7/6/20 1:07 PM, Prashant Malani wrote:
> On Mon, Jul 6, 2020 at 12:41 PM Guenter Roeck <[email protected]> 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 <[email protected]>
>>>> Cc: Prashant Malani <[email protected]>
>>>> Signed-off-by: Guenter Roeck <[email protected]>
>>>> ---
>>>> 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
On Mon, Jul 6, 2020 at 2:38 PM Guenter Roeck <[email protected]> wrote:
>
> On 7/6/20 1:07 PM, Prashant Malani wrote:
> > On Mon, Jul 6, 2020 at 12:41 PM Guenter Roeck <[email protected]> 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 <[email protected]>
> >>>> Cc: Prashant Malani <[email protected]>
> >>>> Signed-off-by: Guenter Roeck <[email protected]>
> >>>> ---
> >>>> 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
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 <[email protected]>
> Cc: Prashant Malani <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
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 <[email protected]>
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;
>
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 <[email protected]>
>> Cc: Prashant Malani <[email protected]>
>> Signed-off-by: Guenter Roeck <[email protected]>
>> ---
>
> 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