Re: [PATCH v2 5/7] platform/x86/amd/hsmp: Serialize the data plane against socket teardown

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Thu, 25 Jun 2026, Muralidhara M K wrote:

> 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 hsmp_sock_rwsem.  hsmp_send_message() now holds it for read across
> the whole bounds-check + MMIO access, and hsmp_sock_teardown_lock()/
> hsmp_sock_teardown_unlock() let a teardown path hold it for write to
> drain any in-flight message and keep new ones out while it tears the
> socket down.
> 
> Wire the non-ACPI platform path into the drain: hsmp_pltdrv_remove()
> and the probe-failure cleanup take the write lock and drop the global
> socket pointer before devres frees the devm_kcalloc() array.  num_sockets
> is left intact so a later rebind can re-create the array.
> 
> Signed-off-by: Muralidhara M K <muralidhara.mk@amd.com>
> ---
>  drivers/platform/x86/amd/hsmp/hsmp.c | 55 +++++++++++++++++++++++++---
>  drivers/platform/x86/amd/hsmp/hsmp.h |  2 +
>  drivers/platform/x86/amd/hsmp/plat.c | 23 ++++++++++++
>  3 files changed, 74 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
> index c3939908d95f..c15acba241c4 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
> @@ -15,6 +15,7 @@
>  #include <linux/io.h>
>  #include <linux/mutex.h>
>  #include <linux/nospec.h>
> +#include <linux/rwsem.h>
>  #include <linux/semaphore.h>
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
> @@ -44,6 +45,16 @@
>  
>  static struct hsmp_plat_device hsmp_pdev;
>  
> +/*
> + * Serializes the lock-free data plane (hsmp_send_message() and the per-socket
> + * MMIO access it performs) against socket teardown.  Callers of the data plane
> + * hold it for read so multiple sockets can be driven concurrently; ACPI
> + * removal holds it for write while it clears sock->dev,

I don't see acpi change in this patch (just hsmp_send_message and plat 
changes)???

> frees the socket array
> + * and unmaps the mailbox, so a reader can never observe a half-torn-down or
> + * freed socket.
> + */
> +static DECLARE_RWSEM(hsmp_sock_rwsem);
> +
>  /*
>   * Send a message to the HSMP port via PCI-e config space registers
>   * or by writing to MMIO space.
> @@ -215,8 +226,19 @@ int hsmp_send_message(struct hsmp_message *msg)
>  	if (ret)
>  		return ret;
>  
> -	if (!hsmp_pdev.sock || msg->sock_ind >= hsmp_pdev.num_sockets)
> -		return -ENODEV;
> +	/*
> +	 * Hold the teardown rwsem for read across the whole MMIO access.  ACPI
> +	 * removal takes it for write before clearing sock->dev, freeing the
> +	 * socket array and unmapping the mailbox, so the lock-free data plane
> +	 * (open /dev/hsmp fds and hwmon sysfs reads) can never dereference a
> +	 * freed socket or touch an unmapped mailbox.
> +	 */
> +	down_read(&hsmp_sock_rwsem);
> +
> +	if (!hsmp_pdev.sock || msg->sock_ind >= hsmp_pdev.num_sockets) {
> +		ret = -ENODEV;
> +		goto out_unlock;
> +	}
>  
>  	/*
>  	 * Sanitize sock_ind after the bounds check.  A mispredicted branch can
> @@ -235,18 +257,22 @@ int hsmp_send_message(struct hsmp_message *msg)
>  	 * semaphore or an unmapped mailbox.  A 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(). */
> -	if (!smp_load_acquire(&sock->dev))
> -		return -ENODEV;
> +	/* Held under hsmp_sock_rwsem; pairs with smp_store_release(&sock->dev). */
> +	if (!smp_load_acquire(&sock->dev)) {
> +		ret = -ENODEV;
> +		goto out_unlock;
> +	}
>  
>  	ret = down_interruptible(&sock->hsmp_sem);
>  	if (ret < 0)
> -		return ret;
> +		goto out_unlock;
>  
>  	ret = __hsmp_send_message(sock, msg);
>  
>  	up(&sock->hsmp_sem);
>  
> +out_unlock:
> +	up_read(&hsmp_sock_rwsem);

Don't add unlock labels at the end but please use cleanup.h. It will 
simplify the patch too as you don't need to alter those direct returns.

>  	return ret;
>  }
>  EXPORT_SYMBOL_NS_GPL(hsmp_send_message, "AMD_HSMP");
> @@ -529,6 +555,23 @@ struct hsmp_plat_device *get_hsmp_pdev(void)
>  }
>  EXPORT_SYMBOL_NS_GPL(get_hsmp_pdev, "AMD_HSMP");
>  
> +/*
> + * Take the write side of the data-plane rwsem.  A caller tearing a socket down
> + * uses this to drain any in-flight hsmp_send_message() and to keep new ones out
> + * while it clears sock->dev, frees the socket array or unmaps the mailbox.
> + */
> +void hsmp_sock_teardown_lock(void)
> +{
> +	down_write(&hsmp_sock_rwsem);
> +}
> +EXPORT_SYMBOL_NS_GPL(hsmp_sock_teardown_lock, "AMD_HSMP");
> +
> +void hsmp_sock_teardown_unlock(void)
> +{
> +	up_write(&hsmp_sock_rwsem);
> +}
> +EXPORT_SYMBOL_NS_GPL(hsmp_sock_teardown_unlock, "AMD_HSMP");

Just export the lock to be taken directly in plat/acpi. It's much easier 
to follow who locks what when you don't add wrappers like this.

I don't understand why acpi doesn't use write side of this lock though 
but has it's own lock (hsmp_acpi_probe_mutex)?

>  MODULE_DESCRIPTION("AMD HSMP Common driver");
>  MODULE_VERSION(DRIVER_VERSION);
>  MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.h b/drivers/platform/x86/amd/hsmp/hsmp.h
> index 91bc21232646..5d0a6d819865 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.h
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.h
> @@ -70,6 +70,8 @@ void hsmp_init_metric_read_locks(struct hsmp_plat_device *pdev, u16 num_sockets)
>  ssize_t hsmp_metric_tbl_read(struct hsmp_socket *sock, char *buf, size_t size);
>  void hsmp_destroy_metric_read_locks(struct hsmp_plat_device *pdev, u16 num_sockets);
>  struct hsmp_plat_device *get_hsmp_pdev(void);
> +void hsmp_sock_teardown_lock(void);
> +void hsmp_sock_teardown_unlock(void);
>  #if IS_ENABLED(CONFIG_HWMON)
>  int hsmp_create_sensor(struct device *dev, u16 sock_ind);
>  #else
> diff --git a/drivers/platform/x86/amd/hsmp/plat.c b/drivers/platform/x86/amd/hsmp/plat.c
> index 685f2d2c574b..26c1363e79a1 100644
> --- a/drivers/platform/x86/amd/hsmp/plat.c
> +++ b/drivers/platform/x86/amd/hsmp/plat.c
> @@ -233,15 +233,38 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
>  	 * init_platform_device() may have ioremap()ed metric tables before
>  	 * failing.  hsmp_destroy_metric_read_locks() unmaps them and tears
>  	 * down the per-socket mutexes; the socket array itself is devm-managed.
> +	 *
> +	 * init_platform_device() also runs the data plane (hsmp_test()), so
> +	 * drain it via the teardown rwsem and drop the global socket pointer
> +	 * before devres frees the array.  num_sockets is left intact: it is
> +	 * only computed once in __init (amd_num_nodes()) and is needed to
> +	 * re-create the array on a later rebind.
>  	 */
> +	hsmp_sock_teardown_lock();
>  	hsmp_destroy_metric_read_locks(hsmp_pdev, hsmp_pdev->num_sockets);
> +	hsmp_pdev->sock = NULL;
> +	hsmp_sock_teardown_unlock();
>  	return ret;
>  }
>  
>  static void hsmp_pltdrv_remove(struct platform_device *pdev)
>  {
> +	/*
> +	 * Drain the lock-free data plane and keep it out while the sockets are
> +	 * torn down.  misc_deregister() does not drain already-open /dev/hsmp
> +	 * fds and the driver permits sysfs unbind, so without this an in-flight
> +	 * hsmp_send_message() could touch the devres-freed socket array or an
> +	 * iounmap()ed metric table.  Dropping the global socket pointer makes
> +	 * later messages bail out at the first check.  num_sockets is left
> +	 * intact so an unbind/rebind cycle can re-create the array; it is only
> +	 * computed once in __init (amd_num_nodes()) and is never recomputed on
> +	 * probe.
> +	 */
> +	hsmp_sock_teardown_lock();
>  	hsmp_misc_deregister();
>  	hsmp_destroy_metric_read_locks(hsmp_pdev, hsmp_pdev->num_sockets);
> +	hsmp_pdev->sock = NULL;
> +	hsmp_sock_teardown_unlock();
>  }
>  
>  static struct platform_driver amd_hsmp_driver = {
> 

-- 
 i.