Re: [PATCH v2 1/6] x86/resctrl: Parse ACPI ERDT table and map RMDD domains by L3 cache ID

["Chen, Yu C" <yu.c.chen@intel.com>]

Hi Thomas,

Thanks for the thorough review.

On 6/5/2026 12:56 AM, Thomas Gleixner wrote:
> On Fri, Jun 05 2026 at 00:08, Chen Yu wrote:
>> @@ -1130,20 +1131,24 @@ static int __init resctrl_arch_late_init(void)
>>   
>>   	check_quirks();
>>   
>> -	if (!get_rdt_resources())
>> -		return -ENODEV;
>> +	if (!get_rdt_resources()) {
>> +		ret = -ENODEV;
>> +		goto out;
> 
> You can spare all that goto mess by renaming this function to
> __resctrl_arch_late_init() and have a new
> 
> static int __init resctrl_arch_late_init(void)
> {
> 	int ret = __resctrl_arch_late_init();
> 
>          if (ret)
>          	erdt_exit();
>          return ret;
> }
> 
> No?
> 

OK, renamed the original to __resctrl_arch_late_init() and added
the wrapper.

>> +struct erdt_domain_info {
>> +	struct acpi_erdt_cmrc *cmrc;
>> +	/* MMIO  address */
>> +	void __iomem *base[ERDT_MMIO_MAX];
>> +};
> 
> https://docs.kernel.org/process/maintainer-tip.html#struct-declarations-and-initializers
> 
> and the rest of that document.
> 

OK, reformatted to use tabular member alignment.

>> +
>> +/* true if ERDT table is present and valid */
>> +static bool erdt_available;
>> +
>> +/* Global variable to hold ERDT ACPI table information for later processing */
>> +static DEFINE_XARRAY(erdt_domain_xa); /* Indexed by L3 cache ID */
> 
> No tail comments please
> 

OK.

>> +#define ERDT_VALID_VERSION 1
>> +
>> +static u32 valid_subtbl_mask;
>> +
>> +/*
>> + * erdt_enabled - Check if the ERDT table is present and enabled
>> + */
>> +bool erdt_enabled(void)
>> +{
>> +	return erdt_available;
> 
> This naming convention is confusing at best. First you declare
> 
>> +/* true if ERDT table is present and valid */
>> +static bool erdt_available;
> 
> Which says nothing about enabled and then you claim that reading the
> available variable tells you whether it is enabled. Can you please make
> your mind up and make this consistent and comprehensible?
> 

OK, renamed the static variable to 'erdt_enabled_flag' so it
matches the accessor erdt_enabled().

>> +/*
>> + * lookup_logical_cpu_by_x2apicid - Map x2APIC ID to logical CPU number
>> + */
>> +static __init int lookup_logical_cpu_by_x2apicid(u32 x2apicid)
>> +{
>> +	int cpu;
>> +
>> +	for_each_possible_cpu(cpu) {
>> +		if (cpu_physical_id(cpu) == x2apicid)
>> +			return cpu;
>> +	}
>> +
>> +	return -1;
> 
> No. The topology code has topo_lookup_cpuid(). Please expose that
> instead of hacking up your own version of it.
> 

OK, dropped lookup_logical_cpu_by_x2apicid() and make
topo_lookup_cpuid() non-static and use this helper instead.

>> +/*
> 
> Not a valid kernel doc comment. Those start with /**
> 

OK, fixed.

>> +
>> +static void __iomem *erdt_ioremap_checked(phys_addr_t base, u32 size,
>> +					  const char *desc)
>> +{
>> +	void __iomem *addr = ioremap(base, size << 12);
>> +
>> +	if (!addr)
>> +		pr_err("ERDT: Failed to map %s at phys addr %#llx (size: %u pages)\n",
>> +		       desc, (unsigned long long)base, size);
> 
> Lacks brackets. See bracket rules.
> 

OK, added brackets since the pr_err() spans multiple lines.

>> +	return addr;
>> +}
>> +
> 
>> +static __init bool cacd_init(struct erdt_domain_info *d,
>> +			     struct acpi_subtbl_hdr_16 *subtbl,
>> +			     int *l3_cache_id)
> 
> You have 100 characters. Please use them.
> 

OK, the functions that fit within 100 columns are now on a
single line(and subsequent patches).

>> +{
>> +	*l3_cache_id = get_l3_cache_id_from_cacd((struct acpi_erdt_cacd *)subtbl);
>> +
>> +	return *l3_cache_id != -1;
>> +}
>> +
>> +static __init bool parse_rmdd_entry(struct acpi_subtbl_hdr_16 *rmdd_hdr)
>> +{
>> +	struct acpi_erdt_rmdd *rmdd;
>> +	struct erdt_domain_info *domain_info;
>> +	struct acpi_subtbl_hdr_16 *subtbl;
>> +	int l3_cache_id = -1;
>> +	u32 subtbl_mask = 0;
>> +	void *rmdd_end;
> 
> See variable declaration doc chapter
> 

OK, reordered in reverse fir-tree.

>> +
>> +	if (rmdd_hdr->length < sizeof(*rmdd)) {
>> +		pr_info(FW_BUG "Invalid RMDD length %u\n", rmdd_hdr->length);
>> +		return false;
>> +	}
>> +
>> +	rmdd = (struct acpi_erdt_rmdd *)rmdd_hdr;
>> +
>> +	/* Quietly ignore non-CPU-based L3 domains */
>> +	if (!(rmdd->flags & 0x1))
> 
> 0x1 is really a useful and intuitive constant. Use a proper define for it.
> 

Added a named constant and use it in the check.

>> +		return true;
>> +
>> +	domain_info = kzalloc(sizeof(*domain_info), GFP_KERNEL);
>> +	if (!domain_info)
>> +		return false;
>> +
>> +	domain_info->base[ERDT_MMIO_RMDD_CREG] = erdt_ioremap_checked(rmdd->creg_base, rmdd->creg_size,
>> +								      "RMDD ctrl base");
>> +	if (!domain_info->base[ERDT_MMIO_RMDD_CREG])
>> +		goto cleanup;
>> +
>> +	rmdd_end = (void *)rmdd + rmdd->header.length;
>> +
>> +	/* Iterate through all sub-structures inside this RMDD block */
>> +	for (subtbl = (void *)rmdd + sizeof(*rmdd);
>> +	     (void *)subtbl + sizeof(*subtbl) <= rmdd_end;
>> +	     subtbl = (void *)subtbl + subtbl->length) {
>> +		if (subtbl->length < sizeof(*subtbl) ||
>> +		    (void *)subtbl + subtbl->length > rmdd_end) {
> 
> This unreadable type cast orgy makes my eyes bleed.
> 
>          for (subtbl = rmdd_subtbl(rmdd); subtbl_valid(rmdd, subtbl); subtbl = next_subtbl(subtbl))
> 
> or something comprehensible like that.
> 

OK, will do.

> Also this gem is made up nonsense:
> 
>> +		if (subtbl->length < sizeof(*subtbl) ||
>> +		    (void *)subtbl + subtbl->length > rmdd_end) {
> 
> Because of the loop condition above it is equivalent to:
> 
>                  if (subtbl->length != sizeof(*subtbl))
>                          fail();
> 

sizeof(*subtbl) is the 8-byte header. CACD's length is variable
(8 + Enumeration-IDs array), so a valid CACD always has
length > sizeof(*subtbl). Using subtbl->length!= sizeof(*subtbl)
might reject the valid entry.

Moved the validation into subtbl_valid().

thanks,
Chenyu