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

["M K, Muralidhara" <muralimk@amd.com>]

On 7/22/2026 6:41 PM, Ilpo Järvinen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> 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. :-)
> 
You're right on both counts, thanks for catching it.

Yes -- the "lock-free" wording described the pre-patch state, not the
behavior after this patch. After the series hsmp_send_message() takes
hsmp_sock_rwsem for read, so the data plane is gated by a lock; the read
side just doesn't exclude other readers. Calling it "lock-free" in a
current-state comment directly contradicted the rwsem documentation in
hsmp.h, so I've dropped that wording everywhere it described the current
code.

I will submit v7 which is a comment-only change on top of v6 (no 
functional change).
> --
>   i.
>