Re: [PATCH v8 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 3 potential issue(s) to consider:
- [Medium] Subsystem guideline violation: Mixing `goto`-based error handling with scope-based `__free()` and `scoped_guard()` declarations.
- [Medium] Data races on `tmd->max_state` and `tmd->cur_state` violating documented thermal lock semantics during QMI server restarts.
- [Medium] Out-of-bounds read in `qmi_tmd_register` due to unverified string null-termination from the remote QMI subsystem.
--

--- Patch [2]: [PATCH v8 2/9] soc: qcom: Add QMI TMD support for remote thermal mitigation ---
commit 3146a8e00a4c8c2a3c39c7e5a390154a8fbfd945
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
> --- /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.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260809-b4-qmi-tmd-v8-0-b15d47adc379@oss.qualcomm.com?part=2