[PATCH 1/1] x86/amd_nb: unexport amd_cache_northbridges()

[PATCH 1/1] x86/amd_nb: unexport amd_cache_northbridges()

[Naveen Krishna Chatradhi]

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

Signed-off-by: Muralidhara M K <muralimk@amd.com>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
Cc: David Airlie <airlied@linux.ie>
---
 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.25.1

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

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette:wq

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

[Chatradhi, Naveen Krishna]

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&amp;data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7Cf178536c5a264e5bd9ca08da06a98c6f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637829622545040298%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=iRnHksY7p4nd4pALbDNp11Y7%2FSMAou9X0XrV%2FC7K8Xk%3D&amp;reserved=0

Regards,

Naveenk

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

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Chatradhi, Naveen Krishna]

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