Re: [PATCH v1 1/1] of: unittest: unflatten device tree on UML when testing

[Brendan Higgins <brendanhiggins@google.com>]

On Thu, Feb 14, 2019 at 6:48 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 2/14/19 5:26 PM, Brendan Higgins wrote:
> > On Thu, Feb 14, 2019 at 4:10 PM Frank Rowand <frowand.list@gmail.com> wrote:
> >>
> >> On 2/12/19 10:53 AM, Brendan Higgins wrote:
> >>> UML supports enabling OF, and is useful for running the device tree
> >>> tests, so add support for unflattening device tree blobs so we can
> >>> actually use it.
> >>>
> >>> Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> >>> ---
> >>>  drivers/of/unittest.c | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> >>> index 84427384654d5..effa4e2b9d992 100644
> >>> --- a/drivers/of/unittest.c
> >>> +++ b/drivers/of/unittest.c
> >>> @@ -2527,6 +2527,9 @@ static int __init of_unittest(void)
> >>>       }
> >>>       of_node_put(np);
> >>>
> >>> +     if (IS_ENABLED(CONFIG_UML))
> >>> +             unflatten_device_tree();
> >>> +
> >>>       pr_info("start of unittest - you will see error messages\n");
> >>>       of_unittest_check_tree_linkage();
> >>>       of_unittest_check_phandles();
> >>>
> >>
> >> (Insert my usual disclaimer that I am clueless about UML, I still need to learn
> >> about it...)
> >>
> >> This does not look correct to me.
> >>
> >> A few lines earlier in of_unittest(), the live devicetree needs to exist for
> >> unittest_data_data() and a few of_*() functions to succeed.  So it seems
> >> that the unflatten_device_tree() for uml should be at the beginning of
> >> of_unittest().
> >
> > It is true that other functions ahead of it depend on the presence of
> > a device tree, but an unflattened tree does get linked in somewhere
> > else. Despite that, this is needed for overlay_base_root. I got
> > similar behavior if you don't link in a flattened device tree on x86.
> > Thus, the order in which you call them doesn't actually seem to
> > matter. I found no difference from changing the order in UML myself.
> >
> > Without my patch we get the following error,
> > ### dt-test ### FAIL of_unittest_overlay_high_level():2372
> > overlay_base_root not initialized
> > ### dt-test ### end of unittest - 219 passed, 1 failed
> >
> > With my patch we get:
> > ### dt-test ### end of unittest - 224 passed, 0 failed
>
> Thanks for reporting both the failure and the success cases,
> that helps me understand a little bit better.
>
> If instead of the above patch, if you add the following (untested,
> not even compile tested) to the beginning of of_unittest():
>
>         if (IS_ENABLED(CONFIG_UML))
>                 unittest_unflatten_overlay_base();
>
> does that also result in a good test result of:
>
>         ### dt-test ### end of unittest - 224 passed, 0 failed

Yep, I just tried it. It works as you suspected.

>
> I think I need to find some time to build and boot a UML kernel soon.

It's really no big deal, just copy the .config I sent and build with
`make ARCH=um` then you "boot" the kernel with `./linux` (note this
will mess up your terminal settings); that's it! (Shameless plug: you
can also try it out with the KUnit patchset with
`./tools/testing/kunit/kunit.py --timeout=30 --jobs=12 --defconfig`,
which builds the kernel with a pretty similar config, boots the
kernel, and then parses the output for you. ;-) )

>
> My current _guess_ is that the original problem was not a failure to
> unflatten any present devicetree in UML but instead that the UML
> kernel does not call unflatten_device_tree() and thus fails to
> indirectly call unittest_unflatten_overlay_base(), which is
> called by unflatten_device_tree().

I think you are right. Sorry for not noticing this before making my
change. Since it was pretty much the only architecture (the only one I
care about) that does not unflatten DT, I assumed that was the
problem. I didn't put too much thought into it after that point beyond
making sure that it did what I wanted.

>
> unittest_unflatten_overlay_base() is an unfortunate wart that I
> added, but I don't have a better alternative yet.

Hey, I get it. No worries.

In any case, it seems like unittest_unflatten_overlay_base() is the
right function to call there. I will send out patch. Do you want me to
send a patch on top of this one, or do you want to revert this one and
for me to send a v2 follow up to this patch? I don't care either way,
whatever you guys prefer.

Cheers