Nishanth Menon <nm@xxxxxx> writes: > On 11/07/2013 05:44 PM, Kevin Hilman wrote: >> Nishanth Menon <nm@xxxxxx> writes: >> >>> OMAP device hooks around suspend|resume_noirq ensures that hwmod >>> devices are forced to idle using omap_device_idle/enable as part of >>> the last stage of suspend activity. >>> >>> For a device such as i2c who uses autosuspend, it is possible to enter >>> the suspend path with dev->power.runtime_status = RPM_ACTIVE. >>> >>> As part of the suspend flow, the generic runtime logic would increment >>> it's dev->power.disable_depth to 1. This should prevent further >>> pm_runtime_get_sync from succeeding once the runtime_status has been >>> set to RPM_SUSPENDED. >>> >>> Now, as part of the suspend_noirq handler in omap_device, we force the >>> following: if the device status is !suspended, we force the device >>> to idle using omap_device_idle (clocks are cut etc..). This ensures >>> that from a hardware perspective, the device is "suspended". However, >>> runtime_status is left to be active. >>> >>> *if* an operation is attempted after this point to >>> pm_runtime_get_sync, runtime framework depends on runtime_status to >>> indicate accurately the device status, and since it sees it to be >>> ACTIVE, it assumes the module is functional and returns a non-error >>> value. As a result the user will see pm_runtime_get succeed, however a >>> register access will crash due to the lack of clocks. >> >> Ouch. >> >> Dumb Q: who is requesting an i2c transaction after ->suspend_noirq(). >> The i2c driver itself should be able to detect that it's being accessed >> after this point and return an error. > > i2c dropped it generic suspend handlers at > commit 584b408d37af4e0b38ad5b60f236381bcdf396bc > Author: Kevin Hilman <khilman@xxxxxx> sheesh, who is that crazy TI guy. They should probably fire him. > Date: Thu Aug 4 07:53:02 2011 -0700 > > Revert "i2c-omap: fix static suspend vs. runtime suspend" > > Also, as of v3.1, the PM domain level code for OMAP handles device > power state transistions automatically for devices, so drivers no > longer need to specifically call the bus/pm_domain methods themselves. > > > - So it is rightly in the mercy of runtime PM to adequately represent > it's state. I disagree that i2c driver should in addition have to > remember what it's generic suspend state is. That's debatable I guess. The ideal world is that runtime PM hides all of this, but I'm not sure it's achievable in all cases. > - See the link to the cpufreq and the logs to see the call stack > where it fails. > > Now, this is fine, since omap_i2c_xfer should ideally fail with a > pm_runtime_get_sync returning -EACCESS when device is really suspended > (remember, we are doing autosuspend - so, in the case I caught, there > was regulator voltage set prior to entering suspend, but timeout was > not yet hit for autosuspend of i2c to kick in yet). Ah, I see. Makes sense. >> >> That being said, I agree that omap_device should still be catching this >> case in order to find/fix driver races like this. > >> >>> To prevent this from happening, we should ensure that runtime_status >>> exactly indicates the device status. As a result of this change >>> any further calls to pm_runtime_get* would return -EACCES (since >>> disable_depth is 1). On resume, we restore the clocks and runtime >>> status exactly as we suspended with. >>> >>> Reported-by: J Keerthy <j-keerthy@xxxxxx> >>> Signed-off-by: Nishanth Menon <nm@xxxxxx> >>> Acked-by: Rajendra Nayak <rnayak@xxxxxx> >>> --- >>> patch baseline: V3.12 tag (also applies on linux-next next-20131107 tag) >>> >>> Logs from 3.12 based vendor kernel: >>> Before: http://pastebin.com/m5KxnB7a >>> After: http://pastebin.com/8AfX4e7r >>> >>> The discussion on cpufreq side of the story which triggered this scenario: >>> http://marc.info/?l=linux-pm&m=138263811321921&w=2 >>> >>> Tested on TI vendor kernel (with dt boot): >>> AM335x: evm, BBB, sk, BBW >>> OMAP5uEVM, DRA7-evm >>> >>> arch/arm/mach-omap2/omap_device.c | 16 ++++++++++++++-- >>> arch/arm/mach-omap2/omap_device.h | 1 + >>> 2 files changed, 15 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c >>> index b69dd9a..87ecbb0 100644 >>> --- a/arch/arm/mach-omap2/omap_device.c >>> +++ b/arch/arm/mach-omap2/omap_device.c >>> @@ -621,6 +621,13 @@ static int _od_suspend_noirq(struct device *dev) >>> >>> if (!ret && !pm_runtime_status_suspended(dev)) { >>> if (pm_generic_runtime_suspend(dev) == 0) { >>> + if (!pm_runtime_suspended(dev)) { >> >> Why the addition check for supended here? > > pm_runtime_status_suspended checks only the dev->power.runtime_status > but that may not truely indicate device was in previous use - in the > case of devices like i2c who depend on autosuspend. > > pm_runtime_suspended checks for dev->power.runtime_status == > RPM_SUSPENDED and disable_depth > >> >> This version (as opposed to the _status_suspended() version above will >> fail if runtime PM has been disabled from userspace (via >> /sys/devices/.../power/control), and will thus prevent low power states >> from being hit in suspend if runtime suspend has been disabled from >> userspace. That's a bug. > > yes, and so it should fail to achieve low power state. we explicitly > stated disable suspend, yet we go behind the runtime PM's back and > force disable the module clocks. now drivers should NOT know what the > state of the omap_device meddling is and should obey the configuration > and completely trust pm_runtime to accurately denote the module state. No, that sysfs knob is for disabling runtime PM. We still want the device to hit low-power state in system suspend. Solving that problem is half the reason we have this omap_device noirq mess in the first place. You need to test this by disabling runtime PM from userspace and ensure that the low power state is still hit during suspend. >> >>> + /* NOTE: *might* indicate driver race */ >> >> Yes, a driver race which should then be fixed in the driver. > > true if this is a non-autosuspend device, in auto suspend devices, > this could be a regular phenomenon if timeout is pretty large.. but > atleast that should allow debug. Agreed. I wasn't thinking about the autosuspend case. Thanks for clarifying. >> >>> + dev_dbg(dev, "%s: Force suspending\n", >>> + __func__); >>> + pm_runtime_set_suspended(dev); >>> + od->flags |= OMAP_DEVICE_SUSPEND_FORCED; >> >> Not sure why you need an additonal flag. Why not just always do this >> and use the existin OMAP_DEVICE_SUSPENDED flag. > > restore of runtime data structure state is only needed for specific > devices - not all.. The question is why do you a flag in addition to OMAP_DEVICE_SUSPEND. Whenever that flag is set, omap_device has gone behind your back, and the runtime PM status should be kept in sync. Kevin -- 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
