i2c: designware: Do nothing in system suspend/resume when RT suspended

Submitted by Jarkko Nikula on March 30, 2017, 12:04 p.m.

Details

Message ID 20170330120444.12499-1-jarkko.nikula@linux.intel.com
State Under Review
Headers show

Commit Message

Jarkko Nikula March 30, 2017, 12:04 p.m.
There is possibility to enter dw_i2c_plat_suspend() callback twice
during system suspend under certain cases which is causing here warnings
from clk_core_disable() and clk_core_unprepare() as well as accessing the
registers that can be power gated.

Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during
system suspend") implemented a prepare callback that checks for runtime
suspended device which allow PM core to set direct_complete flag and
skip system suspend and resume callbacks.

However it can still happen if nothing resumes the device prior system
syspend (e.g. acpi_subsys_suspend()) and there is a slave device which
unsets the direct_complete flag of the parent in __device_suspend() thus
causing PM code to not skip the system suspend/resume callbacks.

Avoid this by checking runtime status in suspend and resume callbacks
and return directly if device is runtime suspended. This affects only
system suspend/resume since during runtime suspend/resume runtime status
is suspending (not suspended) or resuming.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
I'm able to trigger system suspend callback while device is runtime
suspended by removing the pm_runtime_resume() call from
drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C
slave (ACPI enumerated but doesn't bind due an error in probe function).
In that case __device_suspend() for that unbound device has NULL suspend
callback, and thus doesn't cause any runtime resume chain but still unsets
the parent's direct_complete flag.
John Stult <john.stultz@linaro.org> has reported he can trigger this on
HiKey board too.

I'm not sure is this the right thing to do. It feels something the PM core
should do but I'm not sure that either. One alternative could be to resume
runtime suspended parent in in __device_suspend() right after where
parent's direct_complete flag is unset.
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

John Stultz April 4, 2017, 2:55 a.m.
On Thu, Mar 30, 2017 at 5:04 AM, Jarkko Nikula
<jarkko.nikula@linux.intel.com> wrote:
> There is possibility to enter dw_i2c_plat_suspend() callback twice
> during system suspend under certain cases which is causing here warnings
> from clk_core_disable() and clk_core_unprepare() as well as accessing the
> registers that can be power gated.
>
> Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during
> system suspend") implemented a prepare callback that checks for runtime
> suspended device which allow PM core to set direct_complete flag and
> skip system suspend and resume callbacks.
>
> However it can still happen if nothing resumes the device prior system
> syspend (e.g. acpi_subsys_suspend()) and there is a slave device which
> unsets the direct_complete flag of the parent in __device_suspend() thus
> causing PM code to not skip the system suspend/resume callbacks.
>
> Avoid this by checking runtime status in suspend and resume callbacks
> and return directly if device is runtime suspended. This affects only
> system suspend/resume since during runtime suspend/resume runtime status
> is suspending (not suspended) or resuming.
>
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

This works for me on my HiKey board!

Tested-by: John Stultz <john.stultz@linaro.org>

thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang April 19, 2017, 6:59 p.m.
On Thu, Mar 30, 2017 at 03:04:44PM +0300, Jarkko Nikula wrote:
> There is possibility to enter dw_i2c_plat_suspend() callback twice
> during system suspend under certain cases which is causing here warnings
> from clk_core_disable() and clk_core_unprepare() as well as accessing the
> registers that can be power gated.
> 
> Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during
> system suspend") implemented a prepare callback that checks for runtime
> suspended device which allow PM core to set direct_complete flag and
> skip system suspend and resume callbacks.
> 
> However it can still happen if nothing resumes the device prior system
> syspend (e.g. acpi_subsys_suspend()) and there is a slave device which
> unsets the direct_complete flag of the parent in __device_suspend() thus
> causing PM code to not skip the system suspend/resume callbacks.
> 
> Avoid this by checking runtime status in suspend and resume callbacks
> and return directly if device is runtime suspended. This affects only
> system suspend/resume since during runtime suspend/resume runtime status
> is suspending (not suspended) or resuming.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
> I'm able to trigger system suspend callback while device is runtime
> suspended by removing the pm_runtime_resume() call from
> drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C
> slave (ACPI enumerated but doesn't bind due an error in probe function).
> In that case __device_suspend() for that unbound device has NULL suspend
> callback, and thus doesn't cause any runtime resume chain but still unsets
> the parent's direct_complete flag.
> John Stult <john.stultz@linaro.org> has reported he can trigger this on
> HiKey board too.
> 
> I'm not sure is this the right thing to do. It feels something the PM core
> should do but I'm not sure that either. One alternative could be to resume
> runtime suspended parent in in __device_suspend() right after where
> parent's direct_complete flag is unset.

Because of the last paragraph, I'd like a positive comment or tag from
one of the PM people before I apply this one.

> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index a597ba32de7e..42a9cd09aa64 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -377,6 +377,9 @@ static int dw_i2c_plat_suspend(struct device *dev)
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
>  
> +	if (pm_runtime_suspended(dev))
> +		return 0;
> +
>  	i2c_dw_disable(i_dev);
>  	i2c_dw_plat_prepare_clk(i_dev, false);
>  
> @@ -388,6 +391,9 @@ static int dw_i2c_plat_resume(struct device *dev)
>  	struct platform_device *pdev = to_platform_device(dev);
>  	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
>  
> +	if (pm_runtime_suspended(dev))
> +		return 0;
> +
>  	i2c_dw_plat_prepare_clk(i_dev, true);
>  	i2c_dw_init(i_dev);
>  
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki April 19, 2017, 8:24 p.m.
On Thu, Mar 30, 2017 at 2:04 PM, Jarkko Nikula
<jarkko.nikula@linux.intel.com> wrote:
> There is possibility to enter dw_i2c_plat_suspend() callback twice
> during system suspend under certain cases which is causing here warnings
> from clk_core_disable() and clk_core_unprepare() as well as accessing the
> registers that can be power gated.
>
> Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during
> system suspend") implemented a prepare callback that checks for runtime
> suspended device which allow PM core to set direct_complete flag and
> skip system suspend and resume callbacks.
>
> However it can still happen if nothing resumes the device prior system
> syspend (e.g. acpi_subsys_suspend()) and there is a slave device which
> unsets the direct_complete flag of the parent in __device_suspend() thus
> causing PM code to not skip the system suspend/resume callbacks.
>
> Avoid this by checking runtime status in suspend and resume callbacks
> and return directly if device is runtime suspended. This affects only
> system suspend/resume since during runtime suspend/resume runtime status
> is suspending (not suspended) or resuming.
>
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
> I'm able to trigger system suspend callback while device is runtime
> suspended by removing the pm_runtime_resume() call from
> drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C
> slave (ACPI enumerated but doesn't bind due an error in probe function).
> In that case __device_suspend() for that unbound device has NULL suspend
> callback, and thus doesn't cause any runtime resume chain but still unsets
> the parent's direct_complete flag.
> John Stult <john.stultz@linaro.org> has reported he can trigger this on
> HiKey board too.
>
> I'm not sure is this the right thing to do. It feels something the PM core
> should do but I'm not sure that either. One alternative could be to resume
> runtime suspended parent in in __device_suspend() right after where
> parent's direct_complete flag is unset.

In that case the core expects that the ->prepare callback for the
slave will also return 1 (or a positive number in general).

If that doesn't happen, then from the core's perspective it is not
safe to allow the master's system PM callbacks to be skipped and
that's why direct_complete is unset for it.

> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index a597ba32de7e..42a9cd09aa64 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -377,6 +377,9 @@ static int dw_i2c_plat_suspend(struct device *dev)
>         struct platform_device *pdev = to_platform_device(dev);
>         struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
>
> +       if (pm_runtime_suspended(dev))
> +               return 0;
> +
>         i2c_dw_disable(i_dev);
>         i2c_dw_plat_prepare_clk(i_dev, false);
>
> @@ -388,6 +391,9 @@ static int dw_i2c_plat_resume(struct device *dev)
>         struct platform_device *pdev = to_platform_device(dev);
>         struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
>
> +       if (pm_runtime_suspended(dev))
> +               return 0;
> +
>         i2c_dw_plat_prepare_clk(i_dev, true);
>         i2c_dw_init(i_dev);
>
> --
> 2.11.0
>

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki April 19, 2017, 8:24 p.m.
On Wed, Apr 19, 2017 at 8:59 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> On Thu, Mar 30, 2017 at 03:04:44PM +0300, Jarkko Nikula wrote:
>> There is possibility to enter dw_i2c_plat_suspend() callback twice
>> during system suspend under certain cases which is causing here warnings
>> from clk_core_disable() and clk_core_unprepare() as well as accessing the
>> registers that can be power gated.
>>
>> Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during
>> system suspend") implemented a prepare callback that checks for runtime
>> suspended device which allow PM core to set direct_complete flag and
>> skip system suspend and resume callbacks.
>>
>> However it can still happen if nothing resumes the device prior system
>> syspend (e.g. acpi_subsys_suspend()) and there is a slave device which
>> unsets the direct_complete flag of the parent in __device_suspend() thus
>> causing PM code to not skip the system suspend/resume callbacks.
>>
>> Avoid this by checking runtime status in suspend and resume callbacks
>> and return directly if device is runtime suspended. This affects only
>> system suspend/resume since during runtime suspend/resume runtime status
>> is suspending (not suspended) or resuming.
>>
>> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>> ---
>> I'm able to trigger system suspend callback while device is runtime
>> suspended by removing the pm_runtime_resume() call from
>> drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C
>> slave (ACPI enumerated but doesn't bind due an error in probe function).
>> In that case __device_suspend() for that unbound device has NULL suspend
>> callback, and thus doesn't cause any runtime resume chain but still unsets
>> the parent's direct_complete flag.
>> John Stult <john.stultz@linaro.org> has reported he can trigger this on
>> HiKey board too.
>>
>> I'm not sure is this the right thing to do. It feels something the PM core
>> should do but I'm not sure that either. One alternative could be to resume
>> runtime suspended parent in in __device_suspend() right after where
>> parent's direct_complete flag is unset.
>
> Because of the last paragraph, I'd like a positive comment or tag from
> one of the PM people before I apply this one.

No, sorry.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Nikula April 20, 2017, 7:25 a.m.
Hi

On 04/19/2017 11:24 PM, Rafael J. Wysocki wrote:
> On Thu, Mar 30, 2017 at 2:04 PM, Jarkko Nikula
> <jarkko.nikula@linux.intel.com> wrote:
>> There is possibility to enter dw_i2c_plat_suspend() callback twice
>> during system suspend under certain cases which is causing here warnings
>> from clk_core_disable() and clk_core_unprepare() as well as accessing the
>> registers that can be power gated.
>>
>> Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during
>> system suspend") implemented a prepare callback that checks for runtime
>> suspended device which allow PM core to set direct_complete flag and
>> skip system suspend and resume callbacks.
>>
>> However it can still happen if nothing resumes the device prior system
>> syspend (e.g. acpi_subsys_suspend()) and there is a slave device which
>> unsets the direct_complete flag of the parent in __device_suspend() thus
>> causing PM code to not skip the system suspend/resume callbacks.
>>
>> Avoid this by checking runtime status in suspend and resume callbacks
>> and return directly if device is runtime suspended. This affects only
>> system suspend/resume since during runtime suspend/resume runtime status
>> is suspending (not suspended) or resuming.
>>
>> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>> ---
>> I'm able to trigger system suspend callback while device is runtime
>> suspended by removing the pm_runtime_resume() call from
>> drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C
>> slave (ACPI enumerated but doesn't bind due an error in probe function).
>> In that case __device_suspend() for that unbound device has NULL suspend
>> callback, and thus doesn't cause any runtime resume chain but still unsets
>> the parent's direct_complete flag.
>> John Stult <john.stultz@linaro.org> has reported he can trigger this on
>> HiKey board too.
>>
>> I'm not sure is this the right thing to do. It feels something the PM core
>> should do but I'm not sure that either. One alternative could be to resume
>> runtime suspended parent in in __device_suspend() right after where
>> parent's direct_complete flag is unset.
>
> In that case the core expects that the ->prepare callback for the
> slave will also return 1 (or a positive number in general).
>
> If that doesn't happen, then from the core's perspective it is not
> safe to allow the master's system PM callbacks to be skipped and
> that's why direct_complete is unset for it.
>
So it's then right thing to check runtime PM status in driver as patch 
does below?

>> ---
>>  drivers/i2c/busses/i2c-designware-platdrv.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
>> index a597ba32de7e..42a9cd09aa64 100644
>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>> @@ -377,6 +377,9 @@ static int dw_i2c_plat_suspend(struct device *dev)
>>         struct platform_device *pdev = to_platform_device(dev);
>>         struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
>>
>> +       if (pm_runtime_suspended(dev))
>> +               return 0;
>> +
>>         i2c_dw_disable(i_dev);
>>         i2c_dw_plat_prepare_clk(i_dev, false);
>>
>> @@ -388,6 +391,9 @@ static int dw_i2c_plat_resume(struct device *dev)
>>         struct platform_device *pdev = to_platform_device(dev);
>>         struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
>>
>> +       if (pm_runtime_suspended(dev))
>> +               return 0;
>> +
>>         i2c_dw_plat_prepare_clk(i_dev, true);
>>         i2c_dw_init(i_dev);
Rafael J. Wysocki April 20, 2017, 10:33 a.m.
On Thu, Apr 20, 2017 at 9:25 AM, Jarkko Nikula
<jarkko.nikula@linux.intel.com> wrote:
> Hi
>
>
> On 04/19/2017 11:24 PM, Rafael J. Wysocki wrote:
>>
>> On Thu, Mar 30, 2017 at 2:04 PM, Jarkko Nikula
>> <jarkko.nikula@linux.intel.com> wrote:
>>>
>>> There is possibility to enter dw_i2c_plat_suspend() callback twice
>>> during system suspend under certain cases which is causing here warnings
>>> from clk_core_disable() and clk_core_unprepare() as well as accessing the
>>> registers that can be power gated.
>>>
>>> Commit 8503ff166504 ("i2c: designware: Avoid unnecessary resuming during
>>> system suspend") implemented a prepare callback that checks for runtime
>>> suspended device which allow PM core to set direct_complete flag and
>>> skip system suspend and resume callbacks.
>>>
>>> However it can still happen if nothing resumes the device prior system
>>> syspend (e.g. acpi_subsys_suspend()) and there is a slave device which
>>> unsets the direct_complete flag of the parent in __device_suspend() thus
>>> causing PM code to not skip the system suspend/resume callbacks.
>>>
>>> Avoid this by checking runtime status in suspend and resume callbacks
>>> and return directly if device is runtime suspended. This affects only
>>> system suspend/resume since during runtime suspend/resume runtime status
>>> is suspending (not suspended) or resuming.
>>>
>>> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
>>> ---
>>> I'm able to trigger system suspend callback while device is runtime
>>> suspended by removing the pm_runtime_resume() call from
>>> drivers/mfd/intel-lpss.c: resume_lpss_device() and having unbound I2C
>>> slave (ACPI enumerated but doesn't bind due an error in probe function).
>>> In that case __device_suspend() for that unbound device has NULL suspend
>>> callback, and thus doesn't cause any runtime resume chain but still
>>> unsets
>>> the parent's direct_complete flag.
>>> John Stult <john.stultz@linaro.org> has reported he can trigger this on
>>> HiKey board too.
>>>
>>> I'm not sure is this the right thing to do. It feels something the PM
>>> core
>>> should do but I'm not sure that either. One alternative could be to
>>> resume
>>> runtime suspended parent in in __device_suspend() right after where
>>> parent's direct_complete flag is unset.
>>
>>
>> In that case the core expects that the ->prepare callback for the
>> slave will also return 1 (or a positive number in general).
>>
>> If that doesn't happen, then from the core's perspective it is not
>> safe to allow the master's system PM callbacks to be skipped and
>> that's why direct_complete is unset for it.
>>
> So it's then right thing to check runtime PM status in driver as patch does
> below?

If you know for a fact that none of the device's children and none of
the children thereof and so on and nothing that may depend on the
device via a device_link, either directly or indirectly, will ever
need to be resumed during system suspend, then yes, it is.

Otherwise, no, it isn't.

Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index a597ba32de7e..42a9cd09aa64 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -377,6 +377,9 @@  static int dw_i2c_plat_suspend(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
+	if (pm_runtime_suspended(dev))
+		return 0;
+
 	i2c_dw_disable(i_dev);
 	i2c_dw_plat_prepare_clk(i_dev, false);
 
@@ -388,6 +391,9 @@  static int dw_i2c_plat_resume(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
+	if (pm_runtime_suspended(dev))
+		return 0;
+
 	i2c_dw_plat_prepare_clk(i_dev, true);
 	i2c_dw_init(i_dev);