Re: [PATCH v2] x86/amd_nb: unexport amd_cache_northbridges()

From: Chatradhi, Naveen Krishna
Date: Mon Mar 28 2022 - 13:21:34 EST

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@xxxxxxx>

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@xxxxxxx>
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@xxxxxxx>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@xxxxxxx>
Signed-off-by: Borislav Petkov <bp@xxxxxxx>
Link: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fr%2F20220324122729.221765-1-nchatrad%40amd.com&amp;data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7Cad77d5aa72554663e79d08da0dca506b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637837460434170931%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=MsZNFF0%2F5x%2BJkuOPtL%2BNFZRxtBe548sVkEbyHOUcTcQ%3D&amp;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&amp;data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7Cad77d5aa72554663e79d08da0dca506b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637837460434170931%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=JAgdd9bpTtNaF2UG9seKB%2FLI3pF5wrWk4Sj1JurN860%3D&amp;reserved=0