From: Pavel Machek <hidden> Date: 2014-10-09 10:33:06
On Mon 2014-10-06 22:28:05, Guenter Roeck wrote:
quoted hunk
Poweroff handlers may now be installed with register_poweroff_handler.
Use the new API function have_kernel_poweroff to determine if a poweroff
handler has been installed.
Cc: Rafael J. Wysocki <redacted>
Cc: Pavel Machek <redacted>
Cc: Len Brown <redacted>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
kernel/power/hibernate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
+static int ab8500_power_off(struct notifier_block *this, unsigned long unused1,+ void *unused2) { sigset_t old; sigset_t all;
@@ -34,11 +36,6 @@ static void ab8500_power_off(void) struct power_supply *psy; int ret;- if (sysctrl_dev == NULL) {- pr_err("%s: sysctrl not initialized\n", __func__);- return;- }
Can you explain the purpose of this change please?
I guess it's because the sysctrl_dev is already initialised when
registering the power_off handler, so there isn't a way to call the
above function with a NULL sysctrl_dev. Probably even with the original
code you didn't need this check (after some race fix in
ab8500_sysctrl_remove but races is one of the things Guenter's patches
try to address).
--
Catalin
+static int ab8500_power_off(struct notifier_block *this, unsigned long unused1,+ void *unused2) { sigset_t old; sigset_t all;
@@ -34,11 +36,6 @@ static void ab8500_power_off(void) struct power_supply *psy; int ret;- if (sysctrl_dev == NULL) {- pr_err("%s: sysctrl not initialized\n", __func__);- return;- }
Can you explain the purpose of this change please?
I guess it's because the sysctrl_dev is already initialised when
registering the power_off handler, so there isn't a way to call the
above function with a NULL sysctrl_dev. Probably even with the original
code you didn't need this check (after some race fix in
ab8500_sysctrl_remove but races is one of the things Guenter's patches
try to address).
Sounds reasonable, although I think this change should be part of
another patch.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, Oct 7, 2014 at 7:28 AM, Guenter Roeck [off-list ref] wrote:
quoted hunk
+int register_poweroff_handler_simple(void (*handler)(void), int priority)+{+ char symname[KSYM_NAME_LEN];++ if (poweroff_handler_data.handler) {+ lookup_symbol_name((unsigned long)poweroff_handler_data.handler,+ symname);+ pr_warn("Poweroff function already registered (%s)", symname);+ lookup_symbol_name((unsigned long)handler, symname);+ pr_cont(", cannot register %s\n", symname);
Doesn't %ps work to look up symbols?
pr_warn("Poweroff function already registered (%ps), cannot register
%ps\n", poweroff_handler_data.handler, handler);
+ return -EBUSY;
+ }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Dunno; matter of personal preference. I started with that, but ultimately went
with poweroff to distinguish poweroff handler functions from existing code,
specifically kernel_power_off().
Does anyone else have an opinion ?
On Tue, Oct 7, 2014 at 7:28 AM, Guenter Roeck [off-list ref] wrote:
quoted
+int register_poweroff_handler_simple(void (*handler)(void), int priority)+{+ char symname[KSYM_NAME_LEN];++ if (poweroff_handler_data.handler) {+ lookup_symbol_name((unsigned long)poweroff_handler_data.handler,+ symname);+ pr_warn("Poweroff function already registered (%s)", symname);+ lookup_symbol_name((unsigned long)handler, symname);+ pr_cont(", cannot register %s\n", symname);
Doesn't %ps work to look up symbols?
pr_warn("Poweroff function already registered (%ps), cannot register
%ps\n", poweroff_handler_data.handler, handler);
Hi Geert,
That is great. One never stops learning. I'll use that.
Thanks!
Guenter
Poweroff handlers may now be installed with register_poweroff_handler.
Use the new API function have_kernel_poweroff to determine if a poweroff
handler has been installed.
Cc: Rafael J. Wysocki <redacted>
Cc: Pavel Machek <redacted>
Cc: Len Brown <redacted>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
kernel/power/hibernate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
As mentioned in my other reply, that was on purpose to distinguish
existing functions from poweroff handler functions.
But if you are playing with this, anyway... does it make sense to
introduce kernel_power_off() that just works, no need to check
have_..?
Pavel
I am trying not to change existing behavior.
kernel_power_off is an existing function which does some cleanup
before calling machine_power_off which in turn calls do_kernel_poweroff
(or currently pm_power_off and may do some other machine specific stuff.
Sure, poweroff handling could be unified further. We could decide to
enter an endless loop if machine_power_off() returns, or we could decide
to dump a warning or panic in this case. But that is all separate from
the issue I am trying to solve here, which is to provide a capability to
register more than one poweroff handler. It would also not be that simple,
since some architectures call machine_power_off() directly from various
places.
Guenter
+static int ab8500_power_off(struct notifier_block *this, unsigned long unused1,+ void *unused2) { sigset_t old; sigset_t all;
@@ -34,11 +36,6 @@ static void ab8500_power_off(void) struct power_supply *psy; int ret;- if (sysctrl_dev == NULL) {- pr_err("%s: sysctrl not initialized\n", __func__);- return;- }
Can you explain the purpose of this change please?
I guess it's because the sysctrl_dev is already initialised when
registering the power_off handler, so there isn't a way to call the
above function with a NULL sysctrl_dev. Probably even with the original
code you didn't need this check (after some race fix in
ab8500_sysctrl_remove but races is one of the things Guenter's patches
try to address).
Sounds reasonable, although I think this change should be part of
another patch.
Sure, no problem. I'll split this into two patches.
Since we are at it, any idea what to do with the restart function
in the same file ? It is not used anywhere.
Guenter
+static int ab8500_power_off(struct notifier_block *this, unsigned long unused1,+ void *unused2) { sigset_t old; sigset_t all;
@@ -34,11 +36,6 @@ static void ab8500_power_off(void) struct power_supply *psy; int ret;- if (sysctrl_dev == NULL) {- pr_err("%s: sysctrl not initialized\n", __func__);- return;- }
Can you explain the purpose of this change please?
I guess it's because the sysctrl_dev is already initialised when
registering the power_off handler, so there isn't a way to call the
above function with a NULL sysctrl_dev. Probably even with the original
code you didn't need this check (after some race fix in
ab8500_sysctrl_remove but races is one of the things Guenter's patches
try to address).
Sounds reasonable, although I think this change should be part of
another patch.
Sure, no problem. I'll split this into two patches.
Since we are at it, any idea what to do with the restart function
in the same file ? It is not used anywhere.
You can strip it out with Linus Walleij's Ack. Or I'll be happy to do
it?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Thu, Oct 09, 2014 at 12:31:43PM +0200, Pavel Machek wrote:
Hi!
quoted
+/**+ * register_poweroff_handler_simple - Register function to be called to power off+ * the system+ * @handler: Function to be called to power off the system+ * @priority: Handler priority. For priority guidelines see+ * register_poweroff_handler.+ *+ * This is a simplified version of register_poweroff_handler. It does not+ * take a notifier as argument, but a function pointer. The function+ * registers a poweroff handler with specified priority. Poweroff+ * handlers registered with this function can not be unregistered,+ * and only a single poweroff handler can be installed using it.+ *+ * This function must not be called from modules and is therefore+ * not exported.+ *+ * Returns -EBUSY if a poweroff handler has already been registered+ * using register_poweroff_handler_simple. Otherwise returns zero,+ * since atomic_notifier_chain_register() currently always returns zero.+ */+int register_poweroff_handler_simple(void (*handler)(void), int priority)+{+ char symname[KSYM_NAME_LEN];++ if (poweroff_handler_data.handler) {+ lookup_symbol_name((unsigned long)poweroff_handler_data.handler,+ symname);+ pr_warn("Poweroff function already registered (%s)", symname);+ lookup_symbol_name((unsigned long)handler, symname);+ pr_cont(", cannot register %s\n", symname);+ return -EBUSY;+ }
Dunno, are you maybe overdoing the debugging infrastructure a bit?
This is not going to happen in production, and if it does happen,
developer can look the symbol name himself.
On the other side, I don't think it hurts to have that message.
Anyway, I'll use %ps as suggested by Geert.
Guenter
+static int ab8500_power_off(struct notifier_block *this, unsigned long unused1,+ void *unused2) { sigset_t old; sigset_t all;
@@ -34,11 +36,6 @@ static void ab8500_power_off(void) struct power_supply *psy; int ret;- if (sysctrl_dev == NULL) {- pr_err("%s: sysctrl not initialized\n", __func__);- return;- }
Can you explain the purpose of this change please?
I guess it's because the sysctrl_dev is already initialised when
registering the power_off handler, so there isn't a way to call the
above function with a NULL sysctrl_dev. Probably even with the original
code you didn't need this check (after some race fix in
ab8500_sysctrl_remove but races is one of the things Guenter's patches
try to address).
Sounds reasonable, although I think this change should be part of
another patch.
Sure, no problem. I'll split this into two patches.
Since we are at it, any idea what to do with the restart function
in the same file ? It is not used anywhere.
You can strip it out with Linus Walleij's Ack. Or I'll be happy to do
it?
+static int ab8500_power_off(struct notifier_block *this, unsigned long unused1,+ void *unused2) { sigset_t old; sigset_t all;
@@ -34,11 +36,6 @@ static void ab8500_power_off(void) struct power_supply *psy; int ret;- if (sysctrl_dev == NULL) {- pr_err("%s: sysctrl not initialized\n", __func__);- return;- }
Can you explain the purpose of this change please?
I guess it's because the sysctrl_dev is already initialised when
registering the power_off handler, so there isn't a way to call the
above function with a NULL sysctrl_dev. Probably even with the original
code you didn't need this check (after some race fix in
ab8500_sysctrl_remove but races is one of the things Guenter's patches
try to address).
Sounds reasonable, although I think this change should be part of
another patch.
Turns out the options are to either drop the check or to use the device
managed function to register the poweroff handler. I decided to keep
the check and use the device managed function.
Guenter
Dunno; matter of personal preference. I started with that, but ultimately went
with poweroff to distinguish poweroff handler functions from existing code,
specifically kernel_power_off().
Dunno; matter of personal preference. I started with that, but ultimately went
with poweroff to distinguish poweroff handler functions from existing code,
specifically kernel_power_off().
That works for you, but once it is merged, it is ugly/confusing typo.
Pavel
From: Stephen Boyd <hidden> Date: 2015-06-18 01:05:07
On 10/06/2014 10:28 PM, Guenter Roeck wrote:
Various drivers implement architecture and/or device specific means to
remove power from the system. For the most part, those drivers set the
global variable pm_power_off to point to a function within the driver.
This mechanism has a number of drawbacks. Typically only one scheme
to remove power is supported (at least if pm_power_off is used).
At least in theory there can be multiple means remove power, some of
which may be less desirable. For example, some mechanisms may only
power off the CPU or the CPU card, while another may power off the
entire system. Others may really just execute a restart sequence
or drop into the ROM monitor. Using pm_power_off can also be racy
if the function pointer is set from a driver built as module, as the
driver may be in the process of being unloaded when pm_power_off is
called. If there are multiple poweroff handlers in the system, removing
a module with such a handler may inadvertently reset the pointer to
pm_power_off to NULL, leaving the system with no means to remove power.
Introduce a system poweroff handler call chain to solve the described
problems. This call chain is expected to be executed from the
architecture specific machine_power_off() function. Drivers providing
system poweroff functionality are expected to register with this call chain.
By using the priority field in the notifier block, callers can control
poweroff handler execution sequence and thus ensure that the poweroff
handler with the optimal capabilities to remove power for a given system
is called first.
What happened to this series? I want to add shutdown support to my
platform and I need to write a register on the PMIC in one driver to
configure it for shutdown instead of restart and then write an MMIO
register to tell the PMIC to actually do the shutdown in another driver.
It seems that the notifier solves this case for me, albeit with the
slight complication that I need to order the two with some priority.
I'm also considering putting the PMIC configuration part into the reboot
notifier chain, because it only does things to change the configuration
and not actually any shutdown/restart itself. That removes any
requirement to get the priority of notifiers right. This series will
still be useful for the MMIO register that needs to be toggled though.
Right now I have to assign pm_power_off or hook the reboot notifier with
a different priority to make this work.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
From: Frans Klaver <hidden> Date: 2015-06-18 06:54:08
On Thu, Jun 18, 2015 at 3:04 AM, Stephen Boyd [off-list ref] wrote:
On 10/06/2014 10:28 PM, Guenter Roeck wrote:
quoted
Various drivers implement architecture and/or device specific means to
remove power from the system. For the most part, those drivers set the
global variable pm_power_off to point to a function within the driver.
This mechanism has a number of drawbacks. Typically only one scheme
to remove power is supported (at least if pm_power_off is used).
At least in theory there can be multiple means remove power, some of
which may be less desirable. For example, some mechanisms may only
power off the CPU or the CPU card, while another may power off the
entire system. Others may really just execute a restart sequence
or drop into the ROM monitor. Using pm_power_off can also be racy
if the function pointer is set from a driver built as module, as the
driver may be in the process of being unloaded when pm_power_off is
called. If there are multiple poweroff handlers in the system, removing
a module with such a handler may inadvertently reset the pointer to
pm_power_off to NULL, leaving the system with no means to remove power.
Introduce a system poweroff handler call chain to solve the described
problems. This call chain is expected to be executed from the
architecture specific machine_power_off() function. Drivers providing
system poweroff functionality are expected to register with this call chain.
By using the priority field in the notifier block, callers can control
poweroff handler execution sequence and thus ensure that the poweroff
handler with the optimal capabilities to remove power for a given system
is called first.
What happened to this series? I want to add shutdown support to my
platform and I need to write a register on the PMIC in one driver to
configure it for shutdown instead of restart and then write an MMIO
register to tell the PMIC to actually do the shutdown in another driver.
It seems that the notifier solves this case for me, albeit with the
slight complication that I need to order the two with some priority.
I was wondering the same thing. I did find out that things kind of
stalled after Linus cast doubt on the chosen path [1]. I'm not sure
there's any consensus on what would be best to do instead.
Frans
[1] https://lkml.org/lkml/2014/11/6/641
On Thu, Jun 18, 2015 at 3:04 AM, Stephen Boyd [off-list ref] wrote:
quoted
On 10/06/2014 10:28 PM, Guenter Roeck wrote:
quoted
Various drivers implement architecture and/or device specific means to
remove power from the system. For the most part, those drivers set the
global variable pm_power_off to point to a function within the driver.
This mechanism has a number of drawbacks. Typically only one scheme
to remove power is supported (at least if pm_power_off is used).
At least in theory there can be multiple means remove power, some of
which may be less desirable. For example, some mechanisms may only
power off the CPU or the CPU card, while another may power off the
entire system. Others may really just execute a restart sequence
or drop into the ROM monitor. Using pm_power_off can also be racy
if the function pointer is set from a driver built as module, as the
driver may be in the process of being unloaded when pm_power_off is
called. If there are multiple poweroff handlers in the system, removing
a module with such a handler may inadvertently reset the pointer to
pm_power_off to NULL, leaving the system with no means to remove power.
Introduce a system poweroff handler call chain to solve the described
problems. This call chain is expected to be executed from the
architecture specific machine_power_off() function. Drivers providing
system poweroff functionality are expected to register with this call chain.
By using the priority field in the notifier block, callers can control
poweroff handler execution sequence and thus ensure that the poweroff
handler with the optimal capabilities to remove power for a given system
is called first.
What happened to this series? I want to add shutdown support to my
platform and I need to write a register on the PMIC in one driver to
configure it for shutdown instead of restart and then write an MMIO
register to tell the PMIC to actually do the shutdown in another driver.
It seems that the notifier solves this case for me, albeit with the
slight complication that I need to order the two with some priority.
I was wondering the same thing. I did find out that things kind of
stalled after Linus cast doubt on the chosen path [1]. I'm not sure
there's any consensus on what would be best to do instead.
Linus cast doubt on it, then the maintainers started picking it apart.
At the end, trying not to use notifier callbacks made the code so
complicated that even I didn't understand it anymore. With no consensus
in sight, I abandoned it.
Problem is really that the notifier call chain would be perfect to solve
the problem, yet Linus didn't like priorities (which are essential),
and the power maintainers didn't like that a call chain is supposed
to execute _all_ callbacks, which would not be the case here. If I were
to start again, I would insist to use notifiers. However, I don't see
a chance to get that accepted, so I won't. Feel free to pick it up and
give it a try yourself.
Thanks,
Guenter
From: Frans Klaver <hidden> Date: 2015-06-18 12:14:22
On Thu, Jun 18, 2015 at 1:54 PM, Guenter Roeck [off-list ref] wrote:
On 06/17/2015 11:53 PM, Frans Klaver wrote:
quoted
On Thu, Jun 18, 2015 at 3:04 AM, Stephen Boyd [off-list ref]
wrote:
quoted
On 10/06/2014 10:28 PM, Guenter Roeck wrote:
quoted
Various drivers implement architecture and/or device specific means to
remove power from the system. For the most part, those drivers set the
global variable pm_power_off to point to a function within the driver.
This mechanism has a number of drawbacks. Typically only one scheme
to remove power is supported (at least if pm_power_off is used).
At least in theory there can be multiple means remove power, some of
which may be less desirable. For example, some mechanisms may only
power off the CPU or the CPU card, while another may power off the
entire system. Others may really just execute a restart sequence
or drop into the ROM monitor. Using pm_power_off can also be racy
if the function pointer is set from a driver built as module, as the
driver may be in the process of being unloaded when pm_power_off is
called. If there are multiple poweroff handlers in the system, removing
a module with such a handler may inadvertently reset the pointer to
pm_power_off to NULL, leaving the system with no means to remove power.
Introduce a system poweroff handler call chain to solve the described
problems. This call chain is expected to be executed from the
architecture specific machine_power_off() function. Drivers providing
system poweroff functionality are expected to register with this call
chain.
By using the priority field in the notifier block, callers can control
poweroff handler execution sequence and thus ensure that the poweroff
handler with the optimal capabilities to remove power for a given system
is called first.
What happened to this series? I want to add shutdown support to my
platform and I need to write a register on the PMIC in one driver to
configure it for shutdown instead of restart and then write an MMIO
register to tell the PMIC to actually do the shutdown in another driver.
It seems that the notifier solves this case for me, albeit with the
slight complication that I need to order the two with some priority.
I was wondering the same thing. I did find out that things kind of
stalled after Linus cast doubt on the chosen path [1]. I'm not sure
there's any consensus on what would be best to do instead.
Linus cast doubt on it, then the maintainers started picking it apart.
At the end, trying not to use notifier callbacks made the code so
complicated that even I didn't understand it anymore. With no consensus
in sight, I abandoned it.
Problem is really that the notifier call chain would be perfect to solve
the problem, yet Linus didn't like priorities (which are essential),
and the power maintainers didn't like that a call chain is supposed
to execute _all_ callbacks, which would not be the case here. If I were
to start again, I would insist to use notifiers. However, I don't see
a chance to get that accepted, so I won't. Feel free to pick it up and
give it a try yourself.
How about having two phases? One where all interested parts of the
system get notified, one that does the final shutdown. It's a slightly
different approach than you took, but does use the notifier chains as
expected, and can be used to prepare peripherals for shutdown, if
there's a use case for it.
The two-stage approach does keep the single place to power down. I
expect it would become more obvious that it would be silly to have
more than one actual system power down sequence and hiding
pm_power_off and unifying setting of it should become more straight
forward as well.
Thoughts?
Thanks,
Frans
On Wed, Jun 17, 2015 at 06:04:54PM -0700, Stephen Boyd wrote:
[ ... ]
What happened to this series? I want to add shutdown support to my
platform and I need to write a register on the PMIC in one driver to
configure it for shutdown instead of restart and then write an MMIO
register to tell the PMIC to actually do the shutdown in another driver.
It seems that the notifier solves this case for me, albeit with the
slight complication that I need to order the two with some priority.
Can you use the .shutdown driver callback instead ?
I see other drivers use that, and check for system_state == SYSTEM_POWER_OFF
to power off the hardware.
Guenter
From: Stephen Boyd <hidden> Date: 2015-06-18 21:40:37
On 06/18/2015 08:30 AM, Guenter Roeck wrote:
On Wed, Jun 17, 2015 at 06:04:54PM -0700, Stephen Boyd wrote:
[ ... ]
quoted
What happened to this series? I want to add shutdown support to my
platform and I need to write a register on the PMIC in one driver to
configure it for shutdown instead of restart and then write an MMIO
register to tell the PMIC to actually do the shutdown in another driver.
It seems that the notifier solves this case for me, albeit with the
slight complication that I need to order the two with some priority.
Can you use the .shutdown driver callback instead ?
I see other drivers use that, and check for system_state == SYSTEM_POWER_OFF
to power off the hardware.
Yes I think that will work. I'll still have to hook pm_power_off() for
the mmio register, but I guess that's ok and I don't need to worry about
this series then.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project