Re: [PATCH v1 1/1] ARM: dts: draeger: Add devicetree files for M48 on NXP i.MX6(Q)

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Masquerading IOMUXC group control registers as pinctrl pads causes a kernel heap buffer overflow during device tree parsing.
- [High] Alignment Fault (Data Abort) triggered by an unaligned device memory read if pin configurations are read via debugfs.
- [High] The `usdhc3` node in `imx6qp-draeger-m48.dts` overrides the `compatible` property with an unrecognized string, breaking SD/eMMC support.
- [Medium] Malformed `#ifdef` block containing duplicate `bootcmd` properties.
- [Medium] The newly added device tree file `imx6qp-draeger-m48.dts` is not integrated into the build system.
- [Medium] `imx6qp-draeger-m48.dts` includes a non-existent file, causing immediate compilation failure.
--

--- Patch [1]: [PATCH v1 1/1] ARM: dts: draeger: Add devicetree files for M48 on NXP i.MX6(Q) ---
commit 23a07509f15a20dcb81e207c5e98da8aa14ffeb4
Author: Petko Manolov <petko.manolov@konsulko.com>

ARM: dts: draeger: Add devicetree files for M48 on NXP i.MX6(Q)

This commit adds device tree definitions for the Draeger M48 board,
supporting i.MX6 DualLite, Quad, and Quad Plus variants. It introduces
the base DTSI file along with specific DTS files for each variant.

> diff --git a/arch/arm/boot/dts/nxp/imx/Makefile b/arch/arm/boot/dts/nxp/imx/Makefile
> index 1a2539fa19b44..44b3932562358 100644
> --- a/arch/arm/boot/dts/nxp/imx/Makefile
> +++ b/arch/arm/boot/dts/nxp/imx/Makefile
> @@ -220,6 +220,8 @@ dtb-$(CONFIG_SOC_IMX6Q) += \
>  	imx6q-dmo-edmqmx6.dtb \
>  	imx6q-dms-ba16.dtb \
>  	imx6q-ds.dtb \
> +	imx6q-draeger-m48.dtb \
> +	imx6dl-draeger-m48.dtb \

[Severity: Medium]
Will this skip building the new imx6qp-draeger-m48.dts file?

It appears imx6qp-draeger-m48.dtb was omitted from the
dtb-$(CONFIG_SOC_IMX6Q) build list.

>  	imx6q-emcon-avari.dtb \

[ ... ]

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6dl-draeger-m48.dts b/arch/arm/boot/dts/nxp/imx/imx6dl-draeger-m48.dts
> new file mode 100644
> index 0000000000000..45809770d16f3
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6dl-draeger-m48.dts

[ ... ]

> +#include "imx6dl.dtsi"
> +/* PAD config disabled in imx6qdl-draeger-m48.dtsi by setting value to 0x80000000
> + * PAD configuration register must not be 0, so it is set to 0xfff
> + */
> +#define MX6QDL_PAD_CTL_GRP_DDR_TYPE_RGMII        0x768 0xfff 0x000 0x000C0000 0x0
> +#define MX6QDL_PAD_CTL_GRP_RGMII_TERM            0x788 0xfff 0x000 0x00000200 0x0

[Severity: High]
Will the unaligned conf_reg offset (0xfff) cause an Alignment Fault?

If the pinctrl state is later dumped via debugfs, imx_pinconf_get_mmio() 
executes readl(ipctl->base + 0xfff). On ARMv7, non-4-byte-aligned accesses
to strongly-ordered IOMEM regions strictly trigger an Alignment Fault 
hardware exception, which can crash the kernel.

> +#include "imx6qdl-draeger-m48.dtsi"

[ ... ]

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6qdl-draeger-m48.dtsi b/arch/arm/boot/dts/nxp/imx/imx6qdl-draeger-m48.dtsi
> new file mode 100644
> index 0000000000000..bcfe7b2556cbd
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6qdl-draeger-m48.dtsi

[ ... ]

> +		pinctrl_m48_enet: enetgrp {
> +			fsl,pins = <
> +				MX6QDL_PAD_KEY_COL1__ENET_MDIO 0x1b0b0

[ ... ]

> +				MX6QDL_PAD_RGMII_RX_CTL__RGMII_RX_CTL 0x13030
> +				MX6QDL_PAD_CTL_GRP_DDR_TYPE_RGMII 0x80000000
> +				MX6QDL_PAD_CTL_GRP_RGMII_TERM 0x80000000
> +			>;
> +		};

[Severity: Critical]
Does masquerading these IOMUXC group control registers as pinctrl pads cause
a kernel heap buffer overflow during device tree parsing?

The imx_pinctrl_parse_pin_mmio() driver derives an array index by dividing
the offset by 4. Since the driver allocates ipctl->pin_regs with 216
elements for i.MX6Q, injecting these large offsets (like 0x768 and 0x790)
results in array indices well past the 216-element heap allocation, leading
to an out-of-bounds write.

[ ... ]

> +	config {
> +		#ifdef LEGACY_BOOT
> +		bootcmd = "startM48;errorMsg";
> +			bootcmd = "startKernel;errorMsg";
> +		#endif
> +
> +	};
> +};

[Severity: Medium]
Is this #ifdef block missing an #else directive?

Since both bootcmd properties are inside the same branch, this will either
omit the intended default bootcmd entirely in a standard kernel build, or
fail to compile with a duplicate property error if LEGACY_BOOT is ever
defined.

> diff --git a/arch/arm/boot/dts/nxp/imx/imx6qp-draeger-m48.dts b/arch/arm/boot/dts/nxp/imx/imx6qp-draeger-m48.dts
> new file mode 100644
> index 0000000000000..ab331596abd98
> --- /dev/null
> +++ b/arch/arm/boot/dts/nxp/imx/imx6qp-draeger-m48.dts

[ ... ]

> +#include "imx6qdl-draeger-m48.dtsi"
> +#include "imx6qdl-draeger-m48-u-boot.dtsi"

[Severity: Medium]
Will this #include cause a compilation failure?

The file imx6qdl-draeger-m48-u-boot.dtsi does not appear to exist in the
Linux source tree.

[ ... ]

> +&usdhc3 {
> +	compatible = "fsl,imx6qp-usdhc";

[Severity: High]
Does this unrecognized compatible string prevent the SD/eMMC driver from
binding?

The sdhci-esdhc-imx driver expects fsl,imx6q-usdhc, so using
fsl,imx6qp-usdhc will break storage on the Quad Plus variant.

> +	vqmmc-supply = <&vccq_sdhc3>;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825140225.3968019-3-petko.manolov@konsulko.com?part=1