Skip to contentSkip to conversation

FAILED: patch "[PATCH] Revert "apple-gmux: lock iGP IO to protect from vgaarb" failed to apply to 4.9-stable tree

PATCH [email protected] 1 participant 1 message Feb 20, 2018

Subsystems (2)
Stable branch x86 platform drivers
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

The patch below does not apply to the 4.9-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

Quoted 1 line

From d6fa7588fd7a8def4c747c0c574ce85d453e3788 Mon Sep 17 00:00:00 2001

From: Lukas Wunner <[email protected]>
Date: Wed, 24 Jan 2018 19:35:45 +0100
Subject: [PATCH] Revert "apple-gmux: lock iGP IO to protect from vgaarb
changes"
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Commit 4eebd5a4e726 ("apple-gmux: lock iGP IO to protect from vgaarb changes") amended this driver's ->probe hook to lock decoding of normal (non-legacy) I/O space accesses to the integrated GPU on dual-GPU MacBook Pros. The lock stays in place until the driver is unbound.

The change was made to work around an issue with the out-of-tree nvidia graphics driver (available at http://www.nvidia.com/object/unix.html).
It contains the following sequence in nvidia/nv.c:

	#if defined(CONFIG_VGA_ARB) && !defined(NVCPU_PPC64LE)
	#if defined(VGA_DEFAULT_DEVICE)
	    vga_tryget(VGA_DEFAULT_DEVICE, VGA_RSRC_LEGACY_MASK);
	#endif
	    vga_set_legacy_decoding(dev, VGA_RSRC_NONE);
	#endif

This code was reported to cause deadlocks with VFIO already in 2013:
https://devtalk.nvidia.com/default/topic/545560

I've reported the issue to Nvidia developers once more in 2017:
https://www.spinics.net/lists/dri-devel/msg138754.html

On the MacBookPro10,1, this code apparently breaks backlight control (which is handled by apple-gmux via an I/O region starting at 0x700), as reported by Petri Hodju:
https://bugzilla.kernel.org/show_bug.cgi?id=86121

I tried to replicate Petri's observations on my MacBook9,1, which uses the same Intel Ivy Bridge + Nvidia GeForce GT 650M architecture, to no avail. On my machine apple-gmux' I/O region remains accessible even with the nvidia driver loaded and commit 4eebd5a4e726 reverted. Petri reported that apple-gmux becomes accessible again after a suspend/resume cycle because the BIOS changed the VGA routing on the root port to the Nvidia GPU. Perhaps this is a BIOS issue after all that can be fixed with an update?

In any case, the change made by commit 4eebd5a4e726 has turned out to cause two new issues:

* Wilfried Klaebe reports a deadlock when launching Xorg because it opens /dev/vga_arbiter and calls vga_get(), but apple-gmux is holding a lock on I/O space indefinitely. It looks like apple-gmux' current behavior is an abuse of the vgaarb API as locks are not meant to be held for longer periods:
https://bugzilla.kernel.org/show_bug.cgi?id=88861#c11
https://bugzilla.kernel.org/attachment.cgi?id=217541

* On dual GPU MacBook Pros introduced since 2013, the integrated GPU is powergated on boot und thus becomes invisible to Linux unless a custom EFI protocol is used to leave it powered on. (A patch exists but is not in mainline yet due to several negative side effects.) On these machines, locking I/O to the integrated GPU (as done by 4eebd5a4e726) fails and backlight control is therefore broken:
https://bugzilla.kernel.org/show_bug.cgi?id=105051

So let's revert commit 4eebd5a4e726 please. Users experiencing the issue with the proprietary nvidia driver can comment out the above- quoted problematic code as a workaround (or try updating the BIOS).

Cc Petri Hodju <[email protected]>
Cc Bjorn Helgaas <[email protected]>
Cc Bruno Prémont <[email protected]>
Cc Andy Ritger <[email protected]>
Cc Ronald Tschalär <[email protected]>
Tested-by Wilfried Klaebe <[email protected]>
Signed-off-by Lukas Wunner <[email protected]>
Cc [email protected]
Signed-off-by Darren Hart (VMware) <[email protected]>
drivers/platform/x86/apple-gmux.c +1 -47
index 623d322447a2..7c4eb86c851e 100644
@@ -24,7 +24,6 @@
 #include <linux/delay.h>
 #include <linux/pci.h>
 #include <linux/vga_switcheroo.h>
-#include <linux/vgaarb.h>
 #include <acpi/video.h>
 #include <asm/io.h>
 

@@ -54,7 +53,6 @@ struct apple_gmux_data {
 	bool indexed;
 	struct mutex index_lock;
 
-	struct pci_dev *pdev;
 	struct backlight_device *bdev;
 
 	/* switcheroo data */

@@ -599,23 +597,6 @@ static int gmux_resume(struct device *dev)
 	return 0;
 }
 
-static struct pci_dev *gmux_get_io_pdev(void)
-{
-	struct pci_dev *pdev = NULL;
-
-	while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev))) {
-		u16 cmd;
-
-		pci_read_config_word(pdev, PCI_COMMAND, &cmd);
-		if (!(cmd & PCI_COMMAND_IO))
-			continue;
-
-		return pdev;
-	}
-
-	return NULL;
-}
-
 static int is_thunderbolt(struct device *dev, void *data)
 {
 	return to_pci_dev(dev)->is_thunderbolt;

@@ -631,7 +612,6 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	int ret = -ENXIO;
 	acpi_status status;
 	unsigned long long gpe;
-	struct pci_dev *pdev = NULL;
 
 	if (apple_gmux_data)
 		return -EBUSY;

@@ -682,7 +662,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 			ver_minor = (version >> 16) & 0xff;
 			ver_release = (version >> 8) & 0xff;
 		} else {
-			pr_info("gmux device not present or IO disabled\n");
+			pr_info("gmux device not present\n");
 			ret = -ENODEV;
 			goto err_release;
 		}

@@ -690,23 +670,6 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	pr_info("Found gmux version %d.%d.%d [%s]\n", ver_major, ver_minor,
 		ver_release, (gmux_data->indexed ? "indexed" : "classic"));
 
-	/*
-	 * Apple systems with gmux are EFI based and normally don't use
-	 * VGA. In addition changing IO+MEM ownership between IGP and dGPU
-	 * disables IO/MEM used for backlight control on some systems.
-	 * Lock IO+MEM to GPU with active IO to prevent switch.
-	 */
-	pdev = gmux_get_io_pdev();
-	if (pdev && vga_tryget(pdev,
-			       VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM)) {
-		pr_err("IO+MEM vgaarb-locking for PCI:%s failed\n",
-			pci_name(pdev));
-		ret = -EBUSY;
-		goto err_release;
-	} else if (pdev)
-		pr_info("locked IO for PCI:%s\n", pci_name(pdev));
-	gmux_data->pdev = pdev;
-
 	memset(&props, 0, sizeof(props));
 	props.type = BACKLIGHT_PLATFORM;
 	props.max_brightness = gmux_read32(gmux_data, GMUX_PORT_MAX_BRIGHTNESS);

@@ -822,10 +785,6 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 err_notify:
 	backlight_device_unregister(bdev);
 err_release:
-	if (gmux_data->pdev)
-		vga_put(gmux_data->pdev,
-			VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM);
-	pci_dev_put(pdev);
 	release_region(gmux_data->iostart, gmux_data->iolen);
 err_free:
 	kfree(gmux_data);

@@ -845,11 +804,6 @@ static void gmux_remove(struct pnp_dev *pnp)
 					   &gmux_notify_handler);
 	}
 
-	if (gmux_data->pdev) {
-		vga_put(gmux_data->pdev,
-			VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM);
-		pci_dev_put(gmux_data->pdev);
-	}
 	backlight_device_unregister(gmux_data->bdev);
 
 	release_region(gmux_data->iostart, gmux_data->iolen);