| Message ID | 20220324122729.221765-1-nchatrad@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] x86/amd_nb: unexport amd_cache_northbridges() | expand |
On Thu, Mar 24, 2022 at 05:57:29PM +0530, Naveen Krishna Chatradhi wrote: > From: Muralidhara M K <muralimk@amd.com> > > amd_cache_northbridges() is exported by amd_nb.c and is used by > amd64-agp.c and amd64_edac.c modules. > > init_amd_nbs() already calls amd_cache_northbridges() unconditionally, > during fs_initcall() phase, which happens before the device_initcall(). No, that's not even trying. I went and did your work for you. Please try harder in the future to really really explain why you're doing what you're doing so that a reader of your commit message can easily follow your logic and not have to do research just to figure out why your change is ok. --- From f5e82ad4c749afb63cdebba6729452e516bc1fa9 Mon Sep 17 00:00:00 2001 From: Muralidhara M K <muralimk@amd.com> Date: Thu, 24 Mar 2022 17:57:29 +0530 Subject: [PATCH] x86/amd_nb: Unexport amd_cache_northbridges() amd_cache_northbridges() is exported by amd_nb.c and is called by amd64-agp.c and amd64_edac.c modules at module_init() time so that NB descriptors are properly cached before those drivers can use them. However, the init_amd_nbs() initcall already does call amd_cache_northbridges() unconditionally and thus makes sure the NB descriptors are enumerated. That initcall is a fs_initcall type which is on the 5th group (starting from 0) of initcalls that gets run in increasing numerical order by the init code. The module_init() call is turned into an __initcall() in the MODULE=n case and those are device-level initcalls, i.e., group 6. Therefore, the northbridges caching is already finished by the time module initialization starts and thus the correct initialization order is retained. Unexport amd_cache_northbridges(), update dependent modules to call amd_nb_num() instead. While at it, simplify the checks in amd_cache_northbridges(). [ bp: Heavily massage and *actually* explain why the change is ok. ] Signed-off-by: Muralidhara M K <muralimk@amd.com> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com> Signed-off-by: Borislav Petkov <bp@suse.de> Link: https://lore.kernel.org/r/20220324122729.221765-1-nchatrad@amd.com --- arch/x86/include/asm/amd_nb.h | 1 - arch/x86/kernel/amd_nb.c | 7 +++---- drivers/char/agp/amd64-agp.c | 2 +- drivers/edac/amd64_edac.c | 2 +- 4 files changed, 5 insertions(+), 7 deletions(-) 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();
Hi Boris, On 3/25/2022 12:40 AM, Borislav Petkov wrote: > [CAUTION: External Email] > > On Thu, Mar 24, 2022 at 05:57:29PM +0530, Naveen Krishna Chatradhi wrote: >> From: Muralidhara M K <muralimk@amd.com> >> >> amd_cache_northbridges() is exported by amd_nb.c and is used by >> amd64-agp.c and amd64_edac.c modules. >> >> init_amd_nbs() already calls amd_cache_northbridges() unconditionally, >> during fs_initcall() phase, which happens before the device_initcall(). > No, that's not even trying. I went and did your work for you. Please > try harder in the future to really really explain why you're doing what > you're doing so that a reader of your commit message can easily follow > your logic and not have to do research just to figure out why your > change is ok. Understood, will try and explain better for future patches. Thank you > > --- > From f5e82ad4c749afb63cdebba6729452e516bc1fa9 Mon Sep 17 00:00:00 2001 > From: Muralidhara M K <muralimk@amd.com> > Date: Thu, 24 Mar 2022 17:57:29 +0530 > Subject: [PATCH] x86/amd_nb: Unexport amd_cache_northbridges() > > amd_cache_northbridges() is exported by amd_nb.c and is called by > amd64-agp.c and amd64_edac.c modules at module_init() time so that NB > descriptors are properly cached before those drivers can use them. > > However, the init_amd_nbs() initcall already does call > amd_cache_northbridges() unconditionally and thus makes sure the NB > descriptors are enumerated. > > That initcall is a fs_initcall type which is on the 5th group (starting > from 0) of initcalls that gets run in increasing numerical order by the > init code. > > The module_init() call is turned into an __initcall() in the MODULE=n > case and those are device-level initcalls, i.e., group 6. > > Therefore, the northbridges caching is already finished by the time > module initialization starts and thus the correct initialization order > is retained. > > Unexport amd_cache_northbridges(), update dependent modules to > call amd_nb_num() instead. While at it, simplify the checks in > amd_cache_northbridges(). > > [ bp: Heavily massage and *actually* explain why the change is ok. ] > > Signed-off-by: Muralidhara M K <muralimk@amd.com> > Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com> > Signed-off-by: Borislav Petkov <bp@suse.de> > Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fr%2F20220324122729.221765-1-nchatrad%40amd.com&data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7Cad77d5aa72554663e79d08da0dca506b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637837460434170931%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=MsZNFF0%2F5x%2BJkuOPtL%2BNFZRxtBe548sVkEbyHOUcTcQ%3D&reserved=0 > --- > arch/x86/include/asm/amd_nb.h | 1 - > arch/x86/kernel/amd_nb.c | 7 +++---- > drivers/char/agp/amd64-agp.c | 2 +- > drivers/edac/amd64_edac.c | 2 +- > 4 files changed, 5 insertions(+), 7 deletions(-) > > 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(); > -- > 2.35.1 > > -- > 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%7Cad77d5aa72554663e79d08da0dca506b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637837460434170931%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=JAgdd9bpTtNaF2UG9seKB%2FLI3pF5wrWk4Sj1JurN860%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();