[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