[PATCH v4] i2c: designware: Fix unbalanced suspended flag

[PATCH v4] i2c: designware: Fix unbalanced suspended flag

[Richard Fitzgerald]

Ensure that i2c_mark_adapter_suspended() is always balanced by a call to
i2c_mark_adapter_resumed().

dw_i2c_plat_resume() must always be called, so that
i2c_mark_adapter_resumed() is called. This is not compatible with
DPM_FLAG_MAY_SKIP_RESUME, so remove the flag.

Since the controller is always resumed on system resume the
dw_i2c_plat_complete() callback is redundant and has been removed.

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")
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
Changes from v3:
- Fixed wrapping in commit description. No code changes
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index ba043b547393..74182db03a88 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);
@@ -419,21 +417,8 @@ static int dw_i2c_plat_prepare(struct device *dev)
 	 */
 	return !has_acpi_companion(dev);
 }
-
-static void dw_i2c_plat_complete(struct device *dev)
-{
-	/*
-	 * The device can only be in runtime suspend at this point if it has not
-	 * been resumed throughout the ending system suspend/resume cycle, so if
-	 * the platform firmware might mess up with it, request the runtime PM
-	 * framework to resume it.
-	 */
-	if (pm_runtime_suspended(dev) && pm_resume_via_firmware())
-		pm_request_resume(dev);
-}
 #else
 #define dw_i2c_plat_prepare	NULL
-#define dw_i2c_plat_complete	NULL
 #endif
 
 #ifdef CONFIG_PM
@@ -483,7 +468,6 @@ static int __maybe_unused dw_i2c_plat_resume(struct device *dev)
 
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
 	.prepare = dw_i2c_plat_prepare,
-	.complete = dw_i2c_plat_complete,
 	SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
 	SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend, dw_i2c_plat_runtime_resume, NULL)
 };
-- 
2.30.2

Re: [PATCH v4] i2c: designware: Fix unbalanced suspended flag

[Andy Shevchenko]

On Mon, Dec 19, 2022 at 01:01:45PM +0000, Richard Fitzgerald wrote:
> Ensure that i2c_mark_adapter_suspended() is always balanced by a call to
> i2c_mark_adapter_resumed().
> 
> dw_i2c_plat_resume() must always be called, so that
> i2c_mark_adapter_resumed() is called. This is not compatible with
> DPM_FLAG_MAY_SKIP_RESUME, so remove the flag.
> 
> Since the controller is always resumed on system resume the
> dw_i2c_plat_complete() callback is redundant and has been removed.
> 
> 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")

This is still unwrapped propertly, but no need to resend, it so minor, really.
Just a hint for the future submissions.

> 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.

...

> ---
> Changes from v3:
> - Fixed wrapping in commit description. No code changes

Thank you, but see the above :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4] i2c: designware: Fix unbalanced suspended flag

[Jarkko Nikula]

On 12/19/22 15:10, Andy Shevchenko wrote:
> On Mon, Dec 19, 2022 at 01:01:45PM +0000, Richard Fitzgerald wrote:
>> Ensure that i2c_mark_adapter_suspended() is always balanced by a call to
>> i2c_mark_adapter_resumed().
>>
>> dw_i2c_plat_resume() must always be called, so that
>> i2c_mark_adapter_resumed() is called. This is not compatible with
>> DPM_FLAG_MAY_SKIP_RESUME, so remove the flag.
>>
>> Since the controller is always resumed on system resume the
>> dw_i2c_plat_complete() callback is redundant and has been removed.
>>
>> 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")
> 
> This is still unwrapped propertly, but no need to resend, it so minor, really.
> Just a hint for the future submissions.
> 
This version was working on those two machines that saw a regression 
with v2 so

Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH v4] i2c: designware: Fix unbalanced suspended flag

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1961 bytes --]

On Mon, Dec 19, 2022 at 01:01:45PM +0000, Richard Fitzgerald wrote:
> Ensure that i2c_mark_adapter_suspended() is always balanced by a call to
> i2c_mark_adapter_resumed().
> 
> dw_i2c_plat_resume() must always be called, so that
> i2c_mark_adapter_resumed() is called. This is not compatible with
> DPM_FLAG_MAY_SKIP_RESUME, so remove the flag.
> 
> Since the controller is always resumed on system resume the
> dw_i2c_plat_complete() callback is redundant and has been removed.
> 
> 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")
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Does this fix a bug when runtime resuming? This is not clear to me. I
tend to put it to for-next rather than for-current.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4] i2c: designware: Fix unbalanced suspended flag

[Hans de Goede]

Hi,

On 1/9/23 13:01, Wolfram Sang wrote:
> On Mon, Dec 19, 2022 at 01:01:45PM +0000, Richard Fitzgerald wrote:
>> Ensure that i2c_mark_adapter_suspended() is always balanced by a call to
>> i2c_mark_adapter_resumed().
>>
>> dw_i2c_plat_resume() must always be called, so that
>> i2c_mark_adapter_resumed() is called. This is not compatible with
>> DPM_FLAG_MAY_SKIP_RESUME, so remove the flag.
>>
>> Since the controller is always resumed on system resume the
>> dw_i2c_plat_complete() callback is redundant and has been removed.
>>
>> 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")
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> Does this fix a bug when runtime resuming? This is not clear to me. I
> tend to put it to for-next rather than for-current.

This fixes a system suspend/resume bug, so this really should go
to for-current.

Regards,

Hans

Re: [PATCH v4] i2c: designware: Fix unbalanced suspended flag

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1870 bytes --]

On Mon, Dec 19, 2022 at 01:01:45PM +0000, Richard Fitzgerald wrote:
> Ensure that i2c_mark_adapter_suspended() is always balanced by a call to
> i2c_mark_adapter_resumed().
> 
> dw_i2c_plat_resume() must always be called, so that
> i2c_mark_adapter_resumed() is called. This is not compatible with
> DPM_FLAG_MAY_SKIP_RESUME, so remove the flag.
> 
> Since the controller is always resumed on system resume the
> dw_i2c_plat_complete() callback is redundant and has been removed.
> 
> 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")
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]