From: Simon Glass sjg@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@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Mon, 7 Feb 2022 13:22:22 -0700
Hi Mark,
On Sat, 5 Feb 2022 at 16:10, Mark Kettenis kettenis@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@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.
Cheers,
Mark