RE: [PATCH] poweroff: fix bug in orderly_poweroff

[Feng Hong]

Hi, Eric

1. We are developing on an Android phone platform, we use thermal framework to monitor the temperature, when the temperature above like 110 degree, thermal framework will use orderly_shutdown to shutdown phone, however, on Android platform there is no " /sbin/poweroff " cmd ready . Then we want "fail ret" to trigger force shutdown (use kernel_power_off), but always we get "suc ret"
2. Here the caller just wait for "poweroff" userspace application, if it block the called, then it's the "poweroff" problem itself
3. As in the original orderly_shutdown design, we must get the right "ret", if this ret is always "0", then it obey orderly_poweroff design goal. Step 2: force shutdown is always useless code. 
int orderly_poweroff(bool force)
{
        int ret = __orderly_poweroff();

        if (ret && force) {
                printk(KERN_WARNING "Failed to start orderly shutdown: "
                       "forcing the issue\n");

                /*
                 * I guess this should try to kick off some daemon to sync and
                 * poweroff asap.  Or not even bother syncing if we're doing an
                 * emergency shutdown?
                 */
                emergency_sync();
                kernel_power_off();
        }

        return ret;
}1
--
Best Regards,
Feng Hong
Application Processor Software Engnieer 
Marvell Technology (Shanghai) Ltd


-----Original Message-----
From: Eric W. Biederman [mailto:ebiederm@xxxxxxxxxxxx] 
Sent: 2012年9月19日 12:47
To: Feng Hong
Cc: akpm@xxxxxxxxxxxxxxxxxxxx; gorcunov@xxxxxxxxxx; keescook@xxxxxxxxxxxx; serge.hallyn@xxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] poweroff: fix bug in orderly_poweroff

hongfeng <hongfeng@xxxxxxxxxxx> writes:

> orderly_poweroff is trying to poweroff platform by two steps:
> step 1: Call userspace application to poweroff
> step 2: If userspace poweroff fail, then do a force power off if force param is set.
>
> The bug here is, step 1 is always successful with param UMH_NO_WAIT,

This code has existed for 5 years.  Is this a recent regression?  Why
has no one complained before?

It looks to me that step 2 is:
step 2: If we can not launch the userspace poweroff fail.

> should change to UMH_WAIT_PROC which will monitor the return value
> ofuserspace application.

Is it safe to block indefinitely in the callers waiting for userspace?

If the caller is not running in a kernel thread then we can easily get
into a case where the userspace caller will block waiting for us when we
are waiting for the userspace caller.

I don't want to impeded progress but I don't see the evidence that this
change is good enough.


> Change-Id: I2f9ebbb90c0c2443780080ec9507c8d004e5da74
> Signed-off-by: Feng Hong <hongfeng@xxxxxxxxxxx>
> ---
>  kernel/sys.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 241507f..1b30b30 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2204,7 +2204,7 @@ static int __orderly_poweroff(void)
>  		return -ENOMEM;
>  	}
>  
> -	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_NO_WAIT,
> +	ret = call_usermodehelper_fns(argv[0], argv, envp, UMH_WAIT_PROC,
>  				      NULL, argv_cleanup, NULL);
>  	if (ret == -ENOMEM)
>  		argv_free(argv);
韬{.n?????%?lzwm?b?Р骒r?zXЩ??{ay????j?f＂?????ア?⒎?:+v???????赙zZ+????"?!?O???v??m?鹈n?帼Y&—