On ARM64 systems coreboot defers framebuffer allocation to its payload,
to be done by a libpayload function call. In this case, coreboot tables
still include a framebuffer entry with display format details, but the
physical address field is set to zero (as in [1], for example).

Unfortunately, this field is not automatically updated when the
framebuffer is initialized through libpayload, citing that doing so
would invalidate checksums over the entire coreboot table [2].

This can be observed on ARM64 Chromebooks with stock firmware. On a
Google Kevin (RK3399), trying to use coreboot framebuffer driver as
built-in to the kernel results in a benign error. But on Google Hana
(MT8173) and Google Cozmo (MT8183) it causes a hang.

When the framebuffer physical address field in the coreboot table is
zero, we have no idea where coreboot initialized a framebuffer, or even
if it did. Instead of trying to set up a framebuffer located at zero,
return ENODEV to indicate that there isn't one.

[1] https://review.coreboot.org/c/coreboot/+/17109
[2] https://review.coreboot.org/c/coreboot/+/8797

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---
(I was previously told on #coreboot IRC that I could add coreboot
mailing list to CC for kernel patches related to coreboot.)

 drivers/firmware/google/framebuffer-coreboot.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index c323a818805c..5c84bbebfef8 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -36,6 +36,9 @@ static int framebuffer_probe(struct coreboot_device *dev)
 		.format = NULL,
 	};
 
+	if (!fb->physical_address)
+		return -ENODEV;
+
 	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
 		if (fb->bits_per_pixel     == formats[i].bits_per_pixel &&
 		    fb->red_mask_pos       == formats[i].red.offset &&

base-commit: 2220f68f4504aa1ccce0fac721ccdb301e9da32f
-- 
2.42.0

On Wed, Nov 08, 2023 at 09:25:13PM +0300, Alper Nebi Yasak wrote:
> On ARM64 systems coreboot defers framebuffer allocation to its payload,
> to be done by a libpayload function call. In this case, coreboot tables
> still include a framebuffer entry with display format details, but the
> physical address field is set to zero (as in [1], for example).
> 
> Unfortunately, this field is not automatically updated when the
> framebuffer is initialized through libpayload, citing that doing so
> would invalidate checksums over the entire coreboot table [2].
> 
> [...]

Applied, thanks!

[1/1] firmware: coreboot: framebuffer: Avoid invalid zero physical address
        commit: ecea08916418a94f99f89c543303877cb6e08a11

Just realized the bot didn't send the mail for the patch.

Yeah, if the kernel wanted to make use of coreboot display init on
those boards, it would have to reserve its own framebuffer space and
need to have at least enough of a driver for the display controller to
be able to tell it which address it picked. Until someone implements
that, erroring out for those cases makes sense.

Reviewed-by: Julius Werner <jwerner@chromium.org>