linux-kernel - Re: [PATCH 1/2] soc: qcom: rpmh: Add support to read back resource settings

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] [day] [month] [year] [list]

Message-ID: <10b11ae8-b375-40a6-88d5-3796902299e3@oss.qualcomm.com>
Date: Tue, 8 Jul 2025 10:37:25 +0530
From: "Maulik Shah (mkshah)" <maulik.shah@....qualcomm.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
        Kamal Wadhwa <kamal.wadhwa@....qualcomm.com>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>
Cc: linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] soc: qcom: rpmh: Add support to read back resource
 settings



On 6/24/2025 8:14 PM, Konrad Dybcio wrote:
> On 6/23/25 6:43 PM, Kamal Wadhwa wrote:
>> From: Maulik Shah <maulik.shah@....qualcomm.com>
>>
>> All rpmh_*() APIs so far have supported placing votes for various
>> resource settings but the H/W also have option to read resource
>> settings.
>>
>> This change adds a new rpmh_read() API to allow clients
>> to read back resource setting from H/W. This will be useful for
>> clients like regulators, which currently don't have a way to know
>> the settings applied during bootloader stage.
>>
>> Signed-off-by: Maulik Shah <maulik.shah@....qualcomm.com>
>> ---
> 
> [...]
> 
>>  	u32 msgid;
>> -	u32 cmd_msgid = CMD_MSGID_LEN | CMD_MSGID_WRITE;
>> +	u32 cmd_msgid = CMD_MSGID_LEN;
>>  	u32 cmd_enable = 0;
>>  	struct tcs_cmd *cmd;
>>  	int i, j;
>>  
>>  	/* Convert all commands to RR when the request has wait_for_compl set */
>>  	cmd_msgid |= msg->wait_for_compl ? CMD_MSGID_RESP_REQ : 0;
>> +	cmd_msgid |= (!msg->is_read) ? CMD_MSGID_WRITE : 0;
> 
> if (!msg->is_read)
> 	cmd_msgid |= CMD_MSGID_WRITE
> 
> looks more human-readable

I will update in v2.

> 
> [...]
> 
>> +/**
>> + * rpmh_read: Read a resource value
>> + *
>> + * @dev: The device making the request
>> + * @state: Must be Active state
>> + * @cmd: The payload having address of resource to read
>> + * @n: The number of elements in @cmd, must be single command
>> + *
>> + * Reads the value for the resource address given in tcs_cmd->addr
>> + * and returns the tcs_cmd->data filled with same.
>> + *
>> + * May sleep. Do not call from atomic contexts.
>> + *
>> + * Return:
>> + * * 0			- Success
>> + * * -Error             - Error code
> 
> This isn't valid kerneldoc

I will update in v2.

> 
>> + */
>> +int rpmh_read(const struct device *dev, enum rpmh_state state, struct tcs_cmd *cmd, u32 n)
>> +{
>> +	int ret;
>> +	DECLARE_COMPLETION_ONSTACK(compl);
>> +	DEFINE_RPMH_MSG_ONSTACK(dev, state, &compl, rpm_msg);
> 
> A reverse-Christmas-tree sorting would be nice here

I will update in v2 per reverse-Christmas-tree sorting.

> 
>> +
>> +	if (n != 1 || state != RPMH_ACTIVE_ONLY_STATE)
> 
> if n must be one, why is it a parameter?

I will Remove n (which is always 1) and also the state (which is also always RPMH_ACTIVE_ONLY_STATE) from the parameters.

Thanks,
Maulik

> 
> Konrad