diff mbox

[v2,06/22] vga_switcheroo: Lock/unlock DDC lines harder

Message ID ec399430bb382373e8b4edaa5f773563ebc6aaa9.1439288957.git.lukas@wunner.de (mailing list archive)
State New, archived
Headers show

Commit Message

Lukas Wunner March 27, 2015, 11:29 a.m. UTC 
Unlock DDC lines if drm_probe_ddc() fails.

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.

Use mutex_trylock(&vgasr_mutex) in vga_switcheroo_unlock_ddc() to
avoid locking inversion when one client locks vgasr_mutex on entering
vga_switcheroo_lock_ddc() and tries to acquire ddc_lock while another
client is holding ddc_lock and tries to acquire vgasr_mutex on entering
vga_switcheroo_unlock_ddc().

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.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
    [MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/drm_edid.c        |   9 ++--
 drivers/gpu/vga/vga_switcheroo.c  | 110 ++++++++++++++++++++++----------------
 drivers/platform/x86/apple-gmux.c |  15 +++++-
 include/linux/vga_switcheroo.h    |   3 +-
 4 files changed, 87 insertions(+), 50 deletions(-)

Comments

Daniel Vetter Aug. 12, 2015, 2:23 p.m. UTC | #1 
On Fri, Mar 27, 2015 at 12:29:40PM +0100, Lukas Wunner wrote:
> Unlock DDC lines if drm_probe_ddc() fails.
> 
> 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.
> 
> Use mutex_trylock(&vgasr_mutex) in vga_switcheroo_unlock_ddc() to
> avoid locking inversion when one client locks vgasr_mutex on entering
> vga_switcheroo_lock_ddc() and tries to acquire ddc_lock while another
> client is holding ddc_lock and tries to acquire vgasr_mutex on entering
> vga_switcheroo_unlock_ddc().
> 
> 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.

So essentially you have

bool locked = mutex_trylock();

/* nothing that looks at locked at all */

if (locked)
	mutex_unlock;

This doesn't protect anything at all and makes it look _very_ fishy.

I haven't dug deeper yet.
-Daniel

> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
> Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
>     [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
> Tested-by: William Brown <william@blackhats.net.au>
>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
> Tested-by: Lukas Wunner <lukas@wunner.de>
>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
> Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
>     [MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/gpu/drm/drm_edid.c        |   9 ++--
>  drivers/gpu/vga/vga_switcheroo.c  | 110 ++++++++++++++++++++++----------------
>  drivers/platform/x86/apple-gmux.c |  15 +++++-
>  include/linux/vga_switcheroo.h    |   3 +-
>  4 files changed, 87 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 505eed1..cdb2fa1 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1378,17 +1378,20 @@ struct edid *drm_get_edid(struct drm_connector *connector,
>  			  struct i2c_adapter *adapter)
>  {
>  	struct edid *edid;
> +	struct pci_dev *pdev = connector->dev->pdev;
>  
> -	vga_switcheroo_lock_ddc(connector->dev->pdev);
> +	vga_switcheroo_lock_ddc(pdev);
>  
> -	if (!drm_probe_ddc(adapter))
> +	if (!drm_probe_ddc(adapter)) {
> +		vga_switcheroo_unlock_ddc(pdev);
>  		return NULL;
> +	}
>  
>  	edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
>  	if (edid)
>  		drm_get_displayid(connector, edid);
>  
> -	vga_switcheroo_unlock_ddc(connector->dev->pdev);
> +	vga_switcheroo_unlock_ddc(pdev);
>  
>  	return edid;
>  }
> diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
> index 0223020..f02e7fc 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,33 +277,24 @@ 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;
> +	int ret, id;
>  
>  	mutex_lock(&vgasr_mutex);
>  
> -	if (!vgasr_priv.handler) {
> +	if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) {
>  		ret = -ENODEV;
>  		goto out;
>  	}
>  
> -	if (vgasr_priv.handler->switch_ddc) {
> -		mutex_lock(&vgasr_priv.ddc_lock);
> +	mutex_lock(&vgasr_priv.ddc_lock);
> +	id = vgasr_priv.handler->get_client_id(pdev);
> +	ret = vgasr_priv.handler->switch_ddc(id);
>  
> -		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) {
> +		pr_err("vga_switcheroo: failed to switch DDC lines\n");
> +		mutex_unlock(&vgasr_priv.ddc_lock);
> +	} else
> +		vgasr_priv.old_ddc_owner = ret;
>  
>  out:
>  	mutex_unlock(&vgasr_mutex);
> @@ -312,25 +304,33 @@ EXPORT_SYMBOL(vga_switcheroo_lock_ddc);
>  
>  int vga_switcheroo_unlock_ddc(struct pci_dev *pdev)
>  {
> -	int ret = 0;
> -	int id;
> -	mutex_lock(&vgasr_mutex);
> +	int ret, id;
> +	bool vgasr_mutex_acquired = mutex_trylock(&vgasr_mutex);
>  
> -	if (!vgasr_priv.handler) {
> -		ret = -ENODEV;
> +	if (!mutex_is_locked(&vgasr_priv.ddc_lock)) {
> +		ret = -EINVAL;
>  		goto out;
>  	}
>  
> -	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;
> +	if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) {
> +		pr_err("vga_switcheroo: handler disappeared\n");
>  		mutex_unlock(&vgasr_priv.ddc_lock);
> +		ret = -ENODEV;
> +		goto out;
>  	}
> +
> +	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)
> +			pr_err("vga_switcheroo: failed to switch DDC lines\n");
> +	} else
> +		ret = vgasr_priv.old_ddc_owner;
> +	mutex_unlock(&vgasr_priv.ddc_lock);
> +
>  out:
> -	mutex_unlock(&vgasr_mutex);
> +	if (vgasr_mutex_acquired)
> +		mutex_unlock(&vgasr_mutex);
>  	return ret;
>  }
>  EXPORT_SYMBOL(vga_switcheroo_unlock_ddc);
> @@ -433,14 +433,26 @@ 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) {
> +			pr_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) {
> +		pr_err("vga_switcheroo: failed to switch to client %d\n",
> +		       new_client->id);
> +		active->active = true;
> +		/* 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 +464,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 +565,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) {
> +				pr_err("vga_switcheroo: failed to switch DDC lines\n");
> +				goto out;
> +			}
> +		}
>  		ret = vgasr_priv.handler->switchto(client_id);
>  		goto out;
>  	}
> @@ -716,6 +729,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)
> +			pr_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 f0a55a4..08bdf1e 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -275,11 +275,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..39b25b0 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 -ENODEV; }
> +static inline int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) { return -ENODEV; }
>  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
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 505eed1..cdb2fa1 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1378,17 +1378,20 @@  struct edid *drm_get_edid(struct drm_connector *connector,
 			  struct i2c_adapter *adapter)
 {
 	struct edid *edid;
+	struct pci_dev *pdev = connector->dev->pdev;
 
-	vga_switcheroo_lock_ddc(connector->dev->pdev);
+	vga_switcheroo_lock_ddc(pdev);
 
-	if (!drm_probe_ddc(adapter))
+	if (!drm_probe_ddc(adapter)) {
+		vga_switcheroo_unlock_ddc(pdev);
 		return NULL;
+	}
 
 	edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
 	if (edid)
 		drm_get_displayid(connector, edid);
 
-	vga_switcheroo_unlock_ddc(connector->dev->pdev);
+	vga_switcheroo_unlock_ddc(pdev);
 
 	return edid;
 }
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 0223020..f02e7fc 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,33 +277,24 @@  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;
+	int ret, id;
 
 	mutex_lock(&vgasr_mutex);
 
-	if (!vgasr_priv.handler) {
+	if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) {
 		ret = -ENODEV;
 		goto out;
 	}
 
-	if (vgasr_priv.handler->switch_ddc) {
-		mutex_lock(&vgasr_priv.ddc_lock);
+	mutex_lock(&vgasr_priv.ddc_lock);
+	id = vgasr_priv.handler->get_client_id(pdev);
+	ret = vgasr_priv.handler->switch_ddc(id);
 
-		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) {
+		pr_err("vga_switcheroo: failed to switch DDC lines\n");
+		mutex_unlock(&vgasr_priv.ddc_lock);
+	} else
+		vgasr_priv.old_ddc_owner = ret;
 
 out:
 	mutex_unlock(&vgasr_mutex);
@@ -312,25 +304,33 @@  EXPORT_SYMBOL(vga_switcheroo_lock_ddc);
 
 int vga_switcheroo_unlock_ddc(struct pci_dev *pdev)
 {
-	int ret = 0;
-	int id;
-	mutex_lock(&vgasr_mutex);
+	int ret, id;
+	bool vgasr_mutex_acquired = mutex_trylock(&vgasr_mutex);
 
-	if (!vgasr_priv.handler) {
-		ret = -ENODEV;
+	if (!mutex_is_locked(&vgasr_priv.ddc_lock)) {
+		ret = -EINVAL;
 		goto out;
 	}
 
-	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;
+	if (!vgasr_priv.handler || !vgasr_priv.handler->switch_ddc) {
+		pr_err("vga_switcheroo: handler disappeared\n");
 		mutex_unlock(&vgasr_priv.ddc_lock);
+		ret = -ENODEV;
+		goto out;
 	}
+
+	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)
+			pr_err("vga_switcheroo: failed to switch DDC lines\n");
+	} else
+		ret = vgasr_priv.old_ddc_owner;
+	mutex_unlock(&vgasr_priv.ddc_lock);
+
 out:
-	mutex_unlock(&vgasr_mutex);
+	if (vgasr_mutex_acquired)
+		mutex_unlock(&vgasr_mutex);
 	return ret;
 }
 EXPORT_SYMBOL(vga_switcheroo_unlock_ddc);
@@ -433,14 +433,26 @@  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) {
+			pr_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) {
+		pr_err("vga_switcheroo: failed to switch to client %d\n",
+		       new_client->id);
+		active->active = true;
+		/* 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 +464,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 +565,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) {
+				pr_err("vga_switcheroo: failed to switch DDC lines\n");
+				goto out;
+			}
+		}
 		ret = vgasr_priv.handler->switchto(client_id);
 		goto out;
 	}
@@ -716,6 +729,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)
+			pr_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 f0a55a4..08bdf1e 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -275,11 +275,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..39b25b0 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 -ENODEV; }
+static inline int vga_switcheroo_unlock_ddc(struct pci_dev *pdev) { return -ENODEV; }
 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,