[PATCH 1/2] usb: dwc3: of-simple: Don't fail if no clock entries

[PATCH 1/2] usb: dwc3: of-simple: Don't fail if no clock entries

[Roger Quadros]

of_count_phandle_with_args() returns -ENOENT (-2) if
there are no clock entries. Don't fail in such a case.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/dwc3-of-simple.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
index cb2ee96..e98d221 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -36,11 +36,11 @@ static int dwc3_of_simple_clk_init(struct dwc3_of_simple *simple, int count)
 	struct device_node	*np = dev->of_node;
 	int			i;
 
-	simple->num_clocks = count;
-
-	if (!count)
+	if (count <= 0)
 		return 0;
 
+	simple->num_clocks = count;
+
 	simple->clks = devm_kcalloc(dev, simple->num_clocks,
 			sizeof(struct clk *), GFP_KERNEL);
 	if (!simple->clks)
-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

[PATCH 2/2] usb: dwc3: of_simple: don't call pm_runtime_set_active()

[Roger Quadros]

Don't call pm_runtime_set_active() as it will prevent the device
from being activated in the next pm_runtime_get_sync() call.

Also call pm_runtime_get_sync() before of_platform_populate().

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/dwc3-of-simple.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
index e98d221..2cbb5c0 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_resetc_assert;
 
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
+
 	ret = of_platform_populate(np, NULL, NULL, dev);
 	if (ret) {
 		for (i = 0; i < simple->num_clocks; i++) {
@@ -131,10 +134,6 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
 		goto err_resetc_assert;
 	}
 
-	pm_runtime_set_active(dev);
-	pm_runtime_enable(dev);
-	pm_runtime_get_sync(dev);
-
 	return 0;
 
 err_resetc_assert:
-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/2] usb: dwc3: of_simple: don't call pm_runtime_set_active()

[Johan Hovold]

On Mon, May 28, 2018 at 05:36:14PM +0300, Roger Quadros wrote:
> Don't call pm_runtime_set_active() as it will prevent the device
> from being activated in the next pm_runtime_get_sync() call.
> 
> Also call pm_runtime_get_sync() before of_platform_populate().

This paragraph describes what you do, but not why do it.

> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/usb/dwc3/dwc3-of-simple.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
> index e98d221..2cbb5c0 100644
> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_resetc_assert;
>  
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);

This breaks runtime pm as you now get a second round of clock enables
which are never balanced on runtime suspend (the clocks are first
enabled in dwc3_of_simple_clk_init() above and with your change again in
dwc3_of_simple_runtime_resume()).

On the other hand, we currently return from probe() with a positive RPM
count so perhaps the RPM callbacks can just be removed altogether (i.e.
unless some other entity drops that count at some point before
remove()).

>  	ret = of_platform_populate(np, NULL, NULL, dev);
>  	if (ret) {
>  		for (i = 0; i < simple->num_clocks; i++) {
> @@ -131,10 +134,6 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>  		goto err_resetc_assert;
>  	}
>  
> -	pm_runtime_set_active(dev);
> -	pm_runtime_enable(dev);
> -	pm_runtime_get_sync(dev);
> -
>  	return 0;
>  
>  err_resetc_assert:

Also note that there's currently a use-after-free in remove(), where
pm_runtime_put_sync() is called after the clocks have been put.
Something like the below (untested) patch should fix it.

Johan


>From 35c384c31010c344d403c26fc0a1dde0fd68ef4a Mon Sep 17 00:00:00 2001
From: Johan Hovold <johan@kernel.org>
Date: Mon, 28 May 2018 17:31:45 +0200
Subject: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove

The clocks have already been explicitly disabled and put as part of
remove() so the runtime suspend callback must not be run when balancing
the runtime PM usage count before returning.

Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer")
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/dwc3/dwc3-of-simple.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
index cb2ee96fd3e8..b9c869cd6585 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -165,8 +165,9 @@ static int dwc3_of_simple_remove(struct platform_device *pdev)
 
 	reset_control_put(simple->resets);
 
-	pm_runtime_put_sync(dev);
+	pm_runtime_put_noidle(dev);
 	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
 
 	return 0;
 }
-- 
2.17.0

Re: [PATCH 2/2] usb: dwc3: of_simple: don't call pm_runtime_set_active()

[Johan Hovold]

Hi Felipe,

On Mon, May 28, 2018 at 05:41:48PM +0200, Johan Hovold wrote:
> On Mon, May 28, 2018 at 05:36:14PM +0300, Roger Quadros wrote:
> > Don't call pm_runtime_set_active() as it will prevent the device
> > from being activated in the next pm_runtime_get_sync() call.
> > 
> > Also call pm_runtime_get_sync() before of_platform_populate().
> 
> This paragraph describes what you do, but not why do it.
> 
> > Signed-off-by: Roger Quadros <rogerq@ti.com>
> > ---
> >  drivers/usb/dwc3/dwc3-of-simple.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
> > index e98d221..2cbb5c0 100644
> > --- a/drivers/usb/dwc3/dwc3-of-simple.c
> > +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> > @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		goto err_resetc_assert;
> >  
> > +	pm_runtime_enable(dev);
> > +	pm_runtime_get_sync(dev);
> 
> This breaks runtime pm as you now get a second round of clock enables
> which are never balanced on runtime suspend (the clocks are first
> enabled in dwc3_of_simple_clk_init() above and with your change again in
> dwc3_of_simple_runtime_resume()).
> 
> On the other hand, we currently return from probe() with a positive RPM
> count so perhaps the RPM callbacks can just be removed altogether (i.e.
> unless some other entity drops that count at some point before
> remove()).
> 
> >  	ret = of_platform_populate(np, NULL, NULL, dev);
> >  	if (ret) {
> >  		for (i = 0; i < simple->num_clocks; i++) {
> > @@ -131,10 +134,6 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
> >  		goto err_resetc_assert;
> >  	}
> >  
> > -	pm_runtime_set_active(dev);
> > -	pm_runtime_enable(dev);
> > -	pm_runtime_get_sync(dev);
> > -
> >  	return 0;
> >  
> >  err_resetc_assert:
> 
> Also note that there's currently a use-after-free in remove(), where
> pm_runtime_put_sync() is called after the clocks have been put.
> Something like the below (untested) patch should fix it.

What about the use-after-free in remove? Shall I resubmit the fix below
separately?

Thanks,
Johan

> From 35c384c31010c344d403c26fc0a1dde0fd68ef4a Mon Sep 17 00:00:00 2001
> From: Johan Hovold <johan@kernel.org>
> Date: Mon, 28 May 2018 17:31:45 +0200
> Subject: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
> 
> The clocks have already been explicitly disabled and put as part of
> remove() so the runtime suspend callback must not be run when balancing
> the runtime PM usage count before returning.
> 
> Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer")
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/usb/dwc3/dwc3-of-simple.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
> index cb2ee96fd3e8..b9c869cd6585 100644
> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> @@ -165,8 +165,9 @@ static int dwc3_of_simple_remove(struct platform_device *pdev)
>  
>  	reset_control_put(simple->resets);
>  
> -	pm_runtime_put_sync(dev);
> +	pm_runtime_put_noidle(dev);
>  	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);
>  
>  	return 0;
>  }

Re: [PATCH 2/2] usb: dwc3: of_simple: don't call pm_runtime_set_active()

[Alan Stern]

On Thu, 31 May 2018, Johan Hovold wrote:

> > This breaks runtime pm as you now get a second round of clock enables
> > which are never balanced on runtime suspend (the clocks are first
> > enabled in dwc3_of_simple_clk_init() above and with your change again in
> > dwc3_of_simple_runtime_resume()).
> > 
> > On the other hand, we currently return from probe() with a positive RPM
> > count so perhaps the RPM callbacks can just be removed altogether (i.e.
> > unless some other entity drops that count at some point before
> > remove()).
> > 
> > >  	ret = of_platform_populate(np, NULL, NULL, dev);
> > >  	if (ret) {
> > >  		for (i = 0; i < simple->num_clocks; i++) {
> > > @@ -131,10 +134,6 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
> > >  		goto err_resetc_assert;
> > >  	}
> > >  
> > > -	pm_runtime_set_active(dev);
> > > -	pm_runtime_enable(dev);
> > > -	pm_runtime_get_sync(dev);
> > > -
> > >  	return 0;
> > >  
> > >  err_resetc_assert:
> > 
> > Also note that there's currently a use-after-free in remove(), where
> > pm_runtime_put_sync() is called after the clocks have been put.
> > Something like the below (untested) patch should fix it.
> 
> What about the use-after-free in remove? Shall I resubmit the fix below
> separately?
> 
> Thanks,
> Johan
> 
> > From 35c384c31010c344d403c26fc0a1dde0fd68ef4a Mon Sep 17 00:00:00 2001
> > From: Johan Hovold <johan@kernel.org>
> > Date: Mon, 28 May 2018 17:31:45 +0200
> > Subject: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
> > 
> > The clocks have already been explicitly disabled and put as part of
> > remove() so the runtime suspend callback must not be run when balancing
> > the runtime PM usage count before returning.
> > 
> > Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer")
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/usb/dwc3/dwc3-of-simple.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
> > index cb2ee96fd3e8..b9c869cd6585 100644
> > --- a/drivers/usb/dwc3/dwc3-of-simple.c
> > +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> > @@ -165,8 +165,9 @@ static int dwc3_of_simple_remove(struct platform_device *pdev)
> >  
> >  	reset_control_put(simple->resets);
> >  
> > -	pm_runtime_put_sync(dev);
> > +	pm_runtime_put_noidle(dev);
> >  	pm_runtime_disable(dev);
> > +	pm_runtime_set_suspended(dev);
> >  
> >  	return 0;
> >  }

This is a little racy -- there might be a runtime-suspend callback
between the put_noidle and the disable.  (The put_noidle itself won't
cause a callback to happen, but something else could.)

It would be better to do the disable first and then the put_noidle.

Alan Stern

Re: [PATCH 2/2] usb: dwc3: of_simple: don't call pm_runtime_set_active()

[Johan Hovold]

On Thu, May 31, 2018 at 10:07:05AM -0400, Alan Stern wrote:
> On Thu, 31 May 2018, Johan Hovold wrote:
> 
> > > This breaks runtime pm as you now get a second round of clock enables
> > > which are never balanced on runtime suspend (the clocks are first
> > > enabled in dwc3_of_simple_clk_init() above and with your change again in
> > > dwc3_of_simple_runtime_resume()).
> > > 
> > > On the other hand, we currently return from probe() with a positive RPM
> > > count so perhaps the RPM callbacks can just be removed altogether (i.e.
> > > unless some other entity drops that count at some point before
> > > remove()).
> > > 
> > > >  	ret = of_platform_populate(np, NULL, NULL, dev);
> > > >  	if (ret) {
> > > >  		for (i = 0; i < simple->num_clocks; i++) {
> > > > @@ -131,10 +134,6 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
> > > >  		goto err_resetc_assert;
> > > >  	}
> > > >  
> > > > -	pm_runtime_set_active(dev);
> > > > -	pm_runtime_enable(dev);
> > > > -	pm_runtime_get_sync(dev);
> > > > -
> > > >  	return 0;
> > > >  
> > > >  err_resetc_assert:
> > > 
> > > Also note that there's currently a use-after-free in remove(), where
> > > pm_runtime_put_sync() is called after the clocks have been put.
> > > Something like the below (untested) patch should fix it.
> > 
> > What about the use-after-free in remove? Shall I resubmit the fix below
> > separately?
> > 
> > Thanks,
> > Johan
> > 
> > > From 35c384c31010c344d403c26fc0a1dde0fd68ef4a Mon Sep 17 00:00:00 2001
> > > From: Johan Hovold <johan@kernel.org>
> > > Date: Mon, 28 May 2018 17:31:45 +0200
> > > Subject: [PATCH] usb: dwc3: of-simple: fix use-after-free on remove
> > > 
> > > The clocks have already been explicitly disabled and put as part of
> > > remove() so the runtime suspend callback must not be run when balancing
> > > the runtime PM usage count before returning.
> > > 
> > > Fixes: 16adc674d0d6 ("usb: dwc3: add generic OF glue layer")
> > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > ---
> > >  drivers/usb/dwc3/dwc3-of-simple.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
> > > index cb2ee96fd3e8..b9c869cd6585 100644
> > > --- a/drivers/usb/dwc3/dwc3-of-simple.c
> > > +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> > > @@ -165,8 +165,9 @@ static int dwc3_of_simple_remove(struct platform_device *pdev)
> > >  
> > >  	reset_control_put(simple->resets);
> > >  
> > > -	pm_runtime_put_sync(dev);
> > > +	pm_runtime_put_noidle(dev);
> > >  	pm_runtime_disable(dev);
> > > +	pm_runtime_set_suspended(dev);
> > >  
> > >  	return 0;
> > >  }
> 
> This is a little racy -- there might be a runtime-suspend callback
> between the put_noidle and the disable.  (The put_noidle itself won't
> cause a callback to happen, but something else could.)
> 
> It would be better to do the disable first and then the put_noidle.

Good point. I'll send a v2 for Felipe to consider.

Thanks,
Johan

Re: [PATCH 2/2] usb: dwc3: of_simple: don't call pm_runtime_set_active()

[Felipe Balbi]

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

Roger Quadros <rogerq@ti.com> writes:

> Don't call pm_runtime_set_active() as it will prevent the device
> from being activated in the next pm_runtime_get_sync() call.
>
> Also call pm_runtime_get_sync() before of_platform_populate().
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>

This patch is wrong.

> ---
>  drivers/usb/dwc3/dwc3-of-simple.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
> index e98d221..2cbb5c0 100644
> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_resetc_assert;
>  
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);

No, this is the wrong way to do things. My device should be enabled
already from probe, specially since I have already enabled clocks.

>  	ret = of_platform_populate(np, NULL, NULL, dev);
>  	if (ret) {
>  		for (i = 0; i < simple->num_clocks; i++) {
> @@ -131,10 +134,6 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>  		goto err_resetc_assert;
>  	}
>  
> -	pm_runtime_set_active(dev);
> -	pm_runtime_enable(dev);
> -	pm_runtime_get_sync(dev);

this hunk is not wrong at all.

-- 
balbi

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

Re: [PATCH 2/2] usb: dwc3: of_simple: don't call pm_runtime_set_active()

[Roger Quadros]

On 30/05/18 15:31, Felipe Balbi wrote:
> Roger Quadros <rogerq@ti.com> writes:
> 
>> Don't call pm_runtime_set_active() as it will prevent the device
>> from being activated in the next pm_runtime_get_sync() call.
>>
>> Also call pm_runtime_get_sync() before of_platform_populate().
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> This patch is wrong.
> 
>> ---
>>  drivers/usb/dwc3/dwc3-of-simple.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
>> index e98d221..2cbb5c0 100644
>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>> @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>>  	if (ret)
>>  		goto err_resetc_assert;
>>  
>> +	pm_runtime_enable(dev);
>> +	pm_runtime_get_sync(dev);
> 
> No, this is the wrong way to do things. My device should be enabled
> already from probe, specially since I have already enabled clocks.

As far as I understood just enabling clocks (which might not include bus clock)
doesn't ensure device is enabled.

Did you mean that I don't need to do a pm_runtime_get_sync() to enable my device in probe?
Who is enabling by device for me then? Is device core supposed to do it?

> 
>>  	ret = of_platform_populate(np, NULL, NULL, dev);
>>  	if (ret) {
>>  		for (i = 0; i < simple->num_clocks; i++) {
>> @@ -131,10 +134,6 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>>  		goto err_resetc_assert;
>>  	}
>>  
>> -	pm_runtime_set_active(dev);
>> -	pm_runtime_enable(dev);
>> -	pm_runtime_get_sync(dev);
> 
> this hunk is not wrong at all.
> 

The issue I was facing is that without this patch my device wasn't being enabled
as pm_runtime_set_active() is being done _before_ pm_runtime_get_sync().
It could be an issue with the platform's PM domain code as well.

Tero/Dave what do you think?

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/2] usb: dwc3: of_simple: don't call pm_runtime_set_active()

[Felipe Balbi]

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


Hi,

Roger Quadros <rogerq@ti.com> writes:

> On 30/05/18 15:31, Felipe Balbi wrote:
>> Roger Quadros <rogerq@ti.com> writes:
>> 
>>> Don't call pm_runtime_set_active() as it will prevent the device
>>> from being activated in the next pm_runtime_get_sync() call.
>>>
>>> Also call pm_runtime_get_sync() before of_platform_populate().
>>>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> 
>> This patch is wrong.
>> 
>>> ---
>>>  drivers/usb/dwc3/dwc3-of-simple.c | 7 +++----
>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
>>> index e98d221..2cbb5c0 100644
>>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>>> @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>>>  	if (ret)
>>>  		goto err_resetc_assert;
>>>  
>>> +	pm_runtime_enable(dev);
>>> +	pm_runtime_get_sync(dev);
>> 
>> No, this is the wrong way to do things. My device should be enabled
>> already from probe, specially since I have already enabled clocks.
>
> As far as I understood just enabling clocks (which might not include bus clock)
> doesn't ensure device is enabled.
>
> Did you mean that I don't need to do a pm_runtime_get_sync() to enable my device in probe?
> Who is enabling by device for me then? Is device core supposed to do it?

Not device core, but the bus code. Look at how PCI handles it. IIRC,
only TI's omap_device behaves peculiarly WRT probe & pm runtime.

> The issue I was facing is that without this patch my device wasn't being enabled
> as pm_runtime_set_active() is being done _before_ pm_runtime_get_sync().
> It could be an issue with the platform's PM domain code as well.

Could be

-- 
balbi

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

Re: [PATCH 2/2] usb: dwc3: of_simple: don't call pm_runtime_set_active()

[Roger Quadros]

On 31/05/18 10:59, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
> 
>> On 30/05/18 15:31, Felipe Balbi wrote:
>>> Roger Quadros <rogerq@ti.com> writes:
>>>
>>>> Don't call pm_runtime_set_active() as it will prevent the device
>>>> from being activated in the next pm_runtime_get_sync() call.
>>>>
>>>> Also call pm_runtime_get_sync() before of_platform_populate().
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>
>>> This patch is wrong.
>>>
>>>> ---
>>>>  drivers/usb/dwc3/dwc3-of-simple.c | 7 +++----
>>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
>>>> index e98d221..2cbb5c0 100644
>>>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
>>>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>>>> @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>>>>  	if (ret)
>>>>  		goto err_resetc_assert;
>>>>  
>>>> +	pm_runtime_enable(dev);
>>>> +	pm_runtime_get_sync(dev);
>>>
>>> No, this is the wrong way to do things. My device should be enabled
>>> already from probe, specially since I have already enabled clocks.
>>
>> As far as I understood just enabling clocks (which might not include bus clock)
>> doesn't ensure device is enabled.
>>
>> Did you mean that I don't need to do a pm_runtime_get_sync() to enable my device in probe?
>> Who is enabling by device for me then? Is device core supposed to do it?
> 
> Not device core, but the bus code. Look at how PCI handles it. IIRC,
> only TI's omap_device behaves peculiarly WRT probe & pm runtime.

PCI does it doesn't mean it is a rule that bus code has to enable the device before probe.
At least platform bus doesn't seem to do it as part of of_platform_populate().

see __device_attach() and driver_probe_device() in drivers/base/dd.c

It does a pm_runtime_get_sync() on dev->parent but not on dev.

Also, from section 5 of Documentation/power/runtime_pm.txt

"In addition to that, the initial runtime PM status of all devices is
'suspended', but it need not reflect the actual physical state of the device.
Thus, if the device is initially active (i.e. it is able to process I/O), its
runtime PM status must be changed to 'active', with the help of
pm_runtime_set_active(), before pm_runtime_enable() is called for the device."

"Note, if the device may execute pm_runtime calls during the probe (such as
if it is registers with a subsystem that may call back in) then the
pm_runtime_get_sync() call paired with a pm_runtime_put() call will be
appropriate to ensure that the device is not put back to sleep during the
probe. This can happen with systems such as the network device layer."

So looks like we can't assume that the device is "active" when probe() is called.
Which means, we need to do

pm_runtime_get_sync();
enable optional clocks;

pm_runtime_forbid(); if we don't yet want runtime suspend/resume.
OR
pm_runtime_put(); if we want runtime suspend/resume support.

I don't think we should be calling pm_runtime_set_active() as we can't be sure of the
initial state of the device for different buses.
> 
>> The issue I was facing is that without this patch my device wasn't being enabled
>> as pm_runtime_set_active() is being done _before_ pm_runtime_get_sync().
>> It could be an issue with the platform's PM domain code as well.
> 
> Could be
> 

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH 2/2] usb: dwc3: of_simple: don't call pm_runtime_set_active()

[Johan Hovold]

On Wed, Jun 13, 2018 at 02:15:11PM +0300, Roger Quadros wrote:
> On 31/05/18 10:59, Felipe Balbi wrote:
> > 
> > Hi,
> > 
> > Roger Quadros <rogerq@ti.com> writes:
> > 
> >> On 30/05/18 15:31, Felipe Balbi wrote:
> >>> Roger Quadros <rogerq@ti.com> writes:
> >>>
> >>>> Don't call pm_runtime_set_active() as it will prevent the device
> >>>> from being activated in the next pm_runtime_get_sync() call.
> >>>>
> >>>> Also call pm_runtime_get_sync() before of_platform_populate().
> >>>>
> >>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> >>>
> >>> This patch is wrong.
> >>>
> >>>> ---
> >>>>  drivers/usb/dwc3/dwc3-of-simple.c | 7 +++----
> >>>>  1 file changed, 3 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
> >>>> index e98d221..2cbb5c0 100644
> >>>> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> >>>> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> >>>> @@ -121,6 +121,9 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
> >>>>  	if (ret)
> >>>>  		goto err_resetc_assert;
> >>>>  
> >>>> +	pm_runtime_enable(dev);
> >>>> +	pm_runtime_get_sync(dev);
> >>>
> >>> No, this is the wrong way to do things. My device should be enabled
> >>> already from probe, specially since I have already enabled clocks.
> >>
> >> As far as I understood just enabling clocks (which might not
> >> include bus clock) doesn't ensure device is enabled.

In general, that cannot be assumed no. Specifically, this cannot
currently be assumed for OMAP due to how its power domain has been
implemented.

> >> Did you mean that I don't need to do a pm_runtime_get_sync() to
> >> enable my device in probe?
> >> Who is enabling by device for me then? Is device core supposed to
> >> do it?

Driver core calls the power domain activate() callback before probe, but
OMAP does not implement that callback. So you're right that you
currently need a pm_runtime_get_sync() in probe instead of setting
status active explicitly as the OMAP pm domain will not enable the bus
clock otherwise.

You still need to deal with the current driver explicit clock enable if
you want to change this, though.

Johan