Message-ID: <54504B57.3020403@roeck-us.net>
Date:	Tue, 28 Oct 2014 19:05:11 -0700
From:	Guenter Roeck <linux@...ck-us.net>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>
CC:	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
	Len Brown <lenb@...nel.org>, linux-acpi@...r.kernel.org
Subject: Re: [PATCH v3 34/47] acpi: Register power-off handler with kernel
 power-off handler

On 10/28/2014 04:10 PM, Rafael J. Wysocki wrote:
> On Monday, October 27, 2014 07:10:26 PM Guenter Roeck wrote:
>> On 10/27/2014 05:26 PM, Rafael J. Wysocki wrote:
>>> On Monday, October 27, 2014 08:55:41 AM Guenter Roeck wrote:
>>>> Register with kernel power-off handler instead of setting pm_power_off
>>>> directly. Register with high priority to reflect that the driver explicitly
>>>> overrides existing power-off handlers.
>>>
>>> Well, I'm still rather unconvinced that notifiers are particularly suitable for
>>> this purpose.
>>>
>>> Specifically ->
>>>
>>
>> Fine.
>>
>>>> Cc: Rafael J. Wysocki <rjw@...ysocki.net>
>>>> Cc: Len Brown <lenb@...nel.org>
>>>> Signed-off-by: Guenter Roeck <linux@...ck-us.net>
>>>> ---
>>>> v3:
>>>> - Replace poweroff in all newly introduced variables and in text
>>>>     with power_off or power-off as appropriate
>>>> - Replace POWEROFF_PRIORITY_xxx with POWER_OFF_PRIORITY_xxx
>>>> - Replace acpi: with ACPI: in log message
>>>> v2:
>>>> - Use define to specify poweroff handler priority
>>>> - Use pr_warn instead of pr_err
>>>>
>>>>    drivers/acpi/sleep.c | 15 +++++++++++++--
>>>>    1 file changed, 13 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
>>>> index 05a31b5..7875b92 100644
>>>> --- a/drivers/acpi/sleep.c
>>>> +++ b/drivers/acpi/sleep.c
>>>> @@ -16,6 +16,8 @@
>>>>    #include <linux/device.h>
>>>>    #include <linux/interrupt.h>
>>>>    #include <linux/suspend.h>
>>>> +#include <linux/notifier.h>
>>>> +#include <linux/pm.h>
>>>>    #include <linux/reboot.h>
>>>>    #include <linux/acpi.h>
>>>>    #include <linux/module.h>
>>>> @@ -827,14 +829,22 @@ static void acpi_power_off_prepare(void)
>>>>    	acpi_disable_all_gpes();
>>>>    }
>>>>
>>>> -static void acpi_power_off(void)
>>>> +static int acpi_power_off(struct notifier_block *this,
>>>> +			  unsigned long unused1, void *unused2)
>>>>    {
>>>
>>> -> Is there any reason why any notifier in the new chain would use the
>>> second argument for anything meaningful?  And the third argument for
>>> that matter?
>>>
>>>>    	/* acpi_sleep_prepare(ACPI_STATE_S5) should have already been called */
>>>>    	printk(KERN_DEBUG "%s called\n", __func__);
>>>>    	local_irq_disable();
>>>>    	acpi_enter_sleep_state(ACPI_STATE_S5);
>>>> +
>>>> +	return NOTIFY_DONE;
>>>
>>> Also is there any reason for any notifier in the new chain to return anything
>>> different from NOTIFY_DONE and if so, then what happens when anything else
>>> is returned?
>>>
>>
>> As mentioned earlier, notifiers just come handy. That is all.
>>
>> Having said that, I don't have a strong opinion either way. If you want me
>> to implement a priority based callback handler with a single argument,
>> just let me know and I'll be happy to implement it. It is not worth arguing
>> about.
>>
>> Would something like
>>
>> struct power_off_block {
>> 	void (*power_off_call)(struct power_off_block *);
>> 	struct power_off_block __rcu *next;
>> 	int priority;
>> };
>>
>> for the data structure be acceptable ? The power-off handler code would then
>> be something like
>>
>> static void acpi_power_off(struct power_off_block *this)
>> {
>> }
>>
>> ie quite similar to the current power-off handler code, with an added argument.
>> The API would, except for the structure argument, pretty much stay the same.
>
> That's better in my view.
>
> You could also get rid of the priority if you had something like
>
> struct power_off_block *power_off_list[MAX_LEVEL];
>
> and then made your power_off_block registration pass the level as the
> second argument.
>
> I also would use struct list_head instead of the next pointer, because the
> list manipulation would be trivial then (and the above would become
> struct list_head power_off_list[MAX_LEVEL];) and the callers would only
> need to do
>
> static struct power_off_block my_power_off_block = {
> 	.power_off_call = my_routine,
> };
>
> and then something like
>
> 	register_power_off_block(&my_power_off_block, <level>);
>
> which would be just list_add_tail(&block->node, &power_off_list[<level>]) plus
> some bounds checking etc.  To avoid open coding stuff.
>
> That would allow me to avoid arbitrary gaps in the priority space too and
> if more levels need to be added over time, that should be easy to do too if
> an enum is used to define them.
>
> But if you prefer to use a unidirectional list and keep the priority in
> struct power_off_block, I'm fine with that too.
>
I prefer a unidirectional list. It is not as if we expect dozens of registrations;
in most cases there will be one, rarely two and even more rarely three.

> [Dynamic allocation of memory for the struct power_off_block things is worth
> considering too IMHO, so that users can simply pass the name of the routine
> and the level to the registration function, like:
>
> 	ret = register_power_off_call(my_routine, <level>);
> 	if (ret)
> 		complain;
>
> The unregistration would be somewhat less straightforward then, but I'm not
> sure if unregistration is necessary at all in this case.]
>

Problem with dynamic memory allocation is that some callers don't have
the memory subsystem initialized when registering the poweroff function.
That was my initial implementation, and it got me some unexpected crashes.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists