RE: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler

["Limonciello, Mario" <Mario.Limonciello@amd.com>]

[Public]



> -----Original Message-----
> From: Rafael J. Wysocki <rafael@kernel.org>
> Sent: Wednesday, March 16, 2022 10:35
> To: Hans de Goede <hdegoede@redhat.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>
> Cc: Mark Gross <mgross@linux.intel.com>; Rafael J . Wysocki
> <rjw@rjwysocki.net>; open list:X86 PLATFORM DRIVERS <platform-driver-
> x86@vger.kernel.org>; linux-acpi@vger.kernel.org; S-k, Shyam-sundar
> <Shyam-sundar.S-k@amd.com>; Goswami, Sanket
> <Sanket.Goswami@amd.com>
> Subject: Re: [PATCH v3 1/5] ACPI / x86: Add support for LPS0 callback handler
> 
> On Wed, Mar 16, 2022 at 4:02 PM Rafael J. Wysocki <rafael@kernel.org>
> wrote:
> >
> > On Mon, Mar 14, 2022 at 2:37 PM Hans de Goede
> <hdegoede@redhat.com> wrote:
> > >
> > > Hi,
> > >
> > > On 3/14/22 14:32, Limonciello, Mario wrote:
> > > > [Public]
> > > >
> > > >>> +int acpi_register_lps0_callbacks(struct lps0_callback_handler *arg)
> > > >>> +{
> > > >>> +   struct lps0_callback_handler *handler;
> > > >>> +
> > > >>> +   if (!lps0_device_handle || sleep_no_lps0)
> > > >>> +           return -ENODEV;
> > > >>> +
> > > >>> +   handler = kmalloc(sizeof(*handler), GFP_KERNEL);
> > > >>> +   if (!handler)
> > > >>> +           return -ENOMEM;
> > > >>> +   handler->prepare_late_callback = arg->prepare_late_callback;
> > > >>> +   handler->restore_early_callback = arg->restore_early_callback;
> > > >>> +   handler->context = arg->context;
> > > >>> +
> > > >>> +   mutex_lock(&lps0_callback_handler_mutex);
> > > >>> +   list_add(&handler->list_node, &lps0_callback_handler_head);
> > > >>> +   mutex_unlock(&lps0_callback_handler_mutex);
> > > >>> +
> > > >>> +   return 0;
> > > >>> +}
> > > >>> +EXPORT_SYMBOL_GPL(acpi_register_lps0_callbacks);
> > > >>
> > > >> Typically with calls like these we simply let the caller own the struct
> > > >> lps0_callback_handler
> > > >> and only make the list_add() call here. Typically the struct
> > > >> lps0_callback_handler will
> > > >> be embedded in the driver_data of the driver registering the handler
> and it
> > > >> will
> > > >> call the unregister function before free-ing its driver_data.
> > > >>
> > > >
> > > > When I put this together I was modeling it off of `struct
> acpi_wakeup_handler`
> >
> > The structure added by this patch is more like struct dev_pm_ops, though.
> >
> > > > which the handling and the use in the kernel doesn't seem to follow the
> design pattern
> > > > you describe.
> > >
> > > Ah, fair enough. Whatever Rafael prefers works for me.
> >
> > My preference at this point would be to use a notifier chain, unless
> > that's not sufficient for some reason, because it appears to match the
> > notifier usage model.
> 
> Well, I'm actually not sure about that.
> 
> > > I pointed this out, because making this change would also make 4/5 a bit
> > > cleaner. You are recreating the same struct lps0_callback_handler on
> > > stack twice there, which looked weird to me.
> > >
> > > Note if Rafael wants to stick with the approach from this v3, then
> > > I guess that the approach in 4/5 is fine.
> > > > Rafael - can you please confirm which direction you want to see here
> for this?
> 
> So the idea is that the PMC driver's "suspend" method needs to be
> invoked from acpi_s2idle_prepare_late(), so it doesn't interfere with
> the suspend of the other devices in the system and so it can take the
> constraints into account.

The reason to do nothing (besides a debug level message right now) with the constraints
information is that at least on today's OEM platforms there are some instances constraints
aren't met on Linux that need to be investigated and root caused.  These particular constraints
don't actually cause problems reaching s0ix residency though.

> 
> What is it going to do, in the future, depending on whether or not the
> constraints are met?

The idea was that if constraints were met that it would send the OS_HINT as part of
amd_pmc_suspend/amd_pmc_resume, and if they aren't met then skip this step.

It would effectively block the system from getting s0ix residency unless the constraints
are all met.  Given we know some OEM platforms have problems in current generations
with constraints it would probably need to be restricted to this behavior only on a future
SOC that we are confident of all drivers and firmware are doing the right thing.

By passing the information to amd_pmc we can keep that logic restricting the behavior to
only those platforms that we're confident on that behavior.