From: Brian Norris <briannorris@chromium.org>
To: Gwendal Grignou <gwendal@google.com>
Cc: Enrico Granata <egranata@google.com>,
Lee Jones <lee.jones@linaro.org>,
Benson Leung <bleung@chromium.org>,
Olof Johansson <olof@lixom.net>,
Linux Kernel <linux-kernel@vger.kernel.org>,
Enrico Granata <egranata@chromium.org>
Subject: Re: [PATCH] mfd: cros_ec: Add support for MKBP more event flags
Date: Mon, 14 Jan 2019 18:03:54 -0800 [thread overview]
Message-ID: <CA+ASDXPt5fws82r71xjuVucB=gZCYVSjx5dMaGdbW6qFOuG0vA@mail.gmail.com> (raw)
In-Reply-To: <CAMHSBOU0qNrCfg5=N-4KXBS_0fTBRbHGY2V9R7Q9E9ZY7R-1Mg@mail.gmail.com>
Hi Gwendal,
On Mon, Jan 14, 2019 at 3:50 PM Gwendal Grignou <gwendal@google.com> wrote:
> On Fri, Dec 7, 2018 at 2:22 PM Brian Norris <briannorris@chromium.org> wrote:
> > On Thu, Nov 29, 2018 at 11:55:48AM -0800, egranata@google.com wrote:
> > > --- a/drivers/platform/chrome/cros_ec_proto.c
> > > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > > @@ -420,10 +420,14 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
> > > ret = cros_ec_get_host_command_version_mask(ec_dev,
> > > EC_CMD_GET_NEXT_EVENT,
> > > &ver_mask);
> >
> > It's not exactly new here (although you're using 'ver_mask' in new
> > ways), but cros_ec_get_host_command_version_mask() doesn't look 100%
> > right. It doesn't look at msg->result, and instead just assumes that if
> > we got some data back (send_command() > 0), then it must have been a
> > success. I don't think that's really guaranteed in general, although it
> > might be for the specific case of EC_CMD_GET_CMD_VERSIONS.
> It is guaranteed: if msg->result is not EC_RES_SUCCESS, then ret can
> not be greater than 0. At best it will be 0, or a negative number if
> we can already qualify the error in the errno space (see
> cros_ec_pkt_xfer_i2c() for instance).
Sorry, where do you guarantee that? The only enforcements I see in
those xfer implementation are:
(1) if result == EC_RES_IN_PROGRESS, we convert that to an errno
(2) if the expected length or checksum are bad, we turn that to an errno
So technically, the EC *could* be sending a valid, checksummed
response of the expected length, while still setting the ->result
field to something besides EC_RES_SUCCESS or EC_RES_IN_PROGRESS. And
we would treat that as a valid 'ver_mask'.
Albeit, that seems unlikely, given understanding of how the EC is
supposed to behave, but our code is not properly defensive AIUI. This
is basically why cros_ec_cmd_xfer_status() exists -- so that
sub-drivers don't get lazy and use cros_ec_cmd_xfer() without handling
the ->result field properly.
Brian
next prev parent reply other threads:[~2019-01-15 2:04 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-29 19:55 [PATCH] mfd: cros_ec: Add support for MKBP more event flags egranata
2018-12-03 11:08 ` Lee Jones
2018-12-07 22:22 ` Brian Norris
2018-12-21 0:29 ` egranata
2018-12-21 7:24 ` Lee Jones
2018-12-21 21:44 ` [PATCH v2] " egranata
2019-01-14 23:49 ` [PATCH] " Gwendal Grignou
2019-01-15 2:03 ` Brian Norris [this message]
2019-01-15 5:42 ` Gwendal Grignou
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+ASDXPt5fws82r71xjuVucB=gZCYVSjx5dMaGdbW6qFOuG0vA@mail.gmail.com' \
--to=briannorris@chromium.org \
--cc=bleung@chromium.org \
--cc=egranata@chromium.org \
--cc=egranata@google.com \
--cc=gwendal@google.com \
--cc=lee.jones@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=olof@lixom.net \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).