[v3] soc: qcom: Introduce subsystem sleep stats driver
diff mbox series

Message ID 1573039165-30435-1-git-send-email-mkshah@codeaurora.org
State Changes Requested, archived
Headers show
Series
  • [v3] soc: qcom: Introduce subsystem sleep stats driver
Related show

Commit Message

Maulik Shah Nov. 6, 2019, 11:19 a.m. UTC
Multiple subsystems like modem, spss, adsp, cdsp present on
Qualcomm Technologies Inc's (QTI) SoCs maintains low power mode
statistics in shared memory (SMEM). Lets add a driver to read
and display this information using sysfs.

Signed-off-by: Maulik Shah <mkshah@codeaurora.org>
---
Changes in v3:
	- Addressed comments from v2.

Changes in v2:
	- Correct Makefile to use QCOM_SS_SLEEP_STATS config
---
 Documentation/ABI/testing/sysfs-power    |  10 +++
 drivers/soc/qcom/Kconfig                 |   9 ++
 drivers/soc/qcom/Makefile                |   1 +
 drivers/soc/qcom/subsystem_sleep_stats.c | 143 +++++++++++++++++++++++++++++++
 4 files changed, 163 insertions(+)
 create mode 100644 drivers/soc/qcom/subsystem_sleep_stats.c

Comments

Stephen Boyd Nov. 15, 2019, 8:45 p.m. UTC | #1
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@codeaurora.org>
> +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.

> +
>  What:          /sys/power/resume_offset
>  Date:          April 2018
>  Contact:       Mario Limonciello <mario.limonciello@dell.com>
> 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.

> +         SS sleep stats driver to read the sleep stats of various subsystems

what is 'SS'?

> +         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.

> +
> +#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.

> +
> +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.

> +};
> +
> +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.

> +}
> +
> +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.

> +       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.

> +       struct subsystem_stats *record = NULL;

Please don't assign to NULL and then overwrite it without testing in
between.

> +
> +       /* 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.

> +                         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.

> +       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;
> +}
> +
Bjorn Andersson Nov. 16, 2019, 1:45 a.m. UTC | #2
On Wed 06 Nov 03:19 PST 2019, Maulik Shah wrote:
> 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@codeaurora.org>
> +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.

sysfs files must follow the design of "one file, one value" and it must
be well defined. "sleep information" does not have a defined structure.

And as Stephen has pointed out several times, /sys/power/subsystem_sleep
is hardly the right place for a Qualcomm-specific entry.

[..]
> diff --git a/drivers/soc/qcom/subsystem_sleep_stats.c b/drivers/soc/qcom/subsystem_sleep_stats.c
[..]
> +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 (!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;
> +}
> +
> +static void __exit subsystem_sleep_stats_exit(void)
> +{
> +	sysfs_remove_file(prvdata->kobj, &prvdata->ka.attr);
> +	kobject_put(prvdata->kobj);
> +	kfree(prvdata);
> +}
> +
> +module_init(subsystem_sleep_stats_init);
> +module_exit(subsystem_sleep_stats_exit);

In the event that this is compiled as a kernel module, this driver won't
be automatically loaded - there's no references from other drivers nor
devicetree.

But equally big of an issue is that there's a single arm64 defconfig, so
this driver will be available on non-Qualcomm installations as well,
so you will be creating /sys/power/subsystem_sleep/stats regardless if
it ever will be possible to get any data out of this.

Regards,
Bjorn

Patch
diff mbox series

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@codeaurora.org>
+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.
+
 What:		/sys/power/resume_offset
 Date:		April 2018
 Contact:	Mario Limonciello <mario.limonciello@dell.com>
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)
+	  SS sleep stats driver to read the sleep stats of various subsystems
+	  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/Makefile b/drivers/soc/qcom/Makefile
index 9fb35c8..f3a2fb0 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -20,6 +20,7 @@  obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
 obj-$(CONFIG_QCOM_SMP2P)	+= smp2p.o
 obj-$(CONFIG_QCOM_SMSM)	+= smsm.o
 obj-$(CONFIG_QCOM_SOCINFO)	+= socinfo.o
+obj-$(CONFIG_QCOM_SS_SLEEP_STATS)	+= subsystem_sleep_stats.o
 obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
 obj-$(CONFIG_QCOM_APR) += apr.o
 obj-$(CONFIG_QCOM_LLCC) += llcc-qcom.o
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>
+
+#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,
+};
+
+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;
+};
+
+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);
+}
+
+static ssize_t subsystem_stats_show(struct kobject *kobj,
+				    struct kobj_attribute *attr, char *buf)
+{
+	ssize_t length = 0;
+	int i = 0;
+	size_t size = 0;
+	struct subsystem_stats *record = NULL;
+
+	/* Read SMEM data written by other subsystems */
+	for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
+		record = (struct subsystem_stats *) qcom_smem_get(
+			  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 (!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;
+}
+
+static void __exit subsystem_sleep_stats_exit(void)
+{
+	sysfs_remove_file(prvdata->kobj, &prvdata->ka.attr);
+	kobject_put(prvdata->kobj);
+	kfree(prvdata);
+}
+
+module_init(subsystem_sleep_stats_init);
+module_exit(subsystem_sleep_stats_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc subsystem sleep stats driver");