Re: [PATCH v5 5/6] platform/x86/amd/hsmp: ACPI HSMP refcounted sockets and coordinated release

["M K, Muralidhara" <muralimk@amd.com>]

On 7/10/2026 11:21 PM, Ilpo Järvinen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Fri, 10 Jul 2026, Muralidhara M K wrote:
> 
>> The ACPI driver binds one platform device per socket but shares a single
>> socket array and a single /dev/hsmp misc device across them. Replace the
>> is_probed flag with state that tracks this shared ownership:
>>
>>   - miscdevice.this_device tells whether /dev/hsmp is registered, so the
>>     misc device is registered on the first socket and torn down last. A
>>     preceding change clears mdev.this_device on deregister so this gate
>>     stays reliable across a re-probe.
>>
>>   - hsmp_acpi_sock_refs counts the sockets that have probed successfully.
>>     It is guarded by hsmp_sock_rwsem, which probe and remove already hold
>>     for write, so a plain counter is enough and no atomic refcount is
>>     needed. The shared socket array is allocated with kcalloc() on the first
>>     probe and freed by hsmp_acpi_sock_release() when the count drops back to
>>     zero.
>>
>> hsmp_acpi_sock_release() is the single teardown helper: it deregisters
>> /dev/hsmp if registered, unmaps any metric-table DRAM, destroys the
>> per-socket mutexes and frees the array. The remove path and the
>> probe-failure path each call it once they are the last owner, so the
>> teardown lives in one place.
>>
>> Both paths also clear this socket's dev, so a message issued after a
>> non-final unbind (or to a socket that failed to probe on a multi-socket
>> system, whose array stays alive and whose remove() is never called) cannot
>> reach the mailbox that devres is about to unmap.
>>
>> Two lifetime fixes fall out of the array persisting across a non-final
>> unbind:
>>
>>   - hsmp_get_tbl_dram_base() iounmap()s any stale metric_tbl_addr before
>>     remapping, so a rebind does not leak one mapping per cycle. It runs
>>     during (re)probe before the metric sysfs attribute is exposed, so no
>>     reader can be using the old mapping.
>>
>>   - The ACPI path registers /dev/hsmp unparented by passing NULL to
>>     hsmp_misc_register(). Its per-socket devices can be unbound individually
>>     and out of order and the misc device outlives all but the last of them,
>>     so parenting it to one socket's device would leave a dangling parent.
>>     hsmp_misc_register() now takes the parent from its caller, so the
>>     platform driver keeps parenting /dev/hsmp to its single device.
>>
>> hsmp_sock_rwsem is held for write across probe and remove, so the release
>> and probe-failure cleanup run with it already held; an upcoming change adds
>> its read side so the same lock also drains the lock-free data plane.
>>
>> Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
>> ---
>>   drivers/platform/x86/amd/hsmp/acpi.c | 122 ++++++++++++++++++++++-----
>>   drivers/platform/x86/amd/hsmp/hsmp.c |  20 +++++
>>   drivers/platform/x86/amd/hsmp/hsmp.h |   1 -
>>   3 files changed, 123 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
>> index a092d7589bcb..8339af1624f4 100644
>> --- a/drivers/platform/x86/amd/hsmp/acpi.c
>> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
>> @@ -24,6 +24,7 @@
>>   #include <linux/module.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/rwsem.h>
>> +#include <linux/slab.h>
>>   #include <linux/string.h>
>>   #include <linux/sysfs.h>
>>   #include <linux/topology.h>
>> @@ -42,6 +43,14 @@
>>
>>   static struct hsmp_plat_device *hsmp_pdev;
>>
>> +/*
>> + * Number of ACPI socket platform devices that have probed successfully.
>> + * Guarded by hsmp_sock_rwsem, which probe and remove hold for write, so a
>> + * plain counter is enough; no atomic is needed. The shared socket array is
>> + * allocated on the first probe and freed once this drops back to zero.
>> + */
>> +static unsigned int hsmp_acpi_sock_refs;
>> +
>>   struct hsmp_sys_attr {
>>        struct device_attribute dattr;
>>        u32 msg_id;
>> @@ -611,6 +620,60 @@ static const struct acpi_device_id amd_hsmp_acpi_ids[] = {
>>   };
>>   MODULE_DEVICE_TABLE(acpi, amd_hsmp_acpi_ids);
>>
>> +/*
>> + * Tear down the shared ACPI socket state once the last socket is gone:
>> + * deregister /dev/hsmp if it was registered, unmap any metric-table DRAM,
>> + * destroy the per-socket mutexes and free the socket array.
>> + *
>> + * Called with hsmp_sock_rwsem held for write by the remove and probe-failure
>> + * paths. The write lock has drained any in-flight hsmp_send_message(), so
>> + * unmapping the mailbox and freeing the array cannot race the lock-free data
>> + * plane.
>> + */
>> +static void hsmp_acpi_sock_release(void)
>> +{
>> +     lockdep_assert_held_write(&hsmp_sock_rwsem);
>> +
>> +     if (!IS_ERR_OR_NULL(hsmp_pdev->mdev.this_device))
>> +             hsmp_misc_deregister();
>> +     hsmp_unmap_metric_tbls(hsmp_pdev);
>> +     hsmp_destroy_metric_read_locks(hsmp_pdev);
>> +     kfree(hsmp_pdev->sock);
>> +     hsmp_pdev->sock = NULL;
>> +     hsmp_pdev->num_sockets = 0;
>> +     hsmp_pdev->proto_ver = 0;
>> +}
>> +
>> +/**
>> + * hsmp_acpi_probe_failure_cleanup() - Undo a failed ACPI socket probe.
>> + * @dev: ACPI companion device whose probe failed.
>> + *
>> + * This device never incremented hsmp_acpi_sock_refs, so clear its sock->dev
>> + * and, if it was the only socket in play, release the shared state.
>> + *
>> + * Clearing sock->dev matters on multi-socket systems: when a non-first socket
>> + * fails, the array stays alive (owned by an already-probed socket) and
>> + * remove() is never called for this device, yet devres unmaps its mailbox once
>> + * probe() returns. Without clearing dev, a later message to this index would
>> + * pass every gate in hsmp_send_message() and reach the unmapped mailbox.
>> + *
>> + * sock is NULL if probe failed before hsmp_parse_acpi_table() set the drvdata.
>> + *
>> + * Called from hsmp_acpi_probe(), which already holds hsmp_sock_rwsem for write.
>> + */
>> +static void hsmp_acpi_probe_failure_cleanup(struct device *dev)
>> +{
>> +     struct hsmp_socket *sock = dev_get_drvdata(dev);
>> +
>> +     lockdep_assert_held_write(&hsmp_sock_rwsem);
>> +
>> +     if (sock)
>> +             sock->dev = NULL;
>> +
>> +     if (!hsmp_acpi_sock_refs)
>> +             hsmp_acpi_sock_release();
>> +}
>> +
>>   static int hsmp_acpi_probe(struct platform_device *pdev)
>>   {
>>        int ret;
>> @@ -620,23 +683,24 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
>>                return -ENOMEM;
>>
>>        /*
>> -      * Multiple ACPI socket devices probe in parallel, but the is_probed
>> -      * handshake and the one-time socket-array allocation below must run
>> -      * exactly once.  Serialize the whole bring-up against concurrent
>> -      * probe/remove by holding the socket rwsem for write.
>> +      * Multiple ACPI socket devices probe in parallel, but the one-time
>> +      * socket-array allocation and /dev/hsmp registration below must run
>> +      * exactly once. Hold the socket rwsem for write across the whole
>> +      * bring-up so it cannot race a concurrent probe or remove, and so the
>> +      * probe-failure teardown drains the lock-free data plane.
>>         */
>>        guard(rwsem_write)(&hsmp_sock_rwsem);
>>
>> -     if (!hsmp_pdev->is_probed) {
>> +     if (!hsmp_pdev->sock) {
>>                hsmp_pdev->num_sockets = topology_max_packages();
>>                if (!hsmp_pdev->num_sockets) {
>>                        dev_err(&pdev->dev, "No CPU sockets detected\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;
>>
>> @@ -646,35 +710,55 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
>>        ret = init_acpi(&pdev->dev);
>>        if (ret) {
>>                dev_err(&pdev->dev, "Failed to initialize HSMP interface.\n");
>> +             hsmp_acpi_probe_failure_cleanup(&pdev->dev);
>>                return ret;
>>        }
>>
>> -     if (!hsmp_pdev->is_probed) {
>> -             ret = hsmp_misc_register(&pdev->dev);
>> +     if (IS_ERR_OR_NULL(hsmp_pdev->mdev.this_device)) {
>> +             /*
>> +              * Register /dev/hsmp unparented. It is a singleton shared by all
>> +              * ACPI sockets and outlives all but the last of them, so
>> +              * parenting it to this socket's device would leave a dangling
>> +              * parent once that socket is unbound.
>> +              */
>> +             ret = hsmp_misc_register(NULL);
>>                if (ret) {
>>                        dev_err(&pdev->dev, "Failed to register misc device\n");
>> +                     hsmp_acpi_probe_failure_cleanup(&pdev->dev);
>>                        return ret;
>>                }
>> -             hsmp_pdev->is_probed = true;
>> -             dev_dbg(&pdev->dev, "AMD HSMP ACPI is probed successfully\n");
>> +             dev_dbg(&pdev->dev, "AMD HSMP ACPI misc device registered\n");
>>        }
>>
>> +     hsmp_acpi_sock_refs++;
>> +
>>        return 0;
>>   }
>>
>>   static void hsmp_acpi_remove(struct platform_device *pdev)
>>   {
>> +     struct hsmp_socket *sock = dev_get_drvdata(&pdev->dev);
>> +
>> +     /*
>> +      * Serialize the decrement and any release it triggers against a
>> +      * concurrent probe, and drain the lock-free data plane for the whole
>> +      * teardown: this covers the per-socket unbind, whose mailbox devres
>> +      * unmaps once we return, and the last unbind that frees the socket
>> +      * array in hsmp_acpi_sock_release().
>> +      */
>>        guard(rwsem_write)(&hsmp_sock_rwsem);
>>
>>        /*
>> -      * We register only one misc_device even on multi-socket system.
>> -      * So, deregister should happen only once.
>> +      * Clear this socket's dev so hsmp_send_message() rejects it before
>> +      * devres unmaps the mailbox. On a non-final unbind the socket array
>> +      * stays alive, so without this a later message to this index would
>> +      * reach an unmapped iomem region.
>>         */
>> -     if (hsmp_pdev->is_probed) {
>> -             hsmp_misc_deregister();
>> -             hsmp_destroy_metric_read_locks(hsmp_pdev);
>> -             hsmp_pdev->is_probed = false;
>> -     }
>> +     sock->dev = NULL;
>> +
>> +     hsmp_acpi_sock_refs--;
>> +     if (!hsmp_acpi_sock_refs)
> 
> Now that I can actually follow the series this far (pretty easily
> actually, so good work so far!), I again started to wonder why this has
> moved back away from kref to manually handling the reference counting?
> 
> I understand you don't strictly need the atomic part of refcount_t because
> you're under another lock but it would still be cleaner interface with
> kref_get/put().
> 
> It might even be possible to use kref_get_unless_zero() instead of the
> read side of hsmp_sock_rwsem to ensure datastructures won't vanish
> underneath a data place call.
> 
Thanks for your input. I will adress the above suggestion and send next 
series.

>> +             hsmp_acpi_sock_release();
>>   }
>>
>>   static struct platform_driver amd_hsmp_driver = {
>> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
>> index 584fd9b1d31f..967307abe641 100644
>> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
>> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
>> @@ -491,6 +491,18 @@ int hsmp_get_tbl_dram_base(u16 sock_ind)
>>                dev_err(sock->dev, "Invalid DRAM address for metric table\n");
>>                return -ENOMEM;
>>        }
>> +     /*
>> +      * The ACPI socket array is shared across sockets and outlives a
>> +      * per-socket unbind, so metric_tbl_addr may hold a mapping from an
>> +      * earlier bind of this socket. Unmap it before remapping so an
>> +      * unbind/rebind cycle does not leak a metric-table mapping. This runs
>> +      * during probe before the metric sysfs attribute is exposed, so no
>> +      * reader can be using it.
>> +      */
>> +     if (sock->metric_tbl_addr) {
>> +             iounmap(sock->metric_tbl_addr);
>> +             sock->metric_tbl_addr = NULL;
>> +     }
>>        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");
>> @@ -528,6 +540,14 @@ int hsmp_misc_register(struct device *dev)
>>        hsmp_pdev.mdev.name     = HSMP_CDEV_NAME;
>>        hsmp_pdev.mdev.minor    = MISC_DYNAMIC_MINOR;
>>        hsmp_pdev.mdev.fops     = &hsmp_fops;
>> +     /*
>> +      * The caller chooses the parent. The platform driver has a single
>> +      * device whose lifetime matches /dev/hsmp and parents it there. The
>> +      * ACPI driver passes NULL: its /dev/hsmp is a singleton shared by
>> +      * per-socket devices that can be unbound individually and out of order,
>> +      * so parenting it to one would leave it attached to an already-removed
>> +      * device.
>> +      */
>>        hsmp_pdev.mdev.parent   = dev;
>>        hsmp_pdev.mdev.nodename = HSMP_DEVNODE_NAME;
>>        hsmp_pdev.mdev.mode     = 0644;
>> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
>> index ec92c2a429bb..45dab9253c13 100644
>> --- a/drivers/platform/x86/amd/hsmp/hsmp.h
>> +++ b/drivers/platform/x86/amd/hsmp/hsmp.h
>> @@ -58,7 +58,6 @@ struct hsmp_plat_device {
>>        struct hsmp_socket *sock;
>>        u32 proto_ver;
>>        u16 num_sockets;
>> -     bool is_probed;
>>   };
>>
>>   int hsmp_cache_proto_ver(u16 sock_ind);
>>
> 
> --
>   i.
>