[PATCH v6 8/8] platform/x86/amd/hsmp: Serialize metric table access and ACPI HSMP teardown

[Muralidhara M K]

Each ACPI HSMP socket bumps acpi_sock_refcnt after a successful probe;
the last socket to remove runs refcount_dec_and_test() and tears down
shared state once. If init_acpi() or misc_register() fails before
refcount_inc(), call hsmp_acpi_free_sock_alloc_if_idle() to free
hsmp_pdev->sock so a concurrent probe cannot follow a dangling pointer.

1) Metric table sysfs reads ask the SMU to refresh the table, then
   memcpy_fromio() from the mmio mapping.  Concurrent readers could
   interleave those steps and return inconsistent data.  Serialize with
   per-socket metric_tbl_lock and guard(mutex) around
   hsmp_send_message() and memcpy_fromio() so each read() observes one
   coherent snapshot.
2) Every ACPI AMDI0097 device shares hsmp_pdev->sock[].  The driver used
   devm_kcalloc() from whichever device probed first, an unlocked probe
   flag, and misc_register() from the first successful path.  Parallel
   probes could race, and devm tied the array lifetime to one device
   while others (and devm hwmon paths) still called hsmp_send_message(),
   which could use-after-free the array on remove.
3) Metric DRAM used devm_ioremap() and devm_mutex_init() per ACPI device
   while the socket table is global, so devm teardown order did not
   match shared storage.  Switch to ioremap(), one-time mutex_init()
   (metric_lock_inited), and hsmp_metric_tbl_unmap_all() on the last
   ACPI remove together with the rest of global teardown.
4) After kcalloc(sock) but before refcount_inc(), one failing probe
   could kfree() the array while another probe still ran init_acpi() on
   the same pointer.  Hold hsmp_acpi_probe_mutex across init_acpi(),
   misc_register(), and refcount_inc() so setup does not overlap
   teardown or a concurrent probe.
5) On the legacy platform driver, call hsmp_misc_deregister() before
   hsmp_metric_tbl_unmap_all() so /dev/hsmp is gone before metric mmio
   is unmapped, matching the ACPI remove path.

Tested-by: Muthusamy Ramalingam <muthusamy.ramalingam@xxxxxxx>
Signed-off-by: Muralidhara M K <muralidhara.mk@xxxxxxx>
---
 drivers/platform/x86/amd/hsmp/acpi.c | 58 +++++++++++++++++++++-------
 drivers/platform/x86/amd/hsmp/hsmp.c | 56 +++++++++++++++++++++++++--
 drivers/platform/x86/amd/hsmp/hsmp.h |  9 ++++-
 drivers/platform/x86/amd/hsmp/plat.c |  1 +
 4 files changed, 106 insertions(+), 18 deletions(-)

diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
index 8dc6b4a8bd27..e526e2cea3e2 100644
--- a/drivers/platform/x86/amd/hsmp/acpi.c
+++ b/drivers/platform/x86/amd/hsmp/acpi.c
@@ -20,7 +20,10 @@
 #include <linux/ioport.h>
 #include <linux/kstrtox.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/platform_device.h>
+#include <linux/refcount.h>
+#include <linux/slab.h>
 #include <linux/sysfs.h>
 #include <linux/topology.h>
 #include <linux/uuid.h>
@@ -595,6 +598,19 @@ static const struct acpi_device_id amd_hsmp_acpi_ids[] = {
 };
 MODULE_DEVICE_TABLE(acpi, amd_hsmp_acpi_ids);
 
+static DEFINE_MUTEX(hsmp_acpi_probe_mutex);
+
+/* Caller holds hsmp_acpi_probe_mutex. */
+static void hsmp_acpi_free_sock_alloc_if_idle(void)
+{
+	if (refcount_read(&hsmp_pdev->acpi_sock_refcnt) == 0 &&
+	    !hsmp_pdev->acpi_misc_registered && hsmp_pdev->sock) {
+		kfree(hsmp_pdev->sock);
+		hsmp_pdev->sock = NULL;
+		hsmp_pdev->num_sockets = 0;
+	}
+}
+
 static int hsmp_acpi_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -603,49 +619,63 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
 	if (!hsmp_pdev)
 		return -ENOMEM;
 
-	if (!hsmp_pdev->is_probed) {
+	mutex_lock(&hsmp_acpi_probe_mutex);
+	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");
+			mutex_unlock(&hsmp_acpi_probe_mutex);
 			return -ENODEV;
 		}
 
-		hsmp_pdev->sock = devm_kcalloc(&pdev->dev, hsmp_pdev->num_sockets,
-					       sizeof(*hsmp_pdev->sock),
-					       GFP_KERNEL);
-		if (!hsmp_pdev->sock)
+		hsmp_pdev->sock = kcalloc(hsmp_pdev->num_sockets,
+					  sizeof(*hsmp_pdev->sock),
+					  GFP_KERNEL);
+		if (!hsmp_pdev->sock) {
+			mutex_unlock(&hsmp_acpi_probe_mutex);
 			return -ENOMEM;
+		}
 	}
 
 	ret = init_acpi(&pdev->dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to initialize HSMP interface.\n");
+		hsmp_acpi_free_sock_alloc_if_idle();
+		mutex_unlock(&hsmp_acpi_probe_mutex);
 		return ret;
 	}
 
-	if (!hsmp_pdev->is_probed) {
+	if (!hsmp_pdev->acpi_misc_registered) {
 		ret = hsmp_misc_register(&pdev->dev);
 		if (ret) {
 			dev_err(&pdev->dev, "Failed to register misc device\n");
+			hsmp_acpi_free_sock_alloc_if_idle();
+			mutex_unlock(&hsmp_acpi_probe_mutex);
 			return ret;
 		}
-		hsmp_pdev->is_probed = true;
-		dev_dbg(&pdev->dev, "AMD HSMP ACPI is probed successfully\n");
+		hsmp_pdev->acpi_misc_registered = true;
+		dev_dbg(&pdev->dev, "AMD HSMP ACPI misc device registered\n");
 	}
+	refcount_inc(&hsmp_pdev->acpi_sock_refcnt);
+	mutex_unlock(&hsmp_acpi_probe_mutex);
 
 	return 0;
 }
 
 static void hsmp_acpi_remove(struct platform_device *pdev)
 {
-	/*
-	 * We register only one misc_device even on multi-socket system.
-	 * So, deregister should happen only once.
-	 */
-	if (hsmp_pdev->is_probed) {
+	mutex_lock(&hsmp_acpi_probe_mutex);
+	if (refcount_dec_and_test(&hsmp_pdev->acpi_sock_refcnt)) {
 		hsmp_misc_deregister();
-		hsmp_pdev->is_probed = false;
+		hsmp_pdev->acpi_misc_registered = false;
+		hsmp_metric_tbl_unmap_all(hsmp_pdev, hsmp_pdev->num_sockets);
+		kfree(hsmp_pdev->sock);
+		hsmp_pdev->sock = NULL;
+		hsmp_pdev->num_sockets = 0;
+		hsmp_pdev->proto_ver = 0;
+		hsmp_pdev->hsmp_table_size = 0;
 	}
+	mutex_unlock(&hsmp_acpi_probe_mutex);
 }
 
 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 9f7a5fb67728..4dc3826a0d57 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/nospec.h>
 #include <linux/semaphore.h>
 #include <linux/slab.h>
@@ -41,7 +42,9 @@
  */
 #define CHECK_GET_BIT		BIT(31)
 
-static struct hsmp_plat_device hsmp_pdev;
+static struct hsmp_plat_device hsmp_pdev = {
+	.acpi_sock_refcnt = REFCOUNT_INIT(0),
+};
 
 /*
  * Send a message to the HSMP port via PCI-e config space registers
@@ -488,6 +491,7 @@ 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;
 
+	guard(mutex)(&sock->metric_tbl_lock);
 	ret = hsmp_send_message(&msg);
 	if (ret)
 		return ret;
@@ -497,6 +501,28 @@ 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_metric_tbl_unmap_all(struct hsmp_plat_device *pdev, u16 num_sockets)
+{
+	u16 i;
+
+	if (!pdev->sock || !num_sockets)
+		return;
+
+	for (i = 0; i < 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;
+		}
+		if (s->metric_lock_inited) {
+			mutex_destroy(&s->metric_tbl_lock);
+			s->metric_lock_inited = false;
+		}
+	}
+}
+EXPORT_SYMBOL_NS_GPL(hsmp_metric_tbl_unmap_all, "AMD_HSMP");
+
 int hsmp_get_tbl_dram_base(u16 sock_ind)
 {
 	struct hsmp_socket *sock = &hsmp_pdev.sock[sock_ind];
@@ -504,6 +530,31 @@ int hsmp_get_tbl_dram_base(u16 sock_ind)
 	phys_addr_t dram_addr;
 	int ret;
 
+	if (sock->metric_tbl_addr)
+		return 0;
+
+	/*
+	 * Initialize the per-socket lock before anything that can set
+	 * sock->metric_tbl_addr to a non-NULL value.  hsmp_metric_tbl_read()
+	 * gates on sock->metric_tbl_addr being non-NULL and then takes
+	 * metric_tbl_lock unconditionally; both callers of this function
+	 * (init_acpi() and init_platform_device()) intentionally only log
+	 * a failure here and continue probing, so an init order that left
+	 * metric_tbl_addr populated while mutex_init failed would leave the
+	 * read path locking an uninitialized mutex.  Doing the mutex init
+	 * first preserves the invariant "metric_tbl_addr != NULL implies the
+	 * lock is usable" on every error exit.
+	 *
+	 * Use non-devm mutex + ioremap so the shared sock[] array (ACPI
+	 * allocates once for all sockets) can be torn down from a single
+	 * refcounted release path without devm lifetime being tied to the
+	 * wrong struct device.
+	 */
+	if (!sock->metric_lock_inited) {
+		mutex_init(&sock->metric_tbl_lock);
+		sock->metric_lock_inited = true;
+	}
+
 	msg.sock_ind	= 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;
@@ -527,8 +578,7 @@ int hsmp_get_tbl_dram_base(u16 sock_ind)
 	else
 		hsmp_pdev.hsmp_table_size = sizeof(struct hsmp_metric_table);
 
-	sock->metric_tbl_addr = devm_ioremap(sock->dev, dram_addr,
-					     hsmp_pdev.hsmp_table_size);
+	sock->metric_tbl_addr = ioremap(dram_addr, hsmp_pdev.hsmp_table_size);
 	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 7343f8a1681f..5f5ad2885c22 100644
--- a/drivers/platform/x86/amd/hsmp/hsmp.h
+++ b/drivers/platform/x86/amd/hsmp/hsmp.h
@@ -15,7 +15,9 @@
 #include <linux/hwmon.h>
 #include <linux/kconfig.h>
 #include <linux/miscdevice.h>
+#include <linux/mutex.h>
 #include <linux/pci.h>
+#include <linux/refcount.h>
 #include <linux/semaphore.h>
 #include <linux/sysfs.h>
 #include <linux/types.h>
@@ -42,11 +44,14 @@ struct hsmp_socket {
 	struct bin_attribute hsmp_attr;
 	struct hsmp_mbaddr_info mbinfo;
 	void __iomem *metric_tbl_addr;
+	/* Serializes concurrent metric table refreshes from the sysfs path */
+	struct mutex metric_tbl_lock;
 	void __iomem *virt_base_addr;
 	struct semaphore hsmp_sem;
 	char name[HSMP_ATTR_GRP_NAME_SIZE];
 	struct device *dev;
 	u16 sock_ind;
+	bool metric_lock_inited;
 	int (*amd_hsmp_rdwr)(struct hsmp_socket *sock, u32 off, u32 *val, bool rw);
 };
 
@@ -55,7 +60,8 @@ struct hsmp_plat_device {
 	struct hsmp_socket *sock;
 	u32 proto_ver;
 	u16 num_sockets;
-	bool is_probed;
+	refcount_t acpi_sock_refcnt;
+	bool acpi_misc_registered;
 	size_t hsmp_table_size;
 };
 
@@ -66,6 +72,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_metric_tbl_unmap_all(struct hsmp_plat_device *pdev, u16 num_sockets);
 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..2ef2102d0af3 100644
--- a/drivers/platform/x86/amd/hsmp/plat.c
+++ b/drivers/platform/x86/amd/hsmp/plat.c
@@ -230,6 +230,7 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
 static void hsmp_pltdrv_remove(struct platform_device *pdev)
 {
 	hsmp_misc_deregister();
+	hsmp_metric_tbl_unmap_all(hsmp_pdev, hsmp_pdev->num_sockets);
 }
 
 static struct platform_driver amd_hsmp_driver = {
-- 
2.34.1