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@xxxxxxxxx> > [MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina] > Tested-by: William Brown <william@xxxxxxxxxxxxxxxx> > [MBP 8,2 2011 intel SNB + amd turks pre-retina] > Tested-by: Lukas Wunner <lukas@xxxxxxxxx> > [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina] > Tested-by: Bruno Bierbaumer <bruno@xxxxxxxxxxxxxx> > [MBP 11,3 2013 intel HSW + nvidia GK107 retina -- work in progress] > > Signed-off-by: Lukas Wunner <lukas@xxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
