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

[Gaurav Kohli <gaurav.kohli@oss.qualcomm.com>]

On 7/27/2026 8:10 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Unprotected read of `tmd->cur_state` during QMI reconnect can cause the remote mitigation state to permanently desynchronize from the thermal core.
> - [Medium] Data race on `tmd->max_state` between the QMI worker and thermal core callbacks.

This looks false positive, as max state is not going to change with same 
firmware.

> --
> 
> --- Patch [2]: [PATCH v6 2/9] soc: qcom: Add QMI TMD support for remote thermal mitigation ---
> commit 8a125ac63b99f7508e9f1e7c025b0c0da679cac6
> Author: Casey Connolly <casey.connolly@linaro.org>
> 
> soc: qcom: Add QMI TMD support for remote thermal mitigation
> 
> Add support for Qualcomm Messaging Interface (QMI) based Thermal Mitigation
> Device (TMD) cooling devices provided by remote subsystems.
> 
>> diff --git a/drivers/soc/qcom/qmi_tmd.c b/drivers/soc/qcom/qmi_tmd.c
>> new file mode 100644
>> index 0000000000000..555f2aff1ac0f
>> --- /dev/null
>> +++ b/drivers/soc/qcom/qmi_tmd.c
> 
> [ ... ]
> 
>> +static int qmi_tmd_set_cur_state(struct thermal_cooling_device *cdev,
>> +				 unsigned long state)
>> +{
>> +	struct qmi_tmd *tmd = cdev->devdata;
>> +	int ret;
>> +
>> +	if (state > tmd->max_state)
>> +		return -EINVAL;
>> +
>> +	/* cur_state is protected by thermal core's cdev->lock */
>> +	if (tmd->cur_state == state)
>> +		return 0;
>> +
>> +	ret = qmi_tmd_send_state_request(tmd, state);
>> +	if (!ret)
>> +		tmd->cur_state = state;
> 
> [Severity: High]
> Can the remote mitigation state permanently desynchronize from the thermal
> core here?
> 
> If a remote QMI subsystem restarts, it triggers qmi_tmd_svc_arrive() and
> qmi_tmd_register(). Concurrently, the thermal core might update the cooling
> state via qmi_tmd_set_cur_state().
> 
> In qmi_tmd_set_cur_state(), the request is sent first, and tmd->cur_state is
> updated afterwards without holding qmi_tmd_cli->mutex. This creates a window
> where the state has been updated on the remote device, but tmd->cur_state
> does not yet reflect the new state.
> 
> Could the following sequence occur?
> 
> CPU A (thermal core)
> qmi_tmd_set_cur_state()
>      qmi_tmd_send_state_request()
>          // acquires and releases mutex, sends new state
>      <preempted before updating tmd->cur_state>
> 
> CPU B (reconnect work)
> qmi_tmd_register()
>      reads tmd->cur_state (which is still the old state)
>      qmi_tmd_send_state_request()
>          // acquires mutex, sends old state
> 
> CPU A (resumes)
>      tmd->cur_state = state;
> 
> If this happens, the remote device would be left running with the old state,
> while the local thermal core tracks the new state. Should tmd->cur_state be
> read and updated under the protection of a lock?
> 
>> +
>> +	return ret;
>> +}
> 
> [ ... ]
> 
>> +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;
> 
> [Severity: Medium]
> Could this write to tmd->max_state cause a data race?
> 
> This assignment in qmi_tmd_register() is executed from a workqueue when
> the QMI service arrives, but it does not hold a lock. Concurrently, the
> thermal core can invoke qmi_tmd_get_max_state() or qmi_tmd_set_cur_state(),
> which both read tmd->max_state without holding shared locks.
> 
> While tearing might be unlikely on most architectures, could this
> unsynchronized read and write trigger KCSAN warnings or lead to undefined
> behavior?
> 
>> +
>> +	/*
>> +	 * 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);
>> +
>> +	tmd->cdev = thermal_of_cooling_device_register(dev->of_node, index,
>> +						       label, tmd, &qmi_tmd_cooling_ops);
>