[Intel-gfx] [PATCH] vgaarb: call pci_disable|enable_device on decoding vga devices

[Daniel Vetter]

There's the neat little problem that some systems die if vga decoding
isn't decoded anywhere, because linux disabled that pci device.

Atm we solve that problem by simple not calling pci_disable_device at
driver unregister time for drm pci devices. Which isn't to great
because it leaks a pci_enable_device reference on module reload and
also is rather inconsistent, since the driver load codcode in
drm/drm_pci.c _does_ call pci_disable_device on the load error path.

To fix this issue, let the vga arbiter hold a pci_enable_device
reference on its own when the vga device decodes anything. This leaves
the issue still open when the vga arbiter isn't loaded/compiled in,
but I guess since drm module unload is riddled with dragons it's ok to
say "just don't do this".

v2: ret needs to be signed, otherwise ERR_PTR will just store a large
positive value on 64bit machines. Noticed by Jani Nikula.

Signed-Off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
---
 drivers/gpu/vga/vgaarb.c |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 3df8fc0..c0b1271 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -49,7 +49,11 @@
 static void vga_arbiter_notify_clients(void);
 /*
  * We keep a list of all vga devices in the system to speed
- * up the various operations of the arbiter
+ * up the various operations of the arbiter. Note that vgaarb keeps a
+ * pci_enable_device reference for all vga devices with owns != 0. This is to
+ * avoid drm devices from killing the system when they call pci_disable_device
+ * on module unload (as they should), because some systems don't like it if no
+ * one decodes vga.
  */
 struct vga_device {
 	struct list_head list;
@@ -172,6 +176,7 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
 	unsigned int wants, legacy_wants, match;
 	struct vga_device *conflict;
 	unsigned int pci_bits;
+	int ret;
 	u32 flags = 0;
 
 	/* Account for "normal" resources to lock. If we decode the legacy,
@@ -264,6 +269,10 @@ static struct vga_device *__vga_tryget(struct vga_device *vgadev,
 
 		pci_set_vga_state(conflict->pdev, false, pci_bits, flags);
 		conflict->owns &= ~lwants;
+
+		if (conflict->owns == 0)
+			pci_disable_device(conflict->pdev);
+
 		/* If he also owned non-legacy, that is no longer the case */
 		if (lwants & VGA_RSRC_LEGACY_MEM)
 			conflict->owns &= ~VGA_RSRC_NORMAL_MEM;
@@ -290,6 +299,12 @@ enable_them:
 	if (!!(wants & VGA_RSRC_LEGACY_MASK))
 		flags |= PCI_VGA_STATE_CHANGE_BRIDGE;
 
+	if (vgadev->owns == 0) {
+		ret = pci_enable_device(vgadev->pdev);
+		if (ret)
+			return ERR_PTR(ret);
+	}
+
 	pci_set_vga_state(vgadev->pdev, true, pci_bits, flags);
 
 	if (!vgadev->bridge_has_one_vga) {
@@ -376,6 +391,8 @@ int vga_get(struct pci_dev *pdev, unsigned int rsrc, int interruptible)
 		if (conflict == NULL)
 			break;
 
+		if (IS_ERR(conflict))
+			return PTR_ERR(conflict);
 
 		/* We have a conflict, we wait until somebody kicks the
 		 * work queue. Currently we have one work queue that we
@@ -513,6 +530,7 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
 	unsigned long flags;
 	struct pci_bus *bus;
 	struct pci_dev *bridge;
+	int ret;
 	u16 cmd;
 
 	/* Only deal with VGA class devices */
@@ -582,6 +600,12 @@ static bool vga_arbiter_add_pci_device(struct pci_dev *pdev)
 
 	vga_arbiter_check_bridge_sharing(vgadev);
 
+	if (vgadev->owns != 0) {
+		ret = pci_enable_device(vgadev->pdev);
+		if (ret)
+			goto fail;
+	}
+
 	/* Add to the list */
 	list_add(&vgadev->list, &vga_list);
 	vga_count++;
-- 
1.7.7.6