linux-kernel - Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160620181020.GA46079@google.com>
Date:	Mon, 20 Jun 2016 11:10:22 -0700
From:	Brian Norris <briannorris@...omium.org>
To:	Javier Martinez Canillas <javier@....samsung.com>
Cc:	Guenter Roeck <linux@...ck-us.net>,
	Lee Jones <lee.jones@...aro.org>,
	Thierry Reding <thierry.reding@...il.com>,
	Olof Johansson <olof@...om.net>, linux-kernel@...r.kernel.org,
	Doug Anderson <dianders@...omium.org>,
	Brian Norris <computersforpeace@...il.com>,
	linux-pwm@...r.kernel.org, devicetree@...r.kernel.org,
	Boris Brezillon <boris.brezillon@...e-electrons.com>,
	Stephen Barber <smbarber@...omium.org>,
	Benson Leung <bleung@...omium.org>,
	Enric Balletbo <enric.balletbo@...labora.co.uk>,
	Randall Spangler <rspangler@...omium.org>,
	Shawn Nematbakhsh <shawnn@...omium.org>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Todd Broch <tbroch@...omium.org>,
	Gwendal Grignou <gwendal@...omium.org>,
	Tomeu Vizoso <tomeu.vizoso@...labora.com>
Subject: Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper

Hi,

On Mon, Jun 20, 2016 at 10:44:55AM -0700, Brian Norris wrote:
> 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:
> > >> 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.
> 
> 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...

Actually, I had some second thoughts, and others brought similar
concerns up to me:

What do we really win by doing the translation? It'll be difficult to do
a 1:1 translation, and any time the EC adds additional error types,
we'll have to update the translation. What's more, AFAICT, no one will
really be looking at the translated error codes now, and will just be
blindly passing them on. So maybe it makes more sense to just pick a
single error code and pass that on instead. Possibly just -EPROTO (or
nominate your favorite) for all EC error results, and if someone cares,
they decode msg->result in their driver, or check the dev_dbg() log?

Brian