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

[Chen, Yu C]

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