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

[Mark Kettenis]

> 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?

Thanks,

Mark


> > diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
> > index 96a1cb65ba..931a90bdbd 100644
> > --- a/drivers/serial/serial-uclass.c
> > +++ b/drivers/serial/serial-uclass.c
> > @@ -66,7 +66,8 @@ static int serial_check_stdout(const void *blob, struct udevice **devp)
> >          */
> >         if (node > 0 && !lists_bind_fdt(gd->dm_root, offset_to_ofnode(node),
> >                                         devp, NULL, false)) {
> > -               if (!device_probe(*devp))
> > +               if (device_get_uclass_id(*devp) == UCLASS_SERIAL &&
> > +                   !device_probe(*devp))
> >                         return 0;
> >         }
> >
> > --
> > 2.34.1
> >
> 
> Regards,
> Simon
>