Re: [PATCH] platform/x86/amd/hsmp: Use guard mutex to synchronize probe

[Suma Hegde <Suma.Hegde@amd.com>]

Hi Ilpo and Hans,


Thank you for the review.


On 6/27/2025 12:23 AM, Hans de Goede wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> Hi,
>
> On 26-Jun-25 18:31, Ilpo Järvinen wrote:
>> On Wed, 25 Jun 2025, Suma Hegde wrote:
>>
>> In the shortlog, drop word "guard". This should also mention ACPI as the
>> legacy probe is not affected.


Sure, will drop the guard and will mention ACPI.

>>> When async probing is used, 2 hsmp_acpi_probe() calls can race and
>>> make a mess of things.
>> Too vague wording.

I will revise the commit message to enhance clarity.

>>> So, add guard mutex to synchronize them.
>>>
>>> Suggested-by: Hans de Goede <hdegoede@redhat.com>
>>> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
>>> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
>>> ---
>>>   drivers/platform/x86/amd/hsmp/acpi.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
>>> index 2f1faa82d13e..ab2b65f16d1d 100644
>>> --- a/drivers/platform/x86/amd/hsmp/acpi.c
>>> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
>>> @@ -15,11 +15,13 @@
>>>   #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/module.h>
>>> +#include <linux/mutex.h>
>>>   #include <linux/platform_device.h>
>>>   #include <linux/sysfs.h>
>>>   #include <linux/uuid.h>
>>> @@ -44,6 +46,8 @@ struct hsmp_sys_attr {
>>>       u32 msg_id;
>>>   };
>>>
>>> +static DEFINE_MUTEX(hsmp_lock);
>>> +
>>>   static int amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset,
>>>                             u32 *value, bool write)
>>>   {
>>> @@ -585,6 +589,8 @@ static int hsmp_acpi_probe(struct platform_device *pdev)
>>>       if (!hsmp_pdev)
>>>               return -ENOMEM;
>>>
>>> +    guard(mutex)(&hsmp_lock);
>>> +
>>>       if (!hsmp_pdev->is_probed) {
>>>               hsmp_pdev->num_sockets = amd_num_nodes();
>>>               if (hsmp_pdev->num_sockets == 0 || hsmp_pdev->num_sockets > MAX_AMD_NUM_NODES)
>> So is it just the ->sock alloc and misc dev registration that require
>> protection? (The latter doesn't even seem to require that if a local
>> variable carries that information over.)

Yes, the rest of the code, aside from the remove function mentioned 
below by Hans, doesn't require protection as it uses local variables.

Additionally, we have a semaphore in place to protect the other critical 
section.

> Another review note:
>
> hsmp_pdev->is_probed is also used in remove() so that needs a guard(mutex)(&hsmp_lock); too.

This was overlooked. I'll make sure to add it.


>
> Regards,
>
> Hans


Thanks and Regards,

Suma