Re: [PATCH v3 1/5] PM: sleep: Resume children after resuming the parent

[Jon Hunter <jonathanh@nvidia.com>]

Hi Rafael,

On 02/05/2025 21:33, Rafael J. Wysocki wrote:

...

>> I have noticed a suspend regression with -next on a couple of our Tegra
>> boards. Bisect was pointing to the following merge commit ...
>>
>> # first bad commit: [218a7bbf861f83398ac9767620e91983e36eac05] Merge
>> branch 'pm-sleep' into linux-next
>>
>> On top of next-20250429 I found that by reverting the following changes
>> that suspend is working again ...
>>
>> Revert "PM: sleep: Resume children after resuming the parent"
>> Revert "PM: sleep: Suspend async parents after suspending children"
>> Revert "PM: sleep: Make suspend of devices more asynchronous"
> 
> I see.
> 
> Do all three commits need to be reverted to make things work again?
> The first one only touches the resume path, so it would be surprising
> if it caused a suspend regression to occur.

I had to revert the other 2 patches for the kernel to build. I tried to 
only revery this patch, and fix the build issue by defining the missing 
function and mutex, but that did not seem to work. The build worked, but 
suspend still failed. I am not sure if that is conclusive though.

> 
> The most likely commit to cause this issue to happen is the second one
> because it effectively changes the suspend ordering for "async"
> devices.
> 
>> I have been looking into this a bit more to see what device is failing
>> and by adding a bit of debug I found that entry to suspend was failing
>> on the Tegra194 Jetson AGX Xavier (tegra194-p2972-0000.dts) platform
>> when one of the I2C controllers (i2c@c240000) was being suspended.
>>
>> I found that if I disable only this I2C controller in device-tree
>> suspend worked again on top of -next. This I2C controller has 3 devices
>> on the platform; two ina3221 devices and one Cypress Type-C controller.
>> I then found that removing only the two ina3221 devices (in
>> tegra194-p2888.dtsi) also allows suspend to work.
>>
>> At this point, I am still unclear why this is now failing.  If you have
>> any thoughts or things I can try please let me know.
> 
> So are the devices in question "async"?  To check this, please see the
> "async" attribute in the "power" subdirectory of the sysfs device
> directory for each of them.

I checked for both the I2C controller and ina3221 and don't see any 
'async' files ...

$ ls /sys/class/i2c-dev/i2c-2/device/2-0040/power/
autosuspend_delay_ms  runtime_active_time  runtime_suspended_time
control               runtime_status
$ ls /sys/class/i2c-dev/i2c-2/device/2-0041/power/
autosuspend_delay_ms  runtime_active_time  runtime_suspended_time
control               runtime_status
$ ls /sys/class/i2c-dev/i2c-2/power/
autosuspend_delay_ms  runtime_active_time  runtime_suspended_time
control               runtime_status

> If they are "async", you can write "disable" to this attribute to turn
> them into "sync" devices.  I'd do this and see what happens.
> 
> Overall, it looks like some dependencies aren't properly represented
> by device links on this platform.


Yes that would appear to be the case, but at the moment, I don't see 
what it is. The ina3221 devices appear to suspend fine AFAICT, but hangs 
when suspending I2C controller. Exactly where is still a mystery.

Jon

-- 
nvpublic