lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 8 Apr 2019 15:01:07 +0200 From: Enric Balletbo i Serra <enric.balletbo@...labora.com> To: Tim Wawrzynczak <twawrzynczak@...omium.org> Cc: Benson Leung <bleung@...omium.org>, linux-kernel@...r.kernel.org, Guenter Roeck <groeck@...gle.com> Subject: Re: [PATCH v3] platform/chrome: mfd/cros_ec_debugfs: Add debugfs entry to retrieve EC uptime. On 8/4/19 13:29, Enric Balletbo i Serra wrote: > Hi Tim, > > Many thanks for sending this patch upstream, some comments below > > On 27/3/19 19:20, Tim Wawrzynczak wrote: >> 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@...omium.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 > > Thanks to introduce the documentation! > >> @@ -0,0 +1,10 @@ >> +What: /sys/kernel/debug/cros_ec/uptime > > Is that propriety only supported for the standard cros-ec? > > If the answer is yes, is fine, but if this could be supported for other variants > like cros_pd, cros_tp, cros_fp, cros_ish, etc. then I'd name it > > /sys/kernel/debug/<ec-device-name>/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, > > Should this file be seekable? > >> +}; >> + >> 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; > > When calling debugfs functions, there is no need to ever check the > return value. The function can work or not, but the code logic should > never do something different based on this. This has been discussed recently on > the LKML. > > I know that this is currently wrong for the other entries in this file but let's > try to do well and remove this function and just call debugfs_create_file in probe. > Sorry, actually I changed my opinion on this. So you should check if the command is supported and only expose that file if that is the case here. See this commit. https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git/commit/?h=for-next&id=ae8378b081fa970a65001de909d8d6d8deea79b7 > I plan to send patches to fix current status. > >> + >> + 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; >> + > > debugfs_create_file("uptime", 0444, debug_info->dir, debug_info, > &cros_ec_uptime_fops); > >> ec->debug_info = debug_info; >> >> dev_set_drvdata(&pd->dev, ec); >> > > Thanks, > Enric >
Powered by blists - more mailing lists