Re: 2.6.0-ben3: Badness in redraw_screen

[Kevin B.Hendricks <kevin.hendricks@sympatico.ca>]

Hi,

In the first hunk of your patch you can possibly grab the semaphore and 
then do a return 1 with the semaphore held.

If you really need to hold the semaphore to do a vc_allocate then you 
should remember to release that semaphore before doing the return 1

So something along the lines of ...

acquire_console_sem();
if (vc_allocate(SUSPEND_CONSOLE)) {
   release_console_sme();
   return 1;
}

would be better I think if you really do need to hold the console_sem() 
before calling vc_allocate.

Kevin



On Dec 31, 2003, at 9:15 AM, Michael Schmitz wrote:

> [CC to debian-powerpc as this was reported there as well ...]
>
> > > I'm getting this badness in redraw_screen when my iBook wakes up with
> > > 2.6.0-ben3:
> > >
> > > Badness in redraw_screen at drivers/char/vt.c:596
> > > Call trace:
> > >  [c000bd50] dump_stack+0x18/0x28
> > >  [c0008c44] check_bug_trap+0x84/0xac
> > >  [c0008d34] ProgramCheckException+0xc8/0x180
> > >  [c00082cc] ret_from_except_full+0x0/0x4c
> > >  [c015a94c] redraw_screen+0x2c/0x1e0
> > >  [c00376b4] pm_restore_console+0x38/0x48
> > >  [c039ef14] 0xc039ef14
> > >  [c039f468] 0xc039f468
> > >  [c039fdc8] 0xc039fdc8
> > >  [c00701d0] sys_ioctl+0x278/0x2f4
> > >  [c0007d0c] ret_from_syscall+0x0/0x4c
> >
> > Similar badness can be had in set_origin (vt.c:568) and set_palette
> > (vt.c:2851), on my Lombard. That's called from pm_prepare_console, 
> > for a
> > change.
> >
> > > (The three unnamed frames are inside pmac_wakeup_devices,
> > > powerbook_sleep_Core99 and pmu_ioctl).  Does pm_restore_console need
> > > to acquire the console lock, or is that the duty of the callers?
> >
> > None of the other callers does this so I'd guess it needs to be done 
> > in
> > pm_restore_console (and pm_prepare_console).
>
> The following patch works for me... should we submit that to lkml?
>
> 	Michael
>
> --- kernel/power/console.c.org	2003-12-31 14:08:04.000000000 +0100
> +++ kernel/power/console.c	2003-12-31 14:09:26.000000000 +0100
> @@ -20,12 +20,12 @@
>  #ifdef SUSPEND_CONSOLE
>  	orig_fgconsole = fg_console;
>
> +	acquire_console_sem();
>  	if (vc_allocate(SUSPEND_CONSOLE))
>  	  /* we can't have a free VC for now. Too bad,
>  	   * we don't want to mess the screen for now. */
>  		return 1;
>
> -	acquire_console_sem();
>  	set_console(SUSPEND_CONSOLE);
>  	release_console_sem();
>  	if (vt_waitactive(SUSPEND_CONSOLE)) {
> @@ -42,12 +42,14 @@
>  {
>  	console_loglevel = orig_loglevel;
>  #ifdef SUSPEND_CONSOLE
> +	acquire_console_sem();
>  	set_console(orig_fgconsole);
>
>  	/* FIXME:
>  	 * This following part is left over from swsusp. Is it really needed?
>  	 */
>  	update_screen(fg_console);
> +	release_console_sem();
>  #endif
>  	return;
>  }
>
>
> --
> To UNSUBSCRIBE, email to debian-powerpc-request@lists.debian.org
> with a subject of "unsubscribe". Trouble? Contact 
> listmaster@lists.debian.org