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: <5dc31a33.1c69fb81.c563.2043@mx.google.com>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <507d1769-41ba-749a-cafa-d178128bbb8b@codeaurora.org>
References: <20190905091707.14420-1-mkshah@codeaurora.org> <5d7155f2.1c69fb81.61bf.f862@mx.google.com> <507d1769-41ba-749a-cafa-d178128bbb8b@codeaurora.org>
Subject: Re: [PATCH v2] soc: qcom: Introduce subsystem sleep stats driver
From:   Stephen Boyd <swboyd@chromium.org>
To:     Maulik Shah <mkshah@codeaurora.org>, agross@kernel.org,
        david.brown@linaro.org, linux-arm-msm@vger.kernel.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
User-Agent: alot/0.8.1
Date:   Wed, 06 Nov 2019 11:08:34 -0800
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Maulik Shah (2019-11-06 01:22:14)
>=20
> 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 len=
gth,
> >> +                                           struct subsystem_stats *re=
cord,
> >> +                                           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.
>=20
> since stats can be used by userspace for the purpose of computing=20
> battery utilization /sys/power seems to be good place to keep it to me.
>=20
> Adding it under class may require it=C2=A0 to be device. we are using it =
only=20
> as module.
>=20

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.
>=20
> Kconfig depends on QCOM_SMEM which inturn depends on ARCH_QCOM to get=20
> compiled into.
>=20
> 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.