[PATCH] drm: Rip out totally bogus vga_switcheroo->can_switch locking

[Thierry Reding]

On Tue, Apr 22, 2014 at 10:45:19PM +0200, Daniel Vetter wrote:
> So I just wanted to add a new field to struct drm_device and
> accidentally stumbled over something. According to comments
> dev->open_count is protected by dev->count_lock, but that's totally
> not the case. It's protected by drm_global_mutex.
> 
> Unfortunately the vga switcheroo callbacks took this comment at face
> value. The problem is that we can't just take the drm_global_mutex
> because:
> - It would lead to a locking inversion with the driver load/unload
>   paths.
> - It wouldn't actually protect anything, for that we'd need to wrap
>   the entire vga switcheroo code in the drm_global_mutex. And I'm not
>   sure whether that would actually solve anything.
> 
> What we probably want is a try_to_grab_switcheroo reference kind of
> thing which is used in the driver's ->open callback. Then we could
> move all that ->can_switch madness into the vga switcheroo core where
> it really belongs.
> 
> But since that would amount to real work take the easy way out and
> just add a comment. It's definitely not going to make anything worse
> since doing switcheroo state changes while restarting X just isn't
> recommended. Even though the delayed switching code does exactly that.
> 
> v2:
> - Simplify the ->can_switch implementations more (Thierry)
> - Fix comment about the dev->open_count locking (Thierry)
> 
> Cc: Thierry Reding 
> Reviewed-by: Laurent Pinchart  (v1)
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/i915/i915_dma.c| 11 ++-
>  drivers/gpu/drm/nouveau/nouveau_vga.c  | 11 ++-
>  drivers/gpu/drm/radeon/radeon_device.c | 11 ++-
>  include/drm/drmP.h |  2 +-
>  4 files changed, 19 insertions(+), 16 deletions(-)

Looks good:

Reviewed-by: Thierry Reding 
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

[PATCH] drm: Rip out totally bogus vga_switcheroo->can_switch locking

[Daniel Vetter]

So I just wanted to add a new field to struct drm_device and
accidentally stumbled over something. According to comments
dev->open_count is protected by dev->count_lock, but that's totally
not the case. It's protected by drm_global_mutex.

Unfortunately the vga switcheroo callbacks took this comment at face
value. The problem is that we can't just take the drm_global_mutex
because:
- It would lead to a locking inversion with the driver load/unload
  paths.
- It wouldn't actually protect anything, for that we'd need to wrap
  the entire vga switcheroo code in the drm_global_mutex. And I'm not
  sure whether that would actually solve anything.

What we probably want is a try_to_grab_switcheroo reference kind of
thing which is used in the driver's ->open callback. Then we could
move all that ->can_switch madness into the vga switcheroo core where
it really belongs.

But since that would amount to real work take the easy way out and
just add a comment. It's definitely not going to make anything worse
since doing switcheroo state changes while restarting X just isn't
recommended. Even though the delayed switching code does exactly that.

v2:
- Simplify the ->can_switch implementations more (Thierry)
- Fix comment about the dev->open_count locking (Thierry)

Cc: Thierry Reding 
Reviewed-by: Laurent Pinchart  (v1)
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/i915/i915_dma.c| 11 ++-
 drivers/gpu/drm/nouveau/nouveau_vga.c  | 11 ++-
 drivers/gpu/drm/radeon/radeon_device.c | 11 ++-
 include/drm/drmP.h |  2 +-
 4 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 96177eec0a0e..283ff06001bc 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1277,12 +1277,13 @@ static void i915_switcheroo_set_state(struct pci_dev 
*pdev, enum vga_switcheroo_
 static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
 {
struct drm_device *dev = pci_get_drvdata(pdev);
-   bool can_switch;

-   spin_lock(>count_lock);
-   can_switch = (dev->open_count == 0);
-   spin_unlock(>count_lock);
-   return can_switch;
+   /*
+* FIXME: open_count is protected by drm_global_mutex but that would 
lead to
+* locking inversion with the driver load path. And the access here is
+* completely racy anyway. So don't bother with locking for now.
+*/
+   return dev->open_count == 0;
 }

 static const struct vga_switcheroo_client_ops i915_switcheroo_ops = {
diff --git a/drivers/gpu/drm/nouveau/nouveau_vga.c 
b/drivers/gpu/drm/nouveau/nouveau_vga.c
index fb84da3cb50d..4f4c3fec6916 100644
--- a/drivers/gpu/drm/nouveau/nouveau_vga.c
+++ b/drivers/gpu/drm/nouveau/nouveau_vga.c
@@ -64,12 +64,13 @@ static bool
 nouveau_switcheroo_can_switch(struct pci_dev *pdev)
 {
struct drm_device *dev = pci_get_drvdata(pdev);
-   bool can_switch;

-   spin_lock(>count_lock);
-   can_switch = (dev->open_count == 0);
-   spin_unlock(>count_lock);
-   return can_switch;
+   /*
+* FIXME: open_count is protected by drm_global_mutex but that would 
lead to
+* locking inversion with the driver load path. And the access here is
+* completely racy anyway. So don't bother with locking for now.
+*/
+   return dev->open_count == 0;
 }

 static const struct vga_switcheroo_client_ops
diff --git a/drivers/gpu/drm/radeon/radeon_device.c 
b/drivers/gpu/drm/radeon/radeon_device.c
index 511fe26198e4..9aa1afd1786e 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1125,12 +1125,13 @@ static void radeon_switcheroo_set_state(struct pci_dev 
*pdev, enum vga_switchero
 static bool radeon_switcheroo_can_switch(struct pci_dev *pdev)
 {
struct drm_device *dev = pci_get_drvdata(pdev);
-   bool can_switch;

-   spin_lock(>count_lock);
-   can_switch = (dev->open_count == 0);
-   spin_unlock(>count_lock);
-   return can_switch;
+   /*
+* FIXME: open_count is protected by drm_global_mutex but that would 
lead to
+* locking inversion with the driver load path. And the access here is
+* completely racy anyway. So don't bother with locking for now.
+*/
+   return dev->open_count == 0;
 }

 static const struct vga_switcheroo_client_ops radeon_switcheroo_ops = {
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 9f1fb8d36b67..a20d882ca265 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1076,7 +1076,7 @@ struct drm_device {

/** \name Usage Counters */
/*@{ */
-   int open_count; /**< Outstanding files open */
+   int open_count; /**< Outstanding files open, protected 
by drm_global_mutex. */
int buf_use;/**< Buffers in use -- cannot alloc */
atomic_t buf_alloc;