Re: [PATCH 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] Use-After-Free of `struct powercap_zone` due to `devm_kcalloc` allocation mismatch with sysfs lifecycle.
- [Medium] Phantom power limit constraints are exposed for domains lacking hardware support and register definitions.
- [Medium] Writing the time window register completely overwrites it, clobbering reserved or unmapped bits.
- [Medium] Missing bounds check for time window values causes silent truncation.
--

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

powercap: qcom: Add SPEL powercap driver

This commit adds a new powercap driver for the Qualcomm SoC Power and
Electrical Limits (SPEL) hardware. It provides hardware-based power
monitoring and limiting capabilities for various power domains.

> diff --git a/drivers/powercap/qcom_spel.c b/drivers/powercap/qcom_spel.c
> new file mode 100644
> index 0000000000000..fed5647959a53
> --- /dev/null
> +++ b/drivers/powercap/qcom_spel.c
[ ... ]
> +static int spel_write_pl_data(struct spel_domain *sd, int pl,
> +			      enum pl_ops_type pl_op, unsigned long long value)
> +{
[ ... ]
> +	case PL_LIMIT:
> +		new_val = spel_unit_xlate(sd, POWER_UNIT, value, 1);
> +		if (new_val > POWER_LIMIT_MASK)
> +			return -EINVAL;
> +		reg_val = (reg_val & ~POWER_LIMIT_MASK) | new_val;
[ ... ]
> +	case PL_TIME_WINDOW:
> +		/*
> +		 * Encode time window: upper 7 bits to [22:16], lower 15 bits to [14:0]
> +		 * Time window register is separate from limit register (different offset),
> +		 * so we write only the time window bits without preserving any enable bit.
> +		 */
> +		new_val = spel_unit_xlate(sd, TIME_UNIT, value, 1);

Could this silently truncate the time window if the user provides a value
that exceeds the hardware's 22-bit capacity (0x3FFFFF)? 

It looks like there is no bounds check here, similar to the one for PL_LIMIT
above, so the bitwise operations would just discard the upper bits.

> +		reg_val = (((new_val >> 15) & 0x7F) << 16) |
> +			  (new_val & 0x7FFF);

Does this assignment clobber reserved or unmapped bits in the register?

The original register value is read into reg_val via readl(), but this
directly overwrites it, discarding bit 15 and bits 23-31. Should this
perform a read-modify-write by masking out only TIME_WINDOW_MASK_H and
TIME_WINDOW_MASK_L instead?

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	writel(reg_val, reg_addr);
> +	return 0;
> +}
[ ... ]
> +static void spel_init_domains(struct spel_system *sp)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < SPEL_DOMAIN_MAX; 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 + domain_offsets[i];
> +
> +		/* PL1 is always supported (required for powercap registration) */
> +		sp->limits[i] = BIT(POWER_LIMIT1);
> +		sd->pl_name[POWER_LIMIT1] = pl_names[POWER_LIMIT1];

Does this expose phantom power limit constraints for domains lacking hardware
support? 

The constraints array only defines register offsets for SPEL_DOMAIN_SYS and
SPEL_DOMAIN_SOC. If POWER_LIMIT1 is unconditionally enabled for all domains
like CL0 and IGPU, any userspace read or write to these constraint attributes
will fall back to get_pl_ops_offset() and return -EOPNOTSUPP since there are
no matching entries.

Since the powercap framework supports zones with 0 constraints, should the
driver rely purely on the hardware capabilities register to discover supported
limits instead?

> +	}
> +}
[ ... ]
> +static int spel_init_system(struct spel_system *sp, struct device *dev)
> +{
> +	int i, ret;
> +
> +	/* Read unit configuration (common for all domains) */
> +	ret = spel_check_unit(sp);
> +	if (ret) {
> +		dev_err(dev, "Failed to read unit config\n");
> +		return ret;
> +	}
> +
> +	sp->domains = devm_kcalloc(dev, SPEL_DOMAIN_MAX,
> +				   sizeof(struct spel_domain), GFP_KERNEL);

Can this lead to a use-after-free if userspace holds a sysfs file open?

The driver embeds a struct powercap_zone inside struct spel_domain, which is
allocated via devm_kcalloc() here.

When the driver is unbound, powercap_unregister_zone() is called, which calls
device_unregister() to drop a reference. If user-space holds a sysfs file
open, the device reference count remains > 0. When remove() returns, devres
will immediately free the memory. Once the file is closed, the final kref_put()
will attempt to invoke the .release callback on the already-freed memory.

Should structures embedding reference-counted devices be dynamically allocated
and freed inside their .release callback?

> +	if (!sp->domains)
> +		return -ENOMEM;
> +
> +	spel_init_domains(sp);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-qcom_spel_driver_upstream-v1-0-75356d1b7f94@oss.qualcomm.com?part=2