On Wed, Apr 3, 2019 at 12:40 PM Enrico Granata <egran...@google.com> wrote:
>
> I can certainly add a "did_print_error" flag or some such, but in practice,
> if the transfer function is NULL, the initial handshake will fail, and this
> will in turn cause EC registration to fail, and no further communication
> 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 | egran...@google.com | ChromeOS | MTV1600
>
>
> On Wed, Apr 3, 2019 at 11:51 AM Guenter Roeck <gro...@google.com> wrote:
>>
>> On Wed, Apr 3, 2019 at 11:31 AM <egran...@chromium.org> wrote:
>> >
>> > From: Enrico Granata <egran...@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 <egran...@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/platform/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 = ec_dev->cmd_xfer;
>> >
>> > + if (xfer_fxn == NULL) {
>> > + /* This error can happen if a communication error happened
>> > and
>> > + * the EC is trying to use protocol v2, on an underlying
>> > + * 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 = (*xfer_fxn)(ec_dev, msg);
>> > if (msg->result == EC_RES_IN_PROGRESS) {
>> > int i;
>> > --
>> > 2.21.0.392.gf8f6787159e-goog
>> >
