Linux Input Archive on lore.kernel.org
 help / color / Atom feed
From: Brian Norris <briannorris@chromium.org>
To: Guenter Roeck <linux@roeck-us.net>
Cc: "Enric Balletbo i Serra" <enric.balletbo@collabora.com>,
	"Jonathan Cameron" <jic23@kernel.org>,
	"Benson Leung" <bleung@chromium.org>,
	"Dmitry Torokhov" <dmitry.torokhov@gmail.com>,
	"Thierry Reding" <thierry.reding@gmail.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Gwendal Grignou" <gwendal@chromium.org>,
	linux-iio@vger.kernel.org,
	"Linux Kernel" <linux-kernel@vger.kernel.org>,
	linux-input@vger.kernel.org,
	linux-pwm <linux-pwm@vger.kernel.org>,
	"Yu-Hsuan Hsu" <yuhsuan@chromium.org>,
	"Prashant Malani" <pmalani@chromium.org>
Subject: Re: [PATCH v3 6/6] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes
Date: Wed, 29 Jul 2020 16:29:16 -0700
Message-ID: <CA+ASDXNSiuXEUmKhM8KvySK2OrW62yO1ZhR4Eigtx57CG33i4A@mail.gmail.com> (raw)
In-Reply-To: <ae5aeae1-4dfc-0fd7-3392-bf1ca3540866@roeck-us.net>

On Wed, Jul 29, 2020 at 4:22 PM Guenter Roeck <linux@roeck-us.net> wrote:
> On 7/29/20 3:21 PM, Brian Norris wrote:
> > On Sun, Jul 26, 2020 at 03:01:01PM -0700, Guenter Roeck wrote:
> >> --- a/drivers/platform/chrome/cros_ec_proto.c
> >> +++ b/drivers/platform/chrome/cros_ec_proto.c

> > ^^ Maybe we want to double check 'ret != 0'? Or maybe
> >
> >                       ret = cros_ec_error_map[result];
> >                       if (!ret) {
>
> 'ret' won't ever be 0 here. Above:
>                                                         && cros_ec_error_map[result]
>
> and below:
>
>                 else
>                         ret = -EPROTO;

Ah, I'm reading too quickly. You're correct, sorry.

> >                               ret = -EPROTO;
> >                               dev_err(..., "Unexpected EC result code %d\n", result);
> >                       }
> >
> > ? Could even be WARN_ON(), since this would be an actionable programming
> > error, not exactly an external factor. Or maybe I'm being paranoid, and
> > future programmers are perfect.
> >
> I think, if anything, we might consider adding the message below (result >=
> ARRAY_SIZE(cros_ec_error_map) is just as bad). Not sure myself. I am
> open to adding it if people think it would be useful/desirable.

No, my primary motivation was that I thought the logic left room for
error if there were holes. I was mistaken on that point. Secondarily,
it was also potentially useful to point out when we fell into those
holes. I'm not sure logging the warning is that important. Generally,
we only care about a handful of result codes, and as long as the rest
don't end up as "success", I think we're in OK shape.

Sorry for the noise. Here's my tag (which given my misreading so far,
should probably have a heavy discount on its value):

Reviewed-by: Brian Norris <briannorris@chromium.org>

  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-26 22:00 [PATCH v3 0/6] " Guenter Roeck
2020-07-26 22:00 ` [PATCH v3 1/6] iio: cros_ec: Accept -EOPNOTSUPP as 'not supported' error code Guenter Roeck
2020-07-26 22:00 ` [PATCH v3 2/6] cros_ec_lightbar: Accept more error codes from cros_ec_cmd_xfer_status Guenter Roeck
2020-07-26 22:00 ` [PATCH v3 3/6] platform/chrome: cros_ec_sysfs: Report range of error codes from EC Guenter Roeck
2020-07-26 22:00 ` [PATCH v3 4/6] pwm: cros-ec: Accept more error codes from cros_ec_cmd_xfer_status Guenter Roeck
2020-07-27  8:35   ` Thierry Reding
2020-08-01  7:21   ` Uwe Kleine-König
2020-08-01 16:32     ` Guenter Roeck
2020-08-02 20:27       ` Uwe Kleine-König
2020-07-26 22:01 ` [PATCH v3 5/6] platform/input: cros_ec: Replace -ENOTSUPP with -ENOPROTOOPT Guenter Roeck
2020-07-30  4:53   ` Dmitry Torokhov
2020-07-26 22:01 ` [PATCH v3 6/6] platform/chrome: cros_ec_proto: Convert EC error codes to Linux error codes Guenter Roeck
2020-07-29 22:21   ` Brian Norris
2020-07-29 23:22     ` Guenter Roeck
2020-07-29 23:29       ` Brian Norris [this message]
2020-07-29 22:27 ` [PATCH v3 0/6] " Brian Norris

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CA+ASDXNSiuXEUmKhM8KvySK2OrW62yO1ZhR4Eigtx57CG33i4A@mail.gmail.com \
    --to=briannorris@chromium.org \
    --cc=bleung@chromium.org \
    --cc=dmitry.torokhov@gmail.com \
    --cc=enric.balletbo@collabora.com \
    --cc=gwendal@chromium.org \
    --cc=jic23@kernel.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=pmalani@chromium.org \
    --cc=thierry.reding@gmail.com \
    --cc=u.kleine-koenig@pengutronix.de \
    --cc=yuhsuan@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux Input Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-input/0 linux-input/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-input linux-input/ https://lore.kernel.org/linux-input \
		linux-input@vger.kernel.org
	public-inbox-index linux-input

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-input


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git