Re: [PATCH v5 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 5 potential issue(s) to consider:
- [High] Data race on `tmd->cur_state` between `qmi_tmd_register()` and `qmi_tmd_set_cur_state()` causes thermal mitigation state desynchronization.
- [High] The `mitigation_device_list_len` field in `struct tmd_get_dev_list_resp` is defined as a `u8`, causing a 3-byte out-of-bounds write when decoded by the QMI framework.
- [Medium] Current state is not capped to the new maximum state during firmware restart synchronization.
- [Medium] Improper dependencies prevent COMPILE_TEST coverage and force dependencies to become built-in.
- [Low] Building with CONFIG_THERMAL=m and CONFIG_QCOM_QMI_TMD=y causes a linker error.
--

--- Patch [2]: [PATCH v5 2/9] soc: qcom: Add QMI TMD support for remote thermal mitigation ---
commit 977e2b6f2dd6931cec0ac743a4bead6a6e561872
Author: Casey Connolly <casey.connolly@linaro.org>

soc: qcom: Add QMI TMD support for remote thermal mitigation

This commit adds support for QMI-based Thermal Mitigation Device (TMD)
cooling devices provided by remote subsystems. It introduces a driver
that discovers the TMD service via QMI, registers cooling devices for
available mitigation endpoints, and forwards state updates.

> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index fd4d4ecd2df0f..5c38d598e4864 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -319,6 +319,17 @@ config QCOM_QMI_HELPERS
>  	tristate
>  	depends on NET
>  
> +config QCOM_QMI_TMD
> +	bool "Qualcomm remote subsystem TMD" if COMPILE_TEST
> +	depends on ARCH_QCOM && NET
> +	select QRTR
> +	select QCOM_QMI_HELPERS

[Severity: Medium]
Does defining this as a boolean rather than a tristate force the selected
tristate dependencies (QRTR and QCOM_QMI_HELPERS) to become built-in when
enabled? 

Also, since the depends line strictly requires ARCH_QCOM, does this prevent
the COMPILE_TEST coverage from working on other architectures?

[Severity: Low]
If the kernel is configured with THERMAL=m and QCOM_QMI_TMD=y, will this
cause a linker error due to the missing THERMAL dependency?

> +	help
> +	  This enables Qualcomm Messaging Interface (QMI) based Thermal Mitigation

[ ... ]

> diff --git a/drivers/soc/qcom/qmi_tmd.c b/drivers/soc/qcom/qmi_tmd.c
> new file mode 100644
> index 0000000000000..b31046241af8b
> --- /dev/null
> +++ b/drivers/soc/qcom/qmi_tmd.c

[ ... ]

> +struct tmd_get_dev_list_resp {
> +	struct qmi_response_type_v01 resp;
> +	u8 mitigation_device_list_valid;
> +	u8 mitigation_device_list_len;
> +	struct tmd_dev_list
> +		mitigation_device_list[QMI_TMD_DEV_LIST_MAX];
> +};

[Severity: High]
Since the QMI framework decodes QMI_DATA_LEN elements by writing 4 bytes
(sizeof(u32)), does defining this length field as a u8 cause a 3-byte
out-of-bounds write?

Could this memory corruption overwrite the first few bytes of the
mitigation_device_list array during decoding?

[ ... ]

> +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;
> +
> +	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;
> +
> +	/*
> +	 * 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]
If the remote QMI service restarts and advertises a lower maximum mitigation
level than before, will this send the old uncapped state to the firmware?

Should we clamp the current state to the new maximum state before sending
the request?

[Severity: High]
Can a concurrent thermal state update cause a race condition here?

If qmi_tmd_register() runs in the workqueue context and reads the state without
a lock:

qmi_tmd_svc_arrive()->qmi_tmd_register()
    return qmi_tmd_send_state_request(tmd, tmd->cur_state);
    
And qmi_tmd_set_cur_state() concurrently updates the state:

qmi_tmd_set_cur_state()
    ret = qmi_tmd_send_state_request(tmd, state);
    if (!ret)
        tmd->cur_state = state;
        
Could qmi_tmd_register() end up sending a stale state to the firmware, causing
the remote mitigation state to become permanently desynchronized?

> +
> +	tmd->cdev = thermal_of_cooling_device_register(dev->of_node, index,
> +						       label, tmd, &qmi_tmd_cooling_ops);
> +	if (IS_ERR(tmd->cdev)) {
> +		int ret = PTR_ERR(tmd->cdev);
> +
> +		tmd->cdev = NULL;
> +		return ret;
> +	}
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-b4-qmi-tmd-v5-0-99620e07f505@oss.qualcomm.com?part=2