| Message ID | 20240305093628.1094525-1-christian.taedcke-oss@weidmueller.com |
|---|---|
| State | Accepted, archived |
| Commit | f8f3a52b2f924789552e6a3f889162ff07e0887f |
| Headers | show |
| Series | kernel-fitImage: only include valid compatible line | expand |
Hello Christian, On 05.03.24 10:36, Taedcke, Christian wrote: > From: Christian Taedcke <christian.taedcke@weidmueller.com> > > Without this commit the configuration node includes the compatible > line 'compatible = [00];' if EXTERNAL_KERNEL_DEVICETREE is not > defined, i.e. if PREFERRED_PROVIDER_virtual/dtb is not used. > This prevents u-boot from using this configuration and it prints the > message "Could not find configuration node". > > The functionality to add the compatible line was added in commit > f4c82fb6da89 ("kernel-fitImage: add machine compatible to config > section"). > > Signed-off-by: Christian Taedcke <christian.taedcke@weidmueller.com> > --- > dtb_path="${EXTERNAL_KERNEL_DEVICETREE}/${dtb_image_sect}" > - compatible_line="compatible = \"$(fdtget "$dtb_path" / compatible | sed 's/ /", "/g')\";" > + if [ -e "$dtb_path" ]; then > + compatible_line="compatible = \"$(fdtget "$dtb_path" / compatible | sed 's/ /", "/g')\";" > + fi The change is ok, but I'd prefer we harden this against errors a bit more: if [ -e "$dtb_path" ]; then compat=$(fdtget -t -s "$dtb_path" / compatible | sed 's/ /", "/g') if [ -n "$compat" ]; then compatible_line="compatible = \"compat\";" fi fi This should ensure that compatible = "" only results when the DT indeed has compatible = "". What do you think? Thanks, Ahmad > > dtb_image=$(echo $dtb_image | tr '/' '_') > dtb_image_sect=$(echo "${dtb_image_sect}" | tr '/' '_') > > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#196628): https://lists.openembedded.org/g/openembedded-core/message/196628 > Mute This Topic: https://lists.openembedded.org/mt/104741166/4830399 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [a.fatoum@pengutronix.de] > -=-=-=-=-=-=-=-=-=-=-=- >
On 06.03.24 14:39, Ahmad Fatoum wrote: > Hello Christian, > > On 05.03.24 10:36, Taedcke, Christian wrote: >> From: Christian Taedcke <christian.taedcke@weidmueller.com> >> >> Without this commit the configuration node includes the compatible >> line 'compatible = [00];' if EXTERNAL_KERNEL_DEVICETREE is not >> defined, i.e. if PREFERRED_PROVIDER_virtual/dtb is not used. >> This prevents u-boot from using this configuration and it prints the >> message "Could not find configuration node". >> >> The functionality to add the compatible line was added in commit >> f4c82fb6da89 ("kernel-fitImage: add machine compatible to config >> section"). >> >> Signed-off-by: Christian Taedcke <christian.taedcke@weidmueller.com> >> --- > >> dtb_path="${EXTERNAL_KERNEL_DEVICETREE}/${dtb_image_sect}" >> - compatible_line="compatible = \"$(fdtget "$dtb_path" / compatible | sed 's/ /", "/g')\";" >> + if [ -e "$dtb_path" ]; then >> + compatible_line="compatible = \"$(fdtget "$dtb_path" / compatible | sed 's/ /", "/g')\";" >> + fi > > The change is ok, but I'd prefer we harden this against errors a bit more: > > if [ -e "$dtb_path" ]; then > compat=$(fdtget -t -s "$dtb_path" / compatible | sed 's/ /", "/g') > if [ -n "$compat" ]; then > compatible_line="compatible = \"compat\";" > fi > fi > > This should ensure that compatible = "" only results when the DT indeed has compatible = "". Scratch that line. This should ensure we never get an empty compatible (or one with a wrong type). > What do you think? > > Thanks, > Ahmad > >> >> dtb_image=$(echo $dtb_image | tr '/' '_') >> dtb_image_sect=$(echo "${dtb_image_sect}" | tr '/' '_') >> >> >> >> >> > > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#196678): https://lists.openembedded.org/g/openembedded-core/message/196678 > Mute This Topic: https://lists.openembedded.org/mt/104741166/4830399 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [a.fatoum@pengutronix.de] > -=-=-=-=-=-=-=-=-=-=-=- >
Hello Ahmad, On 06.03.2024 14:40, Ahmad Fatoum via lists.openembedded.org wrote: > On 06.03.24 14:39, Ahmad Fatoum wrote: >> Hello Christian, >> >> On 05.03.24 10:36, Taedcke, Christian wrote: >>> From: Christian Taedcke <christian.taedcke@weidmueller.com> >>> >>> Without this commit the configuration node includes the compatible >>> line 'compatible = [00];' if EXTERNAL_KERNEL_DEVICETREE is not >>> defined, i.e. if PREFERRED_PROVIDER_virtual/dtb is not used. >>> This prevents u-boot from using this configuration and it prints the >>> message "Could not find configuration node". >>> >>> The functionality to add the compatible line was added in commit >>> f4c82fb6da89 ("kernel-fitImage: add machine compatible to config >>> section"). >>> >>> Signed-off-by: Christian Taedcke <christian.taedcke@weidmueller.com> >>> --- >> >>> dtb_path="${EXTERNAL_KERNEL_DEVICETREE}/${dtb_image_sect}" >>> - compatible_line="compatible = \"$(fdtget "$dtb_path" / compatible | sed 's/ /", "/g')\";" >>> + if [ -e "$dtb_path" ]; then >>> + compatible_line="compatible = \"$(fdtget "$dtb_path" / compatible | sed 's/ /", "/g')\";" >>> + fi >> >> The change is ok, but I'd prefer we harden this against errors a bit more: >> >> if [ -e "$dtb_path" ]; then >> compat=$(fdtget -t -s "$dtb_path" / compatible | sed 's/ /", "/g') >> if [ -n "$compat" ]; then >> compatible_line="compatible = \"compat\";" >> fi >> fi >> >> This should ensure that compatible = "" only results when the DT indeed has compatible = "". > > Scratch that line. This should ensure we never get an empty compatible > (or one with a wrong type). > >> What do you think? Thanks for your suggestion, i will prepare a new patch. Regards, Christian >> >> Thanks, >> Ahmad >> >>> >>> dtb_image=$(echo $dtb_image | tr '/' '_') >>> dtb_image_sect=$(echo "${dtb_image_sect}" | tr '/' '_') >>> >>> >>> >>> >>> >> >> >> >> >> > > > > -=-=-=-=-=-=-=-=-=-=-=- > Links: You receive all messages sent to this group. > View/Reply Online (#196679): https://lists.openembedded.org/g/openembedded-core/message/196679 > Mute This Topic: https://lists.openembedded.org/mt/104741166/7654653 > Group Owner: openembedded-core+owner@lists.openembedded.org > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [christian.taedcke-oss@weidmueller.com] > -=-=-=-=-=-=-=-=-=-=-=- >
diff --git a/meta/classes-recipe/kernel-fitimage.bbclass b/meta/classes-recipe/kernel-fitimage.bbclass index 7e30a5d47e..31e34716d5 100644 --- a/meta/classes-recipe/kernel-fitimage.bbclass +++ b/meta/classes-recipe/kernel-fitimage.bbclass @@ -421,6 +421,7 @@ fitimage_emit_section_config() { bootscr_line="" setup_line="" default_line="" + compatible_line="" dtb_image_sect=$(symlink_points_below $dtb_image "${EXTERNAL_KERNEL_DEVICETREE}") if [ -z "$dtb_image_sect" ]; then @@ -428,7 +429,9 @@ fitimage_emit_section_config() { fi dtb_path="${EXTERNAL_KERNEL_DEVICETREE}/${dtb_image_sect}" - compatible_line="compatible = \"$(fdtget "$dtb_path" / compatible | sed 's/ /", "/g')\";" + if [ -e "$dtb_path" ]; then + compatible_line="compatible = \"$(fdtget "$dtb_path" / compatible | sed 's/ /", "/g')\";" + fi dtb_image=$(echo $dtb_image | tr '/' '_') dtb_image_sect=$(echo "${dtb_image_sect}" | tr '/' '_')