Re: [PATCH v6 01/19] test: Allow signaling that U-Boot is ready

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 6806 bytes --]

On Thu, Oct 31, 2024 at 07:03:19PM +0100, Simon Glass wrote:
> Hi Tom,
> 
> On Fri, 27 Sept 2024 at 04:52, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Thu, Sep 26, 2024 at 11:36:00PM +0200, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 25 Sept 2024 at 19:26, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Wed, Sep 25, 2024 at 02:49:56PM +0200, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Mon, 23 Sept 2024 at 22:35, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Fri, Sep 20, 2024 at 08:01:36AM +0200, Simon Glass wrote:
> > > > > >
> > > > > >
> > > > > > > When Labgrid is used, it can get U-Boot ready for running tests. It
> > > > > > > prints a message when it has done so.
> > > > > > >
> > > > > > > Add logic to detect this message and accept it.
> > > > > > >
> > > > > > > Signed-off-by: Simon Glass <sjg@chromium.org>
> > > > > > > ---
> > > > > > >
> > > > > > > (no changes since v1)
> > > > > > >
> > > > > > >  test/py/u_boot_console_base.py | 9 +++++----
> > > > > > >  1 file changed, 5 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > What happens is that labgrid can also be told to look for and then
> > > > > > interrupt autoboot, just like our pytests can do, and the system is at
> > > > > > the prompt. But this is also what it's like for a system with autoboot
> > > > > > disabled. Do we actually need this patch to achieve the functionality
> > > > > > you want? Doesn't that already just happen?
> > > > >
> > > > > The point of this patch is actually to remove code in pytest, by
> > > > > allowing it to skip all the banner-detection stuff. It does not affect
> > > > > things in Labgrid, since it still needs to watch for banners, etc.
> > > >
> > > > But you can't remove code from pytest, people can and will run the suite
> > > > outside of labgrid.
> > >
> > > Yes, that's right. I meant that with Labgrid the code is not used.
> >
> > OK. I still think this (and I assume the related flag to tell whichever
> > helper that was that the platform is ready to go) are too implementation
> > specific.
> 
> OK, but it does have the virtue of working properly on all the boards
> I have, rather than just a subset, without this patch.
> 
> >
> > > > > Without this patch, we have to tell Labgrid's U-Boot driver to do
> > > > > nothing, so that pytest does it. But that is not a good idea, since
> > > > > Labgrid has a lot more info about the board than pytest has. For
> > > > > example, look at all the SPL-banner-count stuff.
> > > >
> > > > Why do you have to tell it to do nothing? The pytest suite works fine,
> > > > today, if the board stops at the prompt automatically. To be clear, the
> > > > labgrid yaml file I'm using with my scripts has the information to stop
> > > > autoboot in it and it's not causing a problem.
> > >
> > > But if it wasn't causing a problem I would not have invented this
> > > annoying scheme.
> >
> > Well, I honestly thought it might be a remnant from development that
> > wasn't needed once everything else was done. I do that from time to time
> > myself.
> 
> OK.
> 
> >
> > > For several boards, by the time pytest gets to see
> > > the output it is too late to press a key and stop.
> >
> > Why / how? Do we perhaps need to adjust the timeout upwards, slightly?
> > Especially in light of the changes you also did to detect a "dead" board
> > and so we don't wait 30 minutes for a test run to fail.
> 
> Why / how - because the board prints its SPL banner as soon as reset
> is released, then waits for U-Boot proper to be sent, at which case
> the U-Boot banner is sent .Then labgrid disconnects from the console
> and runs microcom, which connects to the console over ser2net and
> misses some of what has already been sent.
> 
> This is actually a fundamental problem, not something we can adjust
> with timeouts.

Wait, what? *Labgrid* is the thing that's the problem here with dropping
the connection to the board and re-establishing it at some point?

That is the kind of detail which (a) needs to be in the commit message
(b) perhaps an issue filed (or if already exists Link:'d in the commit
message) and (c) would have made it clear why we need this work-around
and then I'd have been a lot happier to accept it as a work-around back
on v1 or so.

> > > Also, some boards
> > > have a different prompt which is not detected by pytest.
> > >
> > > For example:
> > > configs/am62x_beagleplay_a53_defconfig:CONFIG_AUTOBOOT_PROMPT="Press
> > > SPACE to abort autoboot in %d seconds\n"
> >
> > Yeah, I had forgotten that as I turn that off along with turning on
> > other tests via config fragment, on that platform. That really is a
> > deficiency in our pytest version of this.
> 
> OK
> 
> >
> > > > > Basically, without this patch we cannot use '-s uboot' to tell the
> > > > > Labgrid strategy to take us to a U-Boot prompt. We must just use a raw
> > > > > console with no strategy, relying on pytest to do all the work.
> > > > >
> > > > > I hope that helps explain the problem?
> > > >
> > > > I think I see what you're saying, and it's based on the assumption that
> > > > we'll make everyone either use labgrid?
> > >
> > > Not at all. I tested this version of the series again with my
> > > pre-Labgrid lab and it works fine. But I do believe that the hooks
> > > that pytest has are not ideal for use with Labgrid.
> >
> > OK. But, why can't you make use of the labgrid functionality to pause
> > the board? It's the state flag for labgrid-client yes? Dropping that in
> > with my labgrid support would be just a tweak to the console script to
> > check if the board set labgrid_strategy or something, and I think you
> > could adapt yours to that too?
> 
> I am really not sure what to say here. I have explained the reality of
> the situation. I have spent hours debugging it and figuring out the
> root cause. There is no other tweak I can think of that will solve
> this problem.

Well, I hate to get on a tangent here, but I think a common thread is
that after you've spent hours debugging a problem and working out a
solution you then omit much of the details. I'd much rather see a much
longer commit message here describing the problem, or a longer
description under the "---" where you explain that labgrid uses minicom,
gets things up, drops the connection and uses ser2net and so there's a
chance we miss messages and so need something like this. Or, that you've
been debugging on *x86_64* a bunch of EFI_LOADER things and off the
shelf distributions aren't working (and ExitBootServices() is where you
had a problem with Ubuntu I gather still on x86_64).

> 
> Regards,
> Simon

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]