Re: [PATCH v3 3/4] powercap: qcom: Add SPEL powercap driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Calculation of `time_unit` can result in 0 due to integer truncation, leading to a division-by-zero kernel panic on sysfs writes.
- [High] The `sp->domains` array containing `struct device` objects is freed prematurely when the SYS domain is unregistered, leading to a Use-After-Free.
- [High] Double free of the `sp->domains` array occurs in the `spel_probe` error path.
- [Medium] Large user inputs for power limits or time windows can silently overflow during translation, bypassing bounds checks.
- [Low] The driver unconditionally exposes non-functional `POWER_LIMIT1` sysfs attributes for domains that lack hardware constraints.
- [Low] Missing `mutex_destroy()` on initialized domain locks during the early error path in `spel_probe()`.
--

commit a896bac2c899d892a5883e225ce50e98f4608e61
Author: Manaf Meethalavalappu Pallikunhi <manaf.pallikunhi@oss.qualcomm.com>

powercap: qcom: Add SPEL powercap driver
    
The Qualcomm SoC Power and Electrical Limits (SPEL) provides hardware
based power monitoring and limiting capabilities for various power
domains including System, SoC, CPU clusters, GPU, and various other
subsystems.

> diff --git a/drivers/powercap/qcom_spel.c b/drivers/powercap/qcom_spel.c
> new file mode 100644
> index 0000000000000..e4ddf73903915
> --- /dev/null
> +++ b/drivers/powercap/qcom_spel.c

[ ... ]

> +static u64 spel_unit_xlate(struct spel_domain *sd, enum unit_type type,
> +			   u64 value, int to_raw)
> +{
> +	struct spel_system *sp = sd->sp;
> +	u64 units, scale;
> +
> +	switch (type) {
> +	case POWER_UNIT:
> +		units = sp->power_unit;
> +		scale = 1;
> +		break;
> +	case ENERGY_UNIT:
> +		units = sp->energy_unit;
> +		scale = ENERGY_UNIT_SCALE;
> +		break;
> +	case TIME_UNIT:
> +		units = sp->time_unit;
> +		scale = 1;
> +		break;
> +	default:
> +		return value;
> +	}
> +
> +	if (to_raw)
> +		return DIV_ROUND_CLOSEST_ULL(value * scale, units);

[Severity: High]
Can the time_unit calculation result in 0?

If the hardware register provides a shift >= 10, the time_unit calculation in
spel_update_unit() evaluates to 0 due to integer truncation:

	sp->time_unit = 1000 / (1 << shift);

If units is 0, the DIV_ROUND_CLOSEST_ULL() macro here will perform a division
by zero, triggering a kernel panic.

[ ... ]

> +static int spel_write_pl_data(struct spel_domain *sd, int pl,
> +			      enum pl_ops_type pl_op, unsigned long long value)
> +{
> +	struct spel_system *sp = sd->sp;
> +	void __iomem *reg_addr;
> +	u64 reg_val, new_val;
> +	int offset;
> +
> +	if (!is_pl_valid(sd, pl))
> +		return -EINVAL;
> +
> +	offset = get_pl_ops_offset(sd, pl, pl_op);
> +	if (offset < 0)
> +		return offset;
> +
> +	guard(mutex)(&sd->lock);
> +
> +	reg_addr = sp->constraint_base + offset;
> +	reg_val = readl(reg_addr);
> +
> +	switch (pl_op) {
> +	case PL_LIMIT:
> +		new_val = spel_unit_xlate(sd, POWER_UNIT, value, 1);
> +		if (new_val > FIELD_MAX(POWER_LIMIT_MASK))

[Severity: Medium]
Could large user inputs silently overflow during translation?

The user-provided 64-bit value via sysfs is passed to spel_unit_xlate(), which
uses the DIV_ROUND_CLOSEST_ULL() macro. If the user provides a very large
value, the internal addition of units / 2 within the macro could overflow the
64-bit unsigned integer, wrapping around to a small number.

This wrapped value would then pass the FIELD_MAX(POWER_LIMIT_MASK) check,
converting an intended boundary violation into a near-zero limit that could
aggressively throttle the hardware domain.

> +			return -EINVAL;

[ ... ]

> +static int spel_release_zone(struct powercap_zone *power_zone)
> +{
> +	struct spel_domain *sd = power_zone_to_spel_domain(power_zone);
> +
> +	/*
> +	 * Free the domains array when the last zone (SYS domain) is released.
> +	 * This ensures proper cleanup even if sysfs files are held open during unbind.
> +	 */
> +	if (sd->id == SPEL_DOMAIN_SYS) {
> +		struct spel_domain *domains = sd;
> +
> +		/* Destroy all mutexes before freeing the domains array */
> +		for (int i = 0; i < ARRAY_SIZE(spel_domain_names); i++)
> +			mutex_destroy(&domains[i].lock);
> +
> +		kfree(domains);

[Severity: High]
Does this prematurely free the sp->domains array?

The driver allocates all domain device objects in a single contiguous array and
frees the entire array when the SYS domain's release callback fires. However,
during driver unbind in spel_remove(), unregistering a child domain calls
device_unregister(), which synchronously drops the child's reference to its
parent.

This could allow the SYS domain to reach a refcount of 0 and be released,
freeing the domains array while a child domain's device is still in use
(e.g., if a user is holding open a sysfs file). When the user later closes the
file, the core would dereference the freed memory.

> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static void spel_init_domains(struct spel_system *sp)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(spel_domain_names); i++) {
> +		struct spel_domain *sd = &sp->domains[i];
> +
> +		sd->sp = sp;
> +		snprintf(sd->name, SPEL_DOMAIN_NAME_LENGTH, "%s",
> +			 spel_domain_names[i]);
> +		sd->id = i;
> +		sd->status_reg = sp->node_base + glymur_domain_offsets[i];
> +
> +		/* PL1 is always supported (required for powercap registration) */
> +		sp->limits[i] = BIT(POWER_LIMIT1);

[Severity: Low]
Does this unconditionally expose non-functional POWER_LIMIT1 sysfs attributes
for domains that lack hardware constraints?

For domains like IGPU that have no constraint register offsets defined in the
constraints array, reading or writing the exposed PL1 sysfs files will fall
through to get_pl_ops_offset() which returns -EOPNOTSUPP.

> +	}
> +}

[ ... ]

> +static int spel_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	ret = spel_register_powercap(sp);
> +	if (ret) {
> +		dev_err(dev, "Failed to register powercap zones\n");
> +		/*
> +		 * If SYS zone was registered, err_cleanup inside spel_register_powercap
> +		 * already unregistered all zones and spel_release_zone freed sp->domains.
> +		 * If SYS was never registered (sp->power_zone == NULL), free manually.
> +		 */
> +		if (!sp->power_zone)
> +			kfree(sp->domains);

[Severity: High]
Can this result in a double free of the domains array?

If powercap_register_zone() for the SYS domain fails deep within its execution
(such as during device_add()), it internally cleans up, triggering the
spel_release_zone() callback which executes kfree(domains).

The registration function would return an error and sp->power_zone remains NULL.
The error handler here checks if (!sp->power_zone) and calls kfree(sp->domains)
again, mistakenly double-freeing the array because it assumes NULL means the
release callback was never invoked.

[Severity: Low]
If we free sp->domains here before the devices are registered, do we leak
the debugging state of the mutexes?

spel_init_system() initializes a mutex for each of the 16 domains. If zone
registration fails early and we directly free the array, mutex_destroy() is
never called.

> +		kfree(sp);
> +		powercap_unregister_control_type(ct);
> +		return ret;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-qcom_spel_driver_upstream-v3-0-434d50f0c5b0@oss.qualcomm.com?part=3