Re: [PATCH RFC v5 07/18] riscv_cbqri: Add capacity controller probe and allocation device ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Build failure due to undefined macro `CBQRI_MON_CTL_EVT_ID_MASK`.
- [High] Generic registration API unconditionally calls ACPI-specific functions, breaking DT support.
- [High] Synchronous `cache_size` resolution during early boot permanently disables occupancy monitoring.
- [High] Bitwise inversion of `GENMASK` 32-bit masks silently zeroes the upper 32 bits of 64-bit hardware registers.
- [Medium] Invalid bounds check for `rcid_count` uses a register bitmask instead of a max count, and `mcid_count` is unchecked.
--

commit 13b45d9c450f72077987054f81435f84c681a8fe
Author: Drew Fustini <fustini@kernel.org>
Subject: riscv_cbqri: Add capacity controller probe and allocation device ops

Add support for the RISC-V CBQRI capacity controller (CC). The firmware
discovery layer (ACPI or DT) is responsible for passing the
cbqri_controller_info descriptor to riscv_cbqri_register_controller().

> diff --git a/drivers/resctrl/cbqri_devices.c b/drivers/resctrl/cbqri_devices.c
> new file mode 100644
> index 0000000000000..a97511825eedf
> --- /dev/null
> +++ b/drivers/resctrl/cbqri_devices.c
> @@ -0,0 +1,564 @@

[ ... ]

> +static int cbqri_cc_alloc_op(struct cbqri_controller *ctrl, int operation,
> +			     int rcid, enum cbqri_at at)
> +{
> +	int reg_offset = CBQRI_CC_ALLOC_CTL_OFF;
> +	int status;
> +	u64 reg;
> +
> +	lockdep_assert_held(&ctrl->lock);
> +
> +	if (cbqri_wait_busy_flag(ctrl, reg_offset, &reg) < 0) {
> +		pr_err_ratelimited("BUSY timeout before starting operation\n");
> +		return -EIO;
> +	}
> +	FIELD_MODIFY(CBQRI_CONTROL_REGISTERS_OP_MASK, &reg, operation);
> +	FIELD_MODIFY(CBQRI_CONTROL_REGISTERS_RCID_MASK, &reg, rcid);
> +
> +	/*
> +	 * CBQRI Table 1: AT 0=Data, 1=Code. Program AT on controllers
> +	 * that report supports_alloc_at_code. On controllers that don't,
> +	 * AT is reserved-zero and the op acts on both halves.
> +	 */
> +	reg &= ~CBQRI_CONTROL_REGISTERS_AT_MASK;

[Severity: High]
On 32-bit architectures, does bitwise inversion of a GENMASK macro (which
evaluates to unsigned long) silently zero the upper 32 bits of the 64-bit
register variable?

If the 32-bit inverted mask is zero-extended to 64 bits, this would truncate
bits 63:32, potentially clearing the STATUS and BUSY fields. Should
GENMASK_ULL be used for these masks instead?

> +	if (ctrl->cc.supports_alloc_at_code)
> +		reg |= FIELD_PREP(CBQRI_CONTROL_REGISTERS_AT_MASK, at);

[ ... ]

> +static int cbqri_probe_feature(struct cbqri_controller *ctrl, int reg_offset,
> +			       int operation, int *status, bool *access_type_supported)
> +{
> +	const u64 active_mask = CBQRI_CONTROL_REGISTERS_OP_MASK |
> +				CBQRI_CONTROL_REGISTERS_AT_MASK |
> +				CBQRI_CONTROL_REGISTERS_RCID_MASK |
> +				CBQRI_MON_CTL_EVT_ID_MASK;

[Severity: High]
Is CBQRI_MON_CTL_EVT_ID_MASK defined in this patch? It seems to be missing,
which might cause a build failure due to an undeclared identifier.

> +	u64 reg, saved_reg;
> +	int at;

[ ... ]

> +static int cbqri_probe_cc(struct cbqri_controller *ctrl)
> +{
> +	int err, status;
> +	u64 reg;

[ ... ]

> +	/*
> +	 * Resolve cache_size via cacheinfo. cpus_read_lock satisfies
> +	 * lockdep_assert_cpus_held() inside get_cpu_cacheinfo_level(). If
> +	 * every cpu_mask member is offline, cache_size stays 0 and the
> +	 * controller cannot back occupancy monitoring.
> +	 */
> +	cpus_read_lock();
> +	if (!ctrl->cache.cache_size) {
> +		int cpu = cpumask_first_and(&ctrl->cache.cpu_mask, cpu_online_mask);
> +
> +		if (cpu < nr_cpu_ids) {
> +			struct cacheinfo *ci;
> +
> +			ci = get_cpu_cacheinfo_level(cpu, ctrl->cache.cache_level);
> +			if (ci)
> +				ctrl->cache.cache_size = ci->size;

[Severity: High]
Is this resolution actually lazy? cbqri_probe_cc() is called immediately
during registration by cbqri_probe_controller(), which happens at
acpi_arch_init time.

Since cacheinfo is unpopulated during early boot, won't
get_cpu_cacheinfo_level() return NULL and leave cache_size as 0 permanently,
disabling occupancy monitoring?

> +		}
> +	}
> +	cpus_read_unlock();

[ ... ]

> +int riscv_cbqri_register_controller(const struct cbqri_controller_info *info)
> +{
> +	struct cbqri_controller *ctrl;
> +	int err;

[ ... ]

> +	ctrl->addr = info->addr;
> +	ctrl->size = info->size;
> +	ctrl->type = info->type;
> +	ctrl->rcid_count = info->rcid_count;
> +	ctrl->mcid_count = info->mcid_count;
> +
> +	/*
> +	 * SRMCFG encodes RCID in 12 bits.  ACPI's acpi_parse_rqsc() already
> +	 * caps info->rcid_count at CBQRI_MAX_RCID (1024) so this is unreachable
> +	 * today, but a future DT discovery path or a malformed firmware table
> +	 * routed through a different validator could bypass that ceiling.
> +	 * Catch the violation here rather than silently truncating in every
> +	 * FIELD_PREP(SRMCFG_RCID_MASK, closid) on the schedule-in fast path.
> +	 */
> +	if (WARN_ON_ONCE(ctrl->rcid_count > SRMCFG_RCID_MASK)) {

[Severity: Medium]
Does this correctly validate the count limit? If SRMCFG_RCID_MASK is a
register bitmask (like 0xFFF), won't a hardware limit of 4096 IDs be
incorrectly rejected because 4096 is greater than 4095?

Also, should there be a similar bounds check for mcid_count?

> +		cbqri_controller_destroy(ctrl);
> +		return -EINVAL;
> +	}
> +
> +	switch (info->type) {
> +	case CBQRI_CONTROLLER_TYPE_CAPACITY: {
> +		int level;
> +
> +		ctrl->cache.cache_id = info->cache_id;
> +
> +		level = find_acpi_cache_level_from_id(info->cache_id);

[Severity: High]
Does this unconditional call to ACPI-specific functions break Device Tree
support?

If CONFIG_ACPI is disabled, these stubs return -ENOENT. For DT-based
platforms, these functions might fail to resolve DT cache IDs, potentially
causing registration to fail entirely.

> +		if (level < 0) {
> +			pr_warn("Failed to resolve cache level for cache id 0x%x (%d), skipping\n",
> +				info->cache_id, level);
> +			cbqri_controller_destroy(ctrl);
> +			return level;
> +		}
> +		ctrl->cache.cache_level = level;

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260524-ssqosid-cbqri-rqsc-v7-0-v5-0-78d3a7ba9dbe@kernel.org?part=7