[PATCH v2] platform/chrome: mfd/cros_ec_sysfs: Add sysfs entry to retrieve EC uptime.

[PATCH v2] platform/chrome: mfd/cros_ec_sysfs: Add sysfs entry to retrieve EC uptime.

[twawrzynczak]

From: Tim Wawrzynczak <twawrzynczak@chromium.org>

The new sysfs 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 <twawrzynczak@chromium.org>
---
Enric, the use case for this is ChromeOS's userspace daemon, timberslide,
which collects the EC logs for sending bug reports, etc.  This is part of
a series of patches which is prefixing host timestamps before each EC
log line.  The daemon matches up the EC log lines' seconds-since-boot
with the time gotten from the new 'uptime' node and calculates the
host time that the log line was printed at.
---
 drivers/platform/chrome/cros_ec_sysfs.c | 34 +++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index fe0b7614ae1b..b3915d3287c5 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -107,6 +107,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 = to_cros_ec_dev(dev);
+	u32 time_since_ec_boot_ms;
+
+	msg = kmalloc(sizeof(*msg) + sizeof(*resp), GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	/* Get EC uptime information */
+	msg->version = 0;
+	msg->command = EC_CMD_GET_UPTIME_INFO + ec->cmd_offset;
+	msg->insize = sizeof(*resp);
+	msg->outsize = 0;
+	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+	if (ret < 0) {
+		time_since_ec_boot_ms = 0;
+	} else {
+		resp = (struct ec_response_uptime_info *)msg->data;
+		time_since_ec_boot_ms = resp->time_since_ec_boot_ms;
+	}
+
+	ret = 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)
 {
@@ -314,12 +346,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);
 
 static struct attribute *__ec_attrs[] = {
 	&dev_attr_kb_wake_angle.attr,
 	&dev_attr_reboot.attr,
 	&dev_attr_version.attr,
 	&dev_attr_flashinfo.attr,
+	&dev_attr_uptime.attr,
 	NULL,
 };
 
-- 
2.20.1

Re: [PATCH v2] platform/chrome: mfd/cros_ec_sysfs: Add sysfs entry to retrieve EC uptime.

[Enric Balletbo i Serra]

Hi Tim,

On 25/3/19 18:02, twawrzynczak@chromium.org wrote:
> From: Tim Wawrzynczak <twawrzynczak@chromium.org>
> 
> The new sysfs 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 <twawrzynczak@chromium.org>
> ---
> Enric, the use case for this is ChromeOS's userspace daemon, timberslide,
> which collects the EC logs for sending bug reports, etc.  This is part of
> a series of patches which is prefixing host timestamps before each EC
> log line.  The daemon matches up the EC log lines' seconds-since-boot
> with the time gotten from the new 'uptime' node and calculates the
> host time that the log line was printed at.

The EC log is under debugfs (I assume that the daemon reads the log from there)
so I think that this new attribute should be in debugfs instead of sysfs.

Also would be good if you can document this interface, I know that current
debugfs interface for cros-ec is not documented, but if you can at least start
documenting the ABI I'd appreciate. (Documentation/ABI/testing/debugfs-cros-ec)

Please add a changelog so is more to track what changed from v1 to v2.

> ---
>  drivers/platform/chrome/cros_ec_sysfs.c | 34 +++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
> index fe0b7614ae1b..b3915d3287c5 100644
> --- a/drivers/platform/chrome/cros_ec_sysfs.c
> +++ b/drivers/platform/chrome/cros_ec_sysfs.c
> @@ -107,6 +107,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 = to_cros_ec_dev(dev);
> +	u32 time_since_ec_boot_ms;
> +

Please sort function local variables declaration in a reverse christmas
tree order:

        <type_a> longest_variable_name;
        <type_b> shorter_var_name;
        <type_c> even_shorter;
        <type_d> i;

> +	msg = kmalloc(sizeof(*msg) + sizeof(*resp), GFP_KERNEL);

kzmalloc ?

> +	if (!msg)
> +		return -ENOMEM;
> +
> +	/* Get EC uptime information */
> +	msg->version = 0;

Not needed if you use kzmalloc.

> +	msg->command = EC_CMD_GET_UPTIME_INFO + ec->cmd_offset;
> +	msg->insize = sizeof(*resp);
> +	msg->outsize = 0;

Not needed if you use kzmalloc.

> +	ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
> +	if (ret < 0) {
> +		time_since_ec_boot_ms = 0;

You didn't take in consideration my comments on v1 ...

That an error (because the command is not supported or other
reason). You're overlaping the error. Why not just goto an exit label, free the
memory and return the error.

Or there is a problem on returning an error?

> +	} else {

Then that else is not needed.

> +		resp = (struct ec_response_uptime_info *)msg->data;
> +		time_since_ec_boot_ms = resp->time_since_ec_boot_ms;

And you can remove time_since_ec_boot_ms
> +	}
> +
> +	ret = scnprintf(buf, PAGE_SIZE, "%u\n", time_since_ec_boot_ms);

And print directly resp->time_since_ec_boot_ms

> +
> +	kfree(msg);
> +	return ret;
> +}
> +
>  static ssize_t version_show(struct device *dev,
>  			    struct device_attribute *attr, char *buf)
>  {
> @@ -314,12 +346,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);
>  
>  static struct attribute *__ec_attrs[] = {
>  	&dev_attr_kb_wake_angle.attr,
>  	&dev_attr_reboot.attr,
>  	&dev_attr_version.attr,
>  	&dev_attr_flashinfo.attr,
> +	&dev_attr_uptime.attr,

Would be good if we can make this attribute only visible when is supported. I am
not sure if is possible though.

>  	NULL,
>  };
>  
> 

Thanks,
 Enric

[PATCH v3] platform/chrome: mfd/cros_ec_debugfs: Add debugfs entry to retrieve EC uptime.

[twawrzynczak]

From: Tim Wawrzynczak <twawrzynczak@chromium.org>

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 <twawrzynczak@chromium.org>
---
Changelist:
 - Moved uptime file from sysfs to debugfs (/sys/kernel/debug/cros_ec/uptime)
 - Fixed ordering of local variables in cros_ec_uptime_read.
Tested against ChromeOS kernel v4.14
---
 Documentation/ABI/testing/debugfs-cros-ec | 10 +++++
 drivers/platform/chrome/cros_ec_debugfs.c | 54 +++++++++++++++++++++++
 2 files changed, 64 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..3ea42008a59e 100644
--- a/drivers/platform/chrome/cros_ec_debugfs.c
+++ b/drivers/platform/chrome/cros_ec_debugfs.c
@@ -201,6 +201,40 @@ static int cros_ec_console_log_release(struct inode *inode, struct file *file)
 	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;
+	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;
+	char read_buf[32];
+	int ret;
+
+	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;
+
+	ret = scnprintf(read_buf, sizeof(read_buf), "%u\n",
+			resp->time_since_ec_boot_ms);
+
+	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 +303,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 +454,15 @@ 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)
+{
+	if (!debugfs_create_file("uptime", 0444, debug_info->dir, debug_info,
+				&cros_ec_uptime_fops))
+		return -ENOMEM;
+
+	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 +492,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

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

[Tim Wawrzynczak]

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 <twawrzynczak@chromium.org>
---
Enric, 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

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

[Tim Wawrzynczak]

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 <twawrzynczak@chromium.org>
---
Enric, 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