Re: [PATCH v3] dm: core: Do not stop uclass iteration on error

[Simon Glass <sjg@chromium.org>]

Hi Michal,

On Sat, 24 Sept 2022 at 14:10, Michal Suchánek <msuchanek@suse.de> wrote:
>
> Hello,
>
> On Sat, Sep 17, 2022 at 07:04:25PM +0200, Michal Suchánek wrote:
> > Hello,
> >
> > On Sat, Sep 17, 2022 at 09:02:53AM -0600, Simon Glass wrote:
> > > Hi Michal,
> > >
> > > On Wed, 31 Aug 2022 at 11:44, Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Michal,
> > > >
> > > > On Wed, 31 Aug 2022 at 01:39, Michal Suchánek <msuchanek@suse.de> wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > On Tue, Aug 30, 2022 at 09:15:12PM -0600, Simon Glass wrote:
> > > > > > Hi Michal,
> > > > > >
> > > > > > On Tue, 30 Aug 2022 at 10:48, Michal Suchánek <msuchanek@suse.de> wrote:
> > > > > > >
> > > > > > > On Tue, Aug 30, 2022 at 09:56:52AM -0600, Simon Glass wrote:
> > > > > > > > Hi Michal,
> > > > > > > >
> > > > > > > > On Tue, 30 Aug 2022 at 04:23, Michal Suchánek <msuchanek@suse.de> wrote:
> > > > > > > > >
> > > > > > > > > On Sat, Aug 27, 2022 at 07:52:27PM -0600, Simon Glass wrote:
> > > > > > > > > > Hi Michal,
> > > > > > > > > >
> > > > > > > > > > On Fri, 19 Aug 2022 at 14:23, Michal Suchanek <msuchanek@suse.de> wrote:
> > > > > > > > > > >
> > > > > > > > > > > When probing a device fails NULL pointer is returned, and other devices
> > > > > > > > > > > cannot be iterated. Skip to next device on error instead.
> > > > > > > > > > >
> > > > > > > > > > > Fixes: 6494d708bf ("dm: Add base driver model support")
> > > > > > > > > >
> > > > > > > > > > I think you should drop this as you are doing a change of behaviour,
> > > > > > > > > > not fixing a bug!
> > > > > > > > >
> > > > > > > > > You can hardly fix a bug without a change in behavior.
> > > > > > > > >
> > > > > > > > > These functions are used for iterating devices, and are not iterating
> > > > > > > > > devices. That's clearly a bug.
> > > > > > > >
> > > > > > > > If it were clear I would have changed this long ago. The new way you
> > > > > > > > have this function ignores errors, so they cannot be reported.
> > > > > > > >
> > > > > > > > We should almost always report errors, which is why I think your
> > > > > > > > methods should be named differently.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > > > > > > > > > > ---
> > > > > > > > > > > v2: - Fix up tests
> > > > > > > > > > > v3: - Fix up API doc
> > > > > > > > > > >     - Correctly forward error from uclass_get
> > > > > > > > > > >     - Do not return an error when last device fails to probe
> > > > > > > > > > >     - Drop redundant initialization
> > > > > > > > > > >     - Wrap at 80 columns
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/core/uclass.c | 32 ++++++++++++++++++++++++--------
> > > > > > > > > > >  include/dm/uclass.h   | 13 ++++++++-----
> > > > > > > > > > >  test/dm/test-fdt.c    | 20 ++++++++++++++++----
> > > > > > > > > > >  3 files changed, 48 insertions(+), 17 deletions(-)
> > > > > > > > > >
> > > > > > > > > > Unfortunately this still fails one test. Try 'make qcheck' to see it -
> > > > > > > > > > it is ethernet.
> > > > > > > > >
> > > > > > > > > I will look at that.
>
> How do you debug test failures?
>
> There is indeed a test that regresses.
>
> However, when I run
>
> ping 1.1.1.1
>
> I get to see the printfs that I added to net_loop
>
> when I run
>
> ut dm dm_test_eth_act
>
> u-boot crashes, and no printf output is seen, not even the print that
> should report entering net_loop.
> Given that the assert that is reported as failing is
> test/dm/eth.c:133, dm_test_eth_act(): -ENODEV == net_loop(PING): Expected 0xffffffed (-19), got 0x0 (0)
> it should run the net_loop to get that error.
>
> It's nice that we have tests but if they cannot be debugged they are not
> all that useful.

Why don't you try gdb?

This is part of the setup script I use:

function rt_get_suite_and_name() {
        local arg="$1"
        #echo arg $arg
        suite=
        name=

        # The symbol is something like this:
        #       _u_boot_list_2_ut_bootstd_test_2_vbe_simple_test_base
        # Split it into the suite name (bootstd) and test name
        # (vbe_simple_test_base)
        read suite name < \
                <(nm /tmp/b/$exec/u-boot |grep "list_2_ut.*$arg.*" \
                        | cut -d' ' -f3 \
                        | head -1 \
                        | sed -n 's/_u_boot_list_2_ut_\(.*\)_test_2_/\1 /p')
        #echo suite $suite
        #echo name $name
        #name=${1#dm_test_}
        #name=${name#ut_dm_}
}

# Run a test
function rt() {
        local exec=sandbox
        local suite name
        rt_get_suite_and_name $1 $exec

        /tmp/b/$exec/u-boot -T $2 -c "ut $suite $name"
}

# Run a test verbosely
function rtv() {
        local exec=sandbox
        local suite name
        rt_get_suite_and_name $1 $exec

        /tmp/b/$exec/u-boot -v -T $2 -c "ut $suite $name"
}

# Run a test in the debugger
function rtd() {
        local exec=sandbox
        local suite name
        rt_get_suite_and_name $1 $exec

        gdb-multiarch --args /tmp/b/$exec/u-boot -T $2 -c "ut $suite $name"
}

# Run a test in the debugger verbosely
function rtdv() {
        local exec=sandbox
        local suite name
        rt_get_suite_and_name $1 $exec

        gdb-multiarch --args /tmp/b/$exec/u-boot -T $2 -v -c "ut $suite $name"
}


So you can use 'rtdv dm_test_eth_act' for example.

Regards,
Simon