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

["David E. Box" <david.e.box@linux.intel.com>]

On Mon, 2022-03-14 at 13:32 +0000, 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`
> which the handling and the use in the kernel doesn't seem to follow the design
> pattern
> you describe.
> 
> Rafael - can you please confirm which direction you want to see here for this?
> 
> > > +
> > > +void acpi_unregister_lps0_callbacks(struct lps0_callback_handler *arg)
> > > +{
> > > +	struct lps0_callback_handler *handler;
> > > +
> > > +	mutex_lock(&lps0_callback_handler_mutex);
> > > +	list_for_each_entry(handler, &lps0_callback_handler_head,
> > list_node) {
> > > +		if (handler->prepare_late_callback == arg-
> > > prepare_late_callback &&
> > > +		    handler->restore_early_callback == arg-
> > > restore_early_callback &&
> > > +		    handler->context == arg->context) {
> > > +			list_del(&handler->list_node);
> > > +			kfree(handler);
> > > +			break;
> > > +		}
> > > +	}
> > > +	mutex_unlock(&lps0_callback_handler_mutex);
> > > +}
> > > +EXPORT_SYMBOL_GPL(acpi_unregister_lps0_callbacks);
> > 
> > And this then becomes just lock, list_del, unlock.
> > 
> > Regards,
> > 
> > Hans

If you keep v3,

Reviewed-by: David E. Box <david.e.box@linux.intel.com>