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: <20190403183048.56750-1-egranata@chromium.org> <CABXOdTfvRryiMrFAe4wmvt5mm2k==b1BShbzHQ68vp5C3NFETA@mail.gmail.com>
 <CAPR809vTZSFtNp61WQsOFRkFacO_8XEdicqFq9uYMkUMdjEsqg@mail.gmail.com> <CABXOdTeE5=iF24+2JxwjvGtVtFmneYJWC4yXgX1Ah4y+=+ecEg@mail.gmail.com>
In-Reply-To: <CABXOdTeE5=iF24+2JxwjvGtVtFmneYJWC4yXgX1Ah4y+=+ecEg@mail.gmail.com>
From:   Enrico Granata <egranata@google.com>
Date:   Wed, 3 Apr 2019 12:54:55 -0700
Message-ID: <CAPR809tbp_vHUNOprsosHnDm2fvThDsUY3Q4Zp9jQvVyX2=wTw@mail.gmail.com>
Subject: Re: [PATCH] mfd: cros_ec: check for NULL transfer function
To:     Guenter Roeck <groeck@google.com>
Cc:     Enrico Granata <egranata@chromium.org>,
        Benson Leung <bleung@chromium.org>,
        Guenter Roeck <groeck@chromium.org>,
        Gwendal Grignou <gwendal@chromium.org>,
        Alexandru M Stan <amstan@chromium.org>,
        Enric Balletbo i Serra <enric.balletbo@collabora.com>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Because I did not know about it till right now. Thanks for the
suggestion. Will upload a v2 as soon as I get a chance.

On Wed, Apr 3, 2019 at 12:53 PM Guenter Roeck <groeck@google.com> wrote:
>
> On Wed, Apr 3, 2019 at 12:40 PM Enrico Granata <egranata@google.com> wrot=
e:
> >
> > I can certainly add a "did_print_error" flag or some such, but in pract=
ice, if the transfer function is NULL, the initial handshake will fail, and=
 this will in turn cause EC registration to fail, and no further communicat=
ion should occur, so no further log entries will be printed.
> >
> Sorry, I am a bit lost. Why would dev_err_once() not work ?
>
> Guenter
>
> > Thanks
> >
> > Enrico Granata | egranata@google.com | ChromeOS | MTV1600
> >
> >
> > On Wed, Apr 3, 2019 at 11:51 AM Guenter Roeck <groeck@google.com> wrote=
:
> >>
> >> On Wed, Apr 3, 2019 at 11:31 AM <egranata@chromium.org> wrote:
> >> >
> >> > From: Enrico Granata <egranata@chromium.org>
> >> >
> >> > As new transfer mechanisms are added to the EC codebase, they may
> >> > not support v2 of the EC protocol.
> >> >
> >> > If the v3 initial handshake transfer fails, the kernel will try
> >> > and call cmd_xfer as a fallback. If v2 is not supported, cmd_xfer
> >> > will be NULL, and the code will end up causing a kernel panic.
> >> >
> >> > Add a check for NULL before calling the transfer function, along
> >> > with a helpful comment explaining how one might end up in this
> >> > situation.
> >> >
> >> > Signed-off-by: Enrico Granata <egranata@chromium.org>
> >> > ---
> >> >  drivers/platform/chrome/cros_ec_proto.c | 10 ++++++++++
> >> >  1 file changed, 10 insertions(+)
> >> >
> >> > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platf=
orm/chrome/cros_ec_proto.c
> >> > index 97a068dff192d..953076ab401aa 100644
> >> > --- a/drivers/platform/chrome/cros_ec_proto.c
> >> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> >> > @@ -56,6 +56,16 @@ static int send_command(struct cros_ec_device *ec=
_dev,
> >> >         else
> >> >                 xfer_fxn =3D ec_dev->cmd_xfer;
> >> >
> >> > +       if (xfer_fxn =3D=3D NULL) {
> >> > +               /* This error can happen if a communication error ha=
ppened and
> >> > +                * the EC is trying to use protocol v2, on an underl=
ying
> >> > +                * communication mechanism that does not support v2.
> >> > +                */
> >>
> >> I am not personally a friend of networking-style multi-line comments.
> >>
> >> > +               dev_err(ec_dev->dev,
> >> > +                       "missing EC transfer API, cannot send comman=
d\n");
> >>
> >> That message will be displayed each time a message is sent, ie in
> >> practice for each message. Is there any value in that, other than
> >> clogging the log ?
> >>
> >> Guenter
> >>
> >> > +               return -EIO;
> >> > +       }
> >> > +
> >> >         ret =3D (*xfer_fxn)(ec_dev, msg);
> >> >         if (msg->result =3D=3D EC_RES_IN_PROGRESS) {
> >> >                 int i;
> >> > --
> >> > 2.21.0.392.gf8f6787159e-goog
> >> >