Re: [PATCH] PCI/VGA: Select SCREEN_INFO

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3145 bytes --]

On Mon, 13 Oct 2025, Thomas Zimmermann wrote:
> Am 13.10.25 um 16:35 schrieb Ilpo Järvinen:
> > On Mon, 13 Oct 2025, Mario Limonciello wrote:
> > 
> > > On 10/13/25 9:16 AM, Ilpo Järvinen wrote:
> > > > On Mon, 13 Oct 2025, Mario Limonciello (AMD) wrote:
> > > > 
> > > > > commit 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with
> > > > > a screen info check") introduced an implicit dependency upon
> > > > > SCREEN_INFO
> > > > > by removing the open coded implementation.
> > > > > 
> > > > > If a user didn't have CONFIG_SCREEN_INFO set vga_is_firmware_default()
> > > > > would now return false.  Add a select for SCREEN_INFO to ensure that
> > > > > the
> > > > > VGA arbiter works as intended.
> > > > > 
> > > > > Reported-by: Eric Biggers <ebiggers@kernel.org>
> > > > > Closes: https://lore.kernel.org/linux-pci/20251012182302.GA3412@sol/
> > > > > Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> > > > > Fixes: 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default() with
> > > > > a
> > > > > screen info check")
> > > > > Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> > > > > ---
> > > > >    drivers/pci/Kconfig | 1 +
> > > > >    1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> > > > > index 7065a8e5f9b14..c35fed47addd5 100644
> > > > > --- a/drivers/pci/Kconfig
> > > > > +++ b/drivers/pci/Kconfig
> > > > > @@ -306,6 +306,7 @@ config VGA_ARB
> > > > >    	bool "VGA Arbitration" if EXPERT
> > > > >    	default y
> > > > >    	depends on (PCI && !S390)
> > > > > +	select SCREEN_INFO
> > > > >    	help
> > > > >    	  Some "legacy" VGA devices implemented on PCI typically have
> > > > > the same
> > > > >    	  hard-decoded addresses as they did on ISA. When multiple PCI
> > > > > devices
> > > > The commit 337bf13aa9dda ("PCI/VGA: Replace vga_is_firmware_default()
> > > > with
> > > > a screen info check") also changed to #ifdef CONFIG_SCREEN_INFO around
> > > > the
> > > > call, but that now becomes superfluous with this select, no?
> > > Thanks! Will adjust.
> 
> Yes, thanks. You can no longer run into this #else.
> 
> > > 
> > > > Looking into the history of the ifdefs here is quite odd pattern (only
> > > > the last one comes with some explanation but even that is on the vague
> > > > side and fails to remove the actual now unnecessary ifdef :-/):
> > > > 
> > > > #if defined(CONFIG_X86) -> #if defined(CONFIG_X86) -> select SCREEN_INFO
> > > > 
> > > > Was it intentional to allow building without CONFIG_SCREEN_INFO?
> > > > 
> > > You mean in the VGA arbiter code?  Or just in general?
> > Here in the VGA arbiter. I'm just trying to understand why the #else part
> > was here post-337bf13aa9dda.
> 
> The #else branch used to return false (i.e., the device is not the default). 
> But this was only used internally by fbcon for minor features. So it really
> didn't matter that much.
> 
> Now we need the default for userspace, hence we likely should have an #else
> branch at all.

Thanks for the explanation.

-- 
 i.