From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 182872DF15C for ; Thu, 4 Jun 2026 16:56:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780592199; cv=none; b=mHtGmYlytkesdLsEeYVRvquUw3lY8iNtY16OhnFzgfGuAp5crOUK6YjKZVCoi8XOtxRlmIgZT9zm6UwWiSZVTxym92WHa/Vkuy7ttyagogC133CatidLTROm1z7F5HnjVtBtpOfAH1PHFosqJFyzPPXTHfOHdhi1R3LNN8dOkhA= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780592199; c=relaxed/simple; bh=inlIUZwF7pZtWGK5afc2bdCg4xrm2IS3thIolr5tXxs=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=FefseYAcUPJZpSUl6hCv4eHMi8plLnVeLsvhq2DU3bAzXoN1E8GLjDvD0Wwz+/6stg1nW9+8lwMPGNpZ0lxWuQxNuUPj/+f8XAHx4NiUkdUasoE5ecjjEXLN3ub82fbRrR8l2vLRj0ddUQnvz9JYutajXRXM+SulnXoD0M6jfno= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=RPVSBa0D; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="RPVSBa0D" Received: by smtp.kernel.org (Postfix) with ESMTPSA id F17471F00893; Thu, 4 Jun 2026 16:56:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1780592197; bh=LvNBhnmq/kn42oDo95BuvPB+QucpGLpYMdJbRkc3Cvg=; h=From:To:Cc:Subject:In-Reply-To:References:Date; b=RPVSBa0DCBJtUBwWkWqdR6lCNcAv5xdBxflMTiPuGPIYKcQAz4yrzonOkQJdibSoZ HRjFst+JpAwMPyWQfYeinHbTKDLjOC+Z2eQNgzXOxTtg0g5iTq3JmbD57BlLbAk4HX 35pJXFsd4PuP4ZYhupGG4d3LJqRrWEhEYtBSGmylFmbAwHzSIO2msCWKEjv9GYCnIf Sxc6mvGakC3LxNWb3ghQclPSS7VekmMC2C//jH/dz0+27dmHZlQgsPUU8PMxwqd3Vl raTochwvc/6RfPNdOYRgYo3mmeNfbiIC6I9GAnwr/lXjpRyoCfjU+r+EbHKJ3GSJKw vXU8xcGVbZYDw== From: Thomas Gleixner To: Chen Yu , tony.luck@intel.com, reinette.chatre@intel.com Cc: x86@kernel.org, linux-kernel@vger.kernel.org, bp@alien8.de, mingo@redhat.com, dave.hansen@linux.intel.com, hpa@zytor.com, dave.martin@arm.com, james.morse@arm.com, fenghuay@nvidia.com, babu.moger@amd.com, anil.keshavamurthy@broadcom.com Subject: Re: [PATCH v2 1/6] x86/resctrl: Parse ACPI ERDT table and map RMDD domains by L3 cache ID In-Reply-To: <1ee53d0e75ec45e29fc8a72ee8f61f4ba7825a18.1780587063.git.yu.c.chen@intel.com> References: <1ee53d0e75ec45e29fc8a72ee8f61f4ba7825a18.1780587063.git.yu.c.chen@intel.com> Date: Thu, 04 Jun 2026 18:56:34 +0200 Message-ID: <871pem5jnh.ffs@fw13> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain 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