Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <5d7155f2.1c69fb81.61bf.f862@mx.google.com>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20190905091707.14420-1-mkshah@codeaurora.org>
References: <20190905091707.14420-1-mkshah@codeaurora.org>
Cc:     linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
        bjorn.andersson@linaro.org, evgreen@chromium.org,
        dianders@chromium.org, rnayak@codeaurora.org, ilina@codeaurora.org,
        lsrao@codeaurora.org, Maulik Shah <mkshah@codeaurora.org>
Subject: Re: [PATCH v2] soc: qcom: Introduce subsystem sleep stats driver
To:     Maulik Shah <mkshah@codeaurora.org>, agross@kernel.org,
        david.brown@linaro.org, linux-arm-msm@vger.kernel.org
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.8.1
Date:   Thu, 05 Sep 2019 11:37:37 -0700
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.
>=20
> 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
>=20
> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/te=
sting/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.
> =20
> +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 subs=
ystem
> +               sleep information on Qualcomm Technologies, Inc. (QTI) So=
Cs.
> +
> +               Reading from this file will display subsystem level low p=
ower
> +               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 =3D 605,
> +       ADSP,
> +       CDSP,
> +       SLPI,
> +       GPU,
> +       DISPLAY,
> +};
> +
> +enum subsystem_pid {
> +       PID_APSS =3D 0,
> +       PID_MODEM =3D 1,
> +       PID_ADSP =3D 2,
> +       PID_SLPI =3D 3,
> +       PID_CDSP =3D 5,
> +       PID_GPU =3D PID_APSS,
> +       PID_DISPLAY =3D PID_APSS,
> +};
> +
> +struct subsystem_data {
> +       char *name;
> +       enum subsystem_item_id item_id;
> +       enum subsystem_pid pid;
> +};
> +
> +static const struct subsystem_data subsystems[] =3D {
> +       {"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 *recor=
d,
> +                                           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 *bu=
f)
> +{
> +       ssize_t length =3D 0;
> +       int i =3D 0;
> +       size_t size =3D 0;
> +       struct subsystem_stats *record =3D NULL;
> +
> +       /* Read SMEM data written by other subsystems */
> +       for (i =3D 0; i < ARRAY_SIZE(subsystems); i++) {
> +               record =3D (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 +=3D subsystem_stats_print(buf + length,
> +                                                       PAGE_SIZE - lengt=
h,
> +                                                       record,
> +                                                       subsystems[i].nam=
e);
> +       }
> +
> +       return length;
> +}
> +
> +static int __init subsystem_sleep_stats_init(void)
> +{
> +       struct kobject *ss_stats_kobj;
> +       int ret;
> +
> +       prvdata =3D kmalloc(sizeof(*prvdata), GFP_KERNEL);
> +       if (!prvdata)
> +               return -ENOMEM;
> +
> +       ss_stats_kobj =3D kobject_create_and_add("subsystem_sleep",
> +                                              power_kobj);
> +       if (!ss_stats_kobj)
> +               return -ENOMEM;
> +
> +       prvdata->kobj =3D ss_stats_kobj;
> +
> +       sysfs_attr_init(&prvdata->ka.attr);
> +       prvdata->ka.attr.mode =3D 0444;
> +       prvdata->ka.attr.name =3D "stats";
> +       prvdata->ka.show =3D subsystem_stats_show;
> +       prvdata->ka.store =3D NULL;

Does it need to be set to NULL explicitly? Why not kzalloc() prvdata
above?

> +
> +       ret =3D 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.