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

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

On 7/7/2026 5:43 PM, Ilpo Järvinen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> On Tue, 7 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
>> global 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.  Clear
>>     mdev.this_device in hsmp_misc_deregister() so a later re-probe does not
>>     skip registration on a stale pointer.
>>
>>   - hsmp_acpi_sock_refs counts the sockets that have probed successfully.
>>     It is guarded by hsmp_acpi_probe_mutex, 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.  Both the remove path and the
>> probe-failure path call it once they are the last owner, so the teardown
>> lives in one place.
>>
>> Both paths also clear this socket's dev before devres unmaps the mailbox,
>> 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 an unmapped mailbox.
>>
>> Two lifetime fixes fall out of the array now 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.
>>
>>   - /dev/hsmp is no longer parented to a per-socket device.  Sockets 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.  Register it unparented instead.
>>
>> This provides the refcounted socket ownership the ACPI teardown needs;
>> the following patch wires both teardown paths into the data-plane rwsem so
>> they are drained against the lock-free data plane, nested inside
>> hsmp_acpi_probe_mutex.
>>
>> Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
>> ---
>>   drivers/platform/x86/amd/hsmp/acpi.c | 103 ++++++++++++++++++++++-----
>>   drivers/platform/x86/amd/hsmp/hsmp.c |  26 ++++++-
>>   drivers/platform/x86/amd/hsmp/hsmp.h |   1 -
>>   3 files changed, 112 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
>> index 16edcf51c971..11a478503fa7 100644
>> --- a/drivers/platform/x86/amd/hsmp/acpi.c
>> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
>> @@ -44,6 +44,13 @@ static struct hsmp_plat_device *hsmp_pdev;
>>
>>   static DEFINE_MUTEX(hsmp_acpi_probe_mutex);
>>
>> +/*
>> + * Number of ACPI socket platform devices that have probed successfully.
>> + * Guarded by hsmp_acpi_probe_mutex; 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;
> 
> That mutex should say what it protects, though I think you can probably
> just add one comment for this, which covers both followed by the mutex and
> this variable.
> 
Sure will clarify in the comments.
>> +
>>   struct hsmp_sys_attr {
>>        struct device_attribute dattr;
>>        u32 msg_id;
>> @@ -610,6 +617,56 @@ 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_acpi_probe_mutex held (serializing it against a concurrent
>> + * probe).  Coordination with the lock-free data plane (draining in-flight
>> + * hsmp_send_message() before the mailbox is unmapped and the array is freed)
>> + * is added in a subsequent patch via the data-plane rwsem.
>> + */
>> +static void hsmp_acpi_sock_release(void)
>> +{
>> +     lockdep_assert_held(&hsmp_acpi_probe_mutex);
>> +
>> +     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.
>> + */
>> +static void hsmp_acpi_probe_failure_cleanup(struct device *dev)
>> +{
>> +     struct hsmp_socket *sock = dev_get_drvdata(dev);
>> +
>> +     lockdep_assert_held(&hsmp_acpi_probe_mutex);
>> +
>> +     if (sock)
> 
> Is this needed?
> 
>> +             sock->dev = NULL;
>> +
>> +     if (!hsmp_acpi_sock_refs)
>> +             hsmp_acpi_sock_release();
>> +}
>> +
>>   static int hsmp_acpi_probe(struct platform_device *pdev)
>>   {
>>        int ret;
>> @@ -620,16 +677,16 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
>>
>>        guard(mutex)(&hsmp_acpi_probe_mutex);
>>
>> -     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;
>>
>> @@ -639,35 +696,49 @@ 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) {
>> +     if (IS_ERR_OR_NULL(hsmp_pdev->mdev.this_device)) {
>>                ret = hsmp_misc_register(&pdev->dev);
>>                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)
>>   {
>> -     mutex_lock(&hsmp_acpi_probe_mutex);
>> +     struct hsmp_socket *sock = dev_get_drvdata(&pdev->dev);
>> +
>>        /*
>> -      * We register only one misc_device even on multi-socket system.
>> -      * So, deregister should happen only once.
>> +      * Serialize the decrement (and the release it may trigger) against a
>> +      * concurrent probe so the count cannot be revived from zero.
>>         */
>> -     if (hsmp_pdev->is_probed) {
>> -             hsmp_misc_deregister();
>> -             hsmp_destroy_metric_read_locks(hsmp_pdev);
>> -             hsmp_pdev->is_probed = false;
>> -     }
>> -     mutex_unlock(&hsmp_acpi_probe_mutex);
>> +     guard(mutex)(&hsmp_acpi_probe_mutex);
> 
> So here you convert it, please do it with guard() right from the start.
> 
You are right. I missed this and will address in the next version.
>> +
>> +     /*
>> +      * Clear this socket's dev so hsmp_send_message() rejects it before
>> +      * touching the mailbox that devres is about to unmap.  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.
>> +      *
>> +      * This teardown is drained against the lock-free data plane in a
>> +      * subsequent patch that wires it into the data-plane rwsem.
>> +      */
>> +     if (sock)
> 
> Can this ever be not true?
> 
>> +             sock->dev = NULL;
>> +
>> +     if (!--hsmp_acpi_sock_refs)
> 
> This would be simpler form (does not require thinking unlike !-- construct):
> 
>          hsmp_acpi_sock_refs--;
>          if (!hsmp_acpi_sock_refs)
> 
Thanks for the input. I will update.

>> +             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 c3d0e64c415d..8440671235de 100644
>> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
>> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
>> @@ -492,6 +492,17 @@ 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 now persists across a non-final unbind, so a
>> +      * stale mapping from a previous bind can still be recorded here.  Drop
> 
> "a stale mapping from a previous bind can still be recorded here" sounds
> odd, and honestly I cannot fully grasp the meaning of it. I guess it
> revolves around what does "record" mean in this context.
> 
> Are you saying that without this change, we could encounter a stale
> mapping? (which is again history telling more than documenting what the
> current code does, and it is confusing obviously because how code behaved
> in the past doesn't match the behavior now.)
> 
> One space is enough.
> 
>> +      * it before remapping to avoid leaking one metric-table mapping per
>> +      * unbind/rebind cycle.  This runs during (re)probe before the metric
> 
> Ditto.
> 
>> +      * 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");
>> @@ -529,7 +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;
>> -     hsmp_pdev.mdev.parent   = dev;
>> +     /*
>> +      * /dev/hsmp is a singleton shared by all sockets and, on ACPI, is torn
> 
> Remove: on ACPI.
> 
>> +      * down only on the last socket's unbind.  Do not parent it to a single
> 
> Remove: only
> 
>> +      * per-socket device: those can be unbound individually and out of order,
>> +      * which would leave the misc device parented to an already-removed
>> +      * device.  Leave it unparented so its lifetime is independent.
>> +      */
>> +     hsmp_pdev.mdev.parent   = NULL;
>>        hsmp_pdev.mdev.nodename = HSMP_DEVNODE_NAME;
>>        hsmp_pdev.mdev.mode     = 0644;
>>
>> @@ -540,6 +558,12 @@ EXPORT_SYMBOL_NS_GPL(hsmp_misc_register, "AMD_HSMP");
>>   void hsmp_misc_deregister(void)
>>   {
>>        misc_deregister(&hsmp_pdev.mdev);
>> +     /*
>> +      * misc_deregister() leaves mdev.this_device pointing at the now
>> +      * destroyed device.  Clear it so a subsequent re-probe does not skip
>> +      * registration on a stale pointer.
>> +      */
>> +     hsmp_pdev.mdev.this_device = NULL;
> 
> I wonder how hard it would be to just make hsmp_pdev a pointer so the
> struct could be dynamically allocated/freed instead.
> 
> In any case, this looks like an independent problem/fix AFAICT.
> 
I will fix this in a seperate patch.

>>   }
>>   EXPORT_SYMBOL_NS_GPL(hsmp_misc_deregister, "AMD_HSMP");
>>
>> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
>> index 71a8b202aa92..9036cb221635 100644
>> --- a/drivers/platform/x86/amd/hsmp/hsmp.h
>> +++ b/drivers/platform/x86/amd/hsmp/hsmp.h
>> @@ -57,7 +57,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.
>