Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

Re: [PATCH] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Wed, 6 Oct 2010, Rafael J. Wysocki wrote:

> Defer the resume.  That's the only thing you can do in any case, whether you're
> going to busy loop or just use a workqueue.

They are not the same.  With a busy-wait you handle the device as soon
as possible, before the interrupt routine returns.  With a workqueue
you have to mask the entire IRQ line, possibly losing interrupt
requests from other devices, until the workqueue routine can run.

> > On the whole, I don't see any striking reason for the PM core not to 
> > busy-wait during a concurrent state change.
> 
> I do.  That shouldn't happen in a fast path and we're talking about one,
> aren't we?  Besides, I don't like busy waiting as a rule.

On Wed, 6 Oct 2010, Kevin Hilman wrote:

> Not sure I follow where you're going with this last paragraph.  Of
> course, calls from ISR context cannot busy wait.

What do you guys think spin_lock() does?  It busy-waits until the lock 
is free!  If you don't like busy-waiting then you don't like spinlocks, 
and if you believe ISR's can't busy-wait then you believe they can't 
acquire spinlocks.  :-)

Really, what I'm talking about is not much different from continuing to
hold dev->power.lock across the runtime_resume and runtime_suspend
callbacks.  Would you prefer to do that instead of explicitly
busy-waiting?  There would be two disadvantages.  First, the
runtime_suspend routine would not be able to call pm_request_idle() or
pm_schedule_suspend() and then return -EBUSY.  Second, if the driver
uses a private lock then the callback routines might not be able to
acquire it.

On Wed, 6 Oct 2010, Rafael J. Wysocki wrote:

> Overall, we seem to have a corner case to cover and we're considering doing
> intrusive changes to the framework because of that, even though that changes
> may not be really necessary in practice.  I think we should focus more on the
> corner case instead.

I'm not sure what you mean.

Alan Stern

[PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

This patch (as1431b) makes the synchronous runtime-PM interface
suitable for use in interrupt handlers.  Subsystems can call the new
pm_runtime_irq_safe() function to tell the PM core that a device's
runtime-PM callbacks should be invoked with interrupts disabled
(runtime_suspend and runtime_resume callbacks will be invoked with the
spinlock held as well).  This permits the pm_runtime_get_sync() and
pm_runtime_put_sync() routines to be called from within interrupt
handlers.

When a device is declared irq-safe in this way, the PM core increments
the parent's usage count, so the parent will never be runtime
suspended.  This prevents difficult situations in which an irq-safe
device can't resume because it is forced to wait for its non-irq-safe
parent.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

Index: usb-2.6/include/linux/pm.h
===================================================================
--- usb-2.6.orig/include/linux/pm.h
+++ usb-2.6/include/linux/pm.h
@@ -486,6 +486,7 @@ struct dev_pm_info {
 	unsigned int		run_wake:1;
 	unsigned int		runtime_auto:1;
 	unsigned int		no_callbacks:1;
+	unsigned int		irq_safe:1;
 	unsigned int		use_autosuspend:1;
 	unsigned int		timer_autosuspends:1;
 	enum rpm_request	request;
Index: usb-2.6/include/linux/pm_runtime.h
===================================================================
--- usb-2.6.orig/include/linux/pm_runtime.h
+++ usb-2.6/include/linux/pm_runtime.h
@@ -40,6 +40,7 @@ extern int pm_generic_runtime_idle(struc
 extern int pm_generic_runtime_suspend(struct device *dev);
 extern int pm_generic_runtime_resume(struct device *dev);
 extern void pm_runtime_no_callbacks(struct device *dev);
+extern void pm_runtime_irq_safe(struct device *dev);
 extern void __pm_runtime_use_autosuspend(struct device *dev, bool use);
 extern void pm_runtime_set_autosuspend_delay(struct device *dev, int delay);
 extern unsigned long pm_runtime_autosuspend_expiration(struct device *dev);
@@ -123,6 +124,7 @@ static inline int pm_generic_runtime_idl
 static inline int pm_generic_runtime_suspend(struct device *dev) { return 0; }
 static inline int pm_generic_runtime_resume(struct device *dev) { return 0; }
 static inline void pm_runtime_no_callbacks(struct device *dev) {}
+static inline void pm_runtime_irq_safe(struct device *dev) {}
 
 static inline void pm_runtime_mark_last_busy(struct device *dev) {}
 static inline void __pm_runtime_use_autosuspend(struct device *dev,
Index: usb-2.6/drivers/base/power/runtime.c
===================================================================
--- usb-2.6.orig/drivers/base/power/runtime.c
+++ usb-2.6/drivers/base/power/runtime.c
@@ -223,11 +223,19 @@ static int rpm_idle(struct device *dev, 
 		callback = NULL;
 
 	if (callback) {
-		spin_unlock_irq(&dev->power.lock);
+		if (dev->power.irq_safe) {
+			spin_unlock(&dev->power.lock);
 
-		callback(dev);
+			callback(dev);
 
-		spin_lock_irq(&dev->power.lock);
+			spin_lock(&dev->power.lock);
+		} else {
+			spin_unlock_irq(&dev->power.lock);
+
+			callback(dev);
+
+			spin_lock_irq(&dev->power.lock);
+		}
 	}
 
 	dev->power.idle_notification = false;
@@ -250,13 +258,16 @@ static int rpm_callback(int (*cb)(struct
 	if (!cb)
 		return -ENOSYS;
 
-	spin_unlock_irq(&dev->power.lock);
+	if (dev->power.irq_safe) {
+		retval = cb(dev);
+	} else {
+		spin_unlock_irq(&dev->power.lock);
 
-	retval = cb(dev);
+		retval = cb(dev);
 
-	spin_lock_irq(&dev->power.lock);
+		spin_lock_irq(&dev->power.lock);
+	}
 	dev->power.runtime_error = retval;
-
 	return retval;
 }
 
@@ -404,7 +415,7 @@ static int rpm_suspend(struct device *de
 		goto out;
 	}
 
-	if (parent && !parent->power.ignore_children) {
+	if (parent && !parent->power.ignore_children && !dev->power.irq_safe) {
 		spin_unlock_irq(&dev->power.lock);
 
 		pm_request_idle(parent);
@@ -527,10 +538,13 @@ static int rpm_resume(struct device *dev
 
 	if (!parent && dev->parent) {
 		/*
-		 * Increment the parent's resume counter and resume it if
-		 * necessary.
+		 * Increment the parent's usage counter and resume it if
+		 * necessary.  Not needed if dev is irq-safe; then the
+		 * parent is permanently resumed.
 		 */
 		parent = dev->parent;
+		if (dev->power.irq_safe)
+			goto skip_parent;
 		spin_unlock(&dev->power.lock);
 
 		pm_runtime_get_noresume(parent);
@@ -553,6 +567,7 @@ static int rpm_resume(struct device *dev
 			goto out;
 		goto repeat;
 	}
+ skip_parent:
 
 	if (dev->power.no_callbacks)
 		goto no_callback;	/* Assume success. */
@@ -584,7 +599,7 @@ static int rpm_resume(struct device *dev
 		rpm_idle(dev, RPM_ASYNC);
 
  out:
-	if (parent) {
+	if (parent && !dev->power.irq_safe) {
 		spin_unlock_irq(&dev->power.lock);
 
 		pm_runtime_put(parent);
@@ -1065,7 +1080,6 @@ EXPORT_SYMBOL_GPL(pm_runtime_allow);
  * Set the power.no_callbacks flag, which tells the PM core that this
  * device is power-managed through its parent and has no run-time PM
  * callbacks of its own.  The run-time sysfs attributes will be removed.
- *
  */
 void pm_runtime_no_callbacks(struct device *dev)
 {
@@ -1078,6 +1092,28 @@ void pm_runtime_no_callbacks(struct devi
 EXPORT_SYMBOL_GPL(pm_runtime_no_callbacks);
 
 /**
+ * pm_runtime_irq_safe - Leave interrupts disabled during callbacks.
+ * @dev: Device to handle
+ *
+ * Set the power.irq_safe flag, which tells the PM core that the
+ * ->runtime_suspend() and ->runtime_resume() callbacks for this device should
+ * always be invoked with the spinlock held, and all three callbacks should
+ * always be invoked with interrupts disabled.  It also causes the parent's
+ * usage counter to be permanently incremented, preventing the parent from
+ * runtime suspending -- otherwise an irq-safe child might have to wait for a
+ * non-irq-safe parent.
+ */
+void pm_runtime_irq_safe(struct device *dev)
+{
+	if (dev->parent)
+		pm_runtime_get_sync(dev->parent);
+	spin_lock_irq(&dev->power.lock);
+	dev->power.irq_safe = 1;
+	spin_unlock_irq(&dev->power.lock);
+}
+EXPORT_SYMBOL_GPL(pm_runtime_irq_safe);
+
+/**
  * update_autosuspend - Handle a change to a device's autosuspend settings.
  * @dev: Device to handle.
  * @old_delay: The former autosuspend_delay value.
@@ -1199,4 +1235,6 @@ void pm_runtime_remove(struct device *de
 	/* Change the status back to 'suspended' to match the initial status. */
 	if (dev->power.runtime_status == RPM_ACTIVE)
 		pm_runtime_set_suspended(dev);
+	if (dev->power.irq_safe && dev->parent)
+		pm_runtime_put_sync(dev->parent);
 }
Index: usb-2.6/Documentation/power/runtime_pm.txt
===================================================================
--- usb-2.6.orig/Documentation/power/runtime_pm.txt
+++ usb-2.6/Documentation/power/runtime_pm.txt
@@ -50,6 +50,13 @@ type's callbacks are not defined) of giv
 and device class callbacks are 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 callbacks should always be invoked with
+interrupts disabled.  This implies that the 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 from within an interrupt handler.
+
 The subsystem-level suspend callback is _entirely_ _responsible_ for handling
 the suspend of the device as appropriate, which may, but need not include
 executing the device driver's own ->runtime_suspend() callback (from the
@@ -237,6 +244,11 @@ defined in include/linux/pm.h:
       Section 8); it may be modified only by the pm_runtime_no_callbacks()
       helper function
 
+  unsigned int irq_safe;
+    - indicates that the runtime-PM callbacks should be invoked with interrupts
+      disabled; the ->runtime_suspend() and ->runtime_resume() callbacks will
+      be made with the spinlock held as well
+
   unsigned int use_autosuspend;
     - indicates that the device's driver supports delayed autosuspend (see
       Section 9); it may be modified only by the
@@ -397,6 +409,10 @@ drivers/base/power/runtime.c and include
       PM attributes from /sys/devices/.../power (or prevent them from being
       added when the device is registered)
 
+  void pm_runtime_irq_safe(struct device *dev);
+    - set the power.irq_safe flag for the device, causing the runtime-PM
+      callbacks to be invoked with interrupts disabled
+
   void pm_runtime_mark_last_busy(struct device *dev);
     - set the power.last_busy field to the current time
 
@@ -438,6 +454,16 @@ pm_runtime_suspended()
 pm_runtime_mark_last_busy()
 pm_runtime_autosuspend_expiration()
 
+If pm_runtime_irq_safe() has been called for a device then the following helper
+functions may also be called in interrupt context:
+
+pm_runtime_idle()
+pm_runtime_suspend()
+pm_runtime_autosuspend()
+pm_runtime_resume()
+pm_runtime_get_sync()
+pm_runtime_put_sync()
+
 5. Run-time PM Initialization, Device Probing and Removal
 
 Initially, the run-time PM is disabled for all devices, which means that the

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Friday, November 19, 2010, Alan Stern wrote:
> This patch (as1431b) makes the synchronous runtime-PM interface
> suitable for use in interrupt handlers.  Subsystems can call the new
> pm_runtime_irq_safe() function to tell the PM core that a device's
> runtime-PM callbacks should be invoked with interrupts disabled
> (runtime_suspend and runtime_resume callbacks will be invoked with the
> spinlock held as well).  This permits the pm_runtime_get_sync() and
> pm_runtime_put_sync() routines to be called from within interrupt
> handlers.
> 
> When a device is declared irq-safe in this way, the PM core increments
> the parent's usage count, so the parent will never be runtime
> suspended.  This prevents difficult situations in which an irq-safe
> device can't resume because it is forced to wait for its non-irq-safe
> parent.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
> 
> Index: usb-2.6/include/linux/pm.h
> ===================================================================
> --- usb-2.6.orig/include/linux/pm.h
> +++ usb-2.6/include/linux/pm.h
> @@ -486,6 +486,7 @@ struct dev_pm_info {
>  	unsigned int		run_wake:1;
>  	unsigned int		runtime_auto:1;
>  	unsigned int		no_callbacks:1;
> +	unsigned int		irq_safe:1;
>  	unsigned int		use_autosuspend:1;
>  	unsigned int		timer_autosuspends:1;
>  	enum rpm_request	request;
> Index: usb-2.6/include/linux/pm_runtime.h
> ===================================================================
> --- usb-2.6.orig/include/linux/pm_runtime.h
> +++ usb-2.6/include/linux/pm_runtime.h
> @@ -40,6 +40,7 @@ extern int pm_generic_runtime_idle(struc
>  extern int pm_generic_runtime_suspend(struct device *dev);
>  extern int pm_generic_runtime_resume(struct device *dev);
>  extern void pm_runtime_no_callbacks(struct device *dev);
> +extern void pm_runtime_irq_safe(struct device *dev);
>  extern void __pm_runtime_use_autosuspend(struct device *dev, bool use);
>  extern void pm_runtime_set_autosuspend_delay(struct device *dev, int delay);
>  extern unsigned long pm_runtime_autosuspend_expiration(struct device *dev);
> @@ -123,6 +124,7 @@ static inline int pm_generic_runtime_idl
>  static inline int pm_generic_runtime_suspend(struct device *dev) { return 0; }
>  static inline int pm_generic_runtime_resume(struct device *dev) { return 0; }
>  static inline void pm_runtime_no_callbacks(struct device *dev) {}
> +static inline void pm_runtime_irq_safe(struct device *dev) {}
>  
>  static inline void pm_runtime_mark_last_busy(struct device *dev) {}
>  static inline void __pm_runtime_use_autosuspend(struct device *dev,
> Index: usb-2.6/drivers/base/power/runtime.c
> ===================================================================
> --- usb-2.6.orig/drivers/base/power/runtime.c
> +++ usb-2.6/drivers/base/power/runtime.c
> @@ -223,11 +223,19 @@ static int rpm_idle(struct device *dev, 
>  		callback = NULL;
>  
>  	if (callback) {
> -		spin_unlock_irq(&dev->power.lock);
> +		if (dev->power.irq_safe) {
> +			spin_unlock(&dev->power.lock);
>  
> -		callback(dev);
> +			callback(dev);
>  
> -		spin_lock_irq(&dev->power.lock);
> +			spin_lock(&dev->power.lock);
> +		} else {
> +			spin_unlock_irq(&dev->power.lock);
> +
> +			callback(dev);
> +
> +			spin_lock_irq(&dev->power.lock);
> +		}
>  	}

I didn't like this change before and I still don't like it.  Quite frankly, I'm
not sure I can convince Linus to pull it. :-)

Why don't we simply execute the callback under the spinlock in the
IRQ safe case?

Rafael

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Sat, 20 Nov 2010, Rafael J. Wysocki wrote:

> On Friday, November 19, 2010, Alan Stern wrote:
> > This patch (as1431b) makes the synchronous runtime-PM interface
> > suitable for use in interrupt handlers.  Subsystems can call the new
> > pm_runtime_irq_safe() function to tell the PM core that a device's
> > runtime-PM callbacks should be invoked with interrupts disabled
> > (runtime_suspend and runtime_resume callbacks will be invoked with the
> > spinlock held as well).  This permits the pm_runtime_get_sync() and
> > pm_runtime_put_sync() routines to be called from within interrupt
> > handlers.
> > 
> > When a device is declared irq-safe in this way, the PM core increments
> > the parent's usage count, so the parent will never be runtime
> > suspended.  This prevents difficult situations in which an irq-safe
> > device can't resume because it is forced to wait for its non-irq-safe
> > parent.
> > 
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

> > --- usb-2.6.orig/drivers/base/power/runtime.c
> > +++ usb-2.6/drivers/base/power/runtime.c
> > @@ -223,11 +223,19 @@ static int rpm_idle(struct device *dev, 
> >  		callback = NULL;
> >  
> >  	if (callback) {
> > -		spin_unlock_irq(&dev->power.lock);
> > +		if (dev->power.irq_safe) {
> > +			spin_unlock(&dev->power.lock);
> >  
> > -		callback(dev);
> > +			callback(dev);
> >  
> > -		spin_lock_irq(&dev->power.lock);
> > +			spin_lock(&dev->power.lock);
> > +		} else {
> > +			spin_unlock_irq(&dev->power.lock);
> > +
> > +			callback(dev);
> > +
> > +			spin_lock_irq(&dev->power.lock);
> > +		}
> >  	}
> 
> I didn't like this change before and I still don't like it.  Quite frankly, I'm
> not sure I can convince Linus to pull it. :-)
> 
> Why don't we simply execute the callback under the spinlock in the
> IRQ safe case?

Because it wouldn't work.  The job of the runtime_idle callback is to
call pm_runtime_suspend when needed.  But if the callback runs under
the spinlock then pm_runtime_suspend would hang when it tries to grab
the lock.

I don't think Linus will object to this.  What he doesn't like is when
some code drops a lock, reacquires it, and then behaves as though the
lock had been held all along.  That's not the case here; rpm_idle()  
does not depend on any state remaining unchanged across the callback.

Alan Stern

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Sat, 20 Nov 2010, Alan Stern wrote:

> On Sat, 20 Nov 2010, Rafael J. Wysocki wrote:
> 
> > On Friday, November 19, 2010, Alan Stern wrote:
> > > This patch (as1431b) makes the synchronous runtime-PM interface
> > > suitable for use in interrupt handlers.  Subsystems can call the new
> > > pm_runtime_irq_safe() function to tell the PM core that a device's
> > > runtime-PM callbacks should be invoked with interrupts disabled
> > > (runtime_suspend and runtime_resume callbacks will be invoked with the
> > > spinlock held as well).  This permits the pm_runtime_get_sync() and
> > > pm_runtime_put_sync() routines to be called from within interrupt
> > > handlers.
> > > 
> > > When a device is declared irq-safe in this way, the PM core increments
> > > the parent's usage count, so the parent will never be runtime
> > > suspended.  This prevents difficult situations in which an irq-safe
> > > device can't resume because it is forced to wait for its non-irq-safe
> > > parent.
> > > 
> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> > > --- usb-2.6.orig/drivers/base/power/runtime.c
> > > +++ usb-2.6/drivers/base/power/runtime.c
> > > @@ -223,11 +223,19 @@ static int rpm_idle(struct device *dev, 
> > >  		callback = NULL;
> > >  
> > >  	if (callback) {
> > > -		spin_unlock_irq(&dev->power.lock);
> > > +		if (dev->power.irq_safe) {
> > > +			spin_unlock(&dev->power.lock);
> > >  
> > > -		callback(dev);
> > > +			callback(dev);
> > >  
> > > -		spin_lock_irq(&dev->power.lock);
> > > +			spin_lock(&dev->power.lock);
> > > +		} else {
> > > +			spin_unlock_irq(&dev->power.lock);
> > > +
> > > +			callback(dev);
> > > +
> > > +			spin_lock_irq(&dev->power.lock);
> > > +		}
> > >  	}
> > 
> > I didn't like this change before and I still don't like it.  Quite frankly, I'm
> > not sure I can convince Linus to pull it. :-)
> > 
> > Why don't we simply execute the callback under the spinlock in the
> > IRQ safe case?
> 
> Because it wouldn't work.  The job of the runtime_idle callback is to
> call pm_runtime_suspend when needed.  But if the callback runs under
> the spinlock then pm_runtime_suspend would hang when it tries to grab
> the lock.
> 
> I don't think Linus will object to this.  What he doesn't like is when
> some code drops a lock, reacquires it, and then behaves as though the
> lock had been held all along.  That's not the case here; rpm_idle()  
> does not depend on any state remaining unchanged across the callback.

One other thing I forgot to mention...  If Linus doesn't like the way
the new code drops the spinlock and then reacquires it, then he must
also not like the existing code, which does the same thing.  The only
difference lies in whether or not interrupts are re-enabled.

Alan Stern

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Saturday, November 20, 2010, Alan Stern wrote:
> On Sat, 20 Nov 2010, Rafael J. Wysocki wrote:
> 
> > On Friday, November 19, 2010, Alan Stern wrote:
> > > This patch (as1431b) makes the synchronous runtime-PM interface
> > > suitable for use in interrupt handlers.  Subsystems can call the new
> > > pm_runtime_irq_safe() function to tell the PM core that a device's
> > > runtime-PM callbacks should be invoked with interrupts disabled
> > > (runtime_suspend and runtime_resume callbacks will be invoked with the
> > > spinlock held as well).  This permits the pm_runtime_get_sync() and
> > > pm_runtime_put_sync() routines to be called from within interrupt
> > > handlers.
> > > 
> > > When a device is declared irq-safe in this way, the PM core increments
> > > the parent's usage count, so the parent will never be runtime
> > > suspended.  This prevents difficult situations in which an irq-safe
> > > device can't resume because it is forced to wait for its non-irq-safe
> > > parent.
> > > 
> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> > > --- usb-2.6.orig/drivers/base/power/runtime.c
> > > +++ usb-2.6/drivers/base/power/runtime.c
> > > @@ -223,11 +223,19 @@ static int rpm_idle(struct device *dev, 
> > >  		callback = NULL;
> > >  
> > >  	if (callback) {
> > > -		spin_unlock_irq(&dev->power.lock);
> > > +		if (dev->power.irq_safe) {
> > > +			spin_unlock(&dev->power.lock);
> > >  
> > > -		callback(dev);
> > > +			callback(dev);
> > >  
> > > -		spin_lock_irq(&dev->power.lock);
> > > +			spin_lock(&dev->power.lock);
> > > +		} else {
> > > +			spin_unlock_irq(&dev->power.lock);
> > > +
> > > +			callback(dev);
> > > +
> > > +			spin_lock_irq(&dev->power.lock);
> > > +		}
> > >  	}
> > 
> > I didn't like this change before and I still don't like it.  Quite frankly, I'm
> > not sure I can convince Linus to pull it. :-)
> > 
> > Why don't we simply execute the callback under the spinlock in the
> > IRQ safe case?
> 
> Because it wouldn't work.  The job of the runtime_idle callback is to
> call pm_runtime_suspend when needed.  But if the callback runs under
> the spinlock then pm_runtime_suspend would hang when it tries to grab
> the lock.

Yes, in the _idle case.  I actually should have put my comment under
the change in rpm_callback(), which is what I really meant.

Moreover, I'm not sure if we need an "IRQ safe" version of _idle.  Why do
we need it, exactly?

> I don't think Linus will object to this.

Well, I guess we'll see. :-)

Thanks,
Rafael

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Saturday, November 20, 2010, Alan Stern wrote:
> On Sat, 20 Nov 2010, Alan Stern wrote:
> 
> > On Sat, 20 Nov 2010, Rafael J. Wysocki wrote:
> > 
> > > On Friday, November 19, 2010, Alan Stern wrote:
...
> > 
> > I don't think Linus will object to this.  What he doesn't like is when
> > some code drops a lock, reacquires it, and then behaves as though the
> > lock had been held all along.  That's not the case here; rpm_idle()  
> > does not depend on any state remaining unchanged across the callback.
> 
> One other thing I forgot to mention...  If Linus doesn't like the way
> the new code drops the spinlock and then reacquires it, then he must
> also not like the existing code, which does the same thing.  The only
> difference lies in whether or not interrupts are re-enabled.

The problem I have with this change is that switching interrupts off really is
a part of the locking operation, so using spin_unlock() after spin_lock_irq...()
is kind of like releasing the lock partially, which I don't think is valid
(even if we're going to reacquire the lock immediately).

Thanks,
Rafael

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Mon, 22 Nov 2010, Rafael J. Wysocki wrote:

> > > I didn't like this change before and I still don't like it.  Quite frankly, I'm
> > > not sure I can convince Linus to pull it. :-)
> > > 
> > > Why don't we simply execute the callback under the spinlock in the
> > > IRQ safe case?
> > 
> > Because it wouldn't work.  The job of the runtime_idle callback is to
> > call pm_runtime_suspend when needed.  But if the callback runs under
> > the spinlock then pm_runtime_suspend would hang when it tries to grab
> > the lock.
> 
> Yes, in the _idle case.  I actually should have put my comment under
> the change in rpm_callback(), which is what I really meant.

But the new rpm_callback() _does_ simply execute the callback under the
spinlock in the irq-safe case.  So I don't understand what you mean
here.

> Moreover, I'm not sure if we need an "IRQ safe" version of _idle.  Why do
> we need it, exactly?

Because pm_runtime_put_sync() calls rpm_idle().  If there were no 
irq-safe version of rpm_idle() then drivers wouldn't be able to call 
pm_runtime_put_sync() from within an interrupt handler.


> The problem I have with this change is that switching interrupts off really is
> a part of the locking operation, so using spin_unlock() after spin_lock_irq...()
> is kind of like releasing the lock partially, which I don't think is valid
> (even if we're going to reacquire the lock immediately).

On the contrary; spin_unlock() after spin_lock_irq() doesn't partially
release the lock -- it releases the lock _entirely_!  :-)

Besides which, the existing code already releases the spinlock before 
making callbacks, so there should be no reason to worry about that 
issue.  The new code either:

	releases the spinlock but keeps interrupts disabled (in
	rpm_idle), or

	doesn't release the spinlock (in rpm_callback).

Either way, I should think you'd find the new code _less_ objectionable 
than the existing code, not _more_ objectionable.

Alan Stern

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Monday, November 22, 2010, Alan Stern wrote:
> On Mon, 22 Nov 2010, Rafael J. Wysocki wrote:
> 
> > > > I didn't like this change before and I still don't like it.  Quite frankly, I'm
> > > > not sure I can convince Linus to pull it. :-)
> > > > 
> > > > Why don't we simply execute the callback under the spinlock in the
> > > > IRQ safe case?
> > > 
> > > Because it wouldn't work.  The job of the runtime_idle callback is to
> > > call pm_runtime_suspend when needed.  But if the callback runs under
> > > the spinlock then pm_runtime_suspend would hang when it tries to grab
> > > the lock.
> > 
> > Yes, in the _idle case.  I actually should have put my comment under
> > the change in rpm_callback(), which is what I really meant.
> 
> But the new rpm_callback() _does_ simply execute the callback under the
> spinlock in the irq-safe case.  So I don't understand what you mean
> here.

OK, sorry, I confused things.  I have no objections to this part, then, let's
focus on the _idle case.

> > Moreover, I'm not sure if we need an "IRQ safe" version of _idle.  Why do
> > we need it, exactly?
> 
> Because pm_runtime_put_sync() calls rpm_idle().  If there were no 
> irq-safe version of rpm_idle() then drivers wouldn't be able to call 
> pm_runtime_put_sync() from within an interrupt handler.

Right.  Which they can't do now, can they?

Why do you think we should allow them to do that?

> > The problem I have with this change is that switching interrupts off really is
> > a part of the locking operation, so using spin_unlock() after spin_lock_irq...()
> > is kind of like releasing the lock partially, which I don't think is valid
> > (even if we're going to reacquire the lock immediately).
> 
> On the contrary; spin_unlock() after spin_lock_irq() doesn't partially
> release the lock -- it releases the lock _entirely_!  :-)

Well, not really, as far as I understand it.  The semantics of spin_lock_irq()
is "turn interrupts off to prevent interrupt handlers running on _this_ CPU
from racing with us and acquire the lock to prevent _other_ CPUs from racing
with us".  If you build the code for non-SMP it becomes the first part only.
So IMO the "turn interrupts on/off" thing is a part of the full synchronization
mechanism in this case.

Anyway, though, if the only reason of doing this is to allow interrupt handlers
to call pm_runtime_put_sync(), then I rather wouldn't do it at all.

Thanks,
Rafael

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Tue, 23 Nov 2010, Rafael J. Wysocki wrote:

> > > Moreover, I'm not sure if we need an "IRQ safe" version of _idle.  Why do
> > > we need it, exactly?
> > 
> > Because pm_runtime_put_sync() calls rpm_idle().  If there were no 
> > irq-safe version of rpm_idle() then drivers wouldn't be able to call 
> > pm_runtime_put_sync() from within an interrupt handler.
> 
> Right.  Which they can't do now, can they?

True.  That was the point of this patch -- to allow interrupt handlers
to do runtime PM, which they can't do now.

> Why do you think we should allow them to do that?

Are you suggesting that interrupt handlers stick to pm_runtime_suspend 
and pm_runtime_resume, and ignore pm_runtime_get_sync and 
pm_runtime_put_sync?

Recall that after probing is finished, the driver core does a
pm_runtime_put_sync.  That might happen while an interrupt handler is
running on another CPU.  If the interrupt handler didn't increment the
usage_count, the driver core could cause the device to suspend while
the interrupt handler was still using it.

Or are you suggesting that interrupt handlers use pm_runtime_get_sync 
and implement a poor-man's version of pm_runtime_put_sync by doing:

	pm_runtime_put_no_idle();
	pm_runtime_suspend();

Is there some particular reason for denying interrupt handlers the
ability to use pm_runtime_put_sync?  It seems odd to disallow that 
while allowing pm_runtime_get_sync.

Or maybe you think that when pm_runtime_put_sync detects the 
usage_count has decremented to 0 and the device is irq-safe, it should 
call rpm_suspend directly instead of calling rpm_idle?

In short, I don't see any reason not to present the same API to
interrupt handlers for irq-safe devices as we present to
process-context drivers for ordinary devices.

> Anyway, though, if the only reason of doing this is to allow interrupt handlers
> to call pm_runtime_put_sync(), then I rather wouldn't do it at all.

Why not?

Alan Stern

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Tuesday, November 23, 2010, Alan Stern wrote:
> On Tue, 23 Nov 2010, Rafael J. Wysocki wrote:
> 
> > > > Moreover, I'm not sure if we need an "IRQ safe" version of _idle.  Why do
> > > > we need it, exactly?
> > > 
> > > Because pm_runtime_put_sync() calls rpm_idle().  If there were no 
> > > irq-safe version of rpm_idle() then drivers wouldn't be able to call 
> > > pm_runtime_put_sync() from within an interrupt handler.
> > 
> > Right.  Which they can't do now, can they?
> 
> True.  That was the point of this patch -- to allow interrupt handlers
> to do runtime PM, which they can't do now.

The original idea was to allow suspend and resume to be carried out
with interrupts off, not necessarily by interrupt handlers.  We've never
considered doing that with _idle before.

> > Why do you think we should allow them to do that?
> 
> Are you suggesting that interrupt handlers stick to pm_runtime_suspend 
> and pm_runtime_resume, and ignore pm_runtime_get_sync and 
> pm_runtime_put_sync?

Why do they need the _sync versions?  What exactly is wrong with
calling

pm_runtime_put_noidle()
pm_runtime_suspend()

from an interrupt handler if it really wants the synchronous suspend to be
carried out at this point?

I don't really see a reason for calling pm_runtime_put_sync() by an interrupt
handler, but perhaps I'm overlooking something important.

> Recall that after probing is finished, the driver core does a
> pm_runtime_put_sync.  That might happen while an interrupt handler is
> running on another CPU.  If the interrupt handler didn't increment the
> usage_count, the driver core could cause the device to suspend while
> the interrupt handler was still using it.
> 
> Or are you suggesting that interrupt handlers use pm_runtime_get_sync 
> and implement a poor-man's version of pm_runtime_put_sync by doing:
> 
> 	pm_runtime_put_no_idle();
> 	pm_runtime_suspend();

Yes, I am.  Although simply calling pm_runtime_put() should work as well (yes,
the suspend won't occur immediately, but what's wrong with that?).

> Is there some particular reason for denying interrupt handlers the
> ability to use pm_runtime_put_sync?  It seems odd to disallow that 
> while allowing pm_runtime_get_sync.
> 
> Or maybe you think that when pm_runtime_put_sync detects the 
> usage_count has decremented to 0 and the device is irq-safe, it should 
> call rpm_suspend directly instead of calling rpm_idle?

That also would work for me, actually.

> In short, I don't see any reason not to present the same API to
> interrupt handlers for irq-safe devices as we present to
> process-context drivers for ordinary devices.
> 
> > Anyway, though, if the only reason of doing this is to allow interrupt handlers
> > to call pm_runtime_put_sync(), then I rather wouldn't do it at all.
> 
> Why not?

Because it's a fragile design with unusual behavior.  I'm pretty sure we'd see
some interesting abuse of it sooner or later. :-)

Thanks,
Rafael

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> On Tuesday, November 23, 2010, Alan Stern wrote:
>> On Tue, 23 Nov 2010, Rafael J. Wysocki wrote:
>> 
>> > > > Moreover, I'm not sure if we need an "IRQ safe" version of _idle.  Why do
>> > > > we need it, exactly?
>> > > 
>> > > Because pm_runtime_put_sync() calls rpm_idle().  If there were no 
>> > > irq-safe version of rpm_idle() then drivers wouldn't be able to call 
>> > > pm_runtime_put_sync() from within an interrupt handler.
>> > 
>> > Right.  Which they can't do now, can they?
>> 
>> True.  That was the point of this patch -- to allow interrupt handlers
>> to do runtime PM, which they can't do now.
>
> The original idea was to allow suspend and resume to be carried out
> with interrupts off, not necessarily by interrupt handlers.  We've never
> considered doing that with _idle before.
>
>> > Why do you think we should allow them to do that?
>> 
>> Are you suggesting that interrupt handlers stick to pm_runtime_suspend 
>> and pm_runtime_resume, and ignore pm_runtime_get_sync and 
>> pm_runtime_put_sync?
>
> Why do they need the _sync versions?  What exactly is wrong with
> calling
>
> pm_runtime_put_noidle()
> pm_runtime_suspend()
>
> from an interrupt handler if it really wants the synchronous suspend to be
> carried out at this point?
>
> I don't really see a reason for calling pm_runtime_put_sync() by an interrupt
> handler, but perhaps I'm overlooking something important.

While I like the idea of the symmetry of having both _get_sync() and
_put_sync() callable from an interrupt handler, I can't currently think
of a situation where we would need to _put_sync() in the ISR.  A
standard _put() should suffice for all cases I can imagine.

Kevin

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Tue, 23 Nov 2010, Rafael J. Wysocki wrote:

> > Or maybe you think that when pm_runtime_put_sync detects the 
> > usage_count has decremented to 0 and the device is irq-safe, it should 
> > call rpm_suspend directly instead of calling rpm_idle?
> 
> That also would work for me, actually.

Okay, then consider this proposal.  I'll introduce a new
pm_runtime_put_sync_suspend() function which decrements the usage_count
and calls rpm_suspend directly if the count drops to 0.  Then interrupt
handlers could use this function in place of pm_runtime_put_sync(),
provided the device was irq-safe.

Not only that, pm_runtime_put_sync_suspend() would be available for
anyone to use.  It must be reasonably common for runtime_idle routines
to do nothing but call pm_runtime_suspend().  The new API call would
save a little overhead.

Alan Stern

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Tue, 23 Nov 2010, Kevin Hilman wrote:

> While I like the idea of the symmetry of having both _get_sync() and
> _put_sync() callable from an interrupt handler, I can't currently think
> of a situation where we would need to _put_sync() in the ISR.  A
> standard _put() should suffice for all cases I can imagine.

It's wasteful to go through the context switch to the workqueue process
if you don't need to.  And it's time consuming; you want to power down 
the device as soon as possible once the interrupt handler is finished, 
right?

What do you think of the pm_runtime_put_sync_suspend() proposal?

Alan Stern

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Kevin Hilman]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Tue, 23 Nov 2010, Kevin Hilman wrote:
>
>> While I like the idea of the symmetry of having both _get_sync() and
>> _put_sync() callable from an interrupt handler, I can't currently think
>> of a situation where we would need to _put_sync() in the ISR.  A
>> standard _put() should suffice for all cases I can imagine.
>
> It's wasteful to go through the context switch to the workqueue process
> if you don't need to.  And it's time consuming; you want to power down 
> the device as soon as possible once the interrupt handler is finished, 
> right?
>
> What do you think of the pm_runtime_put_sync_suspend() proposal?

That should be fine.

Thinking of this some more, I don't have any use cases currently where
we would any sort of put in the ISR, since the ISR is usually an
indicator that something else will be accessing the device shortly after
the ISR is finished.

Kevin

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Rafael J. Wysocki]

On Wednesday, November 24, 2010, Alan Stern wrote:
> On Tue, 23 Nov 2010, Rafael J. Wysocki wrote:
> 
> > > Or maybe you think that when pm_runtime_put_sync detects the 
> > > usage_count has decremented to 0 and the device is irq-safe, it should 
> > > call rpm_suspend directly instead of calling rpm_idle?
> > 
> > That also would work for me, actually.
> 
> Okay, then consider this proposal.  I'll introduce a new
> pm_runtime_put_sync_suspend() function which decrements the usage_count
> and calls rpm_suspend directly if the count drops to 0.  Then interrupt
> handlers could use this function in place of pm_runtime_put_sync(),
> provided the device was irq-safe.
> 
> Not only that, pm_runtime_put_sync_suspend() would be available for
> anyone to use.  It must be reasonably common for runtime_idle routines
> to do nothing but call pm_runtime_suspend().  The new API call would
> save a little overhead.

Fine by me.

Thanks,
Rafael

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Sylwester Nawrocki]

Hello,

On 11/19/2010 04:45 PM, Alan Stern wrote:
> This patch (as1431b) makes the synchronous runtime-PM interface
> suitable for use in interrupt handlers.  Subsystems can call the new
> pm_runtime_irq_safe() function to tell the PM core that a device's
> runtime-PM callbacks should be invoked with interrupts disabled
> (runtime_suspend and runtime_resume callbacks will be invoked with the
> spinlock held as well).  This permits the pm_runtime_get_sync() and
> pm_runtime_put_sync() routines to be called from within interrupt
> handlers.
> 
> When a device is declared irq-safe in this way, the PM core increments
> the parent's usage count, so the parent will never be runtime
> suspended.  This prevents difficult situations in which an irq-safe
> device can't resume because it is forced to wait for its non-irq-safe
> parent.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> ---
...
>  
> +  void pm_runtime_irq_safe(struct device *dev);
> +    - set the power.irq_safe flag for the device, causing the runtime-PM
> +      callbacks to be invoked with interrupts disabled
> +
>    void pm_runtime_mark_last_busy(struct device *dev);
>      - set the power.last_busy field to the current time
>  
> @@ -438,6 +454,16 @@ pm_runtime_suspended()
>  pm_runtime_mark_last_busy()
>  pm_runtime_autosuspend_expiration()
>  
> +If pm_runtime_irq_safe() has been called for a device then the following helper
> +functions may also be called in interrupt context:

I was wondering what is the proper usage of this API. From a point of view of
a driver, does it mean that in runtime_resume/runtime_suspend helpers any
blocking calls cannot be used, so the device driver is prepared for situations
when some other subsystem invokes pm_runtime_irq_safe() on its device?

Or is pm_runtime_irq_safe() intended to be called only by the device driver
in such case?

I'd like to use blocking calls for a voltage regulator control within
the runtime PM helpers in the driver but I'm not sure whether this wouldn't
violate the API.

Thanks,
Sylwester

> +
> +pm_runtime_idle()
> +pm_runtime_suspend()
> +pm_runtime_autosuspend()
> +pm_runtime_resume()
> +pm_runtime_get_sync()
> +pm_runtime_put_sync()
> +
>  5. Run-time PM Initialization, Device Probing and Removal
>  
>  Initially, the run-time PM is disabled for all devices, which means that the
> 
-- 
Sylwester Nawrocki
Samsung Poland R&D Center

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Mon, 11 Apr 2011, Sylwester Nawrocki wrote:

> > +  void pm_runtime_irq_safe(struct device *dev);
> > +    - set the power.irq_safe flag for the device, causing the runtime-PM
> > +      callbacks to be invoked with interrupts disabled
> > +
> >    void pm_runtime_mark_last_busy(struct device *dev);
> >      - set the power.last_busy field to the current time
> >  
> > @@ -438,6 +454,16 @@ pm_runtime_suspended()
> >  pm_runtime_mark_last_busy()
> >  pm_runtime_autosuspend_expiration()
> >  
> > +If pm_runtime_irq_safe() has been called for a device then the following helper
> > +functions may also be called in interrupt context:
> 
> I was wondering what is the proper usage of this API. From a point of view of
> a driver, does it mean that in runtime_resume/runtime_suspend helpers any
> blocking calls cannot be used, so the device driver is prepared for situations
> when some other subsystem invokes pm_runtime_irq_safe() on its device?

I should have mentioned this in the documentation.  Yes, if 
pm_runtime_irq_safe() has been called for a device then that device's 
runtime_resume and runtime_suspend helpers must be able to run in 
interrupt context.  Hence they must not make any blocking calls.

However, this doesn't mean _all_ runtime_resume/runtime_suspend methods 
have to be IRQ-safe.  Only those for which pm_runtime_irq_safe() has 
been called.

A driver shouldn't worry about some other subsystem calling
pm_runtime_irq_safe() for one of its devices.  If that ever happened,
it would be a gross violation of proper layering.

> Or is pm_runtime_irq_safe() intended to be called only by the device driver
> in such case?

Yes, that's right.

> I'd like to use blocking calls for a voltage regulator control within
> the runtime PM helpers in the driver but I'm not sure whether this wouldn't
> violate the API.

You should be okay.  Just bear in mind that it means the voltage 
regulator's parent won't be able to runtime suspend.  If the regulator 
is a platform device with no meaningful parent then of course this 
won't matter.

Alan Stern

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Sylwester Nawrocki]

On 04/11/2011 06:08 PM, Alan Stern wrote:
> On Mon, 11 Apr 2011, Sylwester Nawrocki wrote:
> 
>>> +  void pm_runtime_irq_safe(struct device *dev);
>>> +    - set the power.irq_safe flag for the device, causing the runtime-PM
>>> +      callbacks to be invoked with interrupts disabled
>>> +
>>>    void pm_runtime_mark_last_busy(struct device *dev);
>>>      - set the power.last_busy field to the current time
>>>  
>>> @@ -438,6 +454,16 @@ pm_runtime_suspended()
>>>  pm_runtime_mark_last_busy()
>>>  pm_runtime_autosuspend_expiration()
>>>  
>>> +If pm_runtime_irq_safe() has been called for a device then the following helper
>>> +functions may also be called in interrupt context:
>>
>> I was wondering what is the proper usage of this API. From a point of view of
>> a driver, does it mean that in runtime_resume/runtime_suspend helpers any
>> blocking calls cannot be used, so the device driver is prepared for situations
>> when some other subsystem invokes pm_runtime_irq_safe() on its device?
> 
> I should have mentioned this in the documentation.  Yes, if 
> pm_runtime_irq_safe() has been called for a device then that device's 
> runtime_resume and runtime_suspend helpers must be able to run in 
> interrupt context.  Hence they must not make any blocking calls.
> 
> However, this doesn't mean _all_ runtime_resume/runtime_suspend methods 
> have to be IRQ-safe.  Only those for which pm_runtime_irq_safe() has 
> been called.

Thank you for the clarification.

OK, my main concerns was, who decides whether the specific runtime PM
helpers are IRQ-safe or not. But indeed, it now makes a little sense
to me to impose such a requirement on all runtime_suspend/resume helpers.

> 
> A driver shouldn't worry about some other subsystem calling
> pm_runtime_irq_safe() for one of its devices.  If that ever happened,
> it would be a gross violation of proper layering.

Ok, that's good news.

> 
>> Or is pm_runtime_irq_safe() intended to be called only by the device driver
>> in such case?
> 
> Yes, that's right.
> 
>> I'd like to use blocking calls for a voltage regulator control within
>> the runtime PM helpers in the driver but I'm not sure whether this wouldn't
>> violate the API.
> 
> You should be okay.  Just bear in mind that it means the voltage 
> regulator's parent won't be able to runtime suspend.  If the regulator 
> is a platform device with no meaningful parent then of course this 
> won't matter.

The regulator driver is a PMIC that uses I2C communication to control
the voltage regulators. So to be able to control the regulator supplying
the device, the runtime_resume/suspend callbacks need to be called with 
interrupts enabled. Otherwise the I2C communication wouldn't work.

I don't really need runtime_suspend/resume to be IRQ-safe, just wanted 
to make sure that in some conditions some other subsystem does not request that.

As I have seen there is no runtime PM call to clear the power.irq_safe
flags once it is set, so it looked like pm_runtime_irq_safe() is a basically 
a "one-time" call.


Regards,
-- 
Sylwester Nawrocki
Samsung Poland R&D Center

Re: [PATCH ver. 2] PM: add synchronous runtime interface for interrupt handlers

[Alan Stern]

On Mon, 11 Apr 2011, Sylwester Nawrocki wrote:

> The regulator driver is a PMIC that uses I2C communication to control
> the voltage regulators. So to be able to control the regulator supplying
> the device, the runtime_resume/suspend callbacks need to be called with 
> interrupts enabled. Otherwise the I2C communication wouldn't work.

Right.  Which obviously means that its callbacks can't be IRQ-safe.

> I don't really need runtime_suspend/resume to be IRQ-safe, just wanted 
> to make sure that in some conditions some other subsystem does not request that.

If it does happen, you can track down the person responsible for 
writing that function call, and complain loudly!  :-)

> As I have seen there is no runtime PM call to clear the power.irq_safe
> flags once it is set, so it looked like pm_runtime_irq_safe() is a basically 
> a "one-time" call.

Yes.  The use cases I could think of were all for platform devices 
where there would never be any reason to clear the flag.

Alan Stern