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

On 8/9/2026 6:17 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

older connections issue is not flagged now. All other looks false 
positive, updated inline.

> - [Medium] Subsystem guideline violation: Mixing `goto`-based error handling with scope-based `__free()` and `scoped_guard()` declarations.

False positive. This is a Style nit, not a bug. Mixing these patterns is 
common. Scoped_guard block correctly unwinds the mutex via the 
for-loop-based macro expansion before jumping — no lock leak.

> - [Medium] Data races on `tmd->max_state` and `tmd->cur_state` violating documented thermal lock semantics during QMI server restarts.

False positive. max_state reflects a fixed firmware characteristic and 
never changes value across svc_arrive reconnects. cur_state is already 
serialized — writes go through qmi_tmd_cli->mutex and reads/writes from 
the thermal framework side go through cdev->lock — so there is no 
unprotected concurrent access.


> - [Medium] Out-of-bounds read in `qmi_tmd_register` due to unverified string null-termination from the remote QMI subsystem.

False positive. qmi_decode_string_elem() in qmi_encdec.c always writes 
buf_dst[string_len] = '\0' after decode, so string is always 
null-terminated.

> --
> 
> --- Patch [2]: [PATCH v8 2/9] soc: qcom: Add QMI TMD support for remote thermal mitigation ---
> commit 3146a8e00a4c8c2a3c39c7e5a390154a8fbfd945
> Author: Casey Connolly <[email protected]>
> 
> 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
>> --- /dev/null
>> +++ b/drivers/soc/qcom/qmi_tmd.c
> [ ... ]
>> +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);
> 
> [Severity: Medium]
> If the remote processor sends a mitigation_dev_id that exactly fills the
> 33-byte array without a null terminator, will this dev_dbg() statement read
> past the end of the buffer when printing the string?
> 
>> +	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) {
>> +		guard(mutex)(&qmi_tmd_cli->mutex);
>> +
>> +		return qmi_tmd_send_state_request(tmd, tmd->cur_state);
>> +	}
> 
> [Severity: Medium]
> Can this result in a data race on tmd->max_state and tmd->cur_state?
> 
> The code in qmi_tmd_get_cur_state() documents that cur_state is protected
> by the thermal core's cdev->lock. However, max_state is updated here without
> holding any lock, and the call to qmi_tmd_send_state_request() updates
> cur_state while holding only qmi_tmd_cli->mutex.
> 
> Will concurrent readers executing thermal framework callbacks see torn or
> stale values?
> 
> [ ... ]
>> +static void qmi_tmd_svc_arrive(struct work_struct *work)
>> +{
>> +	struct qmi_tmd_client *qmi_tmd_cli =
>> +		container_of(work, struct qmi_tmd_client, svc_arrive_work);
>> +
>> +	struct tmd_get_dev_list_req req = { 0 };
>> +	struct tmd_get_dev_list_resp *resp __free(kfree) = NULL;
>> +	int ret, i;
>> +	struct qmi_txn txn;
>> +
>> +	resp = kzalloc_obj(*resp, GFP_KERNEL);
>> +	if (!resp) {
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	scoped_guard(mutex, &qmi_tmd_cli->mutex) {
>> +		if (qmi_tmd_cli->exiting)
>> +			return;
>> +
>> +		ret = qmi_txn_init(&qmi_tmd_cli->handle, &txn,
>> +				   tmd_get_dev_list_resp_ei, resp);
>> +		if (ret < 0)
>> +			goto out;
> 
> [Severity: Medium]
> Is it safe to mix goto error handling with __free() and scoped_guard() in
> this function?
> 
> The cleanup subsystem guidelines document that functions should either use
> goto-based cleanups or scope-based cleanups, but never mix them.
> 
> Additionally, using a goto label to escape a scoped_guard() block violates
> the expected scope lifetime, and defining variables with split
> initialization like __free(kfree) = NULL before later assignment is strongly
> discouraged.
>