Re: [PATCH] PM: Document rules on using pm_runtime_resume() in system suspend callbacks

[Rafael J. Wysocki]

On Wed, Sep 20, 2017 at 6:15 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 20 Sep 2017, Rafael J. Wysocki wrote:
>
>> >> Second, leaving devices in runtime suspend in the "suspend" phase of system
>> >> suspend is fishy even when their runtime PM is disabled, because that doesn't
>> >> guarantee anything regarding their children or possible consumers.  Runtime
>> >> PM may still be enabled for those devices at that time and runtime resume may
>> >> be triggered for them later, in which case it all quickly falls apart.
>> >
>> > This is true, although to me this is a about a different problem and
>> > has very little to do with pm_runtime_force_suspend().
>> >
>> > More precisely, whether runtime PM becomes disabled in the suspend
>> > phase or suspend_late phase, really doesn't matter. Because in the end
>> > this is about suspending/resuming devices in the correct order.
>>
>> Yes, it is, but this is not my point (I didn't make it clear enough I guess).
>>
>> At the time you make the decision to disable runtime PM for a parent
>> (say) and leave it in runtime suspend, all of its children are
>> suspended just fine (otherwise the parent wouldn't have been suspended
>> too).  However, you *also* need to make sure that there will be no
>> attempts to resume any of them *after* that point, which practically
>> means that either runtime PM has to have been disabled already for all
>> of them at the time it is disabled for the parent, or there has to be
>> another guarantee in place.
>>
>> That's why the core tries to enforce the "runtime PM disabled for the
>> entire hierarchy below" guarantee for the devices with direct_complete
>> set, but that may just be overkill in many cases.  I guess it may be
>> better to use WARN_ON() to catch the cases in which things may really
>> go wrong.
>
> That's a good idea.
>
>> >> IOW, there are reasons why the PM core bumps up the runtime PM usage counters
>> >> of all devices during system suspend and they also apply to runtime suspend
>> >> callbacks being invoked directly with runtime PM disabled for the given device.
>> >> Frankly, it generally is only safe to leave a device in runtime suspend during
>> >> system suspend if it can be guarateed that the system suspend callbacks in the
>> >> subsequent suspend phases will not be invoked for it at all.
>> >
>> > I understand this is perfectly true for some of the non-trivial middle
>> > layers, however just to be clear, this statement don't have to serve
>> > as a general rule for all cases, right?
>>
>> Well, a really general version of it is something like "it is only
>> safe to leave a device in runtime suspend during system suspend if it
>> can be guaranteed that the system suspend callbacks in the subsequent
>> suspend phases will not change its state" and the most effective way
>> to make that guarantee is to prevent them from being invoked at all.
>> :-)
>
> Of course, this can be overkill.  It's probably common for there to be
> little physical difference between a device's "suspended" state and
> its "runtime-suspended" state.  In such cases, the middle layer and
> the subsequent callbacks merely have to recognize that the device is
> already at low power (by checking an internal flag, for instance) and
> then do nothing when they see that it is.

Right.

The idea, though, was to make it work even if they didn't do that. :-)

>> > Moreover, bumping the runtime PM usage count
>> > (pm_runtime_get_noresume()) in the device_prepare/suspend() phase, was
>> > originally added to prevent the runtime PM core from runtime
>> > suspending a device, in cases when runtime PM has been enabled for it.
>> > Preventing the ->runtime_suspend() callback from being invoked when
>> > runtime PM is disabled, just doesn't make any sense to me.
>>
>> The problem is that the functionality of ->runtime_suspend() in
>> principle overlaps with the functionality of ->suspend(),
>> ->suspend_late() and ->suspend_noirq() combined, but it need not be
>> entirely the same.  Therefore if you invoke ->runtime_suspend()
>> anywhere between the beginning of ->suspend() and the end of
>> ->suspend_noirq(), the remaining code in the system sleep callbacks
>> needs to know about that in order to avoid, for example, attempting to
>> power down the device for the second time in a row, which very well
>> may kill the system in some extreme cases.
>
> Another possibility is the arrival of an ill-timed runtime resume after
> some of the suspend callbacks have run (because of a wakeup request,
> for example).  In short, system PM and runtime PM do overlap
> considerably, and this means that it would be easy for either one to
> interfere with the other unless the developers are very careful.

Right.

>> Of course, if those callbacks are trivial, this problem goes away, but
>> they need not be trivial and if you are a platform driver (or an
>> i2c/spi driver too for that matter), you aren't guaranteed that they
>> will always be trivial.  That is quite a bit of an issue to me.
>
> Indeed.  It may not be possible to come up with a firm set of detailed
> rules that apply everywhere.  But we should make sure that people are
> aware of the issues.

Agreed.

Thanks,
Rafael