Re: [v2] platform/x86/amd/hsmp: Improve the print messages to avoid confusion

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Thu, 19 Jun 2025, Suma Hegde wrote:

> When the HSMP ACPI device is available, then loading the amd_hsmp.ko
> module incorrectly prints the message "HSMP is not supported on
> Family:%x model:%x\n" despite being supported by the hsmp_acpi.ko
> module, leading to confusion.
> 
> To resolve this, relocate the acpi_dev_present() check to the
> beginning of the hsmp_plt_init() and revise the print message
> to better reflect the current support status.
> 
> Also add messages indicating successful probing for both
> hsmp_acpi.ko and amd_hsmp.ko modules.
> 
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> ---
> Changes since v1
> Move successful probe print message inside the if condition in acpi.c.
> 
>  drivers/platform/x86/amd/hsmp/acpi.c |  1 +
>  drivers/platform/x86/amd/hsmp/plat.c | 17 ++++++++++++-----
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
> index 2f1faa82d13e..bdfb86eeecf2 100644
> --- a/drivers/platform/x86/amd/hsmp/acpi.c
> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
> @@ -608,6 +608,7 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
>  		if (ret)
>  			return ret;
>  		hsmp_pdev->is_probed = true;
> +		dev_info(&pdev->dev, "AMD HSMP ACPI is probed successfully\n");
>  	}
>  
>  	return 0;
> diff --git a/drivers/platform/x86/amd/hsmp/plat.c b/drivers/platform/x86/amd/hsmp/plat.c
> index e3874c47ed9e..724e5c7fc819 100644
> --- a/drivers/platform/x86/amd/hsmp/plat.c
> +++ b/drivers/platform/x86/amd/hsmp/plat.c
> @@ -215,7 +215,12 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	return hsmp_misc_register(&pdev->dev);
> +	ret = hsmp_misc_register(&pdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	dev_info(&pdev->dev, "AMD HSMP is probed successfully\n");

Printing anything should be avoided when something has probed 
succesfully. Thus, either remove these or make them dev_dbg() level.

I guess you added them to have a positive confirmation after the legacy 
side print below, but I think it should be solved differently.

> +	return 0;
>  }
>  
>  static void hsmp_pltdrv_remove(struct platform_device *pdev)
> @@ -287,15 +292,17 @@ static int __init hsmp_plt_init(void)
>  {
>  	int ret = -ENODEV;
>  
> +	if (acpi_dev_present(ACPI_HSMP_DEVICE_HID, NULL, -1)) {
> +		pr_info("HSMP is supported through ACPI on this platform, please use hsmp_acpi.ko\n");

I suggest you make this printing depend on whether AMD_HSMP_ACPI is 
enabled.

I think this should be totally silent if the probe is expected to happen 
through the ACPI driver and AMD_HSMP_ACPI is enabled (or do a pr_debug() 
level print at most).

In the case AMD_HSMP_ACPI is not enabled, you might consider adding 
pr_info() that AMD_HSMP_ACPI should be enabled.

That way, there's zero noise on success. Only downside is, that if legacy 
driver is loaded manually, it doesn't tell about the failure other than 
through the error code, but that seems a very minor problem, and as 
mentioned, pr_debug() could be used to cover that case too if wanted.

> +		return -ENODEV;
> +	}
> +
>  	if (!legacy_hsmp_support()) {
> -		pr_info("HSMP is not supported on Family:%x model:%x\n",
> +		pr_info("HSMP interface is either disabled or not supported on family:%x model:%x\n",
>  			boot_cpu_data.x86, boot_cpu_data.x86_model);
>  		return ret;
>  	}
>  
> -	if (acpi_dev_present(ACPI_HSMP_DEVICE_HID, NULL, -1))
> -		return -ENODEV;
> -
>  	hsmp_pdev = get_hsmp_pdev();
>  	if (!hsmp_pdev)
>  		return -ENOMEM;
> 

-- 
 i.