[PATCH 0/4] i2c: designware: runtime pm fix and improve

[PATCH 0/4] i2c: designware: runtime pm fix and improve

[Jisheng Zhang]

This series tries to fix one hang issue during probe found on arm
platform, and unbalanced clk enable prepare issue. Then applies similar
change as commit 36d48fb5766a ("i2c: designware-platdrv: enable
RuntimePM before registering to the core"), lastly remove the runtime
suspend prevention in i2c_dw_probe().


Jisheng Zhang (4):
  i2c: designware-platdrv: Fix runtime PM initialization
  i2c: designware-platdrv: fix unbalanced clk enable and prepare
  i2c: designware-pcidrv: enable RuntimePM before registering to the
    core
  i2c: designware: remove runtime suspend prevention during registration

 drivers/i2c/busses/i2c-designware-core.c    |  8 --------
 drivers/i2c/busses/i2c-designware-pcidrv.c  | 11 +++++++----
 drivers/i2c/busses/i2c-designware-platdrv.c | 16 +++++++++++++++-
 3 files changed, 22 insertions(+), 13 deletions(-)

-- 
2.8.0.rc3

[PATCH 0/4] i2c: designware: runtime pm fix and improve

[Jisheng Zhang]

This series tries to fix one hang issue during probe found on arm
platform, and unbalanced clk enable prepare issue. Then applies similar
change as commit 36d48fb5766a ("i2c: designware-platdrv: enable
RuntimePM before registering to the core"), lastly remove the runtime
suspend prevention in i2c_dw_probe().


Jisheng Zhang (4):
  i2c: designware-platdrv: Fix runtime PM initialization
  i2c: designware-platdrv: fix unbalanced clk enable and prepare
  i2c: designware-pcidrv: enable RuntimePM before registering to the
    core
  i2c: designware: remove runtime suspend prevention during registration

 drivers/i2c/busses/i2c-designware-core.c    |  8 --------
 drivers/i2c/busses/i2c-designware-pcidrv.c  | 11 +++++++----
 drivers/i2c/busses/i2c-designware-platdrv.c | 16 +++++++++++++++-
 3 files changed, 22 insertions(+), 13 deletions(-)

-- 
2.8.0.rc3

[PATCH 0/4] i2c: designware: runtime pm fix and improve

[Jisheng Zhang]

This series tries to fix one hang issue during probe found on arm
platform, and unbalanced clk enable prepare issue. Then applies similar
change as commit 36d48fb5766a ("i2c: designware-platdrv: enable
RuntimePM before registering to the core"), lastly remove the runtime
suspend prevention in i2c_dw_probe().


Jisheng Zhang (4):
  i2c: designware-platdrv: Fix runtime PM initialization
  i2c: designware-platdrv: fix unbalanced clk enable and prepare
  i2c: designware-pcidrv: enable RuntimePM before registering to the
    core
  i2c: designware: remove runtime suspend prevention during registration

 drivers/i2c/busses/i2c-designware-core.c    |  8 --------
 drivers/i2c/busses/i2c-designware-pcidrv.c  | 11 +++++++----
 drivers/i2c/busses/i2c-designware-platdrv.c | 16 +++++++++++++++-
 3 files changed, 22 insertions(+), 13 deletions(-)

-- 
2.8.0.rc3

[PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization

[Jisheng Zhang]

When pm_runtime_enable() was being called, the device's usage counter
was 0, causing the PM layer to runtime-suspend the device.  We then
went on to call i2c_dw_probe() on a suspended device, which could hung.

Fix this by incrementing the usage counter before pm_runtime_enable().

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d656657..00f9e99 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	if (dev->pm_runtime_disabled) {
 		pm_runtime_forbid(&pdev->dev);
 	} else {
+		pm_runtime_get_noresume(&pdev->dev);
 		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
 		pm_runtime_use_autosuspend(&pdev->dev);
 		pm_runtime_set_active(&pdev->dev);
@@ -253,8 +254,19 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	}
 
 	r = i2c_dw_probe(dev);
-	if (r && !dev->pm_runtime_disabled)
+	if (r)
+		goto rpm_disable;
+
+	if (!dev->pm_runtime_disabled)
+		pm_runtime_put_autosuspend(&pdev->dev);
+
+	return 0;
+
+rpm_disable:
+	if (!dev->pm_runtime_disabled) {
 		pm_runtime_disable(&pdev->dev);
+		pm_runtime_put_noidle(&pdev->dev);
+	}
 
 	return r;
 }
-- 
2.8.0.rc3

[PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization

[Jisheng Zhang]

When pm_runtime_enable() was being called, the device's usage counter
was 0, causing the PM layer to runtime-suspend the device.  We then
went on to call i2c_dw_probe() on a suspended device, which could hung.

Fix this by incrementing the usage counter before pm_runtime_enable().

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d656657..00f9e99 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	if (dev->pm_runtime_disabled) {
 		pm_runtime_forbid(&pdev->dev);
 	} else {
+		pm_runtime_get_noresume(&pdev->dev);
 		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
 		pm_runtime_use_autosuspend(&pdev->dev);
 		pm_runtime_set_active(&pdev->dev);
@@ -253,8 +254,19 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	}
 
 	r = i2c_dw_probe(dev);
-	if (r && !dev->pm_runtime_disabled)
+	if (r)
+		goto rpm_disable;
+
+	if (!dev->pm_runtime_disabled)
+		pm_runtime_put_autosuspend(&pdev->dev);
+
+	return 0;
+
+rpm_disable:
+	if (!dev->pm_runtime_disabled) {
 		pm_runtime_disable(&pdev->dev);
+		pm_runtime_put_noidle(&pdev->dev);
+	}
 
 	return r;
 }
-- 
2.8.0.rc3

[PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization

[Jisheng Zhang]

When pm_runtime_enable() was being called, the device's usage counter
was 0, causing the PM layer to runtime-suspend the device.  We then
went on to call i2c_dw_probe() on a suspended device, which could hung.

Fix this by incrementing the usage counter before pm_runtime_enable().

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d656657..00f9e99 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	if (dev->pm_runtime_disabled) {
 		pm_runtime_forbid(&pdev->dev);
 	} else {
+		pm_runtime_get_noresume(&pdev->dev);
 		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
 		pm_runtime_use_autosuspend(&pdev->dev);
 		pm_runtime_set_active(&pdev->dev);
@@ -253,8 +254,19 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	}
 
 	r = i2c_dw_probe(dev);
-	if (r && !dev->pm_runtime_disabled)
+	if (r)
+		goto rpm_disable;
+
+	if (!dev->pm_runtime_disabled)
+		pm_runtime_put_autosuspend(&pdev->dev);
+
+	return 0;
+
+rpm_disable:
+	if (!dev->pm_runtime_disabled) {
 		pm_runtime_disable(&pdev->dev);
+		pm_runtime_put_noidle(&pdev->dev);
+	}
 
 	return r;
 }
-- 
2.8.0.rc3

Re: [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization

[Jarkko Nikula]

On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> When pm_runtime_enable() was being called, the device's usage counter
> was 0, causing the PM layer to runtime-suspend the device.  We then
> went on to call i2c_dw_probe() on a suspended device, which could hung.
>
> Fix this by incrementing the usage counter before pm_runtime_enable().
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>   drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index d656657..00f9e99 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>   	if (dev->pm_runtime_disabled) {
>   		pm_runtime_forbid(&pdev->dev);
>   	} else {
> +		pm_runtime_get_noresume(&pdev->dev);
>   		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
>   		pm_runtime_use_autosuspend(&pdev->dev);
>   		pm_runtime_set_active(&pdev->dev);

pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend 
as far as I understand which made me thinking if there is some other 
issue that cause the symptoms what you are seeing? Like race with 
runtime PM or similar.

-- 
Jarkko

[PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization

[Jarkko Nikula]

On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> When pm_runtime_enable() was being called, the device's usage counter
> was 0, causing the PM layer to runtime-suspend the device.  We then
> went on to call i2c_dw_probe() on a suspended device, which could hung.
>
> Fix this by incrementing the usage counter before pm_runtime_enable().
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>   drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index d656657..00f9e99 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>   	if (dev->pm_runtime_disabled) {
>   		pm_runtime_forbid(&pdev->dev);
>   	} else {
> +		pm_runtime_get_noresume(&pdev->dev);
>   		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
>   		pm_runtime_use_autosuspend(&pdev->dev);
>   		pm_runtime_set_active(&pdev->dev);

pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend 
as far as I understand which made me thinking if there is some other 
issue that cause the symptoms what you are seeing? Like race with 
runtime PM or similar.

-- 
Jarkko

Re: [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization

[Jisheng Zhang]

Dear Jarkko,

On Wed, 20 Apr 2016 16:53:08 +0300 Jarkko Nikula wrote:

> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> > When pm_runtime_enable() was being called, the device's usage counter
> > was 0, causing the PM layer to runtime-suspend the device.  We then
> > went on to call i2c_dw_probe() on a suspended device, which could hung.
> >
> > Fix this by incrementing the usage counter before pm_runtime_enable().
> >
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >   drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
> >   1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index d656657..00f9e99 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> >   	if (dev->pm_runtime_disabled) {
> >   		pm_runtime_forbid(&pdev->dev);
> >   	} else {
> > +		pm_runtime_get_noresume(&pdev->dev);
> >   		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> >   		pm_runtime_use_autosuspend(&pdev->dev);
> >   		pm_runtime_set_active(&pdev->dev);  
> 
> pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend 
> as far as I understand which made me thinking if there is some other 

FWICT, on arm DT platform, the device usage counter is zero at the beginning,
once we enable rpm, the i2c have chance to runtime suspend.

Thanks,
Jisheng

> issue that cause the symptoms what you are seeing? Like race with 
> runtime PM or similar.
> 

[PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization

[Jisheng Zhang]

Dear Jarkko,

On Wed, 20 Apr 2016 16:53:08 +0300 Jarkko Nikula wrote:

> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> > When pm_runtime_enable() was being called, the device's usage counter
> > was 0, causing the PM layer to runtime-suspend the device.  We then
> > went on to call i2c_dw_probe() on a suspended device, which could hung.
> >
> > Fix this by incrementing the usage counter before pm_runtime_enable().
> >
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >   drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
> >   1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index d656657..00f9e99 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> >   	if (dev->pm_runtime_disabled) {
> >   		pm_runtime_forbid(&pdev->dev);
> >   	} else {
> > +		pm_runtime_get_noresume(&pdev->dev);
> >   		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> >   		pm_runtime_use_autosuspend(&pdev->dev);
> >   		pm_runtime_set_active(&pdev->dev);  
> 
> pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend 
> as far as I understand which made me thinking if there is some other 

FWICT, on arm DT platform, the device usage counter is zero at the beginning,
once we enable rpm, the i2c have chance to runtime suspend.

Thanks,
Jisheng

> issue that cause the symptoms what you are seeing? Like race with 
> runtime PM or similar.
> 

Re: [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization

[Jisheng Zhang]

Dear Jarkko,

On Wed, 20 Apr 2016 16:53:08 +0300 Jarkko Nikula wrote:

> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> > When pm_runtime_enable() was being called, the device's usage counter
> > was 0, causing the PM layer to runtime-suspend the device.  We then
> > went on to call i2c_dw_probe() on a suspended device, which could hung.
> >
> > Fix this by incrementing the usage counter before pm_runtime_enable().
> >
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >   drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
> >   1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index d656657..00f9e99 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> >   	if (dev->pm_runtime_disabled) {
> >   		pm_runtime_forbid(&pdev->dev);
> >   	} else {
> > +		pm_runtime_get_noresume(&pdev->dev);
> >   		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> >   		pm_runtime_use_autosuspend(&pdev->dev);
> >   		pm_runtime_set_active(&pdev->dev);  
> 
> pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend 
> as far as I understand which made me thinking if there is some other 

FWICT, on arm DT platform, the device usage counter is zero at the beginning,
once we enable rpm, the i2c have chance to runtime suspend.

Thanks,
Jisheng

> issue that cause the symptoms what you are seeing? Like race with 
> runtime PM or similar.
> 

Re: [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization

[Jarkko Nikula]

Hi

On 04/21/2016 06:08 AM, Jisheng Zhang wrote:
> Dear Jarkko,
>
> On Wed, 20 Apr 2016 16:53:08 +0300 Jarkko Nikula wrote:
>
>> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
>>> When pm_runtime_enable() was being called, the device's usage counter
>>> was 0, causing the PM layer to runtime-suspend the device.  We then
>>> went on to call i2c_dw_probe() on a suspended device, which could hung.
>>>
>>> Fix this by incrementing the usage counter before pm_runtime_enable().
>>>
>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>>> ---
>>>    drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
>>>    1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> index d656657..00f9e99 100644
>>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>>>    	if (dev->pm_runtime_disabled) {
>>>    		pm_runtime_forbid(&pdev->dev);
>>>    	} else {
>>> +		pm_runtime_get_noresume(&pdev->dev);
>>>    		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
>>>    		pm_runtime_use_autosuspend(&pdev->dev);
>>>    		pm_runtime_set_active(&pdev->dev);
>>
>> pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend
>> as far as I understand which made me thinking if there is some other
>
> FWICT, on arm DT platform, the device usage counter is zero at the beginning,
> once we enable rpm, the i2c have chance to runtime suspend.
>
Yes it is same also for ACPI platforms. What I like to understand what 
is actually causing the runtime suspend in your case.

If I add delay longer than 1 second between pm_runtime_enable() and 
i2c_dw_probe() suspends still happens only after probe finishes. It will 
be triggered by the drivers/base/dd.c: driver_probe_device() when code 
calls the pm_request_idle().

I'm wondering is it possible there is a race with deferred probe etc. 
that is causing the issue you are seeing.

-- 
Jarkko

[PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization

[Jarkko Nikula]

Hi

On 04/21/2016 06:08 AM, Jisheng Zhang wrote:
> Dear Jarkko,
>
> On Wed, 20 Apr 2016 16:53:08 +0300 Jarkko Nikula wrote:
>
>> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
>>> When pm_runtime_enable() was being called, the device's usage counter
>>> was 0, causing the PM layer to runtime-suspend the device.  We then
>>> went on to call i2c_dw_probe() on a suspended device, which could hung.
>>>
>>> Fix this by incrementing the usage counter before pm_runtime_enable().
>>>
>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>>> ---
>>>    drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
>>>    1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> index d656657..00f9e99 100644
>>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>>>    	if (dev->pm_runtime_disabled) {
>>>    		pm_runtime_forbid(&pdev->dev);
>>>    	} else {
>>> +		pm_runtime_get_noresume(&pdev->dev);
>>>    		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
>>>    		pm_runtime_use_autosuspend(&pdev->dev);
>>>    		pm_runtime_set_active(&pdev->dev);
>>
>> pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend
>> as far as I understand which made me thinking if there is some other
>
> FWICT, on arm DT platform, the device usage counter is zero at the beginning,
> once we enable rpm, the i2c have chance to runtime suspend.
>
Yes it is same also for ACPI platforms. What I like to understand what 
is actually causing the runtime suspend in your case.

If I add delay longer than 1 second between pm_runtime_enable() and 
i2c_dw_probe() suspends still happens only after probe finishes. It will 
be triggered by the drivers/base/dd.c: driver_probe_device() when code 
calls the pm_request_idle().

I'm wondering is it possible there is a race with deferred probe etc. 
that is causing the issue you are seeing.

-- 
Jarkko

Re: [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization

[Jisheng Zhang]

On Thu, 21 Apr 2016 10:48:56 +0300
Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:

> Hi
> 
> On 04/21/2016 06:08 AM, Jisheng Zhang wrote:
> > Dear Jarkko,
> >
> > On Wed, 20 Apr 2016 16:53:08 +0300 Jarkko Nikula wrote:
> >  
> >> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:  
> >>> When pm_runtime_enable() was being called, the device's usage counter
> >>> was 0, causing the PM layer to runtime-suspend the device.  We then
> >>> went on to call i2c_dw_probe() on a suspended device, which could hung.
> >>>
> >>> Fix this by incrementing the usage counter before pm_runtime_enable().
> >>>
> >>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >>> ---
> >>>    drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
> >>>    1 file changed, 13 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> index d656657..00f9e99 100644
> >>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> >>>    	if (dev->pm_runtime_disabled) {
> >>>    		pm_runtime_forbid(&pdev->dev);
> >>>    	} else {
> >>> +		pm_runtime_get_noresume(&pdev->dev);
> >>>    		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> >>>    		pm_runtime_use_autosuspend(&pdev->dev);
> >>>    		pm_runtime_set_active(&pdev->dev);  
> >>
> >> pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend
> >> as far as I understand which made me thinking if there is some other  
> >
> > FWICT, on arm DT platform, the device usage counter is zero at the beginning,
> > once we enable rpm, the i2c have chance to runtime suspend.
> >  
> Yes it is same also for ACPI platforms. What I like to understand what 
> is actually causing the runtime suspend in your case.

OK, got your point.

> 
> If I add delay longer than 1 second between pm_runtime_enable() and 
> i2c_dw_probe() suspends still happens only after probe finishes. It will 
> be triggered by the drivers/base/dd.c: driver_probe_device() when code 
> calls the pm_request_idle().
> 
> I'm wondering is it possible there is a race with deferred probe etc. 
> that is causing the issue you are seeing.

Hmm, I do see there's deferred probe for the i2c hosts. something like:

[    0.177338] platform f7fcc000.i2c: Driver i2c_designware requests probe deferral

Is it possible as this:

probe => driver return EPROBE_DEFER => pm_request_idle() => probe again
=> since the device usage count is zero, once rpm is enabled, we have
chance to suspend?

And from another side, is it reasonable to always increase the usage count
until probe is done?

Thanks,
Jisheng

[PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization

[Jisheng Zhang]

On Thu, 21 Apr 2016 10:48:56 +0300
Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:

> Hi
> 
> On 04/21/2016 06:08 AM, Jisheng Zhang wrote:
> > Dear Jarkko,
> >
> > On Wed, 20 Apr 2016 16:53:08 +0300 Jarkko Nikula wrote:
> >  
> >> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:  
> >>> When pm_runtime_enable() was being called, the device's usage counter
> >>> was 0, causing the PM layer to runtime-suspend the device.  We then
> >>> went on to call i2c_dw_probe() on a suspended device, which could hung.
> >>>
> >>> Fix this by incrementing the usage counter before pm_runtime_enable().
> >>>
> >>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >>> ---
> >>>    drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
> >>>    1 file changed, 13 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> index d656657..00f9e99 100644
> >>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> >>>    	if (dev->pm_runtime_disabled) {
> >>>    		pm_runtime_forbid(&pdev->dev);
> >>>    	} else {
> >>> +		pm_runtime_get_noresume(&pdev->dev);
> >>>    		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> >>>    		pm_runtime_use_autosuspend(&pdev->dev);
> >>>    		pm_runtime_set_active(&pdev->dev);  
> >>
> >> pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend
> >> as far as I understand which made me thinking if there is some other  
> >
> > FWICT, on arm DT platform, the device usage counter is zero at the beginning,
> > once we enable rpm, the i2c have chance to runtime suspend.
> >  
> Yes it is same also for ACPI platforms. What I like to understand what 
> is actually causing the runtime suspend in your case.

OK, got your point.

> 
> If I add delay longer than 1 second between pm_runtime_enable() and 
> i2c_dw_probe() suspends still happens only after probe finishes. It will 
> be triggered by the drivers/base/dd.c: driver_probe_device() when code 
> calls the pm_request_idle().
> 
> I'm wondering is it possible there is a race with deferred probe etc. 
> that is causing the issue you are seeing.

Hmm, I do see there's deferred probe for the i2c hosts. something like:

[    0.177338] platform f7fcc000.i2c: Driver i2c_designware requests probe deferral

Is it possible as this:

probe => driver return EPROBE_DEFER => pm_request_idle() => probe again
=> since the device usage count is zero, once rpm is enabled, we have
chance to suspend?

And from another side, is it reasonable to always increase the usage count
until probe is done?

Thanks,
Jisheng

Re: [PATCH 1/4] i2c: designware-platdrv: Fix runtime PM initialization

[Jisheng Zhang]

On Thu, 21 Apr 2016 10:48:56 +0300
Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:

> Hi
> 
> On 04/21/2016 06:08 AM, Jisheng Zhang wrote:
> > Dear Jarkko,
> >
> > On Wed, 20 Apr 2016 16:53:08 +0300 Jarkko Nikula wrote:
> >  
> >> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:  
> >>> When pm_runtime_enable() was being called, the device's usage counter
> >>> was 0, causing the PM layer to runtime-suspend the device.  We then
> >>> went on to call i2c_dw_probe() on a suspended device, which could hung.
> >>>
> >>> Fix this by incrementing the usage counter before pm_runtime_enable().
> >>>
> >>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >>> ---
> >>>    drivers/i2c/busses/i2c-designware-platdrv.c | 14 +++++++++++++-
> >>>    1 file changed, 13 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> index d656657..00f9e99 100644
> >>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> >>> @@ -246,6 +246,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
> >>>    	if (dev->pm_runtime_disabled) {
> >>>    		pm_runtime_forbid(&pdev->dev);
> >>>    	} else {
> >>> +		pm_runtime_get_noresume(&pdev->dev);
> >>>    		pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> >>>    		pm_runtime_use_autosuspend(&pdev->dev);
> >>>    		pm_runtime_set_active(&pdev->dev);  
> >>
> >> pm_runtime_enable() here after pm_runtime_set_active() shouldn't suspend
> >> as far as I understand which made me thinking if there is some other  
> >
> > FWICT, on arm DT platform, the device usage counter is zero at the beginning,
> > once we enable rpm, the i2c have chance to runtime suspend.
> >  
> Yes it is same also for ACPI platforms. What I like to understand what 
> is actually causing the runtime suspend in your case.

OK, got your point.

> 
> If I add delay longer than 1 second between pm_runtime_enable() and 
> i2c_dw_probe() suspends still happens only after probe finishes. It will 
> be triggered by the drivers/base/dd.c: driver_probe_device() when code 
> calls the pm_request_idle().
> 
> I'm wondering is it possible there is a race with deferred probe etc. 
> that is causing the issue you are seeing.

Hmm, I do see there's deferred probe for the i2c hosts. something like:

[    0.177338] platform f7fcc000.i2c: Driver i2c_designware requests probe deferral

Is it possible as this:

probe => driver return EPROBE_DEFER => pm_request_idle() => probe again
=> since the device usage count is zero, once rpm is enabled, we have
chance to suspend?

And from another side, is it reasonable to always increase the usage count
until probe is done?

Thanks,
Jisheng

[PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare

[Jisheng Zhang]

If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
disable and unprepare the clk, otherwise the clk enable and prepare
is left unbalanced.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 00f9e99..1488cea 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -268,6 +268,8 @@ rpm_disable:
 		pm_runtime_put_noidle(&pdev->dev);
 	}
 
+	i2c_dw_plat_prepare_clk(dev, false);
+
 	return r;
 }
 
-- 
2.8.0.rc3

[PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare

[Jisheng Zhang]

If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
disable and unprepare the clk, otherwise the clk enable and prepare
is left unbalanced.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 00f9e99..1488cea 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -268,6 +268,8 @@ rpm_disable:
 		pm_runtime_put_noidle(&pdev->dev);
 	}
 
+	i2c_dw_plat_prepare_clk(dev, false);
+
 	return r;
 }
 
-- 
2.8.0.rc3

[PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare

[Jisheng Zhang]

If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
disable and unprepare the clk, otherwise the clk enable and prepare
is left unbalanced.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 00f9e99..1488cea 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -268,6 +268,8 @@ rpm_disable:
 		pm_runtime_put_noidle(&pdev->dev);
 	}
 
+	i2c_dw_plat_prepare_clk(dev, false);
+
 	return r;
 }
 
-- 
2.8.0.rc3

Re: [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare

[Jarkko Nikula]

On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
> disable and unprepare the clk, otherwise the clk enable and prepare
> is left unbalanced.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>   drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 00f9e99..1488cea 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -268,6 +268,8 @@ rpm_disable:
>   		pm_runtime_put_noidle(&pdev->dev);
>   	}
>
> +	i2c_dw_plat_prepare_clk(dev, false);
> +
>   	return r;
>   }
>
This is a bit unclear to me does devm_clk_get take care of clk disabling 
in case of probe error or driver removal?

I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions 
(devm_*)") removed it but at quick look drivers/clk/clk-devres.c: 
devm_clk_release() calls only clk_put and I don't see disable is done 
down the path.

-- 
Jarkko

[PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare

[Jarkko Nikula]

On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
> disable and unprepare the clk, otherwise the clk enable and prepare
> is left unbalanced.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>   drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 00f9e99..1488cea 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -268,6 +268,8 @@ rpm_disable:
>   		pm_runtime_put_noidle(&pdev->dev);
>   	}
>
> +	i2c_dw_plat_prepare_clk(dev, false);
> +
>   	return r;
>   }
>
This is a bit unclear to me does devm_clk_get take care of clk disabling 
in case of probe error or driver removal?

I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions 
(devm_*)") removed it but at quick look drivers/clk/clk-devres.c: 
devm_clk_release() calls only clk_put and I don't see disable is done 
down the path.

-- 
Jarkko

Re: [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare

[Andy Shevchenko]

On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:
> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> > 
> > If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
> > disable and unprepare the clk, otherwise the clk enable and prepare
> > is left unbalanced.
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> >   drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> > b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index 00f9e99..1488cea 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -268,6 +268,8 @@ rpm_disable:
> >   		pm_runtime_put_noidle(&pdev->dev);
> >   	}
> > 
> > +	i2c_dw_plat_prepare_clk(dev, false);
> > +
> >   	return r;
> >   }
> > 
> This is a bit unclear to me does devm_clk_get take care of clk
> disabling 
> in case of probe error or driver removal?
> 
> I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions 
> (devm_*)") removed it but at quick look drivers/clk/clk-devres.c: 
> devm_clk_release() calls only clk_put and I don't see disable is done 
> down the path.

The following is a mistake of the mentioned patch.
-       clk_disable_unprepare(dev->clk);

I did at the same mistake in dw_dmac driver which had been fixed later
in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
function").

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare

[Andy Shevchenko]

On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:
> On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> > 
> > If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
> > disable and unprepare the clk, otherwise the clk enable and prepare
> > is left unbalanced.
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> > ? drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
> > ? 1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> > b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index 00f9e99..1488cea 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -268,6 +268,8 @@ rpm_disable:
> > ??		pm_runtime_put_noidle(&pdev->dev);
> > ??	}
> > 
> > +	i2c_dw_plat_prepare_clk(dev, false);
> > +
> > ??	return r;
> > ? }
> > 
> This is a bit unclear to me does devm_clk_get take care of clk
> disabling?
> in case of probe error or driver removal?
> 
> I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions?
> (devm_*)") removed it but at quick look drivers/clk/clk-devres.c:?
> devm_clk_release() calls only clk_put and I don't see disable is done?
> down the path.

The following is a mistake of the mentioned patch.
-???????clk_disable_unprepare(dev->clk);

I did at the same mistake in dw_dmac driver which had been fixed later
in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
function").

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare

[Jisheng Zhang]

Dear Jarkko, Andy,

On Wed, 20 Apr 2016 17:16:00 +0300 Andy Shevchenko wrote:

> On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:
> > On 04/14/2016 03:53 PM, Jisheng Zhang wrote:  
> > > 
> > > If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
> > > disable and unprepare the clk, otherwise the clk enable and prepare
> > > is left unbalanced.
> > > 
> > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > > ---
> > >   drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> > > b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > index 00f9e99..1488cea 100644
> > > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > @@ -268,6 +268,8 @@ rpm_disable:
> > >   		pm_runtime_put_noidle(&pdev->dev);
> > >   	}
> > > 
> > > +	i2c_dw_plat_prepare_clk(dev, false);
> > > +
> > >   	return r;
> > >   }
> > >   
> > This is a bit unclear to me does devm_clk_get take care of clk
> > disabling 
> > in case of probe error or driver removal?
> > 
> > I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions 
> > (devm_*)") removed it but at quick look drivers/clk/clk-devres.c: 
> > devm_clk_release() calls only clk_put and I don't see disable is done 
> > down the path.  
> 
> The following is a mistake of the mentioned patch.
> -       clk_disable_unprepare(dev->clk);
> 
> I did at the same mistake in dw_dmac driver which had been fixed later
> in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
> function").
> 

As Andy pointed out, managed devm_clk_get can only automatically put clk
but doesn't unprepare and disable the clk

Thanks,
Jisheng

[PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare

[Jisheng Zhang]

Dear Jarkko, Andy,

On Wed, 20 Apr 2016 17:16:00 +0300 Andy Shevchenko wrote:

> On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:
> > On 04/14/2016 03:53 PM, Jisheng Zhang wrote:  
> > > 
> > > If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
> > > disable and unprepare the clk, otherwise the clk enable and prepare
> > > is left unbalanced.
> > > 
> > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > > ---
> > > ? drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
> > > ? 1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> > > b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > index 00f9e99..1488cea 100644
> > > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > @@ -268,6 +268,8 @@ rpm_disable:
> > > ??		pm_runtime_put_noidle(&pdev->dev);
> > > ??	}
> > > 
> > > +	i2c_dw_plat_prepare_clk(dev, false);
> > > +
> > > ??	return r;
> > > ? }
> > >   
> > This is a bit unclear to me does devm_clk_get take care of clk
> > disabling?
> > in case of probe error or driver removal?
> > 
> > I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions?
> > (devm_*)") removed it but at quick look drivers/clk/clk-devres.c:?
> > devm_clk_release() calls only clk_put and I don't see disable is done?
> > down the path.  
> 
> The following is a mistake of the mentioned patch.
> -???????clk_disable_unprepare(dev->clk);
> 
> I did at the same mistake in dw_dmac driver which had been fixed later
> in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
> function").
> 

As Andy pointed out, managed devm_clk_get can only automatically put clk
but doesn't unprepare and disable the clk

Thanks,
Jisheng

Re: [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare

[Jisheng Zhang]

Dear Jarkko, Andy,

On Wed, 20 Apr 2016 17:16:00 +0300 Andy Shevchenko wrote:

> On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:
> > On 04/14/2016 03:53 PM, Jisheng Zhang wrote:  
> > > 
> > > If i2c_dw_probe() fail, we should call i2c_dw_plat_prepare_clk() to
> > > disable and unprepare the clk, otherwise the clk enable and prepare
> > > is left unbalanced.
> > > 
> > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > > ---
> > >   drivers/i2c/busses/i2c-designware-platdrv.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> > > b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > index 00f9e99..1488cea 100644
> > > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > @@ -268,6 +268,8 @@ rpm_disable:
> > >   		pm_runtime_put_noidle(&pdev->dev);
> > >   	}
> > > 
> > > +	i2c_dw_plat_prepare_clk(dev, false);
> > > +
> > >   	return r;
> > >   }
> > >   
> > This is a bit unclear to me does devm_clk_get take care of clk
> > disabling 
> > in case of probe error or driver removal?
> > 
> > I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions 
> > (devm_*)") removed it but at quick look drivers/clk/clk-devres.c: 
> > devm_clk_release() calls only clk_put and I don't see disable is done 
> > down the path.  
> 
> The following is a mistake of the mentioned patch.
> -       clk_disable_unprepare(dev->clk);
> 
> I did at the same mistake in dw_dmac driver which had been fixed later
> in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
> function").
> 

As Andy pointed out, managed devm_clk_get can only automatically put clk
but doesn't unprepare and disable the clk

Thanks,
Jisheng

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare

[Jarkko Nikula]

On 04/21/2016 05:40 AM, Jisheng Zhang wrote:
> Dear Jarkko, Andy,
>
> On Wed, 20 Apr 2016 17:16:00 +0300 Andy Shevchenko wrote:
>
>> On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:
>>> I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions
>>> (devm_*)") removed it but at quick look drivers/clk/clk-devres.c:
>>> devm_clk_release() calls only clk_put and I don't see disable is done
>>> down the path.
>>
>> The following is a mistake of the mentioned patch.
>> -       clk_disable_unprepare(dev->clk);
>>
>> I did at the same mistake in dw_dmac driver which had been fixed later
>> in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
>> function").
>>
>
> As Andy pointed out, managed devm_clk_get can only automatically put clk
> but doesn't unprepare and disable the clk
>
Ok, then it makes sense to move this fix first in the series and queue 
for stable v4.5+. Then another from you, Andy or me for kernels before 
b33af11de236 ("i2c: designware: Do not require clock when SSCN and FFCN 
are provided") that introduced the i2c_dw_plat_prepare_clk().

-- 
Jarkko

[PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare

[Jarkko Nikula]

On 04/21/2016 05:40 AM, Jisheng Zhang wrote:
> Dear Jarkko, Andy,
>
> On Wed, 20 Apr 2016 17:16:00 +0300 Andy Shevchenko wrote:
>
>> On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:
>>> I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions
>>> (devm_*)") removed it but at quick look drivers/clk/clk-devres.c:
>>> devm_clk_release() calls only clk_put and I don't see disable is done
>>> down the path.
>>
>> The following is a mistake of the mentioned patch.
>> -       clk_disable_unprepare(dev->clk);
>>
>> I did at the same mistake in dw_dmac driver which had been fixed later
>> in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
>> function").
>>
>
> As Andy pointed out, managed devm_clk_get can only automatically put clk
> but doesn't unprepare and disable the clk
>
Ok, then it makes sense to move this fix first in the series and queue 
for stable v4.5+. Then another from you, Andy or me for kernels before 
b33af11de236 ("i2c: designware: Do not require clock when SSCN and FFCN 
are provided") that introduced the i2c_dw_plat_prepare_clk().

-- 
Jarkko

Re: [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare

[Jisheng Zhang]

Dear Jarkko,

On Thu, 21 Apr 2016 10:39:50 +0300 Jarkko Nikula wrote:

> On 04/21/2016 05:40 AM, Jisheng Zhang wrote:
> > Dear Jarkko, Andy,
> >
> > On Wed, 20 Apr 2016 17:16:00 +0300 Andy Shevchenko wrote:
> >  
> >> On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:  
> >>> I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions
> >>> (devm_*)") removed it but at quick look drivers/clk/clk-devres.c:
> >>> devm_clk_release() calls only clk_put and I don't see disable is done
> >>> down the path.  
> >>
> >> The following is a mistake of the mentioned patch.
> >> -       clk_disable_unprepare(dev->clk);
> >>
> >> I did at the same mistake in dw_dmac driver which had been fixed later
> >> in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
> >> function").
> >>  
> >
> > As Andy pointed out, managed devm_clk_get can only automatically put clk
> > but doesn't unprepare and disable the clk
> >  
> Ok, then it makes sense to move this fix first in the series and queue 
> for stable v4.5+. Then another from you, Andy or me for kernels before 
> b33af11de236 ("i2c: designware: Do not require clock when SSCN and FFCN 
> are provided") that introduced the i2c_dw_plat_prepare_clk().

Sounds a good idea. Will send a separate fix for this purpose soon.

Thanks,
Jisheng

[PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare

[Jisheng Zhang]

Dear Jarkko,

On Thu, 21 Apr 2016 10:39:50 +0300 Jarkko Nikula wrote:

> On 04/21/2016 05:40 AM, Jisheng Zhang wrote:
> > Dear Jarkko, Andy,
> >
> > On Wed, 20 Apr 2016 17:16:00 +0300 Andy Shevchenko wrote:
> >  
> >> On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:  
> >>> I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions
> >>> (devm_*)") removed it but at quick look drivers/clk/clk-devres.c:
> >>> devm_clk_release() calls only clk_put and I don't see disable is done
> >>> down the path.  
> >>
> >> The following is a mistake of the mentioned patch.
> >> -       clk_disable_unprepare(dev->clk);
> >>
> >> I did at the same mistake in dw_dmac driver which had been fixed later
> >> in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
> >> function").
> >>  
> >
> > As Andy pointed out, managed devm_clk_get can only automatically put clk
> > but doesn't unprepare and disable the clk
> >  
> Ok, then it makes sense to move this fix first in the series and queue 
> for stable v4.5+. Then another from you, Andy or me for kernels before 
> b33af11de236 ("i2c: designware: Do not require clock when SSCN and FFCN 
> are provided") that introduced the i2c_dw_plat_prepare_clk().

Sounds a good idea. Will send a separate fix for this purpose soon.

Thanks,
Jisheng

Re: [PATCH 2/4] i2c: designware-platdrv: fix unbalanced clk enable and prepare

[Jisheng Zhang]

Dear Jarkko,

On Thu, 21 Apr 2016 10:39:50 +0300 Jarkko Nikula wrote:

> On 04/21/2016 05:40 AM, Jisheng Zhang wrote:
> > Dear Jarkko, Andy,
> >
> > On Wed, 20 Apr 2016 17:16:00 +0300 Andy Shevchenko wrote:
> >  
> >> On Wed, 2016-04-20 at 15:55 +0300, Jarkko Nikula wrote:  
> >>> I see Andy's 1cb715ca4694 ("i2c-designware: move to managed functions
> >>> (devm_*)") removed it but at quick look drivers/clk/clk-devres.c:
> >>> devm_clk_release() calls only clk_put and I don't see disable is done
> >>> down the path.  
> >>
> >> The following is a mistake of the mentioned patch.
> >> -       clk_disable_unprepare(dev->clk);
> >>
> >> I did at the same mistake in dw_dmac driver which had been fixed later
> >> in the commit 8be4f523b480 ("dmaengine: dw: fix regression in dw_probe()
> >> function").
> >>  
> >
> > As Andy pointed out, managed devm_clk_get can only automatically put clk
> > but doesn't unprepare and disable the clk
> >  
> Ok, then it makes sense to move this fix first in the series and queue 
> for stable v4.5+. Then another from you, Andy or me for kernels before 
> b33af11de236 ("i2c: designware: Do not require clock when SSCN and FFCN 
> are provided") that introduced the i2c_dw_plat_prepare_clk().

Sounds a good idea. Will send a separate fix for this purpose soon.

Thanks,
Jisheng

[PATCH 3/4] i2c: designware-pcidrv: enable RuntimePM before registering to the core

[Jisheng Zhang]

As pointed out by commit 36d48fb5766a ("i2c: designware-platdrv: enable
RuntimePM before registering to the core"), "The core may register
clients attached to this master which may use funtionality from the
master", so enable RuntimePM before registering to the core.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/i2c/busses/i2c-designware-pcidrv.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 7368be0..41a01a7 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -248,14 +248,17 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->nr = controller->bus_num;
 
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_allow(&pdev->dev);
+
 	r = i2c_dw_probe(dev);
-	if (r)
+	if (r) {
+		pm_runtime_forbid(&pdev->dev);
 		return r;
+	}
 
-	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
-	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_put_autosuspend(&pdev->dev);
-	pm_runtime_allow(&pdev->dev);
 
 	return 0;
 }
-- 
2.8.0.rc3

[PATCH 3/4] i2c: designware-pcidrv: enable RuntimePM before registering to the core

[Jisheng Zhang]

As pointed out by commit 36d48fb5766a ("i2c: designware-platdrv: enable
RuntimePM before registering to the core"), "The core may register
clients attached to this master which may use funtionality from the
master", so enable RuntimePM before registering to the core.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/i2c/busses/i2c-designware-pcidrv.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 7368be0..41a01a7 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -248,14 +248,17 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->nr = controller->bus_num;
 
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_allow(&pdev->dev);
+
 	r = i2c_dw_probe(dev);
-	if (r)
+	if (r) {
+		pm_runtime_forbid(&pdev->dev);
 		return r;
+	}
 
-	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
-	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_put_autosuspend(&pdev->dev);
-	pm_runtime_allow(&pdev->dev);
 
 	return 0;
 }
-- 
2.8.0.rc3

[PATCH 3/4] i2c: designware-pcidrv: enable RuntimePM before registering to the core

[Jisheng Zhang]

As pointed out by commit 36d48fb5766a ("i2c: designware-platdrv: enable
RuntimePM before registering to the core"), "The core may register
clients attached to this master which may use funtionality from the
master", so enable RuntimePM before registering to the core.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/i2c/busses/i2c-designware-pcidrv.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 7368be0..41a01a7 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -248,14 +248,17 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->nr = controller->bus_num;
 
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_allow(&pdev->dev);
+
 	r = i2c_dw_probe(dev);
-	if (r)
+	if (r) {
+		pm_runtime_forbid(&pdev->dev);
 		return r;
+	}
 
-	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
-	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_put_autosuspend(&pdev->dev);
-	pm_runtime_allow(&pdev->dev);
 
 	return 0;
 }
-- 
2.8.0.rc3

Re: [PATCH 3/4] i2c: designware-pcidrv: enable RuntimePM before registering to the core

[Jarkko Nikula]

On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> As pointed out by commit 36d48fb5766a ("i2c: designware-platdrv: enable
> RuntimePM before registering to the core"), "The core may register
> clients attached to this master which may use funtionality from the
> master", so enable RuntimePM before registering to the core.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>   drivers/i2c/busses/i2c-designware-pcidrv.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
> index 7368be0..41a01a7 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -248,14 +248,17 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
>   	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
>   	adap->nr = controller->bus_num;
>
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_allow(&pdev->dev);
> +
>   	r = i2c_dw_probe(dev);
> -	if (r)
> +	if (r) {
> +		pm_runtime_forbid(&pdev->dev);
>   		return r;
> +	}
>
> -	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> -	pm_runtime_use_autosuspend(&pdev->dev);
>   	pm_runtime_put_autosuspend(&pdev->dev);
> -	pm_runtime_allow(&pdev->dev);
>
Last time I checked this I didn't see urgent reason for the move because 
runtime PM is enabled for PCI devices by default via pci_device_add() -> 
pci_init_capabilities() -> pci_pm_init() -> pm_runtime_enable().

-- 
Jarkko

[PATCH 3/4] i2c: designware-pcidrv: enable RuntimePM before registering to the core

[Jarkko Nikula]

On 04/14/2016 03:53 PM, Jisheng Zhang wrote:
> As pointed out by commit 36d48fb5766a ("i2c: designware-platdrv: enable
> RuntimePM before registering to the core"), "The core may register
> clients attached to this master which may use funtionality from the
> master", so enable RuntimePM before registering to the core.
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>   drivers/i2c/busses/i2c-designware-pcidrv.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
> index 7368be0..41a01a7 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -248,14 +248,17 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
>   	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
>   	adap->nr = controller->bus_num;
>
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_allow(&pdev->dev);
> +
>   	r = i2c_dw_probe(dev);
> -	if (r)
> +	if (r) {
> +		pm_runtime_forbid(&pdev->dev);
>   		return r;
> +	}
>
> -	pm_runtime_set_autosuspend_delay(&pdev->dev, 1000);
> -	pm_runtime_use_autosuspend(&pdev->dev);
>   	pm_runtime_put_autosuspend(&pdev->dev);
> -	pm_runtime_allow(&pdev->dev);
>
Last time I checked this I didn't see urgent reason for the move because 
runtime PM is enabled for PCI devices by default via pci_device_add() -> 
pci_init_capabilities() -> pci_pm_init() -> pm_runtime_enable().

-- 
Jarkko

[PATCH 4/4] i2c: designware: remove runtime suspend prevention during registration

[Jisheng Zhang]

Now all users make sure there won't be runtime suspend when calling
i2c_dw_probe(), so we can remove the prevention code now.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/i2c/busses/i2c-designware-core.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 99b54be..4255eaa 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -881,17 +881,9 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
 		return r;
 	}
 
-	/*
-	 * Increment PM usage count during adapter registration in order to
-	 * avoid possible spurious runtime suspend when adapter device is
-	 * registered to the device core and immediate resume in case bus has
-	 * registered I2C slaves that do I2C transfers in their probe.
-	 */
-	pm_runtime_get_noresume(dev->dev);
 	r = i2c_add_numbered_adapter(adap);
 	if (r)
 		dev_err(dev->dev, "failure adding adapter: %d\n", r);
-	pm_runtime_put_noidle(dev->dev);
 
 	return r;
 }
-- 
2.8.0.rc3

[PATCH 4/4] i2c: designware: remove runtime suspend prevention during registration

[Jisheng Zhang]

Now all users make sure there won't be runtime suspend when calling
i2c_dw_probe(), so we can remove the prevention code now.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/i2c/busses/i2c-designware-core.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 99b54be..4255eaa 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -881,17 +881,9 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
 		return r;
 	}
 
-	/*
-	 * Increment PM usage count during adapter registration in order to
-	 * avoid possible spurious runtime suspend when adapter device is
-	 * registered to the device core and immediate resume in case bus has
-	 * registered I2C slaves that do I2C transfers in their probe.
-	 */
-	pm_runtime_get_noresume(dev->dev);
 	r = i2c_add_numbered_adapter(adap);
 	if (r)
 		dev_err(dev->dev, "failure adding adapter: %d\n", r);
-	pm_runtime_put_noidle(dev->dev);
 
 	return r;
 }
-- 
2.8.0.rc3

[PATCH 4/4] i2c: designware: remove runtime suspend prevention during registration

[Jisheng Zhang]

Now all users make sure there won't be runtime suspend when calling
i2c_dw_probe(), so we can remove the prevention code now.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 drivers/i2c/busses/i2c-designware-core.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 99b54be..4255eaa 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -881,17 +881,9 @@ int i2c_dw_probe(struct dw_i2c_dev *dev)
 		return r;
 	}
 
-	/*
-	 * Increment PM usage count during adapter registration in order to
-	 * avoid possible spurious runtime suspend when adapter device is
-	 * registered to the device core and immediate resume in case bus has
-	 * registered I2C slaves that do I2C transfers in their probe.
-	 */
-	pm_runtime_get_noresume(dev->dev);
 	r = i2c_add_numbered_adapter(adap);
 	if (r)
 		dev_err(dev->dev, "failure adding adapter: %d\n", r);
-	pm_runtime_put_noidle(dev->dev);
 
 	return r;
 }
-- 
2.8.0.rc3