| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <87wrcstd5o.fsf@ti.com>
Date: Wed, 28 Sep 2011 17:17:39 -0700
From: Kevin Hilman <khilman@...com>
To: "Rafael J. Wysocki" <rjw@...k.pl>
Cc: Linux PM mailing list <linux-pm@...ts.linux-foundation.org>,
LKML <linux-kernel@...r.kernel.org>,
"Linux-sh list" <linux-sh@...r.kernel.org>,
Magnus Damm <magnus.damm@...il.com>, jean.pihet@...oldbits.com,
Ming Lei <tom.leiming@...il.com>
Subject: Re: [PATCH 2/3] PM / Runtime: Don't run callbacks under lock for power.irq_safe set
"Rafael J. Wysocki" <rjw@...k.pl> writes:
> On Tuesday, September 27, 2011, Rafael J. Wysocki wrote:
>> On Tuesday, September 27, 2011, Kevin Hilman wrote:
>> > "Rafael J. Wysocki" <rjw@...k.pl> writes:
>> >
>> > > From: Rafael J. Wysocki <rjw@...k.pl>
>> > >
>> > > The rpm_suspend() and rpm_resume() routines execute subsystem or PM
>> > > domain callbacks under power.lock if power.irq_safe is set for the
>> > > given device. This is inconsistent with that rpm_idle() does after
>> > > commit 02b2677 (PM / Runtime: Allow _put_sync() from
>> > > interrupts-disabled context) and is problematic for subsystems and PM
>> > > domains wanting to use power.lock for synchronization in their
>> > > runtime PM callbacks.
>> > >
>> > > Signed-off-by: Rafael J. Wysocki <rjw@...k.pl>
>> >
>> > The part described here looks right, and is much better for consistency.
>> >
>> > Reviewed-by: Kevin Hilman <khilman@...com>
>> >
>> > but...
>> >
>> > [...]
>> >
>> > > @@ -347,6 +353,15 @@ static int rpm_suspend(struct device *de
>> > > goto out;
>> > > }
>> > >
>> > > + if (dev->power.irq_safe) {
>> > > + spin_unlock(&dev->power.lock);
>> > > +
>> > > + cpu_relax();
>> > > +
>> > > + spin_lock(&dev->power.lock);
>> > > + goto repeat;
>> > > + }
>> > > +
>> >
>> >
>> > ... AFAICT, this isn't directly related to the problem described in the
>> > changelog (or at least I didn't find it obvious),
>>
>> It is related. Whether or not it's obvious, I'm not sure. :-)
>>
>> The problem is that after the changes in __rpm_callback() another CPU may start
>> executing the same routine for the same device if dev->power.irq_safe is set
>> (previously, it would block on the dev's power.lock) and it may see
>> dev->power.runtime_status == RPM_RESUMING or
>> dev->power.runtime_status == RPM_SUSPENDING, while previously, it wouldn't
>> reach the relevant code. Thus we have to modify that code to take
>> the dev->power.irq_safe case into account.
>>
>> > and probably deserves a comment in the code as well.
>>
>> Well, the comment in the code would explain why the commit did what it did,
>> but it wouldn't be very useful afterwards IMHO.
>>
>> Perhaps I'll simply add some explanation to the changelog.
>
> Below is the patch with the new changelog, for completness.
Thanks, it's much clearer now.
Kevin
> ---
> From: Rafael J. Wysocki <rjw@...k.pl>
> Subject: PM / Runtime: Don't run callbacks under lock for power.irq_safe set
>
> The rpm_suspend() and rpm_resume() routines execute subsystem or PM
> domain callbacks under power.lock if power.irq_safe is set for the
> given device. This is inconsistent with that rpm_idle() does after
> commit 02b2677 (PM / Runtime: Allow _put_sync() from
> interrupts-disabled context) and is problematic for subsystems and PM
> domains wanting to use power.lock for synchronization in their
> runtime PM callbacks.
>
> This change requires the code checking if the device's runtime PM
> status is RPM_SUSPENDING or RPM_RESUMING to be modified too, to take
> the power.irq_safe set case into account (that code wasn't reachable
> before with power.irq_safe set, because it's executed with the
> device's power.lock held).
>
> Signed-off-by: Rafael J. Wysocki <rjw@...k.pl>
> Reviewed-by: Ming Lei <tom.leiming@...il.com>
> Reviewed-by: Kevin Hilman <khilman@...com>
> ---
> drivers/base/power/runtime.c | 68 +++++++++++++++++++++++++++++--------------
> 1 file changed, 46 insertions(+), 22 deletions(-)
>
> Index: linux/drivers/base/power/runtime.c
> ===================================================================
> --- linux.orig/drivers/base/power/runtime.c
> +++ linux/drivers/base/power/runtime.c
> @@ -155,6 +155,31 @@ static int rpm_check_suspend_allowed(str
> }
>
> /**
> + * __rpm_callback - Run a given runtime PM callback for a given device.
> + * @cb: Runtime PM callback to run.
> + * @dev: Device to run the callback for.
> + */
> +static int __rpm_callback(int (*cb)(struct device *), struct device *dev)
> + __releases(&dev->power.lock) __acquires(&dev->power.lock)
> +{
> + int retval;
> +
> + if (dev->power.irq_safe)
> + spin_unlock(&dev->power.lock);
> + else
> + spin_unlock_irq(&dev->power.lock);
> +
> + retval = cb(dev);
> +
> + if (dev->power.irq_safe)
> + spin_lock(&dev->power.lock);
> + else
> + spin_lock_irq(&dev->power.lock);
> +
> + return retval;
> +}
> +
> +/**
> * rpm_idle - Notify device bus type if the device can be suspended.
> * @dev: Device to notify the bus type about.
> * @rpmflags: Flag bits.
> @@ -225,19 +250,8 @@ static int rpm_idle(struct device *dev,
> else
> callback = NULL;
>
> - if (callback) {
> - if (dev->power.irq_safe)
> - spin_unlock(&dev->power.lock);
> - else
> - spin_unlock_irq(&dev->power.lock);
> -
> - callback(dev);
> -
> - if (dev->power.irq_safe)
> - spin_lock(&dev->power.lock);
> - else
> - spin_lock_irq(&dev->power.lock);
> - }
> + if (callback)
> + __rpm_callback(callback, dev);
>
> dev->power.idle_notification = false;
> wake_up_all(&dev->power.wait_queue);
> @@ -252,22 +266,14 @@ static int rpm_idle(struct device *dev,
> * @dev: Device to run the callback for.
> */
> static int rpm_callback(int (*cb)(struct device *), struct device *dev)
> - __releases(&dev->power.lock) __acquires(&dev->power.lock)
> {
> int retval;
>
> if (!cb)
> return -ENOSYS;
>
> - if (dev->power.irq_safe) {
> - retval = cb(dev);
> - } else {
> - spin_unlock_irq(&dev->power.lock);
> -
> - retval = cb(dev);
> + retval = __rpm_callback(cb, dev);
>
> - spin_lock_irq(&dev->power.lock);
> - }
> dev->power.runtime_error = retval;
> return retval != -EACCES ? retval : -EIO;
> }
> @@ -347,6 +353,15 @@ static int rpm_suspend(struct device *de
> goto out;
> }
>
> + if (dev->power.irq_safe) {
> + spin_unlock(&dev->power.lock);
> +
> + cpu_relax();
> +
> + spin_lock(&dev->power.lock);
> + goto repeat;
> + }
> +
> /* Wait for the other suspend running in parallel with us. */
> for (;;) {
> prepare_to_wait(&dev->power.wait_queue, &wait,
> @@ -496,6 +511,15 @@ static int rpm_resume(struct device *dev
> goto out;
> }
>
> + if (dev->power.irq_safe) {
> + spin_unlock(&dev->power.lock);
> +
> + cpu_relax();
> +
> + spin_lock(&dev->power.lock);
> + goto repeat;
> + }
> +
> /* Wait for the operation carried out in parallel with us. */
> for (;;) {
> prepare_to_wait(&dev->power.wait_queue, &wait,
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists