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

[M K, Muralidhara]

On 7/8/2026 3:47 PM, Ilpo Järvinen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> 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.
Agreed, I'll reword to:

Switch hsmp_get_tbl_dram_base() to plain ioremap() and add 
hsmp_unmap_metric_tbls(). The unmapping of all sockets is still devres 
managed, via devm_add_action_or_reset(); the socket array itself stays 
devres managed too.

> > Signed-off-by: Muralidhara M K <muralidhara.mk@xxxxxxx>
> > ---
> >   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.
Will do.

> > + */
> > +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.