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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

> 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?

Done above.



[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux