[PATCH v2] platform/x86/amd/hsmp: Add CONFIG_ACPI dependency

[PATCH v2] platform/x86/amd/hsmp: Add CONFIG_ACPI dependency

[Suma Hegde]

HSMP interface is only supported on x86 based AMD EPYC line of
processors. Driver uses ACPI APIs, so make it dependent on CONFIG_ACPI.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202401281437.aus91srb-lkp@intel.com/
Signed-off-by: Suma Hegde <suma.hegde@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
---
Changes since v1:
Correct the email id for Naveen Krishna Chatradhi and change it as
Reviewed-by.

 drivers/platform/x86/amd/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig
index 54753213cc61..f88682d36447 100644
--- a/drivers/platform/x86/amd/Kconfig
+++ b/drivers/platform/x86/amd/Kconfig
@@ -8,7 +8,7 @@ source "drivers/platform/x86/amd/pmc/Kconfig"
 
 config AMD_HSMP
 	tristate "AMD HSMP Driver"
-	depends on AMD_NB && X86_64
+	depends on AMD_NB && X86_64 && ACPI
 	help
 	  The driver provides a way for user space tools to monitor and manage
 	  system management functionality on EPYC server CPUs from AMD.
-- 
2.25.1

[PATCH v2] platform/x86/amd/hsmp: Remove NULL dereferencing code

[Suma Hegde]

Do not log using dev_err() in case of !sock, which causes null pointer
dereferencing.

Also remove unnecessary check "boot_cpu_data.x86_model >= 0x00", which is
always true because its an unsigned type.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202401292056.qkUFS09Y-lkp@intel.com/
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202401291311.gzMCj6SP-lkp@intel.com/

Signed-off-by: Suma Hegde <suma.hegde@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
---
Changes since v1:
Correct the email id for Naveen Krishna Chatradhi.

 drivers/platform/x86/amd/hsmp.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
index 1baddf403920..1927be901108 100644
--- a/drivers/platform/x86/amd/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp.c
@@ -566,17 +566,15 @@ static ssize_t hsmp_metric_tbl_read(struct file *filp, struct kobject *kobj,
 	struct hsmp_message msg = { 0 };
 	int ret;
 
+	if (!sock)
+		return -EINVAL;
+
 	/* Do not support lseek(), reads entire metric table */
 	if (count < bin_attr->size) {
 		dev_err(sock->dev, "Wrong buffer size\n");
 		return -EINVAL;
 	}
 
-	if (!sock) {
-		dev_err(sock->dev, "Failed to read attribute private data\n");
-		return -EINVAL;
-	}
-
 	msg.msg_id	= HSMP_GET_METRIC_TABLE;
 	msg.sock_ind	= sock->sock_ind;
 
@@ -739,8 +737,7 @@ static int hsmp_cache_proto_ver(u16 sock_ind)
 
 static inline bool is_f1a_m0h(void)
 {
-	if (boot_cpu_data.x86 == 0x1A &&
-	    (boot_cpu_data.x86_model >= 0x00 && boot_cpu_data.x86_model <= 0x0F))
+	if (boot_cpu_data.x86 == 0x1A && boot_cpu_data.x86_model <= 0x0F)
 		return true;
 
 	return false;
-- 
2.25.1

Re: [PATCH v2] platform/x86/amd/hsmp: Remove NULL dereferencing code

[Hans de Goede]

Hi Suma,

On 1/30/24 08:34, Suma Hegde wrote:
> Do not log using dev_err() in case of !sock, which causes null pointer
> dereferencing.
> 
> Also remove unnecessary check "boot_cpu_data.x86_model >= 0x00", which is
> always true because its an unsigned type.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202401292056.qkUFS09Y-lkp@intel.com/
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202401291311.gzMCj6SP-lkp@intel.com/
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> ---
> Changes since v1:
> Correct the email id for Naveen Krishna Chatradhi.
> 
>  drivers/platform/x86/amd/hsmp.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
> index 1baddf403920..1927be901108 100644
> --- a/drivers/platform/x86/amd/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp.c
> @@ -566,17 +566,15 @@ static ssize_t hsmp_metric_tbl_read(struct file *filp, struct kobject *kobj,
>  	struct hsmp_message msg = { 0 };
>  	int ret;
>  
> +	if (!sock)
> +		return -EINVAL;
> +
>  	/* Do not support lseek(), reads entire metric table */
>  	if (count < bin_attr->size) {
>  		dev_err(sock->dev, "Wrong buffer size\n");
>  		return -EINVAL;
>  	}
>  
> -	if (!sock) {
> -		dev_err(sock->dev, "Failed to read attribute private data\n");
> -		return -EINVAL;
> -	}
> -
>  	msg.msg_id	= HSMP_GET_METRIC_TABLE;
>  	msg.sock_ind	= sock->sock_ind;
> 

sock gets initialized like this:

        struct hsmp_socket *sock = bin_attr->private;

and bin_attr->private is setup before registering the file
and thus it will never be NULL.

So the correct fix would be to simply drop the check
rather then to move it.

> @@ -739,8 +737,7 @@ static int hsmp_cache_proto_ver(u16 sock_ind)
>  
>  static inline bool is_f1a_m0h(void)
>  {
> -	if (boot_cpu_data.x86 == 0x1A &&
> -	    (boot_cpu_data.x86_model >= 0x00 && boot_cpu_data.x86_model <= 0x0F))
> +	if (boot_cpu_data.x86 == 0x1A && boot_cpu_data.x86_model <= 0x0F)
>  		return true;
>  
>  	return false;

This bit looks fine but this really belongs in a separate patch
as it has nothing to do with "Remove NULL dereferencing code"

Regards,

Hans

Re: [PATCH v2] platform/x86/amd/hsmp: Remove NULL dereferencing code

[Hegde, Suma]

Hi Hans,

On 2/19/2024 6:18 PM, Hans de Goede wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> Hi Suma,
>
> On 1/30/24 08:34, Suma Hegde wrote:
>> Do not log using dev_err() in case of !sock, which causes null pointer
>> dereferencing.
>>
>> Also remove unnecessary check "boot_cpu_data.x86_model >= 0x00", which is
>> always true because its an unsigned type.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202401292056.qkUFS09Y-lkp@intel.com/
>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
>> Closes: https://lore.kernel.org/r/202401291311.gzMCj6SP-lkp@intel.com/
>>
>> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
>> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
>> ---
>> Changes since v1:
>> Correct the email id for Naveen Krishna Chatradhi.
>>
>>   drivers/platform/x86/amd/hsmp.c | 11 ++++-------
>>   1 file changed, 4 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
>> index 1baddf403920..1927be901108 100644
>> --- a/drivers/platform/x86/amd/hsmp.c
>> +++ b/drivers/platform/x86/amd/hsmp.c
>> @@ -566,17 +566,15 @@ static ssize_t hsmp_metric_tbl_read(struct file *filp, struct kobject *kobj,
>>        struct hsmp_message msg = { 0 };
>>        int ret;
>>
>> +     if (!sock)
>> +             return -EINVAL;
>> +
>>        /* Do not support lseek(), reads entire metric table */
>>        if (count < bin_attr->size) {
>>                dev_err(sock->dev, "Wrong buffer size\n");
>>                return -EINVAL;
>>        }
>>
>> -     if (!sock) {
>> -             dev_err(sock->dev, "Failed to read attribute private data\n");
>> -             return -EINVAL;
>> -     }
>> -
>>        msg.msg_id      = HSMP_GET_METRIC_TABLE;
>>        msg.sock_ind    = sock->sock_ind;
>>
> sock gets initialized like this:
>
>          struct hsmp_socket *sock = bin_attr->private;
>
> and bin_attr->private is setup before registering the file
> and thus it will never be NULL.
>
> So the correct fix would be to simply drop the check
> rather then to move it.
Thank you for your review comment, I will send a patch to address this 
change.
>> @@ -739,8 +737,7 @@ static int hsmp_cache_proto_ver(u16 sock_ind)
>>
>>   static inline bool is_f1a_m0h(void)
>>   {
>> -     if (boot_cpu_data.x86 == 0x1A &&
>> -         (boot_cpu_data.x86_model >= 0x00 && boot_cpu_data.x86_model <= 0x0F))
>> +     if (boot_cpu_data.x86 == 0x1A && boot_cpu_data.x86_model <= 0x0F)
>>                return true;
>>
>>        return false;
> This bit looks fine but this really belongs in a separate patch
> as it has nothing to do with "Remove NULL dereferencing code"

Ilpo has already merged it into relevant patch(platform/x86/amd/hsmp: 
Non-ACPI support for AMD F1A_M00~0Fh) and the change is available in 
review-ilpo branch.

> Regards,
>
> Hans


Thanks and Regards,

Suma

>

Re: [PATCH v2] platform/x86/amd/hsmp: Add CONFIG_ACPI dependency

[Hans de Goede]

Hi,

On 1/30/24 08:34, Suma Hegde wrote:
> HSMP interface is only supported on x86 based AMD EPYC line of
> processors. Driver uses ACPI APIs, so make it dependent on CONFIG_ACPI.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202401281437.aus91srb-lkp@intel.com/
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.

Regards,

Hans


> ---
> Changes since v1:
> Correct the email id for Naveen Krishna Chatradhi and change it as
> Reviewed-by.
> 
>  drivers/platform/x86/amd/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig
> index 54753213cc61..f88682d36447 100644
> --- a/drivers/platform/x86/amd/Kconfig
> +++ b/drivers/platform/x86/amd/Kconfig
> @@ -8,7 +8,7 @@ source "drivers/platform/x86/amd/pmc/Kconfig"
>  
>  config AMD_HSMP
>  	tristate "AMD HSMP Driver"
> -	depends on AMD_NB && X86_64
> +	depends on AMD_NB && X86_64 && ACPI
>  	help
>  	  The driver provides a way for user space tools to monitor and manage
>  	  system management functionality on EPYC server CPUs from AMD.

Re: [PATCH v2] platform/x86/amd/hsmp: Add CONFIG_ACPI dependency

[Hegde, Suma]

Hi Hans,

On 2/19/2024 6:15 PM, Hans de Goede wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> Hi,
>
> On 1/30/24 08:34, Suma Hegde wrote:
>> HSMP interface is only supported on x86 based AMD EPYC line of
>> processors. Driver uses ACPI APIs, so make it dependent on CONFIG_ACPI.
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202401281437.aus91srb-lkp@intel.com/
>> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
>> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> Thank you for your patch, I've applied this patch to my review-hans
> branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
>
> Note it will show up in my review-hans branch once I've pushed my
> local branch there, which might take a while.
>
> I will include this patch in my next fixes pull-req to Linus
> for the current kernel development cycle.
>
> Regards,
>
> Hans

This change was merged by Ilpo in review-ilpo branch into commit: 
platform/x86/amd/hsmp: Add support for ACPI based probing.

>
>
>> ---
>> Changes since v1:
>> Correct the email id for Naveen Krishna Chatradhi and change it as
>> Reviewed-by.
>>
>>   drivers/platform/x86/amd/Kconfig | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig
>> index 54753213cc61..f88682d36447 100644
>> --- a/drivers/platform/x86/amd/Kconfig
>> +++ b/drivers/platform/x86/amd/Kconfig
>> @@ -8,7 +8,7 @@ source "drivers/platform/x86/amd/pmc/Kconfig"
>>
>>   config AMD_HSMP
>>        tristate "AMD HSMP Driver"
>> -     depends on AMD_NB && X86_64
>> +     depends on AMD_NB && X86_64 && ACPI
>>        help
>>          The driver provides a way for user space tools to monitor and manage
>>          system management functionality on EPYC server CPUs from AMD.

Re: [PATCH v2] platform/x86/amd/hsmp: Add CONFIG_ACPI dependency

[Ilpo Järvinen]

On Tue, 20 Feb 2024, Hegde, Suma wrote:
> On 2/19/2024 6:15 PM, Hans de Goede wrote:
> > On 1/30/24 08:34, Suma Hegde wrote:
> > > HSMP interface is only supported on x86 based AMD EPYC line of
> > > processors. Driver uses ACPI APIs, so make it dependent on CONFIG_ACPI.
> > > 
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Closes:
> > > https://lore.kernel.org/oe-kbuild-all/202401281437.aus91srb-lkp@intel.com/
> > > Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> > > Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> > Thank you for your patch, I've applied this patch to my review-hans
> > branch:
> > https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
> > 
> > Note it will show up in my review-hans branch once I've pushed my
> > local branch there, which might take a while.
> > 
> > I will include this patch in my next fixes pull-req to Linus
> > for the current kernel development cycle.

> This change was merged by Ilpo in review-ilpo branch into commit:
> platform/x86/amd/hsmp: Add support for ACPI based probing.

> > > diff --git a/drivers/platform/x86/amd/Kconfig
> > > b/drivers/platform/x86/amd/Kconfig
> > > index 54753213cc61..f88682d36447 100644
> > > --- a/drivers/platform/x86/amd/Kconfig
> > > +++ b/drivers/platform/x86/amd/Kconfig
> > > @@ -8,7 +8,7 @@ source "drivers/platform/x86/amd/pmc/Kconfig"
> > > 
> > >   config AMD_HSMP
> > >        tristate "AMD HSMP Driver"
> > > -     depends on AMD_NB && X86_64
> > > +     depends on AMD_NB && X86_64 && ACPI

Yes, it only belongs to for-next.

The change that triggered the build fail is (it is only in for-next):

commit ba8dcff0e9c4f8fa6a46315126fb837acb0f98fc
Author: Suma Hegde <suma.hegde@amd.com>
Date:   Sat Jan 6 02:25:28 2024 +0000

    platform/x86/amd/hsmp: Add support for ACPI based probing

-- 
 i.

Re: [PATCH v2] platform/x86/amd/hsmp: Add CONFIG_ACPI dependency

[Hans de Goede]

Hi,

On 2/20/24 13:06, Ilpo Järvinen wrote:
> On Tue, 20 Feb 2024, Hegde, Suma wrote:
>> On 2/19/2024 6:15 PM, Hans de Goede wrote:
>>> On 1/30/24 08:34, Suma Hegde wrote:
>>>> HSMP interface is only supported on x86 based AMD EPYC line of
>>>> processors. Driver uses ACPI APIs, so make it dependent on CONFIG_ACPI.
>>>>
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Closes:
>>>> https://lore.kernel.org/oe-kbuild-all/202401281437.aus91srb-lkp@intel.com/
>>>> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
>>>> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
>>> Thank you for your patch, I've applied this patch to my review-hans
>>> branch:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
>>>
>>> Note it will show up in my review-hans branch once I've pushed my
>>> local branch there, which might take a while.
>>>
>>> I will include this patch in my next fixes pull-req to Linus
>>> for the current kernel development cycle.
> 
>> This change was merged by Ilpo in review-ilpo branch into commit:
>> platform/x86/amd/hsmp: Add support for ACPI based probing.
> 
>>>> diff --git a/drivers/platform/x86/amd/Kconfig
>>>> b/drivers/platform/x86/amd/Kconfig
>>>> index 54753213cc61..f88682d36447 100644
>>>> --- a/drivers/platform/x86/amd/Kconfig
>>>> +++ b/drivers/platform/x86/amd/Kconfig
>>>> @@ -8,7 +8,7 @@ source "drivers/platform/x86/amd/pmc/Kconfig"
>>>>
>>>>   config AMD_HSMP
>>>>        tristate "AMD HSMP Driver"
>>>> -     depends on AMD_NB && X86_64
>>>> +     depends on AMD_NB && X86_64 && ACPI
> 
> Yes, it only belongs to for-next.
> 
> The change that triggered the build fail is (it is only in for-next):

Ah, my bad, thank you both for catching this.

I have dropped this from my review-hans branch now.

Regards,

Hans