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