Re: [PATCH v4 2/6] platform/x86/amd/hsmp: Map the metric table with ioremap() and unmap it explicitly

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

On Wed, 8 Jul 2026, Muralidhara M K wrote:

> The metric table DRAM region is mapped with devm_ioremap(), tying its
> lifetime to the socket device. An upcoming change lets the ACPI front-end
> share the socket array across sockets and coordinate its own teardown, so
> the mapping can no longer be pinned to a single per-socket devres scope.
> 
> Switch hsmp_get_tbl_dram_base() to plain ioremap() and add
> hsmp_unmap_metric_tbls(), which drops every socket's metric_tbl_addr
> mapping. On the platform driver register it with devm_add_action_or_reset()
> so the mapping is released on both remove and probe failure;

I feel the explanation is quite basic level description of devres 
functionality. I'd say something like this instead:

The unmapping for all sockets as whole is still devres managed using 
devm_add_action_or_reset().

> the socket array itself stays devm-managed.

devres managed is probably the better term in textual form. "devm" is C 
codish way to say it in a short way.
 
> Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
> ---
>  drivers/platform/x86/amd/hsmp/hsmp.c | 19 +++++++++++++++++--
>  drivers/platform/x86/amd/hsmp/hsmp.h |  1 +
>  drivers/platform/x86/amd/hsmp/plat.c | 14 ++++++++++++++
>  3 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
> index 1a87931136fd..4586d86f72a3 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
> @@ -12,6 +12,7 @@
>  #include <linux/acpi.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
> +#include <linux/io.h>
>  #include <linux/semaphore.h>
>  #include <linux/sysfs.h>
>  
> @@ -414,6 +415,21 @@ 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_unmap_metric_tbls(struct hsmp_plat_device *pdev)
> +{
> +	u16 i;
> +
> +	for (i = 0; i < pdev->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;
> +		}
> +	}
> +}
> +EXPORT_SYMBOL_NS_GPL(hsmp_unmap_metric_tbls, "AMD_HSMP");
> +
>  int hsmp_get_tbl_dram_base(u16 sock_ind)
>  {
>  	struct hsmp_socket *sock = &hsmp_pdev.sock[sock_ind];
> @@ -438,8 +454,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..98c82a4c0cc3 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.h
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.h
> @@ -64,6 +64,7 @@ void hsmp_misc_deregister(void);
>  int hsmp_misc_register(struct device *dev);
>  int hsmp_get_tbl_dram_base(u16 sock_ind);
>  ssize_t hsmp_metric_tbl_read(struct hsmp_socket *sock, char *buf, size_t size);
> +void hsmp_unmap_metric_tbls(struct hsmp_plat_device *pdev);
>  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..e7ee6e701e5a 100644
> --- a/drivers/platform/x86/amd/hsmp/plat.c
> +++ b/drivers/platform/x86/amd/hsmp/plat.c
> @@ -201,6 +201,16 @@ static int init_platform_device(struct device *dev)
>  	return 0;
>  }
>  
> +/*
> + * The metric tables are mapped with ioremap() rather than devm, so release
> + * them explicitly. Registered as a devres action so it also runs on a probe
> + * failure after init_platform_device() has mapped some of them.

Please put "Registered..." into own paragraph. It's sort of different 
thing it's talking about that the first sentence, and you'll be adding to 
the first paragraph in the other patch as well.

/*
 * Paragraph 1.
 *
 * Paragraph 2.

> + */
> +static void hsmp_pltdrv_release(void *data)
> +{
> +	hsmp_unmap_metric_tbls(hsmp_pdev);
> +}
> +
>  static int hsmp_pltdrv_probe(struct platform_device *pdev)
>  {
>  	int ret;
> @@ -211,6 +221,10 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
>  	if (!hsmp_pdev->sock)
>  		return -ENOMEM;
>  
> +	ret = devm_add_action_or_reset(&pdev->dev, hsmp_pltdrv_release, NULL);
> +	if (ret)
> +		return ret;
> +
>  	ret = init_platform_device(&pdev->dev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to init HSMP mailbox\n");
> 

-- 
 i.