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

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

[Muralidhara M K]

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.

A single rw_semaphore, hsmp_sock_rwsem, serializes everything: the data
plane takes it for read so messages run concurrently, and probe and remove
take it for write to bring sockets up and tear them down while excluding and
draining the data plane. The fix is built up in small, bisectable steps:

 1. Introduce hsmp_sock_rwsem and hold it for write across ACPI probe/remove
    so concurrent per-socket probes cannot race the bring-up handshake or
    the one-time socket-array allocation.
 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 on the
    ACPI path.
 6. Add the read side of hsmp_sock_rwsem to the data plane: split the send
    into hsmp_send_message_locked() (asserts the rwsem is held) and
    hsmp_send_message() (wraps it in guard(rwsem_read)). Route the probe-only
    senders through the locked variant so probe, holding the write lock, does
    not recurse on the rwsem.

Each patch builds on its own and the series is checkpatch --strict clean.

Changes since v4:
 - Collapse the two-lock design onto a single rwsem, as suggested on v4.
   The dedicated ACPI probe mutex is gone; probe and remove take
   hsmp_sock_rwsem for write instead, so there is one lock rather than a
   probe mutex nested outside the data-plane rwsem.
 - Add hsmp_send_message_locked(), the send core that asserts the rwsem is
   held, and reduce hsmp_send_message() to guard(rwsem_read) around it. The
   probe-only senders (hsmp_test(), hsmp_cache_proto_ver(),
   hsmp_get_tbl_dram_base()) call the locked variant, so the probe path can
   hold the rwsem for write across the whole mailbox handshake without
   recursing on it.
 - plat.c takes the rwsem for write around init_platform_device() only, not
   across devm_add_action_or_reset(), so the release action (which also
   takes it for write) cannot deadlock if that registration fails.
 - hsmp_misc_register() now takes the /dev/hsmp parent from its caller: the
   ACPI driver passes NULL (shared singleton, per-socket devices unbind out
   of order), while the platform driver keeps parenting to its single
   device. v4 unparented it unconditionally, which also dropped the parent
   for the platform path.
 - Reword the patch 2 changelog and the devres release comment around the
   "devm-managed" split; minor comment cleanups.

Muralidhara M K (6):
  platform/x86/amd/hsmp: Serialize ACPI HSMP probe and remove with an
    rwsem
  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 | 137 ++++++++++++++++++++++++---
 drivers/platform/x86/amd/hsmp/hsmp.c | 113 ++++++++++++++++++++--
 drivers/platform/x86/amd/hsmp/hsmp.h |  14 ++-
 drivers/platform/x86/amd/hsmp/plat.c |  38 +++++++-
 4 files changed, 279 insertions(+), 23 deletions(-)


base-commit: ff7836fa850c2f815bc219f1e48f6ec8699f4ae7
-- 
2.34.1

[PATCH v5 1/6] platform/x86/amd/hsmp: Serialize ACPI HSMP probe and remove with an rwsem

[Muralidhara M K]

Add hsmp_sock_rwsem and export it, then hold it for write across ACPI
probe, remove and init_acpi() so concurrent per-socket platform probes
cannot race the is_probed handshake or the one-time socket-array
allocation. Use lockdep_assert_held_write() in init_acpi() to catch
incorrect locking under lockdep.

An rw_semaphore is used rather than a plain mutex because an upcoming
change adds a read side so the lock-free data plane runs concurrently with
itself while probe/remove hold it for write to drain in-flight messages.
Introducing it as an rwsem now keeps the lock type stable across that
change.

Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
---
 drivers/platform/x86/amd/hsmp/acpi.c | 22 ++++++++++++++++++++++
 drivers/platform/x86/amd/hsmp/hsmp.c |  9 +++++++++
 drivers/platform/x86/amd/hsmp/hsmp.h |  7 +++++++
 3 files changed, 38 insertions(+)

diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
index 72f68cef1297..a23797bd1dd5 100644
--- a/drivers/platform/x86/amd/hsmp/acpi.c
+++ b/drivers/platform/x86/amd/hsmp/acpi.c
@@ -15,12 +15,15 @@
 #include <linux/array_size.h>
 #include <linux/bits.h>
 #include <linux/bitfield.h>
+#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/dev_printk.h>
 #include <linux/ioport.h>
 #include <linux/kstrtox.h>
+#include <linux/lockdep.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/rwsem.h>
 #include <linux/string.h>
 #include <linux/sysfs.h>
 #include <linux/topology.h>
@@ -482,11 +485,20 @@ static ssize_t hsmp_freq_limit_source_show(struct device *dev, struct device_att
 	return len;
 }
 
+/*
+ * Bring up one ACPI HSMP socket: parse its ACPI table, run the mailbox
+ * handshake and register its sysfs/hwmon interfaces.
+ *
+ * Called with hsmp_sock_rwsem held for write by hsmp_acpi_probe(), so the
+ * per-socket bring-up cannot race a concurrent probe or remove.
+ */
 static int init_acpi(struct device *dev)
 {
 	u16 sock_ind;
 	int ret;
 
+	lockdep_assert_held_write(&hsmp_sock_rwsem);
+
 	ret = hsmp_get_uid(dev, &sock_ind);
 	if (ret)
 		return ret;
@@ -607,6 +619,14 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
 	if (!hsmp_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.
+	 */
+	guard(rwsem_write)(&hsmp_sock_rwsem);
+
 	if (!hsmp_pdev->is_probed) {
 		hsmp_pdev->num_sockets = topology_max_packages();
 		if (!hsmp_pdev->num_sockets) {
@@ -642,6 +662,8 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
 
 static void hsmp_acpi_remove(struct platform_device *pdev)
 {
+	guard(rwsem_write)(&hsmp_sock_rwsem);
+
 	/*
 	 * We register only one misc_device even on multi-socket system.
 	 * So, deregister should happen only once.
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
index 1a87931136fd..e9c17698983c 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/rwsem.h>
 #include <linux/semaphore.h>
 #include <linux/sysfs.h>
 
@@ -40,6 +41,14 @@
 
 static struct hsmp_plat_device hsmp_pdev;
 
+/*
+ * Serializes AMD HSMP socket bring-up and teardown: ACPI probe and remove take
+ * it for write so concurrent per-socket probes cannot race the is_probed
+ * handshake or the one-time socket-array allocation.
+ */
+DECLARE_RWSEM(hsmp_sock_rwsem);
+EXPORT_SYMBOL_NS_GPL(hsmp_sock_rwsem, "AMD_HSMP");
+
 /*
  * Send a message to the HSMP port via PCI-e config space registers
  * or by writing to MMIO space.
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
index 0509a442eaae..129200d0cf81 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.h
+++ b/drivers/platform/x86/amd/hsmp/hsmp.h
@@ -16,6 +16,7 @@
 #include <linux/kconfig.h>
 #include <linux/miscdevice.h>
 #include <linux/pci.h>
+#include <linux/rwsem.h>
 #include <linux/semaphore.h>
 #include <linux/sysfs.h>
 
@@ -71,4 +72,10 @@ int hsmp_create_sensor(struct device *dev, u16 sock_ind);
 static inline int hsmp_create_sensor(struct device *dev, u16 sock_ind) { return 0; }
 #endif
 int hsmp_msg_get_nargs(u16 sock_ind, u32 msg_id, u32 *data, u8 num_args);
+
+/*
+ * Serializes HSMP socket bring-up and teardown. ACPI probe and remove take it
+ * for write.
+ */
+extern struct rw_semaphore hsmp_sock_rwsem;
 #endif /* HSMP_H */
-- 
2.34.1

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

[Muralidhara M K]

The metric-table DRAM region is mapped with devm_ioremap(), which ties the
mapping to the socket device's devres scope. An upcoming change lets the
ACPI front-end share the socket array across sockets and run its own
coordinated teardown, so the mapping can no longer be pinned to a single
per-socket devres scope.

Map it with plain ioremap() instead and add hsmp_unmap_metric_tbls(), which
drops every socket's metric_tbl_addr mapping. The platform driver registers
that helper with devm_add_action_or_reset() so the mappings are released on
both remove and probe failure, while the socket array itself stays
devm-managed.

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 | 17 +++++++++++++++++
 3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
index e9c17698983c..008f3c0b2ad7 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/rwsem.h>
 #include <linux/semaphore.h>
 #include <linux/sysfs.h>
@@ -423,6 +424,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)
+{
+	struct hsmp_socket *sock;
+	u16 i;
+
+	for (i = 0; i < pdev->num_sockets; i++) {
+		sock = &pdev->sock[i];
+		if (sock->metric_tbl_addr) {
+			iounmap(sock->metric_tbl_addr);
+			sock->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];
@@ -447,8 +463,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 129200d0cf81..b0d67b93363d 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.h
+++ b/drivers/platform/x86/amd/hsmp/hsmp.h
@@ -64,6 +64,7 @@ long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg);
 void hsmp_misc_deregister(void);
 int hsmp_misc_register(struct device *dev);
 int hsmp_get_tbl_dram_base(u16 sock_ind);
+void hsmp_unmap_metric_tbls(struct hsmp_plat_device *pdev);
 ssize_t hsmp_metric_tbl_read(struct hsmp_socket *sock, char *buf, size_t size);
 struct hsmp_plat_device *get_hsmp_pdev(void);
 #if IS_ENABLED(CONFIG_HWMON)
diff --git a/drivers/platform/x86/amd/hsmp/plat.c b/drivers/platform/x86/amd/hsmp/plat.c
index e07f68575055..b5f2120765c8 100644
--- a/drivers/platform/x86/amd/hsmp/plat.c
+++ b/drivers/platform/x86/amd/hsmp/plat.c
@@ -201,6 +201,19 @@ static int init_platform_device(struct device *dev)
 	return 0;
 }
 
+/*
+ * The socket array is devm-managed and freed by the driver core, but the
+ * metric-table DRAM regions are mapped with plain ioremap() during probe and
+ * are therefore not covered by devres.
+ *
+ * Drop those mappings from a devres action so both remove and probe failure
+ * unmap them exactly once, before the socket array they refer to is freed.
+ */
+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 +224,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");
-- 
2.34.1

[PATCH v5 3/6] platform/x86/amd/hsmp: Serialize per-socket metric table reads with a mutex

[Muralidhara M K]

HSMP_GET_METRIC_TABLE makes the firmware refill a shared per-socket metric
DRAM region, which hsmp_metric_tbl_read() then copies out with
memcpy_fromio(). Two concurrent readers of the metrics_bin sysfs attribute
on the same socket can race: one can trigger a fresh fill while the other
is mid-copy and return a torn snapshot. (The hwmon path does not touch
this region; it only issues power messages via hsmp_send_message().)

Embed a struct mutex metric_read_lock in each hsmp_socket and hold it
across the fill-and-copy in hsmp_metric_tbl_read(). Add
hsmp_init_metric_read_locks() and hsmp_destroy_metric_read_locks(), which
take only struct hsmp_plat_device and iterate pdev->sock[] over
pdev->num_sockets so the caller cannot pass a count that disagrees with the
array.

Wire them into both front-ends' probe and teardown paths so the mutex is
always initialized before metrics_bin is exposed: the platform driver and
the ACPI driver both drive hsmp_metric_tbl_read() through the same 0444
metrics_bin attribute. Doing this in one patch avoids a bisection point
where an ACPI read would lock an uninitialized mutex.

Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
---
 drivers/platform/x86/amd/hsmp/acpi.c |  3 +++
 drivers/platform/x86/amd/hsmp/hsmp.c | 28 ++++++++++++++++++++++++++++
 drivers/platform/x86/amd/hsmp/hsmp.h |  5 +++++
 drivers/platform/x86/amd/hsmp/plat.c |  3 +++
 4 files changed, 39 insertions(+)

diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
index a23797bd1dd5..a092d7589bcb 100644
--- a/drivers/platform/x86/amd/hsmp/acpi.c
+++ b/drivers/platform/x86/amd/hsmp/acpi.c
@@ -639,6 +639,8 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
 					       GFP_KERNEL);
 		if (!hsmp_pdev->sock)
 			return -ENOMEM;
+
+		hsmp_init_metric_read_locks(hsmp_pdev);
 	}
 
 	ret = init_acpi(&pdev->dev);
@@ -670,6 +672,7 @@ static void hsmp_acpi_remove(struct platform_device *pdev)
 	 */
 	if (hsmp_pdev->is_probed) {
 		hsmp_misc_deregister();
+		hsmp_destroy_metric_read_locks(hsmp_pdev);
 		hsmp_pdev->is_probed = false;
 	}
 }
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
index 008f3c0b2ad7..fd36c8f142c0 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp/hsmp.c
@@ -10,9 +10,11 @@
 #include <asm/amd/hsmp.h>
 
 #include <linux/acpi.h>
+#include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/io.h>
+#include <linux/mutex.h>
 #include <linux/rwsem.h>
 #include <linux/semaphore.h>
 #include <linux/sysfs.h>
@@ -415,6 +417,14 @@ ssize_t hsmp_metric_tbl_read(struct hsmp_socket *sock, char *buf, size_t size)
 	msg.msg_id	= HSMP_GET_METRIC_TABLE;
 	msg.sock_ind	= sock->sock_ind;
 
+	/*
+	 * HSMP_GET_METRIC_TABLE makes firmware refill this socket's shared
+	 * metric DRAM region, which is then copied out below.  Hold the
+	 * per-socket lock across the fill-and-copy so concurrent readers of the
+	 * same socket cannot return a torn snapshot.
+	 */
+	guard(mutex)(&sock->metric_read_lock);
+
 	ret = hsmp_send_message(&msg);
 	if (ret)
 		return ret;
@@ -424,6 +434,24 @@ 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_init_metric_read_locks(struct hsmp_plat_device *pdev)
+{
+	u16 i;
+
+	for (i = 0; i < pdev->num_sockets; i++)
+		mutex_init(&pdev->sock[i].metric_read_lock);
+}
+EXPORT_SYMBOL_NS_GPL(hsmp_init_metric_read_locks, "AMD_HSMP");
+
+void hsmp_destroy_metric_read_locks(struct hsmp_plat_device *pdev)
+{
+	u16 i;
+
+	for (i = 0; i < pdev->num_sockets; i++)
+		mutex_destroy(&pdev->sock[i].metric_read_lock);
+}
+EXPORT_SYMBOL_NS_GPL(hsmp_destroy_metric_read_locks, "AMD_HSMP");
+
 void hsmp_unmap_metric_tbls(struct hsmp_plat_device *pdev)
 {
 	struct hsmp_socket *sock;
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
index b0d67b93363d..ec92c2a429bb 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.h
+++ b/drivers/platform/x86/amd/hsmp/hsmp.h
@@ -15,6 +15,7 @@
 #include <linux/hwmon.h>
 #include <linux/kconfig.h>
 #include <linux/miscdevice.h>
+#include <linux/mutex.h>
 #include <linux/pci.h>
 #include <linux/rwsem.h>
 #include <linux/semaphore.h>
@@ -44,6 +45,8 @@ struct hsmp_socket {
 	void __iomem *metric_tbl_addr;
 	void __iomem *virt_base_addr;
 	struct semaphore hsmp_sem;
+	/* Serializes HSMP_GET_METRIC_TABLE fill-and-copy for this socket */
+	struct mutex metric_read_lock;
 	char name[HSMP_ATTR_GRP_NAME_SIZE];
 	struct device *dev;
 	u16 sock_ind;
@@ -65,6 +68,8 @@ void hsmp_misc_deregister(void);
 int hsmp_misc_register(struct device *dev);
 int hsmp_get_tbl_dram_base(u16 sock_ind);
 void hsmp_unmap_metric_tbls(struct hsmp_plat_device *pdev);
+void hsmp_init_metric_read_locks(struct hsmp_plat_device *pdev);
+void hsmp_destroy_metric_read_locks(struct hsmp_plat_device *pdev);
 ssize_t hsmp_metric_tbl_read(struct hsmp_socket *sock, char *buf, size_t size);
 struct hsmp_plat_device *get_hsmp_pdev(void);
 #if IS_ENABLED(CONFIG_HWMON)
diff --git a/drivers/platform/x86/amd/hsmp/plat.c b/drivers/platform/x86/amd/hsmp/plat.c
index b5f2120765c8..7a16d1ab463b 100644
--- a/drivers/platform/x86/amd/hsmp/plat.c
+++ b/drivers/platform/x86/amd/hsmp/plat.c
@@ -212,6 +212,7 @@ static int init_platform_device(struct device *dev)
 static void hsmp_pltdrv_release(void *data)
 {
 	hsmp_unmap_metric_tbls(hsmp_pdev);
+	hsmp_destroy_metric_read_locks(hsmp_pdev);
 }
 
 static int hsmp_pltdrv_probe(struct platform_device *pdev)
@@ -224,6 +225,8 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
 	if (!hsmp_pdev->sock)
 		return -ENOMEM;
 
+	hsmp_init_metric_read_locks(hsmp_pdev);
+
 	ret = devm_add_action_or_reset(&pdev->dev, hsmp_pltdrv_release, NULL);
 	if (ret)
 		return ret;
-- 
2.34.1

[PATCH v5 4/6] platform/x86/amd/hsmp: Clear mdev.this_device on deregister

[Muralidhara M K]

misc_deregister() destroys the device but leaves miscdevice.this_device
pointing at the freed struct device. Clear it so any later check of
this_device, and a subsequent re-register, does not observe a stale
pointer. An upcoming change uses this_device to track whether /dev/hsmp is
registered across the shared ACPI sockets and relies on it being NULL after
deregister.

Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
---
 drivers/platform/x86/amd/hsmp/hsmp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
index fd36c8f142c0..584fd9b1d31f 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp/hsmp.c
@@ -539,6 +539,7 @@ EXPORT_SYMBOL_NS_GPL(hsmp_misc_register, "AMD_HSMP");
 void hsmp_misc_deregister(void)
 {
 	misc_deregister(&hsmp_pdev.mdev);
+	hsmp_pdev.mdev.this_device = NULL;
 }
 EXPORT_SYMBOL_NS_GPL(hsmp_misc_deregister, "AMD_HSMP");
 
-- 
2.34.1

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

[Muralidhara M K]

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)
+		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);
-- 
2.34.1

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

[Ilpo Järvinen]

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.

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

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

[Muralidhara M K]

The HSMP data plane is lock-free: open /dev/hsmp fds and hwmon sysfs reads
call hsmp_send_message() without any coordination with driver teardown.
misc_deregister() does not drain already-open fds, so an in-flight message
can race a concurrent unbind and touch a freed socket array or an unmapped
mailbox.

Add the read side of hsmp_sock_rwsem to the data plane. Split the message
send into hsmp_send_message_locked(), which does the bounds check and MMIO
access and asserts the rwsem is held, and hsmp_send_message(), which wraps
it in guard(rwsem_read). Probe and remove hold the rwsem for write, so they
drain in-flight messages and keep new ones out while they tear a socket
down.

The probe-time senders run under the probe write lock and so must not take
the rwsem again: route hsmp_test(), hsmp_cache_proto_ver() and
hsmp_get_tbl_dram_base() through hsmp_send_message_locked() to avoid
recursive locking. A single rwsem therefore covers both the data plane and
the probe/remove handshake, with no separate probe lock:

 - acpi.c already holds it for write across probe for the socket-array and
   misc-registration handshake, so the mailbox handshake now nests under
   that same lock.

 - plat.c takes it for write around init_platform_device(). It is not held
   across devm_add_action_or_reset() so the release action, which also
   takes it for write, cannot deadlock if that registration fails.

Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
---
 drivers/platform/x86/amd/hsmp/hsmp.c | 42 ++++++++++++++++++++++------
 drivers/platform/x86/amd/hsmp/hsmp.h |  4 +--
 drivers/platform/x86/amd/hsmp/plat.c | 24 +++++++++++++---
 3 files changed, 56 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
index 967307abe641..11206138a15c 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp/hsmp.c
@@ -45,9 +45,12 @@
 static struct hsmp_plat_device hsmp_pdev;
 
 /*
- * Serializes AMD HSMP socket bring-up and teardown: ACPI probe and remove take
- * it for write so concurrent per-socket probes cannot race the is_probed
- * handshake or the one-time socket-array allocation.
+ * Gates the AMD HSMP data plane against socket bring-up and teardown.
+ *
+ * hsmp_send_message() takes it for read, so open /dev/hsmp fds and hwmon reads
+ * run concurrently. Probe and remove take it for write: probe brings sockets
+ * up (running the mailbox handshake via hsmp_send_message_locked()) and remove
+ * tears them down, both excluding and draining the data plane.
  */
 DECLARE_RWSEM(hsmp_sock_rwsem);
 EXPORT_SYMBOL_NS_GPL(hsmp_sock_rwsem, "AMD_HSMP");
@@ -211,12 +214,20 @@ static int validate_message(struct hsmp_message *msg)
 	return 0;
 }
 
-int hsmp_send_message(struct hsmp_message *msg)
+/*
+ * Core message send. The caller must hold hsmp_sock_rwsem: the data plane
+ * takes it for read so many messages run concurrently, while the probe-time
+ * senders run under the write lock taken by probe. Holding it here serializes
+ * every message against socket teardown, which also holds it for write.
+ */
+static int hsmp_send_message_locked(struct hsmp_message *msg)
 {
 	struct hsmp_socket *sock;
 	unsigned int sock_ind;
 	int ret;
 
+	lockdep_assert_held(&hsmp_sock_rwsem);
+
 	if (!msg)
 		return -EINVAL;
 	ret = validate_message(msg);
@@ -243,7 +254,8 @@ int hsmp_send_message(struct hsmp_message *msg)
 	 * non-NULL dev also guarantees virt_base_addr, the mailbox offsets and
 	 * the semaphore are visible.
 	 *
-	 * Pairs with smp_store_release(&sock->dev) in hsmp_parse_acpi_table().
+	 * Held under hsmp_sock_rwsem; pairs with smp_store_release(&sock->dev)
+	 * in hsmp_parse_acpi_table().
 	 */
 	if (!smp_load_acquire(&sock->dev))
 		return -ENODEV;
@@ -258,6 +270,20 @@ int hsmp_send_message(struct hsmp_message *msg)
 
 	return ret;
 }
+
+int hsmp_send_message(struct hsmp_message *msg)
+{
+	/*
+	 * The data plane is lock-free: open /dev/hsmp fds and hwmon sysfs reads
+	 * issue messages without coordinating with driver teardown. Take
+	 * hsmp_sock_rwsem for read so messages run concurrently with each other
+	 * but are drained and kept out while probe/remove hold it for write to
+	 * tear a socket down.
+	 */
+	guard(rwsem_read)(&hsmp_sock_rwsem);
+
+	return hsmp_send_message_locked(msg);
+}
 EXPORT_SYMBOL_NS_GPL(hsmp_send_message, "AMD_HSMP");
 
 int hsmp_msg_get_nargs(u16 sock_ind, u32 msg_id, u32 *data, u8 num_args)
@@ -298,7 +324,7 @@ int hsmp_test(u16 sock_ind, u32 value)
 	msg.args[0]	= value;
 	msg.sock_ind	= sock_ind;
 
-	ret = hsmp_send_message(&msg);
+	ret = hsmp_send_message_locked(&msg);
 	if (ret)
 		return ret;
 
@@ -478,7 +504,7 @@ int hsmp_get_tbl_dram_base(u16 sock_ind)
 	msg.response_sz	= hsmp_msg_desc_table[HSMP_GET_METRIC_TABLE_DRAM_ADDR].response_sz;
 	msg.msg_id	= HSMP_GET_METRIC_TABLE_DRAM_ADDR;
 
-	ret = hsmp_send_message(&msg);
+	ret = hsmp_send_message_locked(&msg);
 	if (ret)
 		return ret;
 
@@ -521,7 +547,7 @@ int hsmp_cache_proto_ver(u16 sock_ind)
 	msg.sock_ind	= sock_ind;
 	msg.response_sz = hsmp_msg_desc_table[HSMP_GET_PROTO_VER].response_sz;
 
-	ret = hsmp_send_message(&msg);
+	ret = hsmp_send_message_locked(&msg);
 	if (!ret)
 		hsmp_pdev.proto_ver = msg.args[0];
 
diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
index 45dab9253c13..cfd1a8cbd459 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.h
+++ b/drivers/platform/x86/amd/hsmp/hsmp.h
@@ -79,8 +79,8 @@ static inline int hsmp_create_sensor(struct device *dev, u16 sock_ind) { return
 int hsmp_msg_get_nargs(u16 sock_ind, u32 msg_id, u32 *data, u8 num_args);
 
 /*
- * Serializes HSMP socket bring-up and teardown. ACPI probe and remove take it
- * for write.
+ * Gates the HSMP data plane: hsmp_send_message() takes it for read; probe and
+ * remove take it for write to bring sockets up and tear them down.
  */
 extern struct rw_semaphore hsmp_sock_rwsem;
 #endif /* HSMP_H */
diff --git a/drivers/platform/x86/amd/hsmp/plat.c b/drivers/platform/x86/amd/hsmp/plat.c
index 7a16d1ab463b..e9b2b809c0f5 100644
--- a/drivers/platform/x86/amd/hsmp/plat.c
+++ b/drivers/platform/x86/amd/hsmp/plat.c
@@ -13,12 +13,14 @@
 
 #include <linux/acpi.h>
 #include <linux/build_bug.h>
+#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/dev_printk.h>
 #include <linux/kconfig.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/rwsem.h>
 #include <linux/sysfs.h>
 
 #include <asm/amd/node.h>
@@ -204,15 +206,20 @@ static int init_platform_device(struct device *dev)
 /*
  * The socket array is devm-managed and freed by the driver core, but the
  * metric-table DRAM regions are mapped with plain ioremap() during probe and
- * are therefore not covered by devres.
+ * the per-socket mutexes need an explicit mutex_destroy(), neither of which
+ * devres covers.
  *
- * Drop those mappings from a devres action so both remove and probe failure
- * unmap them exactly once, before the socket array they refer to is freed.
+ * Take the data-plane rwsem for write to drain any in-flight
+ * hsmp_send_message(), unmap the metric tables, destroy the mutexes and drop
+ * the global socket pointer, all before devres frees the array. Registered as
+ * a devres action so it runs on both remove and probe failure.
  */
 static void hsmp_pltdrv_release(void *data)
 {
+	guard(rwsem_write)(&hsmp_sock_rwsem);
 	hsmp_unmap_metric_tbls(hsmp_pdev);
 	hsmp_destroy_metric_read_locks(hsmp_pdev);
+	hsmp_pdev->sock = NULL;
 }
 
 static int hsmp_pltdrv_probe(struct platform_device *pdev)
@@ -231,7 +238,16 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	ret = init_platform_device(&pdev->dev);
+	/*
+	 * init_platform_device() runs the mailbox handshake via the probe-only
+	 * senders, which issue messages through hsmp_send_message_locked() and
+	 * so require hsmp_sock_rwsem held. Hold it for write, matching probe's
+	 * role as a socket bring-up path. The lock is not held across
+	 * devm_add_action_or_reset() above so the release action, which also
+	 * takes it for write, does not deadlock if that registration fails.
+	 */
+	scoped_guard(rwsem_write, &hsmp_sock_rwsem)
+		ret = init_platform_device(&pdev->dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to init HSMP mailbox\n");
 		return ret;
-- 
2.34.1