Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200521052832.66620-1-gwendal@chromium.org> <e83f3849-9e2d-4919-1a65-35f63eabe993@collabora.com>
In-Reply-To: <e83f3849-9e2d-4919-1a65-35f63eabe993@collabora.com>
From:   Gwendal Grignou <gwendal@chromium.org>
Date:   Tue, 26 May 2020 11:49:48 -0700
Message-ID: <CAPUE2ut37vnwT0hH+pCWVFRtMFX3v_qv+Zsq_OsM1i+SH95b6w@mail.gmail.com>
Subject: Re: [PATCH] platform: cros_ec_debugfs: control uptime information request
To:     Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc:     Guenter Roeck <groeck@chromium.org>,
        Benson Leung <bleung@chromium.org>,
        Tim Wawrzynczak <twawrzynczak@chromium.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, May 21, 2020 at 2:21 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Gwendal,
>
> Thank you for your patch.
>
> On 21/5/20 7:28, Gwendal Grignou wrote:
> > When EC does not support uptime command (EC_CMD_GET_UPTIME_INFO),
> > return -EPROTO to read of /sys/kernel/debug/cros_ec/uptime without
> > calling the EC after the first try.
> >
> > The EC console log will not contain EC_CMD_GET_UPTIME_INFO anymore.
> >
>
> Oh, what you mean with this? Uptime is only exposed via sysfs or I am missing
> something.
I was checking the ec console log (via
/sys/kernel/debug/cros_ec/console_log) to check the EC was not
receiving and erroring on the command (pixelbook)
>
> > Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> > ---
> >  drivers/platform/chrome/cros_ec_debugfs.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
> > index 6ae484989d1f5..70a29afb6d9e7 100644
> > --- a/drivers/platform/chrome/cros_ec_debugfs.c
> > +++ b/drivers/platform/chrome/cros_ec_debugfs.c
> > @@ -49,6 +49,8 @@ struct cros_ec_debugfs {
> >       struct delayed_work log_poll_work;
> >       /* EC panicinfo */
> >       struct debugfs_blob_wrapper panicinfo_blob;
> > +     /* EC uptime */
> > +     bool uptime_supported;
>
> Ideally, if uptime can be supported or not we should only expose uptime when is
> supported, so the sysfs entry should only be created when is supported, similar
> to what we do with the console_log. See cros_ec_create_console_log()
>
> If doing this doesn't break userspace, I'd prefer doing in that way.
Good point, sending a v2.

Gwendal.
>
> Thanks,
>  Enric
>
>
> >  };
> >
> >  /*
> > @@ -256,12 +258,19 @@ static ssize_t cros_ec_uptime_read(struct file *file, char __user *user_buf,
> >       char read_buf[32];
> >       int ret;
> >
> > +     if (!debug_info->uptime_supported)
> > +             return -EPROTO;
> > +
> >       resp = (struct ec_response_uptime_info *)&msg.resp;
> >
> >       msg.cmd.command = EC_CMD_GET_UPTIME_INFO;
> >       msg.cmd.insize = sizeof(*resp);
> >
> >       ret = cros_ec_cmd_xfer_status(ec_dev, &msg.cmd);
> > +     if (ret == -EPROTO && msg.cmd.result == EC_RES_INVALID_COMMAND) {
> > +             debug_info->uptime_supported = false;
> > +             return ret;
> > +     }
> >       if (ret < 0)
> >               return ret;
> >
> > @@ -434,6 +443,9 @@ static int cros_ec_debugfs_probe(struct platform_device *pd)
> >       debug_info->ec = ec;
> >       debug_info->dir = debugfs_create_dir(name, NULL);
> >
> > +     /* Give uptime a chance to run. */
> > +     debug_info->uptime_supported = true;
> > +
> >       ret = cros_ec_create_panicinfo(debug_info);
> >       if (ret)
> >               goto remove_debugfs;
> >