[v7,2/2] ARM hibernation / suspend-to-disk

On 19 March 2014 08:44, Ezequiel Garcia
<ezequiel.garcia@free-electrons.com> wrote:
> Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Thanks!
> On the other side, this board has no pm_power_off() support, which means
> kernel_halt() is called after kernel_power_off().
>
> I'm not sure if a NULL pm_power_off() is supported, but this makes my kernel
> crash in a reboot notifier that's called twice (first in kernel_power_off
> and then in kernel_halt):

The omap board I'm using has a null pm_power_off.  I added code to
bypass kernel_power_off in the event pm_power_off is null like this:




This follows the behavior in the reboot syscall which does it this way
also.  I'm testing this now, and it seems work fine.  If this looks
good, I can add it as an additional patch.

Thanks,

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 19 March 2014 13:47, Sebastian Capella <sebastian.capella@linaro.org> wrote:
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index a5f702a..d96b910 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -594,7 +594,8 @@ static void power_down(void)
>         case HIBERNATION_PLATFORM:
>                 hibernation_platform_enter();
>         case HIBERNATION_SHUTDOWN:
> -               kernel_power_off();
> +               if (pm_power_off)
> +                       kernel_power_off();
>                 break;
>  #ifdef CONFIG_SUSPEND
>         case HIBERNATION_SUSPEND:
>
>
> This follows the behavior in the reboot syscall which does it this way
> also.  I'm testing this now, and it seems work fine.  If this looks
> good, I can add it as an additional patch.

BTW, one thing I would point out is that kernel_power_off and
kernel_halt call the same notifier but with different parameters
(SYS_POWER_OFF and SYS_HALT).

If pm_power_down is null, I dont see why we'd want to notify
SYS_POWER_OFF before SYS_HALT.  With the previous change I'm assuming
there's no benefit, so please chime in if you know a reason.

Thanks,

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

Hi,

I have no further changes here..  I will submit a separate patch to
cover Ezequiel's concern.  Any final comments?  Otherwise, I'll submit
to Russell's system.

Thanks for the review!

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

Am 19.03.2014 22:06, schrieb Sebastian Capella:
> On 19 March 2014 13:47, Sebastian Capella <sebastian.capella@linaro.org> wrote:
>> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
>> index a5f702a..d96b910 100644
>> --- a/kernel/power/hibernate.c
>> +++ b/kernel/power/hibernate.c
>> @@ -594,7 +594,8 @@ static void power_down(void)
>>          case HIBERNATION_PLATFORM:
>>                  hibernation_platform_enter();
>>          case HIBERNATION_SHUTDOWN:
>> -               kernel_power_off();
>> +               if (pm_power_off)
>> +                       kernel_power_off();
>>                  break;
>>   #ifdef CONFIG_SUSPEND
>>          case HIBERNATION_SUSPEND:
>>
>>
>> This follows the behavior in the reboot syscall which does it this way
>> also.  I'm testing this now, and it seems work fine.  If this looks
>> good, I can add it as an additional patch.
>
> BTW, one thing I would point out is that kernel_power_off and
> kernel_halt call the same notifier but with different parameters
> (SYS_POWER_OFF and SYS_HALT).
>
> If pm_power_down is null, I dont see why we'd want to notify
> SYS_POWER_OFF before SYS_HALT.  With the previous change I'm assuming
> there's no benefit, so please chime in if you know a reason.

Both states, power off and sys halt, do sound pretty final and I would 
assume something is broken, if power off is called before sys halt or 
vice versa. At least I would never expect that the 
reboot/poweroff/syshalt notifier may be called twice (and thats why the 
heartbeat-trigger may crash).

But just in case, changing that behaviour in ledtrig-heartbeat.c would 
be pretty easy, just remove the heartbeat_reboot_notifier (which plays 
nice and deregisters the trigger on reboot) and use the panic_notifier 
(which doesn't unregister the trigger but just turns off the led) for 
reboot too. Another solution would be to unregister the reboot_notifier 
in the reboot_nofifier itself. I've just seen one watchdog driver 
(drivers/rtc/rtc-m41t80.c) which does that. But I still think such 
shouldn't be necessary (and I haven't had a look at other 
reboot_notifier users).


Regards,

Alexander Holler
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Alexander,

Thanks for your comments.

I've posted a different solution for arm only here, that just causes a
while loop in machine_power_off in the event it's called but does not
halt.

https://lkml.org/lkml/2014/3/24/299

On 25 March 2014 11:38, Alexander Holler <holler@ahsoftware.de> wrote:
> Both states, power off and sys halt, do sound pretty final and I would
> assume something is broken, if power off is called before sys halt or vice
> versa. At least I would never expect that the reboot/poweroff/syshalt
> notifier may be called twice (and thats why the heartbeat-trigger may
> crash).
>
> But just in case, changing that behaviour in ledtrig-heartbeat.c would be
> pretty easy, just remove the heartbeat_reboot_notifier (which plays nice and
> deregisters the trigger on reboot) and use the panic_notifier (which doesn't
> unregister the trigger but just turns off the led) for reboot too. Another
> solution would be to unregister the reboot_notifier in the reboot_nofifier
> itself. I've just seen one watchdog driver (drivers/rtc/rtc-m41t80.c) which
> does that. But I still think such shouldn't be necessary (and I haven't had
> a look at other reboot_notifier users).

I hope this will all be handled by the loop in machine_power_off, so
maybe this will not be needed.

Thanks!

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

Am 25.03.2014 19:38, schrieb Alexander Holler:

> reboot too. Another solution would be to unregister the reboot_notifier
> in the reboot_nofifier itself. I've just seen one watchdog driver
> (drivers/rtc/rtc-m41t80.c) which does that. But I still think such

That, btw. is broken. ;)

Right after having send the mail, I've became that intuition, had a look 
and ... notifier.h does state the following:

  * atomic_notifier_chain_unregister(), 
blocking_notifier_chain_unregister(),
  * and srcu_notifier_chain_unregister() _must not_ be called from within
  * the call chain.

(The reboot-notifier chain is of type blocking_notifier_chain)

So I've picked up one user of reboot_notifier by random and had the luck 
to choose a broken one. ;)

I will make a patch and will have a look if the same failure can be 
found elsewhere. There aren't that much users of the reboot-notifier, so 
it shouldn't cost me that much time.

Regards,

Alexander Holler


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Am 26.03.2014 00:36, schrieb Alexander Holler:
> Am 25.03.2014 19:38, schrieb Alexander Holler:
>
>> reboot too. Another solution would be to unregister the reboot_notifier
>> in the reboot_nofifier itself. I've just seen one watchdog driver
>> (drivers/rtc/rtc-m41t80.c) which does that. But I still think such
>
> That, btw. is broken. ;)
>
> Right after having send the mail, I've became that intuition, had a look
> and ... notifier.h does state the following:
>
>   * atomic_notifier_chain_unregister(),
> blocking_notifier_chain_unregister(),
>   * and srcu_notifier_chain_unregister() _must not_ be called from within
>   * the call chain.
>
> (The reboot-notifier chain is of type blocking_notifier_chain)
>
> So I've picked up one user of reboot_notifier by random and had the luck
> to choose a broken one. ;)
>
> I will make a patch and will have a look if the same failure can be
> found elsewhere. There aren't that much users of the reboot-notifier, so
> it shouldn't cost me that much time.

Hmm, and either I was confused, or have looked at some other user of the 
reboot_notifier, but rtc-m41t80.c doesn't call unregister from it's 
notifier. And unfortunately there are a bit more users of the 
reboot_notifier than I first thought. :/

I will check if I can find out at least at which driver I had a look at 
which did call unregister_notifier from the notifier itself.

Regards,

Alexander Holler

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