[U-Boot] [PATCH v3 6/6] test: dm: Fix wrong aliases property names

[Eugeniu Rosca]

Hi Simon,

On Tue, May 22, 2018 at 05:30:40PM -0600, Simon Glass wrote:
> Hi Eugeniu,
> 
> On 19 May 2018 at 06:13, Eugeniu Rosca <roscaeugeniu at gmail.com> wrote:

--snip--

> > v2->v3:
> > * Fixed an issue in the test code (test/dm/test-fdt.c) generated by the
> >   DTS update (arch/sandbox/dts/test.dts) in [PATCH v2].
> > * Changed commit summary line, to cover test/dm/test-fdt.c.
> > * Added: Reported-by: Petr Vorel <pvorel at suse.cz>
> > * [Due to update] Dropped: Reviewed-by: Simon Glass <sjg at chromium.org>
> >
> > v1->v2:
> > * Newly pushed
> >
> >  arch/sandbox/dts/test.dts | 8 ++++----
> >  test/dm/test-fdt.c        | 2 +-
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> >
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
> See below
> 
> > diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> > index 683b1970e0af..3e87c5c0f3fd 100644
> > --- a/arch/sandbox/dts/test.dts
> > +++ b/arch/sandbox/dts/test.dts
> > @@ -27,10 +27,10 @@
> >                 testfdt3 = "/b-test";
> >                 testfdt5 = "/some-bus/c-test at 5";
> >                 testfdt8 = "/a-test";
> > -               fdt_dummy0 = "/translation-test at 8000/dev at 0,0";
> > -               fdt_dummy1 = "/translation-test at 8000/dev at 1,100";
> > -               fdt_dummy2 = "/translation-test at 8000/dev at 2,200";
> > -               fdt_dummy3 = "/translation-test at 8000/noxlatebus at 3,300/dev at 42";
> > +               fdt-dummy0 = "/translation-test at 8000/dev at 0,0";
> > +               fdt-dummy1 = "/translation-test at 8000/dev at 1,100";
> > +               fdt-dummy2 = "/translation-test at 8000/dev at 2,200";
> > +               fdt-dummy3 = "/translation-test at 8000/noxlatebus at 3,300/dev at 42";
> >                 usb0 = &usb_0;
> >                 usb1 = &usb_1;
> >                 usb2 = &usb_2;
> > diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
> > index 8196844e89a7..66d0df5629a2 100644
> > --- a/test/dm/test-fdt.c
> > +++ b/test/dm/test-fdt.c
> > @@ -425,7 +425,7 @@ static const struct udevice_id fdt_dummy_ids[] = {
> >  };
> >
> >  UCLASS_DRIVER(fdt_dummy) = {
> > -       .name           = "fdt_dummy",
> > +       .name           = "fdt-dummy",
> 
> You should not need to change this one, and I worry that it is
> confusing since this is the driver name, not a compatible string.

I am not familiar with the in-tree U-boot unit tests, but doing a small
experiment I get clear evidence that there is a tight
relationship/dependency between the name of the entries in the aliases
DTS node and the "name" field of UCLASS_DRIVER using that DTS.

The experiment is running 'ut dm' in sandbox before and after below
patch (I also had to fix two null pointer dereferences in
test/dm/bus.c to avoid segmentation faults with the patch applied):

diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
index 8196844e89a7..5b715e965269 100644
--- a/test/dm/test-fdt.c
+++ b/test/dm/test-fdt.c
@@ -94,7 +94,7 @@ int testfdt_ping(struct udevice *dev, int pingval, int *pingret)
 }
 
 UCLASS_DRIVER(testfdt) = {
-	.name		= "testfdt",
+	.name		= "test_fdt",
 	.id		= UCLASS_TEST_FDT,
 	.flags		= DM_UC_FLAG_SEQ_ALIAS,
 };

Before the patch, I get: Failures: 9 
After the patch, I get: Failures: 25

So, while the aliases entries are certainly not compatible strings,
not keeping them aligned to UCLASS_DRIVER->name values leads to test
failures. I am not sure if this is something specific to architecture of
the tests or applies generically to any driver which fetches its
configuration from DTS. I was hoping to get an assessment from somebody
with more experience in this area.

Besides the above, it is not clear to me if your Reviewed-by applies to
to this patch partially (since you expressed some concerns) or applies
globally, in which case the concerns are not major?

> 
> >         .id             = UCLASS_TEST_DUMMY,
> >         .flags          = DM_UC_FLAG_SEQ_ALIAS,
> >  };
> > --
> > 2.17.0
> >
> 
> Regards,
> Simon

Best regards,
Eugeniu.