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

[Drew Fustini]

On Mon, May 25, 2026 at 12:30:07AM +0000, sashiko-bot@xxxxxxxxxx wrote:
> > +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?

I will convert everything to GENMASK_ULL.

> > +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.

This is a bisectability bug that I missed. I will fix this.

> > +	/*
> > +	 * 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?

ACPI parsing for RQSC runs at subsys_initcall. Every CPU in cpu_mask is
online and cacheinfo-populated by the time cbqri_probe_cc() runs. The
edge case is maxcpus= but I would rather wait until the usecase arises.

> > +	/*
> > +	 * 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?

I'll fix the off-by-one in v6. However, acpi_parse_rqsc() already caps
both counts at CBQRI_MAX_RCID/MCID = 1024 so the off-by-one is
unreachable today,

> > +		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.

Device tree discovery path can be created once there is a platform
driver that needs it.

-Drew