[v4] PCI: Support hibmc VGA cards behind a misbehaving HiSilicon bridge

Commit Message

Gabriele Paoloni July 14, 2017, 5:03 p.m. UTC

Hi Alex, Ben

> -----Original Message-----

> From: Benjamin Herrenschmidt [mailto:[e-mail]]

> Sent: 14 July 2017 14:50

> To: Gabriele Paoloni; Alex Williamson; Bjorn Helgaas

> Cc: Daniel Axtens; [e-mail]; Liuxinliang (Matthew

> Liu); Rongrong Zou; Catalin Marinas; Will Deacon; linux-arm-

> [e-mail]; David Airlie; Daniel Vetter

> Subject: Re: [PATCH v4] PCI: Support hibmc VGA cards behind a

> misbehaving HiSilicon bridge

> 

> On Fri, 2017-07-14 at 12:26 +0000, Gabriele Paoloni wrote:

> > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c

> > index 92f1452..ab3ad9a 100644

> > --- a/drivers/gpu/vga/vgaarb.c

> > +++ b/drivers/gpu/vga/vgaarb.c

> > @@ -1424,6 +1424,14 @@ static int __init vga_arb_device_init(void)

> >

> >         list_for_each_entry(vgadev, &vga_list, list) {

> >                 struct device *dev = &vgadev->pdev->dev;

> > +

> > +               /* if no legacy device has been set as default VGA

> > +                * device, just pick up the first one in the list */

> > +               if (vga_default == NULL) {

> > +                       vgaarb_info(dev, "setting as boot VGA

> device\n");

> > +                       vga_set_default_device(vgadev->pdev);

> > +               }

> > +

> >  #if defined(CONFIG_X86) || defined(CONFIG_IA64)

> >                 /*

> >                  * Override vga_arbiter_add_pci_device()'s I/O based

> detection

> >

> > Above after we have filled the list of VGA devices by iterating over

> all

> > PCI devices we check if no legacy one has been set as default VGA

> device

> > yet: therefore we set the first VGA device in the list as default

> one...

> >

> > Do you think it would work?

> 

> I honestly don't remember all of the details of the arbiter but just

> make sure that it won't think that device is enabled for things like

> VGA etc... if it's memory/IO decode weren't enabled.

> 


I think the following one should make sure that MEM/IO response are
enabled (also as suggested by Alex)



Also the above one could allow us to get rid of fixup_vga in powerpc....


> I'd rather we have no default device until a driver actually picks up

> though, and then, if we still have no default, use the first driver to

> pick up.


Well from my understanding the PCI host controller driver will enumerate
all the devices in the PCI hierarchy and call pci_device_add() for each of
them, that in turn will call device_add(), at this stage if there is a
driver available for the device such driver will probe otherwise it will not.

Are you suggesting to add the code above in pci_device_add() after device_add()
and after checking that a driver has been bound for such dev?

Thanks
Gab


> 

> Ben.

Comments

Benjamin Herrenschmidt July 14, 2017, 11:54 p.m. UTC | #1

On Fri, 2017-07-14 at 17:03 +0000, Gabriele Paoloni wrote:
> > I'd rather we have no default device until a driver actually picks up
> > though, and then, if we still have no default, use the first driver to
> > pick up.
> 
> Well from my understanding the PCI host controller driver will enumerate
> all the devices in the PCI hierarchy and call pci_device_add() for each of
> them, that in turn will call device_add(), at this stage if there is a
> driver available for the device such driver will probe otherwise it will not.
> 
> Are you suggesting to add the code above in pci_device_add() after device_add()
> and after checking that a driver has been bound for such dev?

I don't like us turning on MEM/IO decoding on a device that has
potentially not been initialized by its driver.

Ben.

Patch

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 92f1452..a90c48c 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -1424,6 +1424,21 @@  static int __init vga_arb_device_init(void)
 
 	list_for_each_entry(vgadev, &vga_list, list) {
 		struct device *dev = &vgadev->pdev->dev;
+
+		/* if no legacy device has been set as default VGA device,
+		 * justpick up the first one in the list capable of responding to
+		 * mem and io requests*/
+		if (vga_default == NULL) {
+			u16 cmd;
+
+			pci_read_config_word(vgadev->pdev, PCI_COMMAND, &cmd);
+			if ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) ||
+					!vga_default_device()) {
+				vga_set_default_device(vgadev->pdev);
+				vgaarb_info(dev, "setting as boot VGA device\n");
+			}
+		}
+
 #if defined(CONFIG_X86) || defined(CONFIG_IA64)
 		/*
 		 * Override vga_arbiter_add_pci_device()'s I/O based detection