1107923 - Use powerctl for reboot & shutdown

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Oleksiy Avramchenko]

Android KK and later supports sys.powerctl property to reboot or shutdown device via the init. Transition is catched by init and it calls powerctl built-in to remount all filesystems to read-only mode, sync and perform reboot. In addition, the good thing is that it's possible to script shutdown actions if it's performed this way.

Comment 1

[Oleksiy Avramchenko]

Attachment: bug-1107923-Use-powerctl-for-reboot-shutdown.patch

Comment 2

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8532541 [details] [diff] [review]
bug-1107923-Use-powerctl-for-reboot-shutdown.patch

Review of attachment 8532541 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing the review from Dave. The functionality looks good overall but I'd like to see some changes before we land it which is why I'm r-ing it.

::: hal/gonk/GonkHal.cpp
@@ +1792,5 @@
> +  property_set("sys.powerctl", aValue);
> +  // device should reboot in few moments, but if it doesn't - call
> +  // android_reboot() to make sure that init isn't stuck somewhere
> +  sleep(10);
> +  HAL_LOG(("Powerctl call takes too long, forcing %s.", aValue));

We don't use the double-parentheses style anymore since bug 1047277 landed. Change this to:

HAL_LOG("Powerctl call takes too long, forcing %s.", aValue);

@@ +1797,5 @@
> +  android_reboot(aCmd, 0, aRebootTarget);
> +}
> +
> +void
> +PowerCtlReboot(const char *aTarget)

This function is never used with an explicit argument, always with the default one so I'd rather see the argument removed and the wrapped call to PowerCtl() always pick "reboot" as its first parameter. It's no use adding functionality that we're not leveraging.

::: hal/linux/LinuxPower.cpp
@@ +14,4 @@
>  namespace mozilla {
>  namespace hal_impl {
>  
> +#if (defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 19)

I find using this code here and defining it in gonk/hal/GonkHal.cpp kind of clunky. This is really gonk-specific but but I don't think we should be moving it to hal/gonk/GonkHal.cpp because we'd duplicate some. IMO what we should make here is:

1. Conditionally include "cutils/android_reboot.h" and "cutils/properties.h" here if MOZ_WIDGET_GONK is defined (I don't think we need anything else).

2. Move the implementation of PowerCtl() and friends here.

2. If MOZ_WIDGET_GONK is defined and we're on KitKat build PowerCtl() & co and use it to power off / restart like you did here already.

3. If MOZ_WIDGET_GONK is defined and we're not on KitKat use android_reboot() instead of the regular sync()/reboot() we use for generic Linux. android_reboot() should be available across all Android versions we care about.

Comment 3

[Oleksiy Avramchenko]

Attachment: bug-1107923-Use-powerctl-for-reboot-shutdown-rev2.patch

Thanks for the review, could you please take a look at the updated version?

The reason why PowerCtlReboot() and PowerCtl() took an extra parameter was that I thought to use it for factory reset as well, but it didn't happen. Since keeping one-liner wrappers seem to be an overkill now removed them as well.

Comment 4

[Gabriele Svelto [:gsvelto] (PTO)]

Comment on attachment 8534468 [details] [diff] [review]
bug-1107923-Use-powerctl-for-reboot-shutdown-rev2.patch

Review of attachment 8534468 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me with the nit I've commented on addressed, thanks for your contribution!

::: hal/linux/LinuxPower.cpp
@@ +20,5 @@
>  namespace hal_impl {
>  
> +#if (defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 19)
> +static void
> +PowerCtl(const char *aValue, int aCmd)

nit: Use 'const char* aValue' here as per our coding guidelines. More info here if you're curious https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Declarations

Comment 5

[Gabriele Svelto [:gsvelto] (PTO)]

BTW assigning the bug to you, I hadn't noticed it wasn't assigned yet.

Comment 6

[Oleksiy Avramchenko]

Attachment: bug-1107923-Use-powerctl-for-reboot-shutdown-rev3.patch

Changed 'const char *' to 'const char*'

Comment 7

[Oleksiy Avramchenko]

Thanks, Gabriele, the style's fixed.

Comment 8

[Gabriele Svelto [:gsvelto] (PTO)]

Oleksiy, you'll need to make a try-run before the patch can land:

https://wiki.mozilla.org/ReleaseEngineering/TryServer

While this piece of code is not covered by unit tests (IIRC) you should at least do a build across all architectures to ensure they all compile correctly.

Comment 9

[Oleksiy Avramchenko]

(In reply to Gabriele Svelto [:gsvelto] from comment #8)
> Oleksiy, you'll need to make a try-run before the patch can land:
> 
> https://wiki.mozilla.org/ReleaseEngineering/TryServer
> 
> While this piece of code is not covered by unit tests (IIRC) you should at
> least do a build across all architectures to ensure they all compile
> correctly.

Yep, got it. Don't have access at the moment, will take some time to get L1.

Comment 10

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Oleksiy Avramchenko from comment #9)
> Yep, got it. Don't have access at the moment, will take some time to get L1.

OK, I'll do the try-run myself so we can land this soon. If you need someone to vouch for you just ask me, I'll find someone to vouch for you (ironically I'm not level 2 yet, though I'm one of the HAL module peers).

Comment 11

[Gabriele Svelto [:gsvelto] (PTO)]

The Try run results can be found here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=89b869cdb76c

Comment 12

[Gabriele Svelto [:gsvelto] (PTO)]

Try is green, time to land.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5dfa513496b

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Followup to fix a non-unified build failure that popped up a few hours after this landed
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed0bd58b64de

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4684807&repo=mozilla-inbound

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c5dfa513496b
https://hg.mozilla.org/mozilla-central/rev/ed0bd58b64de