[PATCH 06/11] vga_switcheroo: Lock/unlock DDC lines harder

[Lukas Wunner <lukas@wunner.de>]

Unlock DDC lines if drm_probe_ddc() fails.

intel_dp_detect_dpcd() calls drm_probe_ddc(), needs to lock DDC
lines.

Don't lock vgasr_mutex in vga_switcheroo_lock/unlock_ddc(), it's
already locked in vga_switcheroo_debugfs_write(), leading to a
deadlock upon reprobe in stage2 when the new client reads the EDID.
Locking ddc_lock is sufficient.

If the inactive client registers before the active client then
old_ddc_owner cannot be determined with find_active_client()
(null pointer dereference). Therefore change semantics of the
switch_ddc() handler callback to return old_ddc_owner on success
or a negative int on failure.

Lock ddc_lock in stage2 to avoid race condition where reading the
EDID and switching happens simultaneously.

Switch DDC lines on MIGD / MDIS commands and on runtime suspend.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/drm_edid.c        |  4 +-
 drivers/gpu/drm/i915/intel_dp.c   |  6 ++-
 drivers/gpu/vga/vga_switcheroo.c  | 82 ++++++++++++++++++++++-----------------
 drivers/platform/x86/apple-gmux.c | 15 ++++++-
 include/linux/vga_switcheroo.h    |  3 +-
 5 files changed, 71 insertions(+), 39 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index f91593b..8950722 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1329,8 +1329,10 @@ struct edid *drm_get_edid(struct drm_connector *connector,
 
 	vga_switcheroo_lock_ddc(connector->dev->pdev);
 
-	if (!drm_probe_ddc(adapter))
+	if (!drm_probe_ddc(adapter)) {
+		vga_switcheroo_unlock_ddc(connector->dev->pdev);
 		return NULL;
+	}
 
 	edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
 	if (edid)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d023710..e9c40b1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -30,6 +30,7 @@
 #include <linux/export.h>
 #include <linux/notifier.h>
 #include <linux/reboot.h>
+#include <linux/vga_switcheroo.h>
 #include <drm/drmP.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_crtc.h>
@@ -4079,6 +4080,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 {
 	uint8_t *dpcd = intel_dp->dpcd;
 	uint8_t type;
+	struct pci_dev *pdev = intel_dp->attached_connector->base.dev->pdev;
 
 	if (!intel_dp_get_dpcd(intel_dp))
 		return connector_status_disconnected;
@@ -4101,7 +4103,9 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	}
 
 	/* If no HPD, poke DDC gently */
-	if (drm_probe_ddc(&intel_dp->aux.ddc))
+	if (vga_switcheroo_lock_ddc(pdev) >= 0 &&
+	    drm_probe_ddc(&intel_dp->aux.ddc) &&
+	    vga_switcheroo_unlock_ddc(pdev) >= 0)
 		return connector_status_connected;
 
 	/* Well we tried, say unknown for unreliable port types */
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 0223020..2534d84 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -9,12 +9,13 @@
 
  Switcher interface - methods require for ATPX and DCM
  - switchto - this throws the output MUX switch
- - discrete_set_power - sets the power state for the discrete card
+ - switch_ddc - switch only DDC lines, return old DDC owner (or < 0 on failure)
+ - power_state - sets the power state for either GPU
 
  GPU driver interface
  - set_gpu_state - this should do the equiv of s/r for the card
 		  - this should *not* set the discrete power state
- - switch_check  - check if the device is in a position to switch now
+ - can_switch - check if the device is in a position to switch now
  */
 
 #include <linux/module.h>
@@ -59,7 +60,7 @@ struct vgasr_priv {
 	struct vga_switcheroo_handler *handler;
 
 	struct mutex ddc_lock;
-	struct pci_dev *old_ddc_owner;
+	enum vga_switcheroo_client_id old_ddc_owner;
 };
 
 #define ID_BIT_AUDIO		0x100
@@ -276,12 +277,9 @@ EXPORT_SYMBOL(vga_switcheroo_client_fb_set);
 
 int vga_switcheroo_lock_ddc(struct pci_dev *pdev)
 {
-	struct vga_switcheroo_client *client;
 	int ret = 0;
 	int id;
 
-	mutex_lock(&vgasr_mutex);
-
 	if (!vgasr_priv.handler) {
 		ret = -ENODEV;
 		goto out;
@@ -290,22 +288,17 @@ int vga_switcheroo_lock_ddc(struct pci_dev *pdev)
 	if (vgasr_priv.handler->switch_ddc) {
 		mutex_lock(&vgasr_priv.ddc_lock);
 
-		client = find_active_client(&vgasr_priv.clients);
-		if (!client) {
-			mutex_unlock(&vgasr_priv.ddc_lock);
-			ret = -ENODEV;
-			goto out;
-		}
-		vgasr_priv.old_ddc_owner = client->pdev;
-		if (client->pdev == pdev)
-			goto out;
-
 		id = vgasr_priv.handler->get_client_id(pdev);
 		ret = vgasr_priv.handler->switch_ddc(id);
+
+		if (ret < 0) {
+			mutex_unlock(&vgasr_priv.ddc_lock);
+			printk(KERN_ERR "vga_switcheroo: failed to switch DDC lines\n");
+		} else
+			vgasr_priv.old_ddc_owner = ret;
 	}
 
 out:
-	mutex_unlock(&vgasr_mutex);
 	return ret;
 }
 EXPORT_SYMBOL(vga_switcheroo_lock_ddc);
@@ -314,7 +307,6 @@ int vga_switcheroo_unlock_ddc(struct pci_dev *pdev)
 {
 	int ret = 0;
 	int id;
-	mutex_lock(&vgasr_mutex);
 
 	if (!vgasr_priv.handler) {
 		ret = -ENODEV;
@@ -322,15 +314,17 @@ int vga_switcheroo_unlock_ddc(struct pci_dev *pdev)
 	}
 
 	if (vgasr_priv.handler->switch_ddc) {
-		if (vgasr_priv.old_ddc_owner != pdev) {
-			id = vgasr_priv.handler->get_client_id(vgasr_priv.old_ddc_owner);
-			ret = vgasr_priv.handler->switch_ddc(id);
-		}
-		vgasr_priv.old_ddc_owner = NULL;
+		id = vgasr_priv.handler->get_client_id(pdev);
+
+		if (vgasr_priv.old_ddc_owner != id)
+			ret = vgasr_priv.handler->switch_ddc(vgasr_priv.old_ddc_owner);
+		if (ret < 0)
+			printk(KERN_ERR "vga_switcheroo: failed to switch DDC lines\n");
+
 		mutex_unlock(&vgasr_priv.ddc_lock);
 	}
+
 out:
-	mutex_unlock(&vgasr_mutex);
 	return ret;
 }
 EXPORT_SYMBOL(vga_switcheroo_unlock_ddc);
@@ -433,14 +427,24 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
 	}
 
 	if (vgasr_priv.handler->switch_ddc) {
+		mutex_lock(&vgasr_priv.ddc_lock);
 		ret = vgasr_priv.handler->switch_ddc(new_client->id);
-		if (ret)
+		mutex_unlock(&vgasr_priv.ddc_lock);
+		if (ret < 0) {
+			printk(KERN_ERR "vga_switcheroo: failed to switch DDC lines\n");
 			return ret;
+		}
 	}
 
 	ret = vgasr_priv.handler->switchto(new_client->id);
-	if (ret)
-		goto restore_ddc;
+	if (ret) {
+		printk(KERN_ERR "vga_switcheroo: failed to switch to client %d\n", new_client->id);
+		/* restore DDC lines */
+		mutex_lock(&vgasr_priv.ddc_lock);
+		vgasr_priv.handler->switch_ddc(active->id);
+		mutex_unlock(&vgasr_priv.ddc_lock);
+		return ret;
+	}
 
 	if (new_client->ops->reprobe)
 		new_client->ops->reprobe(new_client->pdev);
@@ -452,14 +456,6 @@ static int vga_switchto_stage2(struct vga_switcheroo_client *new_client)
 
 	new_client->active = true;
 	return 0;
-
-restore_ddc:
-	if (vgasr_priv.handler->switch_ddc) {
-		int id = (new_client->id == VGA_SWITCHEROO_IGD) ?
-				VGA_SWITCHEROO_DIS : VGA_SWITCHEROO_IGD;
-		ret = vgasr_priv.handler->switch_ddc(id);
-	}
-	return ret;
 }
 
 static bool check_can_switch(void)
@@ -561,6 +557,15 @@ vga_switcheroo_debugfs_write(struct file *filp, const char __user *ubuf,
 	vgasr_priv.delayed_switch_active = false;
 
 	if (just_mux) {
+		if (vgasr_priv.handler->switch_ddc) {
+			mutex_lock(&vgasr_priv.ddc_lock);
+			ret = vgasr_priv.handler->switch_ddc(client_id);
+			mutex_unlock(&vgasr_priv.ddc_lock);
+			if (ret < 0) {
+				printk(KERN_ERR "vga_switcheroo: failed to switch DDC lines\n");
+				goto out;
+			}
+		}
 		ret = vgasr_priv.handler->switchto(client_id);
 		goto out;
 	}
@@ -716,6 +721,13 @@ static int vga_switcheroo_runtime_suspend(struct device *dev)
 	ret = dev->bus->pm->runtime_suspend(dev);
 	if (ret)
 		return ret;
+	if (vgasr_priv.handler->switch_ddc) {
+		mutex_lock(&vgasr_priv.ddc_lock);
+		ret = vgasr_priv.handler->switch_ddc(VGA_SWITCHEROO_IGD);
+		mutex_unlock(&vgasr_priv.ddc_lock);
+		if (ret < 0)
+			printk(KERN_ERR "vga_switcheroo: failed to switch DDC lines\n");
+	}
 	if (vgasr_priv.handler->switchto)
 		vgasr_priv.handler->switchto(VGA_SWITCHEROO_IGD);
 	vga_switcheroo_power_switch(pdev, VGA_SWITCHEROO_OFF);
diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 1cf242c..9aafbde 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -273,11 +273,24 @@ static const struct backlight_ops gmux_bl_ops = {
 
 static int gmux_switch_ddc(enum vga_switcheroo_client_id id)
 {
+	enum vga_switcheroo_client_id old_ddc_owner;
+
+	if (gmux_read8(apple_gmux_data, GMUX_PORT_SWITCH_DDC) == 1)
+		old_ddc_owner = VGA_SWITCHEROO_IGD;
+	else
+		old_ddc_owner = VGA_SWITCHEROO_DIS;
+
+	pr_debug("Switching gmux DDC from %d to %d\n", old_ddc_owner, id);
+
+	if (id == old_ddc_owner)
+		return old_ddc_owner;
+
 	if (id == VGA_SWITCHEROO_IGD)
 		gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 1);
 	else
 		gmux_write8(apple_gmux_data, GMUX_PORT_SWITCH_DDC, 2);
-	return 0;
+
+	return old_ddc_owner;
 }
 
 static int gmux_switchto(enum vga_switcheroo_client_id id)
diff --git a/include/linux/vga_switcheroo.h b/include/linux/vga_switcheroo.h
index 352bef3..8963799 100644
--- a/include/linux/vga_switcheroo.h
+++ b/include/linux/vga_switcheroo.h
@@ -77,7 +77,8 @@ static inline void vga_switcheroo_unregister_client(struct pci_dev *dev) {}
 static inline int vga_switcheroo_register_client(struct pci_dev *dev,
 		const struct vga_switcheroo_client_ops *ops, bool driver_power_control) { return 0; }
 static inline void vga_switcheroo_client_fb_set(struct pci_dev *dev, struct fb_info *info) {}
-static inline void vga_switcheroo_switch_ddc(struct pci_dev *pdev) { return NULL; }
+static inline int vga_switcheroo_lock_ddc(struct pci_dev *pdev) { return 0; }
+static inline int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) { return 0; }
 static inline int vga_switcheroo_register_handler(struct vga_switcheroo_handler *handler) { return 0; }
 static inline int vga_switcheroo_register_audio_client(struct pci_dev *pdev,
 	const struct vga_switcheroo_client_ops *ops,
-- 
1.8.5.2 (Apple Git-48)

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel