2020-05-21 05:32:59

by Gwendal Grignou

Subject: [PATCH] platform: cros_ec_debugfs: control uptime information request

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.

Signed-off-by: Gwendal Grignou <[email protected]>
---
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;
};

/*
@@ -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;
--
2.26.2.761.g0e0b3e54be-goog

2020-05-21 09:23:28

by Enric Balletbo i Serra

Subject: Re: [PATCH] platform: cros_ec_debugfs: control uptime information request

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.

> Signed-off-by: Gwendal Grignou <[email protected]>
> ---
> 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.

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;
>

2020-05-26 18:54:12

by Gwendal Grignou

Subject: Re: [PATCH] platform: cros_ec_debugfs: control uptime information request

On Thu, May 21, 2020 at 2:21 AM Enric Balletbo i Serra
<[email protected]> 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 <[email protected]>
> > ---
> > 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;
> >