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 for Android: free password hash cracker in your pocket

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

Message-ID: <20160620173253.GA40678@google.com>
Date:	Mon, 20 Jun 2016 10:32:53 -0700
From:	Brian Norris <briannorris@...omium.org>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	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>,
	Javier Martinez Canillas <javier@....samsung.com>,
	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

On Fri, Jun 17, 2016 at 09:12:20PM -0700, 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.
> >
> >How do you propose we do that? Do all of the following become EINVAL?
> >
> >         EC_RES_INVALID_COMMAND
> >         EC_RES_INVALID_PARAM
> >         EC_RES_INVALID_VERSION
> >         EC_RES_INVALID_HEADER
> >
> 
> Personal preference, but yes.
> 
> >We lose a lot of information that way. And particularly, cros_ec_num_pwms()
> >won't be able to count as accurately. But I can just go back to not using this
> 
> You lost me there, sorry.

If you look at the currently-proposed user of this API (patch 4), some
of the code in cros_ec_num_pwms() looks like this:

               /*
                * We look for SUCCESS, INVALID_COMMAND, or INVALID_PARAM responses;
                * everything else is treated as an error
                */
               if (ret == -EECRESULT - EC_RES_INVALID_COMMAND)
                       return -ENODEV;
               else if (ret == -EECRESULT - EC_RES_INVALID_PARAM)
                       return i;
               else if (ret < 0)
                       return ret;

I'd really like to know the difference between EC_RES_INVALID_COMMAND,
EC_RES_INVALID_PARAM, and all other error codes. You're suggesting
aliasing those with generic Linux error codes which could easily be used
for other things (e.g., reporting errors from the I2C or SPI layers,
which I'd want to treat differently). So either I decrease the
preciseness of the above code, or I just use cros_ec_cmd_xfer() instead
of cros_ec_cmd_xfer_status(), and then have to reimplement the error
handling that cros_ec_cmd_xfer_status() was going to (sort of) abstract
away.

But maybe that just means that pwm-cros-ec.c is really the odd-man-out
use case compared to the other users of cros_ec_cmd_xfer_status() in the
Chrome OS kernel tree, and so I should be using cros_ec_cmd_xfer().

> >API if that's what you'd like...
> >
> 
> That isn't what I suggested.

Brian