drm/i915: Don't del_timer_sync uninitialized timer

PATCH In review Ben Widawsky 5 participants 11 messages Mar 15, 2014 – Mar 17, 2014

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Ben Widawsky

author

2014-03-15 09:44 UTC #0 lore raw

PATCH 1/1 drm/i915: Don't del_timer_sync uninitialized timer

Broken by:
commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd

Author: Chris Wilson <[email protected]>
Date:   Thu Mar 13 12:00:29 2014 +0000

drm/i915: Consolidate forcewake resetting to a single function

Cc Chris Wilson <[email protected]>

Cc Mika Kuoppala <[email protected]>

Signed-off-by Ben Widawsky <[email protected]>

---
 drivers/gpu/drm/i915/intel_uncore.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_uncore.c +3 -3

index e6bb421..7e55ceb 100644
@@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	setup_timer(&dev_priv->uncore.force_wake_timer,
+		    gen6_force_wake_timer, (unsigned long)dev_priv);
+
 	if (HAS_FPGA_DBG_UNCLAIMED(dev))
 		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);

@@ -724,9 +727,6 @@ void intel_uncore_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	setup_timer(&dev_priv->uncore.force_wake_timer,
-		    gen6_force_wake_timer, (unsigned long)dev_priv);
-
 	if (IS_VALLEYVIEW(dev)) {
 		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
 		dev_priv->uncore.funcs.force_wake_put = __vlv_force_wake_put;

Signature · 1 line

1.9.0

Chris Wilson

Reviewed-by on v1 1/1

2014-03-15 08:22 UTC #1 lore raw

Quoted 6 lines · Ben Widawsky

Broken by:
commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
Author: Chris Wilson <[email protected]>
Date:   Thu Mar 13 12:00:29 2014 +0000
drm/i915: Consolidate forcewake resetting to a single function

as it is now called by intel_uncore_forcewake_reset() much earlier in the initialisation sequence.

Quoted 3 lines

Cc Chris Wilson <[email protected]>
Cc Mika Kuoppala <[email protected]>
Signed-off-by Ben Widawsky <[email protected]>

Thanks,

Reviewed-by Chris Wilson <[email protected]>

-Chris

Signature · 1 line

Chris Wilson, Intel Open Source Technology Centre

Daniel Vetter

2014-03-15 11:47 UTC #2 lore raw

In reply to Ben Widawsky

Quoted 19 lines · Ben Widawsky

Broken by:
commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
Author: Chris Wilson <[email protected]>
Date:   Thu Mar 13 12:00:29 2014 +0000
drm/i915: Consolidate forcewake resetting to a single function
Cc Chris Wilson <[email protected]>
Cc Mika Kuoppala <[email protected]>
Signed-off-by Ben Widawsky <[email protected]>
---
 drivers/gpu/drm/i915/intel_uncore.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_uncore.c +0 -0
index e6bb421..7e55ceb 100644

drivers/gpu/drm/i915/intel_uncore.c void intel_uncore_early_sanitize(struct drm_device *dev) · line 366

Quoted 5 lines

drivers/gpu/drm/i915/intel_uncore.c +2 -0

@@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	setup_timer(&dev_priv->uncore.force_wake_timer,
+		    gen6_force_wake_timer, (unsigned long)dev_priv);

We call early_sanitize also from our resume code, so this will now re-setup the timer again. We generally don't do that since if we ever leak the timer to here in an enabled state it causes havoc.
-Daniel

Quoted 21 lines

drivers/gpu/drm/i915/intel_uncore.c +1 -3

+
 	if (HAS_FPGA_DBG_UNCLAIMED(dev))
 		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);

@@ -724,9 +727,6 @@ void intel_uncore_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	setup_timer(&dev_priv->uncore.force_wake_timer,
-		    gen6_force_wake_timer, (unsigned long)dev_priv);
-
 	if (IS_VALLEYVIEW(dev)) {
 		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
 		dev_priv->uncore.funcs.force_wake_put = __vlv_force_wake_put;

Signature · 6 lines

1.9.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Chris Wilson

2014-03-15 15:20 UTC #3 lore raw

Quoted 23 lines · Daniel Vetter

Quoted 22 lines · Ben Widawsky
Broken by:
commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
Author: Chris Wilson <[email protected]>
Date:   Thu Mar 13 12:00:29 2014 +0000
drm/i915: Consolidate forcewake resetting to a single function
Cc Chris Wilson <[email protected]>
Cc Mika Kuoppala <[email protected]>
Signed-off-by Ben Widawsky <[email protected]>
---
 drivers/gpu/drm/i915/intel_uncore.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_uncore.c +0 -0
index e6bb421..7e55ceb 100644
@@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 

Quoted 6 lines

drivers/gpu/drm/i915/intel_uncore.c void intel_uncore_early_sanitize(struct drm_device *dev) · line 366
Quoted 2 lines
drivers/gpu/drm/i915/intel_uncore.c +2 -0
+	setup_timer(&dev_priv->uncore.force_wake_timer,
+		    gen6_force_wake_timer, (unsigned long)dev_priv);
We call early_sanitize also from our resume code, so this will now re-setup the timer again. We generally don't do that since if we ever leak the timer to here in an enabled state it causes havoc.

Gah, really? intel_uncore_early_init()! There must be a clean way to break this up.
-Chris

Signature · 1 line

Chris Wilson, Intel Open Source Technology Centre

Ben Widawsky

2014-03-15 18:30 UTC #4 lore raw

Quoted 31 lines · Chris Wilson

Quoted 23 lines · Daniel Vetter
Quoted 22 lines · Ben Widawsky
Broken by:
commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
Author: Chris Wilson <[email protected]>
Date:   Thu Mar 13 12:00:29 2014 +0000
drm/i915: Consolidate forcewake resetting to a single function
Cc Chris Wilson <[email protected]>
Cc Mika Kuoppala <[email protected]>
Signed-off-by Ben Widawsky <[email protected]>
---
 drivers/gpu/drm/i915/intel_uncore.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_uncore.c +0 -0
index e6bb421..7e55ceb 100644
@@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
Quoted 6 lines
drivers/gpu/drm/i915/intel_uncore.c void intel_uncore_early_sanitize(struct drm_device *dev) · line 366
Quoted 2 lines
drivers/gpu/drm/i915/intel_uncore.c +2 -0
+	setup_timer(&dev_priv->uncore.force_wake_timer,
+		    gen6_force_wake_timer, (unsigned long)dev_priv);
We call early_sanitize also from our resume code, so this will now re-setup the timer again. We generally don't do that since if we ever leak the timer to here in an enabled state it causes havoc.

Quoted 3 lines

Gah, really? intel_uncore_early_init()! There must be a clean way to break this up.
-Chris

At least in the code base I was looking at, we currently do this also, so I didn't think this was any worse.

With lockdep turned on, the module will not even load, so please either revert the original, or merge this.

Signature · 1 line

Ben Widawsky, Intel Open Source Technology Center

Daniel Vetter

2014-03-15 19:13 UTC #5 lore raw

Quoted 7 lines · Ben Widawsky

Quoted 7 lines
Quoted 6 lines
Quoted 2 lines
+ setup_timer(&dev_priv->uncore.force_wake_timer,
+ gen6_force_wake_timer, (unsigned long)dev_priv);
We call early_sanitize also from our resume code, so this will now re-setup the timer again. We generally don't do that since if we ever leak the timer to here in an enabled state it causes havoc.

Quoted 6 lines

Quoted 3 lines
Gah, really? intel_uncore_early_init()! There must be a clean way to break this up.
-Chris
At least in the code base I was looking at, we currently do this also, so I didn't think this was any worse.

Hm, where? Imo such a thing would be a bug.

Quoted 2 lines

With lockdep turned on, the module will not even load, so please either revert the original, or merge this.

I think we can just move the call to early_sanitize into uncore_init, right after the timer setup.
-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Chris Wilson

2014-03-17 13:17 UTC #6 lore raw

In reply to Ben Widawsky

Quoted 36 lines · Ben Widawsky

Quoted 31 lines · Chris Wilson
Quoted 23 lines · Daniel Vetter
Quoted 22 lines · Ben Widawsky
Broken by:
commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
Author: Chris Wilson <[email protected]>
Date:   Thu Mar 13 12:00:29 2014 +0000
drm/i915: Consolidate forcewake resetting to a single function
Cc Chris Wilson <[email protected]>
Cc Mika Kuoppala <[email protected]>
Signed-off-by Ben Widawsky <[email protected]>
---
 drivers/gpu/drm/i915/intel_uncore.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_uncore.c +0 -0
index e6bb421..7e55ceb 100644
@@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
Quoted 6 lines
drivers/gpu/drm/i915/intel_uncore.c void intel_uncore_early_sanitize(struct drm_device *dev) · line 366
Quoted 2 lines
drivers/gpu/drm/i915/intel_uncore.c +2 -0
+	setup_timer(&dev_priv->uncore.force_wake_timer,
+		    gen6_force_wake_timer, (unsigned long)dev_priv);
We call early_sanitize also from our resume code, so this will now re-setup the timer again. We generally don't do that since if we ever leak the timer to here in an enabled state it causes havoc.
Quoted 3 lines
Gah, really? intel_uncore_early_init()! There must be a clean way to break this up.
-Chris

Quoted 5 lines

At least in the code base I was looking at, we currently do this also, so I didn't think this was any worse.
With lockdep turned on, the module will not even load, so please either revert the original, or merge this.

I think we can just:

drivers/gpu/drm/i915/intel_uncore.c +3 -2

index a62ff21..c9fcc20 100644
@@ -317,9 +317,10 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 {
        struct drm_i915_private *dev_priv = dev->dev_private;
        unsigned long irqflags;
-       int pending;
+       int pending = 0;
 
-       pending = del_timer_sync(&dev_priv->uncore.force_wake_timer);
+       if (dev_priv->uncore.force_wake_timer.function)
+               pending = del_timer_sync(&dev_priv->uncore.force_wake_timer);
 
        /* Hold uncore.lock across reset to prevent any register access
         * with forcewake not set correctly

i.e.

- del_timer_sync(&dev_priv->uncore.force_wake_timer);
+ if (dev_priv->uncore.force_wake_timer.function)
+ del_timer_sync(&dev_priv->uncore.force_wake_timer);

-Chris

Signature · 1 line

Chris Wilson, Intel Open Source Technology Centre

Daniel Vetter

2014-03-17 13:52 UTC #7 lore raw

Quoted 65 lines · Chris Wilson

Quoted 36 lines · Ben Widawsky
Quoted 31 lines · Chris Wilson
Quoted 23 lines · Daniel Vetter
Quoted 22 lines · Ben Widawsky
Broken by:
commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
Author: Chris Wilson <[email protected]>
Date:   Thu Mar 13 12:00:29 2014 +0000
drm/i915: Consolidate forcewake resetting to a single function
Cc Chris Wilson <[email protected]>
Cc Mika Kuoppala <[email protected]>
Signed-off-by Ben Widawsky <[email protected]>
---
 drivers/gpu/drm/i915/intel_uncore.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_uncore.c +0 -0
index e6bb421..7e55ceb 100644
@@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
Quoted 6 lines
drivers/gpu/drm/i915/intel_uncore.c void intel_uncore_early_sanitize(struct drm_device *dev) · line 366
Quoted 2 lines
drivers/gpu/drm/i915/intel_uncore.c +2 -0
+	setup_timer(&dev_priv->uncore.force_wake_timer,
+		    gen6_force_wake_timer, (unsigned long)dev_priv);
We call early_sanitize also from our resume code, so this will now re-setup the timer again. We generally don't do that since if we ever leak the timer to here in an enabled state it causes havoc.
Quoted 3 lines
Gah, really? intel_uncore_early_init()! There must be a clean way to break this up.
-Chris
Quoted 5 lines
At least in the code base I was looking at, we currently do this also, so I didn't think this was any worse.
With lockdep turned on, the module will not even load, so please either revert the original, or merge this.
I think we can just:
drivers/gpu/drm/i915/intel_uncore.c +3 -2
index a62ff21..c9fcc20 100644
@@ -317,9 +317,10 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 {
        struct drm_i915_private *dev_priv = dev->dev_private;
        unsigned long irqflags;
-       int pending;
+       int pending = 0;
 
-       pending = del_timer_sync(&dev_priv->uncore.force_wake_timer);
+       if (dev_priv->uncore.force_wake_timer.function)
+               pending = del_timer_sync(&dev_priv->uncore.force_wake_timer);
 
        /* Hold uncore.lock across reset to prevent any register access
         * with forcewake not set correctly
i.e.

drivers/gpu/drm/i915/intel_uncore.c

Quoted 3 lines

drivers/gpu/drm/i915/intel_uncore.c +2 -1

-       del_timer_sync(&dev_priv->uncore.force_wake_timer);
+       if (dev_priv->uncore.force_wake_timer.function)
+               del_timer_sync(&dev_priv->uncore.force_wake_timer);

I've thought of this:

drivers/gpu/drm/i915/i915_dma.c +0 -2

index e4d2b9f15ae2..9faee49f210d 100644
@@ -1608,8 +1608,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		goto put_bridge;
 	}
 
-	intel_uncore_early_sanitize(dev);
-
 	/* This must be called before any calls to HAS_PCH_* */
 	intel_detect_pch(dev);

drivers/gpu/drm/i915/intel_uncore.c +2 -0

index e2e328d86aff..c3832d9270a6 100644
@@ -736,6 +736,8 @@ void intel_uncore_init(struct drm_device *dev)
 	setup_timer(&dev_priv->uncore.force_wake_timer,
 		    gen6_force_wake_timer, (unsigned long)dev_priv);
 
+	intel_uncore_early_sanitize(dev);
+
 	if (IS_VALLEYVIEW(dev)) {
 		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
 		dev_priv->uncore.funcs.force_wake_put = __vlv_force_wake_put;

-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Jani Nikula

maintainer

2014-03-17 07:57 UTC #8 lore raw

In reply to Ben Widawsky

Quoted 7 lines · Ben Widawsky

Broken by:
commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
Author: Chris Wilson <[email protected]>
Date:   Thu Mar 13 12:00:29 2014 +0000
drm/i915: Consolidate forcewake resetting to a single function

Does this result in https://bugs.freedesktop.org/show_bug.cgi?id=76242 ?

Backtrace in the commit message would help a lot in pattern matching...

BR,
Jani.

Quoted 38 lines

Cc Chris Wilson <[email protected]>

Cc Mika Kuoppala <[email protected]>

Signed-off-by Ben Widawsky <[email protected]>

---
 drivers/gpu/drm/i915/intel_uncore.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_uncore.c +3 -3

index e6bb421..7e55ceb 100644
@@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	setup_timer(&dev_priv->uncore.force_wake_timer,
+		    gen6_force_wake_timer, (unsigned long)dev_priv);
+
 	if (HAS_FPGA_DBG_UNCLAIMED(dev))
 		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);

@@ -724,9 +727,6 @@ void intel_uncore_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	setup_timer(&dev_priv->uncore.force_wake_timer,
-		    gen6_force_wake_timer, (unsigned long)dev_priv);
-
 	if (IS_VALLEYVIEW(dev)) {
 		dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
 		dev_priv->uncore.funcs.force_wake_put = __vlv_force_wake_put;

Signature · 6 lines

1.9.0

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

Jani Nikula

maintainer

2014-03-17 09:14 UTC #9 lore raw

Quoted 10 lines · Jani Nikula

Quoted 7 lines · Ben Widawsky
Broken by:
commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
Author: Chris Wilson <[email protected]>
Date:   Thu Mar 13 12:00:29 2014 +0000
drm/i915: Consolidate forcewake resetting to a single function
Does this result in https://bugs.freedesktop.org/show_bug.cgi?id=76242 ?

QA confirms this patch fixes the bug.

Jani.

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

Daniel Vetter

2014-03-17 09:52 UTC #10 lore raw

Quoted 7 lines · Jani Nikula

Quoted 6 lines · Jani Nikula
Quoted 5 lines · Ben Widawsky
Broken by:
commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
Author: Chris Wilson <[email protected]>
Date:   Thu Mar 13 12:00:29 2014 +0000

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
drm/i915: Consolidate forcewake resetting to a single function
Does this result in https://bugs.freedesktop.org/show_bug.cgi?id=76242 ?
QA confirms this patch fixes the bug.

Also the commit message seriously lacks details. Apparantly this only blows up with lockdep (since most my machines here are still happy), which is a rather crucial detail.
-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch