| Message ID | 20220228161154.54539-1-nchatrad@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] x86/amd_nb: unexport amd_cache_northbridges() | expand |
On Mon, Feb 28, 2022 at 09:41:54PM +0530, Naveen Krishna Chatradhi wrote: > From: Muralidhara M K <muralimk@amd.com> > > amd_cache_northbridges() is called from init_amd_nbs(), during > fs_initcall() and need not be called explicitly. Kernel components > can directly call amd_nb_num() to get the initialized number of > north bridges. > > unexport amd_cache_northbridges(), update dependent modules to > call amd_nb_num() instead. While at it, simplify the while checks > in amd_cache_northbridges(). What I am missing in this commit message is why is it ok to do that? AFAIR, previously, amd_cache_northbridges() wasn't an initcall so the module or builtin - which came first - was forcing the NB caching through the explicit call to amd_cache_northbridges(). fs_inicall() does that now unconditionally so the question is, why can the module init functions assume that the northbridges have been cached already and can simply get the NB number? Thx.
Hi Boris, On 3/15/2022 10:58 PM, Borislav Petkov wrote: > [CAUTION: External Email] > > On Mon, Feb 28, 2022 at 09:41:54PM +0530, Naveen Krishna Chatradhi wrote: >> From: Muralidhara M K <muralimk@amd.com> >> >> amd_cache_northbridges() is called from init_amd_nbs(), during >> fs_initcall() and need not be called explicitly. Kernel components >> can directly call amd_nb_num() to get the initialized number of >> north bridges. >> >> unexport amd_cache_northbridges(), update dependent modules to >> call amd_nb_num() instead. While at it, simplify the while checks >> in amd_cache_northbridges(). Apologies for the late reply. > What I am missing in this commit message is why is it ok to do that? > > AFAIR, previously, amd_cache_northbridges() wasn't an initcall so the > module or builtin - which came first - was forcing the NB caching > through the explicit call to amd_cache_northbridges(). > > fs_inicall() does that now unconditionally so the question is, why can > the module init functions assume that the northbridges have been cached > already and can simply get the NB number? Modules that are using this API comes after the fs_initcall() is called. This patch is prepared based on a previous discussion https://lore.kernel.org/lkml/bcf5e86c-d3f1-0dab-2bed-505b1eb95f17@amd.com/ quoting from the thread > I'm going to venture a pretty sure guess that the initcall runs first > and that amd_cache_northbridges() call in amd64_edac_init() is probably > not even needed anymore... and i've confirmed the call flow. > > Thx. > > -- > Regards/Gruss, > Boris. > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette%3Awq&data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7Cf178536c5a264e5bd9ca08da06a98c6f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637829622545040298%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=iRnHksY7p4nd4pALbDNp11Y7%2FSMAou9X0XrV%2FC7K8Xk%3D&reserved=0 Regards, Naveenk
On Wed, Mar 23, 2022 at 08:54:20PM +0530, Chatradhi, Naveen Krishna wrote:
> Modules that are using this API comes after the fs_initcall() is called.
I don't need you to explain this to me - I need you to:
"What I am missing in this commit message is why is it ok to do that?"
I.e., pls explain in the commit message exactly why it is ok to do that
so that people who read it, will know.
Thx.
On 3/23/2022 9:01 PM, Borislav Petkov wrote: > [CAUTION: External Email] > > On Wed, Mar 23, 2022 at 08:54:20PM +0530, Chatradhi, Naveen Krishna wrote: >> Modules that are using this API comes after the fs_initcall() is called. > I don't need you to explain this to me - I need you to: > > "What I am missing in this commit message is why is it ok to do that?" > > I.e., pls explain in the commit message exactly why it is ok to do that > so that people who read it, will know. Got it, thanks. I will update the commit message. > > Thx. > > -- > Regards/Gruss, > Boris. > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7Cb0cc43042e1d4b9cdb7208da0ce2419b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637836463172288920%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=v4zF2o2W33F1rCEg6e0ZZRRqpvxM4BwkvtGSgGSY5cU%3D&reserved=0
diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h index 00d1a400b7a1..ed0eaf65c437 100644 --- a/arch/x86/include/asm/amd_nb.h +++ b/arch/x86/include/asm/amd_nb.h @@ -16,7 +16,6 @@ extern const struct amd_nb_bus_dev_range amd_nb_bus_dev_ranges[]; extern bool early_is_amd_nb(u32 value); extern struct resource *amd_get_mmconfig_range(struct resource *res); -extern int amd_cache_northbridges(void); extern void amd_flush_garts(void); extern int amd_numa_init(void); extern int amd_get_subcaches(int); diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c index 020c906f7934..190e0f763375 100644 --- a/arch/x86/kernel/amd_nb.c +++ b/arch/x86/kernel/amd_nb.c @@ -188,7 +188,7 @@ int amd_smn_write(u16 node, u32 address, u32 value) EXPORT_SYMBOL_GPL(amd_smn_write); -int amd_cache_northbridges(void) +static int amd_cache_northbridges(void) { const struct pci_device_id *misc_ids = amd_nb_misc_ids; const struct pci_device_id *link_ids = amd_nb_link_ids; @@ -210,14 +210,14 @@ int amd_cache_northbridges(void) } misc = NULL; - while ((misc = next_northbridge(misc, misc_ids)) != NULL) + while ((misc = next_northbridge(misc, misc_ids))) misc_count++; if (!misc_count) return -ENODEV; root = NULL; - while ((root = next_northbridge(root, root_ids)) != NULL) + while ((root = next_northbridge(root, root_ids))) root_count++; if (root_count) { @@ -290,7 +290,6 @@ int amd_cache_northbridges(void) return 0; } -EXPORT_SYMBOL_GPL(amd_cache_northbridges); /* * Ignores subdevice/subvendor but as far as I can figure out diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c index dc78a4fb879e..84a4aa9312cf 100644 --- a/drivers/char/agp/amd64-agp.c +++ b/drivers/char/agp/amd64-agp.c @@ -327,7 +327,7 @@ static int cache_nbs(struct pci_dev *pdev, u32 cap_ptr) { int i; - if (amd_cache_northbridges() < 0) + if (!amd_nb_num()) return -ENODEV; if (!amd_nb_has_feature(AMD_NB_GART)) diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c index fba609ada0e6..af2c578f8ab3 100644 --- a/drivers/edac/amd64_edac.c +++ b/drivers/edac/amd64_edac.c @@ -4269,7 +4269,7 @@ static int __init amd64_edac_init(void) if (!x86_match_cpu(amd64_cpuids)) return -ENODEV; - if (amd_cache_northbridges() < 0) + if (!amd_nb_num()) return -ENODEV; opstate_init();