[PATCH] kernel-fitimage.bbclass: only package unique DTBs

[PATCH] kernel-fitimage.bbclass: only package unique DTBs

[Awais Belal]

The KERNEL_DEVICETREE and related variables could potentially have a device
tree listed multiple times and this works okay for most scenarios. However,
when we create FIT entries for these we get duplicate nodes and uboot-mkimage
fails with

fit-image-initramfs-image.its:219.58-229.19: ERROR (duplicate_node_names): /images/fdt-freescale_imx8mp-evk-ecspi-slave.dtb: Duplicate node name
fit-image-initramfs-image.its:307.50-317.19: ERROR (duplicate_node_names): /images/fdt-freescale_imx8mp-evk-ndm.dtb: Duplicate node name
fit-image-initramfs-image.its:362.54-372.19: ERROR (duplicate_node_names): /images/fdt-freescale_imx8mp-evk-rm67199.dtb: Duplicate node name
fit-image-initramfs-image.its:417.56-427.19: ERROR (duplicate_node_names): /images/fdt-freescale_imx8mp-evk-usdhc1-m2.dtb: Duplicate node name
fit-image-initramfs-image.its:648.59-658.19: ERROR (duplicate_node_names): /configurations/conf-freescale_imx8mp-evk-ecspi-slave.dtb: Duplicate node name
fit-image-initramfs-image.its:744.51-754.19: ERROR (duplicate_node_names): /configurations/conf-freescale_imx8mp-evk-ndm.dtb: Duplicate node name
fit-image-initramfs-image.its:804.55-814.19: ERROR (duplicate_node_names): /configurations/conf-freescale_imx8mp-evk-rm67199.dtb: Duplicate node name
fit-image-initramfs-image.its:864.57-874.19: ERROR (duplicate_node_names): /configurations/conf-freescale_imx8mp-evk-usdhc1-m2.dtb: Duplicate node name
ERROR: Input tree has errors, aborting (use -f to force output)
uboot-mkimage: Can't open arch/arm64/boot/fitImage.tmp: No such file or directory

We fix this by tracking the DTBs we're compiling in the FIT and only picking
up unique ones.

Signed-off-by: Awais Belal <awais_belal@mentor.com>
---
 meta/classes/kernel-fitimage.bbclass | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/meta/classes/kernel-fitimage.bbclass b/meta/classes/kernel-fitimage.bbclass
index 753164551c..2a07a57d5d 100644
--- a/meta/classes/kernel-fitimage.bbclass
+++ b/meta/classes/kernel-fitimage.bbclass
@@ -529,6 +529,14 @@ fitimage_assemble() {
 			fi
 
 			DTB=$(echo "$DTB" | tr '/' '_')
+
+			# Skip DTB if we've picked it up previously
+			case ${DTBS} in
+				*${DTB}*)
+					continue
+					;;
+			esac
+
 			DTBS="$DTBS $DTB"
 			fitimage_emit_section_dtb $1 $DTB $DTB_PATH
 		done
@@ -538,6 +546,14 @@ fitimage_assemble() {
 		dtbcount=1
 		for DTB in $(find "${EXTERNAL_KERNEL_DEVICETREE}" \( -name '*.dtb' -o -name '*.dtbo' \) -printf '%P\n' | sort); do
 			DTB=$(echo "$DTB" | tr '/' '_')
+
+			# Skip DTB if we've picked it up previously
+			case ${DTBS} in
+				*${DTB}*)
+					continue
+					;;
+			esac
+
 			DTBS="$DTBS $DTB"
 			fitimage_emit_section_dtb $1 $DTB "${EXTERNAL_KERNEL_DEVICETREE}/$DTB"
 		done
-- 
2.25.1

Re: [OE-core] [PATCH] kernel-fitimage.bbclass: only package unique DTBs

[Quentin Schulz]

Hi Awais,

On 8/8/22 12:32, Awais Belal wrote:
> The KERNEL_DEVICETREE and related variables could potentially have a device
> tree listed multiple times and this works okay for most scenarios. However,
> when we create FIT entries for these we get duplicate nodes and uboot-mkimage
> fails with
> 
> fit-image-initramfs-image.its:219.58-229.19: ERROR (duplicate_node_names): /images/fdt-freescale_imx8mp-evk-ecspi-slave.dtb: Duplicate node name
> fit-image-initramfs-image.its:307.50-317.19: ERROR (duplicate_node_names): /images/fdt-freescale_imx8mp-evk-ndm.dtb: Duplicate node name
> fit-image-initramfs-image.its:362.54-372.19: ERROR (duplicate_node_names): /images/fdt-freescale_imx8mp-evk-rm67199.dtb: Duplicate node name
> fit-image-initramfs-image.its:417.56-427.19: ERROR (duplicate_node_names): /images/fdt-freescale_imx8mp-evk-usdhc1-m2.dtb: Duplicate node name
> fit-image-initramfs-image.its:648.59-658.19: ERROR (duplicate_node_names): /configurations/conf-freescale_imx8mp-evk-ecspi-slave.dtb: Duplicate node name
> fit-image-initramfs-image.its:744.51-754.19: ERROR (duplicate_node_names): /configurations/conf-freescale_imx8mp-evk-ndm.dtb: Duplicate node name
> fit-image-initramfs-image.its:804.55-814.19: ERROR (duplicate_node_names): /configurations/conf-freescale_imx8mp-evk-rm67199.dtb: Duplicate node name
> fit-image-initramfs-image.its:864.57-874.19: ERROR (duplicate_node_names): /configurations/conf-freescale_imx8mp-evk-usdhc1-m2.dtb: Duplicate node name
> ERROR: Input tree has errors, aborting (use -f to force output)
> uboot-mkimage: Can't open arch/arm64/boot/fitImage.tmp: No such file or directory
> 
> We fix this by tracking the DTBs we're compiling in the FIT and only picking
> up unique ones.
> 
> Signed-off-by: Awais Belal <awais_belal@mentor.com>
> ---
>   meta/classes/kernel-fitimage.bbclass | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/meta/classes/kernel-fitimage.bbclass b/meta/classes/kernel-fitimage.bbclass
> index 753164551c..2a07a57d5d 100644
> --- a/meta/classes/kernel-fitimage.bbclass
> +++ b/meta/classes/kernel-fitimage.bbclass
> @@ -529,6 +529,14 @@ fitimage_assemble() {
>   			fi
>   
>   			DTB=$(echo "$DTB" | tr '/' '_')
> +
> +			# Skip DTB if we've picked it up previously
> +			case ${DTBS} in
> +				*${DTB}*)

I think this might be a bit too much. e.g. I could have a DTS/DTB named 
this-is-a.dtb.dtb and another one this-is-a.dtb and this would say it is 
a duplicate whereas it isn't (poor naming choice, though.. yes :) ).

I believe that:
for DTB in $(echo "${DTBS}" | tr ' ' '\n' | sort -u); do

would work and avoid unnecessary loops.

> +					continue
> +					;;
> +			esac
> +
>   			DTBS="$DTBS $DTB"
>   			fitimage_emit_section_dtb $1 $DTB $DTB_PATH
>   		done
> @@ -538,6 +546,14 @@ fitimage_assemble() {
>   		dtbcount=1
>   		for DTB in $(find "${EXTERNAL_KERNEL_DEVICETREE}" \( -name '*.dtb' -o -name '*.dtbo' \) -printf '%P\n' | sort); do

I believe using sort -u here should be enough for this second diff?

Cheers,
Quentin

Re: [OE-core] [PATCH] kernel-fitimage.bbclass: only package unique DTBs

[Belal, Awais]

[-- Attachment #1: Type: text/plain, Size: 4857 bytes --]

Hi Quentin,


Thanks for the feedback.


> I think this might be a bit too much. e.g. I could have a DTS/DTB named
> this-is-a.dtb.dtb and another one this-is-a.dtb and this would say it is
> a duplicate whereas it isn't (poor naming choice, though.. yes :) ).

> I believe that:
> for DTB in $(echo "${DTBS}" | tr ' ' '\n' | sort -u); do

> would work and avoid unnecessary loops.


this would definitely avoid unnecessary loops however I don't think sorting would be the right thing here as the first dt in the list gets to become the default config which is something assumed for the runtime and sorting would break that. If we want to cover up the corner case you described above would something such fit the bill


echo $DTBS | tr ' ' '\n' | grep -xq $DTB && continue


> I believe using sort -u here should be enough for this second diff?


sort -u wouldn't help here if the same name dt is present in KERNEL_DEVICETREE and the EXTERNEL_KERNEL_DEVICETREE as we'll run into the same problem of having the same dt being packaged twice. I'd rather use the same technique here if you agree.


BR,
Awais
________________________________
From: openembedded-core@lists.openembedded.org <openembedded-core@lists.openembedded.org> on behalf of Quentin Schulz <quentin.schulz@theobroma-systems.com>
Sent: Monday, August 8, 2022 4:15:15 PM
To: Belal, Awais; openembedded-core@lists.openembedded.org
Subject: Re: [OE-core] [PATCH] kernel-fitimage.bbclass: only package unique DTBs

Hi Awais,

On 8/8/22 12:32, Awais Belal wrote:
> The KERNEL_DEVICETREE and related variables could potentially have a device
> tree listed multiple times and this works okay for most scenarios. However,
> when we create FIT entries for these we get duplicate nodes and uboot-mkimage
> fails with
>
> fit-image-initramfs-image.its:219.58-229.19: ERROR (duplicate_node_names): /images/fdt-freescale_imx8mp-evk-ecspi-slave.dtb: Duplicate node name
> fit-image-initramfs-image.its:307.50-317.19: ERROR (duplicate_node_names): /images/fdt-freescale_imx8mp-evk-ndm.dtb: Duplicate node name
> fit-image-initramfs-image.its:362.54-372.19: ERROR (duplicate_node_names): /images/fdt-freescale_imx8mp-evk-rm67199.dtb: Duplicate node name
> fit-image-initramfs-image.its:417.56-427.19: ERROR (duplicate_node_names): /images/fdt-freescale_imx8mp-evk-usdhc1-m2.dtb: Duplicate node name
> fit-image-initramfs-image.its:648.59-658.19: ERROR (duplicate_node_names): /configurations/conf-freescale_imx8mp-evk-ecspi-slave.dtb: Duplicate node name
> fit-image-initramfs-image.its:744.51-754.19: ERROR (duplicate_node_names): /configurations/conf-freescale_imx8mp-evk-ndm.dtb: Duplicate node name
> fit-image-initramfs-image.its:804.55-814.19: ERROR (duplicate_node_names): /configurations/conf-freescale_imx8mp-evk-rm67199.dtb: Duplicate node name
> fit-image-initramfs-image.its:864.57-874.19: ERROR (duplicate_node_names): /configurations/conf-freescale_imx8mp-evk-usdhc1-m2.dtb: Duplicate node name
> ERROR: Input tree has errors, aborting (use -f to force output)
> uboot-mkimage: Can't open arch/arm64/boot/fitImage.tmp: No such file or directory
>
> We fix this by tracking the DTBs we're compiling in the FIT and only picking
> up unique ones.
>
> Signed-off-by: Awais Belal <awais_belal@mentor.com>
> ---
>   meta/classes/kernel-fitimage.bbclass | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/meta/classes/kernel-fitimage.bbclass b/meta/classes/kernel-fitimage.bbclass
> index 753164551c..2a07a57d5d 100644
> --- a/meta/classes/kernel-fitimage.bbclass
> +++ b/meta/classes/kernel-fitimage.bbclass
> @@ -529,6 +529,14 @@ fitimage_assemble() {
>                        fi
>
>                        DTB=$(echo "$DTB" | tr '/' '_')
> +
> +                     # Skip DTB if we've picked it up previously
> +                     case ${DTBS} in
> +                             *${DTB}*)

I think this might be a bit too much. e.g. I could have a DTS/DTB named
this-is-a.dtb.dtb and another one this-is-a.dtb and this would say it is
a duplicate whereas it isn't (poor naming choice, though.. yes :) ).

I believe that:
for DTB in $(echo "${DTBS}" | tr ' ' '\n' | sort -u); do

would work and avoid unnecessary loops.

> +                                     continue
> +                                     ;;
> +                     esac
> +
>                        DTBS="$DTBS $DTB"
>                        fitimage_emit_section_dtb $1 $DTB $DTB_PATH
>                done
> @@ -538,6 +546,14 @@ fitimage_assemble() {
>                dtbcount=1
>                for DTB in $(find "${EXTERNAL_KERNEL_DEVICETREE}" \( -name '*.dtb' -o -name '*.dtbo' \) -printf '%P\n' | sort); do

I believe using sort -u here should be enough for this second diff?

Cheers,
Quentin

[-- Attachment #2: Type: text/html, Size: 8544 bytes --]

Re: [OE-core] [PATCH] kernel-fitimage.bbclass: only package unique DTBs

[Quentin Schulz]

Hi Awais,

On 8/11/22 05:23, Belal, Awais wrote:
> Hi Quentin,
> 
> 
> Thanks for the feedback.
> 
> 
>> I think this might be a bit too much. e.g. I could have a DTS/DTB named
>> this-is-a.dtb.dtb and another one this-is-a.dtb and this would say it is
>> a duplicate whereas it isn't (poor naming choice, though.. yes :) ).
> 
>> I believe that:
>> for DTB in $(echo "${DTBS}" | tr ' ' '\n' | sort -u); do
> 
>> would work and avoid unnecessary loops.
> 
> 
> this would definitely avoid unnecessary loops however I don't think sorting would be the right thing here as the first dt in the list gets to become the default config which is something assumed for the runtime and sorting would break that. If we want to cover up the corner case you described above would something such fit the bill
> 

I completely forgot about this. I think this is a not so great behavior 
:/ But nothing to discuss in this patch :)

> 
> echo $DTBS | tr ' ' '\n' | grep -xq $DTB && continue
> 

Don't forget to quote the shell variables to make shellcheck happy :)

> 
>> I believe using sort -u here should be enough for this second diff?
> 
> 
> sort -u wouldn't help here if the same name dt is present in KERNEL_DEVICETREE and the EXTERNEL_KERNEL_DEVICETREE as we'll run into the same problem of having the same dt being packaged twice. I'd rather use the same technique here if you agree.
> 

Sounds like a plan :)

Cheers,
Quentin