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 <[email protected]>
---
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
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 <[email protected]>
+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 <[email protected]>
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");
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
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 <[email protected]>
> +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 <[email protected]>
> 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;
> +}
> +
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 <[email protected]>
> +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
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 <[email protected]>
>> +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 <[email protected]>
>> 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