[PATCH] drivers: serial: Make sure we really return a serial device
mark.kettenis at xs4all.nl
Sat Feb 19 21:37:10 CET 2022
> From: Simon Glass <sjg at chromium.org>
> Date: Fri, 18 Feb 2022 17:21:13 -0700
> 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
$ size u-boot
text data bss dec he
442903 27700 13448 484051 762d3
$ 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.
More information about the U-Boot