2019-05-02 16:11:28

by Tim Wawrzynczak

[permalink] [raw]
Subject: [PATCH v4] platform/chrome: mfd/cros_ec_debugfs: Add debugfs entry to retrieve EC uptime.

The new debugfs entry 'uptime' is being made available to userspace so that
a userspace daemon can synchronize EC logs with host time.

Signed-off-by: Tim Wawrzynczak <[email protected]>
---
Enric, is there something I can do to help speed this along? This patch
is useful for ChromeOS board bringup, and we would like to see it upstreamed
if at all possible.

Also, AFAIK only the cros_ec supports the 'uptime' command for now.
And yes, the file does need to be seekable; the userspace daemon that
consumes the file keeps the file open and seeks back to the beginning
to get the latest uptime value.
Based on your second response to v3, I kept the separate 'create_uptime'
function b/c of the logic for checking support for the uptime command.
Let me know if you'd like me to move all of that logic into _probe.

Changelist from v3:
1) Don't check return values of debugfs_* functions.
2) Only expose 'uptime' file if EC supports it.
---
Documentation/ABI/testing/debugfs-cros-ec | 10 +++
drivers/platform/chrome/cros_ec_debugfs.c | 78 +++++++++++++++++++++++
2 files changed, 88 insertions(+)
create mode 100644 Documentation/ABI/testing/debugfs-cros-ec

diff --git a/Documentation/ABI/testing/debugfs-cros-ec b/Documentation/ABI/testing/debugfs-cros-ec
new file mode 100644
index 000000000000..24b781c67a4c
--- /dev/null
+++ b/Documentation/ABI/testing/debugfs-cros-ec
@@ -0,0 +1,10 @@
+What: /sys/kernel/debug/cros_ec/uptime
+Date: March 2019
+KernelVersion: 5.1
+Description:
+ Read-only.
+ Reads the EC's current uptime information
+ (using EC_CMD_GET_UPTIME_INFO) and prints
+ time_since_ec_boot_ms into the file.
+ This is used for synchronizing AP host time
+ with the cros_ec log.
diff --git a/drivers/platform/chrome/cros_ec_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
index 71308766e891..226545a2150b 100644
--- a/drivers/platform/chrome/cros_ec_debugfs.c
+++ b/drivers/platform/chrome/cros_ec_debugfs.c
@@ -201,6 +201,50 @@ static int cros_ec_console_log_release(struct inode *inode, struct file *file)
return 0;
}

+static int cros_ec_get_uptime(struct cros_ec_device *ec_dev, u32 *uptime)
+{
+ struct {
+ struct cros_ec_command msg;
+ struct ec_response_uptime_info resp;
+ } __packed ec_buf;
+ struct ec_response_uptime_info *resp;
+ struct cros_ec_command *msg;
+
+ msg = &ec_buf.msg;
+ resp = (struct ec_response_uptime_info *)msg->data;
+
+ msg->command = EC_CMD_GET_UPTIME_INFO;
+ msg->version = 0;
+ msg->insize = sizeof(*resp);
+ msg->outsize = 0;
+
+ ret = cros_ec_cmd_xfer_status(ec_dev, msg);
+ if (ret < 0)
+ return ret;
+
+ *uptime = resp->time_since_ec_boot_ms;
+ return 0;
+}
+
+static ssize_t cros_ec_uptime_read(struct file *file,
+ char __user *user_buf,
+ size_t count,
+ loff_t *ppos)
+{
+ struct cros_ec_debugfs *debug_info = file->private_data;
+ struct cros_ec_device *ec_dev = debug_info->ec->ec_dev;
+ char read_buf[32];
+ int ret;
+ u32 uptime;
+
+ ret = cros_ec_get_uptime(ec_dev, &uptime);
+ if (ret < 0)
+ return ret;
+
+ ret = scnprintf(read_buf, sizeof(read_buf), "%u\n", uptime);
+ return simple_read_from_buffer(user_buf, count, ppos, read_buf, ret);
+}
+
static ssize_t cros_ec_pdinfo_read(struct file *file,
char __user *user_buf,
size_t count,
@@ -269,6 +313,13 @@ const struct file_operations cros_ec_pdinfo_fops = {
.llseek = default_llseek,
};

+const struct file_operations cros_ec_uptime_fops = {
+ .owner = THIS_MODULE,
+ .open = simple_open,
+ .read = cros_ec_uptime_read,
+ .llseek = default_llseek,
+};
+
static int ec_read_version_supported(struct cros_ec_dev *ec)
{
struct ec_params_get_cmd_versions_v1 *params;
@@ -413,6 +464,29 @@ static int cros_ec_create_pdinfo(struct cros_ec_debugfs *debug_info)
return 0;
}

+static int cros_ec_create_uptime(struct cros_ec_debugfs *debug_info)
+{
+ struct cros_ec_debugfs *debug_info = file->private_data;
+ struct cros_ec_device *ec_dev = debug_info->ec->ec_dev;
+ u32 uptime;
+ int ret;
+
+ /*
+ * If the EC does not support the uptime command, which is
+ * indicated by xfer_status() returning -EINVAL, then no
+ * debugfs entry will be created.
+ */
+ ret = cros_ec_get_uptime(ec_dev, &uptime);
+
+ if (ret == -EINVAL)
+ return supported;
+
+ debugfs_create_file("uptime", 0444, debug_info->dir, debug_info,
+ &cros_ec_uptime_fops);
+
+ return 0;
+}
+
static int cros_ec_debugfs_probe(struct platform_device *pd)
{
struct cros_ec_dev *ec = dev_get_drvdata(pd->dev.parent);
@@ -442,6 +516,10 @@ static int cros_ec_debugfs_probe(struct platform_device *pd)
if (ret)
goto remove_log;

+ ret = cros_ec_create_uptime(debug_info);
+ if (ret)
+ goto remove_log;
+
ec->debug_info = debug_info;

dev_set_drvdata(&pd->dev, ec);
--
2.20.1


2019-05-02 21:19:49

by Enric Balletbo Serra

[permalink] [raw]
Subject: Re: [PATCH v4] platform/chrome: mfd/cros_ec_debugfs: Add debugfs entry to retrieve EC uptime.

Hi Tim,

Missatge de Tim Wawrzynczak <[email protected]> del dia dj., 2
de maig 2019 a les 18:10:
>
> The new debugfs entry 'uptime' is being made available to userspace so that
> a userspace daemon can synchronize EC logs with host time.
>
> Signed-off-by: Tim Wawrzynczak <[email protected]>
> ---
> Enric, is there something I can do to help speed this along? This patch
> is useful for ChromeOS board bringup, and we would like to see it upstreamed
> if at all possible.
>

The last version looks good to me. The patch is in my list but is too
late for the next merge window. Will be one of the first patches I'll
queue for 5.3

Thanks,
Enric

> Also, AFAIK only the cros_ec supports the 'uptime' command for now.
> And yes, the file does need to be seekable; the userspace daemon that
> consumes the file keeps the file open and seeks back to the beginning
> to get the latest uptime value.
> Based on your second response to v3, I kept the separate 'create_uptime'
> function b/c of the logic for checking support for the uptime command.
> Let me know if you'd like me to move all of that logic into _probe.
>
> Changelist from v3:
> 1) Don't check return values of debugfs_* functions.
> 2) Only expose 'uptime' file if EC supports it.
> ---
> Documentation/ABI/testing/debugfs-cros-ec | 10 +++
> drivers/platform/chrome/cros_ec_debugfs.c | 78 +++++++++++++++++++++++
> 2 files changed, 88 insertions(+)
> create mode 100644 Documentation/ABI/testing/debugfs-cros-ec
>
> diff --git a/Documentation/ABI/testing/debugfs-cros-ec b/Documentation/ABI/testing/debugfs-cros-ec
> new file mode 100644
> index 000000000000..24b781c67a4c
> --- /dev/null
> +++ b/Documentation/ABI/testing/debugfs-cros-ec
> @@ -0,0 +1,10 @@
> +What: /sys/kernel/debug/cros_ec/uptime
> +Date: March 2019
> +KernelVersion: 5.1
> +Description:
> + Read-only.
> + Reads the EC's current uptime information
> + (using EC_CMD_GET_UPTIME_INFO) and prints
> + time_since_ec_boot_ms into the file.
> + This is used for synchronizing AP host time
> + with the cros_ec log.
> diff --git a/drivers/platform/chrome/cros_ec_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
> index 71308766e891..226545a2150b 100644
> --- a/drivers/platform/chrome/cros_ec_debugfs.c
> +++ b/drivers/platform/chrome/cros_ec_debugfs.c
> @@ -201,6 +201,50 @@ static int cros_ec_console_log_release(struct inode *inode, struct file *file)
> return 0;
> }
>
> +static int cros_ec_get_uptime(struct cros_ec_device *ec_dev, u32 *uptime)
> +{
> + struct {
> + struct cros_ec_command msg;
> + struct ec_response_uptime_info resp;
> + } __packed ec_buf;
> + struct ec_response_uptime_info *resp;
> + struct cros_ec_command *msg;
> +
> + msg = &ec_buf.msg;
> + resp = (struct ec_response_uptime_info *)msg->data;
> +
> + msg->command = EC_CMD_GET_UPTIME_INFO;
> + msg->version = 0;
> + msg->insize = sizeof(*resp);
> + msg->outsize = 0;
> +
> + ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> + if (ret < 0)
> + return ret;
> +
> + *uptime = resp->time_since_ec_boot_ms;
> + return 0;
> +}
> +
> +static ssize_t cros_ec_uptime_read(struct file *file,
> + char __user *user_buf,
> + size_t count,
> + loff_t *ppos)
> +{
> + struct cros_ec_debugfs *debug_info = file->private_data;
> + struct cros_ec_device *ec_dev = debug_info->ec->ec_dev;
> + char read_buf[32];
> + int ret;
> + u32 uptime;
> +
> + ret = cros_ec_get_uptime(ec_dev, &uptime);
> + if (ret < 0)
> + return ret;
> +
> + ret = scnprintf(read_buf, sizeof(read_buf), "%u\n", uptime);
> + return simple_read_from_buffer(user_buf, count, ppos, read_buf, ret);
> +}
> +
> static ssize_t cros_ec_pdinfo_read(struct file *file,
> char __user *user_buf,
> size_t count,
> @@ -269,6 +313,13 @@ const struct file_operations cros_ec_pdinfo_fops = {
> .llseek = default_llseek,
> };
>
> +const struct file_operations cros_ec_uptime_fops = {
> + .owner = THIS_MODULE,
> + .open = simple_open,
> + .read = cros_ec_uptime_read,
> + .llseek = default_llseek,
> +};
> +
> static int ec_read_version_supported(struct cros_ec_dev *ec)
> {
> struct ec_params_get_cmd_versions_v1 *params;
> @@ -413,6 +464,29 @@ static int cros_ec_create_pdinfo(struct cros_ec_debugfs *debug_info)
> return 0;
> }
>
> +static int cros_ec_create_uptime(struct cros_ec_debugfs *debug_info)
> +{
> + struct cros_ec_debugfs *debug_info = file->private_data;
> + struct cros_ec_device *ec_dev = debug_info->ec->ec_dev;
> + u32 uptime;
> + int ret;
> +
> + /*
> + * If the EC does not support the uptime command, which is
> + * indicated by xfer_status() returning -EINVAL, then no
> + * debugfs entry will be created.
> + */
> + ret = cros_ec_get_uptime(ec_dev, &uptime);
> +
> + if (ret == -EINVAL)
> + return supported;
> +
> + debugfs_create_file("uptime", 0444, debug_info->dir, debug_info,
> + &cros_ec_uptime_fops);
> +
> + return 0;
> +}
> +
> static int cros_ec_debugfs_probe(struct platform_device *pd)
> {
> struct cros_ec_dev *ec = dev_get_drvdata(pd->dev.parent);
> @@ -442,6 +516,10 @@ static int cros_ec_debugfs_probe(struct platform_device *pd)
> if (ret)
> goto remove_log;
>
> + ret = cros_ec_create_uptime(debug_info);
> + if (ret)
> + goto remove_log;
> +
> ec->debug_info = debug_info;
>
> dev_set_drvdata(&pd->dev, ec);
> --
> 2.20.1
>

2019-06-06 12:19:17

by Enric Balletbo i Serra

[permalink] [raw]
Subject: Re: [PATCH v4] platform/chrome: mfd/cros_ec_debugfs: Add debugfs entry to retrieve EC uptime.

Hi Tim,

Sorry for making you wait so much, now that the patches about cros_ec_commands
include file has been accepted and the struct is up-to-date I can manage this.
I've some few comments though, one of them is a change I requested but that now
we should revert (sorry about that)

Please rebase the patch on top of chrome-platform-5.3, there are trivial conflicts.

On 2/5/19 23:16, Enric Balletbo Serra wrote:
> Hi Tim,
>
> Missatge de Tim Wawrzynczak <[email protected]> del dia dj., 2
> de maig 2019 a les 18:10:
>>
>> The new debugfs entry 'uptime' is being made available to userspace so that
>> a userspace daemon can synchronize EC logs with host time.
>>
>> Signed-off-by: Tim Wawrzynczak <[email protected]>
>> ---
>> Enric, is there something I can do to help speed this along? This patch
>> is useful for ChromeOS board bringup, and we would like to see it upstreamed
>> if at all possible.
>>
>
> The last version looks good to me. The patch is in my list but is too
> late for the next merge window. Will be one of the first patches I'll
> queue for 5.3
>
> Thanks,
> Enric
>
>> Also, AFAIK only the cros_ec supports the 'uptime' command for now.
>> And yes, the file does need to be seekable; the userspace daemon that
>> consumes the file keeps the file open and seeks back to the beginning
>> to get the latest uptime value.
>> Based on your second response to v3, I kept the separate 'create_uptime'
>> function b/c of the logic for checking support for the uptime command.
>> Let me know if you'd like me to move all of that logic into _probe.
>>
>> Changelist from v3:
>> 1) Don't check return values of debugfs_* functions.
>> 2) Only expose 'uptime' file if EC supports it.
>> ---
>> Documentation/ABI/testing/debugfs-cros-ec | 10 +++
>> drivers/platform/chrome/cros_ec_debugfs.c | 78 +++++++++++++++++++++++
>> 2 files changed, 88 insertions(+)
>> create mode 100644 Documentation/ABI/testing/debugfs-cros-ec
>>
>> diff --git a/Documentation/ABI/testing/debugfs-cros-ec b/Documentation/ABI/testing/debugfs-cros-ec
>> new file mode 100644
>> index 000000000000..24b781c67a4c
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/debugfs-cros-ec
>> @@ -0,0 +1,10 @@
>> +What: /sys/kernel/debug/cros_ec/uptime

Although for now only cros_ec supports it, let's use <cros-ec-device> for
possible future uses.

/sys/kernel/debug/<cros-ec-device>/uptime

>> +Date: March 2019
>> +KernelVersion: 5.1

Will be 5.3

>> +Description:
>> + Read-only.
>> + Reads the EC's current uptime information
>> + (using EC_CMD_GET_UPTIME_INFO) and prints
>> + time_since_ec_boot_ms into the file.
>> + This is used for synchronizing AP host time
>> + with the cros_ec log.
>> diff --git a/drivers/platform/chrome/cros_ec_debugfs.c b/drivers/platform/chrome/cros_ec_debugfs.c
>> index 71308766e891..226545a2150b 100644
>> --- a/drivers/platform/chrome/cros_ec_debugfs.c
>> +++ b/drivers/platform/chrome/cros_ec_debugfs.c
>> @@ -201,6 +201,50 @@ static int cros_ec_console_log_release(struct inode *inode, struct file *file)
>> return 0;
>> }
>>
>> +static int cros_ec_get_uptime(struct cros_ec_device *ec_dev, u32 *uptime)
>> +{
>> + struct {
>> + struct cros_ec_command msg;
>> + struct ec_response_uptime_info resp;
>> + } __packed ec_buf;
>> + struct ec_response_uptime_info *resp;
>> + struct cros_ec_command *msg;
>> +
>> + msg = &ec_buf.msg;
>> + resp = (struct ec_response_uptime_info *)msg->data;
>> +
>> + msg->command = EC_CMD_GET_UPTIME_INFO;
>> + msg->version = 0;
>> + msg->insize = sizeof(*resp);
>> + msg->outsize = 0;
>> +
>> + ret = cros_ec_cmd_xfer_status(ec_dev, msg);
>> + if (ret < 0)
>> + return ret;
>> +
>> + *uptime = resp->time_since_ec_boot_ms;
>> + return 0;
>> +}
>> +
>> +static ssize_t cros_ec_uptime_read(struct file *file,
>> + char __user *user_buf,
>> + size_t count,
>> + loff_t *ppos)
>> +{
>> + struct cros_ec_debugfs *debug_info = file->private_data;
>> + struct cros_ec_device *ec_dev = debug_info->ec->ec_dev;
>> + char read_buf[32];
>> + int ret;
>> + u32 uptime;
>> +
>> + ret = cros_ec_get_uptime(ec_dev, &uptime);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = scnprintf(read_buf, sizeof(read_buf), "%u\n", uptime);
>> + return simple_read_from_buffer(user_buf, count, ppos, read_buf, ret);
>> +}
>> +
>> static ssize_t cros_ec_pdinfo_read(struct file *file,
>> char __user *user_buf,
>> size_t count,
>> @@ -269,6 +313,13 @@ const struct file_operations cros_ec_pdinfo_fops = {
>> .llseek = default_llseek,
>> };
>>
>> +const struct file_operations cros_ec_uptime_fops = {
>> + .owner = THIS_MODULE,
>> + .open = simple_open,
>> + .read = cros_ec_uptime_read,
>> + .llseek = default_llseek,
>> +};
>> +
>> static int ec_read_version_supported(struct cros_ec_dev *ec)
>> {
>> struct ec_params_get_cmd_versions_v1 *params;
>> @@ -413,6 +464,29 @@ static int cros_ec_create_pdinfo(struct cros_ec_debugfs *debug_info)
>> return 0;
>> }
>>
>> +static int cros_ec_create_uptime(struct cros_ec_debugfs *debug_info)
>> +{
>> + struct cros_ec_debugfs *debug_info = file->private_data;
>> + struct cros_ec_device *ec_dev = debug_info->ec->ec_dev;
>> + u32 uptime;
>> + int ret;
>> +
>> + /*
>> + * If the EC does not support the uptime command, which is
>> + * indicated by xfer_status() returning -EINVAL, then no
>> + * debugfs entry will be created.
>> + */
>> + ret = cros_ec_get_uptime(ec_dev, &uptime);
>> +
>> + if (ret == -EINVAL)
>> + return supported;

That doesn't apply anymore, xfer_status will not return -EINVAL (sorry to make
you change this before)

>> +
>> + debugfs_create_file("uptime", 0444, debug_info->dir, debug_info,
>> + &cros_ec_uptime_fops);
>> +
>> + return 0;
>> +}
>> +


Let's remove this function and just do

>> static int cros_ec_debugfs_probe(struct platform_device *pd)
>> {
>> struct cros_ec_dev *ec = dev_get_drvdata(pd->dev.parent);
>> @@ -442,6 +516,10 @@ static int cros_ec_debugfs_probe(struct platform_device *pd)
>> if (ret)
>> goto remove_log;
>>


If the command fails just don't create the file silently. No need to remove the
other files just because this fails.

if (cros_ec_get_uptime() == 0)
debugfs_create_file("uptime", 0444, debug_info->dir, debug_info,
&cros_ec_uptime_fops);

Thanks,
Enric

>> + ret = cros_ec_create_uptime(debug_info);
>> + if (ret)
>> + goto remove_log;
>> +
>> ec->debug_info = debug_info;
>>
>> dev_set_drvdata(&pd->dev, ec);
>> --
>> 2.20.1
>>