Re: [PATCH v4 1/1] of: unittest: overlay: ensure proper alignment of copied FDT
On 4/8/21 9:20 PM, Guenter Roeck wrote: > On 4/8/21 3:53 PM, Frank Rowand wrote: >> On 4/8/21 4:54 PM, Guenter Roeck wrote: >>> On 4/8/21 2:28 PM, Rob Herring wrote: >>>> >>>> Applying now so this gets into linux-next this week. >>>> >>> The patch doesn't apply on top of today's -next; it conflicts >>> with "of: properly check for error returned by fdt_get_name()". >>> >>> I reverted that patch and applied this one, and the DT unittests >>> run with it on openrisc. I do get a single test failure, but I that >>> is a different problem (possibly with the test case itself). >>> >>> ### dt-test ### FAIL of_unittest_dma_ranges_one():923 of_dma_get_range: >>> wrong DMA addr 0x >>> (expecting 1) on node >>> /testcase-data/address-tests/bus@8000/device@1000 >> >> That is a known regression on the target that I use for testing (and >> has been since 5.10-rc1) - the 8074 dragonboard, arm 32. No >> one else has reported it on the list, so even though I want to debug >> and fix it "promptly", other tasks have had higher priority. In my >> notes I list two suspect commits: >> >> e0d072782c73 dma-mapping: introduce DMA range map, supplanting >> dma_pfn_offset >> 0a0f0d8be76d dma-mapping: split >> >> I think that was purely based on looking at the list of commits that >> may have touched OF dma. I have not done a bisect. >> > > Here you are: > > # bad: [2c85ebc57b3e1817b6ce1a6b703928e113a90442] Linux 5.10 > # good: [bbf5c979011a099af5dc76498918ed7df445635b] Linux 5.9 > git bisect start 'v5.10' 'v5.9' > # bad: [4d0e9df5e43dba52d38b251e3b909df8fa1110be] lib, uaccess: add failure > injection to usercopy functions > git bisect bad 4d0e9df5e43dba52d38b251e3b909df8fa1110be > # good: [f888bdf9823c85fe945c4eb3ba353f749dec3856] Merge tag > 'devicetree-for-5.10' of > git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux > git bisect good f888bdf9823c85fe945c4eb3ba353f749dec3856 > # good: [640eee067d9aae0bb98d8706001976ff1affaf00] Merge tag > 'drm-misc-next-fixes-2020-10-13' of > git://anongit.freedesktop.org/drm/drm-misc into drm-next > git bisect good 640eee067d9aae0bb98d8706001976ff1affaf00 > # good: [c6dbef7307629cce855aa6b482b60cbfed88] Merge tag 'usb-5.10-rc1' > of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb > git bisect good c6dbef7307629cce855aa6b482b60cbfed88 > # good: [ce1558c285f9ad04c03b46833a028230771cc0a7] ALSA: hda/hdmi: fix > incorrect locking in hdmi_pcm_close > git bisect good ce1558c285f9ad04c03b46833a028230771cc0a7 > # good: [c48b75b7271db23c1b2d1204d6e8496d91f27711] Merge tag 'sound-5.10-rc1' > of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound > git bisect good c48b75b7271db23c1b2d1204d6e8496d91f27711 > # bad: [0cd7d9795fa82226e7516d38b474bddae8b1ff26] Merge branch 'for-linus' of > git://git.kernel.org/pub/scm/linux/kernel/git/livepatching/livepatching > git bisect bad 0cd7d9795fa82226e7516d38b474bddae8b1ff26 > # good: [b1839e7c2a42ccd9a0587c0092e880c7a213ee2a] dmaengine: xilinx: dpdma: > convert tasklets to use new tasklet_setup() API > git bisect good b1839e7c2a42ccd9a0587c0092e880c7a213ee2a > # bad: [0de327969b61a245e3a47b60009eae73fe513cef] cma: decrease CMA_ALIGNMENT > lower limit to 2 > git bisect bad 0de327969b61a245e3a47b60009eae73fe513cef > # good: [6eb0233ec2d0df288fe8515d5b0b2b15562e05bb] usb: don't inherity DMA > properties for USB devices > git bisect good 6eb0233ec2d0df288fe8515d5b0b2b15562e05bb > # bad: [48d15814dd0fc429e3205b87f1af6cc472018478] lib82596: move DMA > allocation into the callers of i82596_probe > git bisect bad 48d15814dd0fc429e3205b87f1af6cc472018478 > # bad: [eba304c6861613a649ba46cfab835b1eddeacd8e] dma-mapping: better > document dma_addr_t and DMA_MAPPING_ERROR > git bisect bad eba304c6861613a649ba46cfab835b1eddeacd8e > # bad: [b9bb694b9f62f4b31652223ed3ca38cf98bbb370] iommu/io-pgtable-arm: Clean > up faulty sanity check > git bisect bad b9bb694b9f62f4b31652223ed3ca38cf98bbb370 > # bad: [a97740f81874c8063c12c24f34d25f10c4f5e9aa] dma-debug: convert comma to > semicolon > git bisect bad a97740f81874c8063c12c24f34d25f10c4f5e9aa > # bad: [e0d072782c734d27f5af062c62266f2598f68542] dma-mapping: introduce DMA > range map, supplanting dma_pfn_offset > git bisect bad e0d072782c734d27f5af062c62266f2598f68542 > # first bad commit: [e0d072782c734d27f5af062c62266f2598f68542] dma-mapping: > introduce DMA range map, supplanting dma_pfn_offset > > Guenter > Thank you
Re: [PATCH v4 1/1] of: unittest: overlay: ensure proper alignment of copied FDT
On 4/8/21 4:54 PM, Guenter Roeck wrote: > On 4/8/21 2:28 PM, Rob Herring wrote: >> >> Applying now so this gets into linux-next this week. >> > The patch doesn't apply on top of today's -next; it conflicts > with "of: properly check for error returned by fdt_get_name()". > > I reverted that patch and applied this one, and the DT unittests > run with it on openrisc. I do get a single test failure, but I that > is a different problem (possibly with the test case itself). > > ### dt-test ### FAIL of_unittest_dma_ranges_one():923 of_dma_get_range: wrong > DMA addr 0x > (expecting 1) on node > /testcase-data/address-tests/bus@8000/device@1000 That is a known regression on the target that I use for testing (and has been since 5.10-rc1) - the 8074 dragonboard, arm 32. No one else has reported it on the list, so even though I want to debug and fix it "promptly", other tasks have had higher priority. In my notes I list two suspect commits: e0d072782c73 dma-mapping: introduce DMA range map, supplanting dma_pfn_offset 0a0f0d8be76d dma-mapping: split I think that was purely based on looking at the list of commits that may have touched OF dma. I have not done a bisect. One specific report of not seeing the FAIL was Vireshk on 5.11-rc6 with a Hikey board. > > Tested-by: Guenter Roeck Thanks for testing! > > Guenter >
Re: [PATCH v4 1/1] of: unittest: overlay: ensure proper alignment of copied FDT
On 4/8/21 4:28 PM, Rob Herring wrote:
> On Thu, Apr 8, 2021 at 3:45 PM wrote:
>>
>> From: Frank Rowand
>>
>> The Devicetree standard specifies an 8 byte alignment of the FDT.
>> Code in libfdt expects this alignment for an FDT image in memory.
>> kmemdup() returns 4 byte alignment on openrisc. Replace kmemdup()
>> with kmalloc(), align pointer, memcpy() to get proper alignment.
>>
>> The 4 byte alignment exposed a related bug which triggered a crash
>> on openrisc with:
>> commit 79edff12060f ("scripts/dtc: Update to upstream version
>> v1.6.0-51-g183df9e9c2b9")
>> as reported in:
>> https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/
>>
>> Reported-by: Guenter Roeck
>> Signed-off-by: Frank Rowand
>>
>> ---
>>
>> changes since version 1:
>> - use pointer from kmalloc() for kfree() instead of using pointer that
>> has been modified for FDT alignment
>>
>> changes since version 2:
>> - version 1 was a work in progress version, I failed to commit the
>> following
>> final changes
>> - reorder first two arguments of of_overlay_apply()
>>
>> changes since version 3:
>> - size of memory allocation and size of copy after pointer alignment
>> differ, use separate variables with correct values for each case
>> - edit comment to more clearly describe that ovcs->fdt is the allocated
>> memory region, which may be different than where the aligned pointer
>> points
>> - remove unused parameter from of_overlay_apply()
>>
>> drivers/of/of_private.h | 2 ++
>> drivers/of/overlay.c| 27 +--
>> drivers/of/unittest.c | 13 ++---
>> 3 files changed, 29 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
>> index d9e6a324de0a..d717efbd637d 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -8,6 +8,8 @@
>> * Copyright (C) 1996-2005 Paul Mackerras.
>> */
>>
>> +#define FDT_ALIGN_SIZE 8
>> +
>> /**
>> * struct alias_prop - Alias property in 'aliases' node
>> * @link: List node to link the structure in aliases_lookup list
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 50bbe0edf538..ecf967c57900 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -57,7 +57,7 @@ struct fragment {
>> * struct overlay_changeset
>> * @id:changeset identifier
>> * @ovcs_list: list on which we are located
>> - * @fdt: FDT that was unflattened to create @overlay_tree
>> + * @fdt: base of memory allocated to hold aligned FDT that
>> was unflattened to create @overlay_tree
>> * @overlay_tree: expanded device tree that contains the fragment nodes
>> * @count: count of fragment structures
>> * @fragments: fragment nodes in the overlay expanded device tree
>> @@ -719,8 +719,8 @@ static struct device_node *find_target(struct
>> device_node *info_node)
>> /**
>> * init_overlay_changeset() - initialize overlay changeset from overlay tree
>> * @ovcs: Overlay changeset to build
>> - * @fdt: the FDT that was unflattened to create @tree
>> - * @tree: Contains all the overlay fragments and overlay fixup nodes
>> + * @fdt: base of memory allocated to hold aligned FDT that was
>> unflattened to create @tree
>> + * @tree: Contains the overlay fragments and overlay fixup nodes
>> *
>> * Initialize @ovcs. Populate @ovcs->fragments with node information from
>> * the top level of @tree. The relevant top level nodes are the fragment
>> @@ -873,7 +873,7 @@ static void free_overlay_changeset(struct
>> overlay_changeset *ovcs)
>> * internal documentation
>> *
>> * of_overlay_apply() - Create and apply an overlay changeset
>> - * @fdt: the FDT that was unflattened to create @tree
>> + * @fdt: base of memory allocated to hold the aligned FDT
>> * @tree: Expanded overlay device tree
>> * @ovcs_id: Pointer to overlay changeset id
>> *
>> @@ -913,7 +913,7 @@ static void free_overlay_changeset(struct
>> overlay_changeset *ovcs)
>> */
>>
>> static int of_overlay_apply(const void *fdt, struct device_node *tree,
>> - int *ovcs_id)
>> + int *ovcs_id)
>> {
>> struct overlay_changeset *ovcs;
>> int ret = 0, ret
Re: [PATCH v3 1/1] of: unittest: overlay: ensure proper alignment of copied FDT
On 4/8/21 1:27 PM, Rob Herring wrote:
> On Thu, Apr 8, 2021 at 10:17 AM wrote:
>>
>> From: Frank Rowand
>>
>> The Devicetree standard specifies an 8 byte alignment of the FDT.
>> Code in libfdt expects this alignment for an FDT image in memory.
>> kmemdup() returns 4 byte alignment on openrisc. Replace kmemdup()
>> with kmalloc(), align pointer, memcpy() to get proper alignment.
>>
>> The 4 byte alignment exposed a related bug which triggered a crash
>> on openrisc with:
>> commit 79edff12060f ("scripts/dtc: Update to upstream version
>> v1.6.0-51-g183df9e9c2b9")
>> as reported in:
>> https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/
>>
>> Reported-by: Guenter Roeck
>> Signed-off-by: Frank Rowand
>> ---
>>
>> changes since version 1:
>> - use pointer from kmalloc() for kfree() instead of using pointer that
>> has been modified for FDT alignment
>>
>> changes since version 2:
>> - version 1 was a work in progress version, I failed to commit the
>> following
>> final changes
>> - reorder first two arguments of of_overlay_apply()
>>
>> drivers/of/of_private.h | 2 ++
>> drivers/of/overlay.c| 28 +---
>> drivers/of/unittest.c | 12 +---
>> 3 files changed, 28 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
>> index d9e6a324de0a..d717efbd637d 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -8,6 +8,8 @@
>> * Copyright (C) 1996-2005 Paul Mackerras.
>> */
>>
>> +#define FDT_ALIGN_SIZE 8
>> +
>> /**
>> * struct alias_prop - Alias property in 'aliases' node
>> * @link: List node to link the structure in aliases_lookup list
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 50bbe0edf538..cf770452e1e5 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -57,7 +57,7 @@ struct fragment {
>> * struct overlay_changeset
>> * @id:changeset identifier
>> * @ovcs_list: list on which we are located
>> - * @fdt: FDT that was unflattened to create @overlay_tree
>> + * @fdt: base of memory allocated to hold aligned FDT that
>> was unflattened to create @overlay_tree
>> * @overlay_tree: expanded device tree that contains the fragment nodes
>> * @count: count of fragment structures
>> * @fragments: fragment nodes in the overlay expanded device tree
>> @@ -719,8 +719,8 @@ static struct device_node *find_target(struct
>> device_node *info_node)
>> /**
>> * init_overlay_changeset() - initialize overlay changeset from overlay tree
>> * @ovcs: Overlay changeset to build
>> - * @fdt: the FDT that was unflattened to create @tree
>> - * @tree: Contains all the overlay fragments and overlay fixup nodes
>> + * @fdt: base of memory allocated to hold aligned FDT that was
>> unflattened to create @tree
>> + * @tree: Contains the overlay fragments and overlay fixup nodes
>> *
>> * Initialize @ovcs. Populate @ovcs->fragments with node information from
>> * the top level of @tree. The relevant top level nodes are the fragment
>> @@ -873,7 +873,8 @@ static void free_overlay_changeset(struct
>> overlay_changeset *ovcs)
>> * internal documentation
>> *
>> * of_overlay_apply() - Create and apply an overlay changeset
>> - * @fdt: the FDT that was unflattened to create @tree
>> + * @fdt: base of memory allocated to hold *@fdt_align
>> + * @fdt_align: the FDT that was unflattened to create @tree, aligned
>> * @tree: Expanded overlay device tree
>> * @ovcs_id: Pointer to overlay changeset id
>> *
>> @@ -912,8 +913,8 @@ static void free_overlay_changeset(struct
>> overlay_changeset *ovcs)
>> * id is returned to *ovcs_id.
>> */
>>
>> -static int of_overlay_apply(const void *fdt, struct device_node *tree,
>> - int *ovcs_id)
>> +static int of_overlay_apply(const void *fdt, const void *fdt_align,
>> + struct device_node *tree, int *ovcs_id)
>
> I think it's better if you move the kfree's out of this function. It
> would be a broken design if this function was public because you'd
> have no idea if 'fdt' could be freed or not. No reason to have that
> bad design just because it's static. If a function returns an error,
> then it should undo every
Re: [PATCH v3 1/1] of: unittest: overlay: ensure proper alignment of copied FDT
Hi Guenter,
Thanks for the review!
On 4/8/21 10:32 AM, Guenter Roeck wrote:
> On 4/8/21 8:17 AM, frowand.l...@gmail.com wrote:
>> From: Frank Rowand
>>
>> The Devicetree standard specifies an 8 byte alignment of the FDT.
>> Code in libfdt expects this alignment for an FDT image in memory.
>> kmemdup() returns 4 byte alignment on openrisc. Replace kmemdup()
>> with kmalloc(), align pointer, memcpy() to get proper alignment.
>>
>> The 4 byte alignment exposed a related bug which triggered a crash
>> on openrisc with:
>> commit 79edff12060f ("scripts/dtc: Update to upstream version
>> v1.6.0-51-g183df9e9c2b9")
>> as reported in:
>> https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/
>>
>> Reported-by: Guenter Roeck
>> Signed-off-by: Frank Rowand
>> ---
>>
>> changes since version 1:
>> - use pointer from kmalloc() for kfree() instead of using pointer that
>> has been modified for FDT alignment
>>
>> changes since version 2:
>> - version 1 was a work in progress version, I failed to commit the
>> following
>> final changes
>> - reorder first two arguments of of_overlay_apply()
>>
>> drivers/of/of_private.h | 2 ++
>> drivers/of/overlay.c| 28 +---
>> drivers/of/unittest.c | 12 +---
>> 3 files changed, 28 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
>> index d9e6a324de0a..d717efbd637d 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -8,6 +8,8 @@
>> * Copyright (C) 1996-2005 Paul Mackerras.
>> */
>>
>> +#define FDT_ALIGN_SIZE 8
>> +
>
> Use existing define ? Or was that local in libfdt ?
I don't see a define in libfdt. If anyone finds one,
I'll switch to it.
>
>> /**
>> * struct alias_prop - Alias property in 'aliases' node
>> * @link: List node to link the structure in aliases_lookup list
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 50bbe0edf538..cf770452e1e5 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -57,7 +57,7 @@ struct fragment {
>> * struct overlay_changeset
>> * @id: changeset identifier
>> * @ovcs_list: list on which we are located
>> - * @fdt:FDT that was unflattened to create @overlay_tree
>> + * @fdt:base of memory allocated to hold aligned FDT that was
>> unflattened to create @overlay_tree
>> * @overlay_tree: expanded device tree that contains the fragment nodes
>> * @count: count of fragment structures
>> * @fragments: fragment nodes in the overlay expanded device
>> tree
>> @@ -719,8 +719,8 @@ static struct device_node *find_target(struct
>> device_node *info_node)
>> /**
>> * init_overlay_changeset() - initialize overlay changeset from overlay tree
>> * @ovcs: Overlay changeset to build
>> - * @fdt:the FDT that was unflattened to create @tree
>> - * @tree: Contains all the overlay fragments and overlay fixup nodes
>> + * @fdt:base of memory allocated to hold aligned FDT that was
>> unflattened to create @tree
>> + * @tree: Contains the overlay fragments and overlay fixup nodes
>> *
>> * Initialize @ovcs. Populate @ovcs->fragments with node information from
>> * the top level of @tree. The relevant top level nodes are the fragment
>> @@ -873,7 +873,8 @@ static void free_overlay_changeset(struct
>> overlay_changeset *ovcs)
>> * internal documentation
>> *
>> * of_overlay_apply() - Create and apply an overlay changeset
>> - * @fdt:the FDT that was unflattened to create @tree
>> + * @fdt:base of memory allocated to hold *@fdt_align
>> + * @fdt_align: the FDT that was unflattened to create @tree, aligned
>> * @tree: Expanded overlay device tree
>> * @ovcs_id:Pointer to overlay changeset id
>> *
>> @@ -912,8 +913,8 @@ static void free_overlay_changeset(struct
>> overlay_changeset *ovcs)
>> * id is returned to *ovcs_id.
>> */
>>
>> -static int of_overlay_apply(const void *fdt, struct device_node *tree,
>> -int *ovcs_id)
>> +static int of_overlay_apply(const void *fdt, const void *fdt_align,
>
> fdt_align is not used in this function.
Thanks for catching that. Left over from earlier version where I
saved both aligned and unaligned addresses in init_overlay_changeset().
Will remove.
>
>> +
Re: [PATCH v2 1/1] of: unittest: overlay: ensure proper alignment of copied FDT
Hi Rob,
I had a git hiccup, this is not the version I meant to send. v3 coming shortly.
-Frank
On 4/8/21 9:43 AM, frowand.l...@gmail.com wrote:
> From: Frank Rowand
>
> The Devicetree standard specifies an 8 byte alignment of the FDT.
> Code in libfdt expects this alignment for an FDT image in memory.
> kmemdup() returns 4 byte alignment on openrisc. Replace kmemdup()
> with kmalloc(), align pointer, memcpy() to get proper alignment.
>
> The 4 byte alignment exposed a related bug which triggered a crash
> on openrisc with:
> commit 79edff12060f ("scripts/dtc: Update to upstream version
> v1.6.0-51-g183df9e9c2b9")
> as reported in:
> https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/
>
> Reported-by: Guenter Roeck
> Signed-off-by: Frank Rowand
>
> ---
>
> Please review carefully, I am not yet fully awake...
>
> changes since version 1:
> - use pointer from kmalloc() for kfree() instead of using pointer that
> has been modified for FDT alignment
>
> drivers/of/of_private.h | 2 ++
> drivers/of/overlay.c| 28 +---
> drivers/of/unittest.c | 12 +---
> 3 files changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index d9e6a324de0a..d717efbd637d 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -8,6 +8,8 @@
> * Copyright (C) 1996-2005 Paul Mackerras.
> */
>
> +#define FDT_ALIGN_SIZE 8
> +
> /**
> * struct alias_prop - Alias property in 'aliases' node
> * @link:List node to link the structure in aliases_lookup list
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 50bbe0edf538..e0397d70d531 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -57,7 +57,7 @@ struct fragment {
> * struct overlay_changeset
> * @id: changeset identifier
> * @ovcs_list: list on which we are located
> - * @fdt: FDT that was unflattened to create @overlay_tree
> + * @fdt: base of memory allocated to hold aligned FDT that was
> unflattened to create @overlay_tree
> * @overlay_tree:expanded device tree that contains the fragment nodes
> * @count: count of fragment structures
> * @fragments: fragment nodes in the overlay expanded device
> tree
> @@ -719,8 +719,8 @@ static struct device_node *find_target(struct device_node
> *info_node)
> /**
> * init_overlay_changeset() - initialize overlay changeset from overlay tree
> * @ovcs:Overlay changeset to build
> - * @fdt: the FDT that was unflattened to create @tree
> - * @tree:Contains all the overlay fragments and overlay fixup nodes
> + * @fdt: base of memory allocated to hold aligned FDT that was
> unflattened to create @tree
> + * @tree:Contains the overlay fragments and overlay fixup nodes
> *
> * Initialize @ovcs. Populate @ovcs->fragments with node information from
> * the top level of @tree. The relevant top level nodes are the fragment
> @@ -873,7 +873,8 @@ static void free_overlay_changeset(struct
> overlay_changeset *ovcs)
> * internal documentation
> *
> * of_overlay_apply() - Create and apply an overlay changeset
> - * @fdt: the FDT that was unflattened to create @tree
> + * @fdt_align: the FDT that was unflattened to create @tree, aligned
> + * @fdt: base of memory allocated to hold *@fdt_align
> * @tree:Expanded overlay device tree
> * @ovcs_id: Pointer to overlay changeset id
> *
> @@ -912,8 +913,8 @@ static void free_overlay_changeset(struct
> overlay_changeset *ovcs)
> * id is returned to *ovcs_id.
> */
>
> -static int of_overlay_apply(const void *fdt, struct device_node *tree,
> - int *ovcs_id)
> +static int of_overlay_apply(const void *fdt_align, const void *fdt,
> + struct device_node *tree, int *ovcs_id)
> {
> struct overlay_changeset *ovcs;
> int ret = 0, ret_revert, ret_tmp;
> @@ -953,7 +954,7 @@ static int of_overlay_apply(const void *fdt, struct
> device_node *tree,
> /*
>* after overlay_notify(), ovcs->overlay_tree related pointers may have
>* leaked to drivers, so can not kfree() tree, aka ovcs->overlay_tree;
> - * and can not free fdt, aka ovcs->fdt
> + * and can not free memory containing aligned fdt, aka ovcs->fdt
>*/
> ret = overlay_notify(ovcs, OF_OVERLAY_PRE_APPLY);
> if (ret) {
> @@ -1014,7 +1015,8 @@ static int of_overlay_apply(const void *fdt, struct
> device_node *tree,
> int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_
Re: [PATCH 1/1] of: unittest: overlay: ensure proper alignment of copied FDT
On 4/7/21 5:01 PM, Guenter Roeck wrote:
> On 4/7/21 1:59 PM, Frank Rowand wrote:
>> Hi Guenter,
>>
>> On 4/7/21 3:51 PM, frowand.l...@gmail.com wrote:
>>> From: Frank Rowand
>>>
>>> The Devicetree standard specifies an 8 byte alignment of the FDT.
>>> Code in libfdt expects this alignment for an FDT image in memory.
>>> kmemdup() returns 4 byte alignment on openrisc. Replace kmemdup()
>>> with kmalloc(), align pointer, memcpy() to get proper alignment.
>>>
>>> The 4 byte alignment exposed a related bug which triggered a crash
>>> on openrisc with:
>>> commit 79edff12060f ("scripts/dtc: Update to upstream version
>>> v1.6.0-51-g183df9e9c2b9")
>>> as reported in:
>>> https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/
>>
>> Can you please test this patch?
>>
>
> Sure, will do, after you fixed the problem pointed out by Rob.
>
> Sorry, I should have mentioned it - that problem was the reason
> why I didn't propose a fix myself.
No problem, I was aware of the issue but then spaced on actually
dealing with it.
- Space Cadet Frank
>
> Guenter
>
Re: [PATCH 1/1] of: unittest: overlay: ensure proper alignment of copied FDT
On 4/7/21 4:34 PM, Rob Herring wrote:
> On Wed, Apr 7, 2021 at 3:51 PM wrote:
>>
>> From: Frank Rowand
>>
>> The Devicetree standard specifies an 8 byte alignment of the FDT.
>> Code in libfdt expects this alignment for an FDT image in memory.
>> kmemdup() returns 4 byte alignment on openrisc. Replace kmemdup()
>> with kmalloc(), align pointer, memcpy() to get proper alignment.
>>
>> The 4 byte alignment exposed a related bug which triggered a crash
>> on openrisc with:
>> commit 79edff12060f ("scripts/dtc: Update to upstream version
>> v1.6.0-51-g183df9e9c2b9")
>> as reported in:
>> https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/
>>
>> Reported-by: Guenter Roeck
>> Signed-off-by: Frank Rowand
>> ---
>> drivers/of/of_private.h | 2 ++
>> drivers/of/overlay.c| 8 ++--
>> drivers/of/unittest.c | 9 +++--
>> 3 files changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
>> index d9e6a324de0a..d717efbd637d 100644
>> --- a/drivers/of/of_private.h
>> +++ b/drivers/of/of_private.h
>> @@ -8,6 +8,8 @@
>> * Copyright (C) 1996-2005 Paul Mackerras.
>> */
>>
>> +#define FDT_ALIGN_SIZE 8
>> +
>> /**
>> * struct alias_prop - Alias property in 'aliases' node
>> * @link: List node to link the structure in aliases_lookup list
>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
>> index 50bbe0edf538..8b40711ed202 100644
>> --- a/drivers/of/overlay.c
>> +++ b/drivers/of/overlay.c
>> @@ -1014,7 +1014,7 @@ static int of_overlay_apply(const void *fdt, struct
>> device_node *tree,
>> int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
>> int *ovcs_id)
>> {
>> - const void *new_fdt;
>> + void *new_fdt;
>> int ret;
>> u32 size;
>> struct device_node *overlay_root;
>> @@ -1036,10 +1036,14 @@ int of_overlay_fdt_apply(const void *overlay_fdt,
>> u32 overlay_fdt_size,
>> * Must create permanent copy of FDT because of_fdt_unflatten_tree()
>> * will create pointers to the passed in FDT in the unflattened tree.
>> */
>> - new_fdt = kmemdup(overlay_fdt, size, GFP_KERNEL);
>> + size += FDT_ALIGN_SIZE;
>> + new_fdt = kmalloc(size, GFP_KERNEL);
>> if (!new_fdt)
>> return -ENOMEM;
>>
>> + new_fdt = PTR_ALIGN(new_fdt, FDT_ALIGN_SIZE);
>> + memcpy(new_fdt, overlay_fdt, size);
>> +
>> of_fdt_unflatten_tree(new_fdt, NULL, _root);
>> if (!overlay_root) {
>> pr_err("unable to unflatten overlay_fdt\n");
>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>> index eb100627c186..edd6ce807691 100644
>> --- a/drivers/of/unittest.c
>> +++ b/drivers/of/unittest.c
>> @@ -22,6 +22,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include
>> #include
>> @@ -1415,7 +1416,7 @@ static int __init unittest_data_add(void)
>> */
>> extern uint8_t __dtb_testcases_begin[];
>> extern uint8_t __dtb_testcases_end[];
>> - const int size = __dtb_testcases_end - __dtb_testcases_begin;
>> + u32 size = __dtb_testcases_end - __dtb_testcases_begin;
>> int rc;
>>
>> if (!size) {
>> @@ -1425,10 +1426,14 @@ static int __init unittest_data_add(void)
>> }
>>
>> /* creating copy */
>> - unittest_data = kmemdup(__dtb_testcases_begin, size, GFP_KERNEL);
>> + size += FDT_ALIGN_SIZE;
>> + unittest_data = kmalloc(size, GFP_KERNEL);
>> if (!unittest_data)
>> return -ENOMEM;
>>
>> + unittest_data = PTR_ALIGN(unittest_data, FDT_ALIGN_SIZE);
>> + memcpy(unittest_data, __dtb_testcases_begin, size);
>> +
>> of_fdt_unflatten_tree(unittest_data, NULL, _data_node);
>> if (!unittest_data_node) {
>> pr_warn("%s: No tree to attach; not running tests\n",
>> __func__);
>
> The next line here is a kfree(unittest_data) which I assume will fail
> if the ptr address changed. Same issue in the overlay code.
Thanks for catching this.
>
> The error path is easy to fix. Freeing the memory later on, not so
> much...
The overlay subsystem retains ownership of the allocated memory and
responsibility for any subsequent kfree(), so actually not very
difficult.
New version of the patch should be out this morning.
-Frank
> One solution is always alloc a power of 2 size, that's
> guaranteed to be 'size' aligned:
>
> * The allocated object address is aligned to at least ARCH_KMALLOC_MINALIGN
> * bytes. For @size of power of two bytes, the alignment is also guaranteed
> * to be at least to the size.
>
> Rob
> .
>
Re: [PATCH 1/1] of: properly check for error returned by fdt_get_name()
On 4/6/21 3:30 PM, Frank Rowand wrote:
> On 4/6/21 2:21 PM, Rob Herring wrote:
>> On Sun, Apr 04, 2021 at 10:28:45PM -0500, frowand.l...@gmail.com wrote:
>>> From: Frank Rowand
>>>
>>> fdt_get_name() returns error values via a parameter pointer
>>> instead of in function return. Fix check for this error value
>>> in populate_node() and callers of populate_node().
>>>
>>> Chasing up the caller tree showed callers of various functions
>>> failing to initialize the value of pointer parameters that
>>> can return error values. Initialize those values to NULL.
>>>
>>> The bug was introduced by
>>> commit e6a6928c3ea1 ("of/fdt: Convert FDT functions to use libfdt")
>>> but this patch can not be backported directly to that commit
>>> because the relevant code has further been restructured by
>>> commit dfbd4c6eff35 ("drivers/of: Split unflatten_dt_node()")
>>>
>>> The bug became visible by triggering a crash on openrisc with:
>>> commit 79edff12060f ("scripts/dtc: Update to upstream version
>>> v1.6.0-51-g183df9e9c2b9")
>>> as reported in:
>>> https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/
>>>
>>> Fixes: commit 79edff12060f ("scripts/dtc: Update to upstream version
>>> v1.6.0-51-g183df9e9c2b9")
>>> Reported-by: Guenter Roeck
>>> Signed-off-by: Frank Rowand
>>>
>>> ---
>>>
>>> This patch papers over the unaligned pointer passed to
>>> of_fdt_unflatten_tree() bug that Guenter reported in
>>> https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/
>>>
>>> I will create a separate patch to fix that problem.
>
> Likely to be tomorrow (Wed 4/7).
>
> -Frank
>
>>
>> Got an ETA for that?
>>
>> Rob
>> .
>>
>
The patch to fix the alignment issue is:
https://lore.kernel.org/linux-devicetree/20210407205110.2173976-1-frowand.l...@gmail.com/
Hopefully it will pass testing by Guenter.
Sorry about the previous vertical dyslexia response... :-)
-Frank
Re: [PATCH 1/1] of: unittest: overlay: ensure proper alignment of copied FDT
Hi Guenter,
On 4/7/21 3:51 PM, frowand.l...@gmail.com wrote:
> From: Frank Rowand
>
> The Devicetree standard specifies an 8 byte alignment of the FDT.
> Code in libfdt expects this alignment for an FDT image in memory.
> kmemdup() returns 4 byte alignment on openrisc. Replace kmemdup()
> with kmalloc(), align pointer, memcpy() to get proper alignment.
>
> The 4 byte alignment exposed a related bug which triggered a crash
> on openrisc with:
> commit 79edff12060f ("scripts/dtc: Update to upstream version
> v1.6.0-51-g183df9e9c2b9")
> as reported in:
> https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/
Can you please test this patch?
The most complete coverage will be a config with:
OF_UNITTEST=y
OF_OVERLAY=y
OF_DYNAMIC=y
One path will be missed if you test with:
#OF_OVERLAY=n
#OF_DYNAMIC=n
Thanks!
-Frank
>
> Reported-by: Guenter Roeck
> Signed-off-by: Frank Rowand
> ---
> drivers/of/of_private.h | 2 ++
> drivers/of/overlay.c| 8 ++--
> drivers/of/unittest.c | 9 +++--
> 3 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index d9e6a324de0a..d717efbd637d 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -8,6 +8,8 @@
> * Copyright (C) 1996-2005 Paul Mackerras.
> */
>
> +#define FDT_ALIGN_SIZE 8
> +
> /**
> * struct alias_prop - Alias property in 'aliases' node
> * @link:List node to link the structure in aliases_lookup list
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 50bbe0edf538..8b40711ed202 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -1014,7 +1014,7 @@ static int of_overlay_apply(const void *fdt, struct
> device_node *tree,
> int of_overlay_fdt_apply(const void *overlay_fdt, u32 overlay_fdt_size,
>int *ovcs_id)
> {
> - const void *new_fdt;
> + void *new_fdt;
> int ret;
> u32 size;
> struct device_node *overlay_root;
> @@ -1036,10 +1036,14 @@ int of_overlay_fdt_apply(const void *overlay_fdt, u32
> overlay_fdt_size,
>* Must create permanent copy of FDT because of_fdt_unflatten_tree()
>* will create pointers to the passed in FDT in the unflattened tree.
>*/
> - new_fdt = kmemdup(overlay_fdt, size, GFP_KERNEL);
> + size += FDT_ALIGN_SIZE;
> + new_fdt = kmalloc(size, GFP_KERNEL);
> if (!new_fdt)
> return -ENOMEM;
>
> + new_fdt = PTR_ALIGN(new_fdt, FDT_ALIGN_SIZE);
> + memcpy(new_fdt, overlay_fdt, size);
> +
> of_fdt_unflatten_tree(new_fdt, NULL, _root);
> if (!overlay_root) {
> pr_err("unable to unflatten overlay_fdt\n");
> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> index eb100627c186..edd6ce807691 100644
> --- a/drivers/of/unittest.c
> +++ b/drivers/of/unittest.c
> @@ -22,6 +22,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -1415,7 +1416,7 @@ static int __init unittest_data_add(void)
>*/
> extern uint8_t __dtb_testcases_begin[];
> extern uint8_t __dtb_testcases_end[];
> - const int size = __dtb_testcases_end - __dtb_testcases_begin;
> + u32 size = __dtb_testcases_end - __dtb_testcases_begin;
> int rc;
>
> if (!size) {
> @@ -1425,10 +1426,14 @@ static int __init unittest_data_add(void)
> }
>
> /* creating copy */
> - unittest_data = kmemdup(__dtb_testcases_begin, size, GFP_KERNEL);
> + size += FDT_ALIGN_SIZE;
> + unittest_data = kmalloc(size, GFP_KERNEL);
> if (!unittest_data)
> return -ENOMEM;
>
> + unittest_data = PTR_ALIGN(unittest_data, FDT_ALIGN_SIZE);
> + memcpy(unittest_data, __dtb_testcases_begin, size);
> +
> of_fdt_unflatten_tree(unittest_data, NULL, _data_node);
> if (!unittest_data_node) {
> pr_warn("%s: No tree to attach; not running tests\n", __func__);
>
Re: [PATCH 1/1] of: properly check for error returned by fdt_get_name()
On 4/6/21 2:21 PM, Rob Herring wrote:
> On Sun, Apr 04, 2021 at 10:28:45PM -0500, frowand.l...@gmail.com wrote:
>> From: Frank Rowand
>>
>> fdt_get_name() returns error values via a parameter pointer
>> instead of in function return. Fix check for this error value
>> in populate_node() and callers of populate_node().
>>
>> Chasing up the caller tree showed callers of various functions
>> failing to initialize the value of pointer parameters that
>> can return error values. Initialize those values to NULL.
>>
>> The bug was introduced by
>> commit e6a6928c3ea1 ("of/fdt: Convert FDT functions to use libfdt")
>> but this patch can not be backported directly to that commit
>> because the relevant code has further been restructured by
>> commit dfbd4c6eff35 ("drivers/of: Split unflatten_dt_node()")
>>
>> The bug became visible by triggering a crash on openrisc with:
>> commit 79edff12060f ("scripts/dtc: Update to upstream version
>> v1.6.0-51-g183df9e9c2b9")
>> as reported in:
>> https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/
>>
>> Fixes: commit 79edff12060f ("scripts/dtc: Update to upstream version
>> v1.6.0-51-g183df9e9c2b9")
>> Reported-by: Guenter Roeck
>> Signed-off-by: Frank Rowand
>>
>> ---
>>
>> This patch papers over the unaligned pointer passed to
>> of_fdt_unflatten_tree() bug that Guenter reported in
>> https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/
>>
>> I will create a separate patch to fix that problem.
Likely to be tomorrow (Wed 4/7).
-Frank
>
> Got an ETA for that?
>
> Rob
> .
>
Re: Linux 5.12-rc6
Adding Rob, so that he will be in the loop. -Frank On 4/5/21 12:28 PM, Guenter Roeck wrote: > On 4/5/21 10:14 AM, Linus Torvalds wrote: >> On Mon, Apr 5, 2021 at 10:10 AM Guenter Roeck wrote: >>> >>> No change in test results since last week [..] >> >> Let's ping Frank for the alignment issue. If that promised patch >> isn't timely (and trivial), I really think that removing the alignment >> check is by now the way forward for that libftd failure. >> > > Frank sent a patch with a fix/workaround yesterday, and I added my > Tested-by: an hour or so ago. > > https://lore.kernel.org/patchwork/patch/1407418/ > > Hmm, my reply isn't there (yet). I'll monitor and resend if needed. > > Thanks, > Guenter >
Re: [PATCH 1/1] of: properly check for error returned by fdt_get_name()
Hi Guenter,
Can you please test this patch to see if it prevents the crash on
openrisc that you reported in
https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/
Just after start of unittest you should see a warning about
testcases.
Thanks,
Frank
On 4/4/21 10:28 PM, frowand.l...@gmail.com wrote:
> From: Frank Rowand
>
> fdt_get_name() returns error values via a parameter pointer
> instead of in function return. Fix check for this error value
> in populate_node() and callers of populate_node().
>
> Chasing up the caller tree showed callers of various functions
> failing to initialize the value of pointer parameters that
> can return error values. Initialize those values to NULL.
>
> The bug was introduced by
> commit e6a6928c3ea1 ("of/fdt: Convert FDT functions to use libfdt")
> but this patch can not be backported directly to that commit
> because the relevant code has further been restructured by
> commit dfbd4c6eff35 ("drivers/of: Split unflatten_dt_node()")
>
> The bug became visible by triggering a crash on openrisc with:
> commit 79edff12060f ("scripts/dtc: Update to upstream version
> v1.6.0-51-g183df9e9c2b9")
> as reported in:
> https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/
>
> Fixes: commit 79edff12060f ("scripts/dtc: Update to upstream version
> v1.6.0-51-g183df9e9c2b9")
> Reported-by: Guenter Roeck
> Signed-off-by: Frank Rowand
>
> ---
>
> This patch papers over the unaligned pointer passed to
> of_fdt_unflatten_tree() bug that Guenter reported in
> https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/
>
> I will create a separate patch to fix that problem.
>
> drivers/of/fdt.c | 36 +++-
> drivers/of/overlay.c | 2 +-
> drivers/of/unittest.c | 15 ++-
> 3 files changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index dcc1dd96911a..adb26aff481d 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -205,7 +205,7 @@ static void populate_properties(const void *blob,
> *pprev = NULL;
> }
>
> -static bool populate_node(const void *blob,
> +static int populate_node(const void *blob,
> int offset,
> void **mem,
> struct device_node *dad,
> @@ -214,24 +214,24 @@ static bool populate_node(const void *blob,
> {
> struct device_node *np;
> const char *pathp;
> - unsigned int l, allocl;
> + int len;
>
> - pathp = fdt_get_name(blob, offset, );
> + pathp = fdt_get_name(blob, offset, );
> if (!pathp) {
> *pnp = NULL;
> - return false;
> + return len;
> }
>
> - allocl = ++l;
> + len++;
>
> - np = unflatten_dt_alloc(mem, sizeof(struct device_node) + allocl,
> + np = unflatten_dt_alloc(mem, sizeof(struct device_node) + len,
> __alignof__(struct device_node));
> if (!dryrun) {
> char *fn;
> of_node_init(np);
> np->full_name = fn = ((char *)np) + sizeof(*np);
>
> - memcpy(fn, pathp, l);
> + memcpy(fn, pathp, len);
>
> if (dad != NULL) {
> np->parent = dad;
> @@ -295,6 +295,7 @@ static int unflatten_dt_nodes(const void *blob,
> struct device_node *nps[FDT_MAX_DEPTH];
> void *base = mem;
> bool dryrun = !base;
> + int ret;
>
> if (nodepp)
> *nodepp = NULL;
> @@ -322,9 +323,10 @@ static int unflatten_dt_nodes(const void *blob,
> !of_fdt_device_is_available(blob, offset))
> continue;
>
> - if (!populate_node(blob, offset, , nps[depth],
> -[depth+1], dryrun))
> - return mem - base;
> + ret = populate_node(blob, offset, , nps[depth],
> +[depth+1], dryrun);
> + if (ret < 0)
> + return ret;
>
> if (!dryrun && nodepp && !*nodepp)
> *nodepp = nps[depth+1];
> @@ -372,6 +374,10 @@ void *__unflatten_device_tree(const void *blob,
> {
> int size;
> void *mem;
> + int ret;
> +
> + if (mynodes)
> + *mynodes = NULL;
>
> pr_debug(" -> unflatten_device_tree()\n");
>
> @@ -392,7 +398,7 @@ void *__unflatten_device_tree(const void *blob,
>
> /* First pass, scan for size */
>
Re: [PATCH 1/1] of: unittest: rename overlay source files from .dts to .dtso
On 3/27/21 12:40 PM, Rob Herring wrote: > On Wed, Mar 24, 2021 at 05:37:13PM -0500, frowand.l...@gmail.com wrote: >> From: Frank Rowand >> >> Add Makefile rule to build .dtbo.o assembly file from overlay .dtso >> source file. >> >> Rename unittest .dts overlay source files to use .dtso suffix. > > I'm pretty lukewarm on .dtso... I was originally also, but I'm warming up to it. > >> >> Update Makefile to build .dtbo.o objects instead of .dtb.o from >> unittest overlay source files. >> >> Modify unitest.c to use .dtbo.o based symbols instead of .dtb.o >> >> Modify Makefile.lib %.dtbo rule to depend upon %.dtso instead of %.dts >> >> Documentation/devicetree/of_unittest.rst was already out of date. >> This commit would make it even more out of date. Delete the document >> instead of continuing the maintenance burden of keeping the document >> in sync with the source. > > This should be a separate change. It's also going to collide with my doc > improvements. I'll split it out. > > Improvements here would be better than just deleting. OK, I'll make the document more abstract so that code changes will be less likely to require document changes. -Frank
Re: [PATCH] of/fdt: Improve error checking
On 3/29/21 1:21 PM, Rob Herring wrote:
> On Sat, Mar 27, 2021 at 5:41 PM Guenter Roeck wrote:
>>
>> If an unaligned pointer is passed to of_fdt_unflatten_tree(),
>> populate_node() as called from unflatten_dt_nodes() will fail.
>> unflatten_dt_nodes() will return 0 and set *nodepp to NULL.
>> This is not expected to happen in __unflatten_device_tree(),
>> which then tries to write into the NULL pointer, causing a crash
>> on openrisc if CONFIG_OF_UNITTEST=y.
>>
>> ### dt-test ### start of unittest - you will see error messages
>> Unable to handle kernel NULL pointer dereference
>> at virtual address 0x0064
>>
>> Oops#:
>> CPU #: 0
>>PC: c03a25d4SR: 807fSP: c102dd50
>> GPR00: GPR01: c102dd50 GPR02: c102dd78 GPR03: c1704004
>> GPR04: GPR05: c102dc18 GPR06: c102ddc8 GPR07: c102dcf7
>> GPR08: 0001 GPR09: c03a25a0 GPR10: c102c000 GPR11: c16fd75c
>> GPR12: ffb7 GPR13: GPR14: 0008 GPR15:
>> GPR16: c16fd75c GPR17: 0064 GPR18: c1704004 GPR19: 0004
>> GPR20: GPR21: GPR22: c102ddc8 GPR23: 0018
>> GPR24: 0001 GPR25: 0010 GPR26: c16fd75c GPR27: 0008
>> GPR28: deadbeef GPR29: GPR30: c0720128 GPR31: 0006
>> RES: c16fd75c oGPR11:
>> Process swapper (pid: 1, stackpage=c1028ba0)
>>
>> Stack:
>> Call trace:
>> [<(ptrval)>] __unflatten_device_tree+0xe0/0x184
>> [<(ptrval)>] of_fdt_unflatten_tree+0x60/0x90
>> [<(ptrval)>] of_unittest+0xb4/0x3614
>> [<(ptrval)>] ? __kernfs_create_file+0x130/0x188
>> [<(ptrval)>] ? sysfs_add_file_mode_ns+0x13c/0x288
>> [<(ptrval)>] ? of_unittest+0x0/0x3614
>> [<(ptrval)>] ? lock_is_held_type+0x160/0x20c
>> [<(ptrval)>] ? of_unittest+0x0/0x3614
>> [<(ptrval)>] ? ignore_unknown_bootoption+0x0/0x24
>> [<(ptrval)>] do_one_initcall+0x98/0x340
>> [<(ptrval)>] ? parse_args+0x220/0x4e4
>> [<(ptrval)>] ? lock_is_held_type+0x160/0x20c
>> [<(ptrval)>] ? ignore_unknown_bootoption+0x0/0x24
>> [<(ptrval)>] ? rcu_read_lock_sched_held+0x34/0x88
>> [<(ptrval)>] kernel_init_freeable+0x1c0/0x240
>> [<(ptrval)>] ? ignore_unknown_bootoption+0x0/0x24
>> [<(ptrval)>] ? kernel_init+0x0/0x154
>> [<(ptrval)>] kernel_init+0x1c/0x154
>> [<(ptrval)>] ? calculate_sigpending+0x54/0x64
>> [<(ptrval)>] ret_from_fork+0x1c/0x150
>>
>> This problem affects all architectures with a 4-byte memory alignment.
>> Since commit 79edff12060f ("scripts/dtc: Update to upstream version
>> v1.6.0-51-g183df9e9c2b9"), devicetree code in the Linux kernel mandates
>> an 8-byte memory alignment of devicetree pointers, but it does not take
>> into account that functions such as kmalloc() or kmemdup() may not return
>> accordingly aligned pointers.
>
> AFAICT, openrisc would get:
>
> #define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long long)
>
> Is that not 8 bytes?
>
> Specifically, the problem is here is the unittest DT is copied with
> kmemdup(). I don't think there are other allocations which could be a
> problem.
>
>> To fix the immediate crash, check if *mynodes is NULL in
>> __unflatten_device_tree() before writing into it.
>>
>> Also affected by this problem is the code calling of_fdt_unflatten_tree().
>> That code checks for errors using the content of the mynodes pointer,
>> which is not set by the devicetree code if the alignment problem is
>> observed. Result is that the callers of of_fdt_unflatten_tree() check
>> if an uninitialized pointer is set to NULL. Preinitialize that pointer
>> to avoid the problem.
>>
>> With this code in place, devicetree code on openrisc (and any other
>
> "devicetree unittest code"
>
> The only other dtb copy is unflatten_and_copy_device_tree() which
> should be fine since it gives memblock the alignment requirement.
Plus overlays does at least one other copy.
I'll create a patch this week to properly align copies and to add some
of the checks suggested below based on the principal that a test should
not crash the kernel.
-Frank
>
>> architecture with 4-byte memory alignment) will still not work properly,
>> but at least it won't crash anymore. The resulting log message is
>>
>> ### dt-test ### start of unittest - you will see error messages
>> ### dt-test ### unittest_data_add: No tree to attach; not running tests
>>
>> when trying to run devicetree unittests.
>>
>> Fixes: 79edff12060f ("scripts/dtc: Update to upstream version
>> v1.6.0-51-g183df9e9c2b9")
>> Signed-off-by: Guenter Roeck
>> ---
>> drivers/of/fdt.c | 2 +-
>> drivers/of/overlay.c | 2 +-
>> drivers/of/unittest.c | 2 +-
>> 3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index dcc1dd96911a..ab95fdb4461d 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -415,7 +415,7 @@ void *__unflatten_device_tree(const void *blob,
>> pr_warn("End of tree marker overwritten: %08x\n",
>> be32_to_cpup(mem + size));
>>
>> - if (detached && mynodes) {
>> + if (detached &&
Re: Linux 5.12-rc5
On 3/29/21 1:41 PM, Rob Herring wrote: > n Mon, Mar 29, 2021 at 1:05 PM Linus Torvalds > wrote: >> >> On Sun, Mar 28, 2021 at 7:07 PM Guenter Roeck wrote: >>> >>> This is not really a new problem. I enabled devicetree unit tests >>> in the openrisc kernel and was rewarded with a crash. >>> https://lore.kernel.org/lkml/20210327224116.69309-1-li...@roeck-us.net/ >>> has all the glorious details. >> >> Hmm. >> >> I'm not sure I love that patch. >> >> I don't think the patch is _wrong_ per se, but if that "require 8 byte >> alignment" is a problem, then this seems to be papering over the issue >> rather than fixing it. >> >> So your patch protects from a NULL pointer dereference, but the >> underlying issue seems to be a regression, and the fix sounds like the >> kernel shouldn't be so strict about alignment requirements. > > In the interest of the DT unittests not panicking and halting boot, I > think we should handle NULL pointer. Agreed. > >> I guess we could make ARCH_SLAB_MINALIGN be at least 8 (perhaps only >> if the allocations is >= 8) but honestly, I don't think libfdt merits >> making such a big change. Small allocations are actually not uncommon >> in the kernel, and on 32-bit architectures I think 4-byte allocations >> are normal. >> >> So I'd be inclined to just remove the new >> >> /* The device tree must be at an 8-byte aligned address */ >> if ((uintptr_t)fdt & 7) >> return -FDT_ERR_ALIGNMENT; >> >> check in scripts/dtc/libfdt/fdt.c which I assume is the source of the >> problem. Rob? > > That is the source, but I'd rather not remove it as we try to avoid > any modifications from upstream. And we've found a couple of cases of > not following documented alignment requirements. Agreed to not remove. We can be properly aligned without changing kmemdup(). > >> Your patch to then avoid the NULL pointer dereference seems to be then >> an additional safety, but not fixing the actual regression. > > I think the right fix is not using kmemdup which copies the unittest dtb.A This is not the only place a kmemdup() is used by overlays. I'll create a patch this week to fix all of the kmemdup() locations and add the null pointer check. -Frank > > Rob >
Re: [PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
Hi Viresh, On 3/24/21 2:34 AM, Frank Rowand wrote: > On 3/16/21 12:42 AM, Viresh Kumar wrote: >> On 16-03-21, 00:36, Frank Rowand wrote: >>> I should have looked at patch 3/5 more carefully instead of counting on >>> Masahiro to check it out and simply build testing. >>> >>> Patch 3/5 does not seem correct. I'll look over all the makefile related >>> changes that have been accepted (if any) and patch 3/5 later today >>> (Tuesday). >> >> What is already merged by Rob is what everyone reviewed and it wasn't >> related to this .dtso/.dtbo discussion we are having. I will resend a >> new patchset with the stuff left for merging (which will involve the >> thing we are discussing here), so we can get a clear picture of it. >> > > Instead of doing what I suggested in my 16-03-21, 00:36 email (only > partly quoted above), I have made most of the changes to unittest.c > and drivers/of/unitest/unittest-data/Makefile to allow the unittest > .dts files to be .dtso source files and .dtbo FDT files, and tested. > One final step remaining in my work is to actually rename the *.dts > files to *.dtso. > > I hope to finish this later today (Wednesday) after getting some > sleep. I have submitted a patch that I _think_ removes the need for most of patch 3/5, _other_ than the yaml rule, which I would assume is still needed, though I did not examine it. My patch is https://lore.kernel.org/r/20210324223713.1334666-1-frowand.l...@gmail.com Can you do a new patch with just the yaml rule (if Rob accepts my patch). -Frank
Re: [PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
On 3/16/21 12:42 AM, Viresh Kumar wrote: > On 16-03-21, 00:36, Frank Rowand wrote: >> I should have looked at patch 3/5 more carefully instead of counting on >> Masahiro to check it out and simply build testing. >> >> Patch 3/5 does not seem correct. I'll look over all the makefile related >> changes that have been accepted (if any) and patch 3/5 later today (Tuesday). > > What is already merged by Rob is what everyone reviewed and it wasn't > related to this .dtso/.dtbo discussion we are having. I will resend a > new patchset with the stuff left for merging (which will involve the > thing we are discussing here), so we can get a clear picture of it. > Instead of doing what I suggested in my 16-03-21, 00:36 email (only partly quoted above), I have made most of the changes to unittest.c and drivers/of/unitest/unittest-data/Makefile to allow the unittest .dts files to be .dtso source files and .dtbo FDT files, and tested. One final step remaining in my work is to actually rename the *.dts files to *.dtso. I hope to finish this later today (Wednesday) after getting some sleep. -Frank
Re: [PATCH] of: overlay: fix for_each_child.cocci warnings
On 3/22/21 1:21 PM, Julia Lawall wrote:
> From: kernel test robot
>
> Function "for_each_child_of_node" should have of_node_put() before goto.
>
> Generated by: scripts/coccinelle/iterators/for_each_child.cocci
>
> Fixes: 82c2d81361ec ("coccinelle: iterators: Add for_each_child.cocci script")
> CC: Sumera Priyadarsini
> Reported-by: kernel test robot
> Signed-off-by: kernel test robot
> Signed-off-by: Julia Lawall
> ---
>
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> master
> head: 812da4d39463a060738008a46cfc9f775e4bfcf6
> commit: 82c2d81361ecd142a54e84a9da1e287113314a4f coccinelle: iterators: Add
> for_each_child.cocci script
> :: branch date: 13 hours ago
> :: commit date: 5 months ago
>
> overlay.c |1 +
> 1 file changed, 1 insertion(+)
>
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -796,6 +796,7 @@ static int init_overlay_changeset(struct
> if (!fragment->target) {
> of_node_put(fragment->overlay);
> ret = -EINVAL;
> + of_node_put(node);
> goto err_free_fragments;
> }
>
Reviewed-by: Frank Rowand
Tested-by: Frank Rowand
While reading through the code touched by the patch I noticed that
the clean up at label err_free_fragments does not do the required
of_node_put() calls. I'll add creating a patch to fix that to my
todo list.
-Frank
Re: [PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
On 3/15/21 5:12 PM, Laurent Pinchart wrote: > Hi Yamada-san, > > On Tue, Mar 16, 2021 at 02:43:45AM +0900, Masahiro Yamada wrote: >> On Mon, Mar 15, 2021 at 3:40 PM Viresh Kumar wrote: >>> On 14-03-21, 20:16, Frank Rowand wrote: >>>> On 3/12/21 11:11 PM, Frank Rowand wrote: >>>>> On 3/12/21 1:13 AM, Viresh Kumar wrote: >>>>>> On 12-03-21, 01:09, Frank Rowand wrote: >>>>>>> I suggested having the .dtso files include the .dts file because that >>>>>>> is a relatively >>>>>>> small and easy change to test. What would probably make more sense is >>>>>>> the rename >>>>>>> the existing overlay .dts files to be .dtso files and then for each >>>>>>> overlay .dtso >>>>>>> file create a new .dts file that #includes the corresponding .dtso >>>>>>> file. This is >>>>>>> more work and churn, but easier to document that the .dts files are a >>>>>>> hack that is >>>>>>> needed so that the corresponding .dtb.S files will be generated. >>>>>> >>>>>> What about creating links instead then ? >>>>>> >>>>> >>>>> I don't really like the idea of using links here. >>>>> >>>>> Maybe it is best to make the changes needed to allow the unittest >>>>> overlays to be .dtso instead of .dts. >>>>> >>>>> Off the top of my head: >>>>> >>>>> scripts/Makefile.lib: >>>>> The rule for %.dtb.S invokes cmd_dt_S_dtb, which puts the >>>>> overlay data in section .dtb.init.rodata, with a label >>>>> pointing to the beginning of the overlay __dtb_XXX_begin and >>>>> a label pointing to the end of the overlay __dtb_XXX_end, >>>>> for the overlay named XXX. I _think_ that you could simply >>>>> add a corresponding rule for %.dtbo.S using a new command >>>>> cmd_dt_S_dtbo (the same as cmd_dt_S_dtb, except use labels >>>>> __dtbo_XXX_begin and __dtbo_XXX_end). >>>> >>>> If you do the above, please put it in drivers/of/unittest-data/Makefile >>>> instead of scripts/Makefile.lib because it is unittest.c specific and >>>> not meant to be anywhere else in the kernel. >>> >>> What about doing this then in unittest's Makefile instead (which I >>> already suggested earlier), that will make everything work just fine >>> without any other changes ? >>> >>> +# Required for of unittest files as they can't be renamed to .dtso >>> +$(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE >>> + $(call if_changed_dep,dtc) >>> >> >> If those rules are only needed by drivers/of/unittest-data/Makefile, >> they should not be located in scripts/Makefile.lib. >> >> But how can we fix drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a779*.dts >> if these are doing bad things. >> They seem to be overlay files even though the file name suffix is .dts > > That is correct, they are overlays. I have no issue with those files > being renamed to .dtso if that can help (but I haven't checked if that > would have any adverse effect on the R-Car DU driver). As Laurent replied, yes these are overlays. They were grandfathered in as a deprecated use of overlays. > > These files are there to ensure backward compatibility with older DT > bindings. The change was made 3 years ago and I wouldn't object to > dropping this completely, but I understand I may not be the most > cautious person when it comes to ensuring DT backward compatibility :-) My memory is that the goal was to eventually remove these overlays at some point in the future. If everyone agrees that today is the proper time, it would be helpful to go ahead and remove these .dts files and the code that uses them. -Frank > >> $ find drivers -name '*.dts' >> drivers/staging/pi433/Documentation/devicetree/pi433-overlay.dts >> drivers/staging/mt7621-dts/gbpc2.dts >> drivers/staging/mt7621-dts/gbpc1.dts >> drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts >> drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts >> drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts >> drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts >> drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts >> drivers/of/unittest-data/overlay_1.dts >> drivers/of/unittest-data/testcases.dts >> drivers/of/unittest-data/overlay_bad_add_dup_node.dts >
Re: [PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
On 3/15/21 1:40 AM, Viresh Kumar wrote: > On 14-03-21, 20:16, Frank Rowand wrote: >> On 3/12/21 11:11 PM, Frank Rowand wrote: >>> On 3/12/21 1:13 AM, Viresh Kumar wrote: >>>> On 12-03-21, 01:09, Frank Rowand wrote: >>>>> I suggested having the .dtso files include the .dts file because that is >>>>> a relatively >>>>> small and easy change to test. What would probably make more sense is >>>>> the rename >>>>> the existing overlay .dts files to be .dtso files and then for each >>>>> overlay .dtso >>>>> file create a new .dts file that #includes the corresponding .dtso file. >>>>> This is >>>>> more work and churn, but easier to document that the .dts files are a >>>>> hack that is >>>>> needed so that the corresponding .dtb.S files will be generated. >>>> >>>> What about creating links instead then ? >>>> >>> >>> I don't really like the idea of using links here. >>> >>> Maybe it is best to make the changes needed to allow the unittest >>> overlays to be .dtso instead of .dts. >>> >>> Off the top of my head: >>> >>> scripts/Makefile.lib: >>> The rule for %.dtb.S invokes cmd_dt_S_dtb, which puts the >>> overlay data in section .dtb.init.rodata, with a label >>> pointing to the beginning of the overlay __dtb_XXX_begin and >>> a label pointing to the end of the overlay __dtb_XXX_end, >>> for the overlay named XXX. I _think_ that you could simply >>> add a corresponding rule for %.dtbo.S using a new command >>> cmd_dt_S_dtbo (the same as cmd_dt_S_dtb, except use labels >>> __dtbo_XXX_begin and __dtbo_XXX_end). >> >> If you do the above, please put it in drivers/of/unittest-data/Makefile >> instead of scripts/Makefile.lib because it is unittest.c specific and >> not meant to be anywhere else in the kernel. > > What about doing this then in unittest's Makefile instead (which I > already suggested earlier), that will make everything work just fine > without any other changes ? > > +# Required for of unittest files as they can't be renamed to .dtso > +$(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE > + $(call if_changed_dep,dtc) > I should have looked at patch 3/5 more carefully instead of counting on Masahiro to check it out and simply build testing. Patch 3/5 does not seem correct. I'll look over all the makefile related changes that have been accepted (if any) and patch 3/5 later today (Tuesday). -Frank
Re: [PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
Hi VIresh, On 3/12/21 11:11 PM, Frank Rowand wrote: > On 3/12/21 1:13 AM, Viresh Kumar wrote: >> On 12-03-21, 01:09, Frank Rowand wrote: >>> I suggested having the .dtso files include the .dts file because that is a >>> relatively >>> small and easy change to test. What would probably make more sense is the >>> rename >>> the existing overlay .dts files to be .dtso files and then for each overlay >>> .dtso >>> file create a new .dts file that #includes the corresponding .dtso file. >>> This is >>> more work and churn, but easier to document that the .dts files are a hack >>> that is >>> needed so that the corresponding .dtb.S files will be generated. >> >> What about creating links instead then ? >> > > I don't really like the idea of using links here. > > Maybe it is best to make the changes needed to allow the unittest > overlays to be .dtso instead of .dts. > > Off the top of my head: > > scripts/Makefile.lib: > The rule for %.dtb.S invokes cmd_dt_S_dtb, which puts the > overlay data in section .dtb.init.rodata, with a label > pointing to the beginning of the overlay __dtb_XXX_begin and > a label pointing to the end of the overlay __dtb_XXX_end, > for the overlay named XXX. I _think_ that you could simply > add a corresponding rule for %.dtbo.S using a new command > cmd_dt_S_dtbo (the same as cmd_dt_S_dtb, except use labels > __dtbo_XXX_begin and __dtbo_XXX_end). If you do the above, please put it in drivers/of/unittest-data/Makefile instead of scripts/Makefile.lib because it is unittest.c specific and not meant to be anywhere else in the kernel. -Frank > > drivers/of/unittest.o: > would need to have the #define of OVERLAY_INFO() changed to > reflect the changed label names (use __dtbo_##overlayname##begin > and __dtb_##overlay_name##_end). > > drivers/of/unittest-data/Makefile: > In obj-$(CONFIG_OF_OVERLAY) change the *.dtb.o names to *.dtbo.o > > I'm not sure how the DTC_FLAGS_... += -@ differentiates between > .dts / .dtb and .dtso / .dtbo That is worth looking at. > > -Frank >
Re: [PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
On 3/12/21 1:13 AM, Viresh Kumar wrote: > On 12-03-21, 01:09, Frank Rowand wrote: >> I suggested having the .dtso files include the .dts file because that is a >> relatively >> small and easy change to test. What would probably make more sense is the >> rename >> the existing overlay .dts files to be .dtso files and then for each overlay >> .dtso >> file create a new .dts file that #includes the corresponding .dtso file. >> This is >> more work and churn, but easier to document that the .dts files are a hack >> that is >> needed so that the corresponding .dtb.S files will be generated. > > What about creating links instead then ? > I don't really like the idea of using links here. Maybe it is best to make the changes needed to allow the unittest overlays to be .dtso instead of .dts. Off the top of my head: scripts/Makefile.lib: The rule for %.dtb.S invokes cmd_dt_S_dtb, which puts the overlay data in section .dtb.init.rodata, with a label pointing to the beginning of the overlay __dtb_XXX_begin and a label pointing to the end of the overlay __dtb_XXX_end, for the overlay named XXX. I _think_ that you could simply add a corresponding rule for %.dtbo.S using a new command cmd_dt_S_dtbo (the same as cmd_dt_S_dtb, except use labels __dtbo_XXX_begin and __dtbo_XXX_end). drivers/of/unittest.o: would need to have the #define of OVERLAY_INFO() changed to reflect the changed label names (use __dtbo_##overlayname##begin and __dtb_##overlay_name##_end). drivers/of/unittest-data/Makefile: In obj-$(CONFIG_OF_OVERLAY) change the *.dtb.o names to *.dtbo.o I'm not sure how the DTC_FLAGS_... += -@ differentiates between .dts / .dtb and .dtso / .dtbo That is worth looking at. -Frank
Re: [PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
On 3/12/21 1:03 AM, Frank Rowand wrote: > Hi Viresh, > > On 3/11/21 10:47 PM, Viresh Kumar wrote: >> On 10-03-21, 20:24, Masahiro Yamada wrote: >>> Even without "-I dts", >>> >>>inform = guess_input_format(arg, "dts"); >>> >>> seems to fall back to "dts" anyway, >>> but I guess you wanted to make this explicit, correct? >> >> >>>> +# Required for of unit-test files as they can't be renamed to .dtso >>> >>> If you go with *.dtso, I think you will rename >>> all *.dts under the drivers/ directory. >>> >>> What is blocking you from making this consistent? >> >> What about this patch instead ? This localizes the dts->dtbo hack to >> unitest's Makefile at least. > > It is late here, so I am not going to take the time to actually try what > I am going to suggest. I apologize in advance if I send you off on a > wild goose chase. > > Would it work to create a .dtso file for each of the unittest overlay .dts > files, where the .dtso would simply #include the .dts file. Then the > corresponding > .dtbo files could be added to the obj-$(CONFIG_OF_OVERLAY) list. I suggested having the .dtso files include the .dts file because that is a relatively small and easy change to test. What would probably make more sense is the rename the existing overlay .dts files to be .dtso files and then for each overlay .dtso file create a new .dts file that #includes the corresponding .dtso file. This is more work and churn, but easier to document that the .dts files are a hack that is needed so that the corresponding .dtb.S files will be generated. If it works, I am fine with either approach. -Frank > > I would like to avoid having the unitest-data/Makefile have different rules to > build objects because then the normal build rule is not being tested. > > -Frank > >> >> diff --git a/drivers/of/unittest-data/Makefile >> b/drivers/of/unittest-data/Makefile >> index a5d2d9254b2c..9f3426ec3fab 100644 >> --- a/drivers/of/unittest-data/Makefile >> +++ b/drivers/of/unittest-data/Makefile >> @@ -86,3 +86,7 @@ static_test_1-dtbs := static_base_1.dtb >> $(apply_static_overlay_1) >> static_test_2-dtbs := static_base_2.dtb $(apply_static_overlay_2) >> >> dtb-$(CONFIG_OF_OVERLAY) += static_test_1.dtb static_test_2.dtb >> + >> +# Required for of unittest files as they can't be renamed to .dtso >> +$(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE >> + $(call if_changed_dep,dtc) >> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib >> index bc045a54a34e..77a9be055e51 100644 >> --- a/scripts/Makefile.lib >> +++ b/scripts/Makefile.lib >> @@ -347,7 +347,7 @@ cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x >> assembler-with-cpp -o $(dtc-tmp) $< ; >> $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE >> $(call if_changed_dep,dtc) >> >> -$(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE >> +$(obj)/%.dtbo: $(src)/%.dtso $(DTC) FORCE >> $(call if_changed_dep,dtc) >> >> overlay-y := $(addprefix $(obj)/, $(overlay-y)) >> @@ -375,6 +375,9 @@ endef >> $(obj)/%.dt.yaml: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE >> $(call if_changed_rule,dtc,yaml) >> >> +$(obj)/%.dt.yaml: $(src)/%.dtso $(DTC) $(DT_TMP_SCHEMA) FORCE >> + $(call if_changed_rule,dtc,yaml) >> + >> dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp) >> >> # Bzip2 >> >
Re: [PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
Hi Viresh, On 3/11/21 10:47 PM, Viresh Kumar wrote: > On 10-03-21, 20:24, Masahiro Yamada wrote: >> Even without "-I dts", >> >>inform = guess_input_format(arg, "dts"); >> >> seems to fall back to "dts" anyway, >> but I guess you wanted to make this explicit, correct? > > >>> +# Required for of unit-test files as they can't be renamed to .dtso >> >> If you go with *.dtso, I think you will rename >> all *.dts under the drivers/ directory. >> >> What is blocking you from making this consistent? > > What about this patch instead ? This localizes the dts->dtbo hack to > unitest's Makefile at least. It is late here, so I am not going to take the time to actually try what I am going to suggest. I apologize in advance if I send you off on a wild goose chase. Would it work to create a .dtso file for each of the unittest overlay .dts files, where the .dtso would simply #include the .dts file. Then the corresponding .dtbo files could be added to the obj-$(CONFIG_OF_OVERLAY) list. I would like to avoid having the unitest-data/Makefile have different rules to build objects because then the normal build rule is not being tested. -Frank > > diff --git a/drivers/of/unittest-data/Makefile > b/drivers/of/unittest-data/Makefile > index a5d2d9254b2c..9f3426ec3fab 100644 > --- a/drivers/of/unittest-data/Makefile > +++ b/drivers/of/unittest-data/Makefile > @@ -86,3 +86,7 @@ static_test_1-dtbs := static_base_1.dtb > $(apply_static_overlay_1) > static_test_2-dtbs := static_base_2.dtb $(apply_static_overlay_2) > > dtb-$(CONFIG_OF_OVERLAY) += static_test_1.dtb static_test_2.dtb > + > +# Required for of unittest files as they can't be renamed to .dtso > +$(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE > + $(call if_changed_dep,dtc) > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib > index bc045a54a34e..77a9be055e51 100644 > --- a/scripts/Makefile.lib > +++ b/scripts/Makefile.lib > @@ -347,7 +347,7 @@ cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x > assembler-with-cpp -o $(dtc-tmp) $< ; > $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE > $(call if_changed_dep,dtc) > > -$(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE > +$(obj)/%.dtbo: $(src)/%.dtso $(DTC) FORCE > $(call if_changed_dep,dtc) > > overlay-y := $(addprefix $(obj)/, $(overlay-y)) > @@ -375,6 +375,9 @@ endef > $(obj)/%.dt.yaml: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE > $(call if_changed_rule,dtc,yaml) > > +$(obj)/%.dt.yaml: $(src)/%.dtso $(DTC) $(DT_TMP_SCHEMA) FORCE > + $(call if_changed_rule,dtc,yaml) > + > dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp) > > # Bzip2 >
Re: [PATCH V11 0/5] dt: Add fdtoverlay rule and statically build unittest
On 3/11/21 10:31 PM, Viresh Kumar wrote: > On 11-03-21, 17:27, Frank Rowand wrote: >> On 3/9/21 11:35 PM, Viresh Kumar wrote: >>> Viresh Kumar (4): >>> kbuild: Simplify builds with CONFIG_OF_ALL_DTBS >>> kbuild: Allow .dtso format for overlay source files >>> of: unittest: Create overlay_common.dtsi and testcases_common.dtsi >>> of: unittest: Statically apply overlays using fdtoverlay >>> >>> drivers/of/unittest-data/Makefile | 48 ++ >>> drivers/of/unittest-data/overlay_base.dts | 90 +- >>> drivers/of/unittest-data/overlay_common.dtsi | 91 +++ >>> drivers/of/unittest-data/static_base_1.dts| 4 + >>> drivers/of/unittest-data/static_base_2.dts| 4 + >>> drivers/of/unittest-data/testcases.dts| 23 ++--- >>> .../of/unittest-data/testcases_common.dtsi| 19 >>> .../of/unittest-data/tests-interrupts.dtsi| 11 +-- >>> scripts/Makefile.lib | 40 ++-- >>> 9 files changed, 218 insertions(+), 112 deletions(-) >>> create mode 100644 drivers/of/unittest-data/overlay_common.dtsi >>> create mode 100644 drivers/of/unittest-data/static_base_1.dts >>> create mode 100644 drivers/of/unittest-data/static_base_2.dts >>> create mode 100644 drivers/of/unittest-data/testcases_common.dtsi >>> >>> >>> base-commit: a38fd8748464831584a19438cbb3082b5a2dab15 >>> >> >> Does not apply to 5.12-rc2 > > I was based right over the 5.12-rc2 tag. I don't know why I failed. Given your report, I went back to v5.12-rc2 and the patch series applies fine. Either way, my tags are ok. > >> because of a dependency on a patch to >> scripts/Makefile.lib. That patch has been merged by Linus >> somewhere between -rc2 and -rc3. > > git log --oneline v5.12-rc2..origin/master -- scripts/Makefile.lib > > gives no results to me. > >> I had a working version >> between -rc2 and -rc3 at commit e6f197677b2e > > I have tried both Linus' tree and linux-next, and I don't see this > commit. Sorry about that, the commit id I gave was after applying the patch series. The commit id I should have reported is 144c79ef3353. -Frank > >> that does have >> the required patch, so that is the version I used to test >> this series. >> >> There is still confusion caused by the contortions that unittest >> goes through to mis-use base DTBs vs overlay DTBs, so _after_ >> this series is merged by Rob, I will poke around and see if >> I can change unittest so that it does not look like it is >> mis-using DTBs and overlay DTBs. >> >> >> Reviewed-by: Frank Rowand >> Tested-by: Frank Rowand > > Thanks. >
Re: [PATCH V11 0/5] dt: Add fdtoverlay rule and statically build unittest
On 3/9/21 11:35 PM, Viresh Kumar wrote: > Hi, > > This patchset adds a generic rule for applying overlays using fdtoverlay > tool and then updates unittests to get built statically using the same. > > V10->V11: > - Update patch 4/5 to fix checkpatch warning on spaces and tabs. > - Added Acked-by from Masahiro for patch 2/5. > > V9->V10: > - Add a new patch to allow .dtso files. > - Update 2/5 to be more efficient and also generate symbols for base > files automatically. > - No need to add lines like DTC_FLAGS_foo_base += -@ in patch 5/5. > - Add Ack by Masahiro for 1/5. > > V8->V9: > - Added some comment in patch 3/4 based on Frank's suggestions. > > V7->V8: > - Patch 1 is new. > - Platforms need to use dtb-y += foo.dtb instead of overlay-y += > foo.dtb. > - Use multi_depend instead of .SECONDEXPANSION. > - Use dtb-y for unittest instead of overlay-y. > - Rename the commented dtb filess in unittest Makefile as .dtbo. > - Improved Makefile code (I am learning a lot every day :) > > V6->V7: > - Dropped the first 4 patches, already merged. > - Patch 1/3 is new, suggested by Rob and slightly modified by me. > - Adapt Patch 3/3 to the new rule and name the overlay dtbs as .dtbo. > > -- > Viresh > > Rob Herring (1): > kbuild: Add generic rule to apply fdtoverlay > > Viresh Kumar (4): > kbuild: Simplify builds with CONFIG_OF_ALL_DTBS > kbuild: Allow .dtso format for overlay source files > of: unittest: Create overlay_common.dtsi and testcases_common.dtsi > of: unittest: Statically apply overlays using fdtoverlay > > drivers/of/unittest-data/Makefile | 48 ++ > drivers/of/unittest-data/overlay_base.dts | 90 +- > drivers/of/unittest-data/overlay_common.dtsi | 91 +++ > drivers/of/unittest-data/static_base_1.dts| 4 + > drivers/of/unittest-data/static_base_2.dts| 4 + > drivers/of/unittest-data/testcases.dts| 23 ++--- > .../of/unittest-data/testcases_common.dtsi| 19 > .../of/unittest-data/tests-interrupts.dtsi| 11 +-- > scripts/Makefile.lib | 40 ++-- > 9 files changed, 218 insertions(+), 112 deletions(-) > create mode 100644 drivers/of/unittest-data/overlay_common.dtsi > create mode 100644 drivers/of/unittest-data/static_base_1.dts > create mode 100644 drivers/of/unittest-data/static_base_2.dts > create mode 100644 drivers/of/unittest-data/testcases_common.dtsi > > > base-commit: a38fd8748464831584a19438cbb3082b5a2dab15 > Does not apply to 5.12-rc2 because of a dependency on a patch to scripts/Makefile.lib. That patch has been merged by Linus somewhere between -rc2 and -rc3. I had a working version between -rc2 and -rc3 at commit e6f197677b2e that does have the required patch, so that is the version I used to test this series. There is still confusion caused by the contortions that unittest goes through to mis-use base DTBs vs overlay DTBs, so _after_ this series is merged by Rob, I will poke around and see if I can change unittest so that it does not look like it is mis-using DTBs and overlay DTBs. Reviewed-by: Frank Rowand Tested-by: Frank Rowand
Re: [PATCH V11 3/5] kbuild: Allow .dtso format for overlay source files
On 3/10/21 9:15 AM, Masahiro Yamada wrote: > On Wed, Mar 10, 2021 at 11:47 PM Viresh Kumar wrote: >> >> On 10-03-21, 20:24, Masahiro Yamada wrote: >>> On Wed, Mar 10, 2021 at 2:35 PM Viresh Kumar >>> wrote: diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib index bc045a54a34e..59e86f67f9e0 100644 --- a/scripts/Makefile.lib +++ b/scripts/Makefile.lib @@ -339,7 +339,7 @@ $(obj)/%.dtb.S: $(obj)/%.dtb FORCE quiet_cmd_dtc = DTC $@ cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $< ; \ - $(DTC) -O $(patsubst .%,%,$(suffix $@)) -o $@ -b 0 \ + $(DTC) -I dts -O $(patsubst .%,%,$(suffix $@)) -o $@ -b 0 \ >>> >>> Even without "-I dts", >>> >>>inform = guess_input_format(arg, "dts"); >>> >>> seems to fall back to "dts" anyway, >> >> I missed this TBH. >> >>> but I guess you wanted to make this explicit, correct? >> >> That can be a reason now :) >> >>> I will drop the ugly -O. >>> https://patchwork.kernel.org/project/linux-kbuild/patch/20210310110824.782209-1-masahi...@kernel.org/ >> >> But if we are going to depend on DTC to guess it right, then we >> shouldn't add -I at all.. >> >>> I will queue it to linux-kbuild/fixes. >>> >>> >>> $(addprefix -i,$(dir $<) $(DTC_INCLUDE)) $(DTC_FLAGS) \ -d $(depfile).dtc.tmp $(dtc-tmp) ; \ cat $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile) @@ -347,9 +347,13 @@ cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $< ; $(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE $(call if_changed_dep,dtc) +# Required for of unit-test files as they can't be renamed to .dtso >>> >>> If you go with *.dtso, I think you will rename >>> all *.dts under the drivers/ directory. >>> >>> What is blocking you from making this consistent? >> >> The unit-test dts files are designed differently (we have had lots of >> discussion between Frank and David on that) and they aren't purely >> overlay or base files. They are designed to do some tricky testing and >> renaming them to .dtso won't be right, we are just reusing them to do >> static (build time) testing as well. > > > I still do not understand. > > If they are not overlay files, why > do you need to have them suffixed with .dtbo? > > ".dts -> .dtb" should be enough. > > Why do you need to do ".dts -> .dtbo" ? > > > > >> I think it would be better if we can drop the existing %.dtbo rule >> here (i.e. dtbo from .dts) and do some magic in unit-test's Makefile, >> so it is localised at least instead of it here. >> >> Any ideas for that ? > > I do not know. > > My impression is you are doing something fishy. That is accurate. Devicetree unittest plays some tricks to enable testing to occur. These tricks will never be used anywhere else in the kernel. -Frank
Re: [PATCH V8 3/4] of: unittest: Create overlay_common.dtsi and testcases_common.dtsi
Hi Viresh,
I commented on this patch in v7 after you had created v8. You acknowledged
that comment and said that it will be corrected.
-Frank
On 2/12/21 5:18 AM, Viresh Kumar wrote:
> In order to build-test the same unit-test files using fdtoverlay tool,
> move the device nodes from the existing overlay_base.dts and
> testcases_common.dts files to .dtsi counterparts. The .dts files now
> include the new .dtsi files, resulting in exactly the same behavior as
> earlier.
>
> The .dtsi files can now be reused for compile time tests using
> fdtoverlay (will be done by a later commit).
>
> This is required because the base files passed to fdtoverlay tool
> shouldn't be overlays themselves (i.e. shouldn't have the /plugin/;
> tag).
>
> Note that this commit also moves "testcase-device2" node to
> testcases.dts from tests-interrupts.dtsi, as this node has a deliberate
> error in it and is only relevant for runtime testing done with
> unittest.c.
>
> Signed-off-by: Viresh Kumar
> ---
> drivers/of/unittest-data/overlay_base.dts | 90 +-
> drivers/of/unittest-data/overlay_common.dtsi | 91 +++
> drivers/of/unittest-data/testcases.dts| 18 ++--
> .../of/unittest-data/testcases_common.dtsi| 19
> .../of/unittest-data/tests-interrupts.dtsi| 7 --
> 5 files changed, 118 insertions(+), 107 deletions(-)
> create mode 100644 drivers/of/unittest-data/overlay_common.dtsi
> create mode 100644 drivers/of/unittest-data/testcases_common.dtsi
>
> diff --git a/drivers/of/unittest-data/overlay_base.dts
> b/drivers/of/unittest-data/overlay_base.dts
> index 99ab9d12d00b..ab9014589c5d 100644
> --- a/drivers/of/unittest-data/overlay_base.dts
> +++ b/drivers/of/unittest-data/overlay_base.dts
> @@ -2,92 +2,4 @@
> /dts-v1/;
> /plugin/;
>
> -/*
> - * Base device tree that overlays will be applied against.
> - *
> - * Do not add any properties in node "/".
> - * Do not add any nodes other than "/testcase-data-2" in node "/".
> - * Do not add anything that would result in dtc creating node "/__fixups__".
> - * dtc will create nodes "/__symbols__" and "/__local_fixups__".
> - */
> -
> -/ {
> - testcase-data-2 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> -
> - electric_1: substation@100 {
> - compatible = "ot,big-volts-control";
> - reg = < 0x0100 0x100 >;
> - status = "disabled";
> -
> - hvac_1: hvac-medium-1 {
> - compatible = "ot,hvac-medium";
> - heat-range = < 50 75 >;
> - cool-range = < 60 80 >;
> - };
> -
> - spin_ctrl_1: motor-1 {
> - compatible = "ot,ferris-wheel-motor";
> - spin = "clockwise";
> - rpm_avail = < 50 >;
> - };
> -
> - spin_ctrl_2: motor-8 {
> - compatible = "ot,roller-coaster-motor";
> - };
> - };
> -
> - rides_1: fairway-1 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> - compatible = "ot,rides";
> - status = "disabled";
> - orientation = < 127 >;
> -
> - ride@100 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> - compatible = "ot,roller-coaster";
> - reg = < 0x0100 0x100 >;
> - hvac-provider = < _1 >;
> - hvac-thermostat = < 29 > ;
> - hvac-zones = < 14 >;
> - hvac-zone-names = "operator";
> - spin-controller = < _ctrl_2 5 _ctrl_2
> 7 >;
> - spin-controller-names = "track_1", "track_2";
> - queues = < 2 >;
> -
> - track@30 {
> - reg = < 0x0030 0x10 >;
> - };
> -
> - track@40 {
> - reg = < 0x0040 0x10 >;
> - };
> -
> - };
> - };
> -
> - lights_1: lights@3 {
> - compatible = "ot,work-lights";
> - reg = < 0x0003 0x1000 >;
> - status = "disabled";
> - };
> -
> - lights_2: lights@4 {
> - compatible = "ot,show-lights";
> - reg = < 0x0004 0x1000 >;
> - status = "disabled";
> - rate = < 13 138 >;
> - };
> -
> - retail_1: vending@5 {
> -
Re: [PATCH V8 0/4] dt: Add fdtoverlay rule and statically build unittest
Hi Viresh, On 3/1/21 12:56 AM, Viresh Kumar wrote: > On 12-02-21, 16:48, Viresh Kumar wrote: >> Hi, >> >> This patchset adds a generic rule for applying overlays using fdtoverlay >> tool and then updates unittests to get built statically using the same. >> >> V7->V8: >> - Patch 1 is new. >> - Platforms need to use dtb-y += foo.dtb instead of overlay-y += >> foo.dtb. >> - Use multi_depend instead of .SECONDEXPANSION. >> - Use dtb-y for unittest instead of overlay-y. >> - Rename the commented dtb filess in unittest Makefile as .dtbo. >> - Improved Makefile code (I am learning a lot every day :) > > Ping! > Please respin on 5.12-rc1, and pull in the change you said you would make in response to my post v8 comment about the v7 patches. -Frank
Re: [PATCH V7 4/6] kbuild: Add support to build overlays (%.dtbo)
On 2/5/21 3:08 PM, Rob Herring wrote: > On Fri, Feb 05, 2021 at 11:17:10AM +0100, Geert Uytterhoeven wrote: >> Hi Viresh, >> >> On Fri, Feb 5, 2021 at 10:55 AM Viresh Kumar wrote: >>> On 05-02-21, 10:41, Geert Uytterhoeven wrote: On Fri, Feb 5, 2021 at 10:25 AM Viresh Kumar wrote: > On 05-02-21, 10:02, Geert Uytterhoeven wrote: >> Thanks for your patch >> (which I only noticed because it appeared in dt-rh/for-next ;-) >> >> On Fri, Jan 29, 2021 at 8:31 AM Viresh Kumar >> wrote: >>> Add support for building DT overlays (%.dtbo). The overlay's source file >>> will have the usual extension, i.e. .dts, though the blob will have >> >> Why use .dts and not .dtso for overlays? >> Because you originally (until v5) had a single rule for building .dtb >> and .dtbo files? > > I am fine with doing that as well if Rob and David agree to it. Rob > did suggest that at one point but we didn't do much about it later on > for some reason. > > FWIW, this will also require a change in the DTC compiler. Care to explain why? I've been using .dtsi for ages in https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/log/?h=topic/renesas-overlays >>> >>> I don't see you building them anywhere, they aren't added to the >>> Makefile ever. What am I missing ? >>> >>> actually none of the dtso's were added to any makefile in that branch. >> >> E.g. "ARM: dts: Build all overlays if OF_OVERLAY=y"? >> https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/commit/?h=topic/renesas-overlays=597ee90971687a45678cca8d16bf624d174a99eb >> >>> Anyway, the DTC needs to know how to treat the dtso format and it will >>> error out currently with unknown format kind of errors. >>> >>> Below email [1] have some information on the kind of changes required >>> here. Also note that we had to do similar changes for dtbo earlier >>> [2]. >>> >>> -- >>> viresh >>> >>> [1] >>> https://lore.kernel.org/lkml/cak7lnasvicotgr7ydtfh0o+pau+x-p2nwdy4opmuxrr51aw...@mail.gmail.com/ >> >> -@ is handled by "kbuild: Enable DT symbols when CONFIG_OF_OVERLAY is used" >> https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/commit/?h=topic/renesas-overlays=91e9d998514f3743125a707013a30d5f83054579 >> >>> [2] >>> https://lore.kernel.org/lkml/30fd0e5f2156665c713cf191c5fea9a5548360c0.1609926856.git.viresh.ku...@linaro.org/ >> >> I never had a need for those changes to dtc. .dtso/.dtbo work fine >> regardless. > > I think what Viresh means is dtc won't automatically set the input type > to dts if not .dts. > > We stuck with .dtbo as it's already widely used. I don't know about > dtso though. If there's strong consensus to use .dtso, then let's do > that. Whatever color for this shed you like. I overlooked this and mistakenly thought that the move to .dtbo also involved changing to .dtso. My bad. My favorite color here is to use .dtso for the source file that will be compiled to create a .dtbo. Linus has already accepted patch 4/6 to 5.12-rc1, so changing to .dtso will require another patch. -Frank > > Rob >
Re: RFC: oftree based setup of composite board devices
On 2/24/21 7:00 AM, Enrico Weigelt, metux IT consult wrote: > On 15.02.21 02:12, Frank Rowand wrote: > >> Why not compile in ACPI data (tables?) instead of devicetree description? > > The problem is a bit more complex than it might seem. > > Let's take the APU2/37/4 boards as an example. They've got some aux > devices, eg. some gpio controller, and some things (leds, keys, reset > lines, etc) attached to it. > > Now we've got lots of different bios versions in the field, > enumerating only some of the devices. For example, older ones didn't > even contain the gpio, later ones added just gpio, other ones just > added LEDs (with different names than the Linux driver already mainlined > and field-deployed at that time), but still other lines unhandled, etc, etc. > etc. > > A big mess :( And I can't ask everybody to do bios uprade on devices far > out in the field (litterally open field, sometimes offshore, ...). So, I > need a usable solution, that's also maintainable, w/o testing each > single combination of board, bios, etc. IOW: without relying on bios > (except for board identification) > > OTOH, I'm also looking for a solution get rid writing those kind of > relatively huge board drivers, that pretty are much like old fashioned > board files from pre-DT times - just made up of lots of tables and > a few trivial register-something calls. Sounds pretty much like the > original use case of oftree. > > The primary difference between classic oftree and this scanario: > * this is additional to existing platform information, which is > incomplete or even incorrect (and that can't be fixed) > * extra carrier boards that are detected by other means, but no > enumeration of the devices on it. > >>> This is something I've wanted to see for a while. There's use cases >>> for DT based systems too. The example I'd like to see supported are >>> USB serial adapters with downstream serdev, GPIO, I2C, SPI, etc. Then >>> plug more than one of those in. >> >> My understanding from the past is that the experts (those who understand both >> devicetree and ACPI) regard trying to mix devicetree and ACPI in a single >> running Linux kernel image is insanity, or at least likely to be confusing, >> difficult, and problematic. Since you have persisted, a more referenced and emphatic "no" to mixing ACPI and devicetree: https://elinux.org/Device_Tree_Linux#mixing_devicetree_and_ACPI > > Well, mixing different, overlapping data sources tends to be tricky. The > same problem exists with the classic approach of hand-written board > drivers. So there have to be clear border lines. > > In my case (eg. apu2+ boards), the overlap is only that some bios > versions enumerate the gpio chip, others even some of the gpio-based > devices. I'm attempting to solve this by just kicking out those > duplicate devices, if they exist. The alternative could be leaving them > in an trying to bind the missing ones to them. But that would be really > complicatd and needs to be well crafted for lots of different board and > bios versions - a kind of complexity we wanna avoid. So you want to use devicetree data to fix broken ACPI data. Again, why don't you use data in an ACPI format to fix broken ACPI data? -Frank > > My use cases are actually a bit easier than the average dt overlay > cases, as I have almost no interactions with already existing devices > (except that some specific devices have to be moved out of the way) > > The original DT overlay use case, arbitrary expansion boards (eg. on > raspi), are trickier, if the overlays shall be generic over a wider > range of base boards (eg. same overlay for any raspi or odroid). > This is something calling for an own (pseudo-)bus type that handles > the correct probing ... I've hacked up something similar for the APU2+'s > combined msata/usb/mpcie ports. > > BTW: I've already been thinking of ways for internally transforming ACPI > tables into DT data structures (struct device_node) at an early point, > before probing. But that would be another research project with unknown > outcome, and most likely a HUGE change. Not what I'm talking about now. > >> From the devicetree side, I expect nightmares for me if devicetree and ACPI >> are mixed in a single running kernel image. > > Note that I'm not talking about arbitrary configurations. Just re-using > existing device tree code to express things that are currently open > coded C into DT. > > It's NOT trying to boot an ACPI-based machine with DT. (which would be > yet another research project) > >> Multiple root nodes and disjoint trees both seem problematic. Existing >> subsystems and drivers expect a single coh
Re: [PATCH V7 5/6] of: unittest: Create overlay_common.dtsi and testcases_common.dtsi
Hi Viresh,
I am in the wrong version with the comments below. You are at version 8 now.
-Frank
On 2/18/21 3:02 PM, Frank Rowand wrote:
> On 1/29/21 1:24 AM, Viresh Kumar wrote:
>> In order to build-test the same unit-test files using fdtoverlay tool,
>> move the device nodes from the existing overlay_base.dts and
>> testcases_common.dts files to .dtsi counterparts. The .dts files now
>> include the new .dtsi files, resulting in exactly the same behavior as
>> earlier.
>>
>> The .dtsi files can now be reused for compile time tests using
>> fdtoverlay (will be done by a later commit).
>>
>> This is required because the base files passed to fdtoverlay tool
>> shouldn't be overlays themselves (i.e. shouldn't have the /plugin/;
>> tag).
>>
>> Note that this commit also moves "testcase-device2" node to
>> testcases.dts from tests-interrupts.dtsi, as this node has a deliberate
>> error in it and is only relevant for runtime testing done with
>> unittest.c.
>>
>> Signed-off-by: Viresh Kumar
>> ---
>> drivers/of/unittest-data/overlay_base.dts | 90 +-
>> drivers/of/unittest-data/overlay_common.dtsi | 91 +++
>> drivers/of/unittest-data/testcases.dts| 18 ++--
>> .../of/unittest-data/testcases_common.dtsi| 19
>> .../of/unittest-data/tests-interrupts.dtsi| 7 --
>> 5 files changed, 118 insertions(+), 107 deletions(-)
>> create mode 100644 drivers/of/unittest-data/overlay_common.dtsi
>> create mode 100644 drivers/of/unittest-data/testcases_common.dtsi
>>
>> diff --git a/drivers/of/unittest-data/overlay_base.dts
>> b/drivers/of/unittest-data/overlay_base.dts
>> index 99ab9d12d00b..ab9014589c5d 100644
>> --- a/drivers/of/unittest-data/overlay_base.dts
>> +++ b/drivers/of/unittest-data/overlay_base.dts
>> @@ -2,92 +2,4 @@
>> /dts-v1/;
>> /plugin/;
>>
>> -/*
>> - * Base device tree that overlays will be applied against.
>> - *
>> - * Do not add any properties in node "/".
>> - * Do not add any nodes other than "/testcase-data-2" in node "/".
>> - * Do not add anything that would result in dtc creating node "/__fixups__".
>> - * dtc will create nodes "/__symbols__" and "/__local_fixups__".
>> - */
>> -
>> -/ {
>> -testcase-data-2 {
>> -#address-cells = <1>;
>> -#size-cells = <1>;
>> -
>> -electric_1: substation@100 {
>> -compatible = "ot,big-volts-control";
>> -reg = < 0x0100 0x100 >;
>> -status = "disabled";
>> -
>> -hvac_1: hvac-medium-1 {
>> -compatible = "ot,hvac-medium";
>> -heat-range = < 50 75 >;
>> -cool-range = < 60 80 >;
>> -};
>> -
>> -spin_ctrl_1: motor-1 {
>> -compatible = "ot,ferris-wheel-motor";
>> -spin = "clockwise";
>> -rpm_avail = < 50 >;
>> -};
>> -
>> -spin_ctrl_2: motor-8 {
>> -compatible = "ot,roller-coaster-motor";
>> -};
>> -};
>> -
>> -rides_1: fairway-1 {
>> -#address-cells = <1>;
>> -#size-cells = <1>;
>> -compatible = "ot,rides";
>> -status = "disabled";
>> -orientation = < 127 >;
>> -
>> -ride@100 {
>> -#address-cells = <1>;
>> -#size-cells = <1>;
>> -compatible = "ot,roller-coaster";
>> -reg = < 0x0100 0x100 >;
>> -hvac-provider = < _1 >;
>> -hvac-thermostat = < 29 > ;
>> -hvac-zones = < 14 >;
>> -hvac-zone-names = "operator";
>> -spin-controller = < _ctrl_2 5 _ctrl_2
>> 7 >;
>> -spin-controller-names = "track_1", "track_2";
>>
Re: [PATCH V7 5/6] of: unittest: Create overlay_common.dtsi and testcases_common.dtsi
On 1/29/21 1:24 AM, Viresh Kumar wrote:
> In order to build-test the same unit-test files using fdtoverlay tool,
> move the device nodes from the existing overlay_base.dts and
> testcases_common.dts files to .dtsi counterparts. The .dts files now
> include the new .dtsi files, resulting in exactly the same behavior as
> earlier.
>
> The .dtsi files can now be reused for compile time tests using
> fdtoverlay (will be done by a later commit).
>
> This is required because the base files passed to fdtoverlay tool
> shouldn't be overlays themselves (i.e. shouldn't have the /plugin/;
> tag).
>
> Note that this commit also moves "testcase-device2" node to
> testcases.dts from tests-interrupts.dtsi, as this node has a deliberate
> error in it and is only relevant for runtime testing done with
> unittest.c.
>
> Signed-off-by: Viresh Kumar
> ---
> drivers/of/unittest-data/overlay_base.dts | 90 +-
> drivers/of/unittest-data/overlay_common.dtsi | 91 +++
> drivers/of/unittest-data/testcases.dts| 18 ++--
> .../of/unittest-data/testcases_common.dtsi| 19
> .../of/unittest-data/tests-interrupts.dtsi| 7 --
> 5 files changed, 118 insertions(+), 107 deletions(-)
> create mode 100644 drivers/of/unittest-data/overlay_common.dtsi
> create mode 100644 drivers/of/unittest-data/testcases_common.dtsi
>
> diff --git a/drivers/of/unittest-data/overlay_base.dts
> b/drivers/of/unittest-data/overlay_base.dts
> index 99ab9d12d00b..ab9014589c5d 100644
> --- a/drivers/of/unittest-data/overlay_base.dts
> +++ b/drivers/of/unittest-data/overlay_base.dts
> @@ -2,92 +2,4 @@
> /dts-v1/;
> /plugin/;
>
> -/*
> - * Base device tree that overlays will be applied against.
> - *
> - * Do not add any properties in node "/".
> - * Do not add any nodes other than "/testcase-data-2" in node "/".
> - * Do not add anything that would result in dtc creating node "/__fixups__".
> - * dtc will create nodes "/__symbols__" and "/__local_fixups__".
> - */
> -
> -/ {
> - testcase-data-2 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> -
> - electric_1: substation@100 {
> - compatible = "ot,big-volts-control";
> - reg = < 0x0100 0x100 >;
> - status = "disabled";
> -
> - hvac_1: hvac-medium-1 {
> - compatible = "ot,hvac-medium";
> - heat-range = < 50 75 >;
> - cool-range = < 60 80 >;
> - };
> -
> - spin_ctrl_1: motor-1 {
> - compatible = "ot,ferris-wheel-motor";
> - spin = "clockwise";
> - rpm_avail = < 50 >;
> - };
> -
> - spin_ctrl_2: motor-8 {
> - compatible = "ot,roller-coaster-motor";
> - };
> - };
> -
> - rides_1: fairway-1 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> - compatible = "ot,rides";
> - status = "disabled";
> - orientation = < 127 >;
> -
> - ride@100 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> - compatible = "ot,roller-coaster";
> - reg = < 0x0100 0x100 >;
> - hvac-provider = < _1 >;
> - hvac-thermostat = < 29 > ;
> - hvac-zones = < 14 >;
> - hvac-zone-names = "operator";
> - spin-controller = < _ctrl_2 5 _ctrl_2
> 7 >;
> - spin-controller-names = "track_1", "track_2";
> - queues = < 2 >;
> -
> - track@30 {
> - reg = < 0x0030 0x10 >;
> - };
> -
> - track@40 {
> - reg = < 0x0040 0x10 >;
> - };
> -
> - };
> - };
> -
> - lights_1: lights@3 {
> - compatible = "ot,work-lights";
> - reg = < 0x0003 0x1000 >;
> - status = "disabled";
> - };
> -
> - lights_2: lights@4 {
> - compatible = "ot,show-lights";
> - reg = < 0x0004 0x1000 >;
> - status = "disabled";
> - rate = < 13 138 >;
> - };
> -
> - retail_1: vending@5 {
> - reg = < 0x0005 0x1000 >;
> - compatible = "ot,tickets";
> - status = "disabled";
>
Re: DT overlay applied via pinctrl description
+Frank, Rob, devicetree list
On 2/16/21 9:35 AM, Michal Simek wrote:
> Hi,
>
> I have a question about expectations when pinctrl setting is applied. In
> DTS all nodes are described in the order available in DT.
>
> uart-default {
> mux {
> ...
> };
>
> conf {
> ...
> };
> };
>
> I don't know if this standard description or not. I definitely see other
> pinctrl drivers which are using different structure.
>
> Anyway when overlay is applied the order has changed to
> uart-default {
> conf {
> ...
> };
>
> mux {
> ...
> };
> };
>
> which is causing issue because pin is configured first via conf node
> before it is requested via mux. This is something what firmware is
> checking and error out.
>
> That's why I want to check with you if this is issue with DT binding
> description we use in zynqmp pinctrl driver posted here
> https://lore.kernel.org/linux-arm-kernel/1613131643-60062-1-git-send-email-lakshmi.sai.krishna.potth...@xilinx.com/
>
> I have also tried to use init and default configuration where init is
> called just with mux setting and then default is called just with config
> but the issue is there as well because in pinctrl_commit_state()
> previous state is checked and for MUXes pinmux_disable_setting() is
> called which release a pin. And then configuration in default is called
> but without requesting pin which fails for the same reason as above.
>
> That's why my questions are:
> Are we using incorrect DT description?
> And is there a need sort subnodes in a way that mux should be called
> first by core before configuration?
> Or is there any different way how to do it?
Node ordering and property ordering within a node are not defined
in the Linux kernel. If a subsystem or property is depending upon
a certain order, they must implement a method other than the
order as accessed by of_* functions. And as you noted, use of an
overlay may also change ordering.
-Frank
>
> Thanks,
> Michal
>
Re: [RFC PATCH 11/12] platform/x86: skeleton for oftree based board device initialization
On 2/12/21 5:54 AM, Enrico Weigelt, metux IT consult wrote: > On 12.02.21 10:58, Linus Walleij wrote: > > Hi, > >> I think Intel people often take the stance that the ACPI DSDT (or whatever) >> needs to be fixed. > > It should, actually board/firmware vendors should think more carefully > and do it right in the first place. But reality is different. And > firmware upgrade often is anything but easy (as soon as we leave the > field of average Joh Doe's home PC) > >> If the usecase is to explicitly work around deployed firmware that cannot >> and will not be upgraded/fixed by describing the hardware using DT >> instead, based on just the DMI ID then we should spell that out >> explicitly. > > Okay, maybe I should have stated this more clearly. > > OTOH, the scope is also a little bit greater: certain external cards > that don't need much special handling for the card itself, just > enumerate devices (and connections between them) using existing drivers. > > That's a pretty common scenario in industrial backplane systems, where > we have lots of different (even application specific) cards, usually > composed of standard chips, that can be identified by some ID, but > cannot describe themselves. We have to write lots of specific drivers > for them, usually just for instantiating existing drivers. (we rarely > see such code going towards mainline). > > A similar case (mainlined) seems to be the RCAR display unit - they're > using dt overlays that are built into the driver and applied by it > based on the detected DU at runtime. RCAR seems to be a pure DT The RCAR use of overlays that are built into the driver are a known pattern that is explicitly not to be repeated. The driver has been granted a grandfathered in status, thus an exception as long as needed. -Frank > platform, so that's an obvious move. APU2/3/4 is ACPI based, so I went > in a different direction - but I'm now investigating how to make DT > overlays work on an ACPI platform (eg. needs some initial nodes, ...) > In case that's successful, I'll rework my RFC to use overlays, and > it will become much smaller (my oftree core changes then won't be > necessary anymore). > >> It feels a bit like fixing a problem using a different hardware description >> just because we can. Look in drivers/gpio/gpiolib-acpi.c >> table gpiolib_acpi_quirks[]. It's just an example how this is fixed using >> fine granular ACPI-specific mechanisms at several places in the kernel >> instead of just tossing out the whole description and redoing it in >> device tree. > > I'm quite reluctant to put everything in there. Theoretically, for apu > case, I could prevent enumerating the incomplete gpios there, but the > actual driver setup still remains (certainly don't wanna put that into > such a global place). But the original problem of having to write so > much code for just instantiating generic drivers remains. And > distributing knowledge of certain devices over several places doesn't > feel like a good idea to me. > > > --mtx >
Re: RFC: oftree based setup of composite board devices
On 2/8/21 5:48 PM, Rob Herring wrote: > +Johan H > > On Mon, Feb 8, 2021 at 4:22 PM Enrico Weigelt, metux IT consult > wrote: >> >> Hello folks, >> >> here's an RFC for using compiled-in dtb's for initializing board devices >> that can't be probed via bus'es or firmware. I've just been monitoring this thread for several days, hoping that the discussion would make things more clear for me. Disclaimer: I know essentially nothing about ACPI, please excuse improper naming and misunderstandings on my part. Why not compile in ACPI data (tables?) instead of devicetree description? > > I'm not convinced compiled in is the mechanism we want. > >> Use cases are boards with non-oftree firmware (ACPI, etc) where certain >> platform devices can't be directly enumerated via firmware. Traditionally >> we had to write board specific drivers that check for board identification >> (DMI strings, etc), then initialize the actual devices and their links >> (eg. gpio<->leds/buttons, ...). Often this can be expressed just by DT. > > This is something I've wanted to see for a while. There's use cases > for DT based systems too. The example I'd like to see supported are > USB serial adapters with downstream serdev, GPIO, I2C, SPI, etc. Then > plug more than one of those in. My understanding from the past is that the experts (those who understand both devicetree and ACPI) regard trying to mix devicetree and ACPI in a single running Linux kernel image is insanity, or at least likely to be confusing, difficult, and problematic. >From the devicetree side, I expect nightmares for me if devicetree and ACPI are mixed in a single running kernel image. > >> This patch queue does a bunch of preparations in oftree code, so we can >> support multiple fully independent DT's (not using DT overlays). And then >> adds a generic driver parses compiled-in fdt blobs, checks for mathing >> DMI strings and initializes the devices. As an example, the last patch >> adds an alternative implementation for the PC engines APU2/3/4 board >> family based on device tree. > > I think there's a couple of approaches we could take. Either support > multiple root nodes as you have done or keep a single root and add > child nodes to them. I think the latter would be less invasive. In the > non-DT cases, we'd just always create an empty skeleton DT. A 3rd > variation on a DT system is we could want to create parent nodes if > they don't exist to attach this DT to so we have a full hierarchy. > > I'm not saying which one we should do, just laying out some of the options. > Multiple root nodes and disjoint trees both seem problematic. Existing subsystems and drivers expect a single cohesive tree. Changing that architecture looks to me to be a painful exercise. >> The approach can be easily be extended to other kinds of composite devices, >> eg. PCI cards or USB dongles. >> >> >> Yet some drawbacks of the current implementation: >> >> * individual FDT's can't be modularized yet (IMHO, we don't have DMI-based >>modprobing anyways) > > I think we need to use either firmware loading or udev mechanisms to > load the FDTs. > >> * can't reconfigure or attach to devices outside the individual DT's >>(eg. probed by PCI, etc) > > Not sure I follow. > > Rob >
Re: [GIT PULL 2/3] ARM: dts: samsung: DTS for v5.12
On 2/6/21 8:35 AM, Arnd Bergmann wrote: > On Sat, Feb 6, 2021 at 2:45 PM Krzysztof Kozlowski wrote: >> On Mon, Jan 25, 2021 at 08:12:39PM +0100, Krzysztof Kozlowski wrote: >>> >>> >>> Samsung DTS ARM changes for v5.12 >>> >>> 1. Use new compatile to properly configure Exynos5420 USB2 PHY, fixing >>>it suspend/resume cycle. >>> 2. Correct Samsung PMIC interrupt trigger levels on multiple boards. >>> 3. Correct the voltages of Samsung GT-I9100 charger and add top-off >>>charger. >>> >> >> Hi everyone, >> >> Any progress or new comments about this pull request? > > Hi Krzysztof, > > Sorry for not getting back to you on this earlier. I discussed this with > Olof the other day and we decided to merge this, I just haven't > gone through the pull requests over the past few days. My plan is > to do the next round on Monday. > > That said, I'm still not happy about the patch we discussed in the > other email thread[1] and I'd like to handle it a little more strictly in > the future, but I agree this wasn't obvious and we have been rather > inconsistent about it in the past, with some platform maintainers > handling it way more strictly than others. > > I've added the devicetree maintainers and a few other platform > maintainers to Cc here, maybe they can provide some further > opinions on the topic so we can come to an approach that > works for everyone. > > My summary of the thread in [1] is there was a driver bug that > required a DT binding change. Krzysztof and the other involved > parties made sure the driver handles it in a backward-compatible > way (an old dtb file will still run into the bug but keep working > with new kernels), but decided that they did not need to worry > about the opposite case (running an old kernel with an updated > dtb). I noticed the compatibility break and said that I would > prefer this to be done in a way that is compatible both ways, > or at the minimum be alerted about the binding break in the > pull request, with an explanation about why this had to be done, > even when we don't think anyone is going to be affected. > > What do others think about this? Should we generally assume > that breaking old kernels with new dtbs is acceptable, or should > we try to avoid it if possible, the same way we try to avoid > breaking new kernels with old dtbs? Should this be a platform > specific policy or should we try to handle all platforms the same > way? The current policy (since before 2013) is that newer kernels, implementing new bindings, do not break with old existing dtbs. Old existing kernels are not required to work with new dtbs. See Documentation/devicetree/bindings/ABI.rst We can choose to change the rules, so I'm not saying that the discussion should not occur. I'm just pointing out the current policy. I think that ABI.rst does not state "Old existing kernels are not required to work with new dtbs" clearly enough, and should be updated to do so. I also think it would be good to explicitly say that care should be taken with new bindings to not break existing kernels, if reasonably possible. -Frank > > Arnd > > [1] https://lore.kernel.org/lkml/20210130143949.aamac2724esupt7v@kozik-lap/ >
Re: [PATCH V6 5/6] of: unittest: Create overlay_common.dtsi and testcases_common.dtsi
Hi Viresh,
Second attempt, I think the first reply did not properly send.
On 1/26/21 11:56 PM, Viresh Kumar wrote:
> On 22-01-21, 16:20, Viresh Kumar wrote:
>> In order to build-test the same unit-test files using fdtoverlay tool,
>> move the device nodes from the existing overlay_base.dts and
>> testcases_common.dts files to .dtsi files. The .dts files now include
>> the new .dtsi files, resulting in exactly the same behavior as earlier.
>>
>> The .dtsi files can now be reused for compile time tests using
>> fdtoverlay (will be done in a later patch).
>>
>> This is required because the base files passed to fdtoverlay tool
>> shouldn't be overlays themselves (i.e. shouldn't have the /plugin/;
>> tag).
>>
>> Signed-off-by: Viresh Kumar
>> ---
>> drivers/of/unittest-data/overlay_base.dts | 90 +-
>> drivers/of/unittest-data/overlay_common.dtsi | 91 +++
>> drivers/of/unittest-data/testcases.dts| 17 +---
>> .../of/unittest-data/testcases_common.dtsi| 18
>> 4 files changed, 111 insertions(+), 105 deletions(-)
>> create mode 100644 drivers/of/unittest-data/overlay_common.dtsi
>> create mode 100644 drivers/of/unittest-data/testcases_common.dtsi
>
> Frank,
>
> As I mentioned in the cover-letter, I get a build warning right now:
>
> drivers/of/unittest-data/tests-interrupts.dtsi:20.5-28: Warning
> (interrupts_property): /testcase-data/testcase-device2:#interrupt-cells: size
> is (4), expected multiple of 8
Thanks for catching that.
>
> I think I need to add below diff to this patch to fix this warning, will that
> be okay ?
In my first reply, I said "nope", or something to that effect. Upon
reflection, it looks
like the below diff will fix the problem. This is base on source code
inspection and
building with the diff applied.
I did not successfully boot my target (I have some issues to resolve after
updating
the OS on my development host), so I have not verified that unittest is not
impacted.
-Frank
>
> diff --git a/drivers/of/unittest-data/testcases.dts
> b/drivers/of/unittest-data/testcases.dts
> index 185125085784..04b9e7bb30d9 100644
> --- a/drivers/of/unittest-data/testcases.dts
> +++ b/drivers/of/unittest-data/testcases.dts
> @@ -3,3 +3,14 @@
> /plugin/;
>
> #include "testcases_common.dtsi"
> +
> +/ {
> + testcase-data {
> + testcase-device2 {
> + compatible = "testcase-device";
> + interrupt-parent = <_intc2>;
> + interrupts = <1>; /* invalid specifier - too short */
> + };
> + };
> +
> +};
> diff --git a/drivers/of/unittest-data/tests-interrupts.dtsi
> b/drivers/of/unittest-data/tests-interrupts.dtsi
> index ec175e800725..0e5914611107 100644
> --- a/drivers/of/unittest-data/tests-interrupts.dtsi
> +++ b/drivers/of/unittest-data/tests-interrupts.dtsi
> @@ -61,12 +61,5 @@ testcase-device1 {
> interrupt-parent = <_intc0>;
> interrupts = <1>;
> };
> -
> - testcase-device2 {
> - compatible = "testcase-device";
> - interrupt-parent = <_intc2>;
> - interrupts = <1>; /* invalid specifier - too short */
> - };
> };
> -
> };
>
Re: [PATCH V6 2/6] scripts: dtc: Build fdtoverlay tool
On 1/22/21 4:50 AM, Viresh Kumar wrote: > We will start building overlays for platforms soon in the kernel and > would need fdtoverlay going forward. Lets start building it. > > The fdtoverlay program applies (or merges) one or more overlay dtb > blobs to a base dtb blob. The kernel build system would later use > fdtoverlay to generate the overlaid blobs based on platform specific > configurations. > > Signed-off-by: Viresh Kumar > --- > scripts/dtc/Makefile | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile > index 4852bf44e913..5f19386a49eb 100644 > --- a/scripts/dtc/Makefile > +++ b/scripts/dtc/Makefile > @@ -1,13 +1,17 @@ > # SPDX-License-Identifier: GPL-2.0 > # scripts/dtc makefile > > -hostprogs-always-$(CONFIG_DTC) += dtc > +hostprogs-always-$(CONFIG_DTC) += dtc fdtoverlay > hostprogs-always-$(CHECK_DT_BINDING) += dtc > > dtc-objs := dtc.o flattree.o fstree.o data.o livetree.o treesource.o \ > srcpos.o checks.o util.o > dtc-objs += dtc-lexer.lex.o dtc-parser.tab.o > Please add this comment: # The upstream project builds libfdt as a separate library. We are choosing to # instead directly link the libfdt object files into fdtoverly > +libfdt-objs := fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o > fdt_empty_tree.o fdt_addresses.o fdt_overlay.o > +libfdt = $(addprefix libfdt/,$(libfdt-objs)) > +fdtoverlay-objs := $(libfdt) fdtoverlay.o util.o > + > # Source files need to get at the userspace version of libfdt_env.h to > compile > HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt > >
Re: [PATCH V6 5/6] of: unittest: Create overlay_common.dtsi and testcases_common.dtsi
Hi Viresh,
On 1/26/21 11:56 PM, Viresh Kumar wrote:
> On 22-01-21, 16:20, Viresh Kumar wrote:
>> In order to build-test the same unit-test files using fdtoverlay tool,
>> move the device nodes from the existing overlay_base.dts and
>> testcases_common.dts files to .dtsi files. The .dts files now include
>> the new .dtsi files, resulting in exactly the same behavior as earlier.
>>
>> The .dtsi files can now be reused for compile time tests using
>> fdtoverlay (will be done in a later patch).
>>
>> This is required because the base files passed to fdtoverlay tool
>> shouldn't be overlays themselves (i.e. shouldn't have the /plugin/;
>> tag).
>>
>> Signed-off-by: Viresh Kumar
>> ---
>> drivers/of/unittest-data/overlay_base.dts | 90 +-
>> drivers/of/unittest-data/overlay_common.dtsi | 91 +++
>> drivers/of/unittest-data/testcases.dts| 17 +---
>> .../of/unittest-data/testcases_common.dtsi| 18
>> 4 files changed, 111 insertions(+), 105 deletions(-)
>> create mode 100644 drivers/of/unittest-data/overlay_common.dtsi
>> create mode 100644 drivers/of/unittest-data/testcases_common.dtsi
>
> Frank,
>
> As I mentioned in the cover-letter, I get a build warning right now:
>
> drivers/of/unittest-data/tests-interrupts.dtsi:20.5-28: Warning
> (interrupts_property): /testcase-data/testcase-device2:#interrupt-cells: size
> is (4), expected multiple of 8
Thanks for catching that.
>
> I think I need to add below diff to this patch to fix this warning, will that
> be okay ?
Nope, the change below won't work because it removes the node testcase-device2
from the tests
that unittest.c does (if I am thinking correctly). I will double check my
thinking, but I
know you are spinning the patch, so I didn't want to delay this reply.
Note that this node has a deliberate error in it "/* invalid specifier - too
short */".
I'm not sure why the dtc warning triggers on line 20 instead of line 68. I'll
have to go
look at the dtc source to better understand the warning.
-Frank
>
> diff --git a/drivers/of/unittest-data/testcases.dts
> b/drivers/of/unittest-data/testcases.dts
> index 185125085784..04b9e7bb30d9 100644
> --- a/drivers/of/unittest-data/testcases.dts
> +++ b/drivers/of/unittest-data/testcases.dts
> @@ -3,3 +3,14 @@
> /plugin/;
>
> #include "testcases_common.dtsi"
> +
> +/ {
> + testcase-data {
> + testcase-device2 {
> + compatible = "testcase-device";
> + interrupt-parent = <_intc2>;
> + interrupts = <1>; /* invalid specifier - too short */
> + };
> + };
> +
> +};
> diff --git a/drivers/of/unittest-data/tests-interrupts.dtsi
> b/drivers/of/unittest-data/tests-interrupts.dtsi
> index ec175e800725..0e5914611107 100644
> --- a/drivers/of/unittest-data/tests-interrupts.dtsi
> +++ b/drivers/of/unittest-data/tests-interrupts.dtsi
> @@ -61,12 +61,5 @@ testcase-device1 {
> interrupt-parent = <_intc0>;
> interrupts = <1>;
> };
> -
> - testcase-device2 {
> - compatible = "testcase-device";
> - interrupt-parent = <_intc2>;
> - interrupts = <1>; /* invalid specifier - too short */
> - };
> };
> -
> };
>
Re: [PATCH] cmd_dtc: Enable generation of device tree symbols
+frank On 1/25/21 3:53 PM, Masahiro Yamada wrote: > On Mon, Jan 25, 2021 at 8:07 PM Uwe Kleine-König > wrote: >> >> Adding the -@ switch to dtc results in the binary devicetrees containing >> a list of symbolic references and their paths. This is necessary to >> apply device tree overlays e.g. on Raspberry Pi as described on >> https://www.raspberrypi.org/documentation/configuration/device-tree.md. >> >> Obviously the downside of this change is an increas of the size of the >> generated dtbs, for an arm out-of-tree build (multi_v7_defconfig): >> >> $ du -s arch/arm/boot/dts* >> 101380 arch/arm/boot/dts-pre >> 114308 arch/arm/boot/dts-post >> >> so this is in average an increase of 12.8% in size. >> >> Signed-off-by: Uwe Kleine-König > > > (CCing DT ML.) > > > https://www.spinics.net/lists/linux-kbuild/msg27904.html > > See Rob's comment: > > "We've already rejected doing that. Turning on '-@' can grow the dtb > size by a significant amount which could be problematic for some > boards." > > > > > > > > >> --- >> scripts/Makefile.lib | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib >> index 213677a5ed33..0683a5808f7f 100644 >> --- a/scripts/Makefile.lib >> +++ b/scripts/Makefile.lib >> @@ -319,7 +319,7 @@ $(obj)/%.dtb.S: $(obj)/%.dtb FORCE >> >> quiet_cmd_dtc = DTC $@ >> cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) >> $< ; \ >> - $(DTC) -O $(patsubst .%,%,$(suffix $@)) -o $@ -b 0 \ >> + $(DTC) -@ -O $(patsubst .%,%,$(suffix $@)) -o $@ -b 0 \ >> $(addprefix -i,$(dir $<) $(DTC_INCLUDE)) $(DTC_FLAGS) \ >> -d $(depfile).dtc.tmp $(dtc-tmp) ; \ >> cat $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile) >> -- >> 2.29.2 >> > > > -- > Best Regards > > Masahiro Yamada >
Re: [PATCH] cmd_dtc: Enable generation of device tree symbols
+frank On 1/25/21 4:57 AM, Uwe Kleine-König wrote: > Adding the -@ switch to dtc results in the binary devicetrees containing > a list of symbolic references and their paths. This is necessary to > apply device tree overlays e.g. on Raspberry Pi as described on > https://www.raspberrypi.org/documentation/configuration/device-tree.md. > > Obviously the downside of this change is an increas of the size of the > generated dtbs, for an arm out-of-tree build (multi_v7_defconfig): > > $ du -s arch/arm/boot/dts* > 101380 arch/arm/boot/dts-pre > 114308 arch/arm/boot/dts-post > > so this is in average an increase of 12.8% in size. > > Signed-off-by: Uwe Kleine-König > --- > scripts/Makefile.lib | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib > index 213677a5ed33..0683a5808f7f 100644 > --- a/scripts/Makefile.lib > +++ b/scripts/Makefile.lib > @@ -319,7 +319,7 @@ $(obj)/%.dtb.S: $(obj)/%.dtb FORCE > > quiet_cmd_dtc = DTC $@ > cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) > $< ; \ > - $(DTC) -O $(patsubst .%,%,$(suffix $@)) -o $@ -b 0 \ > + $(DTC) -@ -O $(patsubst .%,%,$(suffix $@)) -o $@ -b 0 \ > $(addprefix -i,$(dir $<) $(DTC_INCLUDE)) $(DTC_FLAGS) \ > -d $(depfile).dtc.tmp $(dtc-tmp) ; \ > cat $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile) >
Re: [PATCH] cmd_dtc: Enable generation of device tree symbols
Hi Uwe,
On 1/26/21 12:03 PM, Frank Rowand wrote:
> +frank
>
> On 1/26/21 1:20 AM, Uwe Kleine-König wrote:
>> Hello Masahiro,
>>
>> On 1/25/21 10:53 PM, Masahiro Yamada wrote:
>>> On Mon, Jan 25, 2021 at 8:07 PM Uwe Kleine-König
>>> wrote:
>>>>
>>>> Adding the -@ switch to dtc results in the binary devicetrees containing
>>>> a list of symbolic references and their paths. This is necessary to
>>>> apply device tree overlays e.g. on Raspberry Pi as described on
>>>> https://www.raspberrypi.org/documentation/configuration/device-tree.md.
>>>>
>>>> Obviously the downside of this change is an increas of the size of the
>>>> generated dtbs, for an arm out-of-tree build (multi_v7_defconfig):
>>>>
>>>> $ du -s arch/arm/boot/dts*
>>>> 101380 arch/arm/boot/dts-pre
>>>> 114308 arch/arm/boot/dts-post
>>>>
>>>> so this is in average an increase of 12.8% in size.
>>>>
>>>> Signed-off-by: Uwe Kleine-König
>>>
>>>
>>> (CCing DT ML.)
>>
>> makes sense, thanks.
>>
>>> https://www.spinics.net/lists/linux-kbuild/msg27904.html
>>>
>>> See Rob's comment:
>>>
>>> "We've already rejected doing that. Turning on '-@' can grow the dtb
>>> size by a significant amount which could be problematic for some
>>> boards."
>>
>> The patch was created after some conversation on irc which continued
>> after I sent the patch. I added the participating parties to Cc:.
Unfortunately I have not been on irc recently (now rectified). Do you
perchance have a copy of the irc conversation that you can send me?
(No need to edit out unrelated messages, a simple cut and paste from
the start of the conversation to the end is fine.)
-Frank
>>
>> The (relevant) followups were:
>>
>> Geert suggested to always generate the symbols and provide a way to
>> strip the symbols for installation if and when they are not needed.
>>
>> Rob said: "I'm less concerned with the size increases, but rather that
>> labels go from purely source syntax to an ABI. I'd rather see some
>> decision as to which labels are enabled or not."
>>
>> And then I learned with hints from Rob and Geert that symbols are not
>> really necessary for overlays, you just cannot use named labels. But
>> using
>>
>> target-path = "/soc/i2c@23473245";
>>
>> or
>>
>> target = <&{/soc/i2c@23473245}>;
>>
>> instead of
>>
>> target = <>;
>>
>> works fine. (And if you need to add a phandle the &{/path/to/node}
>> construct should work, too (but I didn't test).) Using labels is a tad
>> nicer, but the problem I wanted to address with my patch now has a known
>> different solution.
>>
>> Best regards
>> Uwe
>>
>>
>> ___
>> linux-arm-kernel mailing list
>> linux-arm-ker...@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
Re: [PATCH] cmd_dtc: Enable generation of device tree symbols
+frank
On 1/26/21 1:20 AM, Uwe Kleine-König wrote:
> Hello Masahiro,
>
> On 1/25/21 10:53 PM, Masahiro Yamada wrote:
>> On Mon, Jan 25, 2021 at 8:07 PM Uwe Kleine-König
>> wrote:
>>>
>>> Adding the -@ switch to dtc results in the binary devicetrees containing
>>> a list of symbolic references and their paths. This is necessary to
>>> apply device tree overlays e.g. on Raspberry Pi as described on
>>> https://www.raspberrypi.org/documentation/configuration/device-tree.md.
>>>
>>> Obviously the downside of this change is an increas of the size of the
>>> generated dtbs, for an arm out-of-tree build (multi_v7_defconfig):
>>>
>>> $ du -s arch/arm/boot/dts*
>>> 101380 arch/arm/boot/dts-pre
>>> 114308 arch/arm/boot/dts-post
>>>
>>> so this is in average an increase of 12.8% in size.
>>>
>>> Signed-off-by: Uwe Kleine-König
>>
>>
>> (CCing DT ML.)
>
> makes sense, thanks.
>
>> https://www.spinics.net/lists/linux-kbuild/msg27904.html
>>
>> See Rob's comment:
>>
>> "We've already rejected doing that. Turning on '-@' can grow the dtb
>> size by a significant amount which could be problematic for some
>> boards."
>
> The patch was created after some conversation on irc which continued
> after I sent the patch. I added the participating parties to Cc:.
>
> The (relevant) followups were:
>
> Geert suggested to always generate the symbols and provide a way to
> strip the symbols for installation if and when they are not needed.
>
> Rob said: "I'm less concerned with the size increases, but rather that
> labels go from purely source syntax to an ABI. I'd rather see some
> decision as to which labels are enabled or not."
>
> And then I learned with hints from Rob and Geert that symbols are not
> really necessary for overlays, you just cannot use named labels. But
> using
>
> target-path = "/soc/i2c@23473245";
>
> or
>
> target = <&{/soc/i2c@23473245}>;
>
> instead of
>
> target = <>;
>
> works fine. (And if you need to add a phandle the &{/path/to/node}
> construct should work, too (but I didn't test).) Using labels is a tad nicer,
> but the problem I wanted to address with my patch now has a known different
> solution.
>
> Best regards
> Uwe
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Re: [PATCH] cmd_dtc: Enable generation of device tree symbols
+frank
On 1/26/21 2:43 AM, Geert Uytterhoeven wrote:
> Hi Uwe,
>
> On Tue, Jan 26, 2021 at 8:21 AM Uwe Kleine-König
> wrote:
>> And then I learned with hints from Rob and Geert that symbols are not
>> really necessary for overlays, you just cannot use named labels. But
>> using
>>
>> target-path = "/soc/i2c@23473245";
>>
>> or
>>
>> target = <&{/soc/i2c@23473245}>;
>>
>> instead of
>>
>> target = <>;
>>
>> works fine. (And if you need to add a phandle the &{/path/to/node}
>> construct should work, too (but I didn't test).) Using labels is a tad
>> nicer, but the problem I wanted to address with my patch now has a known
>> different solution.
>
> Please don't use "target" and "target-path". Since the introduction of
> sugar syntax support in v4.15[1], you can just use "", like in a normal
> DTS file. Paths do need the special "&{/path/to/node}" syntax instead
> of "/path/to/node", though.
>
> As usual, you can find lots of examples of DT overlays in my repo[2].
>
> [1] commit 4201d057ea91c3d6 ("scripts/dtc: Update to upstream version
> v1.4.5-3-gb1a60033c110")
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/log/?h=topic/renesas-overlays
>
> Gr{oetje,eeting}s,
>
> Geert
>
Re: [PATCH] cmd_dtc: Enable generation of device tree symbols
+frank On 1/25/21 5:15 AM, Cyril Brulebois wrote: > Hi, > > Uwe Kleine-König (2021-01-25): >> Adding the -@ switch to dtc results in the binary devicetrees containing >> a list of symbolic references and their paths. This is necessary to >> apply device tree overlays e.g. on Raspberry Pi as described on >> https://www.raspberrypi.org/documentation/configuration/device-tree.md. >> >> Obviously the downside of this change is an increas of the size of the > > (as spotted by Uwe right after sending →) increase > >> generated dtbs, for an arm out-of-tree build (multi_v7_defconfig): >> >> $ du -s arch/arm/boot/dts* >> 101380 arch/arm/boot/dts-pre >> 114308 arch/arm/boot/dts-post >> >> so this is in average an increase of 12.8% in size. >> >> Signed-off-by: Uwe Kleine-König > > Tested-by: Cyril Brulebois > > with: > - a Raspberry Pi CM3 > - a carrier board designed after the official IO Board V3 > - an RTC accessible over I²C, made functional via a DTB overlay, that >can only be enabled once bcm2710-rpi-cm3.dtb has been generated with >this patch applied. > > > Cheers, > > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
Re: [PATCH V6 0/6] dt: build overlays
On 1/22/21 4:50 AM, Viresh Kumar wrote: > Hi Frank/Rob, > > This patchset makes necessary changes to the kernel to add support for > building overlays (%.dtbo) and the required fdtoverlay tool. This also > builds static_test.dtb using most of the existing overlay tests present > in drivers/of/unittest-data/ for better test coverage. > > Note that in order for anyone to test this stuff, you need to manually > run the ./update-dtc-source.sh script once to fetch the necessary > changes from the external DTC project (i.e. fdtoverlay.c and this[1] > patch). > > Also note that Frank has already shared his concerns towards the error > reporting done by fdtoverlay tool [2], and David said it is not that > straight forward to make such changes in fdtoverlay. I have still > included the patch in this series for completeness. I started to reply to this email with questions for David about how to improve the fdtoverlay error reporting. But then decided that instead of trying to paraphrase the comments in v4 of this patch series, it would be more efficient to ask in the v4 thread. So my questions are over there... -Frank > > FWIW, with fdtoverlay we generate a new build warning now, not sure why > though: > > drivers/of/unittest-data/tests-interrupts.dtsi:20.5-28: Warning > (interrupts_property): /testcase-data/testcase-device2:#interrupt-cells: size > is (4), expected multiple of 8 > > V6: > - Create separate rules for dtbo-s and separate entries in .gitignore in > 4/6 (Masahiro). > - A new file layout for handling all overlays for existing and new tests > 5/6 (Frank). > - Include overlay.dts as well now in 6/6 (Frank). > > V5: > > - Don't reuse DTC_SOURCE for fdtoverlay.c in patch 1/5 (Frank). > > - Update .gitignore and scripts/Makefile.dtbinst, drop dtbo-y syntax and > DTC_FLAGS += -@ in patch 4/5 (Masahiro). > > - Remove the intermediate dtb, rename output to static_test.dtb, don't > use overlay.dtb and overlay_base.dtb for static builds, improved > layout/comments in Makefile for patch 5/5 (Frank). > > -- > Viresh > > [1] > https://github.com/dgibson/dtc/commit/163f0469bf2ed8b2fe5aa15bc796b93c70243ddc > [2] > https://lore.kernel.org/lkml/74f8aa8f-ffab-3b0f-186f-31fb7395e...@gmail.com/ > > Viresh Kumar (6): > scripts: dtc: Fetch fdtoverlay.c from external DTC project > scripts: dtc: Build fdtoverlay tool > scripts: dtc: Remove the unused fdtdump.c file > kbuild: Add support to build overlays (%.dtbo) > of: unittest: Create overlay_common.dtsi and testcases_common.dtsi > of: unittest: Statically apply overlays using fdtoverlay > > .gitignore| 1 + > Makefile | 5 +- > drivers/of/unittest-data/Makefile | 51 ++ > drivers/of/unittest-data/overlay_base.dts | 90 +- > drivers/of/unittest-data/overlay_common.dtsi | 91 ++ > drivers/of/unittest-data/static_base.dts | 5 + > drivers/of/unittest-data/testcases.dts| 17 +- > .../of/unittest-data/testcases_common.dtsi| 18 ++ > scripts/Makefile.dtbinst | 3 + > scripts/Makefile.lib | 5 + > scripts/dtc/Makefile | 6 +- > scripts/dtc/fdtdump.c | 163 -- > scripts/dtc/update-dtc-source.sh | 3 +- > 13 files changed, 187 insertions(+), 271 deletions(-) > create mode 100644 drivers/of/unittest-data/overlay_common.dtsi > create mode 100644 drivers/of/unittest-data/static_base.dts > create mode 100644 drivers/of/unittest-data/testcases_common.dtsi > delete mode 100644 scripts/dtc/fdtdump.c >
Re: [PATCH V4 0/3] scripts: dtc: Build fdtoverlay
Hi David, On 1/22/21 12:34 AM, David Gibson wrote: > On Wed, Jan 20, 2021 at 10:47:40AM +0530, Viresh Kumar wrote: >> +David. >> >> On 19-01-21, 11:12, Frank Rowand wrote: >>> On 1/12/21 2:28 AM, Viresh Kumar wrote: >>>> We will start building overlays for platforms soon in the kernel and >>>> would need fdtoverlay tool going forward. Lets start fetching and >>>> building it. >>>> >>>> While at it, also remove fdtdump.c file, which isn't used by the kernel. >>>> >>>> V4: >>>> - Don't fetch and build fdtdump.c >>>> - Remove fdtdump.c >>>> >>>> Viresh Kumar (3): >>>> scripts: dtc: Add fdtoverlay.c to DTC_SOURCE >>>> scripts: dtc: Build fdtoverlay tool >>>> scripts: dtc: Remove the unused fdtdump.c file >>>> >>>> scripts/dtc/Makefile | 6 +- >>>> scripts/dtc/fdtdump.c| 163 --- >>>> scripts/dtc/update-dtc-source.sh | 6 +- >>>> 3 files changed, 8 insertions(+), 167 deletions(-) >>>> delete mode 100644 scripts/dtc/fdtdump.c >>>> >>> >>> My first inclination was to accept fdtoverlay, as is, from the upstream >>> project. >>> >>> But my experiences debugging use of fdtoverlay against the existing >>> unittest overlay files has me very wary of accepting fdtoverlay in >>> it's current form. >>> >>> As an exmple, adding an overlay that fails to reply results in the >>> following build messages: >>> >>>linux--5.11-rc> make zImage >>>make[1]: Entering directory >>> '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc' >>> GEN Makefile >>> CALL >>> /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/checksyscalls.sh >>> CALL >>> /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/atomic/check-atomics.sh >>> CHK include/generated/compile.h >>> FDTOVERLAY drivers/of/unittest-data/static_test.dtb >>> >>>Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND >>>make[4]: *** >>> [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/drivers/of/unittest-data/Makefile:96: >>> drivers/of/unittest-data/static_test.dtb] Error 1 >>>make[3]: *** >>> [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: >>> drivers/of/unittest-data] Error 2 >>>make[2]: *** >>> [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: >>> drivers/of] Error 2 >>>make[1]: *** >>> [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/Makefile:1805: >>> drivers] Error 2 >>>make[1]: Leaving directory >>> '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc' >>>make: *** [Makefile:185: __sub-make] Error 2 >>> >>> >>> The specific error message (copied from above) is: >>> >>>Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND >>> >>> which is cryptic and does not even point to the location in the overlay that >>> is problematic. If you look at the source of fdtoverlay / libfdt, you will >>> find that FDT_ERR_NOTFOUND may be generated in one of many places. >>> >>> I do _not_ want to do a full review of fdtoverlay, but I think that it is >>> reasonable to request enhancing fdtoverlay in the parent project to generate >>> usable error messages before enabling fdtoverlay in the Linux kernel tree. > > That's... actually much harder than it sounds. fdtoverlay is > basically a trivial wrapper around the fdt_overlay_apply() function in > libfdt. Matching the conventions of the rest of the library, really > it's only way to report errors is a single error code. > > Returning richer errors is not an easy problem in a C library, > especially one designed to be usable in embedded systems, without an > allocator or much else available. > > Of course it would be possible to write a friendly command line tool > specifically for applying overlays, which could give better errors. > fdtoverlay as it stands isn't really that - it was pretty much written > just to invoke fdt_overlay_apply() in testcases. Thank you for providing that context. I do not know if there is a way to enable the code that is currently in libfdt to both be useful as an embedded library (for example, U-boot seems to often have a need t
Re: [PATCH V6 6/6] of: unittest: Statically apply overlays using fdtoverlay
On 1/25/21 9:15 PM, Frank Rowand wrote: > On 1/22/21 4:50 AM, 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. Create a new >> base file static_base.dts which includes other .dtsi files. >> >> 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 >> --- >> drivers/of/unittest-data/Makefile| 51 >> drivers/of/unittest-data/static_base.dts | 5 +++ >> 2 files changed, 56 insertions(+) >> create mode 100644 drivers/of/unittest-data/static_base.dts >> >> diff --git a/drivers/of/unittest-data/Makefile >> b/drivers/of/unittest-data/Makefile >> index 009f4045c8e4..586fa8cda916 100644 >> --- a/drivers/of/unittest-data/Makefile >> +++ b/drivers/of/unittest-data/Makefile >> @@ -34,7 +34,58 @@ DTC_FLAGS_overlay += -@ >> DTC_FLAGS_overlay_bad_phandle += -@ >> DTC_FLAGS_overlay_bad_symbol += -@ >> DTC_FLAGS_overlay_base += -@ >> +DTC_FLAGS_static_base += -@ >> 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 static_base.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_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.dtb \ > > rename apply_static_overlay to apply_static_overlay_2: > >apply_static_overlay_2 := overlay.dtb > > Then the remainder of apply_static_overlay becomes apply_static_overlay_1: > >apply_static_overlay_1 := >> +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)/static_base.dtb $(addprefix >> $(obj)/,$(apply_static_overlay)) >> +$(call if_changed,fdtoverlay) > > Split the static_test.dtb into _1 and _2: > >> +$(obj)/static_test_1.dtb: $(obj)/static_base_1.dtb $(addprefix >> $(obj)/,$(apply_static_overlay_1)) >> +$(call if_changed,fdtoverlay) > >> +$(obj)/static_test_2.dtb: $(obj)/static_base_2.dtb $(addprefix >> $(obj)/,$(apply_static_overlay_2)) >> +$(call if_changed,fdtoverlay) > > >> + >> +always-$(CONFIG_OF_OVERLAY) += static_test.dtb and of source the config line becomes static_test_1.dtb and static_test_2.dtb Hopefully I haven't missed any other details... -Frank > > >> diff --git a/drivers/of/unittest-data/static_base.dts >> b/drivers/of/unittest-data/static_base.dts >> new file mode 100644 >> index ..3c9af4aefb96 >> --- /dev/null >> +++ b/drivers/of/unittest-data/static_base.dt> @@ -0,0 +1,5 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/dts-v1/; >> + >> +#include "overlay_common.dtsi" >> +#include "testcases_common.dtsi" >> > > Split static_base.dts into static_base_1.dts and static_base_2.dts: > > static_base_1.dts: > >> +// SPDX-License-Identifier: GPL-2.0 >> +/dts-v1/; >> + >> +#include "testcases_common.dtsi" > > > static_base_2.dts: > >> +// SPDX-License-Identifier: GPL-2.0 >> +/dts-v1/; >> + >> +#include "overlay_common.dtsi"
Re: [PATCH V6 5/6] of: unittest: Create overlay_common.dtsi and testcases_common.dtsi
On 1/22/21 9:07 PM, David Gibson wrote: > On Fri, Jan 22, 2021 at 04:20:35PM +0530, Viresh Kumar wrote: >> In order to build-test the same unit-test files using fdtoverlay tool, >> move the device nodes from the existing overlay_base.dts and >> testcases_common.dts files to .dtsi files. The .dts files now include >> the new .dtsi files, resulting in exactly the same behavior as earlier. >> >> The .dtsi files can now be reused for compile time tests using >> fdtoverlay (will be done in a later patch). >> >> This is required because the base files passed to fdtoverlay tool >> shouldn't be overlays themselves (i.e. shouldn't have the /plugin/; >> tag). >> >> Signed-off-by: Viresh Kumar >> --- >> drivers/of/unittest-data/overlay_base.dts | 90 +- >> drivers/of/unittest-data/overlay_common.dtsi | 91 +++ >> drivers/of/unittest-data/testcases.dts| 17 +--- >> .../of/unittest-data/testcases_common.dtsi| 18 >> 4 files changed, 111 insertions(+), 105 deletions(-) >> create mode 100644 drivers/of/unittest-data/overlay_common.dtsi >> create mode 100644 drivers/of/unittest-data/testcases_common.dtsi >> >> diff --git a/drivers/of/unittest-data/overlay_base.dts >> b/drivers/of/unittest-data/overlay_base.dts >> index 99ab9d12d00b..ab9014589c5d 100644 >> --- a/drivers/of/unittest-data/overlay_base.dts >> +++ b/drivers/of/unittest-data/overlay_base.dts >> @@ -2,92 +2,4 @@ >> /dts-v1/; >> /plugin/; > > This still makes no sense to me. Is this data intended as a base > tree, or as an overlay? If it's an overlay, what are the constraints > on the base tree it's supposed to apply to. I have already replied several times that this should not make sense to anyone unless they read unittest.c and see in detail how these FDTs are abused. I have stated several times that the usage is bizarre and not normal. > > This patch is treating it as both in different places, but that's such > a bizarre usecase it needs detailed justification. It really looks > like the unittest stuff is doing some very bogus stuff that should be > fixed first, before trying to do this on top. > The unittest stuff is bizarre, but it is correct. This patch series does not alter the current usage. -Frank
Re: [PATCH V6 6/6] of: unittest: Statically apply overlays using fdtoverlay
On 1/22/21 4:50 AM, 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. Create a new > base file static_base.dts which includes other .dtsi files. > > 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 > --- > drivers/of/unittest-data/Makefile| 51 > drivers/of/unittest-data/static_base.dts | 5 +++ > 2 files changed, 56 insertions(+) > create mode 100644 drivers/of/unittest-data/static_base.dts > > diff --git a/drivers/of/unittest-data/Makefile > b/drivers/of/unittest-data/Makefile > index 009f4045c8e4..586fa8cda916 100644 > --- a/drivers/of/unittest-data/Makefile > +++ b/drivers/of/unittest-data/Makefile > @@ -34,7 +34,58 @@ DTC_FLAGS_overlay += -@ > DTC_FLAGS_overlay_bad_phandle += -@ > DTC_FLAGS_overlay_bad_symbol += -@ > DTC_FLAGS_overlay_base += -@ > +DTC_FLAGS_static_base += -@ > 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 static_base.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_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.dtb \ rename apply_static_overlay to apply_static_overlay_2: apply_static_overlay_2 := overlay.dtb Then the remainder of apply_static_overlay becomes apply_static_overlay_1: apply_static_overlay_1 := > + 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)/static_base.dtb $(addprefix > $(obj)/,$(apply_static_overlay)) > + $(call if_changed,fdtoverlay) Split the static_test.dtb into _1 and _2: > +$(obj)/static_test_1.dtb: $(obj)/static_base_1.dtb $(addprefix > $(obj)/,$(apply_static_overlay_1)) > + $(call if_changed,fdtoverlay) > +$(obj)/static_test_2.dtb: $(obj)/static_base_2.dtb $(addprefix > $(obj)/,$(apply_static_overlay_2)) > + $(call if_changed,fdtoverlay) > + > +always-$(CONFIG_OF_OVERLAY) += static_test.dtb > diff --git a/drivers/of/unittest-data/static_base.dts > b/drivers/of/unittest-data/static_base.dts > new file mode 100644 > index ..3c9af4aefb96 > --- /dev/null > +++ b/drivers/of/unittest-data/static_base.dt> @@ -0,0 +1,5 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/dts-v1/; > + > +#include "overlay_common.dtsi" > +#include "testcases_common.dtsi" > Split static_base.dts into static_base_1.dts and static_base_2.dts: static_base_1.dts: > +// SPDX-License-Identifier: GPL-2.0 > +/dts-v1/; > + > +#include "testcases_common.dtsi" static_base_2.dts: > +// SPDX-License-Identifier: GPL-2.0 > +/dts-v1/; > + > +#include "overlay_common.dtsi"
Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay
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.
>
Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay
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).
>
Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay
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. >
Re: [PATCH V5 4/5] kbuild: Add support to build overlays (%.dtbo)
On 1/20/21 10:13 PM, Viresh Kumar wrote: > On 21-01-21, 11:49, David Gibson wrote: >> If you're using overlays, you probably need the -@ flag, for both the >> base file and the overlays, which AFAICT is not already the case. > > I think the idea was to do that in the platform specific Makefiles, > unless I have misunderstood that from earlier discussions. So a > platform may want to do that per-file or just enable it for the entire > platform. > Yes, that is correct. For drivers/of/unitest-data/Makefile, we have entries like: DTC_FLAGS_overlay += -@ DTC_FLAGS_overlay_bad_phandle += -@ DTC_FLAGS_overlay_bad_symbol += -@ DTC_FLAGS_overlay_base += -@ DTC_FLAGS_testcases += -@ -Frank
Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay
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 >>> --- >>> 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
Re: [PATCH V5 5/5] of: unittest: Statically apply overlays using fdtoverlay
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 >> --- >> 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
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
+David so I don't have to repeat this in another thread On 1/19/21 11:06 PM, Viresh Kumar wrote: > On 19-01-21, 09:44, Frank Rowand wrote: >> No. overlay_base.dts is intentionally compiled into a base FDT, not >> an overlay. Unittest intentionally unflattens this FDT in early boot, >> in association with unflattening the system FDT. One key intent >> behind this is to use the same memory allocation method that is >> used for the system FDT. >> >> Do not try to convert overlay_base.dts into an overlay. > > Okay, but why does it have /plugin/; specified in it then ? OK, so I sortof lied about overlay_base.dts not being an overlay. It is a Frankenstein monster or a Schrodinger's dts/dtb. It is not a normal object. Nobody who is not looking at how it is abused inside unittest.c should be trying to touch it or understand it. unittest.c first unflattens overlay_base.dtb during early boot. Then later it does some phandle resolution using the overlay metadata from overlay_base. Then it removes the overlay metadata from the in kernel devicetree data structure. It is a hack, it is ugly, but it enables some overlay unit tests. Quit trying to change overlay_base.dts. In my suggested changes to the base patch I put overlay_base.dtb in the list of overlays for fdtoverlay to apply (apply_static_overlay in the Makefile) because overlay_base.dts is compiled as an overlay into overlay_base.dtb and it can be applied on top of the base tree testcases.dtb. This gives a little bit more testcase data for fdtoverlay from an existing dtb. If you keep trying to change overlay_base.dts I will just tell you to remove overlay_base.dtb from apply_static_overlay, and then the test coverage will become smaller. I do not see that as a good change. If you want more extensive testing of fdtoverlay, then create your own specific test cases from scratch and submit patches for them to the kernel or to the dtc compiler project. > > And shouldn't we create two separate dtb-s now, static_test.dtb and > static_overlay_test.dtb ? As fdtoverlay will not be able to merge it with > testcase.dtb anyway. > > Or maybe we can create another file static_overlay.dts (like testcases.dts) > which can include both testcases.dts and overlay_base.dts, and then we can > create static_test.dtb out of it ? That won't impact the runtime tests at all. > Stop trying to use all of the unittest .dts test data files. It is convenient that so many of them can be used in their current form. That is goodness and nice leveraging. Just ignore the .dts test data files that are not easily consumed by fdtoverlay. The email threads around the various versions of this patch series show how normal devicetree knowledgeable people look at the contents of some of the .dts test data files and think that they are incorrect. That is because the way that unittest uses them is not normal. Trying to modify one or two of the many unittest .dts test data files so that they are usable by both the static fdtoverlay and the run time unittest is not worth it. -Frank
Re: [PATCH V4 0/3] scripts: dtc: Build fdtoverlay
On 1/12/21 2:28 AM, Viresh Kumar wrote: > We will start building overlays for platforms soon in the kernel and > would need fdtoverlay tool going forward. Lets start fetching and > building it. > > While at it, also remove fdtdump.c file, which isn't used by the kernel. > > V4: > - Don't fetch and build fdtdump.c > - Remove fdtdump.c > > Viresh Kumar (3): > scripts: dtc: Add fdtoverlay.c to DTC_SOURCE > scripts: dtc: Build fdtoverlay tool > scripts: dtc: Remove the unused fdtdump.c file > > scripts/dtc/Makefile | 6 +- > scripts/dtc/fdtdump.c| 163 --- > scripts/dtc/update-dtc-source.sh | 6 +- > 3 files changed, 8 insertions(+), 167 deletions(-) > delete mode 100644 scripts/dtc/fdtdump.c > My first inclination was to accept fdtoverlay, as is, from the upstream project. But my experiences debugging use of fdtoverlay against the existing unittest overlay files has me very wary of accepting fdtoverlay in it's current form. As an exmple, adding an overlay that fails to reply results in the following build messages: linux--5.11-rc> make zImage make[1]: Entering directory '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc' GEN Makefile CALL /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/checksyscalls.sh CALL /local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/atomic/check-atomics.sh CHK include/generated/compile.h FDTOVERLAY drivers/of/unittest-data/static_test.dtb Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND make[4]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/drivers/of/unittest-data/Makefile:96: drivers/of/unittest-data/static_test.dtb] Error 1 make[3]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: drivers/of/unittest-data] Error 2 make[2]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/scripts/Makefile.build:496: drivers/of] Error 2 make[1]: *** [/local/frowand_nobackup/src/git_linus/linux--5.11-rc/Makefile:1805: drivers] Error 2 make[1]: Leaving directory '/local/frowand_nobackup/src/git_linus/build/dragon_linus_5.11-rc' make: *** [Makefile:185: __sub-make] Error 2 The specific error message (copied from above) is: Failed to apply 'drivers/of/unittest-data/overlay.dtb': FDT_ERR_NOTFOUND which is cryptic and does not even point to the location in the overlay that is problematic. If you look at the source of fdtoverlay / libfdt, you will find that FDT_ERR_NOTFOUND may be generated in one of many places. I do _not_ want to do a full review of fdtoverlay, but I think that it is reasonable to request enhancing fdtoverlay in the parent project to generate usable error messages before enabling fdtoverlay in the Linux kernel tree. fdtoverlay in it's current form adds a potential maintenance burden to me (as the overlay maintainer). I now have the experience of how difficult it was to debug the use of fdtoverlay in the context of the proposed patch to use it with the devicetree unittest overlay .dtb files. -Frank
Re: [PATCH V4 2/3] scripts: dtc: Build fdtoverlay tool
Hi Viresh, I made these comments in the v2 patch series. I am copying them here since this is the current version. On 1/12/21 2:29 AM, Viresh Kumar wrote: > We will start building overlays for platforms soon in the kernel and > would need fdtoverlay going forward. Lets start building it. > > The fdtoverlay program applies (or merges) one ore more overlay dtb > blobs to a base dtb blob. The kernel build system would later use > fdtoverlay to generate the overlaid blobs based on platform specific > configurations. > > Signed-off-by: Viresh Kumar > --- > scripts/dtc/Makefile | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile > index 4852bf44e913..5f19386a49eb 100644 > --- a/scripts/dtc/Makefile > +++ b/scripts/dtc/Makefile > @@ -1,13 +1,17 @@ > # SPDX-License-Identifier: GPL-2.0 > # scripts/dtc makefile > > -hostprogs-always-$(CONFIG_DTC) += dtc > +hostprogs-always-$(CONFIG_DTC) += dtc fdtoverlay > hostprogs-always-$(CHECK_DT_BINDING) += dtc > > dtc-objs := dtc.o flattree.o fstree.o data.o livetree.o treesource.o \ > srcpos.o checks.o util.o > dtc-objs += dtc-lexer.lex.o dtc-parser.tab.o > # The upstream project builds libfdt as a separate library. We are choosing to # instead directly link the libfdt object files into fdtoverly > +libfdt-objs := fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o > fdt_empty_tree.o fdt_addresses.o fdt_overlay.o > +libfdt = $(addprefix libfdt/,$(libfdt-objs)) > +fdtoverlay-objs := $(libfdt) fdtoverlay.o util.o > + > # Source files need to get at the userspace version of libfdt_env.h to > compile > HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt > > In general, I am a proponent of using shared libraries (which the upstream project builds by default) because if a security bug in the library is fixed, it is fixed for all users of the library. In this specific case, I actually prefer the implementation that the patch provides (directly linking the library object files into fdtoverlay, which uses the library) because it is the only user of the library _and_ fdtoverlay will not inadvertently use the system wide libfdt if it happens to be installed (as it is on my system). Any thoughts on this Rob? -Frank
Re: [PATCH V2 2/2] scripts: dtc: Build fdtoverlay and fdtdump tools
On 1/19/21 10:28 AM, Frank Rowand wrote: > On 1/6/21 11:15 PM, Viresh Kumar wrote: >> We will start building overlays for platforms soon in the kernel and >> would need these tools going forward. Lets start building them. >> >> Signed-off-by: Viresh Kumar >> --- >> scripts/dtc/Makefile | 8 +++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile >> index 4852bf44e913..c607980a5c17 100644 >> --- a/scripts/dtc/Makefile >> +++ b/scripts/dtc/Makefile >> @@ -1,12 +1,18 @@ >> # SPDX-License-Identifier: GPL-2.0 >> # scripts/dtc makefile >> >> -hostprogs-always-$(CONFIG_DTC) += dtc >> +hostprogs-always-$(CONFIG_DTC) += dtc fdtdump fdtoverlay >> hostprogs-always-$(CHECK_DT_BINDING)+= dtc >> >> dtc-objs:= dtc.o flattree.o fstree.o data.o livetree.o treesource.o \ >> srcpos.o checks.o util.o >> dtc-objs+= dtc-lexer.lex.o dtc-parser.tab.o >> +fdtdump-objs:= fdtdump.o util.o >> + > > # The upstream project builds libfdt as a separate library. We are choosing > to > # instead directly link the libfdt object files into fdtoverly > >> +libfdt_dir = libfdt >> +libfdt-objs := fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o >> fdt_empty_tree.o fdt_addresses.o fdt_overlay.o >> +libfdt = $(addprefix $(libfdt_dir)/,$(libfdt-objs)) >> +fdtoverlay-objs := $(libfdt) fdtoverlay.o util.o >> >> # Source files need to get at the userspace version of libfdt_env.h to >> compile >> HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt >> > > In general, I am a proponent of using shared libraries (which the upstream > project > builds by default) because if a security bug in the library is fixed, it is > fixed > for all users of the library. > > In this specific case, I actually prefer the implementation that the patch > provides > (directly linking the library object files into fdtoverlay, which uses the > library) > because it is the only user of the library _and_ fdtoverlay will not > inadvertently > use the system wide libfdt if it happens to be installed (as it is on my > system). > > Any thoughts on this Rob? I see that this patch series is up to v4, so I commented in the wrong place. I will repeat this comment in the v4 series. > > -Frank >
Re: [PATCH V4 1/3] scripts: dtc: Add fdtoverlay.c to DTC_SOURCE
On 1/12/21 2:29 AM, Viresh Kumar wrote: > We will start building overlays for platforms soon in the kernel and > would need fdtoverlay tool going forward. Lets start fetching it. > > Signed-off-by: Viresh Kumar > --- > scripts/dtc/update-dtc-source.sh | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/scripts/dtc/update-dtc-source.sh > b/scripts/dtc/update-dtc-source.sh > index bc704e2a6a4a..f1c802011e1e 100755 > --- a/scripts/dtc/update-dtc-source.sh > +++ b/scripts/dtc/update-dtc-source.sh > @@ -31,9 +31,9 @@ set -ev > DTC_UPSTREAM_PATH=`pwd`/../dtc > DTC_LINUX_PATH=`pwd`/scripts/dtc > > -DTC_SOURCE="checks.c data.c dtc.c dtc.h flattree.c fstree.c livetree.c > srcpos.c \ > - srcpos.h treesource.c util.c util.h version_gen.h yamltree.c \ > - dtc-lexer.l dtc-parser.y" > +DTC_SOURCE="checks.c data.c dtc.c dtc.h fdtoverlay.c flattree.c fstree.c \ > + livetree.c srcpos.c srcpos.h treesource.c util.c \ > + util.h version_gen.h yamltree.c dtc-lexer.l dtc-parser.y" > LIBFDT_SOURCE="fdt.c fdt.h fdt_addresses.c fdt_empty_tree.c \ > fdt_overlay.c fdt_ro.c fdt_rw.c fdt_strerror.c fdt_sw.c \ > fdt_wip.c libfdt.h libfdt_env.h libfdt_internal.h" > I made this comment in the v2 email thread. Copying it here since v4 is the current version of the patch series. DTC_SOURCE is for the dtc program. Please add a FDTOVERLAY_SOURCE and related use for the fdtoverlay program. -Frank
Re: [PATCH V2 1/2] scripts: dtc: Add fdtoverlay.c and fdtdump.c to DTC_SOURCE
On 1/19/21 10:21 AM, Frank Rowand wrote: > On 1/6/21 11:15 PM, Viresh Kumar wrote: >> We will start building overlays for platforms soon in the kernel and >> would need these tools going forward. Lets start fetching them. >> >> Note that a copy of fdtdump.c was already copied back in the year 2012, >> but was never updated or built for some reason. >> >> Signed-off-by: Viresh Kumar >> --- >> V2: Separate out this change from Makefile one. >> >> This needs to be followed by invocation of the ./update-dtc-source.sh >> script so the relevant files can be copied before the Makefile is >> updated in the next patch. >> >> scripts/dtc/update-dtc-source.sh | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/scripts/dtc/update-dtc-source.sh >> b/scripts/dtc/update-dtc-source.sh >> index bc704e2a6a4a..9bc4afb71415 100755 >> --- a/scripts/dtc/update-dtc-source.sh >> +++ b/scripts/dtc/update-dtc-source.sh >> @@ -31,9 +31,9 @@ set -ev >> DTC_UPSTREAM_PATH=`pwd`/../dtc >> DTC_LINUX_PATH=`pwd`/scripts/dtc >> >> -DTC_SOURCE="checks.c data.c dtc.c dtc.h flattree.c fstree.c livetree.c >> srcpos.c \ >> -srcpos.h treesource.c util.c util.h version_gen.h yamltree.c \ >> -dtc-lexer.l dtc-parser.y" >> +DTC_SOURCE="checks.c data.c dtc.c dtc.h fdtdump.c fdtoverlay.c flattree.c \ >> +fstree.c livetree.c srcpos.c srcpos.h treesource.c util.c \ >> +util.h version_gen.h yamltree.c dtc-lexer.l dtc-parser.y" >> LIBFDT_SOURCE="fdt.c fdt.h fdt_addresses.c fdt_empty_tree.c \ >> fdt_overlay.c fdt_ro.c fdt_rw.c fdt_strerror.c fdt_sw.c \ >> fdt_wip.c libfdt.h libfdt_env.h libfdt_internal.h" >> > > DTC_SOURCE is for the dtc program. Please add a FDTOVERLAY_SOURCE and > related use for the fdtoverlay program. I see that this patch series is up to v4, so I commented in the wrong place. I will repeat this comment in the v4 series. -Frank > > -Frank >
Re: [PATCH V2 2/2] scripts: dtc: Build fdtoverlay and fdtdump tools
On 1/6/21 11:15 PM, Viresh Kumar wrote: > We will start building overlays for platforms soon in the kernel and > would need these tools going forward. Lets start building them. > > Signed-off-by: Viresh Kumar > --- > scripts/dtc/Makefile | 8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile > index 4852bf44e913..c607980a5c17 100644 > --- a/scripts/dtc/Makefile > +++ b/scripts/dtc/Makefile > @@ -1,12 +1,18 @@ > # SPDX-License-Identifier: GPL-2.0 > # scripts/dtc makefile > > -hostprogs-always-$(CONFIG_DTC) += dtc > +hostprogs-always-$(CONFIG_DTC) += dtc fdtdump fdtoverlay > hostprogs-always-$(CHECK_DT_BINDING) += dtc > > dtc-objs := dtc.o flattree.o fstree.o data.o livetree.o treesource.o \ > srcpos.o checks.o util.o > dtc-objs += dtc-lexer.lex.o dtc-parser.tab.o > +fdtdump-objs := fdtdump.o util.o > + # The upstream project builds libfdt as a separate library. We are choosing to # instead directly link the libfdt object files into fdtoverly > +libfdt_dir = libfdt > +libfdt-objs := fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o > fdt_empty_tree.o fdt_addresses.o fdt_overlay.o > +libfdt = $(addprefix $(libfdt_dir)/,$(libfdt-objs)) > +fdtoverlay-objs := $(libfdt) fdtoverlay.o util.o > > # Source files need to get at the userspace version of libfdt_env.h to > compile > HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt > In general, I am a proponent of using shared libraries (which the upstream project builds by default) because if a security bug in the library is fixed, it is fixed for all users of the library. In this specific case, I actually prefer the implementation that the patch provides (directly linking the library object files into fdtoverlay, which uses the library) because it is the only user of the library _and_ fdtoverlay will not inadvertently use the system wide libfdt if it happens to be installed (as it is on my system). Any thoughts on this Rob? -Frank
Re: [PATCH V2 1/2] scripts: dtc: Add fdtoverlay.c and fdtdump.c to DTC_SOURCE
On 1/6/21 11:15 PM, Viresh Kumar wrote: > We will start building overlays for platforms soon in the kernel and > would need these tools going forward. Lets start fetching them. > > Note that a copy of fdtdump.c was already copied back in the year 2012, > but was never updated or built for some reason. > > Signed-off-by: Viresh Kumar > --- > V2: Separate out this change from Makefile one. > > This needs to be followed by invocation of the ./update-dtc-source.sh > script so the relevant files can be copied before the Makefile is > updated in the next patch. > > scripts/dtc/update-dtc-source.sh | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/scripts/dtc/update-dtc-source.sh > b/scripts/dtc/update-dtc-source.sh > index bc704e2a6a4a..9bc4afb71415 100755 > --- a/scripts/dtc/update-dtc-source.sh > +++ b/scripts/dtc/update-dtc-source.sh > @@ -31,9 +31,9 @@ set -ev > DTC_UPSTREAM_PATH=`pwd`/../dtc > DTC_LINUX_PATH=`pwd`/scripts/dtc > > -DTC_SOURCE="checks.c data.c dtc.c dtc.h flattree.c fstree.c livetree.c > srcpos.c \ > - srcpos.h treesource.c util.c util.h version_gen.h yamltree.c \ > - dtc-lexer.l dtc-parser.y" > +DTC_SOURCE="checks.c data.c dtc.c dtc.h fdtdump.c fdtoverlay.c flattree.c \ > + fstree.c livetree.c srcpos.c srcpos.h treesource.c util.c \ > + util.h version_gen.h yamltree.c dtc-lexer.l dtc-parser.y" > LIBFDT_SOURCE="fdt.c fdt.h fdt_addresses.c fdt_empty_tree.c \ > fdt_overlay.c fdt_ro.c fdt_rw.c fdt_strerror.c fdt_sw.c \ > fdt_wip.c libfdt.h libfdt_env.h libfdt_internal.h" > DTC_SOURCE is for the dtc program. Please add a FDTOVERLAY_SOURCE and related use for the fdtoverlay program. -Frank
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/19/21 2:05 AM, Viresh Kumar wrote: > On 18-01-21, 20:21, frowand.l...@gmail.com wrote: >> From: Frank Rowand >> >> These changes apply on top of the patches in: >> >> [PATCH] of: unittest: Statically apply overlays using fdtoverlay >> Message-Id: >> <1e42183ccafa1afba33b3e79a4e3efd3329fd133.1610095159.git.viresh.ku...@linaro.org> >> >> There are still some issues to be cleaned up, so not ready for acceptance. > > Are you talking about the missing __overlay__ thing ? (more below) No. I am referencing my comments below (I'll copy them up here): I have not used the construct "always-$(CONFIG_OF_OVERLAY)" before, and have not looked into the proper usage of it. [Tested using my own fdtoverlay instead of the one supplied by your patches that added fdtoverlay and fdtdump to the kernel tree.] I have not run this through checkpatch, or my checks for build warnings. I have not run unittests on my target with these patches applied. I will have to get the updated patch, test it more fully, and fill in a gap in my knowledge (use of "always-$(CONFIG_xxx)". > >> I have not used the construct "always-$(CONFIG_OF_OVERLAY)" before, and >> have not looked into the proper usage of it. > > I wasn't sure either, maybe Masahiro can suggest the best fit. > >> I tested this using a hand build libfdt and fdtoverlay from the dtc-compiler >> upstream project. For my testing I added LD_LIBRARY_PATH to the body of >> "cmd_fdtoverlay" to reference my hand built libfdt. The kernel build >> system will have to instead use a libfdt that is built in the kernel >> tree. > > I tested it with this patchset: > > https://lore.kernel.org/lkml/cover.1610431620.git.viresh.ku...@linaro.org/ > >> I have not run this through checkpatch, or my checks for build warnings. >> I have not run unittests on my target with these patches applied. >> >> --- >> drivers/of/unittest-data/Makefile | 67 ++- >> 1 file changed, 48 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/of/unittest-data/Makefile >> b/drivers/of/unittest-data/Makefile >> index f17bce85f65f..28614a123d1e 100644 >> --- a/drivers/of/unittest-data/Makefile >> +++ b/drivers/of/unittest-data/Makefile >> @@ -39,25 +39,54 @@ DTC_FLAGS_testcases += -@ >> # suppress warnings about intentional errors >> DTC_FLAGS_testcases += -Wno-interrupts_property >> >> -# Apply overlays statically with fdtoverlay >> -intermediate-overlay:= overlay.dtb >> -master := 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 \ >> - intermediate-overlay.dtb >> - >> -quiet_cmd_fdtoverlay = fdtoverlay $@ >> - cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^ >> - >> -$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix >> $(obj)/,$(intermediate-overlay)) >> -$(call if_changed,fdtoverlay) >> +# 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 \ >> + >> +apply_static_overlay := overlay_base.dtb \ > > This won't work because of the issues I ment
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/17/21 9:54 PM, Frank Rowand wrote:
> Hi Viresh,
>
> On 1/14/21 11:44 PM, Viresh Kumar wrote:
>> +David,
>>
>> On 14-01-21, 09:01, Rob Herring wrote:
>>> On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar
>>> wrote:
>>>>
>>>> On 11-01-21, 09:46, Rob Herring wrote:
>>>>> On Fri, Jan 8, 2021 at 2:41 AM 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.
>>>>>
>>>>> Nice idea.
>>>>>
>>>>>> The file overlay_base.dtb have symbols of its own and we need to apply
>>>>>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives
>>>>>> us intermediate-overlay.dtb file.
>>>>>
>>>>> Okay? If restructuring things helps we should do that. Frank?
>>>>
>>>> Frank, do we want to do something about it ? Maybe make overlay_base.dts
>>>> an dtsi
>>>> and include it from testcases.dts like the other ones ?
>
> I was not able to look at this until tonight. The unittest world is somewhat
> convoluted and complex. Not at all a normal OF environment since it is
> directly
> using both dynamic OF code and overlay apply/remove code. Not to mention
> deliberately misformed devicetree (.dts) data. And totally hacking the
> loading
> of FDTs in additional ways.
>
> It is late Sunday night here (almost 10:00pm), so I am going to look at this
> first thing Monday morning.
I sent comments in the form of a patch to the original patch email.
-Frank
>
>>>
>>> No, because overlay_base.dts is an overlay dt.
>>
>> What property of a file makes it an overlay ? Just the presence of /plugin/;
>> ?
>
> The "/plugin/;" in a dts file is what tells the dtc compiler to process the
> source
> file as an overlay instead of as a base.
>
>>
>> David, we are talking about the overlay base[1] file here. The fdtoverlay
>> tool
>> fails to apply it to testcases.dts file (in the same directory) because none
>> of
>> its nodes have the __overlay__ label and the dtc routine overlay_merge() [2]
>> skips them intentionally.
>>
>>> I think we need an
>>> empty, minimal base.dtb to apply overlay_base.dtbo to.
>>
>> One way out is adding an (almost-empty) testcase-data-2 in testcases.dtb,
>> that
>> will make it work.
>>
>>> And then fdtoverlay needs a fix to apply overlays to the root node?
>>
>> It isn't just root node I think, but any node for which the __overlay__ label
>> isn't there.
>>
>> So this can make it all work if everyone is fine with it:
>
> I'll look this over Monday morning to see what the side effects are in the
> bizarre world of unittest.
>
>>
>> diff --git a/drivers/of/unittest-data/overlay_base.dts
>> b/drivers/of/unittest-data/overlay_base.dts
>> index 99ab9d12d00b..59172c4c9e5a 100644
>> --- a/drivers/of/unittest-data/overlay_base.dts
>> +++ b/drivers/of/unittest-data/overlay_base.dts
>> @@ -11,8 +11,7 @@
>> * dtc will create nodes "/__symbols__" and "/__local_fixups__".
>> */
>>
>> -/ {
>> - testcase-data-2 {
>> + _base {
>> #address-cells = <1>;
>> #size-cells = <1>;
>>
>> @@ -89,5 +88,3 @@ retail_1: vending@5 {
>> };
>>
>> };
>> -};
>> -
>> diff --git a/drivers/of/unittest-data/testcases.dts
>> b/drivers/of/unittest-data/testcases.dts
>> index a85b5e1c381a..539dc7d9eddc 100644
>> --- a/drivers/of/unittest-data/testcases.dts
>> +++ b/drivers/of/unittest-data/testcases.dts
>> @@ -11,6 +11,11 @@ node-remove {
>> };
>> };
>> };
>> +
>> + overlay_base: testcase-data-2 {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + };
>>
>> -8<-
>>
>> And then we can do this to the Makefile over my changes.
>>
>> -8<-
>>
>> diff --git a/drivers/of/unittest-data/Makefile
>> b/drivers/of/unittest-data/Makefile
>> index 9f3eb30b78f1..8cc23311b778 100644
>> --- a/drivers/of/unittest-data/Makefile
>> +++ b/drivers/of/unittest-data/Make
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/18/21 12:30 AM, David Gibson wrote:
> On Fri, Jan 15, 2021 at 11:14:50AM +0530, Viresh Kumar wrote:
>> +David,
>>
>> On 14-01-21, 09:01, Rob Herring wrote:
>>> On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar
>>> wrote:
On 11-01-21, 09:46, Rob Herring wrote:
> On Fri, Jan 8, 2021 at 2:41 AM 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.
>
> Nice idea.
>
>> The file overlay_base.dtb have symbols of its own and we need to apply
>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives
>> us intermediate-overlay.dtb file.
>
> Okay? If restructuring things helps we should do that. Frank?
Frank, do we want to do something about it ? Maybe make overlay_base.dts
an dtsi
and include it from testcases.dts like the other ones ?
>>>
>>> No, because overlay_base.dts is an overlay dt.
>
> So.. I was confused for a bit here, because the overlay_base.dts
> you're discussing is the one in the kernel tree, not the one in the
> dtc tree.
>
> The kernel file is confusing to me. It has the /plugin/ tag which
It should be confusing to you, without having dug deeply into the
evil things that unittest does. Unittest does a bunch of contortions
and abuse of direct access into the kernel devicetree code internals
to be able to create and test for abnormal conditions. No other code
should emulate the bizarre things that unittest does.
-Frank
> should be used for overlays, but the rest of the file looks like a
> base tree rather than an overlay. There's even a comment saying "Base
> device tree that overlays will be applied against". But it goes on to
> talk about __fixups__ and __local__fixups__ which will never be
> generated for a based tree.
>
> Oh.. and then there's terminology confusion. dtc has had compile time
> resolved overlays for a very long time. Those are usually .dtsi
> files, and should generally not have /plugin/.
>
> More recent is the support for run-time overlays - .dtbo output files,
> which are usually generated from a .dts with /plugin/. They usually
> should *not* have a "/ { ... } " stanza, since that indicates the base
> portion of the tree.
>
>> What property of a file makes it an overlay ? Just the presence of
>> /plugin/; ?
>
> Yes and no. Generally that's how it should work , but it looks to me
> like the presence of /plugin/ there is just wrong. /plugin/ basically
> just activates some extra dtc features, though, so it is possible to
> "manually" construct a valid overlay without it.
>
>> David, we are talking about the overlay base[1] file here. The
>> fdtoverlay tool
>> fails to apply it to testcases.dts file (in the same directory) because none
>> of
>> its nodes have the __overlay__ label and the dtc routine overlay_merge() [2]
>> skips them intentionally.
>
> testcases.dts also has /plugin/ and again, it's not really clear if
> that's right.
>
> The point of /plugin/ is that it will automatically generate the
> __overlay__ subnodes from dtc's { ... } or &{/path} { ... }
> syntax, so you wouldn't expect to see __overlay__ in the source.
>
>>> I think we need an
>>> empty, minimal base.dtb to apply overlay_base.dtbo to.
>>
>> One way out is adding an (almost-empty) testcase-data-2 in testcases.dtb,
>> that
>> will make it work.
>>
>>> And then fdtoverlay needs a fix to apply overlays to the root node?
>>
>> It isn't just root node I think, but any node for which the __overlay__ label
>> isn't there.
>>
>> So this can make it all work if everyone is fine with it:
>>
>> diff --git a/drivers/of/unittest-data/overlay_base.dts
>> b/drivers/of/unittest-data/overlay_base.dts
>> index 99ab9d12d00b..59172c4c9e5a 100644
>> --- a/drivers/of/unittest-data/overlay_base.dts
>> +++ b/drivers/of/unittest-data/overlay_base.dts
>> @@ -11,8 +11,7 @@
>> * dtc will create nodes "/__symbols__" and "/__local_fixups__".
>> */
>>
>> -/ {
>> - testcase-data-2 {
>> + _base {
>> #address-cells = <1>;
>> #size-cells = <1>;
>>
>> @@ -89,5 +88,3 @@ retail_1: vending@5 {
>> };
>>
>> };
>> -};
>> -
>> diff --git a/drivers/of/unittest-data/testcases.dts
>> b/drivers/of/unittest-data/testcases.dts
>> index a85b5e1c381a..539dc7d9eddc 100644
>> --- a/drivers/of/unittest-data/testcases.dts
>> +++ b/drivers/of/unittest-data/testcases.dts
>> @@ -11,6 +11,11 @@ node-remove {
>> };
>> };
>> };
>> +
>> + overlay_base: testcase-data-2 {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + };
>>
>> -8<-
>>
>> And then we can do this to the Makefile over my changes.
>>
>> -8<-
>>
>> diff --git a/drivers/of/unittest-data/Makefile
>>
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/13/21 11:00 PM, Viresh Kumar wrote: > Frank/Rob. > > On 08-01-21, 14:11, Viresh Kumar wrote: >> diff --git a/drivers/of/unittest-data/Makefile >> b/drivers/of/unittest-data/Makefile >> index 009f4045c8e4..f17bce85f65f 100644 >> --- a/drivers/of/unittest-data/Makefile >> +++ b/drivers/of/unittest-data/Makefile >> @@ -38,3 +38,26 @@ DTC_FLAGS_testcases += -@ >> >> # suppress warnings about intentional errors >> DTC_FLAGS_testcases += -Wno-interrupts_property >> + >> +# Apply overlays statically with fdtoverlay > > I will update this part to mention about the dtbs we are not using in the > build > as they will fail (as per Frank's comment). > > Is there anything else you guys want me to change before I send the next > version > ? I sent some changes in the form of a patch, in reply to your original patch. If you can fold the changes into your patch, and look into the comments that I put into the patch, that would be great. -Frank > >> +intermediate-overlay:= overlay.dtb >> +master := 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 \ >> + intermediate-overlay.dtb >> + >> +quiet_cmd_fdtoverlay = fdtoverlay $@ >> + cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^ >> + >> +$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix >> $(obj)/,$(intermediate-overlay)) >> +$(call if_changed,fdtoverlay) >> + >> +$(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master)) >> +$(call if_changed,fdtoverlay) >> + >> +always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb >
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
Hi Viresh,
On 1/14/21 11:44 PM, Viresh Kumar wrote:
> +David,
>
> On 14-01-21, 09:01, Rob Herring wrote:
>> On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar
>> wrote:
>>>
>>> On 11-01-21, 09:46, Rob Herring wrote:
On Fri, Jan 8, 2021 at 2:41 AM 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.
Nice idea.
> The file overlay_base.dtb have symbols of its own and we need to apply
> overlay.dtb to overlay_base.dtb alone first to make it work, which gives
> us intermediate-overlay.dtb file.
Okay? If restructuring things helps we should do that. Frank?
>>>
>>> Frank, do we want to do something about it ? Maybe make overlay_base.dts an
>>> dtsi
>>> and include it from testcases.dts like the other ones ?
I was not able to look at this until tonight. The unittest world is somewhat
convoluted and complex. Not at all a normal OF environment since it is directly
using both dynamic OF code and overlay apply/remove code. Not to mention
deliberately misformed devicetree (.dts) data. And totally hacking the loading
of FDTs in additional ways.
It is late Sunday night here (almost 10:00pm), so I am going to look at this
first thing Monday morning.
>>
>> No, because overlay_base.dts is an overlay dt.
>
> What property of a file makes it an overlay ? Just the presence of /plugin/; ?
The "/plugin/;" in a dts file is what tells the dtc compiler to process the
source
file as an overlay instead of as a base.
>
> David, we are talking about the overlay base[1] file here. The fdtoverlay tool
> fails to apply it to testcases.dts file (in the same directory) because none
> of
> its nodes have the __overlay__ label and the dtc routine overlay_merge() [2]
> skips them intentionally.
>
>> I think we need an
>> empty, minimal base.dtb to apply overlay_base.dtbo to.
>
> One way out is adding an (almost-empty) testcase-data-2 in testcases.dtb, that
> will make it work.
>
>> And then fdtoverlay needs a fix to apply overlays to the root node?
>
> It isn't just root node I think, but any node for which the __overlay__ label
> isn't there.
>
> So this can make it all work if everyone is fine with it:
I'll look this over Monday morning to see what the side effects are in the
bizarre world of unittest.
>
> diff --git a/drivers/of/unittest-data/overlay_base.dts
> b/drivers/of/unittest-data/overlay_base.dts
> index 99ab9d12d00b..59172c4c9e5a 100644
> --- a/drivers/of/unittest-data/overlay_base.dts
> +++ b/drivers/of/unittest-data/overlay_base.dts
> @@ -11,8 +11,7 @@
> * dtc will create nodes "/__symbols__" and "/__local_fixups__".
> */
>
> -/ {
> - testcase-data-2 {
> + _base {
> #address-cells = <1>;
> #size-cells = <1>;
>
> @@ -89,5 +88,3 @@ retail_1: vending@5 {
> };
>
> };
> -};
> -
> diff --git a/drivers/of/unittest-data/testcases.dts
> b/drivers/of/unittest-data/testcases.dts
> index a85b5e1c381a..539dc7d9eddc 100644
> --- a/drivers/of/unittest-data/testcases.dts
> +++ b/drivers/of/unittest-data/testcases.dts
> @@ -11,6 +11,11 @@ node-remove {
> };
> };
> };
> +
> + overlay_base: testcase-data-2 {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + };
>
> -8<-
>
> And then we can do this to the Makefile over my changes.
>
> -8<-
>
> diff --git a/drivers/of/unittest-data/Makefile
> b/drivers/of/unittest-data/Makefile
> index 9f3eb30b78f1..8cc23311b778 100644
> --- a/drivers/of/unittest-data/Makefile
> +++ b/drivers/of/unittest-data/Makefile
> @@ -41,7 +41,6 @@ DTC_FLAGS_testcases += -Wno-interrupts_property
>
> # Apply all overlays (except overlay_bad_* as they are not supposed to apply
> and
> # fail build) statically with fdtoverlay
> -intermediate-overlay := overlay.dtb
> master := 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 \
> @@ -50,15 +49,12 @@ master := overlay_0.dtb
> overlay_1.dtb overlay_2.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 \
> - intermediate-overlay.dtb
> + overlay_base.dtb overlay.dtb
>
> quiet_cmd_fdtoverlay = fdtoverlay $@
>cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
>
> -$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix
> $(obj)/,$(intermediate-overlay))
> - $(call if_changed,fdtoverlay)
> -
>
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/13/21 9:05 AM, Rob Herring wrote: > On Tue, Jan 12, 2021 at 8:20 PM Frank Rowand wrote: >> >> On 1/12/21 2:46 PM, Rob Herring wrote: >>> On Tue, Jan 12, 2021 at 2:05 PM Frank Rowand wrote: >>>> >>>> On 1/12/21 1:41 PM, Rob Herring wrote: >>>>> On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand >>>>> wrote: >>>>>> >>>>>> On 1/12/21 8:04 AM, Rob Herring wrote: >>>>>>> On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand >>>>>>> wrote: >>>>>>>> >>>>>>>> On 1/8/21 2:41 AM, 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. >>>>>>>>> >>>>>>>>> The file overlay_base.dtb have symbols of its own and we need to apply >>>>>>>>> overlay.dtb to overlay_base.dtb alone first to make it work, which >>>>>>>>> gives >>>>>>>>> us intermediate-overlay.dtb file. >>>>>>>>> >>>>>>>>> The intermediate-overlay.dtb file along with all other overlays is >>>>>>>>> them >>>>>>>>> applied to testcases.dtb to generate the master.dtb file. >>>>>>>>> >>>>>>>>> Signed-off-by: Viresh Kumar >>>>>>>> >>>>>>>> NACK to this specific patch, in its current form. >>>>>>>> >>>>>>>> There are restrictions on applying an overlay at runtime that do not >>>>>>>> apply >>>>>>>> to applying an overlay to an FDT that will be loaded by the kernel >>>>>>>> during >>>>>>>> early boot. Thus the unittest overlays _must_ be applied using the >>>>>>>> kernel >>>>>>>> overlay loading methods to test the kernel runtime overlay loading >>>>>>>> feature. >>>>>>> >>>>>>> This patch doesn't take away from any of that and it completely >>>>>>> orthogonal. >>>>>> >>>>>> Mea culpa. I took the patch header comment at face value, and read more >>>>>> into >>>>>> the header comment than what was written there. I then skimmed the patch >>>>>> instead of actually reading what it was doing. >>>>>> >>>>>> I incorrectly _assumed_ (bad!) that the intent was to replace applying >>>>>> the >>>>>> individual overlay dtb's with the master.dtb. Reading more closely, I >>>>>> see >>>>>> that the assumed final step of actually _using_ master.dtb does not >>>>>> exist. >>>>>> >>>>>> So, yes, I agree that the patch as written is orthogonal to my concern. >>>>>> >>>>>> My updated understanding is that this patch is attempting to use the >>>>>> existing >>>>>> unittest overlay dts files as source to test fdtoverlay. And that the >>>>>> resulting >>>>>> dtb from fdtoverlay is not intended to be consumed by the kernel >>>>>> unittest. >>>>> >>>>> The goal is not to test fdtoverlay. dtc unittests do that. The goal is >>>>> testing overlays we expect to be able to apply can actually apply and >>>>> doing this at build time. That's also the goal for all the 'real' >>>>> overlays which get added. >>>>> >>>>>> I do not agree that this is a good approach to testing fdtoverlay. The >>>>>> unittest overlay dts files are constructed specifically to test various >>>>>> parts of the kernel overlay code and dynamic OF code. Some of the >>>>>> content >>>>>> of the overlays is constructed to trigger error conditions in that code, >>>>>> and thus will not be able to be processed without error by fdtoverlay. >>>>> >>>>> Then those should be omitted. >>>>> >>>>>> Trying to use overlay dts files that are constructed to test runtime >>>>>> kernel >>>>>> code as fdtoverlay input data mixes two differen
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/12/21 2:46 PM, Rob Herring wrote: > On Tue, Jan 12, 2021 at 2:05 PM Frank Rowand wrote: >> >> On 1/12/21 1:41 PM, Rob Herring wrote: >>> On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand wrote: >>>> >>>> On 1/12/21 8:04 AM, Rob Herring wrote: >>>>> On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand >>>>> wrote: >>>>>> >>>>>> On 1/8/21 2:41 AM, 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. >>>>>>> >>>>>>> The file overlay_base.dtb have symbols of its own and we need to apply >>>>>>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives >>>>>>> us intermediate-overlay.dtb file. >>>>>>> >>>>>>> The intermediate-overlay.dtb file along with all other overlays is them >>>>>>> applied to testcases.dtb to generate the master.dtb file. >>>>>>> >>>>>>> Signed-off-by: Viresh Kumar >>>>>> >>>>>> NACK to this specific patch, in its current form. >>>>>> >>>>>> There are restrictions on applying an overlay at runtime that do not >>>>>> apply >>>>>> to applying an overlay to an FDT that will be loaded by the kernel during >>>>>> early boot. Thus the unittest overlays _must_ be applied using the >>>>>> kernel >>>>>> overlay loading methods to test the kernel runtime overlay loading >>>>>> feature. >>>>> >>>>> This patch doesn't take away from any of that and it completely >>>>> orthogonal. >>>> >>>> Mea culpa. I took the patch header comment at face value, and read more >>>> into >>>> the header comment than what was written there. I then skimmed the patch >>>> instead of actually reading what it was doing. >>>> >>>> I incorrectly _assumed_ (bad!) that the intent was to replace applying the >>>> individual overlay dtb's with the master.dtb. Reading more closely, I see >>>> that the assumed final step of actually _using_ master.dtb does not exist. >>>> >>>> So, yes, I agree that the patch as written is orthogonal to my concern. >>>> >>>> My updated understanding is that this patch is attempting to use the >>>> existing >>>> unittest overlay dts files as source to test fdtoverlay. And that the >>>> resulting >>>> dtb from fdtoverlay is not intended to be consumed by the kernel unittest. >>> >>> The goal is not to test fdtoverlay. dtc unittests do that. The goal is >>> testing overlays we expect to be able to apply can actually apply and >>> doing this at build time. That's also the goal for all the 'real' >>> overlays which get added. >>> >>>> I do not agree that this is a good approach to testing fdtoverlay. The >>>> unittest overlay dts files are constructed specifically to test various >>>> parts of the kernel overlay code and dynamic OF code. Some of the content >>>> of the overlays is constructed to trigger error conditions in that code, >>>> and thus will not be able to be processed without error by fdtoverlay. >>> >>> Then those should be omitted. >>> >>>> Trying to use overlay dts files that are constructed to test runtime kernel >>>> code as fdtoverlay input data mixes two different test environments and >>>> objectives. If fdtoverlay test cases are desired, then fdtoverlay specific >>>> dts files should be created. >>>> >>>>> >>>>>> I agree that testing fdtoverlay is a good idea. I have not looked at the >>>>>> parent project to see how much testing of fdtoverlay occurs there, but I >>>>>> would prefer that fdtoverlay tests reside in the parent project if >>>>>> practical >>>>>> and reasonable. If there is some reason that some fdtoverlay tests are >>>>>> more practical in the Linux kernel repository then I am open to adding >>>>>> them to the Linux kernel tree. >>>>> >>>>> If you (or more importantly someone else sending us patches) make >>>>> changes to the overlays, you can test that they apply at build time >>>
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/12/21 1:41 PM, Rob Herring wrote: > On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand wrote: >> >> On 1/12/21 8:04 AM, Rob Herring wrote: >>> On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand wrote: >>>> >>>> On 1/8/21 2:41 AM, 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. >>>>> >>>>> The file overlay_base.dtb have symbols of its own and we need to apply >>>>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives >>>>> us intermediate-overlay.dtb file. >>>>> >>>>> The intermediate-overlay.dtb file along with all other overlays is them >>>>> applied to testcases.dtb to generate the master.dtb file. >>>>> >>>>> Signed-off-by: Viresh Kumar >>>> >>>> NACK to this specific patch, in its current form. >>>> >>>> There are restrictions on applying an overlay at runtime that do not apply >>>> to applying an overlay to an FDT that will be loaded by the kernel during >>>> early boot. Thus the unittest overlays _must_ be applied using the kernel >>>> overlay loading methods to test the kernel runtime overlay loading feature. >>> >>> This patch doesn't take away from any of that and it completely orthogonal. >> >> Mea culpa. I took the patch header comment at face value, and read more into >> the header comment than what was written there. I then skimmed the patch >> instead of actually reading what it was doing. >> >> I incorrectly _assumed_ (bad!) that the intent was to replace applying the >> individual overlay dtb's with the master.dtb. Reading more closely, I see >> that the assumed final step of actually _using_ master.dtb does not exist. >> >> So, yes, I agree that the patch as written is orthogonal to my concern. >> >> My updated understanding is that this patch is attempting to use the existing >> unittest overlay dts files as source to test fdtoverlay. And that the >> resulting >> dtb from fdtoverlay is not intended to be consumed by the kernel unittest. > > The goal is not to test fdtoverlay. dtc unittests do that. The goal is > testing overlays we expect to be able to apply can actually apply and > doing this at build time. That's also the goal for all the 'real' > overlays which get added. > >> I do not agree that this is a good approach to testing fdtoverlay. The >> unittest overlay dts files are constructed specifically to test various >> parts of the kernel overlay code and dynamic OF code. Some of the content >> of the overlays is constructed to trigger error conditions in that code, >> and thus will not be able to be processed without error by fdtoverlay. > > Then those should be omitted. > >> Trying to use overlay dts files that are constructed to test runtime kernel >> code as fdtoverlay input data mixes two different test environments and >> objectives. If fdtoverlay test cases are desired, then fdtoverlay specific >> dts files should be created. >> >>> >>>> I agree that testing fdtoverlay is a good idea. I have not looked at the >>>> parent project to see how much testing of fdtoverlay occurs there, but I >>>> would prefer that fdtoverlay tests reside in the parent project if >>>> practical >>>> and reasonable. If there is some reason that some fdtoverlay tests are >>>> more practical in the Linux kernel repository then I am open to adding >>>> them to the Linux kernel tree. >>> >>> If you (or more importantly someone else sending us patches) make >>> changes to the overlays, you can test that they apply at build time >>> rather than runtime. I'll take it! So please help on fixing the issue >>> because I want to apply this. >> >> If the tests can be added to the upstream project, I would much prefer >> they reside there. If there is some reason a certain test is more >> suited to be in the Linux kernel source tree then I also would like >> it to be accepted here. > > Again, this is just about doing sanity checks at build time rather > than *only* rely on runtime. I'm fine with adding tests for applying overlays at build time (in other words, tests of fdtoverlay). But the constraints on applying an overlay at build time are different than the runtime constraints. The existing unittest overlay dts files are not designed to test applying overlays at build time. Tests for fdtov
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/12/21 8:04 AM, Rob Herring wrote: > On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand wrote: >> >> On 1/8/21 2:41 AM, 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. >>> >>> The file overlay_base.dtb have symbols of its own and we need to apply >>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives >>> us intermediate-overlay.dtb file. >>> >>> The intermediate-overlay.dtb file along with all other overlays is them >>> applied to testcases.dtb to generate the master.dtb file. >>> >>> Signed-off-by: Viresh Kumar >> >> NACK to this specific patch, in its current form. >> >> There are restrictions on applying an overlay at runtime that do not apply >> to applying an overlay to an FDT that will be loaded by the kernel during >> early boot. Thus the unittest overlays _must_ be applied using the kernel >> overlay loading methods to test the kernel runtime overlay loading feature. > > This patch doesn't take away from any of that and it completely orthogonal. Mea culpa. I took the patch header comment at face value, and read more into the header comment than what was written there. I then skimmed the patch instead of actually reading what it was doing. I incorrectly _assumed_ (bad!) that the intent was to replace applying the individual overlay dtb's with the master.dtb. Reading more closely, I see that the assumed final step of actually _using_ master.dtb does not exist. So, yes, I agree that the patch as written is orthogonal to my concern. My updated understanding is that this patch is attempting to use the existing unittest overlay dts files as source to test fdtoverlay. And that the resulting dtb from fdtoverlay is not intended to be consumed by the kernel unittest. I do not agree that this is a good approach to testing fdtoverlay. The unittest overlay dts files are constructed specifically to test various parts of the kernel overlay code and dynamic OF code. Some of the content of the overlays is constructed to trigger error conditions in that code, and thus will not be able to be processed without error by fdtoverlay. Trying to use overlay dts files that are constructed to test runtime kernel code as fdtoverlay input data mixes two different test environments and objectives. If fdtoverlay test cases are desired, then fdtoverlay specific dts files should be created. > >> I agree that testing fdtoverlay is a good idea. I have not looked at the >> parent project to see how much testing of fdtoverlay occurs there, but I >> would prefer that fdtoverlay tests reside in the parent project if practical >> and reasonable. If there is some reason that some fdtoverlay tests are >> more practical in the Linux kernel repository then I am open to adding >> them to the Linux kernel tree. > > If you (or more importantly someone else sending us patches) make > changes to the overlays, you can test that they apply at build time > rather than runtime. I'll take it! So please help on fixing the issue > because I want to apply this. If the tests can be added to the upstream project, I would much prefer they reside there. If there is some reason a certain test is more suited to be in the Linux kernel source tree then I also would like it to be accepted here. > > And yes, dtc has fdtoverlay tests. But this patch shows there's at > least 2 issues, > fdtoverlay can't apply overlays to the root A test of that definitely belongs in the upstream project. > and using an overlay as the base tree in UML is odd IMO. Am I still not fully understanding the patch? I'm missing how this patch changes what dtb is used as the base tree in UML. > > Rob > -Frank
Re: [PATCH V3 2/2] scripts: dtc: Build fdtoverlay and fdtdump tools
On 1/11/21 11:08 PM, Viresh Kumar wrote: > On 11-01-21, 18:44, Frank Rowand wrote: >> On 1/7/21 12:25 AM, Viresh Kumar wrote: >>> We will start building overlays for platforms soon in the kernel and >>> would need these tools going forward. Lets start building them. >>> >>> The fdtoverlay program applies (or merges) one ore more overlay dtb >>> blobs to a base dtb blob. The kernel build system would later use >>> fdtoverlay to generate the overlaid blobs based on platform specific >>> configurations. >>> >>> The fdtdump program prints a readable version of a flat device-tree >>> file. This is a very useful tool to analyze the details of the overlay's >>> dtb and the final dtb produced by fdtoverlay after applying the >>> overlay's dtb to a base dtb. >> >> You can calso dump an FDT with: >> >>dtc -O dts XXX.dtb >> >> Is this sufficient for the desired functionality, or is there something >> additional in fdtdump that is needed? > comment 1: > Not for my usecase at least. > >> If nothing additional needed, and there is no other justification for adding >> another program, I would prefer to leave fdtdump out. > comment 2: > Okay, then I will also remove the stale version of fdtdump which is > already there in kernel since a long time. > I'm confused. I read comment 1 as saying that fdtdump does provide a feature that you need to analyze the dtb created by fdtoverlay. But I read comment 2 as implying that you are accepting that fdtdump will not be added to the Linux kernel source. -Frank
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/12/21 4:16 AM, Bill Mills wrote: > > > On 1/12/21 3:37 AM, Viresh Kumar wrote: >> On 11-01-21, 20:22, Bill Mills wrote: >>> On 1/11/21 5:06 PM, Frank Rowand wrote: >>>> NACK to this specific patch, in its current form. >>>> >>>> There are restrictions on applying an overlay at runtime that do not apply >>>> to applying an overlay to an FDT that will be loaded by the kernel during >>>> early boot. Thus the unittest overlays _must_ be applied using the kernel >>>> overlay loading methods to test the kernel runtime overlay loading feature. >>>> >>>> I agree that testing fdtoverlay is a good idea. I have not looked at the >>>> parent project to see how much testing of fdtoverlay occurs there, but I >>>> would prefer that fdtoverlay tests reside in the parent project if >>>> practical >>>> and reasonable. If there is some reason that some fdtoverlay tests are >>>> more practical in the Linux kernel repository then I am open to adding >>>> them to the Linux kernel tree. >> >> I wasn't looking to add any testing for fdtoverlay in the kernel, but >> then I stumbled upon unit-tests here and thought it would be a good >> idea to get this built using static tools as well, as we aren't >> required to add any new source files for this and the existing tests >> already cover a lot of nodes. >> >> And so I am fine if we don't want to do this stuff in kernel. >> >>> I thought we were aligned that any new overlays into the kernel today would >>> only be for boot loader applied case. Applying overlays at kernel runtime >>> was out of scope at your request. >>> >>> Rob had requested that the overlays be test applied at build time. I don't >>> think there is any way to test the kernel runtime method at build time >>> correct? >>> >>> Please clarify your concern and your suggested way forward. >> >> The kernel does some overlay testing currently (at kernel boot only, >> not later), to see if the overlays get applied correctly or not, these >> are the unit tests. >> >> What Frank is probably saying is that the unit-tests dtbs shouldn't >> get used for testing fdtoverlay stuff. He isn't asking to support >> runtime application of overlays, but to not do fdtoverlay testing in >> the kernel. Yes, Viresh is understanding my point. >> > > Thanks Viresh, that makes sense. Sorry for the confusion Frank. No problem Bill, communication by email is hard, and we end up spending a lot of time getting to the point of common understanding, it is the nature of the process. -Frank
Re: [PATCH V3 2/2] scripts: dtc: Build fdtoverlay and fdtdump tools
Hi Viresh, I may have an email hiccup, but I don't see a "[PATCH V3 1/2]" email, I only see this "V3 2/2" email. Please start each new version of a patch series as a stand alone email instead of a reply to an email in a previous version of the series. That way each patch series discussion stands out as a separate thread. Also each version of the patch series needs to include all of the patches in the current version, even if they are unchanged from the previous version. Thanks, Frank On 1/7/21 12:25 AM, Viresh Kumar wrote: > We will start building overlays for platforms soon in the kernel and > would need these tools going forward. Lets start building them. > > The fdtoverlay program applies (or merges) one ore more overlay dtb > blobs to a base dtb blob. The kernel build system would later use > fdtoverlay to generate the overlaid blobs based on platform specific > configurations. > > The fdtdump program prints a readable version of a flat device-tree > file. This is a very useful tool to analyze the details of the overlay's > dtb and the final dtb produced by fdtoverlay after applying the > overlay's dtb to a base dtb. > > Signed-off-by: Viresh Kumar > --- > V3: > - Updated log > - Remove libfdt_dir > > scripts/dtc/Makefile | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile > index 4852bf44e913..472ab8cd590c 100644 > --- a/scripts/dtc/Makefile > +++ b/scripts/dtc/Makefile > @@ -1,12 +1,17 @@ > # SPDX-License-Identifier: GPL-2.0 > # scripts/dtc makefile > > -hostprogs-always-$(CONFIG_DTC) += dtc > +hostprogs-always-$(CONFIG_DTC) += dtc fdtdump fdtoverlay > hostprogs-always-$(CHECK_DT_BINDING) += dtc > > dtc-objs := dtc.o flattree.o fstree.o data.o livetree.o treesource.o \ > srcpos.o checks.o util.o > dtc-objs += dtc-lexer.lex.o dtc-parser.tab.o > +fdtdump-objs := fdtdump.o util.o > + > +libfdt-objs := fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o > fdt_empty_tree.o fdt_addresses.o fdt_overlay.o > +libfdt = $(addprefix libfdt/,$(libfdt-objs)) > +fdtoverlay-objs := $(libfdt) fdtoverlay.o util.o > > # Source files need to get at the userspace version of libfdt_env.h to > compile > HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt >
Re: [PATCH V3 2/2] scripts: dtc: Build fdtoverlay and fdtdump tools
On 1/7/21 12:25 AM, Viresh Kumar wrote: > We will start building overlays for platforms soon in the kernel and > would need these tools going forward. Lets start building them. > > The fdtoverlay program applies (or merges) one ore more overlay dtb > blobs to a base dtb blob. The kernel build system would later use > fdtoverlay to generate the overlaid blobs based on platform specific > configurations. > > The fdtdump program prints a readable version of a flat device-tree > file. This is a very useful tool to analyze the details of the overlay's > dtb and the final dtb produced by fdtoverlay after applying the > overlay's dtb to a base dtb. You can calso dump an FDT with: dtc -O dts XXX.dtb Is this sufficient for the desired functionality, or is there something additional in fdtdump that is needed? If nothing additional needed, and there is no other justification for adding another program, I would prefer to leave fdtdump out. -Frank > > Signed-off-by: Viresh Kumar > --- > V3: > - Updated log > - Remove libfdt_dir > > scripts/dtc/Makefile | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile > index 4852bf44e913..472ab8cd590c 100644 > --- a/scripts/dtc/Makefile > +++ b/scripts/dtc/Makefile > @@ -1,12 +1,17 @@ > # SPDX-License-Identifier: GPL-2.0 > # scripts/dtc makefile > > -hostprogs-always-$(CONFIG_DTC) += dtc > +hostprogs-always-$(CONFIG_DTC) += dtc fdtdump fdtoverlay > hostprogs-always-$(CHECK_DT_BINDING) += dtc > > dtc-objs := dtc.o flattree.o fstree.o data.o livetree.o treesource.o \ > srcpos.o checks.o util.o > dtc-objs += dtc-lexer.lex.o dtc-parser.tab.o > +fdtdump-objs := fdtdump.o util.o > + > +libfdt-objs := fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o > fdt_empty_tree.o fdt_addresses.o fdt_overlay.o > +libfdt = $(addprefix libfdt/,$(libfdt-objs)) > +fdtoverlay-objs := $(libfdt) fdtoverlay.o util.o > > # Source files need to get at the userspace version of libfdt_env.h to > compile > HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt >
Re: [RFC 2/2] scripts: dtc: Handle outform dtbo
On 1/5/21 9:37 AM, Rob Herring wrote:
> On Tue, Jan 5, 2021 at 4:24 AM Viresh Kumar wrote:
>>
>> Update dtc compiler to accept dtbo as an outform.
>>
>> Signed-off-by: Viresh Kumar
>>
>> ---
>> I feel that this needs to go directly to
>> https://git.kernel.org/pub/scm/utils/dtc/dtc.git
>>
>> Right ? I will send it separately if the idea is accepted here.
>
> Yes, needs to go to devicetree-compiler list. I think this came up
> before and IIRC David wasn't completely in agreement. I looked briefly
> and couldn't find the thread though...
>
> We really don't need a different extension because we could just
> examine the dtb to determine if it is an overlay or not. That's less
> obvious. We could also add meta-data to overlays defining what base
> they apply to. If we had that, a tool could just list all overlays
It may be valid to apply an overlay may be valid to more than one base FDT.
And for connector nodes and plugin overlays (which do not exist yet, I'm
way behind on bringing that concept forward), a single overlay may be
applied to more than one connector node in the base FDT.
> that should apply to a base and we could use that info for build time
> applying overlays. Of course, that and a dtbo extension/format are not
> mutually exclusive.
>
>> ---
>> scripts/dtc/dtc.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/scripts/dtc/dtc.c b/scripts/dtc/dtc.c
>> index bdb3f5945699..40fa7128b3d6 100644
>> --- a/scripts/dtc/dtc.c
>> +++ b/scripts/dtc/dtc.c
>> @@ -357,6 +357,8 @@ int main(int argc, char *argv[])
>> #endif
>> } else if (streq(outform, "dtb")) {
>> dt_to_blob(outf, dti, outversion);
>> + } else if (streq(outform, "dtbo")) {
>> + dt_to_blob(outf, dti, outversion);
>> } else if (streq(outform, "asm")) {
>> dt_to_asm(outf, dti, outversion);
>> } else if (streq(outform, "null")) {
>
> You also need to extend guess_type_by_name().
>
>
> Rob
>
Re: [RFC 0/2] kbuild: Add support to build overlays (%.dtbo)
On 1/5/21 9:21 AM, Rob Herring wrote: > On Tue, Jan 5, 2021 at 4:24 AM Viresh Kumar wrote: >> >> Hello, >> >> Here is an attempt to make some changes in the kernel to allow building >> of device tree overlays. >> >> While at it, I would also like to discuss about how we should mention >> the base DT blobs in the Makefiles for the overlays, so they can be >> build tested to make sure the overlays apply properly. >> >> A simple way is to mention that with -base extension, like this: >> >> $(overlay-file)-base := platform-base.dtb >> >> Any other preference ? > > I think we'll want something similar to how '-objs' works for modules: > > foo-board-1-dtbs := foo-board.dtb foo-overlay1.dtbo > foo-board-2-dtbs := foo-board.dtb foo-overlay2.dtbo > foo-board-1-2-dtbs := foo-board.dtb foo-overlay1.dtbo foo-overlay2.dtbo > dtbs-y += foo-board-1.dtb foo-board-2.dtb foo-board-1-2.dtb (Thinking ahead) I'm not sure how to fit connector nodes and the corresponding plugin overlays into this model. A single plugin .dtbo will need to be relocated onto one or more connector nodes. fdtoverlay and the run time overlay apply code do not know how to do this yet. -Frank > > (One difference here is we will want all the intermediate targets > unlike .o files.) > > You wouldn't necessarily have all the above combinations, but you have > to allow for them. I'm not sure how we'd handle applying any common > overlays where the base and overlay are in different directories. > > Another thing here is adding all the above is not really going to > scale on arm32 where we have a single dts directory. We need to move > things to per vendor/soc family directories. I have the script to do > this. We just need to agree on the vendor names and get Arnd/Olof to > run it. I also want that so we can enable schema checks by default > once a vendor is warning free (the whole tree is going to take > forever). > >> Also fdtoverlay is an external entity right now, and is not part of the >> kernel. Do we need to make it part of the kernel ? Or keep using the >> external entity ? > > Part of the kernel. We just need to add it to the dtc sync script and > makefile I think. > > Rob >
Re: [PATCH V2 1/2] scripts: dtc: Add fdtoverlay.c and fdtdump.c to DTC_SOURCE
Hi Viresh, On 1/6/21 11:15 PM, Viresh Kumar wrote: > We will start building overlays for platforms soon in the kernel and > would need these tools going forward. Lets start fetching them. > > Note that a copy of fdtdump.c was already copied back in the year 2012, > but was never updated or built for some reason. > > Signed-off-by: Viresh Kumar > --- > V2: Separate out this change from Makefile one. > > This needs to be followed by invocation of the ./update-dtc-source.sh > script so the relevant files can be copied before the Makefile is > updated in the next patch. Just an FYI that Rob will do the ./update-dtc-source.sh step at the appropriate time, creating a commit to be submitted in his pull request to Linus. That way Rob will ensure that all of the updates from the parent project are updated in a careful manner. -Frank > > scripts/dtc/update-dtc-source.sh | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/scripts/dtc/update-dtc-source.sh > b/scripts/dtc/update-dtc-source.sh > index bc704e2a6a4a..9bc4afb71415 100755 > --- a/scripts/dtc/update-dtc-source.sh > +++ b/scripts/dtc/update-dtc-source.sh > @@ -31,9 +31,9 @@ set -ev > DTC_UPSTREAM_PATH=`pwd`/../dtc > DTC_LINUX_PATH=`pwd`/scripts/dtc > > -DTC_SOURCE="checks.c data.c dtc.c dtc.h flattree.c fstree.c livetree.c > srcpos.c \ > - srcpos.h treesource.c util.c util.h version_gen.h yamltree.c \ > - dtc-lexer.l dtc-parser.y" > +DTC_SOURCE="checks.c data.c dtc.c dtc.h fdtdump.c fdtoverlay.c flattree.c \ > + fstree.c livetree.c srcpos.c srcpos.h treesource.c util.c \ > + util.h version_gen.h yamltree.c dtc-lexer.l dtc-parser.y" > LIBFDT_SOURCE="fdt.c fdt.h fdt_addresses.c fdt_empty_tree.c \ > fdt_overlay.c fdt_ro.c fdt_rw.c fdt_strerror.c fdt_sw.c \ > fdt_wip.c libfdt.h libfdt_env.h libfdt_internal.h" >
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/11/21 9:46 AM, Rob Herring wrote: > On Fri, Jan 8, 2021 at 2:41 AM 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. > > Nice idea. > >> The file overlay_base.dtb have symbols of its own and we need to apply >> overlay.dtb to overlay_base.dtb alone first to make it work, which gives >> us intermediate-overlay.dtb file. > > Okay? If restructuring things helps we should do that. Frank? I'm a little slow responding to this thread. After seeing this question, I responded to the patch email with a NACK (with further explanation in that response). -Frank > >> The intermediate-overlay.dtb file along with all other overlays is them > > s/them/then/ > >> applied to testcases.dtb to generate the master.dtb file. >> >> Signed-off-by: Viresh Kumar >> >> --- >> Depends on: >> >> https://lore.kernel.org/lkml/be5cb12a68d9ac2c35ad9dd50d6b168f7cad6837.1609996381.git.viresh.ku...@linaro.org/ >> >> I have kept the .dtb naming for overlays for now, lets see how we do it >> eventually. >> >> Rob/Frank, this doesn't work properly right now. Maybe I missed how >> these overlays must be applied or there is a bug in fdtoverlay. >> >> The master.dtb doesn't include any nodes from overlay_base.dtb or >> overlay.dtb probably because 'testcase-data-2' node isn't present in >> testcases.dtb and fdtoverlay doesn't allow applying new nodes to the >> root node, i.e. allows new sub-nodes once it gets phandle to the parent >> but nothing can be added to the root node itself. Though I get a feel >> that it works while applying the nodes dynamically and it is expected to >> work here as well. > > Sounds like a bug in fdtoverlay to me. Though maybe you need an empty > base tree. An overlay serving as the base is a bit odd so it's > somewhat understandable fdtoverlay couldn't handle that. OTOH, > combining 2 overlays together seems like a valid use. > >> >> (And yeah, this is my first serious attempt at updating Makefiles, I am >> sure there is a scope of improvement here :)) > > Usually I write something and Masahiro rewrites it for me. :) > > Rob >
Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay
On 1/8/21 2:41 AM, 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. > > The file overlay_base.dtb have symbols of its own and we need to apply > overlay.dtb to overlay_base.dtb alone first to make it work, which gives > us intermediate-overlay.dtb file. > > The intermediate-overlay.dtb file along with all other overlays is them > applied to testcases.dtb to generate the master.dtb file. > > Signed-off-by: Viresh Kumar NACK to this specific patch, in its current form. There are restrictions on applying an overlay at runtime that do not apply to applying an overlay to an FDT that will be loaded by the kernel during early boot. Thus the unittest overlays _must_ be applied using the kernel overlay loading methods to test the kernel runtime overlay loading feature. I agree that testing fdtoverlay is a good idea. I have not looked at the parent project to see how much testing of fdtoverlay occurs there, but I would prefer that fdtoverlay tests reside in the parent project if practical and reasonable. If there is some reason that some fdtoverlay tests are more practical in the Linux kernel repository then I am open to adding them to the Linux kernel tree. -Frank > > --- > Depends on: > > https://lore.kernel.org/lkml/be5cb12a68d9ac2c35ad9dd50d6b168f7cad6837.1609996381.git.viresh.ku...@linaro.org/ > > I have kept the .dtb naming for overlays for now, lets see how we do it > eventually. > > Rob/Frank, this doesn't work properly right now. Maybe I missed how > these overlays must be applied or there is a bug in fdtoverlay. > > The master.dtb doesn't include any nodes from overlay_base.dtb or > overlay.dtb probably because 'testcase-data-2' node isn't present in > testcases.dtb and fdtoverlay doesn't allow applying new nodes to the > root node, i.e. allows new sub-nodes once it gets phandle to the parent > but nothing can be added to the root node itself. Though I get a feel > that it works while applying the nodes dynamically and it is expected to > work here as well. > > (And yeah, this is my first serious attempt at updating Makefiles, I am > sure there is a scope of improvement here :)) > > --- > drivers/of/unittest-data/Makefile | 23 +++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/of/unittest-data/Makefile > b/drivers/of/unittest-data/Makefile > index 009f4045c8e4..f17bce85f65f 100644 > --- a/drivers/of/unittest-data/Makefile > +++ b/drivers/of/unittest-data/Makefile > @@ -38,3 +38,26 @@ DTC_FLAGS_testcases += -@ > > # suppress warnings about intentional errors > DTC_FLAGS_testcases += -Wno-interrupts_property > + > +# Apply overlays statically with fdtoverlay > +intermediate-overlay := overlay.dtb > +master := 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 \ > +intermediate-overlay.dtb > + > +quiet_cmd_fdtoverlay = fdtoverlay $@ > + cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^ > + > +$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix > $(obj)/,$(intermediate-overlay)) > + $(call if_changed,fdtoverlay) > + > +$(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master)) > + $(call if_changed,fdtoverlay) > + > +always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb >
Re: [SPECIFICATION RFC] The firmware and bootloader log specification
On 12/4/20 7:23 AM, Paul Menzel wrote: > Dear Wim, dear Daniel, > > > First, thank you for including all parties in the discussion. > Am 04.12.20 um 13:52 schrieb Wim Vervoorn: > >> I agree with you. Using an existing standard is better than inventing >> a new one in this case. I think using the coreboot logging is a good >> idea as there is indeed a lot of support already available and it is >> lightweight and simple. > In my opinion coreboot’s format is lacking, that it does not record the > timestamp, and the log level is not stored as metadata, but (in coreboot) > only used to decide if to print the message or not. > > I agree with you, that an existing standard should be used, and in my opinion > it’s Linux message format. That is most widely supported, and existing tools > could then also work with pre-Linux messages. > > Sean Hudson from Mentor Graphics presented that idea at Embedded Linux > Conference Europe 2016 [1]. No idea, if anything came out of that effort. > (Unfortunately, I couldn’t find an email. Does somebody have contacts at > Mentor to find out, how to reach him?) I forwarded this to Sean. -Frank > > > Kind regards, > > Paul > > > [1]: > http://events17.linuxfoundation.org/sites/events/files/slides/2016-10-12%20-%20ELCE%20-%20Shared%20Logging%20-%20Part%20Deux.pdf
Re: [RFC PATCH 0/3] Fix errors on DT overlay removal with devlinks
On 10/28/20 11:25 AM, Michael Auchter wrote: > Hey Saravana, > > Thanks for taking the time to look into this! > > On Mon, Oct 26, 2020 at 12:10:33PM -0700, Saravana Kannan wrote: >> On Wed, Oct 21, 2020 at 2:02 PM Frank Rowand wrote: >>> >>> Hi Saravana, >>> >>> Michael found an issue related to the removal of a devicetree node >>> which involves devlinks: >>> >>> On 10/14/20 2:36 PM, Michael Auchter wrote: >>>> After updating to v5.9, I've started seeing errors in the kernel log >>>> when using device tree overlays. Specifically, the problem seems to >>>> happen when removing a device tree overlay that contains two devices >>>> with some dependency between them (e.g., a device that provides a clock >>>> and a device that consumes that clock). Removing such an overlay results >>>> in: >>>> >>>> OF: ERROR: memory leak, expected refcount 1 instead of 2, >>>> of_node_get()/of_node_put() unbalanced - destroy >>>> OF: ERROR: memory leak, expected refcount 1 instead of 2, >>>> of_node_get()/of_node_put() unbalanced - destroy >>>> >>>> followed by hitting some REFCOUNT_WARNs in refcount.c >>>> >>>> In the first patch, I've included a unittest that can be used to >>>> reproduce this when built with CONFIG_OF_UNITTEST [1]. >>>> >>>> I believe the issue is caused by the cleanup performed when releasing >>>> the devlink device that's created to represent the dependency between >>>> devices. The devlink device has references to the consumer and supplier >>>> devices, which it drops in device_link_free; the devlink device's >>>> release callback calls device_link_free via call_srcu. >>>> >>>> When the overlay is being removed, all devices are removed, and >>>> eventually the release callback for the devlink device run, and >>>> schedules cleanup using call_srcu. Before device_link_free can and call >>>> put_device on the consumer/supplier, the rest of the overlay removal >>>> process runs, resulting in the error traces above. >>> >>> When a devicetree node in an overlay is removed, the remove code expects >>> all previous users of the related device to have done the appropriate put >>> of the device and to have no later references. >>> >>> As Michael described above, the devlink release callback defers the >>> put_device(). The cleanup via srcu was implemented in commit >>> 843e600b8a2b01463c4d873a90b2c2ea8033f1f6 "driver core: Fix sleeping >>> in invalid context during device link deletion" to solve yet another >>> issue. >>> >>> >>>> >>>> Patches 2 and 3 are an attempt at fixing this: call srcu_barrier to wait >>>> for any pending device_link_free's to execute before continuing on with >>>> the removal process. >>>> >>>> These patches resolve the issue, but probably not in the best way. In >>>> particular, it seems strange to need to leak details of devlinks into >>>> the device tree overlay code. So, I'd be curious to get some feedback or >>>> hear any other ideas for how to resolve this issue. >>> >>> I agree with Michael that adding an indirect call of >>> srcu_barrier(_links_srcu) >>> into the devicetree overlay code is not an appropriate solution. >> >> I kind of see your point too. I wonder if the srcu_barrier() should >> happen inside like so: >> device_del() -> device_links_purge()->srcu_barrier() >> >> I don't know what contention the use of SRCUs in device links was >> trying to avoid, but I think the srcu_barrier() call path I suggested >> above shouldn't be a problem. If that fixes the issue, the best way to >> know if it's an issue is to send out a patch and see if Rafael has any >> problem with it :) > > I was able to test this by adding the srcu_barrier() at the end of > device_links_purge(), and that does seem to have fixed the issue. > >>> Is there some other way to fix the problem that 843e600b8a2b solves without >>> deferring the put_device() done by the devlink release callback? >> >> Ok I finally got some time to look into this closely. >> >> Even if you revert 843e600b8a2b, you'll see that device_link_free() >> (which drops the reference to the consumer and supplier devices) was >> scheduled to run when the SRCU clean up occurs. So I think this issue >> was present even
Re: [RFC PATCH 0/3] Fix errors on DT overlay removal with devlinks
Hi Saravana, Michael found an issue related to the removal of a devicetree node which involves devlinks: On 10/14/20 2:36 PM, Michael Auchter wrote: > After updating to v5.9, I've started seeing errors in the kernel log > when using device tree overlays. Specifically, the problem seems to > happen when removing a device tree overlay that contains two devices > with some dependency between them (e.g., a device that provides a clock > and a device that consumes that clock). Removing such an overlay results > in: > > OF: ERROR: memory leak, expected refcount 1 instead of 2, > of_node_get()/of_node_put() unbalanced - destroy > OF: ERROR: memory leak, expected refcount 1 instead of 2, > of_node_get()/of_node_put() unbalanced - destroy > > followed by hitting some REFCOUNT_WARNs in refcount.c > > In the first patch, I've included a unittest that can be used to > reproduce this when built with CONFIG_OF_UNITTEST [1]. > > I believe the issue is caused by the cleanup performed when releasing > the devlink device that's created to represent the dependency between > devices. The devlink device has references to the consumer and supplier > devices, which it drops in device_link_free; the devlink device's > release callback calls device_link_free via call_srcu. > > When the overlay is being removed, all devices are removed, and > eventually the release callback for the devlink device run, and > schedules cleanup using call_srcu. Before device_link_free can and call > put_device on the consumer/supplier, the rest of the overlay removal > process runs, resulting in the error traces above. When a devicetree node in an overlay is removed, the remove code expects all previous users of the related device to have done the appropriate put of the device and to have no later references. As Michael described above, the devlink release callback defers the put_device(). The cleanup via srcu was implemented in commit 843e600b8a2b01463c4d873a90b2c2ea8033f1f6 "driver core: Fix sleeping in invalid context during device link deletion" to solve yet another issue. > > Patches 2 and 3 are an attempt at fixing this: call srcu_barrier to wait > for any pending device_link_free's to execute before continuing on with > the removal process. > > These patches resolve the issue, but probably not in the best way. In > particular, it seems strange to need to leak details of devlinks into > the device tree overlay code. So, I'd be curious to get some feedback or > hear any other ideas for how to resolve this issue. I agree with Michael that adding an indirect call of srcu_barrier(_links_srcu) into the devicetree overlay code is not an appropriate solution. Is there some other way to fix the problem that 843e600b8a2b solves without deferring the put_device() done by the devlink release callback? -Frank > > Thanks, > Michael > > 1. Note that this isn't a very good unit test: it will report a "pass" >even if it fails with the aforementioned errors, as these errors >aren't propogated. > > Michael Auchter (3): > of: unittest: add test of overlay with devlinks > driver core: add device_links_barrier > of: dynamic: add device links barrier before detach > > drivers/base/core.c | 10 ++ > drivers/of/dynamic.c| 3 +++ > drivers/of/unittest-data/Makefile | 1 + > drivers/of/unittest-data/overlay_16.dts | 26 + > drivers/of/unittest.c | 16 +++ > include/linux/device.h | 1 + > 6 files changed, 57 insertions(+) > create mode 100644 drivers/of/unittest-data/overlay_16.dts >
Re: [RFC PATCH 0/3] Fix errors on DT overlay removal with devlinks
Hi Michael, On 10/14/20 2:36 PM, Michael Auchter wrote: > After updating to v5.9, I've started seeing errors in the kernel log > when using device tree overlays. Specifically, the problem seems to > happen when removing a device tree overlay that contains two devices > with some dependency between them (e.g., a device that provides a clock > and a device that consumes that clock). Removing such an overlay results > in: > > OF: ERROR: memory leak, expected refcount 1 instead of 2, > of_node_get()/of_node_put() unbalanced - destroy > OF: ERROR: memory leak, expected refcount 1 instead of 2, > of_node_get()/of_node_put() unbalanced - destroy > > followed by hitting some REFCOUNT_WARNs in refcount.c > > In the first patch, I've included a unittest that can be used to > reproduce this when built with CONFIG_OF_UNITTEST [1]. > > I believe the issue is caused by the cleanup performed when releasing > the devlink device that's created to represent the dependency between > devices. The devlink device has references to the consumer and supplier > devices, which it drops in device_link_free; the devlink device's > release callback calls device_link_free via call_srcu. > > When the overlay is being removed, all devices are removed, and > eventually the release callback for the devlink device run, and > schedules cleanup using call_srcu. Before device_link_free can and call > put_device on the consumer/supplier, the rest of the overlay removal > process runs, resulting in the error traces above. > > Patches 2 and 3 are an attempt at fixing this: call srcu_barrier to wait > for any pending device_link_free's to execute before continuing on with > the removal process. > > These patches resolve the issue, but probably not in the best way. In > particular, it seems strange to need to leak details of devlinks into > the device tree overlay code. So, I'd be curious to get some feedback or > hear any other ideas for how to resolve this issue. Thanks for finding the problem, analyzing it, creating a unittest, and creating a fix. I agree with your analysis that there are issues with the implementation of the test and fix. I'll dig into this to see if I can provide some useful improvements. -Frank > > Thanks, > Michael > > 1. Note that this isn't a very good unit test: it will report a "pass" >even if it fails with the aforementioned errors, as these errors >aren't propogated. > > Michael Auchter (3): > of: unittest: add test of overlay with devlinks > driver core: add device_links_barrier > of: dynamic: add device links barrier before detach > > drivers/base/core.c | 10 ++ > drivers/of/dynamic.c| 3 +++ > drivers/of/unittest-data/Makefile | 1 + > drivers/of/unittest-data/overlay_16.dts | 26 + > drivers/of/unittest.c | 16 +++ > include/linux/device.h | 1 + > 6 files changed, 57 insertions(+) > create mode 100644 drivers/of/unittest-data/overlay_16.dts >
Re: (EXT) Re: [PATCH] of: skip disabled CPU nodes
Hi Rob,
On 2020-08-26 08:54, Matthias Schiffer wrote:
> On Wed, 2020-08-26 at 08:01 -0500, Frank Rowand wrote:
>> On 2020-08-26 07:02, Matthias Schiffer wrote:
>>> Allow disabling CPU nodes using status = "disabled".
>>>
>>> This allows a bootloader to change the number of available CPUs
>>> (for
>>> example when a common DTS is used for SoC variants with different
>>> numbers
>>> of cores) without deleting the nodes altogether (which may require
>>> additional fixups where the CPU nodes are referenced, e.g. a
>>> cooling
>>> map).
>>>
>>> Signed-off-by: Matthias Schiffer >>>
>>> ---
>>> drivers/of/base.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>> index ea44fea99813..d547e9deced1 100644
>>> --- a/drivers/of/base.c
>>> +++ b/drivers/of/base.c
>>> @@ -796,6 +796,8 @@ struct device_node *of_get_next_cpu_node(struct
>>> device_node *prev)
>>> of_node_put(node);
>>> }
>>> for (; next; next = next->sibling) {
>>> + if (!__of_device_is_available(next))
>>> + continue;
>>> if (!(of_node_name_eq(next, "cpu") ||
>>> __of_node_is_type(next, "cpu")))
>>> continue;
>>>
>>
>> The original implementation of of_get_next_cpu_node() had
>> that check, but status disabled for cpu nodes has different
>> semantics than other nodes, and the check broke some systems.
>> The check was removed by c961cb3be906 "of: Fix cpu node
>> iterator to not ignore disabled cpu nodes".
>>
>> It would be useful to document that difference in the
>> header comment of of_get_next_cpu_node().
>>
>> -Frank
>
> Hmm, I see. This difference in behaviour is quite unfortunate, as I'm
> currently looking for a way to *really* disable a CPU core.
>
> In arch/arm64/boot/dts/freescale/imx8mn.dtsi (and other variants of the
> i.MX8M), there are 4 CPU nodes for the full-featured quad-core version.
> The reduced single- and dual-core versions are currently handled in
> NXP's U-Boot fork by deleting the additional nodes.
>
> Not doing so causes the kernel to hang for a while when trying to
> online the non-existent cores during boot (at least in linux-imx 5.4 -
> I have not checked a more recent mainline kernel yet), but the deletion
> is non-trivial to do without leaving dangling phandle references.
Any thoughts on implementing another universal property that means
something like "the hardware described by this node does not exist
or is so broken that you better not use it".
Matthias, if Rob thinks that is a good idea, then you should start
with a new proposal that is also sent to
devicetree-s...@vger.kernel.org
-Frank
>
> Kind regards,
> Matthias
>
Re: [PATCH] of: skip disabled CPU nodes
On 2020-08-26 07:02, Matthias Schiffer wrote:
> Allow disabling CPU nodes using status = "disabled".
>
> This allows a bootloader to change the number of available CPUs (for
> example when a common DTS is used for SoC variants with different numbers
> of cores) without deleting the nodes altogether (which may require
> additional fixups where the CPU nodes are referenced, e.g. a cooling
> map).
>
> Signed-off-by: Matthias Schiffer
> ---
> drivers/of/base.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index ea44fea99813..d547e9deced1 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -796,6 +796,8 @@ struct device_node *of_get_next_cpu_node(struct
> device_node *prev)
> of_node_put(node);
> }
> for (; next; next = next->sibling) {
> + if (!__of_device_is_available(next))
> + continue;
> if (!(of_node_name_eq(next, "cpu") ||
> __of_node_is_type(next, "cpu")))
> continue;
>
The original implementation of of_get_next_cpu_node() had
that check, but status disabled for cpu nodes has different
semantics than other nodes, and the check broke some systems.
The check was removed by c961cb3be906 "of: Fix cpu node
iterator to not ignore disabled cpu nodes".
It would be useful to document that difference in the
header comment of of_get_next_cpu_node().
-Frank
Re: [RFC PATCH v2 0/3] mikroBUS driver for add-on boards
Hi Vaishnav, Apologies in advance -- I expect to be very slow in responding this week. Linux Plumbers will take some of my time and I am moving to a new home. -Frank On 2020-08-18 15:38, Frank Rowand wrote: > Hi Vaishnav, > > +me +devicetree > > Please add these two recipients to future versions. > > I will comment more after reading the first version and v2. > > -Frank > > > On 2020-08-18 07:48, Vaishnav M A wrote: >> Hi, >> >> This Patch series is an update to the mikroBUS driver >> RFC v1 Patch : https://lkml.org/lkml/2020/7/24/518 . >> The mikrobus driver is updated to add mikrobus ports from device-tree >> overlays, the debug interfaces for adding mikrobus ports through sysFS >> is removed, and the driver considers the extended usage of mikrobus >> port pins from their standard purposes. >> >> change log: >> v2: support for adding mikroBUS ports from DT overlays, >> remove debug sysFS interface for adding mikrobus ports, >> consider extended pin usage/deviations from mikrobus standard >> specifications, >> use greybus CPort protocol enum instead of new protcol enums >> Fix cases of wrong indendation, ignoring return values, freeing >> allocated resources in case of errors and other style suggestions >> in v1 review. >> >> Vaishnav M A (3): >> add mikrobus descriptors to greybus_manifest >> mikroBUS driver for add-on boards on mikrobus ports >> Add Device Tree Bindings for mikroBUS port >> >> .../bindings/misc/linux,mikrobus.txt | 81 ++ >> MAINTAINERS | 6 + >> drivers/misc/Kconfig | 1 + >> drivers/misc/Makefile | 1 + >> drivers/misc/mikrobus/Kconfig | 16 + >> drivers/misc/mikrobus/Makefile| 7 + >> drivers/misc/mikrobus/mikrobus_core.c | 692 ++ >> drivers/misc/mikrobus/mikrobus_core.h | 191 + >> drivers/misc/mikrobus/mikrobus_manifest.c | 444 +++ >> drivers/misc/mikrobus/mikrobus_manifest.h | 21 + >> drivers/misc/mikrobus/mikrobus_port.c | 171 + >> include/linux/greybus/greybus_manifest.h | 47 ++ >> 12 files changed, 1678 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/misc/linux,mikrobus.txt >> create mode 100644 drivers/misc/mikrobus/Kconfig >> create mode 100644 drivers/misc/mikrobus/Makefile >> create mode 100644 drivers/misc/mikrobus/mikrobus_core.c >> create mode 100644 drivers/misc/mikrobus/mikrobus_core.h >> create mode 100644 drivers/misc/mikrobus/mikrobus_manifest.c >> create mode 100644 drivers/misc/mikrobus/mikrobus_manifest.h >> create mode 100644 drivers/misc/mikrobus/mikrobus_port.c >> >
Re: [RFC PATCH v2 0/3] mikroBUS driver for add-on boards
Hi Vaishnav, +me +devicetree Please add these two recipients to future versions. I will comment more after reading the first version and v2. -Frank On 2020-08-18 07:48, Vaishnav M A wrote: > Hi, > > This Patch series is an update to the mikroBUS driver > RFC v1 Patch : https://lkml.org/lkml/2020/7/24/518 . > The mikrobus driver is updated to add mikrobus ports from device-tree > overlays, the debug interfaces for adding mikrobus ports through sysFS > is removed, and the driver considers the extended usage of mikrobus > port pins from their standard purposes. > > change log: > v2: support for adding mikroBUS ports from DT overlays, > remove debug sysFS interface for adding mikrobus ports, > consider extended pin usage/deviations from mikrobus standard > specifications, > use greybus CPort protocol enum instead of new protcol enums > Fix cases of wrong indendation, ignoring return values, freeing > allocated resources in case of errors and other style suggestions > in v1 review. > > Vaishnav M A (3): > add mikrobus descriptors to greybus_manifest > mikroBUS driver for add-on boards on mikrobus ports > Add Device Tree Bindings for mikroBUS port > > .../bindings/misc/linux,mikrobus.txt | 81 ++ > MAINTAINERS | 6 + > drivers/misc/Kconfig | 1 + > drivers/misc/Makefile | 1 + > drivers/misc/mikrobus/Kconfig | 16 + > drivers/misc/mikrobus/Makefile| 7 + > drivers/misc/mikrobus/mikrobus_core.c | 692 ++ > drivers/misc/mikrobus/mikrobus_core.h | 191 + > drivers/misc/mikrobus/mikrobus_manifest.c | 444 +++ > drivers/misc/mikrobus/mikrobus_manifest.h | 21 + > drivers/misc/mikrobus/mikrobus_port.c | 171 + > include/linux/greybus/greybus_manifest.h | 47 ++ > 12 files changed, 1678 insertions(+) > create mode 100644 Documentation/devicetree/bindings/misc/linux,mikrobus.txt > create mode 100644 drivers/misc/mikrobus/Kconfig > create mode 100644 drivers/misc/mikrobus/Makefile > create mode 100644 drivers/misc/mikrobus/mikrobus_core.c > create mode 100644 drivers/misc/mikrobus/mikrobus_core.h > create mode 100644 drivers/misc/mikrobus/mikrobus_manifest.c > create mode 100644 drivers/misc/mikrobus/mikrobus_manifest.h > create mode 100644 drivers/misc/mikrobus/mikrobus_port.c >
Re: [Q] devicetree overlays
On 2020-08-12 08:27, Enrico Weigelt, metux IT consult wrote: > On 07.08.20 16:17, Sven Van Asbroeck wrote: > > Hi, > >> I believe you're asking: "how do I associate device tree nodes to >> devices on a dynamically discoverable bus such as USB or PCI" right ? >> >> I believe that already exists. You can describe the _expected_ pci or >> usb topology in the >> devicetree. If a device gets detected in a spot on the bus described >> in the tree, that >> snippet will be automatically associated with this device. >> >> How to for usb: >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/usb/usb-device.txt?h=v5.8 >> >> How to for pci: >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/pci.txt?h=v5.8 > > Thanks, that looks good. > > But I've still got another problem: how can I use DT along w/ ACPI ? Some answers from https://elinux.org/Device_tree_plumbers_2016_etherpad Question: what about device tree on x86 Answer: there's already support for DT on x86. But, we should not mix DT and ACPI. A controversial topic is ACPI overlays (putting DT in ACPI or ACPI in DT). Don't want to have drivers that get part of their info from ACPI and part from DT. That's nuts. Question: Can you have ACPI and DT at the same time on x86? Answer: No. Some ARM64 systems have support for both ACPI and DT, but the system selects one to use at runtime. They are not used at the same time. You can run the DT unit tests on x86. -Frank > > The scenario goes like this: > > * machine boots and probes normally w/ ACPI > * device is detected via USB, PCI, DMI, etc -> driver gets active > * driver loads (or carries) a DT snippet > * devices on the bus are instantiated via this DT snippet > > (driver could also be some udev vodoo) > > Example a: > > * generic usb i2c dongle w/ some i2c devices attached behind it > * config (or DT snippet) somewhere in the FS > > Example b: > > * x86 board driver (eg. apu2/3/4), probed via DMI > * just instantiates a bunch of generic drivers and wires up > devices (gpio, leds, keys, ...) > > > Do you think we can already do that ? > Otherwise, what has to be done to achieve that ? > > > --mtx >
Re: [PATCH v1 2/2] of: property: Add device link support for pinctrl-0 through pinctrl-8
On 2020-07-22 15:13, Saravana Kannan wrote:
> Add support for pinctrl-0 through pinctrl-8 explicitly instead of trying
> to add support for pinctrl-%d properties.
>
> Of all the pinctrl-* properties in dts files (20322), only 47% (9531)
> are pinctrl-%d properties. Of all the pinctrl-%d properties, 99.5%
> (9486) are made up of pinctrl-[0-2].
>
> Trying to parse all pinctrl-* properties and checking for pinctrl-%d is
> unnecessarily complicated. So, just add support for pinctrl-[0-8] for
> now. In the unlikely event we ever exceed pinctrl-8, we can come back
> and improve this.
If you were to implement the more general pinctrl-* case, roughly what would
it look like (pseudo-code or english description is fine).
-Frank
>
> Signed-off-by: Saravana Kannan
> ---
> drivers/of/property.c | 18 ++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index b06edeb1f88b..d40d923ffeaf 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1274,6 +1274,15 @@ DEFINE_SIMPLE_PROP(interrupts_extended,
> "interrupts-extended",
> DEFINE_SIMPLE_PROP(nvmem_cells, "nvmem-cells", NULL)
> DEFINE_SIMPLE_PROP(phys, "phys", "#phy-cells")
> DEFINE_SIMPLE_PROP(wakeup_parent, "wakeup-parent", NULL)
> +DEFINE_SIMPLE_PROP(pinctrl0, "pinctrl-0", NULL)
> +DEFINE_SIMPLE_PROP(pinctrl1, "pinctrl-1", NULL)
> +DEFINE_SIMPLE_PROP(pinctrl2, "pinctrl-2", NULL)
> +DEFINE_SIMPLE_PROP(pinctrl3, "pinctrl-3", NULL)
> +DEFINE_SIMPLE_PROP(pinctrl4, "pinctrl-4", NULL)
> +DEFINE_SIMPLE_PROP(pinctrl5, "pinctrl-5", NULL)
> +DEFINE_SIMPLE_PROP(pinctrl6, "pinctrl-6", NULL)
> +DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
> +DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
> DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
> @@ -1303,6 +1312,15 @@ static const struct supplier_bindings
> of_supplier_bindings[] = {
> { .parse_prop = parse_nvmem_cells, },
> { .parse_prop = parse_phys, },
> { .parse_prop = parse_wakeup_parent, },
> + { .parse_prop = parse_pinctrl0, },
> + { .parse_prop = parse_pinctrl1, },
> + { .parse_prop = parse_pinctrl2, },
> + { .parse_prop = parse_pinctrl3, },
> + { .parse_prop = parse_pinctrl4, },
> + { .parse_prop = parse_pinctrl5, },
> + { .parse_prop = parse_pinctrl6, },
> + { .parse_prop = parse_pinctrl7, },
> + { .parse_prop = parse_pinctrl8, },
> { .parse_prop = parse_regulators, },
> { .parse_prop = parse_gpio, },
> { .parse_prop = parse_gpios, },
>
Re: [RFC] MFD's relationship with Device Tree (OF)
+Frank (me)
On 2020-06-22 16:03, Michael Walle wrote:
> Am 2020-06-14 12:26, schrieb Michael Walle:
>> Hi Rob,
>>
>> Am 2020-06-10 00:03, schrieb Rob Herring:
>> [..]
>>> Yes, we should use 'reg' whenever possible. If we don't have 'reg',
>>> then you shouldn't have a unit-address either and you can simply match
>>> on the node name (standard DT driver matching is with compatible,
>>> device_type, and node name (w/o unit-address)). We've generally been
>>> doing 'classname-N' when there's no 'reg' to do 'classname@N'.
>>> Matching on 'classname-N' would work with node name matching as only
>>> unit-addresses are stripped.
>>
>> This still keeps me thinking. Shouldn't we allow the (MFD!) device
>> driver creator to choose between "classname@N" and "classname-N".
>> In most cases N might not be made up, but it is arbitrarily chosen;
>> for example you've chosen the bank for the ab8500 reg. It is not
>> a defined entity, like an I2C address if your parent is an I2C bus,
>> or a SPI chip select, or the memory address in case of MMIO. Instead
>> the device driver creator just chooses some "random" property from
>> the datasheet; another device creator might have chosen another
>> property. Wouldn't it make more sense, to just say this MFD provides
>> N pwm devices and the subnodes are matching based on pwm-{0,1..N-1}?
>> That would also be the logical consequence of the current MFD sub
>> device to OF node matching code, which just supports N=1.
>>
>
> Rob? Lee?
>
> -michael
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On 2020-06-24 11:14, Lee Jones wrote: > On Wed, 24 Jun 2020, Frank Rowand wrote: > >> On 2020-06-24 02:46, Lee Jones wrote: >>> On Tue, 23 Jun 2020, Frank Rowand wrote: >>> >>>> On 2020-06-23 14:59, Lee Jones wrote: >> >> < big snip > >> >> Thanks for the replies in the above portion. > > NP. > >>>>>> But yes or no to my solution #2 (with some slight changes to >>>>>> make it better (more gracious handling of the detected error) as >>>>>> discussed elsewhere in the email thread)? >>>>> >>>>> Please see "[0]" above! >>>>> >>>>> AFAICT your solution #2 involves bombing out *all* devices if there is >>>>> a duplicate compatible with no 'reg' property value. This is a) >>>>> over-kill and b) not an error, as I mentioned: >>>> >>>> As I mentioned above, I set you up to have this misunderstanding by >>>> a mistake in one of my earlier emails. So now that I have pointed >>>> out what I meant here by "more gracious handling of the detected >>>> error", what do you think of my amended solution #2? >>> >>> Explained above, but the LT;DR is that it's not correct. >> >> I don't agree with you, I think my solution is better. Even if I >> prefer my solution, I find your solution to be good enough. > > I still don't see how it could work, but please feel free to submit a > subsequent patch and we can discuss it on its own merits. > >> So I am dropping my specific objection to returning -EAGAIN from >> mfd_match_of_node_to_dev() when the node has previously been >> allocated to a device. > > Great. Thanks for taking an interest. > > Does this mean I can apply your Reviewed-by? > No, please do not. I don't want to give the patch that strong of an endorsement.
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On 2020-06-24 02:46, Lee Jones wrote: > On Tue, 23 Jun 2020, Frank Rowand wrote: > >> On 2020-06-23 14:59, Lee Jones wrote: < big snip > Thanks for the replies in the above portion. >>>> But yes or no to my solution #2 (with some slight changes to >>>> make it better (more gracious handling of the detected error) as >>>> discussed elsewhere in the email thread)? >>> >>> Please see "[0]" above! >>> >>> AFAICT your solution #2 involves bombing out *all* devices if there is >>> a duplicate compatible with no 'reg' property value. This is a) >>> over-kill and b) not an error, as I mentioned: >> >> As I mentioned above, I set you up to have this misunderstanding by >> a mistake in one of my earlier emails. So now that I have pointed >> out what I meant here by "more gracious handling of the detected >> error", what do you think of my amended solution #2? > > Explained above, but the LT;DR is that it's not correct. I don't agree with you, I think my solution is better. Even if I prefer my solution, I find your solution to be good enough. So I am dropping my specific objection to returning -EAGAIN from mfd_match_of_node_to_dev() when the node has previously been allocated to a device. > >>>>> It also suffers with false positives. >>> >> >> Sorry for the very long response, but it seemed we were operating >> under some different understandings and I hope I have clarified some >> things. > > Likewise. :) >
Re: RFC: KTAP documentation - expected messages
On 2020-06-21 17:49, Frank Rowand wrote:
> On 2020-06-21 17:45, Frank Rowand wrote:
>> Tim Bird started a thread [1] proposing that he document the selftest result
>> format used by Linux kernel tests.
>>
>> [1]
>> https://lore.kernel.org/r/cy4pr13mb1175b804e31e502221bc8163fd...@cy4pr13mb1175.namprd13.prod.outlook.com
>>
>> The issue of messages generated by the kernel being tested (that are not
>> messages directly created by the tests, but are instead triggered as a
>> side effect of the test) came up. In this thread, I will call these
>> messages "expected messages". Instead of sidetracking that thread with
>> a proposal to handle expected messages, I am starting this new thread.
>>
>> I implemented an API for expected messages that are triggered by tests
>> in the Devicetree unittest code, with the expectation that the specific
>> details may change when the Devicetree unittest code adapts the KUnit
>> API. It seems appropriate to incorporate the concept of expected
>> messages in Tim's documentation instead of waiting to address the
>> subject when the Devicetree unittest code adapts the KUnit API, since
>> Tim's document may become the kernel selftest standard.
>>
>> Instead of creating a very long email containing multiple objects,
>> I will reply to this email with a separate reply for each of:
>>
>> The "expected messages" API implemention and use can be from
>> drivers/of/unittest.c in the mainline kernel.
>>
>> of_unittest_expect - A proof of concept perl program to filter console
>>output containing expected messages output
>>
>>of_unittest_expect is also available by cloning
>>https://github.com/frowand/dt_tools.git
>>
>> An example raw console output with timestamps and expect messages.
>>
>> An example of console output processed by filter program
>> of_unittest_expect to be more human readable. The expected
>> messages are not removed, but are flagged.
>>
>> An example of console output processed by filter program
>> of_unittest_expect to be more human readable. The expected
>> messages are removed instead of being flagged.
>>
>
> reply 1/5
>
> expected messages API:
>
> - execute EXPECT_BEGIN(), reporting the expected message, before the
> point when the message will occur
>
> - execute EXPECT_END(), reporting the same expected message, after the
> point when the message will occur
>
> - EXPECT_BEGIN() may occur multiple times, before the corresponding
> EXPECT_END()s, when a single test action may result in multiple
> expected messages
>
> - When multiple EXPECT_BEGIN()s are nested, the corresponding (matching)
> EXPECT_END()s occur in the inverse order of the EXPECT_BEGIN()s.
>
> - When the expected message contain a non-constant value, a place holder
> can be placed in the message. Current place holders are:
>
> - <> an integer
> - <> a hexadecimal number
>
> Suggested additional place holder(s) are:
>
>- <> contiguous non white space characters
>
>I have avoided allowing regular expessions, because test frameworks
>may implement their own filtering instead of relying on a generic
>console output filter program. There are multiple definitions for
>regular expressions in different languages, thus it could be
>difficult to set rules for a subset of regular expression usable
>by all languages.
>
> A preliminary version of an expected messages framework has been
> implemented in the mainline drivers/of/unittest.c. The implementation
> is trivial, as seen below.
>
> Note that the define of "pr_fmt()" pre-dates the implementation
> of the EXPECT_BEGIN() and EXPECT_END() macros.
> ---
>
> #define pr_fmt(fmt) "### dt-test ### " fmt
>
>
> /*
> * Expected message may have a message level other than KERN_INFO.
> * Print the expected message only if the current loglevel will allow
> * the actual message to print.
> *
> * Do not use EXPECT_BEGIN() or EXPECT_END() for messages generated by
> * pr_debug().
> */
> #define EXPECT_BEGIN(level, fmt, ...) \
> printk(level pr_fmt("EXPECT \\ : ") fmt, ##__VA_ARGS__)
>
> #define EXPECT_END(level, fmt, ...) \
> printk(level pr_fmt("EXPECT / : ") fmt, ##__VA_ARGS__)
>
>
>
> Example 1 of the API use, single message:
>
Re: RFC: KTAP documentation - expected messages
+Dmitry, since Brendan added him to another reply at this thread level On 2020-06-22 21:46, David Gow wrote: > On Mon, Jun 22, 2020 at 6:45 AM Frank Rowand wrote: >> >> Tim Bird started a thread [1] proposing that he document the selftest result >> format used by Linux kernel tests. >> >> [1] >> https://lore.kernel.org/r/cy4pr13mb1175b804e31e502221bc8163fd...@cy4pr13mb1175.namprd13.prod.outlook.com >> >> The issue of messages generated by the kernel being tested (that are not >> messages directly created by the tests, but are instead triggered as a >> side effect of the test) came up. In this thread, I will call these >> messages "expected messages". Instead of sidetracking that thread with >> a proposal to handle expected messages, I am starting this new thread. > > Thanks for doing this: I think there are quite a few tests which could > benefit from something like this. > > I think there were actually two separate questions: what do we do with > unexpected messages (most of which I expect are useless, but some of > which may end up being related to an unexpected test failure), and how > to have tests "expect" a particular message to appear. I'll stick to Yes. But there is also a third aspect that made this feature important for the Devicetree unittest. There was a question on the devicetree mail list, asking whether some devicetree related kernel warning and/or error messages were devicetree problems. The messages appeared while the unittest was executing, but at the same time a lot of system initialization is in progress, resulting in lots of console messages that are unrelated to unittest. I could not in good conscious reply that the messages were truly of no consequence without actually chasing each of them down and verifying that they were triggered by unittest, and were showing what the devicetree infrastructure should be reporting in response to the test stimulus, vs an underlying bug in the devicetree infrastructure. I found the expected messages API to be a useful tool to document the validity of the messages, both for myself, and for the random developer who might be reading the boot messages. > talking about the latter for this thread, but even there there's two > possible interpretations of "expected messages" we probably want to > explicitly distinguish between: a message which must be present for > the test to pass (which I think best fits the "expected message" > name), and a message which the test is likely to produce, but which > shouldn't alter the result (an "ignored message"). This type of case was the impetus for me to create the API. There was a unittest that resulted in the probe of a device, where the probe executed devicetree subsystem code that invoked a blocking_notifier_call_chain(), that resulted in another subsystem taking some action, and that action just happened to do a printk() reporting a specific action that the unittest was trying to verify. I was able to verify much of the asynchronous activity by creating a fake driver and corresponding devices to be probed and could instrument the fake driver. The printk() information provided the last little bit of checking for correct behavior. > I don't see much > use for the latter at present, but if we wanted to do more things with > messages and had some otherwise very verbose tests, it could > potentially be useful. The use for the "ignored message" is my third aspect above. This points out that yet another possible consumer of the console boot log is the QA or test engineer. They can have the same concerns as any "random developer". > > The other thing I'd note here is that this proposal seems to be doing > all of the actual message filtering in userspace, which makes a lot of > sense for kselftest tests, but does mean that the kernel can't know if > the test has passed or failed. Yes. I had absolutely no interest in my test code examining the history of console messages, which could be generated on any other processor. The printk related code has always been complex, nuanced, and seems to attract the attention of people who want to change it instead of leaving it stable. I would really like to stay away from any dependency on it. > There's definitely a tradeoff between > trying to put too much needless string parsing in the kernel and > having to have a userland tool determine the test results. The > proposed KCSAN test suite[1] is using tracepoints to do this in the > kernel. It's not the cleanest thing, but there's no reason KUnit or > similar couldn't implement a nicer API around it. My interest is in printk() based messages that are present in areas outside of my test code and independent of my test code. I specifically did not want to modify that existin
Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match devices with the correct of_nodes
On 2020-06-11 14:10, Lee Jones wrote:
> Currently, when a child platform device (sometimes referred to as a
> sub-device) is registered via the Multi-Functional Device (MFD) API,
> the framework attempts to match the newly registered platform device
> with its associated Device Tree (OF) node. Until now, the device has
> been allocated the first node found with an identical OF compatible
> string. Unfortunately, if there are, say for example '3' devices
> which are to be handled by the same driver and therefore have the same
> compatible string, each of them will be allocated a pointer to the
> *first* node.
As you mentioned elsewhere in this thread, this series "fixes" the
problem related to the "stericsson,ab8500-pwm" compatible.
I know, I said I would drop discussion of that compatible, but bear
with me for a second. :-)
The "problem" is that the devices for multiple mfd child nodes with
the same compatible value of "stericsson,ab8500-pwm" will all have
a pointer to the first child node. At the moment the same child
of_node being used by more than one device does not cause any
incorrect behavior.
Just in case the driver for "stericsson,ab8500-pwm" is modified
in a way that the child of_node needs to be distinct for each
device, and that changes gets back ported, it would be useful
to have Fixes: tags for this patch series.
So, at your discretion (and I'll let you worry about the correct
Fixes: tag format), this series fixes:
bad76991d7847b7877ae797cc79745d82ffd9120 mfd: Register ab8500 devices using the
newly DT:ed MFD API
c94bb233a9fee3314dc5d9c7de9fa702e91283f2 mfd: Make MFD core code Device Tree
and IRQ domain aware
-Frank
>
> An example Device Tree entry might look like this:
>
> mfd_of_test {
> compatible = "mfd,of-test-parent";
> #address-cells = <0x02>;
> #size-cells = <0x02>;
>
> child@ {
> compatible = "mfd,of-test-child";
> reg = <0x 0x 0 0x11>,
> <0x 0x 0 0x22>;
> };
>
> child@ {
> compatible = "mfd,of-test-child";
> reg = <0x 0x 0 0x33>;
> };
>
> child@ {
> compatible = "mfd,of-test-child";
> reg = <0x 0x 0 0x44>;
> };
> };
>
> When used with example sub-device registration like this:
>
> static const struct mfd_cell mfd_of_test_cell[] = {
> OF_MFD_CELL("mfd-of-test-child", NULL, NULL, 0, 0,
> "mfd,of-test-child"),
> OF_MFD_CELL("mfd-of-test-child", NULL, NULL, 0, 1,
> "mfd,of-test-child"),
> OF_MFD_CELL("mfd-of-test-child", NULL, NULL, 0, 2,
> "mfd,of-test-child")
> };
>
> ... the current implementation will result in all devices being allocated
> the first OF node found containing a matching compatible string:
>
> [0.712511] mfd-of-test-child mfd-of-test-child.0: Probing platform device: 0
> [0.712710] mfd-of-test-child mfd-of-test-child.0: Using OF node:
> child@
> [0.713033] mfd-of-test-child mfd-of-test-child.1: Probing platform device: 1
> [0.713381] mfd-of-test-child mfd-of-test-child.1: Using OF node:
> child@
> [0.713691] mfd-of-test-child mfd-of-test-child.2: Probing platform device: 2
> [0.713889] mfd-of-test-child mfd-of-test-child.2: Using OF node:
> child@
>
> After this patch each device will be allocated a unique OF node:
>
> [0.712511] mfd-of-test-child mfd-of-test-child.0: Probing platform device: 0
> [0.712710] mfd-of-test-child mfd-of-test-child.0: Using OF node:
> child@
> [0.713033] mfd-of-test-child mfd-of-test-child.1: Probing platform device: 1
> [0.713381] mfd-of-test-child mfd-of-test-child.1: Using OF node:
> child@
> [0.713691] mfd-of-test-child mfd-of-test-child.2: Probing platform device: 2
> [0.713889] mfd-of-test-child mfd-of-test-child.2: Using OF node:
> child@
>
> Which is fine if all OF nodes are identical. However if we wish to
> apply an attribute to particular device, we really need to ensure the
> correct OF node will be associated with the device containing the
> correct address. We accomplish this by matching the device's address
> expressed in DT with one provided during sub-device registration.
> Like this:
>
> static const struct mfd_cell mfd_of_test_cell[] = {
> OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 1,
> "mfd,of-test-child", 0x),
> OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 2,
> "mfd,of-test-child", 0x),
> OF_MFD_CELL_REG("mfd-of-test-child", NULL, NULL, 0, 3,
> "mfd,of-test-child", 0x)
> };
>
> This will ensure a specific device (designated here using the
> platform_ids; 1, 2 and 3) is matched with a particular OF node:
>
> [0.712511]
