On Wed, Mar 16, 2022 at 4:43 PM Limonciello, Mario
<Mario.Limonciello@xxxxxxx> wrote:
>
> [Public]
>
>
>
> > -----Original Message-----
> > From: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> > Sent: Wednesday, March 16, 2022 10:35
> > To: Hans de Goede <hdegoede@xxxxxxxxxx>; Limonciello, Mario
> > <Mario.Limonciello@xxxxxxx>
> > Cc: Mark Gross <mgross@xxxxxxxxxxxxxxx>; Rafael J . Wysocki
> > <rjw@xxxxxxxxxxxxx>; open list:X86 PLATFORM DRIVERS <platform-driver-
> > x86@xxxxxxxxxxxxxxx>; linux-acpi@xxxxxxxxxxxxxxx; S-k, Shyam-sundar
> > <Shyam-sundar.S-k@xxxxxxx>; Goswami, Sanket
> > <Sanket.Goswami@xxxxxxx>
> > 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@xxxxxxxxxx>
> > wrote:
> > >
> > > On Mon, Mar 14, 2022 at 2:37 PM Hans de Goede
> > <hdegoede@xxxxxxxxxx> 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.
Why are they useful at all, then?
> >
> > 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.
Why do that?
> 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.
Honestly, I'm not quite sure why it is a good idea to prevent the
platform from attempting to get into S0ix via suspend-to-idle in any
case.
You know you have to suspend. You don't know how much time you will
be suspended. The constraints can only tell you what's the
lowest-power state you can achieve at this point, but why is it
relevant?
