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

[Mark Kettenis]

> From: Simon Glass <sjg at chromium.org>
> Date: Sun, 20 Feb 2022 21:40:09 -0700

Hi Simon,

> Hi Mark,
> 
> On Sun, 20 Feb 2022 at 08:13, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
> >
> > > From: Simon Glass <sjg at chromium.org>
> > > Date: Sat, 19 Feb 2022 15:21:19 -0700
> > >
> > > Hi Mark,
> > >
> > > On Sat, 19 Feb 2022 at 13:37, Mark Kettenis <mark.kettenis at xs4all.nl> wrote:
> > > >
> > > > > From: Simon Glass <sjg at chromium.org>
> > > > > Date: Fri, 18 Feb 2022 17:21:13 -0700
> > > >
> > > > Hi Simon,
> > > >
> > > > > 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.
> > > >
> > > > before:
> > > >
> > > > $ size u-boot
> > > > text    data    bss     dec     he
> > > > 442903  27700   13448   484051  762d3
> > > >
> > > > after:
> > > >
> > > > $ size u-boot
> > > > text    data    bss     dec     hex
> > > > 442925  27700   13448   484073  762e9
> > > >
> > > > So that's 22 bytes.
> > > >
> > > > Thew growth in serial-uclass.o is a bit smaller; only 16 byes.
> > > >
> > > > > Can you perhaps create a 'console' alias so that the line above finds
> > > > > the correct one?
> > > >
> > > > Probably.  But none of the Linux device trees provide a 'console'
> > > > alias so this seems to be a U-Boot convention.  And it doesn't
> > > > actually fix the bug; it just works around it.
> > > >
> > > > > 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.
> > > >
> > > > The serial driver is marked for pre-reloc use.  When
> > > > serial_check_stdout() fails the code at the tail end of
> > > > serial_find_console_or_panic() runs and finds the serial device at
> > > > index 0 and uses that.
> > > >
> > > > The "something odd" that's going on here is that I set
> > > > "/chosen/stdout-path" to point to the framebuffer instead of a serial
> > > > port.  But nowhere does the device tree specification say that
> > > > "stdout-path" has to point at a serial device.  So U-Boot shouldn't
> > > > crash when this isn't the case.
> > >
> > > OK, I think this is the sort of info that should have been in your
> > > commit message.
> > >
> > > However, looking at this, the code you are changing should not be
> > > there now. It was added for backwards-compatibility reasons.
> > >
> > > Let's remove the lists_bind_fdt() call and rely on people doing the
> > > right thing now.
> >
> > Right.  I'm worrying a bit that this will break many boards.  I was
> > thinking in particular about boards like the raspberry pi where we use
> > the device tree provided by the firmware.  But on that the serial
> > drivers have the DM_FLAG_PRE_RELOC flag set when OF_BOARD is defined,
> > so that shouldn't be a problem.
> >
> > But many other serial drivers don't set DM_FLAG_PRE_RELOC, or only set
> > it when !OF_CONTROL.  I checked a few of those and most of them don't
> > seem to have the *-u-boot.dtsi files to add the necessary property to
> > the nodes.
> >
> > I still think we should fix the bug regardless of whether we remove
> > the code somewhere in the near future.
> 
> Well yes if you want this as a bug fix for the upcoming release, I
> agree, in which case we don't need to worry about the code size since
> it is temporary.
> 
> Would you mind sending a second patch to remove it? I will pick up the
> bug-fix now and add the removal to -next when it comes.
> 
> Also please expand the commit message to cover some of the material
> here, i.e. why we end up in this case for your board and why you don't
> want to add the 'console' property.

Actually, adding a 'console' alias doesn't help, since that is only
used if there is no stdout-path property.  But in my case there is.

I extended the commit message and send a v2.  I also sent a separate
diff that removes the whole block of code again.

Cheers,

Mark