Re: [PATCH v2 2/3] powercap: qcom: Add SPEL powercap driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The devm-allocated `sp` structure is dereferenced in the asynchronous `spel_release_zone` callback, causing a Use-After-Free when the driver is unbound.
- [High] Manual kfree of the dynamically allocated `sp->domains` array leads to a Use-After-Free when devres attempts to run `mutex_destroy` on its embedded devm-managed mutexes.
- [Medium] The driver incorrectly exposes phantom powercap constraints for domains lacking hardware support, breaking UAPI expectations.
- [Medium] The formula used to calculate `sp->time_unit` contradicts the driver's own documentation and standard scaling logic, resulting in incorrect time unit conversion.
--

commit 53ea37bb963bb111e31263f8aaab73464a037f7d
Author: Manaf Meethalavalappu Pallikunhi <manaf.pallikunhi@oss.qualcomm.com>
Subject: powercap: qcom: Add SPEL powercap driver

This commit adds support for the Qualcomm SoC Power and Electrical Limits
(SPEL) hardware. It enables power limits to be enforced and monitored on
Qualcomm SoCs across multiple domains using the powercap framework.

> diff --git a/drivers/powercap/qcom_spel.c b/drivers/powercap/qcom_spel.c
> new file mode 100644
> index 0000000000000..4dd91cf36ccca
> --- /dev/null
> +++ b/drivers/powercap/qcom_spel.c
[ ... ]
> +static int spel_release_zone(struct powercap_zone *power_zone)
> +{
> +	struct spel_domain *sd = power_zone_to_spel_domain(power_zone);
> +	struct spel_system *sp = sd->sp;
> +
> +	/* Free the domains array when the last zone (SYS domain) is released */
> +	if (sd->id == SPEL_DOMAIN_SYS)
> +		kfree(sp->domains);

[Severity: High]
If the device is unbound while a sysfs file is held open, devres will free
the devm_kzalloc allocated sp structure immediately. When the user later
closes the sysfs file, the powercap framework will asynchronously call this
release function. Does accessing sp->domains here cause a use-after-free
read?

[Severity: High]
Also, earlier in spel_init_system(), devm_mutex_init() is called on sd->lock
which is embedded inside this dynamically allocated sp->domains array:

spel_init_system() {
        ...
        ret = devm_mutex_init(dev, &sd->lock);

If sp->domains is manually freed here with kfree(), will devres trigger a
use-after-free write when it later runs mutex_destroy() on the embedded
locks during device removal or probe failure?

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

[Severity: Medium]
Is it strictly required for all domains to have PL1 enabled?

The powercap framework supports zones with zero constraints for purely
monitoring purposes. Since hardware limits are only defined for the SYS and
SOC domains in the constraints array, unconditionally setting PL1 for other
domains (like CL0 or IGPU) causes constraint sysfs files to be created that
will always fail with an error when read or written. Does this expose a
broken UAPI?

> +	}
> +}
> +
> +static void spel_update_unit(struct spel_system *sp)
> +{
> +	u32 value, shift;
> +
> +	/* Read power_unit and time_unit from offset 0x0 */
> +	value = readl(sp->config_base);
> +
> +	/*
> +	 * Unit calculation: 1 / (2^shift)
> +	 * Masks limit: TIME_UNIT (4 bits, max 15), POWER_UNIT (3 bits, max 7).
> +	 */
> +	shift = FIELD_GET(POWER_UNIT_MASK, value);
> +	sp->power_unit = 1000000 / (1 << shift);
> +
> +	shift = FIELD_GET(TIME_UNIT_MASK, value);
> +	/*
> +	 * Time window in register is in milliseconds.
> +	 */
> +	sp->time_unit = 1000 * (1 << shift);

[Severity: Medium]
The comment above states the unit calculation is 1 / (2^shift).

For the power unit, the driver correctly divides. However, for the time unit
it multiplies by the shift value instead. Should this be a division
(1000 / (1 << shift)) to correctly scale the time unit into microseconds?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260620-qcom_spel_driver_upstream-v2-0-a3ee6837c18f@oss.qualcomm.com?part=2