Re: [PATCH RFC v7 7/9] PM / devfreq: Introduce the QCOM SCMI Memlat devfreq driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Circular `devm` dependency creates a permanent resource leak of dummy platform devices and devfreq instances, leading to a Use-After-Free of the `memory` struct on module unload.
- [Medium] Dummy platform devices are created with generic names and PLATFORM_DEVID_NONE, causing global platform bus naming collisions.
--

commit 911bb9ff9b30677cf9eaa4d7a6794775e3393fc4
Author: Sibi Sankar <sibi.sankar@oss.qualcomm.com>

PM / devfreq: Introduce the QCOM SCMI Memlat devfreq driver

This commit introduces a devfreq SCMI client driver that uses the MEMLAT
algorithm to detect memory-latency-bound workloads. It controls the frequency
and level of memory buses like DDR and LLCC on Qualcomm platforms.

> diff --git a/drivers/devfreq/scmi-qcom-memlat-devfreq.c b/drivers/devfreq/scmi-qcom-memlat-devfreq.c
> new file mode 100644
> index 0000000000000..99ae229acfdc3
> --- /dev/null
> +++ b/drivers/devfreq/scmi-qcom-memlat-devfreq.c

[ ... ]

> +static void scmi_qcom_memlat_unwind(struct scmi_qcom_memlat_info *info, int count)
> +{
> +	for (int i = 0; i < count; i++) {
> +		struct scmi_qcom_memory_info *memory = info->memory[i];
> +
> +		if (IS_ERR_OR_NULL(memory) || IS_ERR_OR_NULL(memory->pdev))
> +			continue;
> +
> +		dev_pm_opp_remove_all_dynamic(&memory->pdev->dev);
> +		platform_device_unregister(memory->pdev);
> +	}
> +}

[ ... ]

> +	for (i = 0; i < info->memory_cnt; i++) {
> +		struct scmi_qcom_memory_info *memory = info->memory[i];
> +		struct platform_device *pdev = memory->pdev;
> +		struct devfreq_dev_profile *profile = &memory->profile;
> +
> +		/* sampling time should be double the devfreq observing time */
> +		profile->polling_ms = max(1U, info->cpucp_sample_ms / 2);
> +		profile->get_cur_freq = scmi_qcom_devfreq_get_cur_freq;
> +		profile->initial_freq = memory->min_freq > 1 ?
> +					(memory->min_freq * 1000UL) : memory->min_freq;
> +
> +		platform_set_drvdata(pdev, memory);
> +
> +		memory->devfreq = devm_devfreq_add_device(&pdev->dev, profile,
> +							  DEVFREQ_GOV_REMOTE, NULL);

[Severity: High]
Does this create a circular dependency that results in a permanent resource
leak and use-after-free on module unload?

The driver registers dummy platform devices without binding a driver to them,
and attaches devfreq instances to them using devm_devfreq_add_device() here.
The devfreq core internally calls get_device(dev->parent), taking a reference
on pdev->dev.

During module unload or probe failure, scmi_qcom_memlat_unwind() calls
platform_device_unregister(pdev), dropping the initial reference. However, for
a driverless device, devres cleanup actions are only executed when the device
is finally released (device_release()). Because the child devfreq device still
holds a reference to pdev->dev, its refcount never drops to zero, preventing
the devres cleanup from executing.

This creates a circular dependency where the devfreq device is permanently
leaked. When the SCMI driver finishes its removal, its own devres actions free
the memory structure (allocated via devm_kzalloc(&sdev->dev)). The leaked
devfreq device's delayed work queue continues to poll, dereferencing the freed
memory pointer via profile->get_cur_freq() in scmi_qcom_devfreq_get_cur_freq(),
which could result in a panic.

> +		if (IS_ERR(memory->devfreq)) {
> +			dev_err(&sdev->dev, "failed to add devfreq device\n");
> +			/* Stop sampling and voting timer */

[ ... ]

> +	for (i = 0; i < cfg_data->memory_cnt; i++) {
> +		const struct scmi_qcom_memory_cfg *memory_cfg = &cfg_data->memory_cfg[i];
> +		struct platform_device_info pdevinfo = { 0 };
> +
> +		pdevinfo.parent = &sdev->dev;
> +		pdevinfo.name = memory_cfg->name;
> +		pdevinfo.id = PLATFORM_DEVID_NONE;

[Severity: Medium]
Will this cause global platform bus naming collisions?

The driver registers dummy platform devices with generic names (e.g., "ddr",
"llcc", "ddr-qos") and uses PLATFORM_DEVID_NONE instead of PLATFORM_DEVID_AUTO.
As a result, devices are added to the global platform bus with exactly these
generic names.

If any other driver in the kernel registers a platform device with the same
name, or if there are multiple SCMI memlat instances active in the system,
platform_device_register_full() will fail with -EEXIST and abort the probe.

Could this use a driver-specific prefix or auto-assigned instance IDs to
prevent namespace collisions?

> +
> +		memory = devm_kzalloc(&sdev->dev, sizeof(*memory), GFP_KERNEL);
> +		if (!memory)
> +			return -ENOMEM;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610-rfc_v7_scmi_memlat-v7-0-f3f68c608f25@oss.qualcomm.com?part=7