| Message ID | 20221209114034.18025-1-rf@opensource.cirrus.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: designware: Fix unbalanced suspended flag | expand |
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(). > > Don't set DPM_FLAG_MAY_SKIP_RESUME to skip system early_resume stage if the > driver was runtime-suspended. Instead, always call dw_i2c_plat_resume() and > use pm_runtime_suspended() to determine whether we need to power up the > hardware. > > The unbalanced suspended flag was introduced by > commit c57813b8b288 ("i2c: designware: Lock the adapter while setting the > suspended flag") > > Before that commit, the system and runtime PM used the same functions. The > DPM_FLAG_MAY_SKIP_RESUME was used to skip the system resume if the driver > had been in runtime-suspend. If system resume was skipped, the suspended > flag would be cleared by the next runtime resume. The check of the > suspended flag was _after_ the call to pm_runtime_get_sync() in > i2c_dw_xfer(). So either a system resume or a runtime resume would clear > the flag before it was checked. > > Having introduced the unbalanced suspended flag with that commit, a further > commit 80704a84a9f8 > ("i2c: designware: Use the i2c_mark_adapter_suspended/resumed() helpers") > > changed from using a local suspended flag to using the > i2c_mark_adapter_suspended/resumed() functions. These use a flag that is > checked by I2C core code before issuing the transfer to the bus driver, so > there was no opportunity for the bus driver to runtime resume itself before > the flag check. > > Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> > Fixes: c57813b8b288 ("i2c: designware: Lock the adapter while setting the suspended flag") 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 ? I guess that is possible with DPM_FLAG_MAY_SKIP_RESUME, but since we also use SMART_SUSPEND I would expect the system-suspend callback to also always be skipped for runtime-suspended controllers. > --- > Apologies if you get this message multiple times. I'm having trouble > with my SMTP server. > --- > drivers/i2c/busses/i2c-designware-platdrv.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c > index ba043b547393..d805b8c7e797 100644 > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -351,13 +351,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) > > if (dev->flags & ACCESS_NO_IRQ_SUSPEND) { > dev_pm_set_driver_flags(&pdev->dev, > - DPM_FLAG_SMART_PREPARE | > - DPM_FLAG_MAY_SKIP_RESUME); > + DPM_FLAG_SMART_PREPARE); > } else { > dev_pm_set_driver_flags(&pdev->dev, > DPM_FLAG_SMART_PREPARE | > - DPM_FLAG_SMART_SUSPEND | > - DPM_FLAG_MAY_SKIP_RESUME); > + DPM_FLAG_SMART_SUSPEND); > } > > device_enable_async_suspend(&pdev->dev); > @@ -475,7 +473,9 @@ static int __maybe_unused dw_i2c_plat_resume(struct device *dev) > { > struct dw_i2c_dev *i_dev = dev_get_drvdata(dev); > > - dw_i2c_plat_runtime_resume(dev); > + if (!pm_runtime_suspended(dev)) > + dw_i2c_plat_runtime_resume(dev); > + I'm afraid that this is always going to run now, before this callback gets called drivers/base/power/main.c: device_resume_noirq() does: skip_resume = dev_pm_skip_resume(dev); if (skip_resume) pm_runtime_set_suspended(dev); else if (dev_pm_skip_suspend(dev)) pm_runtime_set_active(dev); Where skip_resume now is false since you dropped the DPM_FLAG_MAY_SKIP_RESUME flag and dev_pm_skip_suspend(dev) will return true (see below) for runtime-suspended controllers, so they will be marked active here. and then your !pm_runtime_suspended(dev) will always be false. Did you add a printk to both the if + else paths and have you ever seen the controller not get resumed with this test added ? Regards, Hans bool dev_pm_skip_suspend(struct device *dev) { return dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) && pm_runtime_status_suspended(dev); } > i2c_mark_adapter_resumed(&i_dev->adapter); > > return 0;
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(). >> >> Don't set DPM_FLAG_MAY_SKIP_RESUME to skip system early_resume stage if the >> driver was runtime-suspended. Instead, always call dw_i2c_plat_resume() and >> use pm_runtime_suspended() to determine whether we need to power up the >> hardware. >> >> The unbalanced suspended flag was introduced by >> commit c57813b8b288 ("i2c: designware: Lock the adapter while setting the >> suspended flag") >> >> Before that commit, the system and runtime PM used the same functions. The >> DPM_FLAG_MAY_SKIP_RESUME was used to skip the system resume if the driver >> had been in runtime-suspend. If system resume was skipped, the suspended >> flag would be cleared by the next runtime resume. The check of the >> suspended flag was _after_ the call to pm_runtime_get_sync() in >> i2c_dw_xfer(). So either a system resume or a runtime resume would clear >> the flag before it was checked. >> >> Having introduced the unbalanced suspended flag with that commit, a further >> commit 80704a84a9f8 >> ("i2c: designware: Use the i2c_mark_adapter_suspended/resumed() helpers") >> >> changed from using a local suspended flag to using the >> i2c_mark_adapter_suspended/resumed() functions. These use a flag that is >> checked by I2C core code before issuing the transfer to the bus driver, so >> there was no opportunity for the bus driver to runtime resume itself before >> the flag check. >> >> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> >> Fixes: c57813b8b288 ("i2c: designware: Lock the adapter while setting the suspended flag") > > 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 ? system suspend callback is called, which calls i2c_mark_adapter_suspended(). system resume is NOT called so i2c_mark_adapter_resumed() is NOT called. A subsequent audio playback using an I2C audio amp then does a runtime resume but the amp driver then gets a "Transfer while suspended" error when it tried to access the part over I2C during its own runtime resume. Tested on Aaeon UpXstreme WHL (Intel Whiskylake chipset) Looking in __device_suspend_late() (drivers/base/power/main.c) there are cases which will skip the SMART_SUSPEND check. > > 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 ? > Yes, that's what I did. system suspend callback is called. System resume callback isn't. > I guess that is possible with DPM_FLAG_MAY_SKIP_RESUME, but > since we also use SMART_SUSPEND I would expect the system-suspend > callback to also always be skipped for runtime-suspended controllers. > > > > > > > >> --- >> Apologies if you get this message multiple times. I'm having trouble >> with my SMTP server. >> --- >> drivers/i2c/busses/i2c-designware-platdrv.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c >> index ba043b547393..d805b8c7e797 100644 >> --- a/drivers/i2c/busses/i2c-designware-platdrv.c >> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c >> @@ -351,13 +351,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) >> >> if (dev->flags & ACCESS_NO_IRQ_SUSPEND) { >> dev_pm_set_driver_flags(&pdev->dev, >> - DPM_FLAG_SMART_PREPARE | >> - DPM_FLAG_MAY_SKIP_RESUME); >> + DPM_FLAG_SMART_PREPARE); >> } else { >> dev_pm_set_driver_flags(&pdev->dev, >> DPM_FLAG_SMART_PREPARE | >> - DPM_FLAG_SMART_SUSPEND | >> - DPM_FLAG_MAY_SKIP_RESUME); >> + DPM_FLAG_SMART_SUSPEND); >> } >> >> device_enable_async_suspend(&pdev->dev); >> @@ -475,7 +473,9 @@ static int __maybe_unused dw_i2c_plat_resume(struct device *dev) >> { >> struct dw_i2c_dev *i_dev = dev_get_drvdata(dev); >> >> - dw_i2c_plat_runtime_resume(dev); >> + if (!pm_runtime_suspended(dev)) >> + dw_i2c_plat_runtime_resume(dev); >> + > > I'm afraid that this is always going to run now, before this callback gets > called drivers/base/power/main.c: device_resume_noirq() does: > > skip_resume = dev_pm_skip_resume(dev); > > if (skip_resume) > pm_runtime_set_suspended(dev); > else if (dev_pm_skip_suspend(dev)) > pm_runtime_set_active(dev); > > Where skip_resume now is false since you dropped the > DPM_FLAG_MAY_SKIP_RESUME flag and dev_pm_skip_suspend(dev) > will return true (see below) for runtime-suspended controllers, > so they will be marked active here. and then your > !pm_runtime_suspended(dev) will always be false. > > Did you add a printk to both the if + else paths > and have you ever seen the controller not get > resumed with this test added ? > > Regards, > > Hans > > > > > bool dev_pm_skip_suspend(struct device *dev) > { > return dev_pm_test_driver_flags(dev, DPM_FLAG_SMART_SUSPEND) && > pm_runtime_status_suspended(dev); > } > > > > >> i2c_mark_adapter_resumed(&i_dev->adapter); >> >> return 0; >
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) So it can skip the resume even if it didn't skip the suspend. The SMART_SUSPEND flag is not set on i2c2 and the driver core can skip resume even if it didn't skip suspend.
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(). >> > I'm afraid that this is always going to run now, before this callback gets > called drivers/base/power/main.c: device_resume_noirq() does: > Ok, what do you suggest as the fix? If you post an alternate fix I can test it.
On Fri, Dec 09, 2022 at 01:15:21PM +0100, Hans de Goede wrote: > On 12/9/22 12:40, Richard Fitzgerald wrote: ... > 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 ? Side note: It's better to use ftrace for that, less invasive time-wise. ... > Did you add a printk to both the if + else paths > and have you ever seen the controller not get > resumed with this test added ?
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 [ 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.
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
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index ba043b547393..d805b8c7e797 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -351,13 +351,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) if (dev->flags & ACCESS_NO_IRQ_SUSPEND) { dev_pm_set_driver_flags(&pdev->dev, - DPM_FLAG_SMART_PREPARE | - DPM_FLAG_MAY_SKIP_RESUME); + DPM_FLAG_SMART_PREPARE); } else { dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_SMART_PREPARE | - DPM_FLAG_SMART_SUSPEND | - DPM_FLAG_MAY_SKIP_RESUME); + DPM_FLAG_SMART_SUSPEND); } device_enable_async_suspend(&pdev->dev); @@ -475,7 +473,9 @@ static int __maybe_unused dw_i2c_plat_resume(struct device *dev) { struct dw_i2c_dev *i_dev = dev_get_drvdata(dev); - dw_i2c_plat_runtime_resume(dev); + if (!pm_runtime_suspended(dev)) + dw_i2c_plat_runtime_resume(dev); + i2c_mark_adapter_resumed(&i_dev->adapter); return 0;
Ensure that i2c_mark_adapter_suspended() is always balanced by a call to i2c_mark_adapter_resumed(). Don't set DPM_FLAG_MAY_SKIP_RESUME to skip system early_resume stage if the driver was runtime-suspended. Instead, always call dw_i2c_plat_resume() and use pm_runtime_suspended() to determine whether we need to power up the hardware. The unbalanced suspended flag was introduced by commit c57813b8b288 ("i2c: designware: Lock the adapter while setting the suspended flag") Before that commit, the system and runtime PM used the same functions. The DPM_FLAG_MAY_SKIP_RESUME was used to skip the system resume if the driver had been in runtime-suspend. If system resume was skipped, the suspended flag would be cleared by the next runtime resume. The check of the suspended flag was _after_ the call to pm_runtime_get_sync() in i2c_dw_xfer(). So either a system resume or a runtime resume would clear the flag before it was checked. Having introduced the unbalanced suspended flag with that commit, a further commit 80704a84a9f8 ("i2c: designware: Use the i2c_mark_adapter_suspended/resumed() helpers") changed from using a local suspended flag to using the i2c_mark_adapter_suspended/resumed() functions. These use a flag that is checked by I2C core code before issuing the transfer to the bus driver, so there was no opportunity for the bus driver to runtime resume itself before the flag check. Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com> Fixes: c57813b8b288 ("i2c: designware: Lock the adapter while setting the suspended flag") --- Apologies if you get this message multiple times. I'm having trouble with my SMTP server. --- drivers/i2c/busses/i2c-designware-platdrv.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)