Re: [PATCH v7 2/9] soc: qcom: Add QMI TMD support for remote thermal mitigation

On 8/6/2026 3:10 PM, Dmitry Baryshkov wrote:
> On Fri, Jul 31, 2026 at 10:53:16AM +0000, [email protected] wrote:
>> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>> - [High] Returning success without saving the requested mitigation state when QMI is disconnected permanently loses thermal mitigation updates.
>> - [Medium] TOCTOU race condition when reading `tmd->cur_state` during service reconnection causes permanent thermal state desynchronization.
>> --
>>
>> --- Patch [2]: [PATCH v7 2/9] soc: qcom: Add QMI TMD support for remote thermal mitigation ---
>> commit 02c2244c0a2cebd6be4e1bf4e41d51362be19b5f
>> Author: Casey Connolly <[email protected]>
>>
>> soc: qcom: Add QMI TMD support for remote thermal mitigation
>>
>> This commit adds support for Qualcomm Messaging Interface (QMI) based
>> Thermal Mitigation Device (TMD) cooling devices provided by remote subsystems.
>> It allows client drivers to discover the service and forward cooling state
>> updates to remote processors.
>>
>> diff --git a/drivers/soc/qcom/qmi_tmd.c b/drivers/soc/qcom/qmi_tmd.c
>> new file mode 100644
>> index 0000000000000..37e6ef19a0b34
>> --- /dev/null
>> +++ b/drivers/soc/qcom/qmi_tmd.c
>> [ ... ]
>> @@ -236,25 +236,54 @@
>>> +/* Notify the remote subsystem of the requested cooling state */
>>> +static int qmi_tmd_send_state_request(struct qmi_tmd *tmd, int state)
>>> +{
>>> +	struct tmd_set_level_resp resp = { 0 };
>>> +	struct tmd_set_level_req req = { 0 };
>>> +	struct qmi_tmd_client *qmi_tmd_cli = tmd->qmi_tmd_cli;
>>> +	struct qmi_txn txn;
>>> +	int ret = 0;
>>> +
>>> +	guard(mutex)(&qmi_tmd_cli->mutex);
>>> +
>>> +	if (!qmi_tmd_cli->connection_active)
>>> +		return 0;
>>
>> [Severity: High]
>> Will returning 0 here without saving the new state to tmd->cur_state cause
>> thermal mitigation updates to be permanently lost?
>>
>> If the thermal core requests a new state while the remote QMI service is
>> down, this returns success but drops the state update. When the service
>> reconnects, qmi_tmd_register() will re-send the old tmd->cur_state to the
>> hardware.
>>
>> Could this leave the hardware running unmitigated while the thermal framework
>> believes it is actively mitigating?
> 
> Please address at these comments.
> 

thanks, checking these comments. will update on this.

>>
>> [ ... ]
>> @@ -298,34 +327,33 @@
>>> +static int qmi_tmd_register(struct qmi_tmd_client *qmi_tmd_cli,
>>> +			    const char *label, u8 max_state)
>>> +{
>>> +	struct device *dev = qmi_tmd_cli->dev;
>>> +	struct qmi_tmd *tmd;
>>> +	int index;
>>> +
>>> +	for (index = 0; index < qmi_tmd_cli->num_tmds; index++) {
>>> +		tmd = &qmi_tmd_cli->tmds[index];
>>> +
>>> +		if (!strncasecmp(tmd->name, label,
>>> +				 QMI_TMD_DEV_ID_LEN_MAX + 1))
>>> +			goto found;
>>> +	}
>>> +
>>> +	dev_dbg(qmi_tmd_cli->dev,
>>> +		"TMD '%s' available in firmware but not specified in DT\n",
>>> +		label);
>>> +	return 0;
>>> +
>>> +found:
>>> +	tmd->max_state = max_state;
>>> +
>>> +	/*
>>> +	 * If the cooling device already exists then the QMI service went away and
>>> +	 * came back. So just make sure the current cooling device state is
>>> +	 * reflected on the remote side and then return.
>>> +	 */
>>> +	if (tmd->cdev)
>>> +		return qmi_tmd_send_state_request(tmd, tmd->cur_state);
>>
>> [Severity: Medium]
>> Is there a race condition between reading tmd->cur_state here and concurrent
>> updates from the thermal core?
>>
>> When the QMI service reconnects, tmd->cur_state is read outside of the
>> qmi_tmd_cli->mutex. If a thermal trip point is crossed concurrently, could
>> the following sequence occur:
>>
>> CPU1: qmi_tmd_register()
>>    reads tmd->cur_state (e.g., state A) locklessly
>>    
>> CPU2: qmi_tmd_set_cur_state()
>>    acquires qmi_tmd_cli->mutex inside qmi_tmd_send_state_request()
>>    sends state B to hardware
>>    updates tmd->cur_state = B
>>    releases mutex
>>    
>> CPU1: qmi_tmd_send_state_request(tmd, state A)
>>    acquires qmi_tmd_cli->mutex
>>    sends stale state A to hardware
>>    reverts tmd->cur_state = A
>>
>> Will this overwrite the new state on the hardware and cause the thermal
>> framework's view of the mitigation state to become desynchronized?
>>
>>> +
>>> +	tmd->cdev = thermal_of_cooling_device_register(dev->of_node, index,
>>> +						       label, tmd, &qmi_tmd_cooling_ops);
>>
>> -- 
>> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
>