| Message ID | 1532941173-26969-1-git-send-email-kernelfans@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | pm/reboot: eliminate race between reboot and suspend | expand |
On Mon, Jul 30, 2018 at 10:59 AM, Pingfan Liu <kernelfans@gmail.com> wrote: > At present, "systemctl suspend" and "shutdown" can run in parrallel. A > system can suspend after devices_shutdown(), and resume. Then the shutdown > task goes on to power off. This causes many devices are not really shut > off. Hence replacing reboot_mutex with pm_mutex to achieve the exclusion. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Len Brown <len.brown@intel.com> > Cc: Pavel Machek <pavel@ucw.cz> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com> > --- > kernel/power/main.c | 3 +-- > kernel/reboot.c | 6 +++--- > 2 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/kernel/power/main.c b/kernel/power/main.c > index d9706da..b5b3ac6 100644 > --- a/kernel/power/main.c > +++ b/kernel/power/main.c > @@ -15,11 +15,10 @@ > #include <linux/workqueue.h> > #include <linux/debugfs.h> > #include <linux/seq_file.h> > +#include <linux/suspend.h> > > #include "power.h" > > -DEFINE_MUTEX(pm_mutex); > - > #ifdef CONFIG_PM_SLEEP > > void lock_system_sleep(void) > diff --git a/kernel/reboot.c b/kernel/reboot.c > index e4ced88..45e9db7 100644 > --- a/kernel/reboot.c > +++ b/kernel/reboot.c > @@ -294,7 +294,7 @@ void kernel_power_off(void) > } > EXPORT_SYMBOL_GPL(kernel_power_off); > > -static DEFINE_MUTEX(reboot_mutex); > +DEFINE_MUTEX(pm_mutex); > > /* > * Reboot system call: for obvious reasons only root may call it, > @@ -338,7 +338,7 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd, > if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off) > cmd = LINUX_REBOOT_CMD_HALT; > > - mutex_lock(&reboot_mutex); > + mutex_lock(&pm_mutex); > switch (cmd) { > case LINUX_REBOOT_CMD_RESTART: > kernel_restart(NULL); > @@ -389,7 +389,7 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd, > ret = -EINVAL; > break; > } > - mutex_unlock(&reboot_mutex); > + mutex_unlock(&pm_mutex); > return ret; > } > Well, fair enough, but maybe rename the mutex then too?
On Mon, Jul 30, 2018 at 5:10 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Mon, Jul 30, 2018 at 10:59 AM, Pingfan Liu <kernelfans@gmail.com> wrote: > > At present, "systemctl suspend" and "shutdown" can run in parrallel. A > > system can suspend after devices_shutdown(), and resume. Then the shutdown > > task goes on to power off. This causes many devices are not really shut > > off. Hence replacing reboot_mutex with pm_mutex to achieve the exclusion. > > > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > > Cc: Len Brown <len.brown@intel.com> > > Cc: Pavel Machek <pavel@ucw.cz> > > Signed-off-by: Pingfan Liu <kernelfans@gmail.com> > > --- > > kernel/power/main.c | 3 +-- > > kernel/reboot.c | 6 +++--- > > 2 files changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/kernel/power/main.c b/kernel/power/main.c > > index d9706da..b5b3ac6 100644 > > --- a/kernel/power/main.c > > +++ b/kernel/power/main.c > > @@ -15,11 +15,10 @@ > > #include <linux/workqueue.h> > > #include <linux/debugfs.h> > > #include <linux/seq_file.h> > > +#include <linux/suspend.h> > > > > #include "power.h" > > > > -DEFINE_MUTEX(pm_mutex); > > - > > #ifdef CONFIG_PM_SLEEP > > > > void lock_system_sleep(void) > > diff --git a/kernel/reboot.c b/kernel/reboot.c > > index e4ced88..45e9db7 100644 > > --- a/kernel/reboot.c > > +++ b/kernel/reboot.c > > @@ -294,7 +294,7 @@ void kernel_power_off(void) > > } > > EXPORT_SYMBOL_GPL(kernel_power_off); > > > > -static DEFINE_MUTEX(reboot_mutex); > > +DEFINE_MUTEX(pm_mutex); > > > > /* > > * Reboot system call: for obvious reasons only root may call it, > > @@ -338,7 +338,7 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd, > > if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off) > > cmd = LINUX_REBOOT_CMD_HALT; > > > > - mutex_lock(&reboot_mutex); > > + mutex_lock(&pm_mutex); > > switch (cmd) { > > case LINUX_REBOOT_CMD_RESTART: > > kernel_restart(NULL); > > @@ -389,7 +389,7 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd, > > ret = -EINVAL; > > break; > > } > > - mutex_unlock(&reboot_mutex); > > + mutex_unlock(&pm_mutex); > > return ret; > > } > > > > Well, fair enough, but maybe rename the mutex then too? What about power_mutex, removing the meaning of "manage" ? Thanks, Pingfan
On Mon, Jul 30, 2018 at 12:32 PM, Pingfan Liu <kernelfans@gmail.com> wrote: > On Mon, Jul 30, 2018 at 5:10 PM Rafael J. Wysocki <rafael@kernel.org> wrote: >> >> On Mon, Jul 30, 2018 at 10:59 AM, Pingfan Liu <kernelfans@gmail.com> wrote: >> > At present, "systemctl suspend" and "shutdown" can run in parrallel. A >> > system can suspend after devices_shutdown(), and resume. Then the shutdown >> > task goes on to power off. This causes many devices are not really shut >> > off. Hence replacing reboot_mutex with pm_mutex to achieve the exclusion. >> > >> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> >> > Cc: Len Brown <len.brown@intel.com> >> > Cc: Pavel Machek <pavel@ucw.cz> >> > Signed-off-by: Pingfan Liu <kernelfans@gmail.com> >> > --- >> > kernel/power/main.c | 3 +-- >> > kernel/reboot.c | 6 +++--- >> > 2 files changed, 4 insertions(+), 5 deletions(-) >> > >> > diff --git a/kernel/power/main.c b/kernel/power/main.c >> > index d9706da..b5b3ac6 100644 >> > --- a/kernel/power/main.c >> > +++ b/kernel/power/main.c >> > @@ -15,11 +15,10 @@ >> > #include <linux/workqueue.h> >> > #include <linux/debugfs.h> >> > #include <linux/seq_file.h> >> > +#include <linux/suspend.h> >> > >> > #include "power.h" >> > >> > -DEFINE_MUTEX(pm_mutex); >> > - >> > #ifdef CONFIG_PM_SLEEP >> > >> > void lock_system_sleep(void) >> > diff --git a/kernel/reboot.c b/kernel/reboot.c >> > index e4ced88..45e9db7 100644 >> > --- a/kernel/reboot.c >> > +++ b/kernel/reboot.c >> > @@ -294,7 +294,7 @@ void kernel_power_off(void) >> > } >> > EXPORT_SYMBOL_GPL(kernel_power_off); >> > >> > -static DEFINE_MUTEX(reboot_mutex); >> > +DEFINE_MUTEX(pm_mutex); >> > >> > /* >> > * Reboot system call: for obvious reasons only root may call it, >> > @@ -338,7 +338,7 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd, >> > if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off) >> > cmd = LINUX_REBOOT_CMD_HALT; >> > >> > - mutex_lock(&reboot_mutex); >> > + mutex_lock(&pm_mutex); >> > switch (cmd) { >> > case LINUX_REBOOT_CMD_RESTART: >> > kernel_restart(NULL); >> > @@ -389,7 +389,7 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd, >> > ret = -EINVAL; >> > break; >> > } >> > - mutex_unlock(&reboot_mutex); >> > + mutex_unlock(&pm_mutex); >> > return ret; >> > } >> > >> >> Well, fair enough, but maybe rename the mutex then too? > > What about power_mutex, removing the meaning of "manage" ? It really is not just about power, but about system transitions. Something like system_transition_lock would be unambiguous enough IMO.
Hi! Acked-by: Pavel Machek <pavel@ucw.cz> > >> > } > >> > - mutex_unlock(&reboot_mutex); > >> > + mutex_unlock(&pm_mutex); > >> > return ret; > >> > } > >> > > >> > >> Well, fair enough, but maybe rename the mutex then too? > > > > What about power_mutex, removing the meaning of "manage" ? > > It really is not just about power, but about system transitions. > Something like system_transition_lock would be unambiguous enough IMO. Actually, pm_mutex seems good enough to me. Yes, you can always me more precise with longer names, but... (and it should really be system_transition_mutex, if anything). Pavel
On Mon, Jul 30, 2018 at 10:48 PM Pavel Machek <pavel@ucw.cz> wrote: > > Hi! > > Acked-by: Pavel Machek <pavel@ucw.cz> > > > > >> > } > > >> > - mutex_unlock(&reboot_mutex); > > >> > + mutex_unlock(&pm_mutex); > > >> > return ret; > > >> > } > > >> > > > >> > > >> Well, fair enough, but maybe rename the mutex then too? > > > > > > What about power_mutex, removing the meaning of "manage" ? > > > > It really is not just about power, but about system transitions. > > Something like system_transition_lock would be unambiguous enough IMO. > > Actually, pm_mutex seems good enough to me. Yes, you can always me > more precise with longer names, but... (and it should really be > system_transition_mutex, if anything). > I think that system_transition_mutex can satisfy both of yours' opinion. And will send V2. Thanks, Pingfan
diff --git a/kernel/power/main.c b/kernel/power/main.c index d9706da..b5b3ac6 100644 --- a/kernel/power/main.c +++ b/kernel/power/main.c @@ -15,11 +15,10 @@ #include <linux/workqueue.h> #include <linux/debugfs.h> #include <linux/seq_file.h> +#include <linux/suspend.h> #include "power.h" -DEFINE_MUTEX(pm_mutex); - #ifdef CONFIG_PM_SLEEP void lock_system_sleep(void) diff --git a/kernel/reboot.c b/kernel/reboot.c index e4ced88..45e9db7 100644 --- a/kernel/reboot.c +++ b/kernel/reboot.c @@ -294,7 +294,7 @@ void kernel_power_off(void) } EXPORT_SYMBOL_GPL(kernel_power_off); -static DEFINE_MUTEX(reboot_mutex); +DEFINE_MUTEX(pm_mutex); /* * Reboot system call: for obvious reasons only root may call it, @@ -338,7 +338,7 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd, if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off) cmd = LINUX_REBOOT_CMD_HALT; - mutex_lock(&reboot_mutex); + mutex_lock(&pm_mutex); switch (cmd) { case LINUX_REBOOT_CMD_RESTART: kernel_restart(NULL); @@ -389,7 +389,7 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd, ret = -EINVAL; break; } - mutex_unlock(&reboot_mutex); + mutex_unlock(&pm_mutex); return ret; }
At present, "systemctl suspend" and "shutdown" can run in parrallel. A system can suspend after devices_shutdown(), and resume. Then the shutdown task goes on to power off. This causes many devices are not really shut off. Hence replacing reboot_mutex with pm_mutex to achieve the exclusion. Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: Len Brown <len.brown@intel.com> Cc: Pavel Machek <pavel@ucw.cz> Signed-off-by: Pingfan Liu <kernelfans@gmail.com> --- kernel/power/main.c | 3 +-- kernel/reboot.c | 6 +++--- 2 files changed, 4 insertions(+), 5 deletions(-)