Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1426618

Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper

Show all headers | View raw

Hello Guenter, Brian

On 06/18/2016 01:09 PM, Guenter Roeck wrote:
> On 06/17/2016 06:08 PM, Brian Norris wrote:
>> On Fri, Jun 17, 2016 at 02:41:51PM -0700, Guenter Roeck wrote:
>>> On Fri, Jun 17, 2016 at 12:58:12PM -0700, Brian Norris wrote:
>>>> +int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
>>>> +                struct cros_ec_command *msg)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    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_SUCCESS)
>>>> +        return -EECRESULT - msg->result;
>>>
>>> I have been wondering about the error return codes here, and if they should be
>>> converted to standard Linux error codes. For example, I just hit error -1003
>>> with a driver I am working on. This translates to EC_RES_INVALID_PARAM, or,
>>> in Linux terms, -EINVAL. I think it would be better to use standard error
>>> codes, especially since some of the errors are logged.
>>

Agreed, specially since drivers may (wrongly) propagate whatever is returned
by this function to higher layers where the ChromeOS EC firmware error codes
makes no sense. So that will be a bug and can increase the cognitive load of
getting some weird error codes in core kernel code and developers may wonder
from where those came from until finally find that a EC driver returned that.

>> How do you propose we do that? Do all of the following become EINVAL?
>>

Yes, I would just do that.

The idea of this helper is to remove duplicated code and AFAICT what most EC
drivers do is something similar to the following:

        ret = cros_ec_cmd_xfer(ec, msg);
        if (ret < 0)
	   	return ret;

        if (msg->result != EC_RES_SUCCESS) {
                dev_dbg(ec->dev, "EC result %d\n", msg->result);
                return -EINVAL;
        }

So in practice what most drivers really care is if the result was successful
or not, I don't see specific EC error handling in the EC drivers.  The real
EC error code is still in the message anyways so drivers that do cares about
the real EC error can look at msg->result instead.

>>          EC_RES_INVALID_COMMAND
> 
>     -EOPNOTSUPP
> 
>>          EC_RES_INVALID_PARAM
> 
>     -EINVAL or -EBADMSG
> 
>>          EC_RES_INVALID_VERSION
> 
>     -EPROTO or -EBADR or -EBADE or -EBADRQC or -EPROTOOPT
> 
>>          EC_RES_INVALID_HEADER
> 
>     -EPROTO or -EBADR or -EBADE
> 
> Doesn't look that bad to me. Also, the raw error could still be logged,
> for example with dev_dbg().
>

Yes, I think that adding a dev_dbg() with the real EC error code should
be enough, that's basically what drivers do since they can't propagate
the EC error to higher layers anyways.

> Guenter
> 
>>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v3 0/4] pwm: add support for ChromeOS EC PWM Brian Norris <briannorris@chromium.org> - 2016-06-17 22:00 +0200
  [PATCH v3 1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper Brian Norris <briannorris@chromium.org> - 2016-06-17 22:00 +0200
    Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper Guenter Roeck <linux@roeck-us.net> - 2016-06-17 23:50 +0200
      Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper Brian Norris <briannorris@chromium.org> - 2016-06-18 03:10 +0200
        Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper Guenter Roeck <linux@roeck-us.net> - 2016-06-18 06:20 +0200
          Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper Brian Norris <briannorris@chromium.org> - 2016-06-20 19:40 +0200
        Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper Guenter Roeck <linux@roeck-us.net> - 2016-06-18 19:20 +0200
          Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper Javier Martinez Canillas <javier@osg.samsung.com> - 2016-06-20 15:50 +0200
            Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper Brian Norris <briannorris@chromium.org> - 2016-06-20 19:50 +0200
              Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper Javier Martinez Canillas <javier@osg.samsung.com> - 2016-06-20 20:30 +0200
              Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper Brian Norris <briannorris@chromium.org> - 2016-06-20 22:10 +0200
  [PATCH v3 4/4] pwm: add ChromeOS EC PWM driver Brian Norris <briannorris@chromium.org> - 2016-06-17 22:00 +0200
  [PATCH v3 3/4] doc: dt: pwm: add binding for ChromeOS EC PWM Brian Norris <briannorris@chromium.org> - 2016-06-17 22:00 +0200

csiph-web