diff mbox series

[V5,5/5] of: unittest: Statically apply overlays using fdtoverlay

Message ID 696c137461be8ec4395c733c559c269bb4ad586e.1611124778.git.viresh.kumar@linaro.org
State New
Headers show
Series dt: build overlays | expand

Commit Message

Viresh Kumar Jan. 20, 2021, 7:06 a.m. UTC 
Now that fdtoverlay is part of the kernel build, start using it to test
the unitest overlays we have by applying them statically.

Some unittest overlays deliberately contain errors that unittest checks
for. These overlays will cause fdtoverlay to fail, and are thus not
included in the static_test.dtb.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
 drivers/of/unittest-data/Makefile | 50 +++++++++++++++++++++++++++++++
 1 file changed, 50 insertions(+)

-- 
2.25.0.rc1.19.g042ed3e048af

Comments

David Gibson Jan. 21, 2021, 12:51 a.m. UTC | #1 
On Wed, Jan 20, 2021 at 12:36:47PM +0530, Viresh Kumar wrote:
> Now that fdtoverlay is part of the kernel build, start using it to test

> the unitest overlays we have by applying them statically.

> 

> Some unittest overlays deliberately contain errors that unittest checks

> for. These overlays will cause fdtoverlay to fail, and are thus not

> included in the static_test.dtb.

> 

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> ---

>  drivers/of/unittest-data/Makefile | 50 +++++++++++++++++++++++++++++++

>  1 file changed, 50 insertions(+)

> 

> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile

> index 009f4045c8e4..ece7dfd5cafa 100644

> --- a/drivers/of/unittest-data/Makefile

> +++ b/drivers/of/unittest-data/Makefile

> @@ -38,3 +38,53 @@ DTC_FLAGS_testcases += -@

>  

>  # suppress warnings about intentional errors

>  DTC_FLAGS_testcases += -Wno-interrupts_property

> +

> +# Apply overlays statically with fdtoverlay.  This is a build time test that

> +# the overlays can be applied successfully by fdtoverlay.  This does not

> +# guarantee that the overlays can be applied successfully at run time by

> +# unittest, but it provides a bit of build time test coverage for those

> +# who do not execute unittest.

> +#

> +# The overlays are applied on top of testcases.dtb to create static_test.dtb

> +# If fdtoverlay detects an error than the kernel build will fail.

> +# static_test.dtb is not consumed by unittest.

> +#

> +# Some unittest overlays deliberately contain errors that unittest checks for.

> +# These overlays will cause fdtoverlay to fail, and are thus not included

> +# in the static test:

> +#			overlay.dtb \

> +#			overlay_bad_add_dup_node.dtb \

> +#			overlay_bad_add_dup_prop.dtb \

> +#			overlay_bad_phandle.dtb \

> +#			overlay_bad_symbol.dtb \

> +#			overlay_base.dtb \

> +

> +apply_static_overlay := overlay_0.dtb \

> +			overlay_1.dtb \

> +			overlay_2.dtb \

> +			overlay_3.dtb \

> +			overlay_4.dtb \

> +			overlay_5.dtb \

> +			overlay_6.dtb \

> +			overlay_7.dtb \

> +			overlay_8.dtb \

> +			overlay_9.dtb \

> +			overlay_10.dtb \

> +			overlay_11.dtb \

> +			overlay_12.dtb \

> +			overlay_13.dtb \

> +			overlay_15.dtb \

> +			overlay_gpio_01.dtb \

> +			overlay_gpio_02a.dtb \

> +			overlay_gpio_02b.dtb \

> +			overlay_gpio_03.dtb \

> +			overlay_gpio_04a.dtb \

> +			overlay_gpio_04b.dtb

> +

> +quiet_cmd_fdtoverlay = FDTOVERLAY $@

> +      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^

> +

> +$(obj)/static_test.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(apply_static_overlay))

> +	$(call if_changed,fdtoverlay)

> +

> +always-$(CONFIG_OF_OVERLAY) += static_test.dtb


The fact that testcases.dts includes /plugin/ still seems completely
wrong, if it's being used as the base tree.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Frank Rowand Jan. 21, 2021, 5:14 a.m. UTC | #2 
Hi David,

On 1/20/21 6:51 PM, David Gibson wrote:
> On Wed, Jan 20, 2021 at 12:36:47PM +0530, Viresh Kumar wrote:

>> Now that fdtoverlay is part of the kernel build, start using it to test

>> the unitest overlays we have by applying them statically.

>>

>> Some unittest overlays deliberately contain errors that unittest checks

>> for. These overlays will cause fdtoverlay to fail, and are thus not

>> included in the static_test.dtb.

>>

>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

>> ---

>>  drivers/of/unittest-data/Makefile | 50 +++++++++++++++++++++++++++++++

>>  1 file changed, 50 insertions(+)

>>

>> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile

>> index 009f4045c8e4..ece7dfd5cafa 100644

>> --- a/drivers/of/unittest-data/Makefile

>> +++ b/drivers/of/unittest-data/Makefile

>> @@ -38,3 +38,53 @@ DTC_FLAGS_testcases += -@

>>  

>>  # suppress warnings about intentional errors

>>  DTC_FLAGS_testcases += -Wno-interrupts_property

>> +

>> +# Apply overlays statically with fdtoverlay.  This is a build time test that

>> +# the overlays can be applied successfully by fdtoverlay.  This does not

>> +# guarantee that the overlays can be applied successfully at run time by

>> +# unittest, but it provides a bit of build time test coverage for those

>> +# who do not execute unittest.

>> +#

>> +# The overlays are applied on top of testcases.dtb to create static_test.dtb

>> +# If fdtoverlay detects an error than the kernel build will fail.

>> +# static_test.dtb is not consumed by unittest.

>> +#

>> +# Some unittest overlays deliberately contain errors that unittest checks for.

>> +# These overlays will cause fdtoverlay to fail, and are thus not included

>> +# in the static test:

>> +#			overlay.dtb \

>> +#			overlay_bad_add_dup_node.dtb \

>> +#			overlay_bad_add_dup_prop.dtb \

>> +#			overlay_bad_phandle.dtb \

>> +#			overlay_bad_symbol.dtb \

>> +#			overlay_base.dtb \

>> +

>> +apply_static_overlay := overlay_0.dtb \

>> +			overlay_1.dtb \

>> +			overlay_2.dtb \

>> +			overlay_3.dtb \

>> +			overlay_4.dtb \

>> +			overlay_5.dtb \

>> +			overlay_6.dtb \

>> +			overlay_7.dtb \

>> +			overlay_8.dtb \

>> +			overlay_9.dtb \

>> +			overlay_10.dtb \

>> +			overlay_11.dtb \

>> +			overlay_12.dtb \

>> +			overlay_13.dtb \

>> +			overlay_15.dtb \

>> +			overlay_gpio_01.dtb \

>> +			overlay_gpio_02a.dtb \

>> +			overlay_gpio_02b.dtb \

>> +			overlay_gpio_03.dtb \

>> +			overlay_gpio_04a.dtb \

>> +			overlay_gpio_04b.dtb

>> +

>> +quiet_cmd_fdtoverlay = FDTOVERLAY $@

>> +      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^

>> +

>> +$(obj)/static_test.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(apply_static_overlay))

>> +	$(call if_changed,fdtoverlay)

>> +

>> +always-$(CONFIG_OF_OVERLAY) += static_test.dtb

> 

> The fact that testcases.dts includes /plugin/ still seems completely

> wrong, if it's being used as the base tree.

> 


Yes, the build rule for static_test.dtb is using testcases.dtb as the base FDT.
It is a convenient FDT to use because it provides the frame that the overlays
require to be applied.  It is fortunate that fdtoverlay does not reject the use
of an FDT with overlay metadata as the base blob.

If Viresh wants to test a more realistic data set then he could create a build
rule that copies testcases.dts into (for example) testcases_base.dts, strip
out the '/plugin/;" then compile that into testcases_base.dtb and use that for
fdtoverlay.

   pseudo makefile rule for testcases_base.dts:
       sed -e 's|/plugin/;||' > testcases_base.dts

   add testcases_base.dtb to the list of objects to build

   change the rule for static_test.dtb to use testcases_base.dtb instead of
   testcases.dtb

This is probably a good idea instead of depending on the leniency of fdtoverlay.

-Frank
Viresh Kumar Jan. 21, 2021, 5:34 a.m. UTC | #3 
On 20-01-21, 23:14, Frank Rowand wrote:
> It is a convenient FDT to use because it provides the frame that the overlays

> require to be applied.  It is fortunate that fdtoverlay does not reject the use

> of an FDT with overlay metadata as the base blob.


> This is probably a good idea instead of depending on the leniency of fdtoverlay.


I believe fdtoverlay allows that intentionally, that would be required
for the cases where we have a hierarchy of extension boards or
overlays.

A platform can have a base dtb (with /plugin/;), then we can have an
overlay (1) for an extension board (with /plugin/;) and then an
overlay (2) for an extension board for the previous extension board.

In such a case overlay-(2) can't be applied directly to the base dtb
as it may not find all the nodes it is trying to update. And so
overlay-(2) needs to be applied to overlay-(1) and then the output of
this can be applied to the base dtb.

This is very similar to what I tried with the intermediate.dtb
earlier.

-- 
viresh
Frank Rowand Jan. 21, 2021, 5:34 a.m. UTC | #4 
Hi Viresh,

On 1/20/21 11:14 PM, Frank Rowand wrote:
> Hi David,

> 

> On 1/20/21 6:51 PM, David Gibson wrote:

>> On Wed, Jan 20, 2021 at 12:36:47PM +0530, Viresh Kumar wrote:

>>> Now that fdtoverlay is part of the kernel build, start using it to test

>>> the unitest overlays we have by applying them statically.

>>>

>>> Some unittest overlays deliberately contain errors that unittest checks

>>> for. These overlays will cause fdtoverlay to fail, and are thus not

>>> included in the static_test.dtb.

>>>

>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

>>> ---

>>>  drivers/of/unittest-data/Makefile | 50 +++++++++++++++++++++++++++++++

>>>  1 file changed, 50 insertions(+)

>>>

>>> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile

>>> index 009f4045c8e4..ece7dfd5cafa 100644

>>> --- a/drivers/of/unittest-data/Makefile

>>> +++ b/drivers/of/unittest-data/Makefile

>>> @@ -38,3 +38,53 @@ DTC_FLAGS_testcases += -@

>>>  

>>>  # suppress warnings about intentional errors

>>>  DTC_FLAGS_testcases += -Wno-interrupts_property

>>> +

>>> +# Apply overlays statically with fdtoverlay.  This is a build time test that

>>> +# the overlays can be applied successfully by fdtoverlay.  This does not

>>> +# guarantee that the overlays can be applied successfully at run time by

>>> +# unittest, but it provides a bit of build time test coverage for those

>>> +# who do not execute unittest.

>>> +#

>>> +# The overlays are applied on top of testcases.dtb to create static_test.dtb

>>> +# If fdtoverlay detects an error than the kernel build will fail.

>>> +# static_test.dtb is not consumed by unittest.

>>> +#

>>> +# Some unittest overlays deliberately contain errors that unittest checks for.

>>> +# These overlays will cause fdtoverlay to fail, and are thus not included

>>> +# in the static test:

>>> +#			overlay.dtb \

>>> +#			overlay_bad_add_dup_node.dtb \

>>> +#			overlay_bad_add_dup_prop.dtb \

>>> +#			overlay_bad_phandle.dtb \

>>> +#			overlay_bad_symbol.dtb \

>>> +#			overlay_base.dtb \

>>> +

>>> +apply_static_overlay := overlay_0.dtb \

>>> +			overlay_1.dtb \

>>> +			overlay_2.dtb \

>>> +			overlay_3.dtb \

>>> +			overlay_4.dtb \

>>> +			overlay_5.dtb \

>>> +			overlay_6.dtb \

>>> +			overlay_7.dtb \

>>> +			overlay_8.dtb \

>>> +			overlay_9.dtb \

>>> +			overlay_10.dtb \

>>> +			overlay_11.dtb \

>>> +			overlay_12.dtb \

>>> +			overlay_13.dtb \

>>> +			overlay_15.dtb \

>>> +			overlay_gpio_01.dtb \

>>> +			overlay_gpio_02a.dtb \

>>> +			overlay_gpio_02b.dtb \

>>> +			overlay_gpio_03.dtb \

>>> +			overlay_gpio_04a.dtb \

>>> +			overlay_gpio_04b.dtb

>>> +

>>> +quiet_cmd_fdtoverlay = FDTOVERLAY $@

>>> +      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^

>>> +

>>> +$(obj)/static_test.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(apply_static_overlay))

>>> +	$(call if_changed,fdtoverlay)

>>> +

>>> +always-$(CONFIG_OF_OVERLAY) += static_test.dtb

>>

>> The fact that testcases.dts includes /plugin/ still seems completely

>> wrong, if it's being used as the base tree.

>>

> 

> Yes, the build rule for static_test.dtb is using testcases.dtb as the base FDT.

> It is a convenient FDT to use because it provides the frame that the overlays

> require to be applied.  It is fortunate that fdtoverlay does not reject the use

> of an FDT with overlay metadata as the base blob.

> 

> If Viresh wants to test a more realistic data set then he could create a build

> rule that copies testcases.dts into (for example) testcases_base.dts, strip

> out the '/plugin/;" then compile that into testcases_base.dtb and use that for

> fdtoverlay.

> 

>    pseudo makefile rule for testcases_base.dts:

>        sed -e 's|/plugin/;||' > testcases_base.dts

> 

>    add testcases_base.dtb to the list of objects to build

> 

>    change the rule for static_test.dtb to use testcases_base.dtb instead of

>    testcases.dtb

> 

> This is probably a good idea instead of depending on the leniency of fdtoverlay.


It should be possible to apply this same concept to copying overlay_base.dts
to overlay_base_base.dts, removing the "/plugin/;" from overlay_base_base.dts
and using an additional rule to use fdtoverlay to apply overlay.dtb on top
of overlay_base_base.dtb.

Yes, overlay_base_base is a terrible name.  Just used to illustrate the point.

I tried this by hand and am failing miserably.  But I am not using the proper
environment (just a quick hack to see if the method might work).  So I would
have to set things up properly to really test this.

If this does work, it would remove my objections to you trying to transform
the existing unittest .dts test data files (because you would not have to
actually modify the existing .dts files).

> 

> -Frank

>
Viresh Kumar Jan. 21, 2021, 5:43 a.m. UTC | #5 
Hi Frank,

On 20-01-21, 23:34, Frank Rowand wrote:
> It should be possible to apply this same concept to copying overlay_base.dts

> to overlay_base_base.dts, removing the "/plugin/;" from overlay_base_base.dts

> and using an additional rule to use fdtoverlay to apply overlay.dtb on top

> of overlay_base_base.dtb.


Are you suggesting to then merge this with testcases.dtb to get
static_test.dtb or keep two output files (static_test.dtb from
testcases.dtb + overlays and static_test2.dtb from overlay_base.dtb
and overlay.dtb) ?

Asking because as I mentioned earlier, overlay_base.dtb doesn't have
__overlay__ property for its nodes and we can't apply that to
testcases.dtb using fdtoverlay.

> Yes, overlay_base_base is a terrible name.  Just used to illustrate the point.

> 

> I tried this by hand and am failing miserably.  But I am not using the proper

> environment (just a quick hack to see if the method might work).  So I would

> have to set things up properly to really test this.

> 

> If this does work, it would remove my objections to you trying to transform

> the existing unittest .dts test data files (because you would not have to

> actually modify the existing .dts files).


-- 
viresh
Frank Rowand Jan. 21, 2021, 5:45 a.m. UTC | #6 
On 1/20/21 11:34 PM, Viresh Kumar wrote:
> On 20-01-21, 23:14, Frank Rowand wrote:

>> It is a convenient FDT to use because it provides the frame that the overlays

>> require to be applied.  It is fortunate that fdtoverlay does not reject the use

>> of an FDT with overlay metadata as the base blob.

> 

>> This is probably a good idea instead of depending on the leniency of fdtoverlay.

> 

> I believe fdtoverlay allows that intentionally, that would be required

> for the cases where we have a hierarchy of extension boards or

> overlays.

> 

> A platform can have a base dtb (with /plugin/;), then we can have an

> overlay (1) for an extension board (with /plugin/;) and then an

> overlay (2) for an extension board for the previous extension board.

> 

> In such a case overlay-(2) can't be applied directly to the base dtb

> as it may not find all the nodes it is trying to update. And so

> overlay-(2) needs to be applied to overlay-(1) and then the output of

> this can be applied to the base dtb.


I have only the most surface knowledge of fdtoverlay, mostly from
"fdtoverlay --help", but you can apply multiple overlays with a
single invocation of fdtoverlay.  My _assumption_ was that the
overlays would be applied in order, and after any given overlay
was applied, subsequent overlays could reference the previously
applied overlay.

Is my assumption incorrect?

> 

> This is very similar to what I tried with the intermediate.dtb

> earlier.

>
Viresh Kumar Jan. 21, 2021, 5:50 a.m. UTC | #7 
On 20-01-21, 23:45, Frank Rowand wrote:
> I have only the most surface knowledge of fdtoverlay, mostly from

> "fdtoverlay --help", but you can apply multiple overlays with a

> single invocation of fdtoverlay.  My _assumption_ was that the

> overlays would be applied in order, and after any given overlay

> was applied, subsequent overlays could reference the previously

> applied overlay.

> 

> Is my assumption incorrect?


I think yes, if everything is in order then it should work just fine.

I was only suggesting that fdtoverlay accepting the base overlay with
/plugin/; may well be a requirement and so intentionally done.

-- 
viresh
Frank Rowand Jan. 21, 2021, 5:55 a.m. UTC | #8 
On 1/20/21 11:43 PM, Viresh Kumar wrote:
> Hi Frank,

> 

> On 20-01-21, 23:34, Frank Rowand wrote:

>> It should be possible to apply this same concept to copying overlay_base.dts

>> to overlay_base_base.dts, removing the "/plugin/;" from overlay_base_base.dts

>> and using an additional rule to use fdtoverlay to apply overlay.dtb on top

>> of overlay_base_base.dtb.

> 

> Are you suggesting to then merge this with testcases.dtb to get

> static_test.dtb


no

> or keep two output files (static_test.dtb from

> testcases.dtb + overlays and static_test2.dtb from overlay_base.dtb

> and overlay.dtb) ?


yes, but using the modified versions ("/plugin/;" removed) of
testcases.dtb and overlay_base.dtb.

> 

> Asking because as I mentioned earlier, overlay_base.dtb doesn't have

> __overlay__ property for its nodes and we can't apply that to

> testcases.dtb using fdtoverlay.


Correct.

I apologize in advance if I get confused in these threads or cause confusion.
I find myself going in circles and losing track of how things fit together as
I move through the various pieces of unittest.

-Frank

> 

>> Yes, overlay_base_base is a terrible name.  Just used to illustrate the point.

>>

>> I tried this by hand and am failing miserably.  But I am not using the proper

>> environment (just a quick hack to see if the method might work).  So I would

>> have to set things up properly to really test this.

>>

>> If this does work, it would remove my objections to you trying to transform

>> the existing unittest .dts test data files (because you would not have to

>> actually modify the existing .dts files).

>
Viresh Kumar Jan. 21, 2021, 5:58 a.m. UTC | #9 
On 20-01-21, 23:55, Frank Rowand wrote:
> yes, but using the modified versions ("/plugin/;" removed) of

> testcases.dtb and overlay_base.dtb.


Okay, that would work fine I guess. I will try to implement this in
the new version.

> I apologize in advance if I get confused in these threads or cause confusion.

> I find myself going in circles and losing track of how things fit together as

> I move through the various pieces of unittest.


Me too :)

Today is the first time where we have some overlap in our work hours
(probably you working late :)), and we are able to get this sorted out
quickly enough.

Thanks for your feedback Frank.

-- 
viresh
David Gibson Jan. 21, 2021, 6:34 a.m. UTC | #10 
On Thu, Jan 21, 2021 at 11:04:26AM +0530, Viresh Kumar wrote:
> On 20-01-21, 23:14, Frank Rowand wrote:

> > It is a convenient FDT to use because it provides the frame that the overlays

> > require to be applied.  It is fortunate that fdtoverlay does not reject the use

> > of an FDT with overlay metadata as the base blob.

> 

> > This is probably a good idea instead of depending on the leniency of fdtoverlay.

> 

> I believe fdtoverlay allows that intentionally, that would be required

> for the cases where we have a hierarchy of extension boards or

> overlays.


Um.. no.

> A platform can have a base dtb (with /plugin/;), then we can have an

> overlay (1) for an extension board (with /plugin/;) and then an

> overlay (2) for an extension board for the previous extension board.

> 

> In such a case overlay-(2) can't be applied directly to the base dtb

> as it may not find all the nodes it is trying to update. And so

> overlay-(2) needs to be applied to overlay-(1) and then the output of

> this can be applied to the base dtb.


No, this is the wrong way around.  The expected operation here is that
you apply overlay (1) to the base tree, giving you, say, output1.dtb.
output1.dtb is (effectively) a base tree itself, to which you can then
apply overlay-(2).

What you're talking about is "merging" overlays: combingin overlay (1)
and (2) into overlay-(X) which would have the same effect applied to
base.dtb as (1) and (2) applied in sequence.  Merging overlays is
something that could make sense, but fdtoverlay will not do it at
present.

> This is very similar to what I tried with the intermediate.dtb

> earlier.

> 


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
David Gibson Jan. 21, 2021, 6:36 a.m. UTC | #11 
On Thu, Jan 21, 2021 at 11:20:40AM +0530, Viresh Kumar wrote:
> On 20-01-21, 23:45, Frank Rowand wrote:

> > I have only the most surface knowledge of fdtoverlay, mostly from

> > "fdtoverlay --help", but you can apply multiple overlays with a

> > single invocation of fdtoverlay.  My _assumption_ was that the

> > overlays would be applied in order, and after any given overlay

> > was applied, subsequent overlays could reference the previously

> > applied overlay.

> > 

> > Is my assumption incorrect?

> 

> I think yes, if everything is in order then it should work just fine.

> 

> I was only suggesting that fdtoverlay accepting the base overlay with

> /plugin/; may well be a requirement and so intentionally done.


No.  It's simply the result of the fact that a dtbo is still a dtb.
So, you can still apply an overlay to it.  However, a dtbo is a
weirdly structured dtb, so applying an overlay to it is very unlikely
to give you something useful.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
David Gibson Jan. 21, 2021, 6:37 a.m. UTC | #12 
On Wed, Jan 20, 2021 at 11:55:08PM -0600, Frank Rowand wrote:
> On 1/20/21 11:43 PM, Viresh Kumar wrote:

> > Hi Frank,

> > 

> > On 20-01-21, 23:34, Frank Rowand wrote:

> >> It should be possible to apply this same concept to copying overlay_base.dts

> >> to overlay_base_base.dts, removing the "/plugin/;" from overlay_base_base.dts

> >> and using an additional rule to use fdtoverlay to apply overlay.dtb on top

> >> of overlay_base_base.dtb.

> > 

> > Are you suggesting to then merge this with testcases.dtb to get

> > static_test.dtb

> 

> no

> 

> > or keep two output files (static_test.dtb from

> > testcases.dtb + overlays and static_test2.dtb from overlay_base.dtb

> > and overlay.dtb) ?

> 

> yes, but using the modified versions ("/plugin/;" removed) of

> testcases.dtb and overlay_base.dtb.


I really don't understand why you want /plugin/ in *any* version of
testcases.dtb.
> > 

> > Asking because as I mentioned earlier, overlay_base.dtb doesn't have

> > __overlay__ property for its nodes and we can't apply that to

> > testcases.dtb using fdtoverlay.

> 

> Correct.

> 

> I apologize in advance if I get confused in these threads or cause confusion.

> I find myself going in circles and losing track of how things fit together as

> I move through the various pieces of unittest.

> 

> -Frank

> 

> > 

> >> Yes, overlay_base_base is a terrible name.  Just used to illustrate the point.

> >>

> >> I tried this by hand and am failing miserably.  But I am not using the proper

> >> environment (just a quick hack to see if the method might work).  So I would

> >> have to set things up properly to really test this.

> >>

> >> If this does work, it would remove my objections to you trying to transform

> >> the existing unittest .dts test data files (because you would not have to

> >> actually modify the existing .dts files).

> > 

> 


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Viresh Kumar Jan. 21, 2021, 6:57 a.m. UTC | #13 
On 21-01-21, 17:34, David Gibson wrote:
> No, this is the wrong way around.  The expected operation here is that

> you apply overlay (1) to the base tree, giving you, say, output1.dtb.

> output1.dtb is (effectively) a base tree itself, to which you can then

> apply overlay-(2).


Thanks for the confirmation about this.

> Merging overlays is

> something that could make sense, but fdtoverlay will not do it at

> present.


FWIW, I think it works fine right now even if it not intentional. I
did inspect the output dtb (made by merging two overlays) using
fdtdump and it looked okay. But yeah, I understand that we shouldn't
do it.

-- 
viresh
Frank Rowand Jan. 21, 2021, 7:04 a.m. UTC | #14 
On 1/20/21 11:58 PM, Viresh Kumar wrote:
> On 20-01-21, 23:55, Frank Rowand wrote:

>> yes, but using the modified versions ("/plugin/;" removed) of

>> testcases.dtb and overlay_base.dtb.

> 

> Okay, that would work fine I guess. I will try to implement this in

> the new version.

> 

>> I apologize in advance if I get confused in these threads or cause confusion.

>> I find myself going in circles and losing track of how things fit together as

>> I move through the various pieces of unittest.

> 

> Me too :)

> 

> Today is the first time where we have some overlap in our work hours

> (probably you working late :)), and we are able to get this sorted out

> quickly enough.


Working quite late.  I swear I stopped working 3 hours ago and that was
already late.

I reacted quite negatively to the attempt to restructure the unittest
.dts file in the original patch.  Now after walking around the problem
space a bit, and reviewing the ugly things that unittest.c does, and
coming up with the approach of sed to copy and modify the two base
.dts files, I think I finally have my head wrapped around an easier
and cleaner approach than sed.

I'll describe it for just one of the two base files, but the same
concept would apply to the other.  Don't take my file names as
good suggestions, I am avoiding using the brain power to come up
with good names at the moment.

1) rename overlay_base.dts to overlay_base.dtsi

2) remove "/dtgs-v1/" and "/plugin/:" from overlay_base.dtsi

3) create a new overlay_base.dts:
   // SPDX-License-Identifier: GPL-2.0
   /dts-v1/;
   /plugin/;
   #include overlay_base_dtsi

4) create a new file overlay_base_static.dts:
   // SPDX-License-Identifier: GPL-2.0
   /dts-v1/;
   #include overlay_base_dtsi

5) then use overlay_base_static.dtb as the base blob for ftdoverlay
   applying overlay.dtb

Untested, hand typed, hopefully no typos.

-Frank

> 

> Thanks for your feedback Frank.

>
David Gibson Jan. 21, 2021, 11:39 p.m. UTC | #15 
On Thu, Jan 21, 2021 at 12:27:28PM +0530, Viresh Kumar wrote:
> On 21-01-21, 17:34, David Gibson wrote:

> > No, this is the wrong way around.  The expected operation here is that

> > you apply overlay (1) to the base tree, giving you, say, output1.dtb.

> > output1.dtb is (effectively) a base tree itself, to which you can then

> > apply overlay-(2).

> 

> Thanks for the confirmation about this.

> 

> > Merging overlays is

> > something that could make sense, but fdtoverlay will not do it at

> > present.

> 

> FWIW, I think it works fine right now even if it not intentional.


No, it definitely will not work in general.  It might kinda work in a
few trivial cases, but it absolutely will not do the neccessary
handling in some cases.

> I

> did inspect the output dtb (made by merging two overlays) using

> fdtdump and it looked okay.


Ok.. but if you're using these bizarre messed up "dtbs" that this test
code seems to be, I don't really trust that tells you much.

> But yeah, I understand that we shouldn't

> do it.



-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
David Gibson Jan. 21, 2021, 11:41 p.m. UTC | #16 
On Thu, Jan 21, 2021 at 01:04:46AM -0600, Frank Rowand wrote:
> On 1/20/21 11:58 PM, Viresh Kumar wrote:

> > On 20-01-21, 23:55, Frank Rowand wrote:

> >> yes, but using the modified versions ("/plugin/;" removed) of

> >> testcases.dtb and overlay_base.dtb.

> > 

> > Okay, that would work fine I guess. I will try to implement this in

> > the new version.

> > 

> >> I apologize in advance if I get confused in these threads or cause confusion.

> >> I find myself going in circles and losing track of how things fit together as

> >> I move through the various pieces of unittest.

> > 

> > Me too :)

> > 

> > Today is the first time where we have some overlap in our work hours

> > (probably you working late :)), and we are able to get this sorted out

> > quickly enough.

> 

> Working quite late.  I swear I stopped working 3 hours ago and that was

> already late.

> 

> I reacted quite negatively to the attempt to restructure the unittest

> .dts file in the original patch.  Now after walking around the problem

> space a bit, and reviewing the ugly things that unittest.c does, and

> coming up with the approach of sed to copy and modify the two base

> .dts files, I think I finally have my head wrapped around an easier

> and cleaner approach than sed.

> 

> I'll describe it for just one of the two base files, but the same

> concept would apply to the other.  Don't take my file names as

> good suggestions, I am avoiding using the brain power to come up

> with good names at the moment.

> 

> 1) rename overlay_base.dts to overlay_base.dtsi

> 

> 2) remove "/dtgs-v1/" and "/plugin/:" from overlay_base.dtsi

> 

> 3) create a new overlay_base.dts:

>    // SPDX-License-Identifier: GPL-2.0

>    /dts-v1/;

>    /plugin/;

>    #include overlay_base_dtsi


"overlay_base" is a terrible name - it's not clear if it's supposed to
be a base tree or an overlay.  I'm still not seeing any reasonable
use for the plugin version either, fwiw.

> 

> 4) create a new file overlay_base_static.dts:

>    // SPDX-License-Identifier: GPL-2.0

>    /dts-v1/;

>    #include overlay_base_dtsi

> 

> 5) then use overlay_base_static.dtb as the base blob for ftdoverlay

>    applying overlay.dtb

> 

> Untested, hand typed, hopefully no typos.

> 

> -Frank

> 

> > 

> > Thanks for your feedback Frank.

> > 

> 


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
Viresh Kumar Jan. 22, 2021, 3:10 a.m. UTC | #17 
On 22-01-21, 10:39, David Gibson wrote:
> No, it definitely will not work in general.  It might kinda work in a

> few trivial cases, but it absolutely will not do the neccessary

> handling in some cases.

> 

> > I

> > did inspect the output dtb (made by merging two overlays) using

> > fdtdump and it looked okay.

> 

> Ok.. but if you're using these bizarre messed up "dtbs" that this test

> code seems to be, I don't really trust that tells you much.


I only looked if the changes from the second overlay were present in
the merge and they were. And so I assumed that it must have worked.

What about checking the base dtb for /plugin/; in fdtoverlay and fail
the whole thing in case it is present ? I think it is possible for
people to get confused otherwise, like I did.

-- 
viresh
David Gibson Jan. 22, 2021, 4:27 a.m. UTC | #18 
On Fri, Jan 22, 2021 at 08:40:49AM +0530, Viresh Kumar wrote:
> On 22-01-21, 10:39, David Gibson wrote:

> > No, it definitely will not work in general.  It might kinda work in a

> > few trivial cases, but it absolutely will not do the neccessary

> > handling in some cases.

> > 

> > > I

> > > did inspect the output dtb (made by merging two overlays) using

> > > fdtdump and it looked okay.

> > 

> > Ok.. but if you're using these bizarre messed up "dtbs" that this test

> > code seems to be, I don't really trust that tells you much.

> 

> I only looked if the changes from the second overlay were present in

> the merge and they were. And so I assumed that it must have worked.

> 

> What about checking the base dtb for /plugin/; in fdtoverlay and fail

> the whole thing in case it is present ? I think it is possible for

> people to get confused otherwise, like I did.


/plugin/ doesn't exist in the dtb, only in the dts.  From the dtb
encoding point of view, there's no difference between a dtb and a
dtbo, a dtbo is just a dtb that follows some conventions for its
content.

If we were doing this from scratch, it would be better for dtbos to
have a different magic number from regular dtbs.  I think I actually
suggested that sometime in the past, but by the time that came up,
dtbos were already in pretty widespread use with the existing format.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
diff mbox series

Patch

diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index 009f4045c8e4..ece7dfd5cafa 100644
--- a/drivers/of/unittest-data/Makefile
+++ b/drivers/of/unittest-data/Makefile
@@ -38,3 +38,53 @@  DTC_FLAGS_testcases += -@
 
 # suppress warnings about intentional errors
 DTC_FLAGS_testcases += -Wno-interrupts_property
+
+# Apply overlays statically with fdtoverlay.  This is a build time test that
+# the overlays can be applied successfully by fdtoverlay.  This does not
+# guarantee that the overlays can be applied successfully at run time by
+# unittest, but it provides a bit of build time test coverage for those
+# who do not execute unittest.
+#
+# The overlays are applied on top of testcases.dtb to create static_test.dtb
+# If fdtoverlay detects an error than the kernel build will fail.
+# static_test.dtb is not consumed by unittest.
+#
+# Some unittest overlays deliberately contain errors that unittest checks for.
+# These overlays will cause fdtoverlay to fail, and are thus not included
+# in the static test:
+#			overlay.dtb \
+#			overlay_bad_add_dup_node.dtb \
+#			overlay_bad_add_dup_prop.dtb \
+#			overlay_bad_phandle.dtb \
+#			overlay_bad_symbol.dtb \
+#			overlay_base.dtb \
+
+apply_static_overlay := overlay_0.dtb \
+			overlay_1.dtb \
+			overlay_2.dtb \
+			overlay_3.dtb \
+			overlay_4.dtb \
+			overlay_5.dtb \
+			overlay_6.dtb \
+			overlay_7.dtb \
+			overlay_8.dtb \
+			overlay_9.dtb \
+			overlay_10.dtb \
+			overlay_11.dtb \
+			overlay_12.dtb \
+			overlay_13.dtb \
+			overlay_15.dtb \
+			overlay_gpio_01.dtb \
+			overlay_gpio_02a.dtb \
+			overlay_gpio_02b.dtb \
+			overlay_gpio_03.dtb \
+			overlay_gpio_04a.dtb \
+			overlay_gpio_04b.dtb
+
+quiet_cmd_fdtoverlay = FDTOVERLAY $@
+      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
+
+$(obj)/static_test.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(apply_static_overlay))
+	$(call if_changed,fdtoverlay)
+
+always-$(CONFIG_OF_OVERLAY) += static_test.dtb