On 29 August 2017 at 22:19, Rafael J. Wysocki <rjw@xxxxxxxxxxxxx> wrote: > On Tuesday, August 29, 2017 4:56:42 PM CEST Ulf Hansson wrote: >> The i2c designware platform driver, drivers/i2c/busses/i2c-designware-platdrv.c, >> isn't well optimized for system sleep. >> >> What makes this driver particularly interesting is because it's a cross-SoC >> driver, which sometimes means there is an ACPI PM domain attached to the i2c >> device and sometimes not. The driver is being used on both x86 and ARM. >> >> In principle, to optimize the system sleep support in i2c driver, this series >> enables the proven runtime PM centric path for the i2c driver. However, to do >> that the ACPI PM domain also have to collaborate and understand this behaviour. >> From earlier versions, Rafael has also pointed out that also the PM core needs >> to be involved. > > Earlier today I realized that drivers pointing their ->suspend_late and > ->resume_early callbacks, respectively, to pm_runtime_force_suspend() and > pm_runtime_force_resume(), are fundamentally incompatible with any bus type > doing nontrivial PM and with almost any nontrivial PM domains, for two reasons. > > First, it basically requires the bus type or PM domain to expect that its > ->runtime_suspend callback may or may not be indirectly invoked from its > own ->suspend_late callback, depending on the driver (and analogously > for ->runtime_resume and ->resume early), which is insane. > > Second, it is a layering violation, because it makes the driver effectively > override the upper layer's decisions about what code to run. You are right that for more complex bus types and PM domains, those needs to play along. So that is what I am trying to implement for the ACPI PM domain in this series. The generic PM domain, is simple in this regards. There is only a minor adaptation for the ->runtime_suspend|resume() callbacks, which avoids validating dev_pm_qos constraints during system sleep. Nothing special is needed in ->suspend_late|noirq callbacks, etc. For most other simple bus types, like the platform bus, spi, i2c, amba, no particular adoptions is needed at all. Instead those just trust the drivers to do the right thing. Before we had the direct_complete path, using the pm_runtime_force_suspend|resume() helpers, was the only good way for these kind of drivers, to in an optimized manner, deal with system sleep when runtime PM also was enabled for their devices. Now this method has become widely deployed, unfortunate whether you like it or not. Besides the slightly better optimizations you get when using pm_runtime_force_suspend|resume(), comparing to the direct_complete path - I think it's also worth to consider, how easy it becomes for drivers to deploy system sleep support. In many cases, only two lines of code is needed to add system sleep support in a driver. Now, some complex code always needs to be implemented somewhere. When using the runtime PM centric path, that code consist of the pm_runtime_force_suspend|resume() helpers itself - and some adaptations in buses/PM domains in cases when those needs special care. My point is, the runtime PM centric path, allows us to keep the complex part of the code at a few centralized places, instead of having it spread/duplicated into drivers. So yes, you could consider it insane, but to me and many others, it seems to work out quite well. Yeah, and the laying violation is undoubtedly the most controversial part of the runtime PM centric path - I agree to that! The direct_complete path don't have this, as you implemented it. :-) On the other hand, one could consider that these upper layers, in many cases anyway needs to play along with the behavior of the driver. So, I guess it depends on how one see it. > > That's why I'm afraid that we've reached a dead end here. :-( That's said news. Is was really hoping I could find a way to move this forward. You don't have any other ideas on how I can adjust the series to make you happy? > > Thanks, > Rafael > Kind regards Uffe
