[PATCH] platform/x86/amd/hsmp: fix compile-testing without CONFiG_AMD_NB

[PATCH] platform/x86/amd/hsmp: fix compile-testing without CONFiG_AMD_NB

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

node_to_amd_nb() is defined to NULL in non-AMD configs:

drivers/platform/x86/amd/hsmp/plat.c: In function 'init_platform_device':
drivers/platform/x86/amd/hsmp/plat.c:165:68: error: dereferencing 'void *' pointer [-Werror]
  165 |                 sock->root                      = node_to_amd_nb(i)->root;
      |                                                                    ^~
drivers/platform/x86/amd/hsmp/plat.c:165:68: error: request for member 'root' in something not a structure or union

Change the definition to something that builds. This does introduce a
NULL pointer dereference but the code is never called since the driver
won't probe successfully.

Fixes: 7d3135d16356 ("platform/x86/amd/hsmp: Create separate ACPI, plat and common drivers")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 arch/x86/include/asm/amd_nb.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h
index 6f3b6aef47ba..d0caac26533f 100644
--- a/arch/x86/include/asm/amd_nb.h
+++ b/arch/x86/include/asm/amd_nb.h
@@ -116,7 +116,10 @@ static inline bool amd_gart_present(void)
 
 #define amd_nb_num(x)		0
 #define amd_nb_has_feature(x)	false
-#define node_to_amd_nb(x)	NULL
+static inline struct amd_northbridge *node_to_amd_nb(int node)
+{
+	return NULL;
+}
 #define amd_gart_present(x)	false
 
 #endif
-- 
2.39.5

Re: [PATCH] platform/x86/amd/hsmp: fix compile-testing without CONFiG_AMD_NB

[Borislav Petkov]

On Tue, Oct 29, 2024 at 09:23:20AM +0000, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> node_to_amd_nb() is defined to NULL in non-AMD configs:
> 
> drivers/platform/x86/amd/hsmp/plat.c: In function 'init_platform_device':
> drivers/platform/x86/amd/hsmp/plat.c:165:68: error: dereferencing 'void *' pointer [-Werror]
>   165 |                 sock->root                      = node_to_amd_nb(i)->root;
>       |                                                                    ^~
> drivers/platform/x86/amd/hsmp/plat.c:165:68: error: request for member 'root' in something not a structure or union
> 
> Change the definition to something that builds. This does introduce a
> NULL pointer dereference but the code is never called since the driver
> won't probe successfully.
> 
> Fixes: 7d3135d16356 ("platform/x86/amd/hsmp: Create separate ACPI, plat and common drivers")
         ^^^^^^^^^^^^

I'm guessing this is queued for 6.13...

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  arch/x86/include/asm/amd_nb.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h
> index 6f3b6aef47ba..d0caac26533f 100644
> --- a/arch/x86/include/asm/amd_nb.h
> +++ b/arch/x86/include/asm/amd_nb.h
> @@ -116,7 +116,10 @@ static inline bool amd_gart_present(void)
>  
>  #define amd_nb_num(x)		0
>  #define amd_nb_has_feature(x)	false
> -#define node_to_amd_nb(x)	NULL
> +static inline struct amd_northbridge *node_to_amd_nb(int node)
> +{
> +	return NULL;
> +}
>  #define amd_gart_present(x)	false

... so this fix should go to Linus now so that build testing doesn't break?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] platform/x86/amd/hsmp: fix compile-testing without CONFiG_AMD_NB

[Arnd Bergmann]

On Tue, Oct 29, 2024, at 10:33, Borislav Petkov wrote:
> On Tue, Oct 29, 2024 at 09:23:20AM +0000, Arnd Bergmann wrote:
>>  
>>  #define amd_nb_num(x)		0
>>  #define amd_nb_has_feature(x)	false
>> -#define node_to_amd_nb(x)	NULL
>> +static inline struct amd_northbridge *node_to_amd_nb(int node)
>> +{
>> +	return NULL;
>> +}
>>  #define amd_gart_present(x)	false
>
> ... so this fix should go to Linus now so that build testing doesn't break?
>

That would work, or Ilpo can pick it into the tree that
has the driver changes, possibly folding the fix into the
other changes.

     Arnd

Re: [PATCH] platform/x86/amd/hsmp: fix compile-testing without CONFiG_AMD_NB

[Borislav Petkov]

On Tue, Oct 29, 2024 at 10:56:12AM +0000, Arnd Bergmann wrote:
> That would work, or Ilpo can pick it into the tree that
> has the driver changes, possibly folding the fix into the
> other changes.

Right, while it shouldn't be a problem, I'd like to keep all amd_nb changes in
one tree because we have stuff in-flight touching and changing exactly that
area so...

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] platform/x86/amd/hsmp: fix compile-testing without CONFiG_AMD_NB

[Ilpo Järvinen]

On Tue, 29 Oct 2024, Borislav Petkov wrote:

> On Tue, Oct 29, 2024 at 10:56:12AM +0000, Arnd Bergmann wrote:
> > That would work, or Ilpo can pick it into the tree that
> > has the driver changes, possibly folding the fix into the
> > other changes.
> 
> Right, while it shouldn't be a problem, I'd like to keep all amd_nb changes in
> one tree because we have stuff in-flight touching and changing exactly that
> area so...

Hi Boris,

I'm unfortunately left a bit unsure what exactly is your suggestion here, 
so could you please elaborate?

(I'm assuming the big amd_nb series will go through the x86 tree.)

Thanks in advance.

-- 
 i.

Re: [PATCH] platform/x86/amd/hsmp: fix compile-testing without CONFiG_AMD_NB

[Borislav Petkov]

Hi,

On Tue, Oct 29, 2024 at 02:40:00PM +0200, Ilpo Järvinen wrote:
> I'm unfortunately left a bit unsure what exactly is your suggestion here, 
> so could you please elaborate?

My suggestion is, I send it to Linus now so it appears in 6.12-rc6 and thus
the build error is gone in every other tree.

> (I'm assuming the big amd_nb series will go through the x86 tree.)

Yeah, eventually. Not now but see above: if the fix appears in mainline now,
no problems.

Right?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] platform/x86/amd/hsmp: fix compile-testing without CONFiG_AMD_NB

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

On Tue, 29 Oct 2024, Borislav Petkov wrote:
> On Tue, Oct 29, 2024 at 02:40:00PM +0200, Ilpo Järvinen wrote:
> > I'm unfortunately left a bit unsure what exactly is your suggestion here, 
> > so could you please elaborate?
> 
> My suggestion is, I send it to Linus now so it appears in 6.12-rc6 and thus
> the build error is gone in every other tree.
> 
> > (I'm assuming the big amd_nb series will go through the x86 tree.)
> 
> Yeah, eventually. Not now but see above: if the fix appears in mainline now,
> no problems.
> 
> Right?

Yes, I'm fine with you sending it to Linus in this cycle. Thanks.

-- 
 i.

Re: [PATCH] platform/x86/amd/hsmp: fix compile-testing without CONFiG_AMD_NB

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

On Tue, 29 Oct 2024, Arnd Bergmann wrote:

> From: Arnd Bergmann <arnd@arndb.de>
> 
> node_to_amd_nb() is defined to NULL in non-AMD configs:
> 
> drivers/platform/x86/amd/hsmp/plat.c: In function 'init_platform_device':
> drivers/platform/x86/amd/hsmp/plat.c:165:68: error: dereferencing 'void *' pointer [-Werror]
>   165 |                 sock->root                      = node_to_amd_nb(i)->root;
>       |                                                                    ^~
> drivers/platform/x86/amd/hsmp/plat.c:165:68: error: request for member 'root' in something not a structure or union
> 
> Change the definition to something that builds. This does introduce a
> NULL pointer dereference but the code is never called since the driver
> won't probe successfully.

I don't like this very wording because what the code very much does is 
NULL check on node_to_amd_nb() which leads to immediate failure of 
.probe(). (We don't call other deferences after a NULL check "NULL pointer 
dereference" either so none is introduced by this patch, IMO.)

With that fixed,

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.

> Fixes: 7d3135d16356 ("platform/x86/amd/hsmp: Create separate ACPI, plat and common drivers")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  arch/x86/include/asm/amd_nb.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h
> index 6f3b6aef47ba..d0caac26533f 100644
> --- a/arch/x86/include/asm/amd_nb.h
> +++ b/arch/x86/include/asm/amd_nb.h
> @@ -116,7 +116,10 @@ static inline bool amd_gart_present(void)
>  
>  #define amd_nb_num(x)		0
>  #define amd_nb_has_feature(x)	false
> -#define node_to_amd_nb(x)	NULL
> +static inline struct amd_northbridge *node_to_amd_nb(int node)
> +{
> +	return NULL;
> +}
>  #define amd_gart_present(x)	false
>  
>  #endif
> 

Re: [PATCH] platform/x86/amd/hsmp: fix compile-testing without CONFiG_AMD_NB

[Borislav Petkov]

On Tue, Oct 29, 2024 at 02:39:41PM +0200, Ilpo Järvinen wrote:
> I don't like this very wording because what the code very much does is 
> NULL check on node_to_amd_nb() which leads to immediate failure of 
> .probe(). (We don't call other deferences after a NULL check "NULL pointer 
> dereference" either so none is introduced by this patch, IMO.)

I was wondering that too: where does this line

	 sock->root                      = node_to_amd_nb(i)->root;

quoted by gcc come from?

IOW, what is the correct Fixes: tag?

The commit 7d3135d16356 ("platform/x86/amd/hsmp: Create separate ACPI, plat and common drivers

is only in next AFAICT, so I'll drop the Fixes: tag when sending...

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] platform/x86/amd/hsmp: fix compile-testing without CONFiG_AMD_NB

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

On Tue, 29 Oct 2024, Borislav Petkov wrote:

> On Tue, Oct 29, 2024 at 02:39:41PM +0200, Ilpo Järvinen wrote:
> > I don't like this very wording because what the code very much does is 
> > NULL check on node_to_amd_nb() which leads to immediate failure of 
> > .probe(). (We don't call other deferences after a NULL check "NULL pointer 
> > dereference" either so none is introduced by this patch, IMO.)
> 
> I was wondering that too: where does this line
> 
> 	 sock->root                      = node_to_amd_nb(i)->root;
> 
> quoted by gcc come from?
>
> IOW, what is the correct Fixes: tag?
> 
> The commit 7d3135d16356 ("platform/x86/amd/hsmp: Create separate ACPI, plat and common drivers
> 
> is only in next AFAICT, so I'll drop the Fixes: tag when sending...

To clarify,

The assignment line is old (from 287a821c76be8 or even before that in 
some form which would have not lead to compiler warning though).

It's the COMPILE_TEST that got enabled in 7d3135d16356, before that hsmp 
depended on AMD_NB so the condition could never trigger.

-- 
 i.

Re: [PATCH] platform/x86/amd/hsmp: fix compile-testing without CONFiG_AMD_NB

[Borislav Petkov]

On Tue, Oct 29, 2024 at 05:19:30PM +0200, Ilpo Järvinen wrote:
> It's the COMPILE_TEST that got enabled in 7d3135d16356, before that hsmp 
> depended on AMD_NB so the condition could never trigger.

This COMPILE_TEST thing is only causing more trouble than it is solving... :-\

Lemme fixup Arnd's commit message and queue it.

Thx.

-- 
Regards/Gruss,
    Boris.

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

[tip: x86/urgent] x86/amd_nb: Fix compile-testing without CONFIG_AMD_NB

[tip-bot2 for Arnd Bergmann]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     fce9642c765a18abd1db0339a7d832c29b68456a
Gitweb:        https://git.kernel.org/tip/fce9642c765a18abd1db0339a7d832c29b68456a
Author:        Arnd Bergmann <arnd@arndb.de>
AuthorDate:    Tue, 29 Oct 2024 09:23:20 
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Tue, 29 Oct 2024 18:16:05 +01:00

x86/amd_nb: Fix compile-testing without CONFIG_AMD_NB

node_to_amd_nb() is defined to NULL in non-AMD configs:

  drivers/platform/x86/amd/hsmp/plat.c: In function 'init_platform_device':
  drivers/platform/x86/amd/hsmp/plat.c:165:68: error: dereferencing 'void *' pointer [-Werror]
    165 |                 sock->root                      = node_to_amd_nb(i)->root;
        |                                                                    ^~
  drivers/platform/x86/amd/hsmp/plat.c:165:68: error: request for member 'root' in something not a structure or union

Users of the interface who also allow COMPILE_TEST will cause the above build
error so provide an inline stub to fix that.

  [ bp: Massage commit message. ]

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Link: https://lore.kernel.org/r/20241029092329.3857004-1-arnd@kernel.org
---
 arch/x86/include/asm/amd_nb.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h
index 6f3b6ae..d0caac2 100644
--- a/arch/x86/include/asm/amd_nb.h
+++ b/arch/x86/include/asm/amd_nb.h
@@ -116,7 +116,10 @@ static inline bool amd_gart_present(void)
 
 #define amd_nb_num(x)		0
 #define amd_nb_has_feature(x)	false
-#define node_to_amd_nb(x)	NULL
+static inline struct amd_northbridge *node_to_amd_nb(int node)
+{
+	return NULL;
+}
 #define amd_gart_present(x)	false
 
 #endif