Re: [PATCH v3 8/8] riscv_cbqri: Add CBQRI capacity allocation platform driver

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Drew,

Apologies, my reply ended up in the wrong version of this series. Please consider this as a reply to v4.
ok with me if you respond in v4.

Reinette

On 7/7/26 11:18 AM, Reinette Chatre wrote:
> Hi Drew,
> 
> On 6/28/26 2:18 PM, Drew Fustini wrote:
> 
> ...> diff --git a/drivers/resctrl/cbqri_capacity.c b/drivers/resctrl/cbqri_capacity.c
>> new file mode 100644
>> index 000000000000..2172432eb328
>> --- /dev/null
>> +++ b/drivers/resctrl/cbqri_capacity.c
> 
> ...
> 
>> +static int cbqri_capacity_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct cbqri_controller_info info = {};
>> +	struct device_node *cache_np;
>> +	cpumask_var_t cpu_mask;
>> +	struct resource *res;
>> +	u32 rcid_count, cache_level;
>> +	int cache_id, cpu, ret;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res)
>> +		return -EINVAL;
>> +
>> +	ret = of_property_read_u32(dev->of_node, "riscv,cbqri-rcid", &rcid_count);
>> +	if (ret) {
>> +		dev_err(dev, "missing riscv,cbqri-rcid\n");
>> +		return ret;
>> +	}
>> +
>> +	cache_np = of_parse_phandle(dev->of_node, "riscv,cbqri-cache", 0);
>> +	if (!cache_np) {
>> +		dev_err(dev, "missing riscv,cbqri-cache phandle\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = of_property_read_u32(cache_np, "cache-level", &cache_level);
>> +	if (ret) {
>> +		dev_err(dev, "%pOF: missing cache-level\n", cache_np);
>> +		goto out_put;
>> +	}
>> +
>> +	if (!zalloc_cpumask_var(&cpu_mask, GFP_KERNEL)) {
>> +		ret = -ENOMEM;
>> +		goto out_put;
>> +	}
>> +
>> +	/*
>> +	 * Associate the controller with its cache instance via
>> +	 * cacheinfo. The matching cache provides the cache id and the
>> +	 * set of harts that share the cache.
>> +	 */
>> +	cache_id = -1;
>> +	cpus_read_lock();
>> +	for_each_online_cpu(cpu) {
>> +		struct cacheinfo *ci = get_cpu_cacheinfo_level(cpu, cache_level);
>> +
>> +		if (ci && ci->fw_token == cache_np) {
>> +			cache_id = ci->id;
>> +			cpumask_copy(cpu_mask, &ci->shared_cpu_map);
> 
> The way I understand cacheinfo::shared_cpu_map is that it only contains the online
> CPUs that share the cache with this CPU and if the CPU is offline then shared_cpu_map
> only contains the CPU self.
> 
> It is thus not clear to me that this handles all the possible CPU online vs offline
> scenarios. For example, if all or some CPUs of a domain are offline during cbqri_capacity_probe()
> and then come online later. It is not clear to me whether cbqri_controller_info::cache_id,
> cbqri_controller::cache_controller::cache_id, or cbqri_controller::cache_controller::cpu_mask
> are needed. Could the cache ID associated with a CPU at the time it comes online to dynamically
> associate it with the resctrl domain that is indexed by the cache ID? This may simplify a couple
> of flows.
> 
>> +			break;
>> +		}
>> +	}
>> +	cpus_read_unlock();
>> +
>> +	if (cache_id < 0) {
>> +		dev_err(dev, "%pOF: no online hart reports an L%u cache for this node\n",
>> +			cache_np, cache_level);
>> +		ret = -ENODEV;
>> +		goto out_free;
>> +	}
>> +
>> +	info.type = CBQRI_CONTROLLER_TYPE_CAPACITY;
>> +	info.addr = res->start;
>> +	info.size = resource_size(res);
>> +	info.rcid_count = rcid_count;
>> +	info.cache_id = cache_id;
>> +
>> +	ret = riscv_cbqri_register_cc_dt(&info, cache_level, cpu_mask);
>> +	if (ret) {
>> +		dev_err(dev, "failed to register capacity controller: %d\n", ret);
>> +		goto out_free;
>> +	}
>> +
>> +	dev_info(dev, "registered L%u capacity controller at %pa (cache_id=%d, rcid=%u)\n",
>> +		 cache_level, &info.addr, cache_id, rcid_count);
>> +
>> +out_free:
>> +	free_cpumask_var(cpu_mask);
>> +out_put:
>> +	of_node_put(cache_np);
>> +	return ret;
>> +}
> Reinette
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv