Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20181129195548.204153-1-egranata@chromium.org>
 <20181207222223.GA240898@google.com> <CAMHSBOU0qNrCfg5=N-4KXBS_0fTBRbHGY2V9R7Q9E9ZY7R-1Mg@mail.gmail.com>
 <CA+ASDXPt5fws82r71xjuVucB=gZCYVSjx5dMaGdbW6qFOuG0vA@mail.gmail.com>
In-Reply-To: <CA+ASDXPt5fws82r71xjuVucB=gZCYVSjx5dMaGdbW6qFOuG0vA@mail.gmail.com>
From:   Gwendal Grignou <gwendal@google.com>
Date:   Mon, 14 Jan 2019 21:42:30 -0800
Message-ID: <CAMHSBOVy+n+nf+Y_mu21QkNd9U7=Tq4=R6dQFLB_xy=Uh4AHkQ@mail.gmail.com>
Subject: Re: [PATCH] mfd: cros_ec: Add support for MKBP more event flags
To:     Brian Norris <briannorris@chromium.org>
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>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jan 14, 2019 at 6:04 PM Brian Norris <briannorris@chromium.org> wrote:
>
> 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
> > T() 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'.
You're right, I misread cros_ec_pkt_xfer_i2c().
> 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.
send_command is called for a very small subset of command where the
ec_dev mutex is already held. We indeed need to be careful when
calling it directly.

Gwendal.
>
> Brian