Re: [PATCH RFC v4 06/13] dt-bindings: display: add a boot logo node under /chosen

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dt-bindings: display: boot-logo memory zeroing leads to spurious drm_err
--

commit 3ef097b6e764420e6247f9d9dcc28aba1f88538d
Author: Màxim Pedraza Padilla <maximpedraza@gmail.com>

dt-bindings: display: add a boot logo node under /chosen

Products built on the same board often differ only in branding. On UEFI
systems the firmware hands the operating system its boot logo through
the ACPI BGRT; a device tree system has no such table, so today the logo
is built into the kernel, and each product needs its own kernel image.

> diff --git a/Documentation/devicetree/bindings/display/boot-logo.yaml b/Documentation/devicetree/bindings/display/boot-logo.yaml
> new file mode 100644
> index 0000000000000..b540a04deb789
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/boot-logo.yaml
> @@ -0,0 +1,151 @@
[ ... ]
> +  memory-region:
> +    maxItems: 1
> +    description: |
> +      Reserved memory region the bootloader loaded the BMP file into, starting
> +      at the beginning of the region. The BMP header says how much of the
> +      region is image.
> +
> +      This keeps the image out of the device tree, so that it can be changed
> +      without rebuilding the device tree, for instance by loading it from a
> +      partition of its own that userspace can update.
> +
> +      Memory keeps its contents across a reset, and may even across a short
> +      power cycle, so a bootloader that loads no image into the region has to
> +      clear it: the operating system cannot tell a stale image from a fresh
> +      one.

[Severity: Medium]
Does clearing the memory region result in the OS trying to parse zeroed memory?

If the bootloader follows this documentation and zeroes the memory region when
there is no image, the kernel will still parse the static /chosen/logo node and
map the memory region.

This causes drm_splash_bmp_to_scanout() in drivers/gpu/drm/clients/drm_splash.c
to read 0x0000 for the magic bytes:

	bmp_id = get_unaligned_le16(&file_header->id);
	if (bmp_id != BMP_FILE_MAGIC_ID) {
		drm_err(client->dev, "splash: invalid BMP magic 0x%04X", bmp_id);
		return -EINVAL;
	}

Because of this, the kernel will emit a spurious error on every boot where no
splash is provided.

Would it be better for the bootloader to dynamically inject the /chosen/logo
node or the memory-region property only when an image is successfully loaded,
rather than relying on the OS to parse and reject zeroed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001195847.141192-1-maximpedraza@gmail.com?part=6