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 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)
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
