Re: [PATCH v2] soc: qcom: Introduce subsystem sleep stats driver

[Stephen Boyd]

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.