* [PATCH v3 34/47] acpi: Register power-off handler with kernel power-off handler [not found] <1414425354-10359-1-git-send-email-linux@roeck-us.net> @ 2014-10-27 15:55 ` Guenter Roeck 2014-10-28 0:26 ` Rafael J. Wysocki 0 siblings, 1 reply; 6+ messages in thread From: Guenter Roeck @ 2014-10-27 15:55 UTC (permalink / raw) To: linux-kernel Cc: linux-pm, Guenter Roeck, Rafael J. Wysocki, Len Brown, linux-acpi 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. Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Len Brown <lenb@kernel.org> Signed-off-by: Guenter Roeck <linux@roeck-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) { /* 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; } +static struct notifier_block acpi_power_off_nb = { + .notifier_call = acpi_power_off, + .priority = POWER_OFF_PRIORITY_HIGH, +}; + int __init acpi_sleep_init(void) { char supported[ACPI_S_STATE_COUNT * 3 + 1]; @@ -851,7 +861,8 @@ int __init acpi_sleep_init(void) if (acpi_sleep_state_supported(ACPI_STATE_S5)) { sleep_states[ACPI_STATE_S5] = 1; pm_power_off_prepare = acpi_power_off_prepare; - pm_power_off = acpi_power_off; + if (register_power_off_handler(&acpi_power_off_nb)) + pr_warn("ACPI: Failed to register power-off handler\n"); } supported[0] = 0; -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 34/47] acpi: Register power-off handler with kernel power-off handler 2014-10-27 15:55 ` [PATCH v3 34/47] acpi: Register power-off handler with kernel power-off handler Guenter Roeck @ 2014-10-28 0:26 ` Rafael J. Wysocki 2014-10-28 2:10 ` Guenter Roeck 0 siblings, 1 reply; 6+ messages in thread From: Rafael J. Wysocki @ 2014-10-28 0:26 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-kernel, linux-pm, Len Brown, linux-acpi 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 -> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Len Brown <lenb@kernel.org> > Signed-off-by: Guenter Roeck <linux@roeck-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? > } > > +static struct notifier_block acpi_power_off_nb = { > + .notifier_call = acpi_power_off, > + .priority = POWER_OFF_PRIORITY_HIGH, > +}; > + > int __init acpi_sleep_init(void) > { > char supported[ACPI_S_STATE_COUNT * 3 + 1]; > @@ -851,7 +861,8 @@ int __init acpi_sleep_init(void) > if (acpi_sleep_state_supported(ACPI_STATE_S5)) { > sleep_states[ACPI_STATE_S5] = 1; > pm_power_off_prepare = acpi_power_off_prepare; > - pm_power_off = acpi_power_off; > + if (register_power_off_handler(&acpi_power_off_nb)) > + pr_warn("ACPI: Failed to register power-off handler\n"); > } > > supported[0] = 0; > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 34/47] acpi: Register power-off handler with kernel power-off handler 2014-10-28 0:26 ` Rafael J. Wysocki @ 2014-10-28 2:10 ` Guenter Roeck 2014-10-28 23:10 ` Rafael J. Wysocki 0 siblings, 1 reply; 6+ messages in thread From: Guenter Roeck @ 2014-10-28 2:10 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: linux-kernel, linux-pm, Len Brown, linux-acpi 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@rjwysocki.net> >> Cc: Len Brown <lenb@kernel.org> >> Signed-off-by: Guenter Roeck <linux@roeck-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. Thanks, Guenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 34/47] acpi: Register power-off handler with kernel power-off handler 2014-10-28 2:10 ` Guenter Roeck @ 2014-10-28 23:10 ` Rafael J. Wysocki 2014-10-29 2:05 ` Guenter Roeck 0 siblings, 1 reply; 6+ messages in thread From: Rafael J. Wysocki @ 2014-10-28 23:10 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-kernel, linux-pm, Len Brown, linux-acpi 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@rjwysocki.net> > >> Cc: Len Brown <lenb@kernel.org> > >> Signed-off-by: Guenter Roeck <linux@roeck-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. [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.] Kind regards, Rafael ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 34/47] acpi: Register power-off handler with kernel power-off handler 2014-10-28 23:10 ` Rafael J. Wysocki @ 2014-10-29 2:05 ` Guenter Roeck 2014-10-29 15:13 ` Rafael J. Wysocki 0 siblings, 1 reply; 6+ messages in thread From: Guenter Roeck @ 2014-10-29 2:05 UTC (permalink / raw) To: Rafael J. Wysocki; +Cc: linux-kernel, linux-pm, Len Brown, linux-acpi 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@rjwysocki.net> >>>> Cc: Len Brown <lenb@kernel.org> >>>> Signed-off-by: Guenter Roeck <linux@roeck-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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 34/47] acpi: Register power-off handler with kernel power-off handler 2014-10-29 2:05 ` Guenter Roeck @ 2014-10-29 15:13 ` Rafael J. Wysocki 0 siblings, 0 replies; 6+ messages in thread From: Rafael J. Wysocki @ 2014-10-29 15:13 UTC (permalink / raw) To: Guenter Roeck; +Cc: linux-kernel, linux-pm, Len Brown, linux-acpi On Tuesday, October 28, 2014 07:05:11 PM Guenter Roeck wrote: > 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@rjwysocki.net> > >>>> Cc: Len Brown <lenb@kernel.org> > >>>> Signed-off-by: Guenter Roeck <linux@roeck-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. OK > > [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. I see. -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-10-29 15:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1414425354-10359-1-git-send-email-linux@roeck-us.net>
2014-10-27 15:55 ` [PATCH v3 34/47] acpi: Register power-off handler with kernel power-off handler Guenter Roeck
2014-10-28 0:26 ` Rafael J. Wysocki
2014-10-28 2:10 ` Guenter Roeck
2014-10-28 23:10 ` Rafael J. Wysocki
2014-10-29 2:05 ` Guenter Roeck
2014-10-29 15:13 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox