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 <[email protected]> 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/[email protected]?part=1
