Re: [PATCH v4 0/6] platform/x86/amd/hsmp: Serialize the data plane against socket teardown

[Ilpo Järvinen]

On Thu, 9 Jul 2026, M K, Muralidhara wrote:
> On 7/8/2026 3:37 PM, Ilpo Järvinen wrote:
> > On Wed, 8 Jul 2026, Muralidhara M K wrote:
> > 
> > > This series makes the AMD HSMP driver safe against concurrent probe/remove
> > > of its per-socket devices and against the lock-free data plane (open
> > > /dev/hsmp fds and hwmon/sysfs reads) racing socket teardown.
> > > 
> > > The ACPI front-end binds one platform device per socket but shares a
> > > single
> > > socket array and a single /dev/hsmp misc device across them, while the
> > > data
> > > plane issues mailbox messages with no coordination with driver teardown.
> > > misc_deregister() does not drain already-open fds, so an in-flight message
> > > can touch a freed socket array or an unmapped mailbox on unbind.
> > > 
> > > The fix is built up in small, bisectable steps:
> > > 
> > >   1. Serialize the ACPI probe/remove handshake with a dedicated mutex.
> > >   2. Map the metric table with ioremap() and release it via a devres
> > > action,
> > >      so its lifetime is no longer pinned to a single per-socket devres
> > > scope.
> > >   3. Serialize the per-socket metric-table fill-and-copy with a mutex.
> > >   4. Clear mdev.this_device on deregister (independent hygiene fix that
> > > the
> > >      next patch relies on to track /dev/hsmp registration).
> > >   5. Track shared socket ownership with a refcount and a single
> > > coordinated
> > >      release helper, drop the is_probed flag and unparent /dev/hsmp.
> > >   6. Add hsmp_sock_rwsem: the data plane holds it for read, a teardown
> > > path
> > >      holds it for write to drain in-flight messages before freeing the
> > > socket
> > >      array or unmapping the mailbox.
> > > 
> > > Each patch builds on its own and the series is checkpatch --strict clean.
> > > 
> > > Changes since v3:
> > >   - Use guard()/scoped locking consistently for both the probe mutex and
> > > the
> > >     data-plane rwsem, from the first patch that introduces each lock.
> > >   - Platform teardown now uses devm_add_action_or_reset(): the
> > > metric-table
> > >     unmap and the per-socket mutex destroy run from a single devres action
> > >     instead of explicit remove()/probe-failure code. The ACPI array is
> > > shared
> > >     across per-socket devices and must outlive an individual unbind, so it
> > >     stays on explicit teardown.
> > >   - Split the mdev.this_device clear into its own patch (new patch 4).
> > >   - Drop dead defensive checks: the !sock guard in
> > > hsmp_unmap_metric_tbls()
> > >     and the if (sock) in hsmp_acpi_remove(). Keep and document the one in
> > > the
> > >     probe-failure path, where sock can legitimately be NULL.
> > >   - Replace the !--refs construct with a plain decrement-then-test.
> > >   - Explain why the probe path uses a separate mutex rather than the rwsem
> > >     write side: the probe path itself drives the data plane via
> > > hsmp_test(),
> > >     which takes the rwsem for read, so holding it for write across probe
> > >     would deadlock.
> > 
> > Hi,
> > 
> > Could we have xx_locked() variant function for hsmp_test() to use so the
> > recursive locking problem is avoided?
> > 
> Yes, that works and let me drop hsmp_acpi_probe_mutex entirely and serialize
> the ACPI control plane on hsmp_sock_rwsem alone.
> 
> One clarification I am thinking of is: the recursion isn't only through
> hsmp_test(). init_acpi()/init_platform_device() also reach the data plane via
> hsmp_cache_proto_ver(), hsmp_get_tbl_dram_base() and the hwmon setup, all
> through hsmp_send_message(). So rather than a one-off hsmp_test_locked(), I'll
> split hsmp_send_message() into a hsmp_send_message_locked() core
> (bounds/nospec/dev check + per-socket semaphore + MMIO, no rwsem) and a thin
> hsmp_send_message() wrapper that takes the rwsem for read.

Yes, if that's the case.

> The probe-only
> senders call the _locked core, the probe path takes the rwsem for write, and
> hsmp_acpi_probe_mutex goes away.
> 
> The one behavioral change is the probe then holds the rwsem for write across
> the whole mailbox handshake (a few messages, up to HSMP_MSG_TIMEOUT each), so
> the data plane on already-probed sockets is blocked for that window. Probe is
> a boot/rare-rebind event so I think that's fine.

Yes, I think it's okay to reuse an existing rwsem for probe/remove. It 
just keeps things much simpler.

I'm actually expecting it to save us some trouble in future over separate 
locks exactly because we can now know the other sockets cannot race with 
the probe.

> I'll add
> lockdep_assert_held_write() in the locked core to keep the "rwsem already
> held" contract explicit.

Good.
 
> or Let me know your inputs ?
> 
> > >   - Comment cleanups: drop history and "patch N" references, drop
> > >     parenthetical function asides, single space after periods.
> > > 
> > > Muralidhara M K (6):
> > >    platform/x86/amd/hsmp: Serialize ACPI HSMP is_probed with a probe
> > >      mutex
> > >    platform/x86/amd/hsmp: Map the metric table with ioremap() and unmap
> > >      it explicitly
> > >    platform/x86/amd/hsmp: Serialize per-socket metric table reads with a
> > >      mutex
> > >    platform/x86/amd/hsmp: Clear mdev.this_device on deregister
> > >    platform/x86/amd/hsmp: ACPI HSMP refcounted sockets and coordinated
> > >      release
> > >    platform/x86/amd/hsmp: Serialize the data plane against socket
> > >      teardown
> > > 
> > >   drivers/platform/x86/amd/hsmp/acpi.c | 135 ++++++++++++++++++++++++---
> > >   drivers/platform/x86/amd/hsmp/hsmp.c | 107 ++++++++++++++++++++-
> > >   drivers/platform/x86/amd/hsmp/hsmp.h |  15 ++-
> > >   drivers/platform/x86/amd/hsmp/plat.c |  26 ++++++
> > >   4 files changed, 265 insertions(+), 18 deletions(-)
> > > 
> > > 
> > > base-commit: ff7836fa850c2f815bc219f1e48f6ec8699f4ae7
> > > 
> > 
> > --
> >   i.
> > 
> 

-- 
 i.