On Saturday, December 18, 2010, Alan Stern wrote: > On Sat, 18 Dec 2010, Ohad Ben-Cohen wrote: > > > > Not so. Even if a driver uses runtime PM correctly, there will still > > > be times when someone else has increased the usage_count. This happens > > > during probing and during system resume, for example. > > > > I'm aware of these two examples; normally we're good with them since > > during probing we're not toggling the power, and during suspend/resume > > the SDIO core is responsible for manipulating the power (and it does > > so directly). Are there (or do you think there will be) additional > > examples where this can happen ? > > I can't think of any other examples. But it's possible that more will > be added; there is no commitment to avoid this. echo "on" > /sys/devices/.../power/control from user space. You can't possibly override that. > > But this leads me to a real problem which we have encountered. > > > > During system suspend, our driver is asked (by mac80211's suspend > > handler) to power off its device. When this happens, the driver has no > > idea that the system is suspending - regular driver code (responsible > > to remove the wlan interface and stop the device) is being called. > > Obviously pm_runtime_put_sync() won't power off the device at this > > point, but later during suspend, when the SDIO core will suspend, the > > device will be powered off and things would work as expected. > > This descripion is very confusing. You seem to be talking about two > different "power down" operations (one carried out by the driver and > one carried out by the SDIO core), where one of them really does remove > power from the device and the other doesn't. Or maybe one reduces the > power to an intermediate level and the other reduces it a lot more. I > can't tell what you actually mean. > > > That breaks when the suspend transition is cancelled (e.g. due to an > > error) before SDIO core gets the chance to power off the device: the > > driver will be asked (by mac80211's resume handler) to power up the > > device and reinitialize it, but since the device was never powered > > off, the driver will fail doing so because the device is quiescent (a > > power cycle should have put him into a communicable state, but that > > never happened in this scenario). > > To summarize my understanding: After the device was suspended by the > driver, the SDIO core may or may not have powered it off and back on. > The driver doesn't know, but it needs to know in order to resume the > device. Right? > > Therefore the problem is that the SDIO core doesn't tell the driver > when it powers down the device. That should be easy to fix -- just add > a flag in a shared structure that can be set when a power-down occurs. > > > At that resume point the device is always on - whether the system > > suspended successfully or not - and the driver can't tell whether the > > device was indeed powered off beforehand or not. In addition, the > > driver code that is going to fail is not a resume handler - it's just > > regular driver code responsible for powering on the device and > > reinitializing it (which is being called by mac80211's resume > > handler). > > > > One way to solve this is by allowing certain type of devices to keep > > using runtime PM during system suspend transitions. Obviously this is > > generally unsafe, but for certain drivers/devices, that use runtime PM > > very accurately, synchronously and without being affected by children > > devices, this may be helpful (and those drivers should be responsible > > to make sure they are not racing with system suspend). > > ... > > > What do you think ? > > I think it is a bad idea. It's an attempt to complicate the PM core in > order to cover up a deficiency in the SDIO core. I agree. > > Otherwise, the driver will need to use some combination of both > > runtime PM API and direct calls to the ->runtime_suspend() handler. > > I don't see why. Maybe it would help if you explained in more detail > how the driver's runtime_suspend and runtime_resume routines get used. > > > The end result will be the same, but it won't be pretty. > > Or, it may not be as ugly as you think. Hard for me to tell, without > knowing how it all hangs together. > > > In addition, others with a similar problem might eventually show up, > > so supporting this within the runtime PM framework itself will help > > them too. > > The underlying problem seems to be that the driver doesn't know what to > do when it wants to resume the device, because it doesn't know what has > happened while the device was suspended. To put this another way, the > SDIO core makes significant changes to the device's state without > informing the driver. This is a self-evident bug. If the driver was > told what had happened then it could simply do whatever was needed. > > > > >> if a context is willing to > > >> synchronously suspend a device (either a driver using put_sync() or > > >> the PM worker), what do we gain by deferring the idling of the parent > > >> and not synchronously suspending as much ancestors as possible in the > > >> same context ? > > > > > > We gain stack space (by not having a potentially lengthy sequence of > > > nested calls), which is relatively unimportant > > > > Agree, if that was a problem, resume wouldn't work too, since it is > > completely symmetric. > > It isn't. The suspend path uses more stack space, since it involves > calling both the runtime_idle and runtime_suspend routines at each > step, whereas the resume path calls only the runtime_resume routine. > > > >, and we gain latency. > > > The latency may help in some situations; if the device is resumed again > > > quickly enough then we may avoid suspending the parent entirely. > > > > I guess it depends on the implementation, but if that parent cares > > about wake-up latency, couldn't it set its ->runtime_idle() handler to > > call pm_runtime_autosuspend() (and set autosuspend appropriately of > > course) instead of pm_runtime_suspend() ? > > Sure. > > > This way it'd be both possible to suspend devices as fast as possible, > > in a symmetric fashion to the way resume works, and also, if > > subsystems care, they could use autosuspend to explicitly indicate the > > period of inactivity that suits them and completely change this > > behavior. > > You are overlooking the fact that in our model, runtime suspend and > resume are _not_ symmetric. The runtime PM model is demand driven; it > wants to keep devices at full power whenever they are needed and allows > them to suspend only when they aren't needed (and hopefully aren't > going to be needed in the near future). In this model it is imperative > not to delay resumes, but it often is a good idea to delay suspends. > That's why, for example, the API includes pm_schedule_suspend() and > pm_runtime_autosuspend() but not pm_schedule_resume() or > pm_runtime_autoresume(). It's also why there is a runtime_idle > callback but no runtime_needed callback. > > > They would also gain deterministic and controllable behavior that we > > can't guarantee when we rely on a context switch, because obviously > > the latter yields different results for different platforms and > > workloads (for the exact same subsystem). > > Like I said before, it's a trade-off. There are arguments for both > sides, and which is better is unclear. Rafael has to make a judgment > call. Well, I'll do it when I see a patch sent to me. :-) Thanks, Rafael _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
