Re: [v3] soc: qcom: Introduce subsystem sleep stats driver

From: Maulik Shah
Date: Fri Feb 21 2020 - 04:47:40 EST



On 11/16/2019 2:15 AM, Stephen Boyd wrote:
Quoting Maulik Shah (2019-11-06 03:19:25)
diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
index 6f87b9d..e095eae 100644
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -288,6 +288,16 @@ Description:
writing a "0" (default) to it disables them. Reads from
this file return the current value.
+What: /sys/power/subsystem_sleep/stats
+Date: December 2017
+Contact: Maulik Shah <mkshah@xxxxxxxxxxxxxx>
+Description:
+ The /sys/power/subsystem_sleep/stats file prints the subsystem
+ sleep information on Qualcomm Technologies, Inc. (QTI) SoCs.
+
+ Reading from this file will display subsystem level low power
+ mode statistics.
I still don't understand what this has to do with the kernel's power
management support.

Using debugfs in v2 in single stats driver.

https://lore.kernel.org/patchwork/project/lkml/list/?series=430622


+
What: /sys/power/resume_offset
Date: April 2018
Contact: Mario Limonciello <mario.limonciello@xxxxxxxx>
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 79d8265..bed0704 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -185,6 +185,15 @@ config QCOM_SOCINFO
Say yes here to support the Qualcomm socinfo driver, providing
information about the SoC to user space.
+config QCOM_SS_SLEEP_STATS
+ tristate "Qualcomm Technologies Inc. Subsystem Sleep Stats driver"
+ depends on QCOM_SMEM
+ help
+ Say y here to enable support for the Qualcomm Technologies Inc (QTI)
This 'Inc' is missing the full stop like in the summary above. Please be
consistent.
corrected.

+ SS sleep stats driver to read the sleep stats of various subsystems
what is 'SS'?
corrected.

+ from SMEM. The stats are exported to sysfs. The driver also maintains
+ application processor sleep stats.
+
config QCOM_WCNSS_CTRL
tristate "Qualcomm WCNSS control driver"
depends on ARCH_QCOM || COMPILE_TEST
diff --git a/drivers/soc/qcom/subsystem_sleep_stats.c b/drivers/soc/qcom/subsystem_sleep_stats.c
new file mode 100644
index 00000000..724b213
--- /dev/null
+++ b/drivers/soc/qcom/subsystem_sleep_stats.c
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * Copyright (c) 2017-2019, The Linux Foundation. All rights reserved.
+ */
+
+#define pr_fmt(fmt) "%s: " fmt, KBUILD_MODNAME
+
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/types.h>
I think we need to include linux/kernel.h for scnprintf() too.
done.
+
+#include <linux/soc/qcom/smem.h>
+
+enum subsystem_item_id {
+ MODEM = 605,
+ ADSP,
+ CDSP,
+ SLPI,
+ GPU,
+ DISPLAY,
+};
+
+enum subsystem_pid {
+ PID_APSS = 0,
+ PID_MODEM = 1,
+ PID_ADSP = 2,
+ PID_SLPI = 3,
+ PID_CDSP = 5,
+ PID_GPU = PID_APSS,
+ PID_DISPLAY = PID_APSS,
+};
Can these just be defines? There seems to be no value in enum because
we're not testing these in switch statements and they're randomly
assigned values.

this is inline with subsystem_item_id enum which uses 605 to 610 item ids in enum.

keeping it same for better readbility.


+
+struct subsystem_data {
+ char *name;
+ enum subsystem_item_id item_id;
+ enum subsystem_pid pid;
+};
+
+static const struct subsystem_data subsystems[] = {
+ { "MODEM", MODEM, PID_MODEM },
+ { "ADSP", ADSP, PID_ADSP },
+ { "CDSP", CDSP, PID_CDSP },
+ { "SLPI", SLPI, PID_SLPI },
+ { "GPU", GPU, PID_GPU },
+ { "DISPLAY", DISPLAY, PID_DISPLAY },
+};
+
+struct subsystem_stats {
+ uint32_t version_id;
+ uint32_t count;
+ uint64_t last_entered;
+ uint64_t last_exited;
+ uint64_t accumulated_duration;
We use u32 and u64 in kernel code. Also, is this the value in shared
memory? Probably it's little endian so needs to be __le32 an __le64.
corrected.
+};
+
+struct subsystem_stats_prv_data {
+ struct kobj_attribute ka;
+ struct kobject *kobj;
+};
+
+static struct subsystem_stats_prv_data *prvdata;
+
+static inline ssize_t subsystem_stats_print(char *prvbuf, ssize_t length,
+ struct subsystem_stats *record,
+ const char *name)
+{
+ return scnprintf(prvbuf, length, "%s\n\tVersion:0x%x\n"
+ "\tSleep Count:0x%x\n"
+ "\tSleep Last Entered At:0x%llx\n"
+ "\tSleep Last Exited At:0x%llx\n"
+ "\tSleep Accumulated Duration:0x%llx\n\n",
+ name, record->version_id, record->count,
+ record->last_entered, record->last_exited,
+ record->accumulated_duration);
This isn't one value per file as per sysfs rules. Why can't this go to
debugfs? Otherwise, it would be better to split it up into multiple
files.

And it still looks like something that should be plumbed into the remote
proc subsystem so we can see from userspace what remote processors are
1) present in the system and 2) how long they've been in a sleep state.
Using debugfs.

+}
+
+static ssize_t subsystem_stats_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ ssize_t length = 0;
+ int i = 0;
Drop assignment to i here.
done.

+ size_t size = 0;
Why assign size to 0? It looks unused in this function besides to store
the size in qcom_smem_get(). It looks like we can pass NULL for that
argument if we don't care to actually look at the size of what is
returned.
done.
+ struct subsystem_stats *record = NULL;
Please don't assign to NULL and then overwrite it without testing in
between.
done.

+
+ /* Read SMEM data written by other subsystems */
+ for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
+ record = (struct subsystem_stats *) qcom_smem_get(
The cast is unnecessary, it returns a void * already.
done.
+ subsystems[i].pid, subsystems[i].item_id, &size);
+
+ if (!IS_ERR(record) && (PAGE_SIZE - length > 0))
+ length += subsystem_stats_print(buf + length,
+ PAGE_SIZE - length,
+ record,
+ subsystems[i].name);
+ }
+
+ return length;
+}
+
+static int __init subsystem_sleep_stats_init(void)
+{
+ struct kobject *ss_stats_kobj;
+ int ret;
+
+ prvdata = kzalloc(sizeof(*prvdata), GFP_KERNEL);
+ if (!prvdata)
+ return -ENOMEM;
+
+ ss_stats_kobj = kobject_create_and_add("subsystem_sleep",
+ power_kobj);
If this module is loaded on non-qcom platforms we'll create
subsystem_sleep directory still. Please don't do that. If this was
connected to remote proc it would be easier to avoid this problem.
This is clubbed in single stats driver for both sleep stats in v2 link pasted above.

+ if (!ss_stats_kobj)
+ return -ENOMEM;
+
+ prvdata->kobj = ss_stats_kobj;
+
+ sysfs_attr_init(&prvdata->ka.attr);
+ prvdata->ka.attr.mode = 0444;
+ prvdata->ka.attr.name = "stats";
+ prvdata->ka.show = subsystem_stats_show;
+
+ ret = sysfs_create_file(prvdata->kobj, &prvdata->ka.attr);
+ if (ret) {
+ kobject_put(prvdata->kobj);
+ kfree(prvdata);
+ }
+
+ return ret;
+}
+

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation