Re: [PATCH v2 2/4] platform/x86/amd/hsmp: Add reference counter to track ACPI probe and removal

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

On Wed, 23 Jul 2025, Suma Hegde wrote:

> Implement reference counting to ensure that the memory allocated for
> the hsmp_pdev->sock structure is released, and the miscellaneous device
> is deregistered only after all socket's removal operations have been
> completed.
> 
> Also replace hsmp_pdev->is_probed with hsmp_pdev->ref_cnt.
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> ---
> Changes since v1:
> New patch
> 
>  drivers/platform/x86/amd/hsmp/acpi.c | 16 ++++++++--------
>  drivers/platform/x86/amd/hsmp/hsmp.c | 17 +++++++++++++++++
>  drivers/platform/x86/amd/hsmp/hsmp.h |  2 +-
>  3 files changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
> index b981ae3157ea..232105226407 100644
> --- a/drivers/platform/x86/amd/hsmp/acpi.c
> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
> @@ -23,6 +23,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/platform_device.h>
> +#include <linux/slab.h>
>  #include <linux/sysfs.h>
>  #include <linux/uuid.h>
>  
> @@ -590,16 +591,15 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	guard(mutex)(&hsmp_lock);
>  
> -	if (!hsmp_pdev->is_probed) {
> +	if (!hsmp_pdev->ref_cnt) {
>  		hsmp_pdev->num_sockets = amd_num_nodes();
>  		if (hsmp_pdev->num_sockets == 0 || hsmp_pdev->num_sockets > MAX_AMD_NUM_NODES) {
>  			dev_err(&pdev->dev, "Wrong number of sockets\n");
>  			return -ENODEV;
>  		}
>  
> -		hsmp_pdev->sock = devm_kcalloc(&pdev->dev, hsmp_pdev->num_sockets,
> -					       sizeof(*hsmp_pdev->sock),
> -					       GFP_KERNEL);
> +		hsmp_pdev->sock = kcalloc(hsmp_pdev->num_sockets, sizeof(*hsmp_pdev->sock),
> +					  GFP_KERNEL);
>  		if (!hsmp_pdev->sock)
>  			return -ENOMEM;
>  	}
> @@ -610,15 +610,15 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	if (!hsmp_pdev->is_probed) {
> +	if (!hsmp_pdev->ref_cnt) {
>  		ret = hsmp_misc_register(&pdev->dev);
>  		if (ret) {
>  			dev_err(&pdev->dev, "Failed to register misc device\n");
>  			return ret;
>  		}
> -		hsmp_pdev->is_probed = true;
>  		dev_dbg(&pdev->dev, "AMD HSMP ACPI is probed successfully\n");
>  	}
> +	hsmp_pdev->ref_cnt++;

Unfortunately, also this is buggy.

hsmp_pdev->sock is assigned before we're committed to increase ref_cnt 
which causes a memleak as the next one probing after the mutex is unlocked 
on return will overwrite hsmp_pdev->sock.

There are also linux/kref.h linux/refcount.h which should be considered 
instead of creating own ref count code.

>  	return 0;
>  }
> @@ -630,9 +630,9 @@ static void hsmp_acpi_remove(struct platform_device *pdev)
>  	 * We register only one misc_device even on multi-socket system.
>  	 * So, deregister should happen only once.
>  	 */
> -	if (hsmp_pdev->is_probed) {
> +	if (!(--hsmp_pdev->ref_cnt)) {
>  		hsmp_misc_deregister();
> -		hsmp_pdev->is_probed = false;
> +		kfree(hsmp_pdev->sock);
>  	}
>  }
>  
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
> index 885e2f8136fd..39804ee848ba 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
> @@ -12,6 +12,8 @@
>  #include <linux/acpi.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
>  #include <linux/semaphore.h>
>  #include <linux/sysfs.h>
>  
> @@ -454,6 +456,21 @@ struct hsmp_plat_device *get_hsmp_pdev(void)
>  {
>  	return &hsmp_pdev;
>  }
> +
> +static int __init hsmp_common_init(void)
> +{
> +	hsmp_pdev.ref_cnt = 0;
> +
> +	return 0;
> +}
> +
> +static void __exit hsmp_common_exit(void)
> +{
> +}
> +
> +device_initcall(hsmp_common_init);
> +module_exit(hsmp_common_exit);
> +
>  EXPORT_SYMBOL_NS_GPL(get_hsmp_pdev, "AMD_HSMP");
>  
>  MODULE_DESCRIPTION("AMD HSMP Common driver");
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
> index 0509a442eaae..1b16fd6a38e1 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.h
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.h
> @@ -54,7 +54,7 @@ struct hsmp_plat_device {
>  	struct hsmp_socket *sock;
>  	u32 proto_ver;
>  	u16 num_sockets;
> -	bool is_probed;
> +	int ref_cnt;
>  };
>  
>  int hsmp_cache_proto_ver(u16 sock_ind);
> 

-- 
 i.