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

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

[Suma Hegde]

When async probing is used, 2 hsmp_acpi_probe() calls can race and
make a mess of things. 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)
-- 
2.25.1

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

[Ilpo Järvinen]

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.

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

Too vague wording.

> 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.)

-- 
 i.

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

[Hans de Goede]

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.
> 
>> When async probing is used, 2 hsmp_acpi_probe() calls can race and
>> make a mess of things.
> 
> Too vague wording.
> 
>> 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.)

Another review note:

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

Regards,

Hans

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

[Suma Hegde]

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

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3703 bytes --]

On Fri, 27 Jun 2025, Suma Hegde wrote:

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

Hmm... I was going to suggest replacing ->is_probed with 
devm_add_action_or_reset() but then started to think probe/remove 
ordering between different pdevs.

Is there anything that guarantees ->sock isn't teared down too early, that 
is, pdev that did the allocation should be removed last to not prematurely 
free ->sock?

-- 
 i.

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

[Suma Hegde]

Hi Ilpo,


On 6/27/2025 1:25 PM, Ilpo Järvinen wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Fri, 27 Jun 2025, Suma Hegde wrote:
>
>> 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.
> Hmm... I was going to suggest replacing ->is_probed with
> devm_add_action_or_reset() but then started to think probe/remove
> ordering between different pdevs.
>
> Is there anything that guarantees ->sock isn't teared down too early, that
> is, pdev that did the allocation should be removed last to not prematurely
> free ->sock?
Currently, there is no assurance that the pdev responsible for the 
allocation will be the last one removed.

Should I implement reference counting to address this? The reference 
count could be initialized to num_sockets and incremented in probe and

decremented with each removal, freeing the "sock" when the count reaches 
zero. Socket allocation will be performed using kmalloc instead of 
devm_alloc.

> --
>   i.

Thanks and Regards,

Suma

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

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5456 bytes --]

On Mon, 7 Jul 2025, Suma Hegde wrote:
> On 6/27/2025 1:25 PM, Ilpo Järvinen wrote:
> > Caution: This message originated from an External Source. Use proper caution
> > when opening attachments, clicking links, or responding.
> > 
> > 
> > On Fri, 27 Jun 2025, Suma Hegde wrote:
> > 
> > > 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.
> > Hmm... I was going to suggest replacing ->is_probed with
> > devm_add_action_or_reset() but then started to think probe/remove
> > ordering between different pdevs.
> > 
> > Is there anything that guarantees ->sock isn't teared down too early, that
> > is, pdev that did the allocation should be removed last to not prematurely
> > free ->sock?
> Currently, there is no assurance that the pdev responsible for the allocation
> will be the last one removed.
> 
> Should I implement reference counting to address this? The reference count
> could be initialized to num_sockets and incremented in probe and

I think you want to initialize it to zero as otherwise it won't reach 
zero ever on removal.

> decremented with each removal, freeing the "sock" when the count reaches zero.
> Socket allocation will be performed using kmalloc instead of devm_alloc.

Yes, it would be better to use counter (I think you can replace is_probed 
with it).

You'll still need the mutex though to ensure the concurrent probes all see 
the allocated sock as the allocating one and some other pdev could race 
otherwise.


One more potentially problematic scenario is partial state where not all 
pdevs are there yet/aren't there anymore. hsmp_send_message() seems to 
assume sock[n] is always valid, but that's not true unless all sockets 
have already probed and none removed yet.

-- 
 i.