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

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

On Mon, 13 Jul 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 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>
> ---

While the code seems okay AFAICT, I think the comments are somewhat 
misleading:

> +int hsmp_send_message(struct hsmp_message *msg)
> +{
> +	/*
> +	 * The data plane is lock-free: open /dev/hsmp fds and hwmon sysfs reads

This says data plane is lock-free.

> +	 * 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");


> 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;

This says data plane is gated.

Both cannot be true?

> @@ -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

This too talks about data-plane rwsem.

> + * 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;
>  }

My own understanding here is that the first comment tried to tell what 
the situation was before this patch (data plane was indeed lock free), not 
the behavior after the patch. Is my understanding correct?

To avoid potential terminology related confusion, rwsem is still a lock 
(even if the read sides are not block each other).

We don't write about historic state of things but document only the 
current code does. So if it's no longer lock free, stating it's lock 
free is sure way to confuse the reader. :-)

-- 
 i.