From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
To: Tim Wawrzynczak <twawrzynczak@chromium.org>
Cc: Benson Leung <bleung@chromium.org>,
linux-kernel@vger.kernel.org, Guenter Roeck <groeck@google.com>
Subject: Re: [PATCH v3] platform/chrome: mfd/cros_ec_debugfs: Add debugfs entry to retrieve EC uptime.
Date: Mon, 8 Apr 2019 15:01:07 +0200 [thread overview]
Message-ID: <28d1d39e-ef60-6c5d-675a-7c86f2ec874d@collabora.com> (raw)
In-Reply-To: <147049bd-d661-bb6d-9631-fc15c5b4c9bb@collabora.com>
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@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
>
> 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
>
next prev parent reply other threads:[~2019-04-08 13:01 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20190327182040.112651-1-twawrzynczak@chromium.org>
2019-04-08 11:29 ` [PATCH v3] platform/chrome: mfd/cros_ec_debugfs: Add debugfs entry to retrieve EC uptime Enric Balletbo i Serra
2019-04-08 13:01 ` Enric Balletbo i Serra [this message]
2019-04-08 21:09 ` [PATCH v4] " Tim Wawrzynczak
2019-05-02 16:09 ` Tim Wawrzynczak
2019-05-02 21:16 ` Enric Balletbo Serra
2019-06-06 11:15 ` Enric Balletbo i Serra
2019-03-25 17:02 [PATCH v2] platform/chrome: mfd/cros_ec_sysfs: Add sysfs " twawrzynczak
2019-04-05 15:28 ` [PATCH v3] platform/chrome: mfd/cros_ec_debugfs: Add debugfs " twawrzynczak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=28d1d39e-ef60-6c5d-675a-7c86f2ec874d@collabora.com \
--to=enric.balletbo@collabora.com \
--cc=bleung@chromium.org \
--cc=groeck@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=twawrzynczak@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).