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

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

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