Disclaimer: I'm falling asleep, so I probably shouldn't reply to email right
now, but tomorrow I may not be able to get to email at all, so I'll try anyway.

On Wednesday, August 30, 2017 11:57:28 AM CEST Ulf Hansson wrote:

On 29 August 2017 at 22:19, Rafael J. Wysocki [off-list ref] wrote:

quoted

On Tuesday, August 29, 2017 4:56:42 PM CEST Ulf Hansson wrote:

quoted

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.

Well, "play along" is a bit of an understatement here.

They would need to turn into horrible mess and that's not going to happen.

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.

They are the trivial ones.

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.

So can you please remind me why the _force_ wrappers are needed?

In particular, why can't drivers arrange their callbacks the way I did that
in https://patchwork.kernel.org/patch/9928583/ ?

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.

You are doing a wrong comparison here IMO.  You essentially are comparing two
bandaids with each other and arguing that one of them somehow is better.

What about doing something which is not a bandaid instead?

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.

Because it only has been used with trivial middle layer code so far
and I'm quite disappointed that you don't seem to see a problem here. :-/

I mean something like

PM core => bus type / PM domain ->suspend_late => driver ->suspend_late

is far more straightforward than

PM core => bus type / PM domain ->suspend_late => driver ->suspend_late =>
	bus type / PM domain ->runtime_suspend => driver ->runtime_suspend

with the bus type / PM domain having to figure out somehow at the
->suspend_late time whether or not its ->runtume_suspend is going to be invoked
in the middle of it.

Apart from this just being aesthetically disgusting to me, which admittedly is
a matter of personal opinion, it makes debugging new driver code harder (if it
happens to not work) and reviewing it almost impossible, because now you need
to take all of the tangling between callbacks into accont and sometimes not
just for one bus type / PM domain.

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.

quoted

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?

So to be precise, patches [2-3/8] are basically fine by me.  Patch [4/8]
sort of works too, but I'd do the splitting slightly differently and I don't
see much value in it alone.

The rest of the ACPI changes is mostly not acceptable to me, mostly because
of what is done to the PM domain's ->runtime_suspend/resume and
->suspend_late/->resume_early callbacks.

I guess the only way that could be made work for me would be by not using
_force_suspend/resume() at all, but that would defeat the point, right?

I don't like the flag too, but that might be worked out.

Also, when I looked at _force_suspend/resume() again, I got concerned.
There is stuff in there that shouldn't be necessary in a driver's
->late_suspend/->early_resume and some things in there just made me
scratch my head.

Thanks,
Rafael