filo circular console list

[Arne Georg Gleditsch]

Hi,

The latest svn revisions of filo and libpayload (r83 and r3708) conspire
to add the video console driver twice to the console_out list.  This
causes the list to become circular, with the effect that device_putchar
spins in a loop printing the first character it is given.  ("F".)

Trivial patch and .config appended.  (One might perhaps also consider
making device_putchar or console_add_* more robust in the face of this
kind of error, but I settled for this.)

[Jordan Crouse]

On 30/10/08 13:14 +0100, Arne Georg Gleditsch wrote:
> Hi,
>
> The latest svn revisions of filo and libpayload (r83 and r3708) conspire
> to add the video console driver twice to the console_out list.  This
> causes the list to become circular, with the effect that device_putchar
> spins in a loop printing the first character it is given.  ("F".)
>
> Trivial patch and .config appended.  (One might perhaps also consider
> making device_putchar or console_add_* more robust in the face of this
> kind of error, but I settled for this.)

Hmmm - okay - I think I sort of see whats going on here.  I'm not
sure why they are calling console_init() (or video_console_init())
and lib_get_sysinfo() in the first place - that should have already
been run before callin main().   If you are right about
the console_out list, and I think you are, then running console_init()
twice should cause the same problem (or perhaps it doesn't end up 
looping).  Anyway, thats bad.  We need to get Patrick and Stefan to
explain what they did here.

Jordan
> Index: main/filo.c
>
> --- main/filo.c	(revision 83)
> +++ main/filo.c	(working copy)
> @@ -49,9 +49,6 @@
>  {
>      /* Set up the consoles. */
>      console_init();
> -#ifdef CONFIG_VIDEO_CONSOLE
> video_console_init();
>
> -#endif
>  /* Gather system information, and implicitly sets up timers */
>  lib_get_sysinfo();
> #
> # Automatically generated make config: don't edit
> # FILO version: 0.6.0rc1
> # Thu Oct 30 12:01:16 2008
> #
> CONFIG_TARGET_I386=y
> # CONFIG_REVIEW is not set
>
> #
> # Interface Options
> #
> CONFIG_USE_GRUB=y
> CONFIG_PROMPT="filo"
> CONFIG_MENULST_FILE="hde1:/grub/menu.lst"
> CONFIG_MENULST_TIMEOUT=3
> CONFIG_USE_MD5_PASSWORDS=y
>
> #
> # Drivers
> #
> CONFIG_IDE_DISK=y
> CONFIG_IDE_DISK_POLL_DELAY=0
> # CONFIG_SLOW_SATA is not set
> # CONFIG_PCMCIA_CF is not set
> CONFIG_USB_NEW_DISK=y
> # CONFIG_USB_DISK is not set
> # CONFIG_FLASH_DISK is not set
> CONFIG_SUPPORT_PCI=y
> # CONFIG_PCI_BRUTE_SCAN is not set
> # CONFIG_SUPPORT_SOUND is not set
>
> #
> # Filesystems
> #
> CONFIG_FSYS_EXT2FS=y
> CONFIG_FSYS_FAT=y
> # CONFIG_FSYS_JFS is not set
> # CONFIG_FSYS_MINIX is not set
> # CONFIG_FSYS_REISERFS is not set
> # CONFIG_FSYS_XFS is not set
> CONFIG_FSYS_ISO9660=y
> CONFIG_ELTORITO=y
> # CONFIG_FSYS_CRAMFS is not set
> # CONFIG_FSYS_SQUASHFS is not set
>
> #
> # Loaders
> #
> CONFIG_LINUX_LOADER=y
> # CONFIG_WINCE_LOADER is not set
> # CONFIG_ARTEC_BOOT is not set
>
> #
> # Debugging & Experimental
> #
> # CONFIG_EXPERIMENTAL is not set
> # CONFIG_DEBUG_ALL is not set
> # CONFIG_DEBUG_ELFBOOT is not set
> # CONFIG_DEBUG_ELFNOTE is not set
> # CONFIG_DEBUG_SEGMENT is not set
> # CONFIG_DEBUG_SYS_INFO is not set
> # CONFIG_DEBUG_BLOCKDEV is not set
> # CONFIG_DEBUG_VFS is not set
> # CONFIG_DEBUG_FSYS_EXT2FS is not set
> # CONFIG_DEBUG_PCI is not set
> # CONFIG_DEBUG_LINUXLOAD is not set
> # CONFIG_DEBUG_IDE is not set
> # CONFIG_DEBUG_ELTORITO is not set
> CONFIG_DEVELOPER_TOOLS=y
>

[Jordan Crouse]

On 30/10/08 08:38 -0600, Jordan Crouse wrote:
> On 30/10/08 13:14 +0100, Arne Georg Gleditsch wrote:
> > Hi,
> >
> > The latest svn revisions of filo and libpayload (r83 and r3708) conspire
> > to add the video console driver twice to the console_out list.  This
> > causes the list to become circular, with the effect that device_putchar
> > spins in a loop printing the first character it is given.  ("F".)
> >
> > Trivial patch and .config appended.  (One might perhaps also consider
> > making device_putchar or console_add_* more robust in the face of this
> > kind of error, but I settled for this.)
>
> Hmmm - okay - I think I sort of see whats going on here.  I'm not
> sure why they are calling console_init() (or video_console_init())
> and lib_get_sysinfo() in the first place - that should have already
> been run before callin main().   If you are right about
> the console_out list, and I think you are, then running console_init()
> twice should cause the same problem (or perhaps it doesn't end up 
> looping).  Anyway, thats bad.  We need to get Patrick and Stefan to
> explain what they did here.

My bad - they are using a custom entry point (see i386/switch.S in filo).
So, they do need to call console_init() and lib_get_sysinfo(). Your
patch is correct.

Acked-by: Jordan Crouse
> Jordan
> > Index: main/filo.c
> >
> > --- main/filo.c	(revision 83)
> > +++ main/filo.c	(working copy)
> > @@ -49,9 +49,6 @@
> >  {
> >      /* Set up the consoles. */
> >      console_init();
> > -#ifdef CONFIG_VIDEO_CONSOLE
> > video_console_init();
> >
> > -#endif
> >  /* Gather system information, and implicitly sets up timers */
> >  lib_get_sysinfo();
> > #
> > # Automatically generated make config: don't edit
> > # FILO version: 0.6.0rc1
> > # Thu Oct 30 12:01:16 2008
> > #
> > CONFIG_TARGET_I386=y
> > # CONFIG_REVIEW is not set
> >
> > #
> > # Interface Options
> > #
> > CONFIG_USE_GRUB=y
> > CONFIG_PROMPT="filo"
> > CONFIG_MENULST_FILE="hde1:/grub/menu.lst"
> > CONFIG_MENULST_TIMEOUT=3
> > CONFIG_USE_MD5_PASSWORDS=y
> >
> > #
> > # Drivers
> > #
> > CONFIG_IDE_DISK=y
> > CONFIG_IDE_DISK_POLL_DELAY=0
> > # CONFIG_SLOW_SATA is not set
> > # CONFIG_PCMCIA_CF is not set
> > CONFIG_USB_NEW_DISK=y
> > # CONFIG_USB_DISK is not set
> > # CONFIG_FLASH_DISK is not set
> > CONFIG_SUPPORT_PCI=y
> > # CONFIG_PCI_BRUTE_SCAN is not set
> > # CONFIG_SUPPORT_SOUND is not set
> >
> > #
> > # Filesystems
> > #
> > CONFIG_FSYS_EXT2FS=y
> > CONFIG_FSYS_FAT=y
> > # CONFIG_FSYS_JFS is not set
> > # CONFIG_FSYS_MINIX is not set
> > # CONFIG_FSYS_REISERFS is not set
> > # CONFIG_FSYS_XFS is not set
> > CONFIG_FSYS_ISO9660=y
> > CONFIG_ELTORITO=y
> > # CONFIG_FSYS_CRAMFS is not set
> > # CONFIG_FSYS_SQUASHFS is not set
> >
> > #
> > # Loaders
> > #
> > CONFIG_LINUX_LOADER=y
> > # CONFIG_WINCE_LOADER is not set
> > # CONFIG_ARTEC_BOOT is not set
> >
> > #
> > # Debugging & Experimental
> > #
> > # CONFIG_EXPERIMENTAL is not set
> > # CONFIG_DEBUG_ALL is not set
> > # CONFIG_DEBUG_ELFBOOT is not set
> > # CONFIG_DEBUG_ELFNOTE is not set
> > # CONFIG_DEBUG_SEGMENT is not set
> > # CONFIG_DEBUG_SYS_INFO is not set
> > # CONFIG_DEBUG_BLOCKDEV is not set
> > # CONFIG_DEBUG_VFS is not set
> > # CONFIG_DEBUG_FSYS_EXT2FS is not set
> > # CONFIG_DEBUG_PCI is not set
> > # CONFIG_DEBUG_LINUXLOAD is not set
> > # CONFIG_DEBUG_IDE is not set
> > # CONFIG_DEBUG_ELTORITO is not set
> > CONFIG_DEVELOPER_TOOLS=y
> >

[Arne Georg Gleditsch]

Jordan Crouse  writes:
> My bad - they are using a custom entry point (see i386/switch.S in filo).
> So, they do need to call console_init() and lib_get_sysinfo(). Your
> patch is correct.

Yes, I verified that console_out was NULL when entering filo.c's init().
> Acked-by: Jordan Crouse

Oh, in case it matters:

Signed-off-by: Arne Georg Gleditsch
> > > Index: main/filo.c
> > >
> > > --- main/filo.c	(revision 83)
> > > +++ main/filo.c	(working copy)
> > > @@ -49,9 +49,6 @@
> > >  {
> > >      /* Set up the consoles. */
> > >      console_init();
> > > -#ifdef CONFIG_VIDEO_CONSOLE
> > > video_console_init();
> > >
> > > -#endif
> > >  /* Gather system information, and implicitly sets up timers */
> > >  lib_get_sysinfo();

[Carl-Daniel Hailfinger]

On 30.10.2008 15:59, Arne Georg Gleditsch wrote:
> Jordan Crouse  writes:
> > My bad - they are using a custom entry point (see i386/switch.S in filo).
> > So, they do need to call console_init() and lib_get_sysinfo(). Your
> > patch is correct.
>
> Yes, I verified that console_out was NULL when entering filo.c's init().
> > Acked-by: Jordan Crouse
>
> Oh, in case it matters:
>
> Signed-off-by: Arne Georg Gleditsch

Committed by Stefan in r84.

Regards,
Carl-Daniel