From: Alan Stern <stern@rowland.harvard.edu> To: "Rafael J. Wysocki" <rjw@sisk.pl> Cc: linux-sh@vger.kernel.org, Linux PM mailing list <linux-pm@lists.linux-foundation.org>, LKML <linux-kernel@vger.kernel.org> Subject: Re: [linux-pm] [PATCH 1/2] PM / Runtime: Introduce pm_runtime_irq_unsafe() Date: Sun, 21 Aug 2011 14:55:37 +0000 [thread overview] Message-ID: <Pine.LNX.4.44L0.1108211048060.12143-100000@netrider.rowland.org> (raw) In-Reply-To: <201108202132.13304.rjw@sisk.pl> In-Reply-To: <201108202132.13304.rjw@sisk.pl> On Sat, 20 Aug 2011, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rjw@sisk.pl> > > Add a helper function allowing drivers and subsystems to clear > the power.irq_safe device flag. > --- linux.orig/drivers/base/power/runtime.c > +++ linux/drivers/base/power/runtime.c > @@ -1109,22 +1109,23 @@ void pm_runtime_no_callbacks(struct devi > EXPORT_SYMBOL_GPL(pm_runtime_no_callbacks); > > /** > - * pm_runtime_irq_safe - Leave interrupts disabled during callbacks. > + * __pm_runtime_irq_safe - Manipulate a device's power.irq_safe flag. > * @dev: Device to handle > + * @irq_safe: Whether or not to leave interrupts disabled during callbacks. > * > - * Set the power.irq_safe flag, which tells the PM core that the > + * Set or unset 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 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) > +void __pm_runtime_irq_safe(struct device *dev, bool irq_safe) > { > if (dev->parent) > pm_runtime_get_sync(dev->parent); > spin_lock_irq(&dev->power.lock); > - dev->power.irq_safe = 1; > + dev->power.irq_safe = irq_safe; > spin_unlock_irq(&dev->power.lock); It's not quite this easy. There are two important aspects that must be considered. Firstly, I originally envisioned pm_runtime_irq_safe() being called just once, before the device is enabled for runtime PM. If you allow the flag to be turned on and off like this, you raise the possibility of races with runtime PM callbacks. (That is, if a callback occurs at about the same time as the irq_safe flag is changed, nobody can predict whether the callback will be invoked with interrupts enabled.) Maybe that's something the driver needs to take care of, but it should at least be mentioned in the documentation. Secondly, this doesn't manage the parent's usage counter correctly. Do the pm_runtime_get_sync(dev->parent) at the beginning only when the irq_safe flag was off and is being turned on. And at the end, if the irq_safe flag was on and is being turned off, do pm_runtime_put_sync(dev->parent). See pm_runtime_remove() for why this matters. (Also update the documentation; the change to the parent isn't necessarily permanent any more.) Alan Stern
WARNING: multiple messages have this Message-ID (diff)
From: Alan Stern <stern@rowland.harvard.edu> To: "Rafael J. Wysocki" <rjw@sisk.pl> Cc: linux-sh@vger.kernel.org, Linux PM mailing list <linux-pm@lists.linux-foundation.org>, LKML <linux-kernel@vger.kernel.org> Subject: Re: [linux-pm] [PATCH 1/2] PM / Runtime: Introduce pm_runtime_irq_unsafe() Date: Sun, 21 Aug 2011 10:55:37 -0400 (EDT) [thread overview] Message-ID: <Pine.LNX.4.44L0.1108211048060.12143-100000@netrider.rowland.org> (raw) In-Reply-To: <201108202132.13304.rjw@sisk.pl> On Sat, 20 Aug 2011, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rjw@sisk.pl> > > Add a helper function allowing drivers and subsystems to clear > the power.irq_safe device flag. > --- linux.orig/drivers/base/power/runtime.c > +++ linux/drivers/base/power/runtime.c > @@ -1109,22 +1109,23 @@ void pm_runtime_no_callbacks(struct devi > EXPORT_SYMBOL_GPL(pm_runtime_no_callbacks); > > /** > - * pm_runtime_irq_safe - Leave interrupts disabled during callbacks. > + * __pm_runtime_irq_safe - Manipulate a device's power.irq_safe flag. > * @dev: Device to handle > + * @irq_safe: Whether or not to leave interrupts disabled during callbacks. > * > - * Set the power.irq_safe flag, which tells the PM core that the > + * Set or unset 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 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) > +void __pm_runtime_irq_safe(struct device *dev, bool irq_safe) > { > if (dev->parent) > pm_runtime_get_sync(dev->parent); > spin_lock_irq(&dev->power.lock); > - dev->power.irq_safe = 1; > + dev->power.irq_safe = irq_safe; > spin_unlock_irq(&dev->power.lock); It's not quite this easy. There are two important aspects that must be considered. Firstly, I originally envisioned pm_runtime_irq_safe() being called just once, before the device is enabled for runtime PM. If you allow the flag to be turned on and off like this, you raise the possibility of races with runtime PM callbacks. (That is, if a callback occurs at about the same time as the irq_safe flag is changed, nobody can predict whether the callback will be invoked with interrupts enabled.) Maybe that's something the driver needs to take care of, but it should at least be mentioned in the documentation. Secondly, this doesn't manage the parent's usage counter correctly. Do the pm_runtime_get_sync(dev->parent) at the beginning only when the irq_safe flag was off and is being turned on. And at the end, if the irq_safe flag was on and is being turned off, do pm_runtime_put_sync(dev->parent). See pm_runtime_remove() for why this matters. (Also update the documentation; the change to the parent isn't necessarily permanent any more.) Alan Stern
next prev parent reply other threads:[~2011-08-21 14:55 UTC|newest] Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-08-20 19:31 [PATCH 0/2] sh-sci / PM: Fix problem with runtime PM callbacks run with interrupts off Rafael J. Wysocki 2011-08-20 19:31 ` Rafael J. Wysocki 2011-08-20 19:32 ` [PATCH 1/2] PM / Runtime: Introduce pm_runtime_irq_unsafe() Rafael J. Wysocki 2011-08-20 19:32 ` Rafael J. Wysocki 2011-08-21 14:55 ` Alan Stern [this message] 2011-08-21 14:55 ` [linux-pm] " Alan Stern 2011-08-21 18:09 ` Rafael J. Wysocki 2011-08-21 18:09 ` [linux-pm] " Rafael J. Wysocki 2011-08-21 18:09 ` Rafael J. Wysocki 2011-08-21 14:55 ` Alan Stern 2011-08-20 19:32 ` Rafael J. Wysocki 2011-08-20 19:33 ` [PATCH 2/2] sh-sci / PM: Use power.irq_safe Rafael J. Wysocki 2011-08-20 19:33 ` Rafael J. Wysocki 2011-08-20 19:33 ` Rafael J. Wysocki 2011-08-21 19:09 ` [PATCH 0/2 v2] sh-sci / PM: Fix problem with runtime PM callbacks run with interrupts off Rafael J. Wysocki 2011-08-21 19:09 ` Rafael J. Wysocki 2011-08-21 19:09 ` Rafael J. Wysocki 2011-08-21 19:10 ` [PATCH 1/2 v2] PM: Change PM subsys_data lock type into spinlock Rafael J. Wysocki 2011-08-21 19:10 ` Rafael J. Wysocki 2011-08-22 6:18 ` [Replacement][PATCH 1/2 v2] PM: Use spinlock instead of mutex in clock management functions Rafael J. Wysocki 2011-08-22 6:18 ` Rafael J. Wysocki 2011-08-22 6:18 ` Rafael J. Wysocki 2011-08-21 19:10 ` [PATCH 1/2 v2] PM: Change PM subsys_data lock type into spinlock Rafael J. Wysocki 2011-08-21 19:11 ` [PATCH 2/2 v2] sh-sci / PM: Use power.irq_safe Rafael J. Wysocki 2011-08-21 19:11 ` Rafael J. Wysocki 2011-08-24 5:33 ` Paul Mundt 2011-08-24 5:33 ` Paul Mundt 2011-08-24 20:52 ` Rafael J. Wysocki 2011-08-24 20:52 ` Rafael J. Wysocki 2011-08-25 1:33 ` Paul Mundt 2011-08-25 1:33 ` Paul Mundt 2011-08-25 1:33 ` Paul Mundt 2011-08-24 20:52 ` Rafael J. Wysocki 2011-08-24 5:33 ` Paul Mundt 2011-08-21 19:11 ` Rafael J. Wysocki
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=Pine.LNX.4.44L0.1108211048060.12143-100000@netrider.rowland.org \ --to=stern@rowland.harvard.edu \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@lists.linux-foundation.org \ --cc=linux-sh@vger.kernel.org \ --cc=rjw@sisk.pl \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.