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: <20190315170413.108114-1-twawrzynczak@chromium.org> <20190315170413.108114-2-twawrzynczak@chromium.org>
In-Reply-To: <20190315170413.108114-2-twawrzynczak@chromium.org>
From:   Enric Balletbo Serra <eballetbo@gmail.com>
Date:   Thu, 21 Mar 2019 17:05:13 +0100
Message-ID: <CAFqH_50m-fkPHHPbLx_o-hYmc1vfV9MkTgvGWdVOTmOO36N86A@mail.gmail.com>
Subject: Re: [PATCH] platform/chrome: mfd/cros_ec_sysfs: Add sysfs entry to
 retrieve EC uptime.
To:     Tim Wawrzynczak <twawrzynczak@chromium.org>
Cc:     Benson Leung <bleung@chromium.org>,
        Olof Johansson <olof@lixom.net>,
        Lee Jones <lee.jones@linaro.org>,
        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

Hi Tim,

Thanks for sending this upstream, some comments below

Missatge de Tim Wawrzynczak <twawrzynczak@chromium.org> del dia dv.,
15 de mar=C3=A7 2019 a les 18:07:
>
> Adds a new sysfs node under the cros_ec device which contains the uptime
> of the EC in milliseconds since boot.
>
> Signed-off-by: Tim Wawrzynczak <twawrzynczak@chromium.org>
> ---
>  drivers/platform/chrome/cros_ec_sysfs.c | 34 +++++++++++++++++++++++++

>  include/linux/mfd/cros_ec_commands.h    | 15 +++++++++++

All this is already included in the following patch so it's not really
needed, rebase your patch on top of this and just resend the patch
without these modifications and tell that depends on that patch.

https://lore.kernel.org/patchwork/patch/1046363/

>  2 files changed, 49 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/c=
hrome/cros_ec_sysfs.c
> index 5a6db3fe213a..7e5ca1e2900b 100644
> --- a/drivers/platform/chrome/cros_ec_sysfs.c
> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> @@ -123,6 +123,38 @@ static ssize_t reboot_store(struct device *dev,
>         return count;
>  }
>
> +static ssize_t uptime_show(struct device *dev,
> +                          struct device_attribute *attr, char *buf)
> +{
> +       struct ec_response_uptime_info *resp;
> +       struct cros_ec_command *msg;
> +       int ret;
> +       struct cros_ec_dev *ec =3D to_cros_ec_dev(dev);
> +       u32 time_since_ec_boot_ms;
> +

nit: Reversed X-mas tree order if you don't mind.

> +       msg =3D kmalloc(sizeof(*msg) + sizeof(*resp), GFP_KERNEL);
> +       if (!msg)
> +               return -ENOMEM;
> +
> +       /* Get EC uptime information */
> +       msg->version =3D 0;
> +       msg->command =3D EC_CMD_GET_UPTIME_INFO + ec->cmd_offset;
> +       msg->insize =3D sizeof(*resp);
> +       msg->outsize =3D 0;
> +       ret =3D cros_ec_cmd_xfer_status(ec->ec_dev, msg);
> +       if (ret < 0) {
> +               time_since_ec_boot_ms =3D 0;

That's an error (because the command is not supported or other
reason).  Just goto the and exit label, free the memory and return the
error.

> +       } else {

else not needed

> +               resp =3D (struct ec_response_uptime_info *)msg->data;
> +               time_since_ec_boot_ms =3D resp->time_since_ec_boot_ms;
> +       }
> +
> +       ret =3D scnprintf(buf, PAGE_SIZE, "%u\n", time_since_ec_boot_ms);
> +
> +       kfree(msg);
> +       return ret;
> +}
> +
>  static ssize_t version_show(struct device *dev,
>                             struct device_attribute *attr, char *buf)
>  {
> @@ -330,12 +362,14 @@ static DEVICE_ATTR_RW(reboot);
>  static DEVICE_ATTR_RO(version);
>  static DEVICE_ATTR_RO(flashinfo);
>  static DEVICE_ATTR_RW(kb_wake_angle);
> +static DEVICE_ATTR_RO(uptime);
>

The time is stored in u32 which means that 2^32 in ms is less than 50
days. Overflow this counter doesn't seem too complicated. What happens
when overflows just starts from 0 again? I'm curious why is this
really useful show this in sysfs? There is a use case for that or is
just information.

>  static struct attribute *__ec_attrs[] =3D {
>         &dev_attr_kb_wake_angle.attr,
>         &dev_attr_reboot.attr,
>         &dev_attr_version.attr,
>         &dev_attr_flashinfo.attr,
> +       &dev_attr_uptime.attr,

Is the EC_CMD_GET_UPTIME_INFO supported by all the Cros EC|PD|FP|TP etc.?

I tested on my Samsung Chromebook Plus and seems to be not supported
(ectool fails)

If there is a way to filter if the command is supported or not will be
good only show the attribute when it is supposed to work.

>         NULL,
>  };
>
> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cro=
s_ec_commands.h
> index a8c882b5c222..38e7e6dcca97 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h
> @@ -4697,6 +4697,21 @@ struct __ec_align4 ec_params_rwsig_action {
>         uint32_t action;
>  };
>
> +/* For getting EC uptime & AP reset information */
> +#define EC_CMD_GET_UPTIME_INFO 0x0121
> +
> +struct __ec_align4 ec_response_uptime_info {
> +       uint32_t time_since_ec_boot_ms;   /* uptime in ms */
> +       uint32_t ap_resets_since_ec_boot; /* AP resets recorded by EC */
> +       uint32_t ec_reset_flags;          /* RESET_* flags */
> +       /* Empty log entries have cause and timestamp set to zero */
> +       struct ap_reset_log_entry {
> +               uint16_t reset_cause;     /* enum chipset_*_reason */
> +               uint16_t reserved;        /* reserved for protocol growth=
 */
> +               uint32_t reset_time_ms;   /* time of reset assertion */
> +       } recent_ap_reset[4];
> +};
> +
>  /***********************************************************************=
******/
>  /* The command range 0x200-0x2FF is reserved for Rotor. */
>
> --
> 2.20.1
>

Thanks,
 Enric