linux-kernel - Re: [PATCH v11 2/5] soc: qcom: Add Sleep stats driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 8 Oct 2021 14:45:20 +0530
From:   Maulik Shah <mkshah@...eaurora.org>
To:     Stephan Gerhold <stephan@...hold.net>
Cc:     swboyd@...omium.org, mka@...omium.org, evgreen@...omium.org,
        bjorn.andersson@...aro.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, agross@...nel.org,
        dianders@...omium.org, linux@...ck-us.net, rnayak@...eaurora.org,
        lsrao@...eaurora.org,
        Mahesh Sivasubramanian <msivasub@...eaurora.org>,
        Lina Iyer <ilina@...eaurora.org>,
        Yassine Oudjana <y.oudjana@...tonmail.com>
Subject: Re: [PATCH v11 2/5] soc: qcom: Add Sleep stats driver

Hi,

On 10/7/2021 11:59 PM, Stephan Gerhold wrote:
> Hi Maulik,
> 
> On Thu, Oct 07, 2021 at 03:27:26PM +0530, Maulik Shah wrote:
>> From: Mahesh Sivasubramanian <msivasub@...eaurora.org>
>>
>> Let's add a driver to read the stats from remote processor and
>> export to debugfs.
>>
>> The driver creates "qcom_sleep_stats" directory in debugfs and
>> adds files for various low power mode available. Below is sample
>> output with command
>>
>> cat /sys/kernel/debug/qcom_sleep_stats/ddr
>> count = 0
>> Last Entered At = 0
>> Last Exited At = 0
>> Accumulated Duration = 0
>>
>> Signed-off-by: Mahesh Sivasubramanian <msivasub@...eaurora.org>
>> Signed-off-by: Lina Iyer <ilina@...eaurora.org>
>> [mkshah: add subsystem sleep stats, create one file for each stat]
>> Signed-off-by: Maulik Shah <mkshah@...eaurora.org>
>> ---
>>   drivers/soc/qcom/Kconfig            |  10 ++
>>   drivers/soc/qcom/Makefile           |   1 +
>>   drivers/soc/qcom/qcom_sleep_stats.c | 259 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 270 insertions(+)
>>   create mode 100644 drivers/soc/qcom/qcom_sleep_stats.c
>>
>> [...]
>> +
>> +static int qcom_subsystem_sleep_stats_show(struct seq_file *s, void *unused)
>> +{
>> +	struct subsystem_data *subsystem = s->private;
>> +	struct sleep_stats *stat;
>> +
>> +	/* Items are allocated lazily, so lookup pointer each time */
>> +	stat = qcom_smem_get(subsystem->pid, subsystem->smem_item, NULL);
>> +	if (IS_ERR(stat))
>> +		return -EIO;
>> +
>> [...]
>> +
>> +static void qcom_create_subsystem_stat_files(struct dentry *root)
>> +{
>> +	const struct sleep_stats *stat;
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(subsystems); i++) {
>> +		stat = qcom_smem_get(subsystems[i].pid, subsystems[i].smem_item, NULL);
>> +		if (IS_ERR(stat))
>> +			continue;
>> +
>> +		debugfs_create_file(subsystems[i].name, 0400, root, (void *)&subsystems[i],
>> +				    &qcom_subsystem_sleep_stats_fops);
> 
> This causes WARNINGs on MSM8996 and MSM8916:
> 
> [    0.503054] ------------[ cut here ]------------
> [    0.503100] WARNING: CPU: 1 PID: 1 at drivers/soc/qcom/smem.c:587 qcom_smem_get+0x184/0x1b0
> [    0.503184] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.15.0-rc4+ #378
> [    0.503218] Hardware name: Xiaomi Mi Note 2 (DT)
> [    0.503278] pc : qcom_smem_get+0x184/0x1b0
> [    0.503307] lr : qcom_sleep_stats_probe+0xfc/0x20
> [    0.503875] Call trace:
> [    0.503896]  qcom_smem_get+0x184/0x1b0
> [    0.503925]  qcom_sleep_stats_probe+0xfc/0x270
> 
> AFAICT from downstream the smem subsystem information is only read in
> the rpmh_master_stat.c driver, should this be specific to RPMh?

Thanks for checking this on MSM8996. Probably it doesnot have SMEM items 
allocated so causes WARNINGs.

Keeping subsystem stats info in SMEM is not limited to only RPMH 
targets. Downstream has some RPM targets which also uses SMEM to store
subsystem stats so the driver is kept generic.

> 
> There is a rpm_master_stat.c too but that looks quite different,
> so I guess the approach is different with RPM?

Right. on existing upstream RPM targets i can skip to create/get SMEM 
items since
they are not guranteed to be present and one should continue to use 
rpm_master_stats.c to get subsystem stats. (this uses entirely different 
data structure for sleep stats and are not part of RPM data ram/SMEM and 
are deprecated in downstream).

> 
> Two more (unrelated) issues here:
> 
>    1. This will silently not register anything if SMEM probes after the
>       qcom-sleep-stats driver (qcom_smem_get() will return -EPROBE_DEFER)
>       and you will just skip registering the debugfs files.

I think module loading internally takes care of this.
we're making a direct function call into the qcom_smem driver, so we
already have a hard dependency on qcom_smem.ko being loaded.

> 
>    2. In qcom_subsystem_sleep_stats_show() you say
>       /* Items are allocated lazily, so lookup pointer each time */
> 
>       But, if the lookup fails here you don't register the debugfs file
>       at all. Does this work if the subsystem is started after this driver?

Good catch. if the subsystem starts after this driver is loaded, the 
look up fails during probe and we don't create debugfs file for the 
subsystem.

one need to unload/load the driver again after sometime in bootup so by 
that time all the subsytems (modem, adsp, cdsp, etc) are up and we 
create debugfs file for them.

we have downstream fix for this to create the debugfs files irrespective 
of look up fails or not. i have plan to add it once the base driver gets 
merged.

Thanks,
Maulik

> 
> Thanks,
> Stephan
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of Code Aurora Forum, hosted by The Linux Foundation