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

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.

> 
> [ ... ]
> @@ -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

-- 
With best wishes
Dmitry