* [PATCH v2] soc: qcom: Introduce subsystem sleep stats driver
@ 2019-09-05 9:17 Maulik Shah
2019-09-05 18:37 ` Stephen Boyd
0 siblings, 1 reply; 4+ messages in thread
From: Maulik Shah @ 2019-09-05 9:17 UTC (permalink / raw)
To: swboyd, agross, david.brown, linux-arm-msm
Cc: linux-kernel, linux-pm, bjorn.andersson, evgreen, dianders,
rnayak, ilina, lsrao, Maulik Shah
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 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 | 146 +++++++++++++++++++++++
4 files changed, 166 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 18b7dc929234..1f8bb201246a 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 880cf0290962..da53a96c6cce 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -163,6 +163,15 @@ config QCOM_SMSM
Say yes here to support the Qualcomm Shared Memory State Machine.
The state machine is represented by bits in shared memory.
+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 ffe519b0cb66..f00f21b87a22 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_QCOM_SMEM) += smem.o
obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
obj-$(CONFIG_QCOM_SMSM) += smsm.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-slice.o
diff --git a/drivers/soc/qcom/subsystem_sleep_stats.c b/drivers/soc/qcom/subsystem_sleep_stats.c
new file mode 100644
index 000000000000..5379714b6ba4
--- /dev/null
+++ b/drivers/soc/qcom/subsystem_sleep_stats.c
@@ -0,0 +1,146 @@
+// 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_OR_NULL(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 = kmalloc(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;
+ prvdata->ka.store = NULL;
+
+ ret = sysfs_create_file(prvdata->kobj, &prvdata->ka.attr);
+ if (ret) {
+ pr_err("sysfs_create_file failed\n");
+ kobject_put(prvdata->kobj);
+ kfree(prvdata);
+ return ret;
+ }
+
+ 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 the Code Aurora Forum, hosted by The Linux Foundation.
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] soc: qcom: Introduce subsystem sleep stats driver
2019-09-05 9:17 [PATCH v2] soc: qcom: Introduce subsystem sleep stats driver Maulik Shah
@ 2019-09-05 18:37 ` Stephen Boyd
2019-11-06 9:22 ` Maulik Shah
0 siblings, 1 reply; 4+ messages in thread
From: Stephen Boyd @ 2019-09-05 18:37 UTC (permalink / raw)
To: Maulik Shah, agross, david.brown, linux-arm-msm
Cc: linux-kernel, linux-pm, bjorn.andersson, evgreen, dianders,
rnayak, ilina, lsrao, Maulik Shah
Quoting Maulik Shah (2019-09-05 02:17:07)
> 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 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 | 146 +++++++++++++++++++++++
> 4 files changed, 166 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 18b7dc929234..1f8bb201246a 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
It isn't 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.
> +
This directory doesn't make any sense to me. It's in the top-level power
directory and it is specific to qcom. Is this debugging information? Why
does userspace care about understanding the sleep stats information?
Please Cc Rafael on anything touching /sys/power/
> What: /sys/power/resume_offset
> Date: April 2018
> Contact: Mario Limonciello <mario.limonciello@dell.com>
> diff --git a/drivers/soc/qcom/subsystem_sleep_stats.c b/drivers/soc/qcom/subsystem_sleep_stats.c
> new file mode 100644
> index 000000000000..5379714b6ba4
> --- /dev/null
> +++ b/drivers/soc/qcom/subsystem_sleep_stats.c
> @@ -0,0 +1,146 @@
> +// 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},
Please put spaces around braces.
> +};
> +
> +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);
Information in sysfs is supposed to be one value per file. This is a
bunch of different values and it includes a version field. Looks almost
like something we would put into /proc, but of course that doesn't make
any sense to put in /proc either.
Please rethink the whole approach here. Can this be placed under the
remoteproc nodes for each remote processor that's in the system? That
would make it more discoverable by userspace looking at the remoteproc
devices. I suppose GPU and DISPLAY aren't "remoteproc"s though so maybe
this should be a new 'class' for devices that have an RPMh RSC? Maybe
make a qcom_rpmh_rsc class and then have these be stats in there.
> +}
> +
> +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_OR_NULL(record) && (PAGE_SIZE - length > 0))
It can return ERR pointer or NULL? Why?
> + 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 = kmalloc(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;
> + prvdata->ka.store = NULL;
Does it need to be set to NULL explicitly? Why not kzalloc() prvdata
above?
> +
> + ret = sysfs_create_file(prvdata->kobj, &prvdata->ka.attr);
> + if (ret) {
> + pr_err("sysfs_create_file failed\n");
Seems useless. Presumably sysfs_create_file() can complain itself.
> + kobject_put(prvdata->kobj);
> + kfree(prvdata);
> + return ret;
> + }
> +
> + 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);
So if this is compiled into an arm/arm64 image that doesn't include qcom
platform support it will create this directory? That's just nonsensical.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] soc: qcom: Introduce subsystem sleep stats driver
2019-09-05 18:37 ` Stephen Boyd
@ 2019-11-06 9:22 ` Maulik Shah
2019-11-06 19:08 ` Stephen Boyd
0 siblings, 1 reply; 4+ messages in thread
From: Maulik Shah @ 2019-11-06 9:22 UTC (permalink / raw)
To: Stephen Boyd, agross, david.brown, linux-arm-msm
Cc: linux-kernel, linux-pm, bjorn.andersson, evgreen, dianders,
rnayak, ilina, lsrao
On 9/6/2019 12:07 AM, Stephen Boyd wrote:
> Quoting Maulik Shah (2019-09-05 02:17:07)
>> 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 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 | 146 +++++++++++++++++++++++
>> 4 files changed, 166 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 18b7dc929234..1f8bb201246a 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
> It isn't December 2017.
Hi Stephen,
Keeping it 2017 since driver is present from 2017 (driver copyright is
also from 2017-2019)
>
>> +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.
>> +
> This directory doesn't make any sense to me. It's in the top-level power
> directory and it is specific to qcom. Is this debugging information? Why
> does userspace care about understanding the sleep stats information?
> Please Cc Rafael on anything touching /sys/power/
Stats can be used by userspace for the purpose of computing battery
utilization.
Sure i will include Rafael in next revision.
>
>> What: /sys/power/resume_offset
>> Date: April 2018
>> Contact: Mario Limonciello <mario.limonciello@dell.com>
>> diff --git a/drivers/soc/qcom/subsystem_sleep_stats.c b/drivers/soc/qcom/subsystem_sleep_stats.c
>> new file mode 100644
>> index 000000000000..5379714b6ba4
>> --- /dev/null
>> +++ b/drivers/soc/qcom/subsystem_sleep_stats.c
>> @@ -0,0 +1,146 @@
>> +// 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},
> Please put spaces around braces.
Sure.
>> +};
>> +
>> +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);
> Information in sysfs is supposed to be one value per file. This is a
> bunch of different values and it includes a version field. Looks almost
> like something we would put into /proc, but of course that doesn't make
> any sense to put in /proc either.
>
> Please rethink the whole approach here. Can this be placed under the
> remoteproc nodes for each remote processor that's in the system? That
> would make it more discoverable by userspace looking at the remoteproc
> devices. I suppose GPU and DISPLAY aren't "remoteproc"s though so maybe
> this should be a new 'class' for devices that have an RPMh RSC? Maybe
> make a qcom_rpmh_rsc class and then have these be stats in there.
since stats can be used by userspace for the purpose of computing
battery utilization /sys/power seems to be good place to keep it to me.
Adding it under class may require it to be device. we are using it only
as module.
>> +}
>> +
>> +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_OR_NULL(record) && (PAGE_SIZE - length > 0))
> It can return ERR pointer or NULL? Why?
Updated to check only IS_ERR.
Thanks for pointing.
>> + 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 = kmalloc(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;
>> + prvdata->ka.store = NULL;
> Does it need to be set to NULL explicitly? Why not kzalloc() prvdata
> above?
Fixed.
>> +
>> + ret = sysfs_create_file(prvdata->kobj, &prvdata->ka.attr);
>> + if (ret) {
>> + pr_err("sysfs_create_file failed\n");
> Seems useless. Presumably sysfs_create_file() can complain itself.
Fixed.
>> + kobject_put(prvdata->kobj);
>> + kfree(prvdata);
>> + return ret;
>> + }
>> +
>> + 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);
> So if this is compiled into an arm/arm64 image that doesn't include qcom
> platform support it will create this directory? That's just nonsensical.
Kconfig depends on QCOM_SMEM which inturn depends on ARCH_QCOM to get
compiled into.
It won't get compiled for other than qcom platforms.
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] soc: qcom: Introduce subsystem sleep stats driver
2019-11-06 9:22 ` Maulik Shah
@ 2019-11-06 19:08 ` Stephen Boyd
0 siblings, 0 replies; 4+ messages in thread
From: Stephen Boyd @ 2019-11-06 19:08 UTC (permalink / raw)
To: Maulik Shah, agross, david.brown, linux-arm-msm
Cc: linux-kernel, linux-pm, bjorn.andersson, evgreen, dianders,
rnayak, ilina, lsrao
Quoting Maulik Shah (2019-11-06 01:22:14)
>
> On 9/6/2019 12:07 AM, Stephen Boyd wrote:
> > Quoting Maulik Shah (2019-09-05 02:17:07)
> >> +
> >> +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);
> > Information in sysfs is supposed to be one value per file. This is a
> > bunch of different values and it includes a version field. Looks almost
> > like something we would put into /proc, but of course that doesn't make
> > any sense to put in /proc either.
> >
> > Please rethink the whole approach here. Can this be placed under the
> > remoteproc nodes for each remote processor that's in the system? That
> > would make it more discoverable by userspace looking at the remoteproc
> > devices. I suppose GPU and DISPLAY aren't "remoteproc"s though so maybe
> > this should be a new 'class' for devices that have an RPMh RSC? Maybe
> > make a qcom_rpmh_rsc class and then have these be stats in there.
>
> since stats can be used by userspace for the purpose of computing
> battery utilization /sys/power seems to be good place to keep it to me.
>
> Adding it under class may require it to be device. we are using it only
> as module.
>
I believe /sys/power is for the power management subsystem, not
specifically battery utilization or remote processor power states.
Wouldn't battery be /sys/class/power_supply? Why not put this underneath
some /sys/class/remoteproc or so?
> >> + kobject_put(prvdata->kobj);
> >> + kfree(prvdata);
> >> + return ret;
> >> + }
> >> +
> >> + 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);
> > So if this is compiled into an arm/arm64 image that doesn't include qcom
> > platform support it will create this directory? That's just nonsensical.
>
> Kconfig depends on QCOM_SMEM which inturn depends on ARCH_QCOM to get
> compiled into.
>
> It won't get compiled for other than qcom platforms.
Sure it won't get compiled for anything that doesn't have ARCH_QCOM
enabled, but it can run on a board or SoC that isn't qcom. That's the
concern.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-11-06 19:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-05 9:17 [PATCH v2] soc: qcom: Introduce subsystem sleep stats driver Maulik Shah
2019-09-05 18:37 ` Stephen Boyd
2019-11-06 9:22 ` Maulik Shah
2019-11-06 19:08 ` Stephen Boyd
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).