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

[Thomas Gleixner <tglx@kernel.org>]

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?

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

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

> +#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?

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

> +/*

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

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

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

> +{
> +	*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 

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

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

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();

But that's not convoluted enough it seems.

Thanks,

        tglx