[PATCH] drivers: serial: Make sure we really return a serial device

[Simon Glass]

Hi Mark,

On Mon, 7 Feb 2022 at 14:20, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
>
> > From: Simon Glass <sjg at chromium.org>
> > Date: Mon, 7 Feb 2022 13:22:22 -0700
> >
> > Hi Mark,
> >
> > On Sat, 5 Feb 2022 at 16:10, Mark Kettenis <kettenis at openbsd.org> wrote:
> > >
> > > The stdout-path property in the device tree does not necessarily
> > > point at a serial device. The code that binds the device if it
> > > isn't marked to be bound before relocation does not check whether
> > > the device really is a serial device. So check that its uclass is
> > > UCLASS_SERIAL before probing it.
> > >
> > > Signed-off-by: Mark Kettenis <kettenis at openbsd.org>
> > > ---
> > >  drivers/serial/serial-uclass.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > This would be better as an assert() to avoid code bloat. Under what
> > circumstances was this wrong?
>
> I'm passing a debive tree to U-Boot that has stdout-path set to
> "/chosen/framebuffer" to indicate that output should go to the
> framebuffer instead of the serial port.  This is a node with a
> "simple-framebuffer" compatible, which means the list_bind_fdt() call
> binds the simple_video (simplefb.c) driver and stores a pointer to its
> struct udevice into devp.  I've verified that the uclass is indeed
> UCLASS_VIDEO at that point with my device tree.
>
> Since the function returns 0, the caller of this function thinks we
> returned a valid UCLASS_SERIAL device and when u-boot starts using it
> as such it crashes (without printing anything since there is no
> console output device yet).
>
> So an assert won't work.  We should simply return an error an run the
> fallback code below that looks for the serial port with the specified
> index.  That way U-Boot continues to work as expected (with output on
> both the serial port and the framebuffer).  But once my OS is running,
> it knows to pick the framebuffer console and all's fine.
>
> I haven't looked at the binary growth, but it can't be much.  But if
> you think it is a problem, maybe we should make this code that binds
> the console driver optional?

OK I see. It likely isn't much, but in SPL it is still growth. Worth checking.

Can you perhaps create a 'console' alias so that the line above finds
the correct one?

But I'm a bit unsure of how you get the serial console to work if
serial_check_stdout() fails? Is the serial driver not marked for
pre-reloc use? There is something odd going on.

Regards,
Simon