Re: [PATCH] pci/vgaarb: make vga_is_firmware_default() arch independent

[Bjorn Helgaas]

On Mon, May 29, 2023 at 11:45:04PM +0800, Sui Jingfeng wrote:
> The vga_is_firmware_default() function will work on non-x86 architectures
> as long as the arch has UEFI GOP support, which passes the firmware
> framebuffer base address and size.
> 
> This patch makes the vga_is_firmware_default() function arch-independent.
> This could help the VGAARB subsystem make the right choice for multiple
> GPU systems. Usually an integrated one and a discrete one for desktop
> computers. Depending on the firmware framebuffer being put into which
> GPU's VRAM, VGAARB could inherit the firmware's choice, which in turn,
> is the exact choice of the user.

Is there a system that needs this change?  If so, the commit log
should mention it.

It's definitely nice to remove #ifdefs, but it's better if we have an
actual reason and some testing of another arch that makes use of this.

Also, take a look at the git history and match the subject line and
commit log style (prefix, capitalization, imperative voice).

> Signed-off-by: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> ---
>  drivers/pci/vgaarb.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/vgaarb.c b/drivers/pci/vgaarb.c
> index 5a696078b382..f81b6c54e327 100644
> --- a/drivers/pci/vgaarb.c
> +++ b/drivers/pci/vgaarb.c
> @@ -61,7 +61,6 @@ static bool vga_arbiter_used;
>  static DEFINE_SPINLOCK(vga_lock);
>  static DECLARE_WAIT_QUEUE_HEAD(vga_wait_queue);
>  
> -
>  static const char *vga_iostate_to_str(unsigned int iostate)
>  {
>  	/* Ignore VGA_RSRC_IO and VGA_RSRC_MEM */
> @@ -545,7 +544,6 @@ EXPORT_SYMBOL(vga_put);
>  
>  static bool vga_is_firmware_default(struct pci_dev *pdev)
>  {
> -#if defined(CONFIG_X86) || defined(CONFIG_IA64)
>  	u64 base = screen_info.lfb_base;
>  	u64 size = screen_info.lfb_size;
>  	struct resource *r;
> @@ -571,7 +569,7 @@ static bool vga_is_firmware_default(struct pci_dev *pdev)
>  
>  		return true;
>  	}
> -#endif
> +
>  	return false;
>  }
>  
> @@ -865,8 +863,7 @@ static bool vga_arbiter_del_pci_device(struct pci_dev *pdev)
>  }
>  
>  /* this is called with the lock */
> -static inline void vga_update_device_decodes(struct vga_device *vgadev,
> -					     int new_decodes)
> +static void vga_update_device_decodes(struct vga_device *vgadev, int new_decodes)

I don't mind removing the "inline" here, but it shouldn't be combined
with the rest of the patch.  When it's combined, I can't tell whether
there's a reason we need this change or if it's just a cleanup.

>  {
>  	struct device *dev = &vgadev->pdev->dev;
>  	int old_decodes, decodes_removed, decodes_unlocked;
> -- 
> 2.25.1
>