linux-kernel - Re: [PATCH v2 1/3] soc: qcom: qcom_stats: Add support to read DDR statistic

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3684b6c5-91c1-44b9-aab5-b768db9fd08a@oss.qualcomm.com>
Date: Thu, 22 May 2025 16:24:24 +0200
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: "Maulik Shah (mkshah)" <maulik.shah@....qualcomm.com>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>
Cc: linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        Doug Anderson <dianders@...omium.org>
Subject: Re: [PATCH v2 1/3] soc: qcom: qcom_stats: Add support to read DDR
 statistic

On 5/22/25 7:48 AM, Maulik Shah (mkshah) wrote:
> 
> 
> On 5/21/2025 10:31 PM, Konrad Dybcio wrote:
>> On 5/21/25 10:32 AM, Maulik Shah wrote:
>>> DDR statistic provide different DDR LPM and DDR frequency statistic.
>>> Add support to read from MSGRAM and display via debugfs.
>>>
>>> Signed-off-by: Maulik Shah <maulik.shah@....qualcomm.com>
>>> ---
>>
>> [...]
>>
>>> +	case 0:
>>> +		seq_printf(s, "DDR LPM Stat Name:0x%x\tcount:%u\tDuration (ticks):%llu\n",
>>> +			   DDR_STATS_LPM_NAME(data->name), data->count, data->duration);
>>> +		break;
>>> +	case 1:
>>> +		if (!data->count || !DDR_STATS_FREQ(data->name))
>>> +			return;
>>> +
>>> +		cp_idx = DDR_STATS_CP_IDX(data->name);
>>> +		seq_printf(s, "DDR Freq %uMhz:\tCP IDX:%u\tcount:%u\tDuration (ticks):%llu\n",
>>> +			   DDR_STATS_FREQ(data->name), cp_idx, data->count, data->duration);
>>
>> clang complains about both prints:
>>
>> drivers/soc/qcom/qcom_stats.c:173:7: warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat]
>>   172 |                 seq_printf(s, "DDR LPM Stat Name:0x%x\tcount:%u\tDuration (ticks):%llu\n",
>>       |                                                    ~~
>>       |                                                    %lx
>>   173 |                            DDR_STATS_LPM_NAME(data->name), data->count, data->duration);
>>       |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> drivers/soc/qcom/qcom_stats.c:181:7: warning: format specifies type 'unsigned int' but the argument has type 'unsigned long' [-Wformat]
>>   180 |                 seq_printf(s, "DDR Freq %uMhz:\tCP IDX:%u\tcount:%u\tDuration (ticks):%llu\n",
>>       |                                         ~~
>>       |                                         %lu
>>   181 |                            DDR_STATS_FREQ(data->name), cp_idx, data->count, data->duration);
>>       |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>
>>
> 
> I will update correct format specifiers in v3.
> 
>>
>>> +
>>> +	key = readl_relaxed(reg + config->ddr_stats_offset + DDR_STATS_MAGIC_KEY_ADDR);
>>> +	if (key == DDR_STATS_MAGIC_KEY)
>>> +		debugfs_create_file("ddr_stats", 0400, root,
>>> +				    (__force void *)reg + config->ddr_stats_offset,
>>> +				    &qcom_ddr_stats_fops);
>>
>> else
>> 	pr_err("Found invalid DDR stats magic\n");
>>
>> (because through the compatible, we much expect it to be present)
> 
> The qcom,rpmh-stats compatible does not guarantee the DDR stats presence. DDR stats is only present if
> magic value matches. The ddr stats was incrementally added over time so older SoCs like SM8150, QCS615
> will not have the ddr stats and would end up printing this error during boot up but yes all almost all
> rpmh targets do have the DDR stats present. If we are ok to print this error for older SoCs i can add it
> or how about using pr_warn instead of pr_err?

OK, Let's not print anything then

Konrad