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  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]
Date:   Sat, 1 Aug 2020 09:32:19 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
Cc:     Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Benson Leung <bleung@...omium.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Gwendal Grignou <gwendal@...omium.org>,
        Brian Norris <briannorris@...omium.org>,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-input@...r.kernel.org, linux-pwm@...r.kernel.org,
        Yu-Hsuan Hsu <yuhsuan@...omium.org>,
        Prashant Malani <pmalani@...omium.org>
Subject: Re: [PATCH v3 4/6] pwm: cros-ec: Accept more error codes from
 cros_ec_cmd_xfer_status

On Sat, Aug 01, 2020 at 09:21:30AM +0200, Uwe Kleine-König wrote:
> On Sun, Jul 26, 2020 at 03:00:59PM -0700, Guenter Roeck wrote:
> > Since commit c5cd2b47b203 ("platform/chrome: cros_ec_proto: Report command
> > not supported") we can no longer assume that cros_ec_cmd_xfer_status()
> > reports -EPROTO for all errors returned by the EC itself. A follow-up
> > patch will change cros_ec_cmd_xfer_status() to report additional errors
> > reported by the EC as distinguished Linux error codes.
> > 
> > Handle this change by no longer assuming that only -EPROTO is used
> > to report all errors returned by the EC itself. Instead, support both
> > the old and the new error codes.
> > 
> > Cc: Gwendal Grignou <gwendal@...omium.org>
> > Cc: Yu-Hsuan Hsu <yuhsuan@...omium.org>
> > Cc: Prashant Malani <pmalani@...omium.org>
> > Cc: Brian Norris <briannorris@...omium.org>
> > Signed-off-by: Guenter Roeck <linux@...ck-us.net>
> > ---
> > v3: Added patch
> > 
> >  drivers/pwm/pwm-cros-ec.c | 21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
> > index 09c08dee099e..ef05fba1bd37 100644
> > --- a/drivers/pwm/pwm-cros-ec.c
> > +++ b/drivers/pwm/pwm-cros-ec.c
> > @@ -213,20 +213,27 @@ static int cros_ec_num_pwms(struct cros_ec_device *ec)
> >  		u32 result = 0;
> >  
> >  		ret = __cros_ec_pwm_get_duty(ec, i, &result);
> > -		/* We want to parse EC protocol errors */
> > -		if (ret < 0 && !(ret == -EPROTO && result))
> > -			return ret;
> > -
> >  		/*
> >  		 * We look for SUCCESS, INVALID_COMMAND, or INVALID_PARAM
> >  		 * responses; everything else is treated as an error.
> >  		 */
> 
> This comment is at least misleading now.
> 
Good point. I'll rephrase.

> > -		if (result == EC_RES_INVALID_COMMAND)
> > +		switch (ret) {
> > +		case -EOPNOTSUPP:	/* invalid command */
> >  			return -ENODEV;
> 
> My first reaction here was to wonder why -EOPNOTSUPP isn't passed to the
> upper layer. OK, this is a loop to test the number of available devices.
> 
I'll be happy to add a comment.

> > -		else if (result == EC_RES_INVALID_PARAM)
> > +		case -EINVAL:		/* invalid parameter */
> >  			return i;
> > -		else if (result)
> > +		case -EPROTO:
> > +			/* Old or new error return code: Handle both */
> > +			if (result == EC_RES_INVALID_COMMAND)
> > +				return -ENODEV;
> > +			else if (result == EC_RES_INVALID_PARAM)
> > +				return i;
> 
> If I understand correctly this surprising calling convention (output
> parameter is filled even though the function returned an error) is the
> old one that is to be fixed.
> 
Sorry, I don't get your point. This is the old convention, correct,
which we still want to support at this point. Plus, it matches the
current code, as surprosing as it may be.

Guenter

> >  			return -EPROTO;
> > +		default:
> > +			if (ret < 0)
> > +				return ret;
> > +			break;
> > +		}
> >  	}
> >  
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |


Powered by blists - more mailing lists