[PATCH] i2c: OMAP: fix static suspend vs. runtime suspend

[PATCH] i2c: OMAP: fix static suspend vs. runtime suspend

[Kevin Hilman]

When runtime PM is enabled, each OMAP i2c device is suspended after
each i2c xfer.  However, there are two cases when the static suspend
methods must be used to ensure the devices are suspended:

1) runtime PM is disabled, either at compile time or dynamically
    via /sys/devices/.../power/control.
2) an i2c client driver uses i2c during it's suspend callback, thus
   leaving the i2c driver active (NOTE: runtime suspend transitions are
   disabled during system suspend, so i2c activity during system
   suspend will runtime resume the device, but not runtime (re)suspend it.)

Since the actual work to suspend the device is handled by the
subsytem, call the bus methods to take care of it.

NOTE: This takes care of a known suspend problem on OMAP3 where the
TWL RTC driver does i2c xfers during its suspend path leaving the i2c
driver in an active state (since runtime suspend transistions are
disabled.)

Signed-off-by: Kevin Hilman <khilman@ti.com>
---
Ben, this is a regression in 2.6.38 so hopefully this can be queued
in the 2.6.38-rc cycle.  Thanks.

 drivers/i2c/busses/i2c-omap.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index b605ff3..0541df9 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1137,12 +1137,40 @@ omap_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_SUSPEND
+static int omap_i2c_suspend(struct device *dev)
+{
+	if (!pm_runtime_suspended(dev))
+		if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend)
+			dev->bus->pm->runtime_suspend(dev);
+
+	return 0;
+}
+
+static int omap_i2c_resume(struct device *dev)
+{
+	if (!pm_runtime_suspended(dev))
+		if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume)
+			dev->bus->pm->runtime_resume(dev);
+
+	return 0;
+}
+
+static struct dev_pm_ops omap_i2c_pm_ops = {
+	.suspend = omap_i2c_suspend,
+	.resume = omap_i2c_resume,
+};
+#else
+#define omap_i2c_pm_ops NULL
+#endif
+
 static struct platform_driver omap_i2c_driver = {
 	.probe		= omap_i2c_probe,
 	.remove		= omap_i2c_remove,
 	.driver		= {
 		.name	= "omap_i2c",
 		.owner	= THIS_MODULE,
+		.pm     = &omap_i2c_pm_ops,
 	},
 };
 
-- 
1.7.3.5

[PATCH] i2c: OMAP: fix static suspend vs. runtime suspend

[Rajendra Nayak]

Hi Kevin,

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman at ti.com]
> Sent: Friday, January 28, 2011 5:49 AM
> To: Ben Dooks; Rajendra Nayak; linux-i2c at vger.kernel.org
> Cc: linux-omap at vger.kernel.org; linux-pm at lists.linux-foundation.org;
linux-arm-kernel at lists.infradead.org
> Subject: [PATCH] i2c: OMAP: fix static suspend vs. runtime suspend
>
> When runtime PM is enabled, each OMAP i2c device is suspended after
> each i2c xfer.  However, there are two cases when the static suspend
> methods must be used to ensure the devices are suspended:
>
> 1) runtime PM is disabled, either at compile time or dynamically
>     via /sys/devices/.../power/control.
> 2) an i2c client driver uses i2c during it's suspend callback, thus
>    leaving the i2c driver active (NOTE: runtime suspend transitions are
>    disabled during system suspend, so i2c activity during system
>    suspend will runtime resume the device, but not runtime (re)suspend
it.)
>
> Since the actual work to suspend the device is handled by the
> subsytem, call the bus methods to take care of it.

The patch looks good to me. Thanks for the fix.
Validated suspend on OMAP4sdp with the patch.
Can you elaborate a bit more on how/why runtime PM transitions
are disabled during system suspend, and how is it taken care
of that a runtime resume of a device works however a subsequent
runtime (re)suspend does not?

Regards,
Rajendra

>
> NOTE: This takes care of a known suspend problem on OMAP3 where the
> TWL RTC driver does i2c xfers during its suspend path leaving the i2c
> driver in an active state (since runtime suspend transistions are
> disabled.)
>
> Signed-off-by: Kevin Hilman <khilman@ti.com>
> ---
> Ben, this is a regression in 2.6.38 so hopefully this can be queued
> in the 2.6.38-rc cycle.  Thanks.
>
>  drivers/i2c/busses/i2c-omap.c |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c
b/drivers/i2c/busses/i2c-omap.c
> index b605ff3..0541df9 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -1137,12 +1137,40 @@ omap_i2c_remove(struct platform_device *pdev)
>  	return 0;
>  }
>
> +#ifdef CONFIG_SUSPEND
> +static int omap_i2c_suspend(struct device *dev)
> +{
> +	if (!pm_runtime_suspended(dev))
> +		if (dev->bus && dev->bus->pm &&
dev->bus->pm->runtime_suspend)
> +			dev->bus->pm->runtime_suspend(dev);
> +
> +	return 0;
> +}
> +
> +static int omap_i2c_resume(struct device *dev)
> +{
> +	if (!pm_runtime_suspended(dev))
> +		if (dev->bus && dev->bus->pm &&
dev->bus->pm->runtime_resume)
> +			dev->bus->pm->runtime_resume(dev);
> +
> +	return 0;
> +}
> +
> +static struct dev_pm_ops omap_i2c_pm_ops = {
> +	.suspend = omap_i2c_suspend,
> +	.resume = omap_i2c_resume,
> +};
> +#else
> +#define omap_i2c_pm_ops NULL
> +#endif
> +
>  static struct platform_driver omap_i2c_driver = {
>  	.probe		= omap_i2c_probe,
>  	.remove		= omap_i2c_remove,
>  	.driver		= {
>  		.name	= "omap_i2c",
>  		.owner	= THIS_MODULE,
> +		.pm     = &omap_i2c_pm_ops,
>  	},
>  };
>
> --
> 1.7.3.5

[linux-pm] [PATCH] i2c: OMAP: fix static suspend vs. runtime suspend

[Alan Stern]

On Mon, 31 Jan 2011, Rajendra Nayak wrote:

> Can you elaborate a bit more on how/why runtime PM transitions
> are disabled during system suspend, and how is it taken care
> of that a runtime resume of a device works however a subsequent
> runtime (re)suspend does not?

I'll answer for Kevin.  This is done by the PM core, in order to 
prevent runtime power transitions from interfering with a system power 
transition.  The PM core increments the device's usage_count; this 
prevents the device from being runtime-suspended but it allows 
runtime-resume calls to go through.

Alan Stern

[linux-pm] [PATCH] i2c: OMAP: fix static suspend vs. runtime suspend

[Rajendra Nayak]

> -----Original Message-----
> From: Alan Stern [mailto:stern at rowland.harvard.edu]
> Sent: Monday, January 31, 2011 8:43 PM
> To: Rajendra Nayak
> Cc: Kevin Hilman; Ben Dooks; linux-i2c at vger.kernel.org;
linux-pm at lists.linux-foundation.org; linux-
> omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
> Subject: Re: [linux-pm] [PATCH] i2c: OMAP: fix static suspend vs.
runtime suspend
>
> On Mon, 31 Jan 2011, Rajendra Nayak wrote:
>
> > Can you elaborate a bit more on how/why runtime PM transitions
> > are disabled during system suspend, and how is it taken care
> > of that a runtime resume of a device works however a subsequent
> > runtime (re)suspend does not?
>
> I'll answer for Kevin.  This is done by the PM core, in order to
> prevent runtime power transitions from interfering with a system power
> transition.  The PM core increments the device's usage_count; this
> prevents the device from being runtime-suspended but it allows
> runtime-resume calls to go through.

Thanks, I did remember seeing the pm_runtime_get_noresume()
in dpm_prepare(). Just did not correlate it was the same Kevin
was trying to say.

Regards,
Rajendra

>
> Alan Stern

[linux-pm] [PATCH] i2c: OMAP: fix static suspend vs. runtime suspend

[Kevin Hilman]

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

> On Mon, 31 Jan 2011, Rajendra Nayak wrote:
>
>> Can you elaborate a bit more on how/why runtime PM transitions
>> are disabled during system suspend, and how is it taken care
>> of that a runtime resume of a device works however a subsequent
>> runtime (re)suspend does not?
>
> I'll answer for Kevin.  This is done by the PM core, in order to 
> prevent runtime power transitions from interfering with a system power 
> transition.  The PM core increments the device's usage_count; this 
> prevents the device from being runtime-suspended but it allows 
> runtime-resume calls to go through.

I understand how this works, but frankly I'm still a bit fuzzy on why.

I guess I'm still missing a good understanding of what "interfering with a
system power transition" means, and why a runtime suspend qualifies as
interfering but not a runtime resume.

More specifically, the reason for $SUBJECT patch is precisely because a
runtime resume is allowed, a runtime suspend is not, and thus a system
power transititon is prevented.

Kevin

[linux-pm] [PATCH] i2c: OMAP: fix static suspend vs. runtime suspend

[Alan Stern]

On Mon, 31 Jan 2011, Kevin Hilman wrote:

> I understand how this works, but frankly I'm still a bit fuzzy on why.
> 
> I guess I'm still missing a good understanding of what "interfering with a
> system power transition" means, and why a runtime suspend qualifies as
> interfering but not a runtime resume.

These are good questions.  Rafael implemented this design originally; 
my contribution was only to warn him of the potential for problems.  
Therefore he should explain the rationale for the design.

> More specifically, the reason for $SUBJECT patch is precisely because a
> runtime resume is allowed, a runtime suspend is not, and thus a system
> power transititon is prevented.

Alan Stern

[linux-pm] [PATCH] i2c: OMAP: fix static suspend vs. runtime suspend

[Rafael J. Wysocki]

On Monday, January 31, 2011, Alan Stern wrote:
> On Mon, 31 Jan 2011, Kevin Hilman wrote:
> 
> > I understand how this works, but frankly I'm still a bit fuzzy on why.
> > 
> > I guess I'm still missing a good understanding of what "interfering with a
> > system power transition" means, and why a runtime suspend qualifies as
> > interfering but not a runtime resume.
> 
> These are good questions.  Rafael implemented this design originally; 
> my contribution was only to warn him of the potential for problems.  
> Therefore he should explain the rationale for the design.

The reason why runtime resume is allowed during system power transitions is
because in some cases during system suspend we simply have to resume devices
that were previously runtime-suspended (for example, the PCI bus type does
that).

The reason why runtime suspend is not allowed during system power transitions
if the following race:

- A device has been suspended via a system suspend callback.
- The runtime PM framework executes a (scheduled) suspend on that device,
  not knowing that it's already been suspended, which potentially results in
  accessing the device's registers in a low-power state.

Now, it can be avoided if every driver does the right thing and checks whether
the device is already suspended in its runtime suspend callback, but that would
kind of defeat the purpose of the runtime PM framework, at least partially.

Thanks,
Rafael

[PATCH] PM: Allow pm_runtime_suspend() to succeed during system suspend

[Rafael J. Wysocki]

On Monday, January 31, 2011, Rafael J. Wysocki wrote:
> On Monday, January 31, 2011, Alan Stern wrote:
> > On Mon, 31 Jan 2011, Kevin Hilman wrote:
> > 
> > > I understand how this works, but frankly I'm still a bit fuzzy on why.
> > > 
> > > I guess I'm still missing a good understanding of what "interfering with a
> > > system power transition" means, and why a runtime suspend qualifies as
> > > interfering but not a runtime resume.
> > 
> > These are good questions.  Rafael implemented this design originally; 
> > my contribution was only to warn him of the potential for problems.  
> > Therefore he should explain the rationale for the design.
> 
> The reason why runtime resume is allowed during system power transitions is
> because in some cases during system suspend we simply have to resume devices
> that were previously runtime-suspended (for example, the PCI bus type does
> that).
> 
> The reason why runtime suspend is not allowed during system power transitions
> if the following race:
> 
> - A device has been suspended via a system suspend callback.
> - The runtime PM framework executes a (scheduled) suspend on that device,
>   not knowing that it's already been suspended, which potentially results in
>   accessing the device's registers in a low-power state.
> 
> Now, it can be avoided if every driver does the right thing and checks whether
> the device is already suspended in its runtime suspend callback, but that would
> kind of defeat the purpose of the runtime PM framework, at least partially.

In fact, I've just realized that the above race cannot really occur, because
pm_wq is freezable, so I'm proposing the following change.

Of course, it still doesn't prevent user space from disabling the runtime PM
framework's helpers via /sys/devices/.../power/control.

Thanks,
Rafael


---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM: Allow pm_runtime_suspend() to succeed during system suspend

The dpm_prepare() function increments the runtime PM reference
counters of all devices to prevent pm_runtime_suspend() from
executing subsystem-level callbacks.  However, this was supposed to
guard against a specific race condition that cannot happen, because
the power management workqueue is freezable, so pm_runtime_suspend()
can only be called synchronously during system suspend and we can
rely on subsystems and device drivers to avoid doing that
unnecessarily.

Make dpm_prepare() drop the runtime PM reference to each device
after making sure that runtime resume is not pending for it.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/main.c |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

Index: linux-2.6/drivers/base/power/main.c
===================================================================
--- linux-2.6.orig/drivers/base/power/main.c
+++ linux-2.6/drivers/base/power/main.c
@@ -669,7 +669,6 @@ static void dpm_complete(pm_message_t st
 		mutex_unlock(&dpm_list_mtx);
 
 		device_complete(dev, state);
-		pm_runtime_put_sync(dev);
 
 		mutex_lock(&dpm_list_mtx);
 		put_device(dev);
@@ -1005,12 +1004,9 @@ static int dpm_prepare(pm_message_t stat
 		if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
 			pm_wakeup_event(dev, 0);
 
-		if (pm_wakeup_pending()) {
-			pm_runtime_put_sync(dev);
-			error = -EBUSY;
-		} else {
-			error = device_prepare(dev, state);
-		}
+		pm_runtime_put_sync(dev);
+		error = pm_wakeup_pending() ?
+				-EBUSY : device_prepare(dev, state);
 
 		mutex_lock(&dpm_list_mtx);
 		if (error) {

[PATCH] PM: Allow pm_runtime_suspend() to succeed during system suspend

[Alan Stern]

On Fri, 11 Feb 2011, Rafael J. Wysocki wrote:

> > The reason why runtime suspend is not allowed during system power transitions
> > if the following race:
> > 
> > - A device has been suspended via a system suspend callback.
> > - The runtime PM framework executes a (scheduled) suspend on that device,
> >   not knowing that it's already been suspended, which potentially results in
> >   accessing the device's registers in a low-power state.
> > 
> > Now, it can be avoided if every driver does the right thing and checks whether
> > the device is already suspended in its runtime suspend callback, but that would
> > kind of defeat the purpose of the runtime PM framework, at least partially.
> 
> In fact, I've just realized that the above race cannot really occur, because
> pm_wq is freezable, so I'm proposing the following change.

Yes, I had reached essentially the same conclusion.  Of course, there 
may still be other kernel threads running or interrupt handlers that 
can interfere.  It's probably okay to assume that drivers will handle 
these things.

> Of course, it still doesn't prevent user space from disabling the runtime PM
> framework's helpers via /sys/devices/.../power/control.

True.  So in the end this won't make much difference, but we might as 
well do it.

Alan Stern

[PATCH] PM: Allow pm_runtime_suspend() to succeed during system suspend

[Kevin Hilman]

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

> On Monday, January 31, 2011, Rafael J. Wysocki wrote:
>> On Monday, January 31, 2011, Alan Stern wrote:
>> > On Mon, 31 Jan 2011, Kevin Hilman wrote:
>> > 
>> > > I understand how this works, but frankly I'm still a bit fuzzy on why.
>> > > 
>> > > I guess I'm still missing a good understanding of what "interfering with a
>> > > system power transition" means, and why a runtime suspend qualifies as
>> > > interfering but not a runtime resume.
>> > 
>> > These are good questions.  Rafael implemented this design originally; 
>> > my contribution was only to warn him of the potential for problems.  
>> > Therefore he should explain the rationale for the design.
>> 
>> The reason why runtime resume is allowed during system power transitions is
>> because in some cases during system suspend we simply have to resume devices
>> that were previously runtime-suspended (for example, the PCI bus type does
>> that).
>> 
>> The reason why runtime suspend is not allowed during system power transitions
>> if the following race:
>> 
>> - A device has been suspended via a system suspend callback.
>> - The runtime PM framework executes a (scheduled) suspend on that device,
>>   not knowing that it's already been suspended, which potentially results in
>>   accessing the device's registers in a low-power state.
>> 
>> Now, it can be avoided if every driver does the right thing and checks whether
>> the device is already suspended in its runtime suspend callback, but that would
>> kind of defeat the purpose of the runtime PM framework, at least partially.
>
> In fact, I've just realized that the above race cannot really occur, because
> pm_wq is freezable, so I'm proposing the following change.
>
> Of course, it still doesn't prevent user space from disabling the runtime PM
> framework's helpers via /sys/devices/.../power/control.
>
> Thanks,
> Rafael
>
>
> ---
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: PM: Allow pm_runtime_suspend() to succeed during system suspend
>
> The dpm_prepare() function increments the runtime PM reference
> counters of all devices to prevent pm_runtime_suspend() from
> executing subsystem-level callbacks.  However, this was supposed to
> guard against a specific race condition that cannot happen, because
> the power management workqueue is freezable, so pm_runtime_suspend()
> can only be called synchronously during system suspend and we can
> rely on subsystems and device drivers to avoid doing that
> unnecessarily.
>
> Make dpm_prepare() drop the runtime PM reference to each device
> after making sure that runtime resume is not pending for it.
>
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---

Yes!

Acked-by: Kevin Hilman <khilman@ti.com>


>  drivers/base/power/main.c |   10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> Index: linux-2.6/drivers/base/power/main.c
> ===================================================================
> --- linux-2.6.orig/drivers/base/power/main.c
> +++ linux-2.6/drivers/base/power/main.c
> @@ -669,7 +669,6 @@ static void dpm_complete(pm_message_t st
>  		mutex_unlock(&dpm_list_mtx);
>  
>  		device_complete(dev, state);
> -		pm_runtime_put_sync(dev);
>  
>  		mutex_lock(&dpm_list_mtx);
>  		put_device(dev);
> @@ -1005,12 +1004,9 @@ static int dpm_prepare(pm_message_t stat
>  		if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
>  			pm_wakeup_event(dev, 0);
>  
> -		if (pm_wakeup_pending()) {
> -			pm_runtime_put_sync(dev);
> -			error = -EBUSY;
> -		} else {
> -			error = device_prepare(dev, state);
> -		}
> +		pm_runtime_put_sync(dev);
> +		error = pm_wakeup_pending() ?
> +				-EBUSY : device_prepare(dev, state);
>  
>  		mutex_lock(&dpm_list_mtx);
>  		if (error) {
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] PM: Allow pm_runtime_suspend() to succeed during system suspend

[Rafael J. Wysocki]

On Friday, February 11, 2011, Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > On Monday, January 31, 2011, Rafael J. Wysocki wrote:
> >> On Monday, January 31, 2011, Alan Stern wrote:
> >> > On Mon, 31 Jan 2011, Kevin Hilman wrote:
> >> > 
> >> > > I understand how this works, but frankly I'm still a bit fuzzy on why.
> >> > > 
> >> > > I guess I'm still missing a good understanding of what "interfering with a
> >> > > system power transition" means, and why a runtime suspend qualifies as
> >> > > interfering but not a runtime resume.
> >> > 
> >> > These are good questions.  Rafael implemented this design originally; 
> >> > my contribution was only to warn him of the potential for problems.  
> >> > Therefore he should explain the rationale for the design.
> >> 
> >> The reason why runtime resume is allowed during system power transitions is
> >> because in some cases during system suspend we simply have to resume devices
> >> that were previously runtime-suspended (for example, the PCI bus type does
> >> that).
> >> 
> >> The reason why runtime suspend is not allowed during system power transitions
> >> if the following race:
> >> 
> >> - A device has been suspended via a system suspend callback.
> >> - The runtime PM framework executes a (scheduled) suspend on that device,
> >>   not knowing that it's already been suspended, which potentially results in
> >>   accessing the device's registers in a low-power state.
> >> 
> >> Now, it can be avoided if every driver does the right thing and checks whether
> >> the device is already suspended in its runtime suspend callback, but that would
> >> kind of defeat the purpose of the runtime PM framework, at least partially.
> >
> > In fact, I've just realized that the above race cannot really occur, because
> > pm_wq is freezable, so I'm proposing the following change.
> >
> > Of course, it still doesn't prevent user space from disabling the runtime PM
> > framework's helpers via /sys/devices/.../power/control.
> >
> > Thanks,
> > Rafael
> >
> >
> > ---
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > Subject: PM: Allow pm_runtime_suspend() to succeed during system suspend
> >
> > The dpm_prepare() function increments the runtime PM reference
> > counters of all devices to prevent pm_runtime_suspend() from
> > executing subsystem-level callbacks.  However, this was supposed to
> > guard against a specific race condition that cannot happen, because
> > the power management workqueue is freezable, so pm_runtime_suspend()
> > can only be called synchronously during system suspend and we can
> > rely on subsystems and device drivers to avoid doing that
> > unnecessarily.
> >
> > Make dpm_prepare() drop the runtime PM reference to each device
> > after making sure that runtime resume is not pending for it.
> >
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> 
> Yes!
> 
> Acked-by: Kevin Hilman <khilman@ti.com>

Well, I hope you realize that it doesn't help you a lot?

Rafael

[PATCH] PM: Allow pm_runtime_suspend() to succeed during system suspend

[Kevin Hilman]

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

> On Friday, February 11, 2011, Kevin Hilman wrote:
>> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
>> 
>> > On Monday, January 31, 2011, Rafael J. Wysocki wrote:
>> >> On Monday, January 31, 2011, Alan Stern wrote:
>> >> > On Mon, 31 Jan 2011, Kevin Hilman wrote:
>> >> > 
>> >> > > I understand how this works, but frankly I'm still a bit fuzzy on why.
>> >> > > 
>> >> > > I guess I'm still missing a good understanding of what "interfering with a
>> >> > > system power transition" means, and why a runtime suspend qualifies as
>> >> > > interfering but not a runtime resume.
>> >> > 
>> >> > These are good questions.  Rafael implemented this design originally; 
>> >> > my contribution was only to warn him of the potential for problems.  
>> >> > Therefore he should explain the rationale for the design.
>> >> 
>> >> The reason why runtime resume is allowed during system power transitions is
>> >> because in some cases during system suspend we simply have to resume devices
>> >> that were previously runtime-suspended (for example, the PCI bus type does
>> >> that).
>> >> 
>> >> The reason why runtime suspend is not allowed during system power transitions
>> >> if the following race:
>> >> 
>> >> - A device has been suspended via a system suspend callback.
>> >> - The runtime PM framework executes a (scheduled) suspend on that device,
>> >>   not knowing that it's already been suspended, which potentially results in
>> >>   accessing the device's registers in a low-power state.
>> >> 
>> >> Now, it can be avoided if every driver does the right thing and checks whether
>> >> the device is already suspended in its runtime suspend callback, but that would
>> >> kind of defeat the purpose of the runtime PM framework, at least partially.
>> >
>> > In fact, I've just realized that the above race cannot really occur, because
>> > pm_wq is freezable, so I'm proposing the following change.
>> >
>> > Of course, it still doesn't prevent user space from disabling the runtime PM
>> > framework's helpers via /sys/devices/.../power/control.
>> >
>> > Thanks,
>> > Rafael
>> >
>> >
>> > ---
>> > From: Rafael J. Wysocki <rjw@sisk.pl>
>> > Subject: PM: Allow pm_runtime_suspend() to succeed during system suspend
>> >
>> > The dpm_prepare() function increments the runtime PM reference
>> > counters of all devices to prevent pm_runtime_suspend() from
>> > executing subsystem-level callbacks.  However, this was supposed to
>> > guard against a specific race condition that cannot happen, because
>> > the power management workqueue is freezable, so pm_runtime_suspend()
>> > can only be called synchronously during system suspend and we can
>> > rely on subsystems and device drivers to avoid doing that
>> > unnecessarily.
>> >
>> > Make dpm_prepare() drop the runtime PM reference to each device
>> > after making sure that runtime resume is not pending for it.
>> >
>> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
>> > ---
>> 
>> Yes!
>> 
>> Acked-by: Kevin Hilman <khilman@ti.com>
>
> Well, I hope you realize that it doesn't help you a lot?
>

If you mean that because we still have to implement system PM methods
because of /sys/devices/.../power/control, I agree.

If something else, please explain.

But to me it is still very helpful in terms of consistency and what
driver writers would expect to happen if they used pm_runtime_suspend()
in their system suspend method.

Thanks,

Kevin

[PATCH] PM: Allow pm_runtime_suspend() to succeed during system suspend

[Rafael J. Wysocki]

On Saturday, February 12, 2011, Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> 
> > On Friday, February 11, 2011, Kevin Hilman wrote:
> >> "Rafael J. Wysocki" <rjw@sisk.pl> writes:
> >> 
> >> > On Monday, January 31, 2011, Rafael J. Wysocki wrote:
> >> >> On Monday, January 31, 2011, Alan Stern wrote:
> >> >> > On Mon, 31 Jan 2011, Kevin Hilman wrote:
> >> >> > 
> >> >> > > I understand how this works, but frankly I'm still a bit fuzzy on why.
> >> >> > > 
> >> >> > > I guess I'm still missing a good understanding of what "interfering with a
> >> >> > > system power transition" means, and why a runtime suspend qualifies as
> >> >> > > interfering but not a runtime resume.
> >> >> > 
> >> >> > These are good questions.  Rafael implemented this design originally; 
> >> >> > my contribution was only to warn him of the potential for problems.  
> >> >> > Therefore he should explain the rationale for the design.
> >> >> 
> >> >> The reason why runtime resume is allowed during system power transitions is
> >> >> because in some cases during system suspend we simply have to resume devices
> >> >> that were previously runtime-suspended (for example, the PCI bus type does
> >> >> that).
> >> >> 
> >> >> The reason why runtime suspend is not allowed during system power transitions
> >> >> if the following race:
> >> >> 
> >> >> - A device has been suspended via a system suspend callback.
> >> >> - The runtime PM framework executes a (scheduled) suspend on that device,
> >> >>   not knowing that it's already been suspended, which potentially results in
> >> >>   accessing the device's registers in a low-power state.
> >> >> 
> >> >> Now, it can be avoided if every driver does the right thing and checks whether
> >> >> the device is already suspended in its runtime suspend callback, but that would
> >> >> kind of defeat the purpose of the runtime PM framework, at least partially.
> >> >
> >> > In fact, I've just realized that the above race cannot really occur, because
> >> > pm_wq is freezable, so I'm proposing the following change.
> >> >
> >> > Of course, it still doesn't prevent user space from disabling the runtime PM
> >> > framework's helpers via /sys/devices/.../power/control.
> >> >
> >> > Thanks,
> >> > Rafael
> >> >
> >> >
> >> > ---
> >> > From: Rafael J. Wysocki <rjw@sisk.pl>
> >> > Subject: PM: Allow pm_runtime_suspend() to succeed during system suspend
> >> >
> >> > The dpm_prepare() function increments the runtime PM reference
> >> > counters of all devices to prevent pm_runtime_suspend() from
> >> > executing subsystem-level callbacks.  However, this was supposed to
> >> > guard against a specific race condition that cannot happen, because
> >> > the power management workqueue is freezable, so pm_runtime_suspend()
> >> > can only be called synchronously during system suspend and we can
> >> > rely on subsystems and device drivers to avoid doing that
> >> > unnecessarily.
> >> >
> >> > Make dpm_prepare() drop the runtime PM reference to each device
> >> > after making sure that runtime resume is not pending for it.
> >> >
> >> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> >> > ---
> >> 
> >> Yes!
> >> 
> >> Acked-by: Kevin Hilman <khilman@ti.com>
> >
> > Well, I hope you realize that it doesn't help you a lot?
> >
> 
> If you mean that because we still have to implement system PM methods
> because of /sys/devices/.../power/control, I agree.

Yes, I meant that.
 
> If something else, please explain.
>
> But to me it is still very helpful in terms of consistency and what
> driver writers would expect to happen if they used pm_runtime_suspend()
> in their system suspend method.

OK

Thanks,
Rafael

[PATCH] i2c: OMAP: fix static suspend vs. runtime suspend

[Ben Dooks]

On Thu, Jan 27, 2011 at 04:18:41PM -0800, Kevin Hilman wrote:
> When runtime PM is enabled, each OMAP i2c device is suspended after
> each i2c xfer.  However, there are two cases when the static suspend
> methods must be used to ensure the devices are suspended:
> 
> 1) runtime PM is disabled, either at compile time or dynamically
>     via /sys/devices/.../power/control.
> 2) an i2c client driver uses i2c during it's suspend callback, thus
>    leaving the i2c driver active (NOTE: runtime suspend transitions are
>    disabled during system suspend, so i2c activity during system
>    suspend will runtime resume the device, but not runtime (re)suspend it.)
> 
> Since the actual work to suspend the device is handled by the
> subsytem, call the bus methods to take care of it.
> 
> NOTE: This takes care of a known suspend problem on OMAP3 where the
> TWL RTC driver does i2c xfers during its suspend path leaving the i2c
> driver in an active state (since runtime suspend transistions are
> disabled.)
> 
> Signed-off-by: Kevin Hilman <khilman@ti.com>
> ---
> Ben, this is a regression in 2.6.38 so hopefully this can be queued
> in the 2.6.38-rc cycle.  Thanks.

Ok, after all the discussions should I keep it in my -rc queue?
 
>  drivers/i2c/busses/i2c-omap.c |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index b605ff3..0541df9 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -1137,12 +1137,40 @@ omap_i2c_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_SUSPEND
> +static int omap_i2c_suspend(struct device *dev)
> +{
> +	if (!pm_runtime_suspended(dev))
> +		if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_suspend)
> +			dev->bus->pm->runtime_suspend(dev);
> +
> +	return 0;
> +}
> +
> +static int omap_i2c_resume(struct device *dev)
> +{
> +	if (!pm_runtime_suspended(dev))
> +		if (dev->bus && dev->bus->pm && dev->bus->pm->runtime_resume)
> +			dev->bus->pm->runtime_resume(dev);
> +
> +	return 0;
> +}
> +
> +static struct dev_pm_ops omap_i2c_pm_ops = {
> +	.suspend = omap_i2c_suspend,
> +	.resume = omap_i2c_resume,
> +};
> +#else
> +#define omap_i2c_pm_ops NULL
> +#endif
> +
>  static struct platform_driver omap_i2c_driver = {
>  	.probe		= omap_i2c_probe,
>  	.remove		= omap_i2c_remove,
>  	.driver		= {
>  		.name	= "omap_i2c",
>  		.owner	= THIS_MODULE,
> +		.pm     = &omap_i2c_pm_ops,
>  	},
>  };
>  
> -- 
> 1.7.3.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ben Dooks, ben at fluff.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

[PATCH] i2c: OMAP: fix static suspend vs. runtime suspend

[Kevin Hilman]

Ben Dooks <ben-i2c@fluff.org> writes:

> On Thu, Jan 27, 2011 at 04:18:41PM -0800, Kevin Hilman wrote:
>> When runtime PM is enabled, each OMAP i2c device is suspended after
>> each i2c xfer.  However, there are two cases when the static suspend
>> methods must be used to ensure the devices are suspended:
>> 
>> 1) runtime PM is disabled, either at compile time or dynamically
>>     via /sys/devices/.../power/control.
>> 2) an i2c client driver uses i2c during it's suspend callback, thus
>>    leaving the i2c driver active (NOTE: runtime suspend transitions are
>>    disabled during system suspend, so i2c activity during system
>>    suspend will runtime resume the device, but not runtime (re)suspend it.)
>> 
>> Since the actual work to suspend the device is handled by the
>> subsytem, call the bus methods to take care of it.
>> 
>> NOTE: This takes care of a known suspend problem on OMAP3 where the
>> TWL RTC driver does i2c xfers during its suspend path leaving the i2c
>> driver in an active state (since runtime suspend transistions are
>> disabled.)
>> 
>> Signed-off-by: Kevin Hilman <khilman@ti.com>
>> ---
>> Ben, this is a regression in 2.6.38 so hopefully this can be queued
>> in the 2.6.38-rc cycle.  Thanks.
>
> Ok, after all the discussions should I keep it in my -rc queue?
>  

Yes, please.

Kevin