PM / Runtime: allow _put_sync() from interrupts-disabled context

Currently the use of pm_runtime_put_sync() is not safe from interrupts-disabled context because rpm_idle() will release the spinlock and enable interrupts for the idle callbacks. This enables interrupts during a time where interrupts were expected to be disabled, and can have strange side effects on drivers that expected interrupts to be disabled.

This is not a bug since the documentation clearly states that only _put_sync_suspend() is safe in IRQ-safe mode.

However, pm_runtime_put_sync() could be made safe when in IRQ-safe mode by releasing the spinlock but not re-enabling interrupts, which is what this patch aims to do.

Problem was found when using some buggy drivers that set pm_runtime_irq_safe() and used _put_sync() in interrupts-disabled context.

The offending drivers have been fixed to use _put_sync_suspend(), But this patch is an RFC to see if it might make sense to allow using _put_sync() from interrupts-disabled context.

Reported-by Colin Cross <[email protected]>

Tested-by Nishanth Menon <[email protected]>

Signed-off-by Kevin Hilman <[email protected]>

---
v2: update documentation also

 Documentation/power/runtime_pm.txt |   10 +++++-----
 drivers/base/power/runtime.c       |   10 ++++++++--
 2 files changed, 13 insertions(+), 7 deletions(-)

Documentation/power/runtime_pm.txt +5 -5

index 14dd3c6..4ce5450 100644
@@ -54,11 +54,10 @@ referred to as subsystem-level callbacks in what follows.
 By default, the callbacks are always invoked in process context with interrupts
 enabled.  However, subsystems can use the pm_runtime_irq_safe() helper function
 to tell the PM core that a device's ->runtime_suspend() and ->runtime_resume()
-callbacks should be invoked in atomic context with interrupts disabled
-(->runtime_idle() is still invoked the default way).  This implies that these
-callback routines must not block or sleep, but it also means that the
-synchronous helper functions listed at the end of Section 4 can be used within
-an interrupt handler or in an atomic context.
+callbacks should be invoked in atomic context with interrupts disabled.
+This implies that these callback routines must not block or sleep, but it also
+means that the synchronous helper functions listed at the end of Section 4 can
+be used within an interrupt handler or in an atomic context.
 
 The subsystem-level suspend callback is _entirely_ _responsible_ for handling
 the suspend of the device as appropriate, which may, but need not include

@@ -483,6 +482,7 @@ pm_runtime_suspend()
 pm_runtime_autosuspend()
 pm_runtime_resume()
 pm_runtime_get_sync()
+pm_runtime_put_sync()
 pm_runtime_put_sync_suspend()
 
 5. Runtime PM Initialization, Device Probing and Removal

drivers/base/power/runtime.c +8 -2

index 8dc247c..acb3f83 100644
@@ -226,11 +226,17 @@ static int rpm_idle(struct device *dev, int rpmflags)
 		callback = NULL;
 
 	if (callback) {
-		spin_unlock_irq(&dev->power.lock);
+		if (dev->power.irq_safe)
+			spin_unlock(&dev->power.lock);
+		else
+			spin_unlock_irq(&dev->power.lock);
 
 		callback(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		if (dev->power.irq_safe)
+			spin_lock(&dev->power.lock);
+		else
+			spin_lock_irq(&dev->power.lock);
 	}
 
 	dev->power.idle_notification = false;

Quoted 18 lines · Kevin Hilman

Currently the use of pm_runtime_put_sync() is not safe from interrupts-disabled context because rpm_idle() will release the spinlock and enable interrupts for the idle callbacks. This enables interrupts during a time where interrupts were expected to be disabled, and can have strange side effects on drivers that expected interrupts to be disabled.

This is not a bug since the documentation clearly states that only _put_sync_suspend() is safe in IRQ-safe mode.

However, pm_runtime_put_sync() could be made safe when in IRQ-safe mode by releasing the spinlock but not re-enabling interrupts, which is what this patch aims to do.

Problem was found when using some buggy drivers that set pm_runtime_irq_safe() and used _put_sync() in interrupts-disabled context.

Quoted 3 lines

The offending drivers have been fixed to use _put_sync_suspend(), But this patch is an RFC to see if it might make sense to allow using _put_sync() from interrupts-disabled context.

OK, I'm going to take this for 3.2.

Quoted 23 lines · Rafael J. Wysocki

Quoted 21 lines · Kevin Hilman

Currently the use of pm_runtime_put_sync() is not safe from interrupts-disabled context because rpm_idle() will release the spinlock and enable interrupts for the idle callbacks. This enables interrupts during a time where interrupts were expected to be disabled, and can have strange side effects on drivers that expected interrupts to be disabled.

This is not a bug since the documentation clearly states that only _put_sync_suspend() is safe in IRQ-safe mode.

However, pm_runtime_put_sync() could be made safe when in IRQ-safe mode by releasing the spinlock but not re-enabling interrupts, which is what this patch aims to do.

Problem was found when using some buggy drivers that set pm_runtime_irq_safe() and used _put_sync() in interrupts-disabled context.

The offending drivers have been fixed to use _put_sync_suspend(), But this patch is an RFC to see if it might make sense to allow using _put_sync() from interrupts-disabled context.

Quoted 2 lines

OK, I'm going to take this for 3.2.

OK, great. Thanks.

Might want to just drop the last paragraph from the changelog since it doesn't really belong in the permanant history.

Quoted 23 lines · Rafael J. Wysocki

Quoted 21 lines · Kevin Hilman

Currently the use of pm_runtime_put_sync() is not safe from interrupts-disabled context because rpm_idle() will release the spinlock and enable interrupts for the idle callbacks. This enables interrupts during a time where interrupts were expected to be disabled, and can have strange side effects on drivers that expected interrupts to be disabled.

This is not a bug since the documentation clearly states that only _put_sync_suspend() is safe in IRQ-safe mode.

However, pm_runtime_put_sync() could be made safe when in IRQ-safe mode by releasing the spinlock but not re-enabling interrupts, which is what this patch aims to do.

Problem was found when using some buggy drivers that set pm_runtime_irq_safe() and used _put_sync() in interrupts-disabled context.

The offending drivers have been fixed to use _put_sync_suspend(), But this patch is an RFC to see if it might make sense to allow using _put_sync() from interrupts-disabled context.

Quoted 2 lines

OK, I'm going to take this for 3.2.

OK, great. Thanks.

Might want to just drop the last paragraph from the changelog since it doesn't really belong in the permanant history.

Quoted 18 lines · Kevin Hilman

Currently the use of pm_runtime_put_sync() is not safe from interrupts-disabled context because rpm_idle() will release the spinlock and enable interrupts for the idle callbacks. This enables interrupts during a time where interrupts were expected to be disabled, and can have strange side effects on drivers that expected interrupts to be disabled.

This is not a bug since the documentation clearly states that only _put_sync_suspend() is safe in IRQ-safe mode.

However, pm_runtime_put_sync() could be made safe when in IRQ-safe mode by releasing the spinlock but not re-enabling interrupts, which is what this patch aims to do.

Problem was found when using some buggy drivers that set pm_runtime_irq_safe() and used _put_sync() in interrupts-disabled context.

Quoted 3 lines

The offending drivers have been fixed to use _put_sync_suspend(), But this patch is an RFC to see if it might make sense to allow using _put_sync() from interrupts-disabled context.

OK, I'm going to take this for 3.2.

Quoted 18 lines · Kevin Hilman

Currently the use of pm_runtime_put_sync() is not safe from interrupts-disabled context because rpm_idle() will release the spinlock and enable interrupts for the idle callbacks. This enables interrupts during a time where interrupts were expected to be disabled, and can have strange side effects on drivers that expected interrupts to be disabled.

This is not a bug since the documentation clearly states that only _put_sync_suspend() is safe in IRQ-safe mode.

However, pm_runtime_put_sync() could be made safe when in IRQ-safe mode by releasing the spinlock but not re-enabling interrupts, which is what this patch aims to do.

Problem was found when using some buggy drivers that set pm_runtime_irq_safe() and used _put_sync() in interrupts-disabled context.

Quoted 3 lines

The offending drivers have been fixed to use _put_sync_suspend(), But this patch is an RFC to see if it might make sense to allow using _put_sync() from interrupts-disabled context.

OK, I'm going to take this for 3.2.

Quoted 23 lines · Rafael J. Wysocki

Quoted 21 lines · Kevin Hilman

Currently the use of pm_runtime_put_sync() is not safe from interrupts-disabled context because rpm_idle() will release the spinlock and enable interrupts for the idle callbacks. This enables interrupts during a time where interrupts were expected to be disabled, and can have strange side effects on drivers that expected interrupts to be disabled.

This is not a bug since the documentation clearly states that only _put_sync_suspend() is safe in IRQ-safe mode.

However, pm_runtime_put_sync() could be made safe when in IRQ-safe mode by releasing the spinlock but not re-enabling interrupts, which is what this patch aims to do.

Problem was found when using some buggy drivers that set pm_runtime_irq_safe() and used _put_sync() in interrupts-disabled context.

The offending drivers have been fixed to use _put_sync_suspend(), But this patch is an RFC to see if it might make sense to allow using _put_sync() from interrupts-disabled context.

Quoted 1 line

OK, I'm going to take this for 3.2.

Rafael,

Since you're planning to merge this, maybe we should consider merging this as a fix for v3.1, and possibly even for v3.0 stable. That way, any current drivers using irq_safe and the normal _put_sync() will not have this problem.

Quoted 27 lines · Kevin Hilman

Quoted 24 lines · Rafael J. Wysocki

Quoted 14 lines · Kevin Hilman

Currently the use of pm_runtime_put_sync() is not safe from interrupts-disabled context because rpm_idle() will release the spinlock and enable interrupts for the idle callbacks. This enables interrupts during a time where interrupts were expected to be disabled, and can have strange side effects on drivers that expected interrupts to be disabled.

This is not a bug since the documentation clearly states that only _put_sync_suspend() is safe in IRQ-safe mode.

However, pm_runtime_put_sync() could be made safe when in IRQ-safe mode by releasing the spinlock but not re-enabling interrupts, which is what this patch aims to do.

Quoted 7 lines

Problem was found when using some buggy drivers that set pm_runtime_irq_safe() and used _put_sync() in interrupts-disabled context.

The offending drivers have been fixed to use _put_sync_suspend(), But this patch is an RFC to see if it might make sense to allow using _put_sync() from interrupts-disabled context.

OK, I'm going to take this for 3.2.

Quoted 6 lines

Rafael,

Since you're planning to merge this, maybe we should consider merging this as a fix for v3.1, and possibly even for v3.0 stable. That way, any current drivers using irq_safe and the normal _put_sync() will not have this problem.

I think I can push it for 3.1, but I don't think it's stable material.

Quoted 27 lines · Kevin Hilman

Quoted 24 lines · Rafael J. Wysocki

Quoted 14 lines · Kevin Hilman

Currently the use of pm_runtime_put_sync() is not safe from interrupts-disabled context because rpm_idle() will release the spinlock and enable interrupts for the idle callbacks. This enables interrupts during a time where interrupts were expected to be disabled, and can have strange side effects on drivers that expected interrupts to be disabled.

This is not a bug since the documentation clearly states that only _put_sync_suspend() is safe in IRQ-safe mode.

However, pm_runtime_put_sync() could be made safe when in IRQ-safe mode by releasing the spinlock but not re-enabling interrupts, which is what this patch aims to do.

Quoted 7 lines

Problem was found when using some buggy drivers that set pm_runtime_irq_safe() and used _put_sync() in interrupts-disabled context.

The offending drivers have been fixed to use _put_sync_suspend(), But this patch is an RFC to see if it might make sense to allow using _put_sync() from interrupts-disabled context.

OK, I'm going to take this for 3.2.

Quoted 6 lines

Rafael,

Since you're planning to merge this, maybe we should consider merging this as a fix for v3.1, and possibly even for v3.0 stable. That way, any current drivers using irq_safe and the normal _put_sync() will not have this problem.

I think I can push it for 3.1, but I don't think it's stable material.

Quoted 23 lines · Rafael J. Wysocki

Quoted 21 lines · Kevin Hilman

Currently the use of pm_runtime_put_sync() is not safe from interrupts-disabled context because rpm_idle() will release the spinlock and enable interrupts for the idle callbacks. This enables interrupts during a time where interrupts were expected to be disabled, and can have strange side effects on drivers that expected interrupts to be disabled.

This is not a bug since the documentation clearly states that only _put_sync_suspend() is safe in IRQ-safe mode.

However, pm_runtime_put_sync() could be made safe when in IRQ-safe mode by releasing the spinlock but not re-enabling interrupts, which is what this patch aims to do.

Problem was found when using some buggy drivers that set pm_runtime_irq_safe() and used _put_sync() in interrupts-disabled context.

The offending drivers have been fixed to use _put_sync_suspend(), But this patch is an RFC to see if it might make sense to allow using _put_sync() from interrupts-disabled context.

Quoted 1 line

OK, I'm going to take this for 3.2.

Rafael,

Since you're planning to merge this, maybe we should consider merging this as a fix for v3.1, and possibly even for v3.0 stable. That way, any current drivers using irq_safe and the normal _put_sync() will not have this problem.

Quoted 18 lines · Kevin Hilman

Currently the use of pm_runtime_put_sync() is not safe from interrupts-disabled context because rpm_idle() will release the spinlock and enable interrupts for the idle callbacks. This enables interrupts during a time where interrupts were expected to be disabled, and can have strange side effects on drivers that expected interrupts to be disabled.

This is not a bug since the documentation clearly states that only _put_sync_suspend() is safe in IRQ-safe mode.

However, pm_runtime_put_sync() could be made safe when in IRQ-safe mode by releasing the spinlock but not re-enabling interrupts, which is what this patch aims to do.

Problem was found when using some buggy drivers that set pm_runtime_irq_safe() and used _put_sync() in interrupts-disabled context.

Quoted 3 lines

The offending drivers have been fixed to use _put_sync_suspend(), But this patch is an RFC to see if it might make sense to allow using _put_sync() from interrupts-disabled context.

OK, I'm going to take this for 3.2.

Currently the use of pm_runtime_put_sync() is not safe from interrupts-disabled context because rpm_idle() will release the spinlock and enable interrupts for the idle callbacks. This enables interrupts during a time where interrupts were expected to be disabled, and can have strange side effects on drivers that expected interrupts to be disabled.

This is not a bug since the documentation clearly states that only _put_sync_suspend() is safe in IRQ-safe mode.

However, pm_runtime_put_sync() could be made safe when in IRQ-safe mode by releasing the spinlock but not re-enabling interrupts, which is what this patch aims to do.

Problem was found when using some buggy drivers that set pm_runtime_irq_safe() and used _put_sync() in interrupts-disabled context.

The offending drivers have been fixed to use _put_sync_suspend(), But this patch is an RFC to see if it might make sense to allow using _put_sync() from interrupts-disabled context.

Reported-by Colin Cross <[email protected]>

Tested-by Nishanth Menon <[email protected]>

Signed-off-by Kevin Hilman <[email protected]>

---
v2: update documentation also

 Documentation/power/runtime_pm.txt |   10 +++++-----
 drivers/base/power/runtime.c       |   10 ++++++++--
 2 files changed, 13 insertions(+), 7 deletions(-)

Documentation/power/runtime_pm.txt +5 -5

index 14dd3c6..4ce5450 100644
@@ -54,11 +54,10 @@ referred to as subsystem-level callbacks in what follows.
 By default, the callbacks are always invoked in process context with interrupts
 enabled.  However, subsystems can use the pm_runtime_irq_safe() helper function
 to tell the PM core that a device's ->runtime_suspend() and ->runtime_resume()
-callbacks should be invoked in atomic context with interrupts disabled
-(->runtime_idle() is still invoked the default way).  This implies that these
-callback routines must not block or sleep, but it also means that the
-synchronous helper functions listed at the end of Section 4 can be used within
-an interrupt handler or in an atomic context.
+callbacks should be invoked in atomic context with interrupts disabled.
+This implies that these callback routines must not block or sleep, but it also
+means that the synchronous helper functions listed at the end of Section 4 can
+be used within an interrupt handler or in an atomic context.
 
 The subsystem-level suspend callback is _entirely_ _responsible_ for handling
 the suspend of the device as appropriate, which may, but need not include

@@ -483,6 +482,7 @@ pm_runtime_suspend()
 pm_runtime_autosuspend()
 pm_runtime_resume()
 pm_runtime_get_sync()
+pm_runtime_put_sync()
 pm_runtime_put_sync_suspend()
 
 5. Runtime PM Initialization, Device Probing and Removal

drivers/base/power/runtime.c +8 -2

index 8dc247c..acb3f83 100644
@@ -226,11 +226,17 @@ static int rpm_idle(struct device *dev, int rpmflags)
 		callback = NULL;
 
 	if (callback) {
-		spin_unlock_irq(&dev->power.lock);
+		if (dev->power.irq_safe)
+			spin_unlock(&dev->power.lock);
+		else
+			spin_unlock_irq(&dev->power.lock);
 
 		callback(dev);
 
-		spin_lock_irq(&dev->power.lock);
+		if (dev->power.irq_safe)
+			spin_lock(&dev->power.lock);
+		else
+			spin_lock_irq(&dev->power.lock);
 	}
 
 	dev->power.idle_notification = false;