Message-ID: <5dcf0e86.1c69fb81.a126f.5beb@mx.google.com>
Date:   Fri, 15 Nov 2019 12:45:56 -0800
From:   Stephen Boyd <swboyd@...omium.org>
To:     Maulik Shah <mkshah@...eaurora.org>, agross@...nel.org,
        david.brown@...aro.org, rafael@...nel.org
Cc:     linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, bjorn.andersson@...aro.org,
        evgreen@...omium.org, dianders@...omium.org, rnayak@...eaurora.org,
        ilina@...eaurora.org, lsrao@...eaurora.org,
        Maulik Shah <mkshah@...eaurora.org>
Subject: Re: [v3] soc: qcom: Introduce subsystem sleep stats driver

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

Powered by blists - more mailing lists