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

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

[Mark Kettenis]

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(-)

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

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

[Simon Glass]

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?

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

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

[Mark Kettenis]

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

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
> 

Re: [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@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.

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

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

[Mark Kettenis]

> 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

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

[Simon Glass]

Hi Mark,

On Sat, 19 Feb 2022 at 13:37, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > 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.

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.

Regards,
Simon

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

[Mark Kettenis]

> From: Simon Glass <sjg@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@xs4all.nl> wrote:
> >
> > > 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.
> 
> 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.

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

[Simon Glass]

Hi Mark,

On Sun, 20 Feb 2022 at 08:13, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > From: Simon Glass <sjg@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@xs4all.nl> wrote:
> > >
> > > > 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.
> >
> > 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.

Regards,
Simon

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

[Mark Kettenis]

> From: Simon Glass <sjg@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@xs4all.nl> wrote:
> >
> > > From: Simon Glass <sjg@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@xs4all.nl> wrote:
> > > >
> > > > > 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.
> > >
> > > 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