Re: [PATCH v2 3/7] platform/x86/amd/hsmp: Add explicit metric DRAM mapping and per-socket mutexes

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

On Thu, 25 Jun 2026, Muralidhara M K wrote:

> Add hsmp_destroy_metric_read_locks(), hsmp_init_metric_read_locks(), and
> switch metric table DRAM from devm_ioremap() to ioremap() so ACPI can
> coordinate teardown.  Embed struct mutex metric_read_lock in each
> hsmp_socket for later serialization of metric table reads.
> 
> Call the new helpers from the platform driver probe and remove paths
> so non-ACPI builds initialize and tear down metric_read_lock consistently
> with the ACPI driver.
> 
> Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
> ---
>  drivers/platform/x86/amd/hsmp/hsmp.c | 38 ++++++++++++++++++++++++++--
>  drivers/platform/x86/amd/hsmp/hsmp.h |  5 ++++
>  drivers/platform/x86/amd/hsmp/plat.c | 16 ++++++++++--
>  3 files changed, 55 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
> index 91be0cdb6af1..a96c59fcba0a 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
> @@ -12,7 +12,11 @@
>  #include <linux/acpi.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/mutex.h>
> +#include <linux/nospec.h>
>  #include <linux/semaphore.h>
> +#include <linux/slab.h>
>  #include <linux/sysfs.h>
>  
>  #include "hsmp.h"
> @@ -410,6 +414,37 @@ ssize_t hsmp_metric_tbl_read(struct hsmp_socket *sock, char *buf, size_t size)
>  }
>  EXPORT_SYMBOL_NS_GPL(hsmp_metric_tbl_read, "AMD_HSMP");
>  
> +void hsmp_init_metric_read_locks(struct hsmp_plat_device *pdev, u16 num_sockets)
> +{
> +	u16 i;
> +
> +	if (!pdev->sock || !num_sockets)
> +		return;
> +
> +	for (i = 0; i < num_sockets; i++)
> +		mutex_init(&pdev->sock[i].metric_read_lock);
> +}
> +EXPORT_SYMBOL_NS_GPL(hsmp_init_metric_read_locks, "AMD_HSMP");
> +
> +void hsmp_destroy_metric_read_locks(struct hsmp_plat_device *pdev, u16 num_sockets)
> +{
> +	u16 i;
> +
> +	if (!pdev->sock || !num_sockets)
> +		return;
> +
> +	for (i = 0; i < num_sockets; i++) {
> +		struct hsmp_socket *s = &pdev->sock[i];
> +
> +		if (s->metric_tbl_addr) {
> +			iounmap(s->metric_tbl_addr);
> +			s->metric_tbl_addr = NULL;
> +		}
> +		mutex_destroy(&s->metric_read_lock);
> +	}
> +}
> +EXPORT_SYMBOL_NS_GPL(hsmp_destroy_metric_read_locks, "AMD_HSMP");
> +
>  int hsmp_get_tbl_dram_base(u16 sock_ind)
>  {
>  	struct hsmp_socket *sock = &hsmp_pdev.sock[sock_ind];
> @@ -434,8 +469,7 @@ int hsmp_get_tbl_dram_base(u16 sock_ind)
>  		dev_err(sock->dev, "Invalid DRAM address for metric table\n");
>  		return -ENOMEM;
>  	}
> -	sock->metric_tbl_addr = devm_ioremap(sock->dev, dram_addr,
> -					     sizeof(struct hsmp_metric_table));
> +	sock->metric_tbl_addr = ioremap(dram_addr, sizeof(struct hsmp_metric_table));
>  	if (!sock->metric_tbl_addr) {
>  		dev_err(sock->dev, "Failed to ioremap metric table addr\n");
>  		return -ENOMEM;
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
> index 0509a442eaae..91bc21232646 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.h
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.h
> @@ -15,6 +15,7 @@
>  #include <linux/hwmon.h>
>  #include <linux/kconfig.h>
>  #include <linux/miscdevice.h>
> +#include <linux/mutex.h>
>  #include <linux/pci.h>
>  #include <linux/semaphore.h>
>  #include <linux/sysfs.h>
> @@ -41,6 +42,8 @@ struct hsmp_socket {
>  	struct bin_attribute hsmp_attr;
>  	struct hsmp_mbaddr_info mbinfo;
>  	void __iomem *metric_tbl_addr;
> +	/* Protects metric table snapshot reads for this socket */
> +	struct mutex metric_read_lock;

After applying all the patches in your series (in this series only), I 
only see this:

~/linux/platform/drivers/platform/x86/amd/hsmp$ git grep -w metric_read_lock
hsmp.c:         mutex_init(&pdev->sock[i].metric_read_lock);
hsmp.c:         mutex_destroy(&s->metric_read_lock);
hsmp.h: struct mutex metric_read_lock;

Is this lock ever even taken???

--
 i.

>  	void __iomem *virt_base_addr;
>  	struct semaphore hsmp_sem;
>  	char name[HSMP_ATTR_GRP_NAME_SIZE];
> @@ -63,7 +66,9 @@ long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg);
>  void hsmp_misc_deregister(void);
>  int hsmp_misc_register(struct device *dev);
>  int hsmp_get_tbl_dram_base(u16 sock_ind);
> +void hsmp_init_metric_read_locks(struct hsmp_plat_device *pdev, u16 num_sockets);
>  ssize_t hsmp_metric_tbl_read(struct hsmp_socket *sock, char *buf, size_t size);
> +void hsmp_destroy_metric_read_locks(struct hsmp_plat_device *pdev, u16 num_sockets);
>  struct hsmp_plat_device *get_hsmp_pdev(void);
>  #if IS_ENABLED(CONFIG_HWMON)
>  int hsmp_create_sensor(struct device *dev, u16 sock_ind);
> diff --git a/drivers/platform/x86/amd/hsmp/plat.c b/drivers/platform/x86/amd/hsmp/plat.c
> index e07f68575055..685f2d2c574b 100644
> --- a/drivers/platform/x86/amd/hsmp/plat.c
> +++ b/drivers/platform/x86/amd/hsmp/plat.c
> @@ -211,25 +211,37 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
>  	if (!hsmp_pdev->sock)
>  		return -ENOMEM;
>  
> +	hsmp_init_metric_read_locks(hsmp_pdev, hsmp_pdev->num_sockets);
> +
>  	ret = init_platform_device(&pdev->dev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to init HSMP mailbox\n");
> -		return ret;
> +		goto err_destroy_locks;
>  	}
>  
>  	ret = hsmp_misc_register(&pdev->dev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to register misc device\n");
> -		return ret;
> +		goto err_destroy_locks;
>  	}
>  
>  	dev_dbg(&pdev->dev, "AMD HSMP is probed successfully\n");
>  	return 0;
> +
> +err_destroy_locks:
> +	/*
> +	 * init_platform_device() may have ioremap()ed metric tables before
> +	 * failing.  hsmp_destroy_metric_read_locks() unmaps them and tears
> +	 * down the per-socket mutexes; the socket array itself is devm-managed.
> +	 */
> +	hsmp_destroy_metric_read_locks(hsmp_pdev, hsmp_pdev->num_sockets);
> +	return ret;
>  }
>  
>  static void hsmp_pltdrv_remove(struct platform_device *pdev)
>  {
>  	hsmp_misc_deregister();
> +	hsmp_destroy_metric_read_locks(hsmp_pdev, hsmp_pdev->num_sockets);
>  }
>  
>  static struct platform_driver amd_hsmp_driver = {
>