PM regression with commit 5de85b9d57ab PM runtime re-init in v4.5-rc1

Hi,

Looks like commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") broke PM on at least omap3. It seems we now need to additionally also call pm_runtime_dont_use_autosuspend() to get things working again?

The following fixes idling on omap3, but I'm wondering if we should do something in pm_runtime_reinit() instead?

8< ---------------------

drivers/mmc/host/omap_hsmmc.c +1 -1

@@ -2232,6 +2232,7 @@ err_irq:
 		dma_release_channel(host->tx_chan);
 	if (host->rx_chan)
 		dma_release_channel(host->rx_chan);
+	pm_runtime_dont_use_autosuspend(host->dev);
 	pm_runtime_put_sync(host->dev);
 	pm_runtime_disable(host->dev);
 	if (host->dbclk)
--

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

Hi,

Looks like commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") broke PM on at least omap3. It seems we now need to additionally also call pm_runtime_dont_use_autosuspend() to get things working again?

The following fixes idling on omap3, but I'm wondering if we should do something in pm_runtime_reinit() instead?

Quoted 7 lines · Tony Lindgren

Hi,

Looks like commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") broke PM on at least omap3. It seems we now need to additionally also call pm_runtime_dont_use_autosuspend() to get things working again?

Quoted 2 lines

The following fixes idling on omap3, but I'm wondering if we should do something in pm_runtime_reinit() instead?

Well, does it actually help if you add pm_runtime_dont_use_autosuspend(dev) to pm_runtime_reinit()?

Quoted 8 lines · Rafael J. Wysocki

Quoted 7 lines · Tony Lindgren

Hi,

Looks like commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") broke PM on at least omap3. It seems we now need to additionally also call pm_runtime_dont_use_autosuspend() to get things working again?

Quoted 5 lines

Quoted 2 lines

The following fixes idling on omap3, but I'm wondering if we should do something in pm_runtime_reinit() instead?

Well, does it actually help if you add pm_runtime_dont_use_autosuspend(dev) to pm_runtime_reinit()?

No adding it to pm_runtime_reinit() does not help.

Looks like we have RPM_ACTIVE set in pm_runtime_reinit() if that gives any clues.

Quoted 9 lines · Tony Lindgren

Quoted 8 lines · Rafael J. Wysocki

Quoted 7 lines · Tony Lindgren

Hi,

Looks like commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") broke PM on at least omap3. It seems we now need to additionally also call pm_runtime_dont_use_autosuspend() to get things working again?

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

The following fixes idling on omap3, but I'm wondering if we should do something in pm_runtime_reinit() instead?

Well, does it actually help if you add pm_runtime_dont_use_autosuspend(dev) to pm_runtime_reinit()?

No adding it to pm_runtime_reinit() does not help.

Yes, I realized that it wouldn't help only after sending the previous message, sorry about that.

The reason why it helps in the driver code seems to be that autosuspend_delay happens to be negative, so update_autosuspend() decrements the usage counter that would have stayed incremented otherwise. Or at least that's the only way it can help I see ATM.

Quoted 2 lines

Looks like we have RPM_ACTIVE set in pm_runtime_reinit() if that gives any clues.

It looks like pm_runtime_reinit() should clear the usage counter too.

Quoted 18 lines · Rafael J. Wysocki

Quoted 9 lines · Tony Lindgren

Quoted 8 lines · Rafael J. Wysocki

Quoted 7 lines · Tony Lindgren

Hi,

Looks like commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") broke PM on at least omap3. It seems we now need to additionally also call pm_runtime_dont_use_autosuspend() to get things working again?

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

The following fixes idling on omap3, but I'm wondering if we should do something in pm_runtime_reinit() instead?

Well, does it actually help if you add pm_runtime_dont_use_autosuspend(dev) to pm_runtime_reinit()?

No adding it to pm_runtime_reinit() does not help.

Quoted 7 lines

Yes, I realized that it wouldn't help only after sending the previous message, sorry about that.

The reason why it helps in the driver code seems to be that autosuspend_delay happens to be negative, so update_autosuspend() decrements the usage counter that would have stayed incremented otherwise. Or at least that's the only way it can help I see ATM.

Oh OK.

Quoted 4 lines

Quoted 2 lines

Looks like we have RPM_ACTIVE set in pm_runtime_reinit() if that gives any clues.

It looks like pm_runtime_reinit() should clear the usage counter too.

Yeah if we do this when !pm_runtime_enabled(dev) it seems it's safe to pretty much reset everything?

Quoted 32 lines · Tony Lindgren

Quoted 18 lines · Rafael J. Wysocki

Quoted 9 lines · Tony Lindgren

Quoted 8 lines · Rafael J. Wysocki

Quoted 7 lines · Tony Lindgren

Hi,

Looks like commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") broke PM on at least omap3. It seems we now need to additionally also call pm_runtime_dont_use_autosuspend() to get things working again?

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

The following fixes idling on omap3, but I'm wondering if we should do something in pm_runtime_reinit() instead?

Well, does it actually help if you add pm_runtime_dont_use_autosuspend(dev) to pm_runtime_reinit()?

No adding it to pm_runtime_reinit() does not help.

Quoted 7 lines

Yes, I realized that it wouldn't help only after sending the previous message, sorry about that.

The reason why it helps in the driver code seems to be that autosuspend_delay happens to be negative, so update_autosuspend() decrements the usage counter that would have stayed incremented otherwise. Or at least that's the only way it can help I see ATM.

Oh OK.

Quoted 3 lines

Quoted 2 lines

Looks like we have RPM_ACTIVE set in pm_runtime_reinit() if that gives any clues.

Quoted 4 lines

Quoted 1 line

It looks like pm_runtime_reinit() should clear the usage counter too.

Yeah if we do this when !pm_runtime_enabled(dev) it seems it's safe to pretty much reset everything?

Not only safe, but also a good idea apparently. :-)

Quoted 5 lines · Rafael J. Wysocki

Quoted 2 lines · Tony Lindgren

Yeah if we do this when !pm_runtime_enabled(dev) it seems it's safe to pretty much reset everything?

Not only safe, but also a good idea apparently. :-)

I happy to send a patch, extending pm_runtime_reinit() with some more data to be reset.

Or, you or Tony intend to send a patch?

Quoted 35 lines · Rafael J. Wysocki

Quoted 34 lines · Tony Lindgren

Quoted 18 lines · Rafael J. Wysocki

Quoted 9 lines · Tony Lindgren

Quoted 8 lines · Rafael J. Wysocki

Quoted 7 lines · Tony Lindgren

Hi,

Looks like commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") broke PM on at least omap3. It seems we now need to additionally also call pm_runtime_dont_use_autosuspend() to get things working again?

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

The following fixes idling on omap3, but I'm wondering if we should do something in pm_runtime_reinit() instead?

Well, does it actually help if you add pm_runtime_dont_use_autosuspend(dev) to pm_runtime_reinit()?

No adding it to pm_runtime_reinit() does not help.

Quoted 7 lines

Yes, I realized that it wouldn't help only after sending the previous message, sorry about that.

The reason why it helps in the driver code seems to be that autosuspend_delay happens to be negative, so update_autosuspend() decrements the usage counter that would have stayed incremented otherwise. Or at least that's the only way it can help I see ATM.

Oh OK.

Quoted 4 lines

Quoted 2 lines

Looks like we have RPM_ACTIVE set in pm_runtime_reinit() if that gives any clues.

It looks like pm_runtime_reinit() should clear the usage counter too.

Quoted 4 lines

Quoted 2 lines

Yeah if we do this when !pm_runtime_enabled(dev) it seems it's safe to pretty much reset everything?

Not only safe, but also a good idea apparently. :-)

I happy to send a patch, extending pm_runtime_reinit() with some more data to be reset.

Quoted 7 lines · Rafael J. Wysocki

Quoted 6 lines · Tony Lindgren

Quoted 5 lines · Rafael J. Wysocki

Quoted 4 lines · Tony Lindgren

Hi,

Looks like commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") broke PM on at least

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

omap3. It seems we now need to additionally also call pm_runtime_dont_use_autosuspend() to get things working again?

The following fixes idling on omap3, but I'm wondering if we should do something in pm_runtime_reinit() instead?

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

Well, does it actually help if you add pm_runtime_dont_use_autosuspend(dev) to pm_runtime_reinit()?

No adding it to pm_runtime_reinit() does not help.

Yes, I realized that it wouldn't help only after sending the previous message, sorry about that.

Quoted 4 lines

The reason why it helps in the driver code seems to be that autosuspend_delay happens to be negative, so update_autosuspend() decrements the usage counter that would have stayed incremented otherwise. Or at least that's the only way it can help I see ATM.

Oh OK.

Quoted 2 lines

Quoted 2 lines

Looks like we have RPM_ACTIVE set in pm_runtime_reinit() if that gives any clues.

Quoted 2 lines

It looks like pm_runtime_reinit() should clear the usage counter too.

Yeah if we do this when !pm_runtime_enabled(dev) it seems it's safe to pretty much reset everything?

Regards,

Tony

Quoted 6 lines · Tony Lindgren

Quoted 5 lines · Rafael J. Wysocki

Quoted 4 lines · Tony Lindgren

Quoted 3 lines · Rafael J. Wysocki

Quoted 2 lines · Tony Lindgren

Hi,

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Looks like commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") broke PM on at least

I need to understand the issue here in a bit more detail, could you please try to fill out some of my gaps!?

In *what way* did it break PM?

Did the driver not probe successfully the second try? If so, what happened.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

omap3. It seems we now need to additionally also call pm_runtime_dont_use_autosuspend() to get things working again?

The following fixes idling on omap3, but I'm wondering if we should do something in pm_runtime_reinit() instead?

I understand this as the second (or third, forth, whatever) probing attempt actually succeeds, right!?

Is the issue you are seeing, that the device never becomes runtime suspended due to commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind")?

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Looks like we have RPM_ACTIVE set in pm_runtime_reinit() if that gives any clues.

That's a good clue. Although, there could be several reasons to why.

Rafael has pointed out one valid potential case, but I want to be sure that's really what happening here.

*If* the problem is that the device doesn't becomes runtime suspended, that will anyway be prevented as long as the autosuspend_delay has been set to a negative value. That's why I wonder whether this really is the case here.

For omap3, I assume there's a PM domain (the so called hwmod) being attached to the omap_hsmmc device at device registration point!?

In that path depending on a specific hwmod flag, the device will be given the an initial *active* runtime PM status, via invoking pm_runtime_set_active().

*If* that's the case, it affects the probing sequence, as the pm_runtime_get_sync() won't trigger the ->runtime_resume() callbacks to be invoked in the first probe attempt.

Moreover, according to the data I received in this regression report so far, it seems like probing the second time has *always* been done with the device in runtime PM active state. Could that be the reason to why it "happens" to work?

Hi,

Quoted 7 lines · Ulf Hansson

Quoted 6 lines · Tony Lindgren

Quoted 5 lines · Rafael J. Wysocki

Quoted 4 lines · Tony Lindgren

Quoted 3 lines · Rafael J. Wysocki

Quoted 2 lines · Tony Lindgren

Hi,

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Looks like commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") broke PM on at least

I need to understand the issue here in a bit more detail, could you please try to fill out some of my gaps!?

In *what way* did it break PM?

The MMC hardware will not get idled properly any longer blocking any deeper idle states.

Quoted 1 line

Did the driver not probe successfully the second try? If so, what happened.

It probes fine after a -EPROBE_DEFER on the vmmc i2c regulator. But the PM runtime usecounts are wrong.

Quoted 9 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

omap3. It seems we now need to additionally also call pm_runtime_dont_use_autosuspend() to get things working again?

The following fixes idling on omap3, but I'm wondering if we should do something in pm_runtime_reinit() instead?

I understand this as the second (or third, forth, whatever) probing attempt actually succeeds, right!?

Right.

Quoted 3 lines

Is the issue you are seeing, that the device never becomes runtime suspended due to commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind")?

Correct.

Quoted 8 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Looks like we have RPM_ACTIVE set in pm_runtime_reinit() if that gives any clues.

That's a good clue. Although, there could be several reasons to why.

Rafael has pointed out one valid potential case, but I want to be sure that's really what happening here.

Quoted 4 lines

*If* the problem is that the device doesn't becomes runtime suspended, that will anyway be prevented as long as the autosuspend_delay has been set to a negative value. That's why I wonder whether this really is the case here.

That seems to be the case here. In device driver probe, commenting out pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY) also makes things work again.

So one of the failing cases seems to be:

1. Device driver probe sets pm_runtime_set_autosuspend_delay()

2. Device driver probe initially fails with -EPROBE_DEFER

3. PM runtime usecounts get messed up

Quoted 3 lines

For omap3, I assume there's a PM domain (the so called hwmod) being attached to the omap_hsmmc device at device registration point!?

Correct. It uses the notifiers like bus code does in general.

FYI, most SoCs don't have hardware based autoidle signaling between the interconnect and the interconnect targets. So the hwmod code is still needed until we have converted it into a proper interconnect + module target drivers.

Quoted 4 lines

In that path depending on a specific hwmod flag, the device will be given the an initial *active* runtime PM status, via invoking pm_runtime_set_active().

Quoted 3 lines

*If* that's the case, it affects the probing sequence, as the pm_runtime_get_sync() won't trigger the ->runtime_resume() callbacks to be invoked in the first probe attempt.

It has worked since pm runtime. And it works with MMC as as a loadable module just fine when no -EPROBE_DEFER happens.

Quoted 5 lines

Moreover, according to the data I received in this regression report so far, it seems like probing the second time has *always* been done with the device in runtime PM active state. Could that be the reason to why it "happens" to work?

Not correct. I think that speculation is not related to the $subject regression at all.

BTW, do you have some hardware to test with that has PM runtime implemnted and actually working with mainline kernel?

Quoted 8 lines · Tony Lindgren

The MMC hardware will not get idled properly any longer blocking any deeper idle states.

Quoted 1 line

Did the driver not probe successfully the second try? If so, what happened.

It probes fine after a -EPROBE_DEFER on the vmmc i2c regulator. But the PM runtime usecounts are wrong.

Okay. How did you verify this?

I think the easiest way would be to make sure the usage count is *one*, just before the omap_hsmmc calls mmc_add_host() in its ->probe() function.

If it's *two*, that confirms this theory.

Quoted 3 lines

So one of the failing cases seems to be:

1. Device driver probe sets pm_runtime_set_autosuspend_delay()

Only when "someone" in-before has set the autosuspend delay to a negative value, this will affect the usage count.

I didn't find any in-kernel user that would set this for the omap_hsmmc device, so it needs to be done from userspace via sysfs. It would be really great if you could help to confirm this.

But more importantly, I fail to understand why commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind"), is changing the behaviour in this regards. Potentially we might have a different runtime PM status than we had before when probing the second try, but how could that mess up the usage count...?

Although, I wondering whether it could be that it's the PM domain that's preventing the omap_hsmmc device from becoming runtime suspended.

Perhaps the PM domain returns an error code from its ->runtime_suspend() callback?

Okay, so we definitely seem to have an issue with the changed runtime PM status (suspended) the second probe time.

That's indeed caused by 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind").

For example due to commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind"), the ->runtime_resume() callback seems now to be called twice in a row, without in-between having the ->runtime_suspend() callback being invoked. Does really the PM domain cope with that correctly?

Quoted 2 lines

BTW, do you have some hardware to test with that has PM runtime implemnted and actually working with mainline kernel?

Oh, yes. Although I don't have an omap3, I wish I had.

Also, I have locally a "runtime PM test driver", which helps me to test various runtime PM sequences.

Quoted 7 lines · Ulf Hansson

Quoted 6 lines · Tony Lindgren

The MMC hardware will not get idled properly any longer blocking any deeper idle states.

Quoted 1 line

Did the driver not probe successfully the second try? If so, what happened.

Quoted 4 lines

Quoted 2 lines

It probes fine after a -EPROBE_DEFER on the vmmc i2c regulator. But the PM runtime usecounts are wrong.

Okay. How did you verify this?

Well that was just based on what I see in the dmesg:

omap_device: omap_device_enable() called from invalid state 1

Quoted 8 lines · Tony Lindgren

Quoted 7 lines · Ulf Hansson

Quoted 6 lines · Tony Lindgren

The MMC hardware will not get idled properly any longer blocking any deeper idle states.

Quoted 1 line

Did the driver not probe successfully the second try? If so, what happened.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

It probes fine after a -EPROBE_DEFER on the vmmc i2c regulator. But the PM runtime usecounts are wrong.

Okay. How did you verify this?

Well that was just based on what I see in the dmesg:

omap_device: omap_device_enable() called from invalid state 1

So we're now missing the idling of hardare after -EPROBE_DEFER.. Does the following patch work for you guys?

Regards,

Tony

8< -----------------------------
From: Tony Lindgren <[email protected]>
Date: Mon, 1 Feb 2016 13:40:46 -0800
Subject: [PATCH] PM / runtime: Fix PM runtime reinit

Commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") added calls to reinit the PM runtime. This however broke things for idling the hardware at least if the driver probing has pm_runtime_set_autosuspend_delay() and -EPROBE_DEFER happens.

Fix the problem by adding a check for configured autosuspend if RPM_ACTIVE is set. Then reset the autosuspend, and suspend the device to make sure the hardware gets idled.

Let's also cut down one level of nestedness and remove a negative test by returning early if pm_runtime_enabled(dev) as there is currently nothing for us to do in that case.

Fixes 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at

probe error and driver unbind")

Signed-off-by Tony Lindgren <[email protected]>

drivers/base/power/runtime.c +0 -0

@@ -1419,17 +1419,25 @@ void pm_runtime_init(struct device *dev)
  */
 void pm_runtime_reinit(struct device *dev)

drivers/base/power/runtime.c +9 -6

+               } else {
                        pm_runtime_set_suspended(dev);
-               if (dev->power.irq_safe) {
-                       spin_lock_irq(&dev->power.lock);
-                       dev->power.irq_safe = 0;
-                       spin_unlock_irq(&dev->power.lock);
-                       if (dev->parent)
-                               pm_runtime_put(dev->parent);
                }
        }
+
+       if (dev->power.irq_safe) {
+               spin_lock_irq(&dev->power.lock);
+               dev->power.irq_safe = 0;
+               spin_unlock_irq(&dev->power.lock);
+               if (dev->parent)
+                       pm_runtime_put(dev->parent);
+       }
 }
 
 /**

Quoted 9 lines · Tony Lindgren

drivers/base/power/runtime.c +1 -2

@@ -1419,17 +1419,25 @@ void pm_runtime_init(struct device *dev)
  */
 void pm_runtime_reinit(struct device *dev)
 {
-       if (!pm_runtime_enabled(dev)) {
-               if (dev->power.runtime_status == RPM_ACTIVE)
+       if (pm_runtime_enabled(dev))

drivers/base/power/runtime.c void pm_runtime_init(struct device *dev) · line 1428

Quoted 6 lines

drivers/base/power/runtime.c +6 -0

+               return;
+
+       if (dev->power.runtime_status == RPM_ACTIVE) {
+               if (dev->power.use_autosuspend) {
+                       __pm_runtime_use_autosuspend(dev, false);
+                       pm_runtime_suspend(dev);

This won't work, because runtime PM is disabled at this point.

What about doing this instead:

               if (dev->power.use_autosuspend)
                       __pm_runtime_use_autosuspend(dev, false);

               pm_runtime_set_suspended(dev);

Quoted 10 lines · Rafael J. Wysocki

Quoted 9 lines · Tony Lindgren

drivers/base/power/runtime.c +1 -2

@@ -1419,17 +1419,25 @@ void pm_runtime_init(struct device *dev)
  */
 void pm_runtime_reinit(struct device *dev)
 {
-       if (!pm_runtime_enabled(dev)) {
-               if (dev->power.runtime_status == RPM_ACTIVE)
+       if (pm_runtime_enabled(dev))

Quoted 8 lines

drivers/base/power/runtime.c void pm_runtime_init(struct device *dev) · line 1428

Quoted 6 lines

drivers/base/power/runtime.c +6 -0

+               return;
+
+       if (dev->power.runtime_status == RPM_ACTIVE) {
+               if (dev->power.use_autosuspend) {
+                       __pm_runtime_use_autosuspend(dev, false);
+                       pm_runtime_suspend(dev);

This won't work, because runtime PM is disabled at this point.

Hmm right OK. It does work from idling the hardware point of view though..

Quoted 6 lines

What about doing this instead:

               if (dev->power.use_autosuspend)
                       __pm_runtime_use_autosuspend(dev, false);

               pm_runtime_set_suspended(dev);

..while this does not work. The hardware is never idled in this case.

What else does __pm_runtime_use_autosuspend() set initially that changes things here?

Quoted 20 lines · Tony Lindgren

Quoted 18 lines · Rafael J. Wysocki

Quoted 5 lines · Tony Lindgren

drivers/base/power/runtime.c +0 -0

@@ -1419,17 +1419,25 @@ void pm_runtime_init(struct device *dev)
  */
 void pm_runtime_reinit(struct device *dev)

drivers/base/power/runtime.c void pm_runtime_init(struct device *dev) · line 1428

Quoted 10 lines

drivers/base/power/runtime.c +7 -2

 {
-       if (!pm_runtime_enabled(dev)) {
-               if (dev->power.runtime_status == RPM_ACTIVE)
+       if (pm_runtime_enabled(dev))
+               return;
+
+       if (dev->power.runtime_status == RPM_ACTIVE) {
+               if (dev->power.use_autosuspend) {
+                       __pm_runtime_use_autosuspend(dev, false);
+                       pm_runtime_suspend(dev);

This won't work, because runtime PM is disabled at this point.

Quoted 2 lines

Hmm right OK. It does work from idling the hardware point of view though..

pm_runtime_suspend() with runtime PM disabled is a NOP. It will do nothing and return -EACCES.

Quoted 5 lines

Quoted 5 lines

What about doing this instead:

               if (dev->power.use_autosuspend)
                       __pm_runtime_use_autosuspend(dev, false);

Quoted 4 lines

Quoted 1 line

               pm_runtime_set_suspended(dev);

..while this does not work. The hardware is never idled in this case.

I'm not sure what you mean. pm_runtime_set_suspended() sets the status to RPM_SUSPENDED for devices with runtime PM disabled. It has nothing to do with "idling" in principle.

Quoted 2 lines

What else does __pm_runtime_use_autosuspend() set initially that changes things here?

The usage counter, if the delay is negative.

I'll look at this in detail, but not right now, sorry. I'm working on something else ATM and I was hoping that Ulf would be able to figure out what's going on here.

Does the patch below make more sense to you where we call rpm_idle? That seems to make things behave here also.

Quoted 4 lines

Quoted 2 lines

What else does __pm_runtime_use_autosuspend() set initially that changes things here?

The usage counter, if the delay is negative.

Yeah I don't see any difference with those.

Quoted 3 lines

I'll look at this in detail, but not right now, sorry. I'm working on something else ATM and I was hoping that Ulf would be able to figure out what's going on here.

Yeah we need to understand what's going on here. Having the PM runtime framework out of sync with the hardare is not good.. If we can't figure this out we should probably revert the patch until we understand it.

Regards,

Tony

8< ------------

drivers/base/power/runtime.c +0 -2

@@ -1419,17 +1419,28 @@ void pm_runtime_init(struct device *dev)
  */
 void pm_runtime_reinit(struct device *dev)
 {
-	if (!pm_runtime_enabled(dev)) {
-		if (dev->power.runtime_status == RPM_ACTIVE)

You get here only if runtime PM is disabled, right? So the rpm_idle call won't do anything -- "disabled" means don't make any callbacks.

Tony, exactly what are you trying to do here? Do you want this to invoke a runtime-PM callback in the subsystem, power domain, or driver? (Is there even a driver bound to the device when this function runs?)

The function's name suggests that it merely resets the data stored in dev->power without actually touching the hardware. Is that what you really want?

Hi,

Quoted 3 lines · Alan Stern

You get here only if runtime PM is disabled, right? So the rpm_idle call won't do anything -- "disabled" means don't make any callbacks.

Hmm sorry yes I'm confused again. Yeah it looks like calling rpm_idle just has a side effect that makes a difference here.

Quoted 3 lines

Tony, exactly what are you trying to do here? Do you want this to invoke a runtime-PM callback in the subsystem, power domain, or driver? (Is there even a driver bound to the device when this function runs?)

I guess I need to add more printks to figure out what's going on here. But yeah, I'm not seeing the callback happening at the interconnect level so hardware and PM runtime states won't match on the following probe after -EPROBE_DEFER.

Quoted 3 lines

The function's name suggests that it merely resets the data stored in dev->power without actually touching the hardware. Is that what you really want?

I guess you mean pm_runtime_set_suspended() above? I'm seeing a state where we now set pm_runtime_set_suspended() between failed device probes and the device is still active in hardware.

The patch below also helps with the problem and leaves out the rpm_suspend() call from loop so it might give more hints.

The difference here from what Rafael suggested earlier is calling __pm_runtime_use_autosuspend() and then not calling pm_runtime_set_suspended().

However, it seems the below patch keeps hardware active in the autoidle case though, so chances are there is more that needs to be done here. Anyways, I'll try to debug it more tomorrow.

Instead of the suggested approaches, I think the regression should be fixed at the PM domain level (omap hwmod). I have attached a patch below, please give it try as it's untested.

Quoted 30 lines · Tony Lindgren

Hi,

Quoted 3 lines · Alan Stern

You get here only if runtime PM is disabled, right? So the rpm_idle call won't do anything -- "disabled" means don't make any callbacks.

Hmm sorry yes I'm confused again. Yeah it looks like calling rpm_idle just has a side effect that makes a difference here.

Quoted 3 lines

Tony, exactly what are you trying to do here? Do you want this to invoke a runtime-PM callback in the subsystem, power domain, or driver? (Is there even a driver bound to the device when this function runs?)

I guess I need to add more printks to figure out what's going on here. But yeah, I'm not seeing the callback happening at the interconnect level so hardware and PM runtime states won't match on the following probe after -EPROBE_DEFER.

Quoted 3 lines

The function's name suggests that it merely resets the data stored in dev->power without actually touching the hardware. Is that what you really want?

I guess you mean pm_runtime_set_suspended() above? I'm seeing a state where we now set pm_runtime_set_suspended() between failed device probes and the device is still active in hardware.

The patch below also helps with the problem and leaves out the rpm_suspend() call from loop so it might give more hints.

Quoted 8 lines

The difference here from what Rafael suggested earlier is calling __pm_runtime_use_autosuspend() and then not calling pm_runtime_set_suspended().

However, it seems the below patch keeps hardware active in the autoidle case though, so chances are there is more that needs to be done here. Anyways, I'll try to debug it more tomorrow.

Your observations is correct. The hardware will be kept active in-between the probe attempts (and thus also if probing fails). Although, that's not a regression as that's the behaviour you get from runtime PM, when drivers are implemented like omap_hsmmc.

Instead of the suggested approaches, I think the regression should be fixed at the PM domain level (omap hwmod). I have attached a patch below, please give it try as it's untested.

To solve the other problem (allowing devices to become inactive in-between at probe failures), I see two options (not treated as regressions).
1)
Change the behaviour of pm_runtime_put_sync(), to *not* respect the autosuspend mode.
I think I prefer this option, as it seems like autosuspend should be respected only via the asynchronous runtime PM APIs.

2)
Change the failing drivers, to before calling pm_runtime_put_sync() also invoke pm_runtime_dont_use_autosusend(). Or other similar approach.

That's not what we want. So I moved the checks to the actual ->runtime_resume() callback in the PM domain, instead of in the omap_device_enable() function. A version 2 is attached. Please give it at try.

[...]

From: Ulf Hansson <[email protected]>
Date: Tue, 2 Feb 2016 10:05:39 +0100
Subject: [PATCH V2] ARM: OMAP2+: omap-device: Allow devices to be pre-enabled

Commit 5de85b9d57ab ("PM / runtime: Re-init runtime PM states at probe error and driver unbind") may re-initialize the runtime PM status of the device to RPM_SUSPENDED at driver probe failures.

For the omap_hsmmc and likely also other omap drivers, which needs more than one attempt to ->probe() (returning -EPROBE_DEFER), this commit causes a regression at the PM domain level (omap hwmod).

The reason is that the drivers don't put back the device into low power state while bailing out in ->probe to return -EPROBE_DEFER. This leads to that pm_runtime_reinit() in driver core, is re-initializing the runtime PM status from RPM_ACTIVE to RPM_SUSPENDED.

The next ->probe() attempt then triggers the ->runtime_resume() callback to be invoked, which means this happens two times in a row. At the PM domain level (omap hwmod) this is being treated as an error and thus the runtime PM status of the device isn't correctly synchronized with the runtime PM core.

In the end, ->probe() anyway succeeds (as the driver don't checks the error code from the runtime PM APIs), but results in that the PM domain always stays powered on. This because of the runtime PM core believes the device is RPM_SUSPENDED.

Fix this regression by allowing devices to be pre-enabled when the PM domain's (omap hwmod) ->runtime_resume() callback is requested to enable the device. In such cases, return zero to synchronize the runtime PM status with the runtime PM core.

For the omap_hsmmc and likely also other omap drivers, which needs more than one attempt to ->probe() (returning -EPROBE_DEFER), this commit causes a regression at the PM domain level (omap hwmod).

The reason is that the drivers don't put back the device into low power state while bailing out in ->probe to return -EPROBE_DEFER. This leads to that pm_runtime_reinit() in driver core, is re-initializing the runtime PM status from RPM_ACTIVE to RPM_SUSPENDED.

The next ->probe() attempt then triggers the ->runtime_resume() callback to be invoked, which means this happens two times in a row. At the PM domain level (omap hwmod) this is being treated as an error and thus the runtime PM status of the device isn't correctly synchronized with the runtime PM core.

In the end, ->probe() anyway succeeds (as the driver don't checks the error code from the runtime PM APIs), but results in that the PM domain always stays powered on. This because of the runtime PM core believes the device is RPM_SUSPENDED.

Quoted 5 lines · Ulf Hansson

Your observations is correct. The hardware will be kept active in-between the probe attempts (and thus also if probing fails). Although, that's not a regression as that's the behaviour you get from runtime PM, when drivers are implemented like omap_hsmmc.

Perhaps this is what we should do. If probing gets deferred a few times, doing runtime suspends and resumes in between will be a waste of time.

? pm_runtime_put_sync() _already_ does not respect the autosuspend mode. If you want to respect it, you have to call pm_runtime_put_sync_autosuspend() instead.

Quoted 3 lines · Alan Stern

? pm_runtime_put_sync() _already_ does not respect the autosuspend mode. If you want to respect it, you have to call pm_runtime_put_sync_autosuspend() instead.

I think you found the real bug there. So the right fix is to call pm_runtime_put_sync_autosuspend() at the end of failed probe in omap_hsmmc. Let me give that a try here.

Can we add some warning to pm_runtime_put_sync() about that?

Quoted 6 lines · Tony Lindgren

Quoted 3 lines · Alan Stern

? pm_runtime_put_sync() _already_ does not respect the autosuspend mode. If you want to respect it, you have to call pm_runtime_put_sync_autosuspend() instead.

Quoted 3 lines

I think you found the real bug there. So the right fix is to call pm_runtime_put_sync_autosuspend() at the end of failed probe in omap_hsmmc. Let me give that a try here.

Nope that's not it but getting closer.

The following seems to make things behave for me. Now the question is.. Does it have some undesired side effects?

Quoted 1 line

Can we add some warning to pm_runtime_put_sync() about that?

Probably no need for that, I misunderstood the meaning of pm_runtime_put_sync_autosuspend().

Regards,

Tony

8< --------------

The following seems to make things behave for me. Now the question is.. Does it have some undesired side effects?

Quoted 11 lines · Tony Lindgren

Quoted 9 lines · Tony Lindgren

Quoted 3 lines · Alan Stern

? pm_runtime_put_sync() _already_ does not respect the autosuspend mode. If you want to respect it, you have to call pm_runtime_put_sync_autosuspend() instead.

I think you found the real bug there. So the right fix is to call pm_runtime_put_sync_autosuspend() at the end of failed probe in omap_hsmmc. Let me give that a try here.

Quoted 4 lines

Nope that's not it but getting closer.

The following seems to make things behave for me. Now the question is.. Does it have some undesired side effects?

Yes, it does.

I'm still not clear on what you want to accomplish. It sounds like you want to perform a runtime suspend following the last probe (if the probe fails), and in between probes you don't really care (although it would be preferable to avoid suspending).

Does pm_runtime_use_autosuspend() get called by the probe routine? If it does, then perhaps you can get what you want by having the probe routine call pm_runtime_dont_use_autosuspend() whenever it's about to return an error -- particularly -EDEFER.

Quoted 4 lines

I'm still not clear on what you want to accomplish. It sounds like you want to perform a runtime suspend following the last probe (if the probe fails), and in between probes you don't really care (although it would be preferable to avoid suspending).

I'd like to have pm_runtime_put_sync() disable the hardware after the initial failed probe. Currently that does not happen unless pm_runtime_dont_use_autosuspend() is called before pm_runtime_put_sync().

Quoted 4 lines

Does pm_runtime_use_autosuspend() get called by the probe routine? If it does, then perhaps you can get what you want by having the probe routine call pm_runtime_dont_use_autosuspend() whenever it's about to return an error -- particularly -EDEFER.

Yes so far that's the only fix that seems to work like I posted earlier. But is that the right fix though?

If we wanted to have some generic fix, it seems we would have to pass a new flag in pm_runtime_put_sync() to ignore any autosuspend configuration. But I don't know if that's what we want to or should do though?

Quoted 5 lines

Your observations is correct. The hardware will be kept active in-between the probe attempts (and thus also if probing fails). Although, that's not a regression as that's the behaviour you get from runtime PM, when drivers are implemented like omap_hsmmc.

Perhaps this is what we should do. If probing gets deferred a few times, doing runtime suspends and resumes in between will be a waste of time.

Quoted 5 lines

Instead of the suggested approaches, I think the regression should be fixed at the PM domain level (omap hwmod). I have attached a patch below, please give it try as it's untested.

Quoted 8 lines

To solve the other problem (allowing devices to become inactive in-between at probe failures), I see two options (not treated as regressions).
1)
Change the behaviour of pm_runtime_put_sync(), to *not* respect the autosuspend mode.
I think I prefer this option, as it seems like autosuspend should be respected only via the asynchronous runtime PM APIs.

? pm_runtime_put_sync() _already_ does not respect the autosuspend mode. If you want to respect it, you have to call pm_runtime_put_sync_autosuspend() instead.

From Tony's regression report and from mine own local runtime PM test

Quoted 5 lines

2)
Change the failing drivers, to before calling pm_runtime_put_sync() also invoke pm_runtime_dont_use_autosusend(). Or other similar approach.

Given the above, this seems unnecessary.

Hi,

Quoted 9 lines · Ulf Hansson

For the omap_hsmmc and likely also other omap drivers, which needs more than one attempt to ->probe() (returning -EPROBE_DEFER), this commit causes a regression at the PM domain level (omap hwmod).

The reason is that the drivers don't put back the device into low power state while bailing out in ->probe to return -EPROBE_DEFER. This leads to that pm_runtime_reinit() in driver core, is re-initializing the runtime PM status from RPM_ACTIVE to RPM_SUSPENDED.

Yup, that's the bug here. It seems that we never call the runtime_suspend callback at the end of a first failed device driver probe if the driver has set pm_runtime_use_autosuspend. Only rpm_idle runtime_idle callback gets called. So the device stays on.

This does not happen if pm_runtime_dont_use_autosuspend() is added to the end of the device driver probe before pm_runtime_put_sync().

Quoted 6 lines

The next ->probe() attempt then triggers the ->runtime_resume() callback to be invoked, which means this happens two times in a row. At the PM domain level (omap hwmod) this is being treated as an error and thus the runtime PM status of the device isn't correctly synchronized with the runtime PM core.

That's a valid error though, let's not remove it. The reason why we call runtime_resume() twice is because runtime_suspend callback never gets called like I explain above.

Quoted 5 lines

In the end, ->probe() anyway succeeds (as the driver don't checks the error code from the runtime PM APIs), but results in that the PM domain always stays powered on. This because of the runtime PM core believes the device is RPM_SUSPENDED.

FYI, the following allows runtime_suspend callback to get called at the end of a failed driver probe so the hardware state matches the PM runtime state. Need to debug more.

Regards,

Tony

8< ------------

Quoted 3 lines · Tony Lindgren

I'd like to have pm_runtime_put_sync() disable the hardware after the initial failed probe. Currently that does not happen unless pm_runtime_dont_use_autosuspend() is called before pm_runtime_put_sync().

pm_runtime_put_sync() doesn't do anything to the hardware if the usage count was > 1, because after the decrement it's still nonzero. Where is the particular call of pm_runtime_put_sync() that you're interested in, and what is the usage count when it runs? It's not at all unusual for the usage count to be > 1 during a probe.

Also, what is autosuspend_delay set to for your device? And is runtime_auto set?

Quoted 5 lines

Quoted 4 lines

Does pm_runtime_use_autosuspend() get called by the probe routine? If it does, then perhaps you can get what you want by having the probe routine call pm_runtime_dont_use_autosuspend() whenever it's about to return an error -- particularly -EDEFER.

Quoted 2 lines

Yes so far that's the only fix that seems to work like I posted earlier. But is that the right fix though?

No, not really. Ideally you would leave autosuspend turned on. The delay would be long enough to that after -EDEFER, another probe would start before the delay expired. But shortly after the last probe attempt, the delay would expire and the device would then be put in low power.

Quoted 4 lines

If we wanted to have some generic fix, it seems we would have to pass a new flag in pm_runtime_put_sync() to ignore any autosuspend configuration. But I don't know if that's what we want to or should do though?

I don't think so.

Quoted 3 lines

? pm_runtime_put_sync() _already_ does not respect the autosuspend mode. If you want to respect it, you have to call pm_runtime_put_sync_autosuspend() instead.

Then there's a bug in the runtime PM core.

From Tony's regression report and from mine own local runtime PM test driver, I can see that the device doesn't get RPM_SUSPENDED (the ->runtime_suspend() callback isn't called), even when the usage count is zero - when pm_runtime_put_sync() is called.

To find the sequence of runtime PM commands, go ahead an have look in the omap_hsmmc driver. The problem occurs when the driver bails out in probe, when it receives -EPROBE_DEFER when fetching regulators.

I have some more data to share on this problem from my runtime PM test driver. I will try my best to share it tomorrow.

Quoted 5 lines

Quoted 4 lines

2)
Change the failing drivers, to before calling pm_runtime_put_sync() also invoke pm_runtime_dont_use_autosusend(). Or other similar approach.

Quoted 1 line

Given the above, this seems unnecessary.

Okay, so you are saying that the pm_runtime_put_sync() should idle the device even if autosuspend is in use. That seems reasonable, I will look into this problem.

Quoted 4 lines

? pm_runtime_put_sync() _already_ does not respect the autosuspend mode. If you want to respect it, you have to call pm_runtime_put_sync_autosuspend() instead.

Then there's a bug in the runtime PM core.

From Tony's regression report and from mine own local runtime PM test driver, I can see that the device doesn't get RPM_SUSPENDED (the ->runtime_suspend() callback isn't called), even when the usage count is zero - when pm_runtime_put_sync() is called.

Okay, so you are saying that the pm_runtime_put_sync() should idle the device even if autosuspend is in use. That seems reasonable, I will look into this problem.

Quoted 2 lines

Also, what is autosuspend_delay set to for your device? And is runtime_auto set?

It's 100 at that point, see the commented snippet below from omap_hsmmc_probe():

	pm_runtime_enable(host->dev);
	pm_runtime_get_sync(host->dev);
	pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY);
	/* NOTE: pm_runtime_dont_use_autosuspend(host->dev) needed here? */
	pm_runtime_use_autosuspend(host->dev);
	...
	/* gets -EPROBE_DEFER */
err_irq:
	...
	pm_runtime_put_sync(host->dev);

        pm_runtime_disable(host->dev);
	/* NOTE: suspend callback never gets called unless
	 * pm_runtime_dont_use_autosuspend() is called
	 * before pm_runtime_put_sync() above.
	 */
	 ...

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Does pm_runtime_use_autosuspend() get called by the probe routine? If it does, then perhaps you can get what you want by having the probe routine call pm_runtime_dont_use_autosuspend() whenever it's about to return an error -- particularly -EDEFER.

Quoted 8 lines

Quoted 2 lines

Yes so far that's the only fix that seems to work like I posted earlier. But is that the right fix though?

No, not really. Ideally you would leave autosuspend turned on. The delay would be long enough to that after -EDEFER, another probe would start before the delay expired. But shortly after the last probe attempt, the delay would expire and the device would then be put in low power.

But then what about the new reinit function? To me it seems that we should not attempt to maintain a state from the earlier failed probe. Or are you thinking we just skip the reinit if autosuspend is set?

Quoted 6 lines

Quoted 4 lines

If we wanted to have some generic fix, it seems we would have to pass a new flag in pm_runtime_put_sync() to ignore any autosuspend configuration. But I don't know if that's what we want to or should do though?

I don't think so.

So should we just establish a policy that pm_runtime_use_autosuspend() needs to be paired with pm_runtime_dont_use_autosuspend() for pm_runtime_put_sync() to work?

Quoted 3 lines · Tony Lindgren

That's a valid error though, let's not remove it. The reason why we call runtime_resume() twice is because runtime_suspend callback never gets called like I explain above.

This isn't an error, it's just a hickup in the synchronization of the runtime PM status.

Very similar what happens at the first probe, when the driver core has initialized the runtime PM status to RPM_SUSPENDED at the device registration.

To me, it's the responsible of the PM domain to *help* with the synchronization, not prevent it as it currently does.

drivers/mmc/host/omap_hsmmc.c err_irq: · line 2235

Quoted 7 lines

drivers/mmc/host/omap_hsmmc.c +1 -0

@@ -2232,6 +2232,7 @@ err_irq:
                dma_release_channel(host->tx_chan);
        if (host->rx_chan)
                dma_release_channel(host->rx_chan);
+       pm_runtime_dont_use_autosuspend(host->dev);

It's good know this works, although do you intend to fix this sequence for all omap drivers/devices that's part of the hwmod PM domain?

I haven't checked the number of drivers this would affect, but I imagine there could be quite many with similar behaviour and thus may suffer from the same issue.

Could you please test my version 2 of the patch I attached earlier. I still believe it's the best way to solve the regression, if it works of course. :-)

Quoted 6 lines · Tony Lindgren

Quoted 2 lines

Also, what is autosuspend_delay set to for your device? And is runtime_auto set?

It's 100 at that point, see the commented snippet below from omap_hsmmc_probe():

Quoted 10 lines

	pm_runtime_enable(host->dev);
	pm_runtime_get_sync(host->dev);
	pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY);
	/* NOTE: pm_runtime_dont_use_autosuspend(host->dev) needed here? */
	pm_runtime_use_autosuspend(host->dev);
	...
	/* gets -EPROBE_DEFER */
err_irq:
	...
	pm_runtime_put_sync(host->dev);

You could try changing this to pm_runtime_put_sync_suspend(). But putting pm_runtime_dont_use_autosuspend() before the put_sync seems like a perfectly reasonable thing to do, especially if you feel you should reverse all the changes you made at the start.

Quoted 21 lines

        pm_runtime_disable(host->dev);
	/* NOTE: suspend callback never gets called unless
	 * pm_runtime_dont_use_autosuspend() is called
	 * before pm_runtime_put_sync() above.
	 */
	 ...

Quoted 13 lines

Quoted 7 lines

Quoted 4 lines

Does pm_runtime_use_autosuspend() get called by the probe routine? If it does, then perhaps you can get what you want by having the probe routine call pm_runtime_dont_use_autosuspend() whenever it's about to return an error -- particularly -EDEFER.

Yes so far that's the only fix that seems to work like I posted earlier. But is that the right fix though?

No, not really. Ideally you would leave autosuspend turned on. The delay would be long enough to that after -EDEFER, another probe would start before the delay expired. But shortly after the last probe attempt, the delay would expire and the device would then be put in low power.

Quoted 4 lines

But then what about the new reinit function? To me it seems that we should not attempt to maintain a state from the earlier failed probe. Or are you thinking we just skip the reinit if autosuspend is set?

The reinit function gets called too late to do what you want -- namely, put the hardware in a low-power state.

That _is_ what you want, isn't it? The alternative is to leave dev->power.rpm_status set to RPM_ACTIVE, to match the hardware's actual state.

Given that the reinit function is supposed to restore the initial settings, it probably ought to call pm_runtime_dont_use_autosuspend(). But that won't be enough to fix your problem.

Quoted 10 lines

Quoted 6 lines

Quoted 4 lines

If we wanted to have some generic fix, it seems we would have to pass a new flag in pm_runtime_put_sync() to ignore any autosuspend configuration. But I don't know if that's what we want to or should do though?

I don't think so.

So should we just establish a policy that pm_runtime_use_autosuspend() needs to be paired with pm_runtime_dont_use_autosuspend() for pm_runtime_put_sync() to work?

Your understanding is slightly wrong. pm_runtime_put_sync() _does_ work -- that is, it does what it's supposed to do. The difficulty is that what it's supposed to do doesn't match what you think.

pm_runtime_put_sync() is supposed to follow the driver's wishes. It invokes the driver's runtime_idle callback if there is one, and the callback routine can start a suspend or an autosuspend. If there is no callback, it will use whatever autosuspend setting the driver has set up. If you want to override the autosuspend setting, use pm_runtime_put_sync_suspend() instead.

Quoted 7 lines · Ulf Hansson

Quoted 3 lines · Tony Lindgren

That's a valid error though, let's not remove it. The reason why we call runtime_resume() twice is because runtime_suspend callback never gets called like I explain above.

This isn't an error, it's just a hickup in the synchronization of the runtime PM status.

I'd rather not get the hardware state out of sync with PM runtime..

Quoted 3 lines

Very similar what happens at the first probe, when the driver core has initialized the runtime PM status to RPM_SUSPENDED at the device registration.

Well we actually pretty much have devices in that state to start with.

Quoted 3 lines

To me, it's the responsible of the PM domain to *help* with the synchronization, not prevent it as it currently does.

The problem is that the hardware state gets out of sync with PM runtime. And that's going to be a pain to debug later on.

Quoted 12 lines

drivers/mmc/host/omap_hsmmc.c err_irq: · line 2235

Quoted 8 lines

drivers/mmc/host/omap_hsmmc.c +1 -0

@@ -2232,6 +2232,7 @@ err_irq:
                dma_release_channel(host->tx_chan);
        if (host->rx_chan)
                dma_release_channel(host->rx_chan);
+       pm_runtime_dont_use_autosuspend(host->dev);

It's good know this works, although do you intend to fix this sequence for all omap drivers/devices that's part of the hwmod PM domain?

Quoted 3 lines

I haven't checked the number of drivers this would affect, but I imagine there could be quite many with similar behaviour and thus may suffer from the same issue.

Yeah not sure what the right fix is. But I'd rather patch the few drivers using autosuspend if we come to the conclusion that there is no bug in PM runtime.

Quoted 3 lines

Could you please test my version 2 of the patch I attached earlier. I still believe it's the best way to solve the regression, if it works of course. :-)

And I don't like it because of the reasons above :) But yeah gave it a quick try and that too works as expected.

I don't see the problem, but of course you know omap for better than I do.

So if you are concerned about this, perhaps adding a dev_dbg print when the omap hwmod's ->runtime_suspend() callback returns zero could

Quoted 5 lines

I don't see the problem, but of course you know omap for better than I do.

So if you are concerned about this, perhaps adding a dev_dbg print when the omap hwmod's ->runtime_suspend() callback returns zero could

/s/runtime_suspend/runtime_resume

Quoted 11 lines · Ulf Hansson

Quoted 4 lines

? pm_runtime_put_sync() _already_ does not respect the autosuspend mode. If you want to respect it, you have to call pm_runtime_put_sync_autosuspend() instead.

Then there's a bug in the runtime PM core.

From Tony's regression report and from mine own local runtime PM test driver, I can see that the device doesn't get RPM_SUSPENDED (the ->runtime_suspend() callback isn't called), even when the usage count is zero - when pm_runtime_put_sync() is called.

...

Quoted 3 lines

Okay, so you are saying that the pm_runtime_put_sync() should idle the device even if autosuspend is in use. That seems reasonable, I will look into this problem.

The patch below fixes pm_runtime_put_sync() to not respect the autosuspend mode to match what Alan is saying above. Seems to also fixes the $subject issue for me. And seems to behave for PM runtime for other devices during runtime too based on light testing here.

Regards,

Tony

8< ---------------

drivers/base/power/runtime.c +3 -1

@@ -353,7 +353,9 @@ static int rpm_idle(struct device *dev, int rpmflags)
 
  out:
        trace_rpm_return_int(dev, _THIS_IP_, retval);
-       return retval ? retval : rpm_suspend(dev, rpmflags | RPM_AUTO);
+       if (!(rpmflags & RPM_IGNORE_AUTO))
+               rpmflags |= RPM_AUTO;
+       return retval ? retval : rpm_suspend(dev, rpmflags);
 }
 
 /**

include/linux/pm_runtime.h +2 -0

@@ -23,6 +23,8 @@
                                            usage_count */
 #define RPM_AUTO               0x08    /* Use autosuspend_delay */
 
+#define RPM_IGNORE_AUTO                0x10    /* Ignore autosuspend */
+
 #ifdef CONFIG_PM
 extern struct workqueue_struct *pm_wq;
 

Quoted 19 lines · Alan Stern

Quoted 6 lines · Tony Lindgren

Quoted 2 lines

Also, what is autosuspend_delay set to for your device? And is runtime_auto set?

It's 100 at that point, see the commented snippet below from omap_hsmmc_probe():

Quoted 10 lines

    pm_runtime_enable(host->dev);
    pm_runtime_get_sync(host->dev);
    pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY);
    /* NOTE: pm_runtime_dont_use_autosuspend(host->dev) needed here? */
    pm_runtime_use_autosuspend(host->dev);
    ...
    /* gets -EPROBE_DEFER */
err_irq:
    ...
    pm_runtime_put_sync(host->dev);

Quoted 4 lines

You could try changing this to pm_runtime_put_sync_suspend(). But putting pm_runtime_dont_use_autosuspend() before the put_sync seems like a perfectly reasonable thing to do, especially if you feel you should reverse all the changes you made at the start.

They both seem to fix the problem.

Quoted 21 lines

Quoted 21 lines

        pm_runtime_disable(host->dev);
    /* NOTE: suspend callback never gets called unless
     * pm_runtime_dont_use_autosuspend() is called
     * before pm_runtime_put_sync() above.
     */
     ...

Quoted 13 lines

Quoted 7 lines

Quoted 4 lines

Does pm_runtime_use_autosuspend() get called by the probe routine? If it does, then perhaps you can get what you want by having the probe routine call pm_runtime_dont_use_autosuspend() whenever it's about to return an error -- particularly -EDEFER.

Yes so far that's the only fix that seems to work like I posted earlier. But is that the right fix though?

No, not really. Ideally you would leave autosuspend turned on. The delay would be long enough to that after -EDEFER, another probe would start before the delay expired. But shortly after the last probe attempt, the delay would expire and the device would then be put in low power.

Quoted 7 lines

Quoted 4 lines

But then what about the new reinit function? To me it seems that we should not attempt to maintain a state from the earlier failed probe. Or are you thinking we just skip the reinit if autosuspend is set?

The reinit function gets called too late to do what you want -- namely, put the hardware in a low-power state.

Right, the problem is the lack of suspend on the first probe. But for having autosuspend timeout long enough for the next probe would mean that we can't reset the PM runtime state in between.

Quoted 3 lines

That _is_ what you want, isn't it? The alternative is to leave dev->power.rpm_status set to RPM_ACTIVE, to match the hardware's actual state.

As far as I can tell things work just fine if the failed probe suspend the device at the end of the failed probe.

Quoted 19 lines

Given that the reinit function is supposed to restore the initial settings, it probably ought to call pm_runtime_dont_use_autosuspend(). But that won't be enough to fix your problem.

Quoted 10 lines

Quoted 6 lines

Quoted 4 lines

If we wanted to have some generic fix, it seems we would have to pass a new flag in pm_runtime_put_sync() to ignore any autosuspend configuration. But I don't know if that's what we want to or should do though?

I don't think so.

So should we just establish a policy that pm_runtime_use_autosuspend() needs to be paired with pm_runtime_dont_use_autosuspend() for pm_runtime_put_sync() to work?

Your understanding is slightly wrong. pm_runtime_put_sync() _does_ work -- that is, it does what it's supposed to do. The difficulty is that what it's supposed to do doesn't match what you think.

Quoted 6 lines

pm_runtime_put_sync() is supposed to follow the driver's wishes. It invokes the driver's runtime_idle callback if there is one, and the callback routine can start a suspend or an autosuspend. If there is no callback, it will use whatever autosuspend setting the driver has set up. If you want to override the autosuspend setting, use pm_runtime_put_sync_suspend() instead.

Yes.. That works too. I guess the thing to consider is if we should make pm_runtime_put_sync() always sync along the lines of a patch I posted earlier today. That could avoid quite a bit of confusion as already seen in this thread :)

Quoted 7 lines · Ulf Hansson

Quoted 5 lines

I don't see the problem, but of course you know omap for better than I do.

So if you are concerned about this, perhaps adding a dev_dbg print when the omap hwmod's ->runtime_suspend() callback returns zero could

/s/runtime_suspend/runtime_resume

OK

Let me summarize my understanding of this thread so far.

It looks like the omap3 code initializes hardware in ->probe() and then it may return -EPROBE_DEFER due to some unmet dependencies. In that case the hardware is not reset to the previous state and the runtime PM framework is left in the state that corresponds to the current hardware state. Before we had pm_runtime_reinit(), everything worked as expected on the second ->probe() call, because things were in sync then.

The introduction of pm_runtime_reinit() changed the situation and now effectively the hardware is required to be reset to the initial state if -EPROBE_DEFER is to be returned too.

Quoted 9 lines

Let me summarize my understanding of this thread so far.

It looks like the omap3 code initializes hardware in ->probe() and then it may return -EPROBE_DEFER due to some unmet dependencies. In that case the hardware is not reset to the previous state and the runtime PM framework is left in the state that corresponds to the current hardware state. Before we had pm_runtime_reinit(), everything worked as expected on the second ->probe() call, because things were in sync then.

Correct!

Before pm_runtime_reinit(), the failing probe case (-EPROBE_DEFER) worked fine because the HW state and the runtime PM status was in sync. The device was powered and the runtime PM status was RPM_ACTIVE.

Quoted 4 lines

The introduction of pm_runtime_reinit() changed the situation and now effectively the hardware is required to be reset to the initial state if -EPROBE_DEFER is to be returned too.

Not correct. The hardware doesn't need a reset as it stays powered after a failed probe.

Instead, only the runtime PM status needs to be synchronized with the HW state the next probe attempt.

In other words, when the PM domain's ->runtime_resume() callbacks gets called due to a pm_runtime_get_sync() in the second probe attempt, it may find that the HW is already powered and can return zero instead of resetting it.

Quoted 19 lines · Alan Stern

Quoted 16 lines · Tony Lindgren

Quoted 2 lines

Also, what is autosuspend_delay set to for your device? And is runtime_auto set?

It's 100 at that point, see the commented snippet below from omap_hsmmc_probe():

	pm_runtime_enable(host->dev);
	pm_runtime_get_sync(host->dev);
	pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY);
	/* NOTE: pm_runtime_dont_use_autosuspend(host->dev) needed here? */
	pm_runtime_use_autosuspend(host->dev);
	...
	/* gets -EPROBE_DEFER */
err_irq:
	...
	pm_runtime_put_sync(host->dev);

Quoted 4 lines

You could try changing this to pm_runtime_put_sync_suspend(). But putting pm_runtime_dont_use_autosuspend() before the put_sync seems like a perfectly reasonable thing to do, especially if you feel you should reverse all the changes you made at the start.

They both seem to fix the problem.

Quoted 2 lines

The reinit function gets called too late to do what you want -- namely, put the hardware in a low-power state.

Right, the problem is the lack of suspend on the first probe. But for having autosuspend timeout long enough for the next probe would mean that we can't reset the PM runtime state in between.

Quoted 6 lines

pm_runtime_put_sync() is supposed to follow the driver's wishes. It invokes the driver's runtime_idle callback if there is one, and the callback routine can start a suspend or an autosuspend. If there is no callback, it will use whatever autosuspend setting the driver has set up. If you want to override the autosuspend setting, use pm_runtime_put_sync_suspend() instead.

Yes.. That works too. I guess the thing to consider is if we should make pm_runtime_put_sync() always sync along the lines of a patch I posted earlier today. That could avoid quite a bit of confusion as already seen in this thread :)

Quoted 10 lines · Ulf Hansson

Quoted 9 lines

Let me summarize my understanding of this thread so far.

It looks like the omap3 code initializes hardware in ->probe() and then it may return -EPROBE_DEFER due to some unmet dependencies. In that case the hardware is not reset to the previous state and the runtime PM framework is left in the state that corresponds to the current hardware state. Before we had pm_runtime_reinit(), everything worked as expected on the second ->probe() call, because things were in sync then.

Quoted 1 line

Correct!

Quoted 11 lines

Before pm_runtime_reinit(), the failing probe case (-EPROBE_DEFER) worked fine because the HW state and the runtime PM status was in sync. The device was powered and the runtime PM status was RPM_ACTIVE.

Quoted 4 lines

The introduction of pm_runtime_reinit() changed the situation and now effectively the hardware is required to be reset to the initial state if -EPROBE_DEFER is to be returned too.

Not correct. The hardware doesn't need a reset as it stays powered after a failed probe.

Quoted 2 lines

Instead, only the runtime PM status needs to be synchronized with the HW state the next probe attempt.

In other words, the probe routine assumes the actual state is the same as the PM status. This may have been true before pm_runtime_reinit() came along, but it's not true now.

Quoted 4 lines

In other words, when the PM domain's ->runtime_resume() callbacks gets called due to a pm_runtime_get_sync() in the second probe attempt, it may find that the HW is already powered and can return zero instead of resetting it.

What's wrong with going ahead and resetting the hardware anyway?

Quoted 12 lines · Alan Stern

Quoted 10 lines · Ulf Hansson

Quoted 9 lines

Let me summarize my understanding of this thread so far.

It looks like the omap3 code initializes hardware in ->probe() and then it may return -EPROBE_DEFER due to some unmet dependencies. In that case the hardware is not reset to the previous state and the runtime PM framework is left in the state that corresponds to the current hardware state. Before we had pm_runtime_reinit(), everything worked as expected on the second ->probe() call, because things were in sync then.

Quoted 1 line

Quoted 1 line

Correct!

Well not quite correct. After failed probe PM runtime is set to reset state while hardware is still enabled.

Quoted 10 lines

Quoted 10 lines

Before pm_runtime_reinit(), the failing probe case (-EPROBE_DEFER) worked fine because the HW state and the runtime PM status was in sync. The device was powered and the runtime PM status was RPM_ACTIVE.

Quoted 4 lines

The introduction of pm_runtime_reinit() changed the situation and now effectively the hardware is required to be reset to the initial state if -EPROBE_DEFER is to be returned too.

Quoted 2 lines

Quoted 2 lines

Not correct. The hardware doesn't need a reset as it stays powered after a failed probe.

It is really best to disable the hardware after a failed probe like we do with pm_runtime_put_sync_(suspend)() and pm_runtime_disable().

This is because there may never be another probe attempt and we want to have unclaimed devices shut off (or idled) for PM.

Quoted 12 lines

Quoted 3 lines

Instead, only the runtime PM status needs to be synchronized with the HW state the next probe attempt.

In other words, the probe routine assumes the actual state is the same as the PM status. This may have been true before pm_runtime_reinit() came along, but it's not true now.

Quoted 4 lines

In other words, when the PM domain's ->runtime_resume() callbacks gets called due to a pm_runtime_get_sync() in the second probe attempt, it may find that the HW is already powered and can return zero instead of resetting it.

Certainly that should at least produce a warning in the hardware specific implementation. It's a state mismatch between PM runtime and the hardware specific implementation.

And as discussed, the problem does not exist as long as we understand that we need to use pm_runtime_put_sync_suspend() if the driver has set pm_runtime_use_autoidle(). Or else use the combination of pm_runtime_dont_use_autoidle() and pm_runtime_put_sync().

Quoted 2 lines

What's wrong with going ahead and resetting the hardware anyway?

Nothing, unless a state needs to be maintained for things like GPIO pins power memory :)

Regards,

Tony

Quoted 2 lines · Tony Lindgren

Well we actually pretty much have devices in that state to start with.

That's not true, not even for omap.

There are definitely devices which HW state is reflected by RPM_ACTIVE at boot. It's the responsible of the subsystem/driver (including PM domains) to make sure the runtime PM status is updated accordingly, to reflect the real HW state.

For omap hwmod, at device registration in omap_device_build_from_dt() it may actually invoke pm_runtime_set_active() if the device is enabled. To my understanding, that's done to synchronize the real HW state with the runtime PM status, right?

Quoted 5 lines

Quoted 2 lines

To me, it's the responsible of the PM domain to *help* with the synchronization, not prevent it as it currently does.

The problem is that the hardware state gets out of sync with PM runtime. And that's going to be a pain to debug later on.

I don't see the problem, but of course you know omap for better than I do.

So if you are concerned about this, perhaps adding a dev_dbg print when the omap hwmod's ->runtime_suspend() callback returns zero could be a way forward?

Quoted 5 lines · Tony Lindgren

Yes.. That works too. I guess the thing to consider is if we should make pm_runtime_put_sync() always sync along the lines of a patch I posted earlier today. That could avoid quite a bit of confusion as already seen in this thread :)

No, we shouldn't.

As I said, the current behavior is actually well documented (in kerneldoc comments and elsewhere).

Quoted 20 lines · Tony Lindgren

Quoted 19 lines · Alan Stern

Quoted 16 lines · Tony Lindgren

Quoted 2 lines

Also, what is autosuspend_delay set to for your device? And is runtime_auto set?

It's 100 at that point, see the commented snippet below from omap_hsmmc_probe():

	pm_runtime_enable(host->dev);
	pm_runtime_get_sync(host->dev);
	pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY);
	/* NOTE: pm_runtime_dont_use_autosuspend(host->dev) needed here? */
	pm_runtime_use_autosuspend(host->dev);
	...
	/* gets -EPROBE_DEFER */
err_irq:
	...
	pm_runtime_put_sync(host->dev);

Quoted 6 lines

Quoted 4 lines

You could try changing this to pm_runtime_put_sync_suspend(). But putting pm_runtime_dont_use_autosuspend() before the put_sync seems like a perfectly reasonable thing to do, especially if you feel you should reverse all the changes you made at the start.

They both seem to fix the problem.

So you could use either one. In my opinion, the pm_runtime_dont_use_autosuspend() solution is a little cleaner.

Quoted 6 lines

Quoted 2 lines

The reinit function gets called too late to do what you want -- namely, put the hardware in a low-power state.

Right, the problem is the lack of suspend on the first probe. But for having autosuspend timeout long enough for the next probe would mean that we can't reset the PM runtime state in between.

That's one way to look at it. But in principle you don't need to suspend the device after an unsuccessful probe. You can just leave it at high power. If this causes problems for a second probe, it's the second probe's own fault for assuming the actual state matches the PM status.

Quoted 11 lines

Quoted 6 lines

pm_runtime_put_sync() is supposed to follow the driver's wishes. It invokes the driver's runtime_idle callback if there is one, and the callback routine can start a suspend or an autosuspend. If there is no callback, it will use whatever autosuspend setting the driver has set up. If you want to override the autosuspend setting, use pm_runtime_put_sync_suspend() instead.

Yes.. That works too. I guess the thing to consider is if we should make pm_runtime_put_sync() always sync along the lines of a patch I posted earlier today. That could avoid quite a bit of confusion as already seen in this thread :)

As Rafael pointed out, pm_runtime_put_sync() has well-documented behavior. It shouldn't be changed. I don't see how changing the behavior would reduce anybody's confusion. At least, anybody who reads the documentation carefully.

Quoted 59 lines · Tony Lindgren

Quoted 11 lines · Ulf Hansson

Quoted 4 lines

? pm_runtime_put_sync() _already_ does not respect the autosuspend mode. If you want to respect it, you have to call pm_runtime_put_sync_autosuspend() instead.

Then there's a bug in the runtime PM core.

From Tony's regression report and from mine own local runtime PM test driver, I can see that the device doesn't get RPM_SUSPENDED (the ->runtime_suspend() callback isn't called), even when the usage count is zero - when pm_runtime_put_sync() is called.

...

Quoted 3 lines

Okay, so you are saying that the pm_runtime_put_sync() should idle the device even if autosuspend is in use. That seems reasonable, I will look into this problem.

The patch below fixes pm_runtime_put_sync() to not respect the autosuspend mode to match what Alan is saying above. Seems to also fixes the $subject issue for me. And seems to behave for PM runtime for other devices during runtime too based on light testing here.

Regards,

Tony

8< ---------------

drivers/base/power/runtime.c +3 -1

@@ -353,7 +353,9 @@ static int rpm_idle(struct device *dev, int rpmflags)
 
  out:
        trace_rpm_return_int(dev, _THIS_IP_, retval);
-       return retval ? retval : rpm_suspend(dev, rpmflags | RPM_AUTO);
+       if (!(rpmflags & RPM_IGNORE_AUTO))
+               rpmflags |= RPM_AUTO;
+       return retval ? retval : rpm_suspend(dev, rpmflags);
 }
 
 /**

include/linux/pm_runtime.h +3 -1

@@ -23,6 +23,8 @@
                                            usage_count */
 #define RPM_AUTO               0x08    /* Use autosuspend_delay */
 
+#define RPM_IGNORE_AUTO                0x10    /* Ignore autosuspend */
+
 #ifdef CONFIG_PM
 extern struct workqueue_struct *pm_wq;
 

@@ -241,7 +243,7 @@ static inline int pm_runtime_put_autosuspend(struct device *dev)
 
 static inline int pm_runtime_put_sync(struct device *dev)
 {
-       return __pm_runtime_idle(dev, RPM_GET_PUT);
+       return __pm_runtime_idle(dev, RPM_GET_PUT | RPM_IGNORE_AUTO);
 }
 

include/linux/pm_runtime.h static inline int pm_runtime_put_autosuspend(struct device *dev) · line 249

Quoted 1 line

include/linux/pm_runtime.h +0 -0

 static inline int pm_runtime_put_sync_suspend(struct device *dev)

This changes a well-documented behavior that someone may be relying on.

Not the safest thing to do I have to say.

Okay, so I understand that you decided to not pick up the omap hwmod patch I posted.

If you want some further help in fixing the omap drivers, please just tell me I am at your service. :-)

Also, we must not forget to also update their runtime PM calls in their ->remove() callbacks, as those seems to suffer from the same problem as in the -EPROBE_DEFER case.

Quoted 13 lines · Tony Lindgren

Quoted 12 lines · Alan Stern

Quoted 10 lines · Ulf Hansson

Quoted 9 lines

Let me summarize my understanding of this thread so far.

It looks like the omap3 code initializes hardware in ->probe() and then it may return -EPROBE_DEFER due to some unmet dependencies. In that case the hardware is not reset to the previous state and the runtime PM framework is left in the state that corresponds to the current hardware state. Before we had pm_runtime_reinit(), everything worked as expected on the second ->probe() call, because things were in sync then.

Quoted 4 lines

Quoted 1 line

Quoted 1 line

Correct!

Well not quite correct. After failed probe PM runtime is set to reset state while hardware is still enabled.

Yes, but that's *after* pm_runtime_reinit() was added.

I think Rafael was thinking about how it worked *before*.

Quoted 19 lines · Alan Stern

Quoted 16 lines · Tony Lindgren

Quoted 2 lines

Also, what is autosuspend_delay set to for your device? And is runtime_auto set?

It's 100 at that point, see the commented snippet below from omap_hsmmc_probe():

    pm_runtime_enable(host->dev);
    pm_runtime_get_sync(host->dev);
    pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY);
    /* NOTE: pm_runtime_dont_use_autosuspend(host->dev) needed here? */
    pm_runtime_use_autosuspend(host->dev);
    ...
    /* gets -EPROBE_DEFER */
err_irq:
    ...
    pm_runtime_put_sync(host->dev);

Quoted 4 lines

You could try changing this to pm_runtime_put_sync_suspend(). But putting pm_runtime_dont_use_autosuspend() before the put_sync seems like a perfectly reasonable thing to do, especially if you feel you should reverse all the changes you made at the start.

Quoted 20 lines · Tony Lindgren

Quoted 19 lines · Alan Stern

Quoted 16 lines · Tony Lindgren

Quoted 2 lines

Also, what is autosuspend_delay set to for your device? And is runtime_auto set?

It's 100 at that point, see the commented snippet below from omap_hsmmc_probe():

    pm_runtime_enable(host->dev);
    pm_runtime_get_sync(host->dev);
    pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY);
    /* NOTE: pm_runtime_dont_use_autosuspend(host->dev) needed here? */
    pm_runtime_use_autosuspend(host->dev);
    ...
    /* gets -EPROBE_DEFER */
err_irq:
    ...
    pm_runtime_put_sync(host->dev);

Quoted 4 lines

Quoted 4 lines

You could try changing this to pm_runtime_put_sync_suspend(). But putting pm_runtime_dont_use_autosuspend() before the put_sync seems like a perfectly reasonable thing to do, especially if you feel you should reverse all the changes you made at the start.

FWIW, I'd call pm_runtime_dont_use_autosuspend() before put_sync().

After all, the driver doesn't want to use autosuspend going forward, so stating that explicitly looks like the right thing to do.

Quoted 26 lines · Rafael J. Wysocki

Quoted 20 lines · Tony Lindgren

Quoted 19 lines · Alan Stern

Quoted 16 lines · Tony Lindgren

Quoted 2 lines

Also, what is autosuspend_delay set to for your device? And is runtime_auto set?

It's 100 at that point, see the commented snippet below from omap_hsmmc_probe():

    pm_runtime_enable(host->dev);
    pm_runtime_get_sync(host->dev);
    pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY);
    /* NOTE: pm_runtime_dont_use_autosuspend(host->dev) needed here? */
    pm_runtime_use_autosuspend(host->dev);
    ...
    /* gets -EPROBE_DEFER */
err_irq:
    ...
    pm_runtime_put_sync(host->dev);

Quoted 4 lines

Quoted 4 lines

You could try changing this to pm_runtime_put_sync_suspend(). But putting pm_runtime_dont_use_autosuspend() before the put_sync seems like a perfectly reasonable thing to do, especially if you feel you should reverse all the changes you made at the start.

Quoted 4 lines

FWIW, I'd call pm_runtime_dont_use_autosuspend() before put_sync().

After all, the driver doesn't want to use autosuspend going forward, so stating that explicitly looks like the right thing to do.

Yeah agreed. FYI, this is what I typed up here into a commit message:

1. For sections of code that needs the device disabled, use pm_runtime_put_sync_suspend() if pm_runtime_set_autosuspend() has been set.

2. For driver exit code, use pm_runtime_dont_use_autosuspend() before pm_runtime_put_sync() if pm_runtime_use_autosuspend() has been set.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

To me, it's the responsible of the PM domain to *help* with the synchronization, not prevent it as it currently does.

The problem is that the hardware state gets out of sync with PM runtime. And that's going to be a pain to debug later on.

I don't see the problem, but of course you know omap for better than I do.

Well there's also the long term maintenance aspect at least I need to consider.

Quoted 3 lines

So if you are concerned about this, perhaps adding a dev_dbg print when the omap hwmod's ->runtime_suspend() callback returns zero could be a way forward?

If we downgrade it to a debug statement or a warning, we'll soon end up with even more driver specific warnings than we already have. And I don't want to be chasing people around to fix their drivers for eavery new driver that gets submitted.

Also, without this error I would not even originally have noticed we have a problem :) So I suggest the following:

1. I'll do a series of patches to fix up the handful of omap specific drivers with pm_runtime_use_autosuspend() that depend on omap_device

2. I'll also do a patch to improve the omap_device error message so new drivers are easy to fix. Something like:

"%() called from invalid state %d, use pm_runtime_put_sync_suspend()?"

Does that sounds OK to you?

Regards,

Tony

One more thing though. I just realized that you have yet another issue to consider going for the approach fixing *only* drivers.

Let me summarize it here:

If userspace has prevented runtime PM (pm_runtime_forbid()) when a driver becomes unbound, the driver will not be able to suspend the device by using any of the pm_runtime_suspend() APIs, as the usage count is isn't zero.

As pm_runtime_reinit() is invoked as part of the driver unbind sequence, the runtime PM status goes out of sync. A following driver rebind will then trigger the warning when the PM domain's ->runtime_resume() callback gets invoked. Again, forever preventing the device from being runtime suspended.

How do you intend to solve this case?
I guess there are two options, pick up the patch I posted for omap hwmod or make use of pm_runtime_force_suspend() in the driver.

Quoted 11 lines · Ulf Hansson

One more thing though. I just realized that you have yet another issue to consider going for the approach fixing *only* drivers.

Let me summarize it here:

If userspace has prevented runtime PM (pm_runtime_forbid()) when a driver becomes unbound, the driver will not be able to suspend the device by using any of the pm_runtime_suspend() APIs, as the usage count is isn't zero.

Quoted 5 lines

As pm_runtime_reinit() is invoked as part of the driver unbind sequence, the runtime PM status goes out of sync. A following driver rebind will then trigger the warning when the PM domain's ->runtime_resume() callback gets invoked. Again, forever preventing the device from being runtime suspended.

Hmm yeah that's a good point.

Quoted 3 lines

How do you intend to solve this case?
I guess there are two options, pick up the patch I posted for omap hwmod or make use of pm_runtime_force_suspend() in the driver.

My gut feeling right now is we should just have BUS_NOTIFY_UNBIND_DRIVER shut down the device on the interconnect automatically as it's unused after the driver has unloaded :)

Quoted 20 lines · Tony Lindgren

Quoted 11 lines · Ulf Hansson

One more thing though. I just realized that you have yet another issue to consider going for the approach fixing *only* drivers.

Let me summarize it here:

If userspace has prevented runtime PM (pm_runtime_forbid()) when a driver becomes unbound, the driver will not be able to suspend the device by using any of the pm_runtime_suspend() APIs, as the usage count is isn't zero.

Quoted 5 lines

As pm_runtime_reinit() is invoked as part of the driver unbind sequence, the runtime PM status goes out of sync. A following driver rebind will then trigger the warning when the PM domain's ->runtime_resume() callback gets invoked. Again, forever preventing the device from being runtime suspended.

Hmm yeah that's a good point.

Quoted 7 lines

Quoted 3 lines

How do you intend to solve this case?
I guess there are two options, pick up the patch I posted for omap hwmod or make use of pm_runtime_force_suspend() in the driver.

My gut feeling right now is we should just have BUS_NOTIFY_UNBIND_DRIVER shut down the device on the interconnect automatically as it's unused after the driver has unloaded :)

BUS_NOTIFY_UNBIND_DRIVER is sent prior the ->remove() callbacks is invoked from driver core.
So if the driver requires to do a pm_runtime_get_sync() during ->remove() callback, this won't work.

BUS_NOTIFY_UNBOUND_DRIVER may work though.

Quoted 34 lines · Ulf Hansson

Quoted 20 lines · Tony Lindgren

Quoted 11 lines · Ulf Hansson

One more thing though. I just realized that you have yet another issue to consider going for the approach fixing *only* drivers.

Let me summarize it here:

If userspace has prevented runtime PM (pm_runtime_forbid()) when a driver becomes unbound, the driver will not be able to suspend the device by using any of the pm_runtime_suspend() APIs, as the usage count is isn't zero.

Quoted 5 lines

As pm_runtime_reinit() is invoked as part of the driver unbind sequence, the runtime PM status goes out of sync. A following driver rebind will then trigger the warning when the PM domain's ->runtime_resume() callback gets invoked. Again, forever preventing the device from being runtime suspended.

Hmm yeah that's a good point.

Quoted 7 lines

Quoted 3 lines

How do you intend to solve this case?
I guess there are two options, pick up the patch I posted for omap hwmod or make use of pm_runtime_force_suspend() in the driver.

My gut feeling right now is we should just have BUS_NOTIFY_UNBIND_DRIVER shut down the device on the interconnect automatically as it's unused after the driver has unloaded :)

BUS_NOTIFY_UNBIND_DRIVER is sent prior the ->remove() callbacks is invoked from driver core.
So if the driver requires to do a pm_runtime_get_sync() during ->remove() callback, this won't work.

Quoted 1 line

BUS_NOTIFY_UNBOUND_DRIVER may work though.

Right sorry that's what I meant. Naturally we can't do it before remove :)

I'll take a look.

Quoted 20 lines · Tony Lindgren

Quoted 19 lines · Alan Stern

Quoted 16 lines · Tony Lindgren

Quoted 2 lines

Also, what is autosuspend_delay set to for your device? And is runtime_auto set?

It's 100 at that point, see the commented snippet below from omap_hsmmc_probe():

    pm_runtime_enable(host->dev);
    pm_runtime_get_sync(host->dev);
    pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY);
    /* NOTE: pm_runtime_dont_use_autosuspend(host->dev) needed here? */
    pm_runtime_use_autosuspend(host->dev);
    ...
    /* gets -EPROBE_DEFER */
err_irq:
    ...
    pm_runtime_put_sync(host->dev);

Quoted 4 lines

Quoted 4 lines

You could try changing this to pm_runtime_put_sync_suspend(). But putting pm_runtime_dont_use_autosuspend() before the put_sync seems like a perfectly reasonable thing to do, especially if you feel you should reverse all the changes you made at the start.

FWIW, I'd call pm_runtime_dont_use_autosuspend() before put_sync().

After all, the driver doesn't want to use autosuspend going forward, so stating that explicitly looks like the right thing to do.

Just wanted to add yet some new findings while I was looking into this regression.

So, the reason why pm_runtime_put_sync() doesn't idle the device in these cases, is because autosuspend is respected and for some reason the autosuspend timer hasn't expired.

I was wondering *why* the timer isn't considered as expired, because the driver don't call pm_runtime_mark_last_busy() in the failing probe case...

Then I realized the following commit was merged a few releases ago, which makes the runtime PM core to invoke pm_runtime_mark_last_busy() for us.

commit 56f487c78015936097474fd89b2ccb229d500d0f
Author: Tony Lindgren <[email protected]>
Date: Wed May 13 16:36:32 2015 -0700
PM / Runtime: Update last_busy in rpm_resume

So, this commit actually causes the devices to stay active after a failed probe attempt.

I think it's a good idea to revert this change, but what to you think?

Quoted 6 lines · Ulf Hansson

Just wanted to add yet some new findings while I was looking into this regression.

So, the reason why pm_runtime_put_sync() doesn't idle the device in these cases, is because autosuspend is respected and for some reason the autosuspend timer hasn't expired.

No doubt the autosuspend delay is longer than the time it takes for a probe to fail.

Quoted 16 lines

I was wondering *why* the timer isn't considered as expired, because the driver don't call pm_runtime_mark_last_busy() in the failing probe case...

Then I realized the following commit was merged a few releases ago, which makes the runtime PM core to invoke pm_runtime_mark_last_busy() for us.

commit 56f487c78015936097474fd89b2ccb229d500d0f
Author: Tony Lindgren <[email protected]>
Date: Wed May 13 16:36:32 2015 -0700
PM / Runtime: Update last_busy in rpm_resume

So, this commit actually causes the devices to stay active after a failed probe attempt.

Quoted 1 line

I think it's a good idea to revert this change, but what to you think?

I disagree. The whole idea of autosuspend is to prevent the device's power state from bouncing up and down too rapidly. This implies that whenever the device gets resumed, we should wait at least the minimum autosuspend delay before allowing the next autosuspend.

Quoted 6 lines · Ulf Hansson

Just wanted to add yet some new findings while I was looking into this regression.

So, the reason why pm_runtime_put_sync() doesn't idle the device in these cases, is because autosuspend is respected and for some reason the autosuspend timer hasn't expired.

No doubt the autosuspend delay is longer than the time it takes for a probe to fail.

Quoted 16 lines

I was wondering *why* the timer isn't considered as expired, because the driver don't call pm_runtime_mark_last_busy() in the failing probe case...

Then I realized the following commit was merged a few releases ago, which makes the runtime PM core to invoke pm_runtime_mark_last_busy() for us.

commit 56f487c78015936097474fd89b2ccb229d500d0f
Author: Tony Lindgren <[email protected]>
Date: Wed May 13 16:36:32 2015 -0700
PM / Runtime: Update last_busy in rpm_resume

So, this commit actually causes the devices to stay active after a failed probe attempt.

Quoted 1 line

I think it's a good idea to revert this change, but what to you think?

I disagree. The whole idea of autosuspend is to prevent the device's power state from bouncing up and down too rapidly. This implies that whenever the device gets resumed, we should wait at least the minimum autosuspend delay before allowing the next autosuspend.

Perhaps you think that it's silly to behave that way in this case, because the device wasn't accessed at all during the time it was at full power. That's a valid objection, but the proper solution is not to revert the 56f487c78015 commit. Rather, change the driver to avoid doing a pm_runtime_resume_sync until you _know_ that the device will be accessed soon.

I am really not questioning the autosuspend feature at all, it's a really great feature!

Now, I question the minor benefit we actually gain from having the runtime PM core to update the mark in rpm_resume().

To me, the best decision when to update the mark is know by the driver/subsystem for the device and not the core.

In most cases the mark will be updated after a request has been completed, which leads to one unnecessary update at rpm_resume().

In this path (the resume), you really want to keep latencies to a minimum and for sure not do unnecessary things.

Quoted 7 lines

Perhaps you think that it's silly to behave that way in this case, because the device wasn't accessed at all during the time it was at full power. That's a valid objection, but the proper solution is not to revert the 56f487c78015 commit. Rather, change the driver to avoid doing a pm_runtime_resume_sync until you _know_ that the device will be accessed soon.

That's not always going to work.

Sometimes you need to access the device when trying to probe. Failing later in probe, shows just *one* case where it doesn't make sense to update the last busy mark. I suspect there may be other cases as well.

Of course one can always use runtime PM APIs which overrides the autosuspend mode, so it's not a big deal.

Quoted 5 lines · Ulf Hansson

I am really not questioning the autosuspend feature at all, it's a really great feature!

Now, I question the minor benefit we actually gain from having the runtime PM core to update the mark in rpm_resume().

As Tony pointed out, it prevents some devices from going to sleep right away.

Quoted 6 lines

To me, the best decision when to update the mark is know by the driver/subsystem for the device and not the core.

In most cases the mark will be updated after a request has been completed, which leads to one unnecessary update at rpm_resume().

Sure, but that update is a simple assignment statement. It's about as cheap as you can get, short of doing nothing at all.

Quoted 5 lines · Alan Stern

Quoted 3 lines · Ulf Hansson

I am really not questioning the autosuspend feature at all, it's a really great feature!

Quoted 5 lines

Quoted 2 lines

Now, I question the minor benefit we actually gain from having the runtime PM core to update the mark in rpm_resume().

As Tony pointed out, it prevents some devices from going to sleep right away.

Because their drivers don't care to update the last busy mark!?

Quoted 9 lines · Ulf Hansson

Quoted 8 lines · Alan Stern

Quoted 5 lines · Ulf Hansson

I am really not questioning the autosuspend feature at all, it's a really great feature!

Now, I question the minor benefit we actually gain from having the runtime PM core to update the mark in rpm_resume().

Quoted 4 lines

Quoted 2 lines

As Tony pointed out, it prevents some devices from going to sleep right away.

Because their drivers don't care to update the last busy mark!?

Nope. Without that devices may never resume at all so the drivers can't do anything about it.

Quoted 13 lines · Tony Lindgren

Quoted 12 lines · Ulf Hansson

Quoted 10 lines · Alan Stern

Quoted 5 lines · Ulf Hansson

I am really not questioning the autosuspend feature at all, it's a really great feature!

Now, I question the minor benefit we actually gain from having the runtime PM core to update the mark in rpm_resume().

As Tony pointed out, it prevents some devices from going to sleep right away.

Quoted 4 lines

Quoted 1 line

Because their drivers don't care to update the last busy mark!?

Nope. Without that devices may never resume at all so the drivers can't do anything about it.

I don't get it. Why not? Because of another abuse of the runtime PM API?

Or we should probably continue to focus on fixing the regression. :-)