[PATCH v2 06/22] vga_switcheroo: Lock/unlock DDC lines harder
Lukas Wunner
lukas at wunner.de
Wed Aug 12 04:39:52 PDT 2015
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 at gmail.com>
[MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
Tested-by: William Brown <william at blackhats.net.au>
[MBP 8,2 2011 intel SNB + amd turks pre-retina]
Tested-by: Lukas Wunner <lukas at wunner.de>
[MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
Tested-by: Bruno Bierbaumer <bruno at bierbaumer.net>
[MBP 11,3 2013 intel HSW + nvidia GK107 retina -- work in progress]
Signed-off-by: Lukas Wunner <lukas at 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)
More information about the dri-devel
mailing list