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

[Hans de Goede <hdegoede@redhat.com>]

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`
> 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.

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?

Regards,

Hans



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