i2c: OMAP: fix static suspend vs. runtime suspend

-----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;

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

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 <[email protected]>

drivers/i2c/busses/i2c-omap.c +14 -1

+			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

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?

-----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;

omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [linux-pm] [PATCH] i2c: OMAP: fix static suspend vs.

Quoted 5 lines

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

-----Original Message-----
From: Alan Stern [mailto:[email protected]]
Sent: Monday, January 31, 2011 8:43 PM
To: Rajendra Nayak
Cc: Kevin Hilman; Ben Dooks; [email protected];

[email protected]; [email protected]
Subject: Re: [linux-pm] [PATCH] i2c: OMAP: fix static suspend vs.

Quoted 5 lines

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

Quoted 4 lines · Rajendra Nayak

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.

Quoted 5 lines · Kevin Hilman

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.

Quoted 11 lines · Alan Stern

Quoted 5 lines · Kevin Hilman

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.

Quoted 8 lines

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.

Quoted 3 lines

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.

Quoted 8 lines

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.

Quoted 3 lines

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.

Quoted 21 lines · Rafael J. Wysocki

Quoted 11 lines · Alan Stern

Quoted 5 lines · Kevin Hilman

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:

Quoted 8 lines

- 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 <[email protected]>
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 <[email protected]>

---

 drivers/base/power/main.c |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

Index: linux-2.6/drivers/base/power/main.c
===================================================================

drivers/base/power/main.c +3 -8

@@ -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

Quoted 57 lines · Rafael J. Wysocki

Quoted 21 lines · Rafael J. Wysocki

Quoted 11 lines · Alan Stern

Quoted 5 lines · Kevin Hilman

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:

Quoted 8 lines

- 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 <[email protected]>
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.

Quoted 2 lines

Signed-off-by Rafael J. Wysocki <[email protected]>

---

Yes!

Acked-by Kevin Hilman <[email protected]>

Quoted 59 lines · Kevin Hilman

Quoted 57 lines · Rafael J. Wysocki

Quoted 21 lines · Rafael J. Wysocki

Quoted 11 lines · Alan Stern

Quoted 5 lines · Kevin Hilman

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:

Quoted 8 lines

- 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 <[email protected]>
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.

Quoted 6 lines

Quoted 2 lines

Signed-off-by Rafael J. Wysocki <[email protected]>

---

Yes!

Acked-by Kevin Hilman <[email protected]>

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

Quoted 65 lines · Rafael J. Wysocki

Quoted 64 lines · Kevin Hilman

Quoted 54 lines · Rafael J. Wysocki

Quoted 21 lines · Rafael J. Wysocki

Quoted 11 lines · Alan Stern

Quoted 5 lines · Kevin Hilman

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:

Quoted 8 lines

- 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 <[email protected]>
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.

Quoted 5 lines

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 <[email protected]>

---

Yes!

Quoted 4 lines

Quoted 1 line

Acked-by Kevin Hilman <[email protected]>

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.

Quoted 65 lines · Rafael J. Wysocki

Quoted 64 lines · Kevin Hilman

Quoted 54 lines · Rafael J. Wysocki

Quoted 21 lines · Rafael J. Wysocki

Quoted 11 lines · Alan Stern

Quoted 5 lines · Kevin Hilman

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:

Quoted 8 lines

- 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 <[email protected]>
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.

Quoted 5 lines

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 <[email protected]>

---

Yes!

Quoted 4 lines

Quoted 1 line

Acked-by Kevin Hilman <[email protected]>

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.

Quoted 59 lines · Kevin Hilman

Quoted 57 lines · Rafael J. Wysocki

Quoted 21 lines · Rafael J. Wysocki

Quoted 11 lines · Alan Stern

Quoted 5 lines · Kevin Hilman

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:

Quoted 8 lines

- 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 <[email protected]>
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.

Quoted 6 lines

Quoted 2 lines

Signed-off-by Rafael J. Wysocki <[email protected]>

---

Yes!

Acked-by Kevin Hilman <[email protected]>

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

Quoted 57 lines · Rafael J. Wysocki

Quoted 21 lines · Rafael J. Wysocki

Quoted 11 lines · Alan Stern

Quoted 5 lines · Kevin Hilman

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:

Quoted 8 lines

- 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 <[email protected]>
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.

Quoted 2 lines

Signed-off-by Rafael J. Wysocki <[email protected]>

---

Yes!

Acked-by Kevin Hilman <[email protected]>

Quoted 21 lines · Rafael J. Wysocki

Quoted 11 lines · Alan Stern

Quoted 5 lines · Kevin Hilman

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:

Quoted 8 lines

- 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 <[email protected]>
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 <[email protected]>

---

 drivers/base/power/main.c |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

Index: linux-2.6/drivers/base/power/main.c
===================================================================

drivers/base/power/main.c +3 -8

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 11 lines · Alan Stern

Quoted 5 lines · Kevin Hilman

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.

Quoted 5 lines · Kevin Hilman

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.

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.

Quoted 4 lines · Rajendra Nayak

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.

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?

-----Original Message-----
From: Kevin Hilman [mailto:[email protected]]
Sent: Friday, January 28, 2011 5:49 AM
To: Ben Dooks; Rajendra Nayak; [email protected]
Cc: [email protected]; [email protected];

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

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 <[email protected]>

drivers/i2c/busses/i2c-omap.c +14 -1

+			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

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 <[email protected]>

---
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(-)

drivers/i2c/busses/i2c-omap.c +28 -0

index b605ff3..0541df9 100644
@@ -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,
 	},
 };
 

Signature · 6 lines

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

Quoted 19 lines · 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.)

Quoted 4 lines

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

---
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?

Quoted 19 lines · 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.)

Quoted 4 lines

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

---
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?

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 <[email protected]>

---
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(-)

drivers/i2c/busses/i2c-omap.c +28 -0

index b605ff3..0541df9 100644
@@ -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,
 	},
 };
 

Signature · 6 lines

1.7.3.5

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html