Hi, On 12/9/22 15:22, Richard Fitzgerald wrote: > On 9/12/22 13:36, Richard Fitzgerald wrote: >> On 9/12/22 12:15, Hans de Goede wrote: >>> Hi Richard, >>> >>> On 12/9/22 12:40, Richard Fitzgerald wrote: >>>> Ensure that i2c_mark_adapter_suspended() is always balanced by a call to >>>> i2c_mark_adapter_resumed(). >> >> <snip> >> >>> >>> It is not entirely clear to me where the unbalance you claim to see comes >>> from? When runtime-suspended SMART_SUSPEND should keep it suspended at which point >>> the system suspend callback will never run ? >>> >>> Are you sure that you are not maybe seeing a suspend/resume ordering issue? >>> >>> Did you add printk messages to the suspend/resume callbacks of >>> i2c-designware-platdrv.c which show the system suspend callback >>> being called but not the system resume one ? >>> >> >> With messages in strategic places. >> >> [ 169.607358] i2c_designware i2c_designware.2: PM: dev_pm_skip_suspend: SMART_SUSPEND=0 pm_runtime_status_suspended=1 >> [ 169.607361] i2c_designware i2c_designware.2: PM: __device_suspend_late: dev_pm_skip_suspend:false >> [ 169.607364] i2c_designware i2c_designware.2: dw_i2c_plat_suspend >> ... >> [ 169.702511] i2c_designware i2c_designware.2: PM: dev_pm_skip_resume: 1 because !power.must_resume >> [ 169.706241] i2c_designware i2c_designware.2: PM: dev_pm_skip_resume: 1 because !power.must_resume >> [ 169.706244] i2c_designware i2c_designware.2: PM: device_resume_early: dev_pm_skip_resume:true >> ... >> [ 175.254832] i2c i2c-2: Transfer while suspended >> >> (Just to prove my logging isn't lying, for i2c3 it reports >> SMART_SUSPEND=1) >> > > Oh, that's embarrassing. After confidently telling you my logging > is perfect, actually there was a bug in it... > > New log summary: > > [ 162.253431] i2c_designware i2c_designware.2: PM: dev_pm_skip_suspend: SMART_SUSPEND=1 pm_runtime_status_suspended=0 Ok, so the device's pm_runtime_get() count is 0 here (otherwise must_resume should be 1 later on) but the device is not run-time suspended yet. Probably because of some timeout; or because of runtime pm getting disabled durig suspend before the count dropped to 0. And this scenario will indeed cause the system-level suspend callback to get called, but not the resume one ... > [ 162.253438] i2c_designware i2c_designware.2: PM: __device_suspend_late: dev_pm_skip_suspend:false > [ 162.253445] i2c_designware i2c_designware.2: dw_i2c_plat_suspend > [ 162.273115] i2c_designware i2c_designware.2: PM: dev_pm_skip_suspend: SMART_SUSPEND=1 pm_runtime_status_suspended=0 > [ 162.362547] i2c_designware i2c_designware.2: PM: dev_pm_skip_resume: 1 because !power.must_resume > [ 162.369216] i2c_designware i2c_designware.2: PM: dev_pm_skip_resume: 1 because !power.must_resume > [ 162.369220] i2c_designware i2c_designware.2: PM: device_resume_early: dev_pm_skip_resume:true > [ 167.901269] i2c i2c-2: Transfer while suspended > > Same result that it doesn't skip suspend but does skip resume. >From your other email: > Ok, what do you suggest as the fix? > If you post an alternate fix I can test it. I don't really see a better solution, so lets go with your solution, but then: 1. Simply drop the flag but don't add the if (!pm_runtime_suspended(dev)) check. The runtime status is always going to be set to active at this point so the check does not do anything. 2. Drop the dw_i2c_plat_complete() callback since we now always resume the controller on system resume. Regards, Hans
