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

From: Frank Rowand
Date: Fri Feb 15 2019 - 18:06:13 EST

On 2/15/19 1:49 AM, Brendan Higgins wrote:
> On Thu, Feb 14, 2019 at 6:48 PM Frank Rowand <frowand.list@xxxxxxxxx> wrote:
>> On 2/14/19 5:26 PM, Brendan Higgins wrote:
>>> On Thu, Feb 14, 2019 at 4:10 PM Frank Rowand <frowand.list@xxxxxxxxx> 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@xxxxxxxxxx>
>>>>> ---
>>>>> 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);
>>>>> + 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():
>> 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/ --timeout=30 --jobs=12 --defconfig`,
> which builds the kernel with a pretty similar config, boots the
> kernel, and then parses the output for you. ;-) )

Thanks, that was enough info to prod me into building and "booting"
a uml kernel. I have another framework that I use, so I did not
try, but reading that filled in any missing details that
I needed.

As I mentioned, I used my own framework, but the commands that it
emitted essentially boil down to a rather simple recipe:

export ARCH=um
make kunit_defconfig
make olddefconfig
make linux

# KBUILD_OUTPUT is my build directory
${KBUILD_OUTPUT}/linux mem=256m

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

Thanks for the updated patch.

> 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