Re: [PATCH 3/4] OMAP: PM: use omap_device API for suspend/resume

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Kevin,

On Mon, 21 Jun 2010, Kevin Hilman wrote:

> Paul Walmsley <paul@xxxxxxxxx> writes:
> 
> > I guess the intent of your patch is to avoid having to patch in 
> > omap_device_{idle,enable}() into a bunch of driver DPM suspend/resume 
> > callbacks?  
> 
> Partially.  Actually, I think many (most?) drivers can get rid of
> static suspend/resume paths all together and just use runtime PM.
> 
> There are some cases though (MMC comes to mind, more on that below)
> where static suspend has some extra steps necessary and behaves
> differently than runtime PM.

It's not just MMC, any driver that can be in the middle of doing something 
during DPM ->suspend() will need to have DPM suspend code to stop what 
it's doing and quiesce the device before returning from the suspend 
callback.  Either that, or ->suspend() needs to return an error and block 
the system suspend process...

> > If so, some comments:
> > - dev_pm_ops->suspend_noirq() is intended for use on devices with
> >   shared interrupts.  
> 
> Just to clarify.  The functions I'm overriding here is the bus
> functions (bus->pm->[suspend|resume]_noirq(), not any driver functions

OK, I see that now - this code was confusing in the patch's 
platform_pm_suspend_noirq():

+       if (drv->pm) {
+               if (drv->pm->suspend_noirq)
+                       ret = drv->pm->suspend_noirq(dev);
+       }

This is already done by the DPM core in 
drivers/base/power/main.c:device_suspend_noirq() and will result in 
re-executing the driver's suspend_noirq function, which is potentially 
quite bad.  Same thing for platform_pm_resume_noirq() in the patch.

> Indeed, shared IRQs were an intended usage, but I don't see that as a
> requirement.  Indeed, Documentation/power/devices.txt even seems to
> suggest that the _noirq version is the place to turn the device "as
> off as possible:
> 
>     "For simple drivers, suspend might quiesce the device using class code
>     and then turn its hardware as "off" as possible during suspend_noirq"

Further down in that file, it says:

"2.  The suspend methods should quiesce the device to stop it from 
     performing I/O.  They also may save the device registers and put it into the
     appropriate low-power state, depending on the bus type the device
     is on, and they may enable wakeup events."

... and then:

"The majority of subsystems and device drivers need not implement 
 [the suspend_noirq] callback.  However, bus types allowing devices to 
 share interrupt vectors, like PCI, generally need it; otherwise a driver 
 might encounter an error during the suspend phase by fielding a shared 
 interrupt generated by some other device after its own device had been 
 set to low power."

> >   Right now the OMAP2+ codebase doesn't use any
> >   shared interrupts for on-chip devices, as far as I can see.  It
> >   looks like you use ->suspend_noirq() to ensure your code runs after
> >   the existing driver suspend functions.  
> 
> No. The primary reason for using _noirq() is that if any driver ever
> did use the _noirq() hooks (for any atomic activity, or late wakeup
> detection, etc.)  the device will still be accessible.
>
> >   Using ->suspend_noirq() for this purpose seems like a hack.  
> >   A better place for that code would be in a bus->pm->suspend()
> >   callback, which will run after the device's dev_pm_ops->suspend()
> >   callback.
> 
> I originally put it in bus->pm->suspend, but moved it to
> bus->pm->suspend_noirq() since I didn't do a full audit so see
> if anything was using the _noirq hooks.
> 
> If we want to have a requirement that no on-chip devices can use the
> _noirq hooks (or at least they can't touch hardware in those hooks)
> then I can move it back to bus->pm->suspend().

That sounds like the best argument for keeping these hooks in 
_noirq() -- some driver writer may be likely to use suspend_noirq() 
without understanding that they shouldn't... maybe we should add some code 
that looks for this and warns.

But thinking about it further, it also seems possible that a handful of 
OMAP drivers might use shared IRQs at some point in the future.  DSS comes 
to mind -- that really is a shared IRQ.  So, _noirq() is fine, then.

> But personally, I would see that as an artificial requirement based on 
> a very restrictive definiton of the _noirq() methods.

It's just the definition from the kernel documentation.

> > - It is not safe to call omap_device_{idle,enable}() from DPM
> >   callbacks as the patch does, because they will race with runtime PM
> >   calls to omap_device_{idle,enable}().  
> 
> No, they cannot race.  
> 
> Runtime PM transitions are prevented by the system PM core during a
> system PM transition.  The system suspend path does a pm_runtime_get()
> for each device being suspended, and then does a _put() upon resume.
> (see drivers/base/power/main.c, grep for pm_runtime_)

Yes, you are correct in terms of my original statement.  But the problem 
-- and the race -- that I did a poor job of describing is still present.

What if this pm_bus ->suspend_noirq() calls omap_device_idle() while other 
code in the driver is still in the middle of interacting with the device?  
Although that would certainly be a driver bug, I certainly wouldn't trust 
all of our existing driver DPM suspend* functions to adequately wait for 
in-progress operations to complete and block new operations from starting 
before returning.

We shouldn't idle (and potentially power off) a device unless we know its 
driver is done with it.  In an ideal world, this would always be taken 
care of by driver ->suspend()/->suspend_noirq() functions.  But we know 
this isn't always the case -- perhaps it's not even the case for most of 
the OMAP drivers.

We can use the device's runtime PM state to figure out whether the driver 
thinks it's done with the device.  Unfortunately, the existing Linux DPM 
suspend code makes it difficult to deal with this by calling 
pm_runtime_get_noresume() before entering suspend and never calling 
pm_runtime_put() until after resume -- no idea why.  That strikes me as a 
bug.  From a semantic perspective it is certainly confusing: the PM 
runtime implementation will think the device is still active after it's 
been suspended.  I wouldn't want the full Linux system suspend code to 
enter some low power state while some driver still thought its device 
should stay powered.

Anyway, given this strange behavior, I think there is probably a simple 
workaround.  Rather than calling omap_device_idle() in 
platform_pm_suspend_noirq(), how about calling pm_runtime_put_sync()?  It 
probably needs a comment to indicate that it's intended to balance the 
pm_runtime_get_noresume() that's in dpm_prepare().  Similarly it should be 
possible to replace the omap_device_enable() in platform_pm_resume_noirq() 
with pm_runtime_get_sync().  That way the pm_bus code will not call 
omap_device_idle() on a device that the driver has not yet indicated is 
safe to enter idle.

> >   (This is part of the reason
> >   why it's important to be anal about enforcing the
> >   omap_device_{enable,idle,shutdown}() state transition limitations -
> >   so we get warned early when these types of conflicts appear.)
> >
> >   omap_device*() calls should either be in runtime PM functions, or
> >   DPM ->suspend/resume() callbacks, but not both.  
> 
> Why not both?  I don't follow the reason for this restriction.  We
> certainly did the equivalent clock enable/disable calls in both cases
> in the past.

clk_enable()/clk_disable() use-count, unlike the omap_device_idle() 
function.  So if an existing driver calls clk_enable()/clk_disable() in 
its suspend function, if other driver code had been previously entered 
that called clk_enable(), the kernel will not try to disable the clock 
until that code completes.

Also, clk_disable()/clk_enable() never touched the OCP_SYSCONFIG.*IDLEMODE 
bits.  So even if clk_disable() told the PRCM to turn the device clock 
off, the PRCM might not actually turn the clock off if the device doesn't 
idle-ack, which would prevent the enclosing powerdomain from actually 
turning off and destroying device context.  But with omap_device_idle(), 
if we have any hwmods that have the HWMOD_SWSUP_SIDLE/HWMOD_SWSUP_MSTANDBY 
flags set, those hwmods are going to be force-idled.  The PRCM will shut 
off their clocks and potentially cause device context to be lost if the 
driver wasn't expecting it.

> >   Since we've decided
> >   to focus on runtime PM power management as our primary driver power
> >   management approach, and we expect drivers to aggressively idle
> >   their devices, it seems best to keep the omap_device*() functions in
> >   runtime PM code.
> 
> I agree, mostly.
> 
> As I mentioned above, I expect most drivers not to even have system PM
> methods implemented.  They should implement everything as runtime PM.
> However, there will be some exceptions.
> 
> The use case that brought this patch into existence was the MMC driver
> where the suspend hook had to do some extra "stuff" before suspending.
> Even if already runtime suspended, the MMC suspend hook has to
> re-enable the device do "stuff" and then re-idle the device.

I don't think it's just MMC; any driver that does external I/O will 
probably need a DPM ->suspend() function to ensure that its external I/O 
is completed before returning from ->suspend().  Even drivers that are 
just doing some internal device work will need to know somehow that the 
device is finished before completing ->suspend().

> Initially, I tried to just use the same runtime PM calls in the suspend 
> hook, but that doesn't work since runtime PM transitions are prevented 
> during system PM.  So, I was forced to call omap_device_idle() in the 
> suspend path, which led to the decision to move that into common code.

Thanks, I see that now.  I think as long as the pm_bus common code ensures 
that the PM runtime system thinks that the device is idle, it should be 
okay to have this in common code.

> >   At that point, the common DPM suspend/idle callbacks that you
> >   propose can simply block until runtime PM indicates that the device
> >   is idle.  
> >
> >   For example, you could use a per-omap_device mutex.
> >   A sketch of a sample implementation follows this message.
> 
> That is an option, but since there is no potential racing between
> runtime PM and system PM, I don't see the need for extra locking.

Yep, but as mentioned above, we still need to check to see if the driver 
thinks that the device is in use before calling omap_device_idle().  
(Preferably it should just use the PM runtime put code for this rather 
than calling omap_device_idle() directly).

> Also, in your example, by manually [dis|en]abling IRQs, you're
> re-inventing the _noirq versions of the hooks, even though the DPM
> core is going to do it (again) soon.
>
> I'd rather just use the noirq hooks that are already in place, and let
> the driver manage IRQs itself.  Especially when it comes to wakeup
> IRQs, I think it problematic to start managing IRQs in common code.

Yeah, that part of the suggestion wasn't right.


- Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux