Re: [PATCH] arm64: dts: mediatek: mt7986a-bananapi-bpi-r3: add ramoops region

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `ramoops` reserved-memory node is missing the `no-map` property, leading to initialization failure and preventing the driver from loading.
- [Low] The commit message mathematically miscalculates the memory layout and fundamentally misrepresents how `ramoops` allocates space for ECC parity.
--

commit b448b96b8ca8171732f9a19b1cb33fbb0c73f34d
Author: Martino Dell'Ambrogio <tillo@tillo.ch>

arm64: dts: mediatek: mt7986a-bananapi-bpi-r3: add ramoops region

This patch reserves a 64 KiB memory region for pstore/ramoops on the
BPI-R3 board. The carve-out is intended to store kernel log data to
persist across warm resets.

> The carve-out is divided into 8 KiB kmsg records, a 32 KiB rolling console,
> 8 KiB ftrace, and 8 KiB pmsg, leaving the remainder for the per-record
> Reed-Solomon ECC parity (ecc-size=16) which significantly improves dump
> readability when the panic path truncates writes mid-record.

[Severity: Low]
Is this description of the ECC parity location accurate?

With 32 KiB for the console, 8 KiB for ftrace, 8 KiB for pmsg, and two 8 KiB
kmsg records, the allocations sum to exactly 64 KiB, leaving 0 bytes of
remainder.

It appears the ramoops driver implements ECC by carving the ecc-size
directly out of the payload capacity of each individual zoned region, rather
than placing it in a separate remainder pool.

Could the commit message be updated to clarify how the ECC parity is actually
allocated within the zones?

> diff --git a/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3.dts b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3.dts
> index 19f538d160ab5..31ee189dedc73 100644
> --- a/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt7986a-bananapi-bpi-r3.dts

[ ... ]

> +&{/reserved-memory} {
> +	ramoops@42ff0000 {
> +		compatible = "ramoops";
> +		reg = <0 0x42ff0000 0 0x10000>;
> +		record-size = <0x2000>;
> +		console-size = <0x8000>;
> +		ftrace-size = <0x2000>;
> +		pmsg-size = <0x2000>;
> +		ecc-size = <16>;
> +	};
> +};

[Severity: High]
Does this reserved memory node need the no-map property to function correctly?

Since this region sits within the system DRAM starting at 0x40000000,
without no-map the OS will incorporate it into the kernel's linear mapping
as Normal Cacheable memory.

When the pstore/ramoops driver attempts to map this region using ioremap_wc(),
will ioremap_prot() on ARM64 detect the attempt to remap active RAM with
different memory attributes and trigger an aliasing violation warning,
returning NULL and preventing the driver from loading?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260528093038.1945245-1-tillo@tillo.ch?part=1