Hi,
On Mon, Jun 20, 2016 at 09:46:57AM -0400, Javier Martinez Canillas wrote:
> 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.
Agreed, I suppose. It's probably best to make it unlikely other that
"client" drivers of cros-ec will blindly propagate nonstandard error
codes.
> >> 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.
Ah, well that last point is a nice reminder I guess, although that still
doesn't fit the way cros_ec_num_pwms() uses this API. But I can try to
refactor it to fit; cros_ec_num_pwms() will need to get two return
codes from cros_ec_pwm_get_duty() -- uglier, IMO, but doable.
> >> 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.
I'll take a look at adding an error code translation table when I get a
chance. Hopefully that doesn't delay the others who are planning to use
this API shortly...
Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
