platform/x86/amd/hsmp: mark hsmp_msg_desc_table[] as maybe_unused

[PATCH] platform/x86/amd/hsmp: mark hsmp_msg_desc_table[] as maybe_unused

Posted by Arnd Bergmann 3 weeks, 6 days ago

From: Arnd Bergmann <arnd@arndb.de>

After the file got split, there are now W=1 warnings for users that
include it without referencing hsmp_msg_desc_table:

In file included from arch/x86/include/asm/amd_hsmp.h:6,
                 from drivers/platform/x86/amd/hsmp/plat.c:12:
arch/x86/include/uapi/asm/amd_hsmp.h:91:35: error: 'hsmp_msg_desc_table' defined but not used [-Werror=unused-const-variable=]
   91 | static const struct hsmp_msg_desc hsmp_msg_desc_table[] = {
      |                                   ^~~~~~~~~~~~~~~~~~~

Mark it as __attribute__((maybe_unused)) to shut up the warning but
keep it in the file in case it is used from userland. The __maybe_unused
shorthand unfurtunately isn't available in userspace, so this has to
be the long form.

Fixes: e47c018a0ee6 ("platform/x86/amd/hsmp: Move platform device specific code to plat.c")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
Ideally this array wouldn't be part of the UAPI at all, since it is
not really a interface, but it's hard to know what part  of the header
is actually used outside of the kernel.
---
 arch/x86/include/uapi/asm/amd_hsmp.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/uapi/asm/amd_hsmp.h b/arch/x86/include/uapi/asm/amd_hsmp.h
index e5d182c7373c..4a7cace06204 100644
--- a/arch/x86/include/uapi/asm/amd_hsmp.h
+++ b/arch/x86/include/uapi/asm/amd_hsmp.h
@@ -88,7 +88,8 @@ struct hsmp_msg_desc {
  *
  * Not supported messages would return -ENOMSG.
  */
-static const struct hsmp_msg_desc hsmp_msg_desc_table[] = {
+static const struct hsmp_msg_desc hsmp_msg_desc_table[]
+				__attribute__((unused)) = {
 	/* RESERVED */
 	{0, 0, HSMP_RSVD},
 
-- 
2.39.5

Re: [PATCH] platform/x86/amd/hsmp: mark hsmp_msg_desc_table[] as maybe_unused

Posted by Ilpo Järvinen 3 weeks, 5 days ago

On Mon, 28 Oct 2024, Arnd Bergmann wrote:

+ Hans

> From: Arnd Bergmann <arnd@arndb.de>
>
> After the file got split, there are now W=1 warnings for users that
> include it without referencing hsmp_msg_desc_table:
> 
> In file included from arch/x86/include/asm/amd_hsmp.h:6,
>                  from drivers/platform/x86/amd/hsmp/plat.c:12:
> arch/x86/include/uapi/asm/amd_hsmp.h:91:35: error: 'hsmp_msg_desc_table' defined but not used [-Werror=unused-const-variable=]
>    91 | static const struct hsmp_msg_desc hsmp_msg_desc_table[] = {
>       |                                   ^~~~~~~~~~~~~~~~~~~
> 
> Mark it as __attribute__((maybe_unused)) to shut up the warning but
> keep it in the file in case it is used from userland. The __maybe_unused
> shorthand unfurtunately isn't available in userspace, so this has to

unfortunately

> be the long form.
> 
> Fixes: e47c018a0ee6 ("platform/x86/amd/hsmp: Move platform device specific code to plat.c")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> Ideally this array wouldn't be part of the UAPI at all, since it is
> not really a interface, but it's hard to know what part  of the header
> is actually used outside of the kernel.

Sadly this slipped through during review even if it was brought up by 
somebody back then. The (rather weak) reasoning for having it as a part of 
UAPI was seemingly accepted uncontested :-(.

> ---
>  arch/x86/include/uapi/asm/amd_hsmp.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/uapi/asm/amd_hsmp.h b/arch/x86/include/uapi/asm/amd_hsmp.h
> index e5d182c7373c..4a7cace06204 100644
> --- a/arch/x86/include/uapi/asm/amd_hsmp.h
> +++ b/arch/x86/include/uapi/asm/amd_hsmp.h
> @@ -88,7 +88,8 @@ struct hsmp_msg_desc {
>   *
>   * Not supported messages would return -ENOMSG.
>   */
> -static const struct hsmp_msg_desc hsmp_msg_desc_table[] = {
> +static const struct hsmp_msg_desc hsmp_msg_desc_table[]
> +				__attribute__((unused)) = {

It seems that the main goal why it was put into UAPI was "to give the user 
some reference about proper num_args and response_size for each message":

https://lore.kernel.org/all/CAPhsuW5V0BJT+YSwv1U=hRG0k9zBWXeRd=E1n4U5hvcnwEV3mQ@mail.gmail.com/

Are we actually expecting userspace to benefit from this in C form?
Suma? Hans?

>  	/* RESERVED */
>  	{0, 0, HSMP_RSVD},


-- 
 i.

Re: [PATCH] platform/x86/amd/hsmp: mark hsmp_msg_desc_table[] as maybe_unused

Posted by Hans de Goede 3 weeks, 4 days ago

Hi,

On 29-Oct-24 1:55 PM, Ilpo Järvinen wrote:
> On Mon, 28 Oct 2024, Arnd Bergmann wrote:
> 
> + Hans
> 
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> After the file got split, there are now W=1 warnings for users that
>> include it without referencing hsmp_msg_desc_table:
>>
>> In file included from arch/x86/include/asm/amd_hsmp.h:6,
>>                  from drivers/platform/x86/amd/hsmp/plat.c:12:
>> arch/x86/include/uapi/asm/amd_hsmp.h:91:35: error: 'hsmp_msg_desc_table' defined but not used [-Werror=unused-const-variable=]
>>    91 | static const struct hsmp_msg_desc hsmp_msg_desc_table[] = {
>>       |                                   ^~~~~~~~~~~~~~~~~~~
>>
>> Mark it as __attribute__((maybe_unused)) to shut up the warning but
>> keep it in the file in case it is used from userland. The __maybe_unused
>> shorthand unfurtunately isn't available in userspace, so this has to
> 
> unfortunately
> 
>> be the long form.
>>
>> Fixes: e47c018a0ee6 ("platform/x86/amd/hsmp: Move platform device specific code to plat.c")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>> Ideally this array wouldn't be part of the UAPI at all, since it is
>> not really a interface, but it's hard to know what part  of the header
>> is actually used outside of the kernel.
> 
> Sadly this slipped through during review even if it was brought up by 
> somebody back then. The (rather weak) reasoning for having it as a part of 
> UAPI was seemingly accepted uncontested :-(.
> 
>> ---
>>  arch/x86/include/uapi/asm/amd_hsmp.h | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/uapi/asm/amd_hsmp.h b/arch/x86/include/uapi/asm/amd_hsmp.h
>> index e5d182c7373c..4a7cace06204 100644
>> --- a/arch/x86/include/uapi/asm/amd_hsmp.h
>> +++ b/arch/x86/include/uapi/asm/amd_hsmp.h
>> @@ -88,7 +88,8 @@ struct hsmp_msg_desc {
>>   *
>>   * Not supported messages would return -ENOMSG.
>>   */
>> -static const struct hsmp_msg_desc hsmp_msg_desc_table[] = {
>> +static const struct hsmp_msg_desc hsmp_msg_desc_table[]
>> +				__attribute__((unused)) = {
> 
> It seems that the main goal why it was put into UAPI was "to give the user 
> some reference about proper num_args and response_size for each message":
> 
> https://lore.kernel.org/all/CAPhsuW5V0BJT+YSwv1U=hRG0k9zBWXeRd=E1n4U5hvcnwEV3mQ@mail.gmail.com/
> 
> Are we actually expecting userspace to benefit from this in C form?
> Suma? Hans?

I can see how having this available in the uapi header as documentation
of sorts is somewhat useful.

OTOH I do agree that this array should probably not be used by userspace.

And there is only 1 way to find out if it is actually used (which I do not
expect) and that is to just drop it and find out (and to be willing to
revert the change if it breaks things).

So we can either move the array in its entirety to the c-code consuming it,
which I think would be best; or we can go with Arnd's patch + add

#ifdef __KERNEL__ 

around the array so that it is there for people reading the header, but
it is no longer exposed as uapi.

Regards,

Hans






> 
>>  	/* RESERVED */
>>  	{0, 0, HSMP_RSVD},
> 
>

Re: [PATCH] platform/x86/amd/hsmp: mark hsmp_msg_desc_table[] as maybe_unused

Posted by Arnd Bergmann 3 weeks, 4 days ago

On Wed, Oct 30, 2024, at 14:16, Hans de Goede wrote:
> On 29-Oct-24 1:55 PM, Ilpo Järvinen wrote:
>> On Mon, 28 Oct 2024, Arnd Bergmann wrote:
>> 
>> It seems that the main goal why it was put into UAPI was "to give the user 
>> some reference about proper num_args and response_size for each message":
>> 
>> https://lore.kernel.org/all/CAPhsuW5V0BJT+YSwv1U=hRG0k9zBWXeRd=E1n4U5hvcnwEV3mQ@mail.gmail.com/
>> 
>> Are we actually expecting userspace to benefit from this in C form?
>> Suma? Hans?
>
> I can see how having this available in the uapi header as documentation
> of sorts is somewhat useful.
>
> OTOH I do agree that this array should probably not be used by userspace.
>
> And there is only 1 way to find out if it is actually used (which I do not
> expect) and that is to just drop it and find out (and to be willing to
> revert the change if it breaks things).
>
> So we can either move the array in its entirety to the c-code consuming it,
> which I think would be best; or we can go with Arnd's patch + add
>
> #ifdef __KERNEL__ 
>
> around the array so that it is there for people reading the header, but
> it is no longer exposed as uapi.
>

I don't think that would work, because the 'make headers_install'
step just removes the contents of #ifdef __KERNEL__, in this case
you just end up with a uapi header that doesn't have the array.

      Arnd

Re: [PATCH] platform/x86/amd/hsmp: mark hsmp_msg_desc_table[] as maybe_unused

Posted by Hans de Goede 3 weeks, 4 days ago

Hi,

On 30-Oct-24 3:19 PM, Arnd Bergmann wrote:
> On Wed, Oct 30, 2024, at 14:16, Hans de Goede wrote:
>> On 29-Oct-24 1:55 PM, Ilpo Järvinen wrote:
>>> On Mon, 28 Oct 2024, Arnd Bergmann wrote:
>>>
>>> It seems that the main goal why it was put into UAPI was "to give the user 
>>> some reference about proper num_args and response_size for each message":
>>>
>>> https://lore.kernel.org/all/CAPhsuW5V0BJT+YSwv1U=hRG0k9zBWXeRd=E1n4U5hvcnwEV3mQ@mail.gmail.com/
>>>
>>> Are we actually expecting userspace to benefit from this in C form?
>>> Suma? Hans?
>>
>> I can see how having this available in the uapi header as documentation
>> of sorts is somewhat useful.
>>
>> OTOH I do agree that this array should probably not be used by userspace.
>>
>> And there is only 1 way to find out if it is actually used (which I do not
>> expect) and that is to just drop it and find out (and to be willing to
>> revert the change if it breaks things).
>>
>> So we can either move the array in its entirety to the c-code consuming it,
>> which I think would be best; or we can go with Arnd's patch + add
>>
>> #ifdef __KERNEL__ 
>>
>> around the array so that it is there for people reading the header, but
>> it is no longer exposed as uapi.
>>
> 
> I don't think that would work, because the 'make headers_install'
> step just removes the contents of #ifdef __KERNEL__, in this case
> you just end up with a uapi header that doesn't have the array.

Ah I did not realize that.

Ok so lets just move the definition to the .c file which actually
uses the array and then add a comment like this:

/*
 * See hsmp_msg_desc_table[] in:
 * https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/amd/hsmp.c
 * 
 * for some information on number of input- and output arguments
 * for the various functions.
 *
 * Please find the supported list of messages and message definition
 * in the HSMP chapter of respective family/model PPR.
 */

Note please replace hsmp.c with the .c file to which the array
is actually moving ...

Regards,

Hans