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

["Rafael J. Wysocki" <rafael@kernel.org>]

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.

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