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

[sashiko-bot@kernel.org]

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.
--

--- 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);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-b4-qmi-tmd-v6-0-973cd3a226af@oss.qualcomm.com?part=2