linux-kernel - Re: [PATCH] pci/vgaarb: make vga_is_firmware

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZHZZ1qETtCOlmkXU@bhelgaas>
Date:   Tue, 30 May 2023 15:17:26 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Sui Jingfeng <suijingfeng@...ngson.cn>
Cc:     Bjorn Helgaas <bhelgaas@...gle.com>,
        loongson-kernel@...ts.loongnix.cn, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        Li Yi <liyi@...ngson.cn>
Subject: Re: [PATCH] pci/vgaarb: make vga_is_firmware_default() arch
 independent

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@...ngson.cn>
> ---
>  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
>