Re: [PATCH RFC v6 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 1 potential issue(s) to consider:
- [High] `cache_size` permanently remains 0 because cacheinfo lookup happens synchronously during early discovery before it is populated.
--

commit 931d91fef2d993a3edb9eef2d3d5e5329e33655b
Author: Drew Fustini <fustini@kernel.org>

riscv_cbqri: Add capacity controller probe and allocation device ops

Add support for the RISC-V CBQRI capacity controller (CC). The firmware
discovery layer 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
> --- /dev/null
> +++ b/drivers/resctrl/cbqri_devices.c

[ ... ]

> +static int cbqri_probe_cc(struct cbqri_controller *ctrl)
> +{

[ ... ]

> +	/*
> +	 * 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]
Since this lookup happens synchronously as part of the initial controller
registration (which happens during early discovery), will
get_cpu_cacheinfo_level() always return NULL here?

If cacheinfo is unpopulated at this stage, it seems cache_size will be left
as 0, permanently disabling occupancy monitoring for this controller.

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

[ ... ]

> +int riscv_cbqri_register_controller(const struct cbqri_controller_info *info)
> +{

[ ... ]

> +	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);
> +		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;
> +
> +		/*
> +		 * cache_size stays at 0 here. cacheinfo is not populated
> +		 * yet at acpi_arch_init time. Filled lazily during probe
> +		 * via get_cpu_cacheinfo_level().
> +		 */

[Severity: High]
The comment mentions that the size will be filled lazily during probe because
cacheinfo is not populated yet at acpi_arch_init time.

However, doesn't the registration function immediately call
cbqri_probe_controller() below, making the probe synchronous rather than lazy?

> +		err = acpi_pptt_get_cpumask_from_cache_id(info->cache_id,
> +							  &ctrl->cache.cpu_mask);
> +		if (err) {
> +			pr_warn("Failed to get cpumask for cache id 0x%x (%d), skipping\n",
> +				info->cache_id, err);
> +			cbqri_controller_destroy(ctrl);
> +			return err;
> +		}
> +		break;
> +	}
> +	default:
> +		pr_warn("controller at %pa: unknown type %u, skipping\n",
> +			&ctrl->addr, info->type);
> +		cbqri_controller_destroy(ctrl);
> +		return -EINVAL;
> +	}
> +
> +	err = cbqri_probe_controller(ctrl);
> +	if (err) {
> +		cbqri_controller_destroy(ctrl);
> +		return err;
> +	}
> +
> +	list_add_tail(&ctrl->list, &cbqri_controllers);
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601-ssqosid-cbqri-rqsc-v7-0-v6-0-baf00f50028a@kernel.org?part=7