Re: [PATCH] PCI/VGA: Fix boot device detection after framebuffer relocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] PCI/VGA: premature and duplicate screen_info_apply_fixups() in vga_arb_device_init()
- [Medium] PCI/VGA: undefined reference to screen_info_apply_fixups() when !SCREEN_INFO
--

commit da40d2d05299ef123ed4789d2ecb58829165fbe3
Author: wuqianhai <wuqianhai@loongson.cn>

PCI/VGA: Fix boot device detection after framebuffer relocation

This commit calls screen_info_apply_fixups() in vga_arb_device_init() so that
screen_info's lfb_base is updated before VGA devices are registered and
checked by vga_is_firmware_default().

> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 3e58b2c05c854..aca594651f580 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -1527,6 +1527,14 @@ static int __init vga_arb_device_init(void)
>  	int rc;
>  	struct pci_dev *pdev;
>  
> +	/*
> +	 * PCI resource assignment may have relocated the firmware
> +	 * framebuffer BAR. Update screen_info's lfb_base so that
> +	 * vga_is_firmware_default() can match it against the boot
> +	 * device's relocated resource.
> +	 */
> +	screen_info_apply_fixups();

[Severity: Medium]
Will calling screen_info_apply_fixups() unconditionally in
vga_arb_device_init() fail to link when CONFIG_PCI=y, CONFIG_VGA_ARB=y, and
CONFIG_SCREEN_INFO=n?

In drivers/pci/Kconfig, VGA_ARB only selects SCREEN_INFO on x86:

drivers/pci/Kconfig:
	config VGA_ARB
		bool "VGA Arbitration" if EXPERT
		default y
		depends on !S390
		select SCREEN_INFO if X86

which matches the CONFIG_X86 guard in vga_is_firmware_default():

drivers/pci/vgaarb.c:vga_is_firmware_default() {
#if defined CONFIG_X86
	return pdev == screen_info_pci_dev(&sysfb_primary_display.screen);
#else
	return false;
#endif
}

However, include/linux/screen_info.h declares screen_info_apply_fixups() as an
external function whenever CONFIG_PCI is enabled, while
drivers/video/Makefile only builds screen_info_pci.o when CONFIG_SCREEN_INFO
is enabled:

include/linux/screen_info.h:
#if defined(CONFIG_PCI)
void screen_info_apply_fixups(void);
struct pci_dev *screen_info_pci_dev(const struct screen_info *si);
#else
static inline void screen_info_apply_fixups(void)
{ }

drivers/video/Makefile:
obj-$(CONFIG_SCREEN_INFO)         += screen_info.o
...
screen_info-$(CONFIG_PCI)         += screen_info_pci.o

On non-x86 architectures with CONFIG_PCI=y, CONFIG_VGA_ARB=y, and
CONFIG_SCREEN_INFO=n, doesn't this result in an undefined reference to
screen_info_apply_fixups()?

[Severity: High]
Also, is subsys_initcall_sync(vga_arb_device_init) the right place in the
initcall sequence to call screen_info_apply_fixups()?

Upstream vga_is_firmware_default() in drivers/pci/vgaarb.c only calls
screen_info_pci_dev() under CONFIG_X86. On x86, subsys_initcall(pci_subsys_init)
runs pcibios_resource_survey() -> pcibios_allocate_dev_resources(). When the
firmware framebuffer BAR has a resource conflict and awaits reassignment,
pcibios_allocate_dev_resources() saves the firmware address and resets
r->start to 0 while keeping resource_size(r) intact:

arch/x86/pci/i386.c:pcibios_allocate_dev_resources() {
	...
	/* We'll assign a new address later */
	pcibios_save_fw_addr(dev, idx, r->start);
	r->end -= r->start;
	r->start = 0;
	...
}

The new BAR address on x86 is not assigned until
fs_initcall(pcibios_assign_resources) -> pci_assign_unassigned_resources(),
which runs after subsys_initcall_sync(vga_arb_device_init).

When screen_info_apply_fixups() runs at subsys_initcall_sync while a
conflicting BAR has pr->start == 0, pr->start != screen_info_lfb_res_start is
true and __screen_info_relocation_is_valid(si, pr) succeeds because
resource_size(pr) is unchanged:

drivers/video/screen_info_pci.c:screen_info_apply_fixups() {
	if (screen_info_lfb_pdev) {
		struct resource *pr = &screen_info_lfb_pdev->resource[screen_info_lfb_bar];

		if (pr->start != screen_info_lfb_res_start) {
			if (__screen_info_relocation_is_valid(si, pr)) {
				__screen_info_set_lfb_base(si, pr->start + screen_info_lfb_offset);
				pr_info("Relocating firmware framebuffer to offset %pa[d] within %pr\n",
					&screen_info_lfb_offset, pr);
			}
			...
		}
	}
}

Wouldn't this clobber si->lfb_base to 0 + screen_info_lfb_offset with a
[mem 0x00000000-...] message before fs_initcall(pcibios_assign_resources)
assigns the actual BAR address, while still failing to match in
vga_is_firmware_default()?

Similarly, for PCI host bridges probed at device_initcall via
pci_host_probe(), vga_arb_device_init() runs before the PCI bus is scanned
and before VGA devices are added via pci_notify().

Finally, since screen_info_apply_fixups() does not update
screen_info_lfb_res_start or clear screen_info_lfb_pdev after applying the
fixup, won't calling it in both vga_arb_device_init() and
device_initcall(sysfb_init) in drivers/firmware/sysfb.c cause the relocation
and log messages to run twice?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028094634.766723-1-wuqianhai@loongson.cn?part=1