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>
In-Reply-To: <CAPR809vTZSFtNp61WQsOFRkFacO_8XEdicqFq9uYMkUMdjEsqg@mail.gmail.com>
From:   Guenter Roeck <groeck@google.com>
Date:   Wed, 3 Apr 2019 12:53:40 -0700
Message-ID: <CABXOdTeE5=iF24+2JxwjvGtVtFmneYJWC4yXgX1Ah4y+=+ecEg@mail.gmail.com>
Subject: Re: [PATCH] mfd: cros_ec: check for NULL transfer function
To:     Enrico Granata <egranata@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

On Wed, Apr 3, 2019 at 12:40 PM Enrico Granata <egranata@google.com> wrote:
>
> I can certainly add a "did_print_error" flag or some such, but in practic=
e, if the transfer function is NULL, the initial handshake will fail, and t=
his will in turn cause EC registration to fail, and no further communicatio=
n 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/platfor=
m/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_d=
ev,
>> >         else
>> >                 xfer_fxn =3D ec_dev->cmd_xfer;
>> >
>> > +       if (xfer_fxn =3D=3D NULL) {
>> > +               /* This error can happen if a communication error happ=
ened and
>> > +                * the EC is trying to use protocol v2, on an underlyi=
ng
>> > +                * 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 command\=
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
>> >