Re: [linux-pm] [PATCH] PM: Fix suspend_console/resume_console touse only one semaphore.

[Benjamin Herrenschmidt]

On Mon, 2009-02-02 at 18:03 -0800, Arve HjÃnnevÃg wrote:
> This fixes a race where a thread acquires the console while the
> console is suspended, and the console is resumed before this
> thread releases it. In this case, the secondary console
> semaphore would be left locked, and the primary semaphore would
> be released twice. This in turn would cause the console switch
> on suspend or resume to hang forever.
> 
> Note that suspend_console does not actually lock the console
> for clients that use acquire_console_sem, it only locks it for
> clients that use try_acquire_console_sem. If we change
> suspend_console to fully lock the console, then the kernel
> may deadlock on suspend. One client of try_acquire_console_sem
> is acquire_console_semaphore_for_printk, which uses it to
> prevent printk from using the console while it is suspended.

Right, we shouldn't hold the semaphore over the whole time. In fact, I
even have some doubts about the need for suspending the console at
all...

The way I did it back then for powerbooks was to implement a "suspended"
state in the fbdev/fbcon core which the drivers call effectively causing
the core to stop touching the framebuffer. Works fine for kernel
consoles though of course userspace or X needs something different,
but that's orthogonal.

Of course that doesn't help with serial, vgacon etc... so each drivers
needs to suspend itself and be made to ignore incoming requests but
that's something that has to be done anyway at some stage...

I suspect the suspend console trick is mostly helpful with vgacon..

Ben.


> Signed-off-by: Arve HjÃnnevÃg <arve@xxxxxxxxxxx>
> ---
>  kernel/printk.c |   15 +++++++++------
>  1 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/printk.c b/kernel/printk.c
> index 69188f2..e3602d0 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -73,7 +73,6 @@ EXPORT_SYMBOL(oops_in_progress);
>   * driver system.
>   */
>  static DECLARE_MUTEX(console_sem);
> -static DECLARE_MUTEX(secondary_console_sem);
>  struct console *console_drivers;
>  EXPORT_SYMBOL_GPL(console_drivers);
>  
> @@ -891,12 +890,14 @@ void suspend_console(void)
>  	printk("Suspending console(s) (use no_console_suspend to debug)\n");
>  	acquire_console_sem();
>  	console_suspended = 1;
> +	up(&console_sem);
>  }
>  
>  void resume_console(void)
>  {
>  	if (!console_suspend_enabled)
>  		return;
> +	down(&console_sem);
>  	console_suspended = 0;
>  	release_console_sem();
>  }
> @@ -912,11 +913,9 @@ void resume_console(void)
>  void acquire_console_sem(void)
>  {
>  	BUG_ON(in_interrupt());
> -	if (console_suspended) {
> -		down(&secondary_console_sem);
> -		return;
> -	}
>  	down(&console_sem);
> +	if (console_suspended)
> +		return;
>  	console_locked = 1;
>  	console_may_schedule = 1;
>  }
> @@ -926,6 +925,10 @@ int try_acquire_console_sem(void)
>  {
>  	if (down_trylock(&console_sem))
>  		return -1;
> +	if (console_suspended) {
> +		up(&console_sem);
> +		return -1;
> +	}
>  	console_locked = 1;
>  	console_may_schedule = 0;
>  	return 0;
> @@ -979,7 +982,7 @@ void release_console_sem(void)
>  	unsigned wake_klogd = 0;
>  
>  	if (console_suspended) {
> -		up(&secondary_console_sem);
> +		up(&console_sem);
>  		return;
>  	}
>  
> -- 
> 1.6.1
> 
> _______________________________________________
> linux-pm mailing list
> linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm

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