* usb: dwc3: gadget: Change runtime pm function for DWC3 runtime suspend
[not found] <CGME20210215025057epcas2p205c3c283a8806d818d71f90c872c6e51@epcas2p2.samsung.com>
@ 2021-02-15 2:38 ` Daehwan Jung
2021-02-15 7:54 ` Sergei Shtylyov
2021-02-15 17:41 ` Alan Stern
0 siblings, 2 replies; 6+ messages in thread
From: Daehwan Jung @ 2021-02-15 2:38 UTC (permalink / raw)
To: Felipe Balbi, Greg Kroah-Hartman, linux-usb, linux-kernel; +Cc: Daehwan Jung
It seems pm_runtime_put calls runtime_idle callback not runtime_suspend callback.
It's better to use pm_runtime_put_sync_suspend to allow DWC3 runtime suspend.
Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
---
drivers/usb/dwc3/gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index aebcf8e..4a4b93b 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2229,7 +2229,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
*/
ret = pm_runtime_get_sync(dwc->dev);
if (!ret || ret < 0) {
- pm_runtime_put(dwc->dev);
+ pm_runtime_put_sync_suspend(dwc->dev);
return 0;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: usb: dwc3: gadget: Change runtime pm function for DWC3 runtime suspend
2021-02-15 2:38 ` usb: dwc3: gadget: Change runtime pm function for DWC3 runtime suspend Daehwan Jung
@ 2021-02-15 7:54 ` Sergei Shtylyov
2021-02-15 17:41 ` Alan Stern
1 sibling, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2021-02-15 7:54 UTC (permalink / raw)
To: Daehwan Jung, Felipe Balbi, Greg Kroah-Hartman, linux-usb, linux-kernel
Hello!
On 15.02.2021 5:38, Daehwan Jung wrote:
> It seems pm_runtime_put calls runtime_idle callback not runtime_suspend callback.
> It's better to use pm_runtime_put_sync_suspend to allow DWC3 runtime suspend.
>
> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> ---
> drivers/usb/dwc3/gadget.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index aebcf8e..4a4b93b 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2229,7 +2229,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> */
> ret = pm_runtime_get_sync(dwc->dev);
> if (!ret || ret < 0) {
BTW, this can be shortened to (ret <= 0)...
> - pm_runtime_put(dwc->dev);
> + pm_runtime_put_sync_suspend(dwc->dev);
> return 0;
> }
>
MBR, Sergei
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: usb: dwc3: gadget: Change runtime pm function for DWC3 runtime suspend
2021-02-15 2:38 ` usb: dwc3: gadget: Change runtime pm function for DWC3 runtime suspend Daehwan Jung
2021-02-15 7:54 ` Sergei Shtylyov
@ 2021-02-15 17:41 ` Alan Stern
2021-02-16 1:30 ` Jung Daehwan
1 sibling, 1 reply; 6+ messages in thread
From: Alan Stern @ 2021-02-15 17:41 UTC (permalink / raw)
To: Daehwan Jung; +Cc: Felipe Balbi, Greg Kroah-Hartman, linux-usb, linux-kernel
On Mon, Feb 15, 2021 at 11:38:58AM +0900, Daehwan Jung wrote:
> It seems pm_runtime_put calls runtime_idle callback not runtime_suspend callback.
How is this fact related to your patch?
> It's better to use pm_runtime_put_sync_suspend to allow DWC3 runtime suspend.
Why do you think it is better? The advantage of pm_runtime_put is that
it allows the suspend to occur at a later time in a workqueue thread, so
the caller doesn't have to wait for the device to go into suspend.
Alan Stern
> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> ---
> drivers/usb/dwc3/gadget.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index aebcf8e..4a4b93b 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2229,7 +2229,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> */
> ret = pm_runtime_get_sync(dwc->dev);
> if (!ret || ret < 0) {
> - pm_runtime_put(dwc->dev);
> + pm_runtime_put_sync_suspend(dwc->dev);
> return 0;
> }
>
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: usb: dwc3: gadget: Change runtime pm function for DWC3 runtime suspend
2021-02-15 17:41 ` Alan Stern
@ 2021-02-16 1:30 ` Jung Daehwan
2021-02-16 15:31 ` Alan Stern
2021-02-17 0:07 ` Wesley Cheng
0 siblings, 2 replies; 6+ messages in thread
From: Jung Daehwan @ 2021-02-16 1:30 UTC (permalink / raw)
To: Alan Stern, Wesley Cheng
Cc: Felipe Balbi, Greg Kroah-Hartman,
open list:DESIGNWARE USB3 DRD IP DRIVER, open list
[-- Attachment #1: Type: text/plain, Size: 2161 bytes --]
Hello, Alan
On Mon, Feb 15, 2021 at 12:41:45PM -0500, Alan Stern wrote:
> On Mon, Feb 15, 2021 at 11:38:58AM +0900, Daehwan Jung wrote:
> > It seems pm_runtime_put calls runtime_idle callback not runtime_suspend callback.
>
> How is this fact related to your patch?
I think we should cause dwc3_runtime_suspend at the time.
That's why I use pm_runtime_put_sync_suspend.
>
> > It's better to use pm_runtime_put_sync_suspend to allow DWC3 runtime suspend.
>
> Why do you think it is better? The advantage of pm_runtime_put is that
> it allows the suspend to occur at a later time in a workqueue thread, so
> the caller doesn't have to wait for the device to go into suspend.
>
We can assume DWC3 was already in suspend state if pm_runtime_get_sync
returns 0. DWC3 resumes due to pm_rumtime_get_sync but it doesn't
re-enter runtime_suspend but runtime_idle. pm_runtime_put decreases
usage_count but doesn't cause runtime_suspend.
1. USB disconnected
2. UDC unbinded
3. DWC3 runtime suspend
4. UDC binded unexpectedly
5. DWC3 runtime resume (pm_runtime_get_sync)
6. DWC3 runtime idle (pm_runtime_put)
-> DWC3 runtime suspend again (pm_runtime_put_sync_suspend)
I've talked with Wesley in other patch.
usbb: dwc3: gadget: skip pullup and set_speed after suspend
patchwork.kernel.org/project/linux-usb/patch/1611113968-102424-1-git-send-email-dh10.jung@samsung.com
@ Wesley
I think We should guarantee DWC3 enters suspend again at the time.
How do you think?
Best Regards,
Jung Daehwan
> Alan Stern
>
> > Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> > ---
> > drivers/usb/dwc3/gadget.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index aebcf8e..4a4b93b 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -2229,7 +2229,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> > */
> > ret = pm_runtime_get_sync(dwc->dev);
> > if (!ret || ret < 0) {
> > - pm_runtime_put(dwc->dev);
> > + pm_runtime_put_sync_suspend(dwc->dev);
> > return 0;
> > }
> >
> > --
> > 2.7.4
> >
>
[-- Attachment #2: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: usb: dwc3: gadget: Change runtime pm function for DWC3 runtime suspend
2021-02-16 1:30 ` Jung Daehwan
@ 2021-02-16 15:31 ` Alan Stern
2021-02-17 0:07 ` Wesley Cheng
1 sibling, 0 replies; 6+ messages in thread
From: Alan Stern @ 2021-02-16 15:31 UTC (permalink / raw)
To: eg Kroah-Hartman
Cc: Wesley Cheng, Felipe Balbi,
open list:DESIGNWARE USB3 DRD IP DRIVER, open list
On Tue, Feb 16, 2021 at 10:30:52AM +0900, Jung Daehwan wrote:
> Hello, Alan
>
> On Mon, Feb 15, 2021 at 12:41:45PM -0500, Alan Stern wrote:
> > On Mon, Feb 15, 2021 at 11:38:58AM +0900, Daehwan Jung wrote:
> > > It seems pm_runtime_put calls runtime_idle callback not runtime_suspend callback.
> >
> > How is this fact related to your patch?
>
> I think we should cause dwc3_runtime_suspend at the time.
Why do you think so?
> That's why I use pm_runtime_put_sync_suspend.
>
> >
> > > It's better to use pm_runtime_put_sync_suspend to allow DWC3 runtime suspend.
> >
> > Why do you think it is better? The advantage of pm_runtime_put is that
> > it allows the suspend to occur at a later time in a workqueue thread, so
> > the caller doesn't have to wait for the device to go into suspend.
> >
>
> We can assume DWC3 was already in suspend state if pm_runtime_get_sync
> returns 0. DWC3 resumes due to pm_rumtime_get_sync but it doesn't
> re-enter runtime_suspend but runtime_idle. pm_runtime_put decreases
> usage_count but doesn't cause runtime_suspend.
>
> 1. USB disconnected
> 2. UDC unbinded
> 3. DWC3 runtime suspend
> 4. UDC binded unexpectedly
> 5. DWC3 runtime resume (pm_runtime_get_sync)
> 6. DWC3 runtime idle (pm_runtime_put)
> -> DWC3 runtime suspend again (pm_runtime_put_sync_suspend)
That's what happens with your patch. Now look at what happens without
the patch:
1. USB disconnected
2. UDC unbound
3. DWC3 suspend request is added to waitqueue
4. UDC bound unexpectedly
5. DWC3 suspend request is removed from waitqueue
6. DWC3 runtime idle
7. DWC3 runtime suspend
The difference is that this way, we avoid doing an unnecessary suspend
and resume, and we save the time they would have required.
> I've talked with Wesley in other patch.
>
> usbb: dwc3: gadget: skip pullup and set_speed after suspend
> patchwork.kernel.org/project/linux-usb/patch/1611113968-102424-1-git-send-email-dh10.jung@samsung.com
>
> @ Wesley
>
> I think We should guarantee DWC3 enters suspend again at the time.
Why do you think we should guarantee this?
Alan Stern
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: usb: dwc3: gadget: Change runtime pm function for DWC3 runtime suspend
2021-02-16 1:30 ` Jung Daehwan
2021-02-16 15:31 ` Alan Stern
@ 2021-02-17 0:07 ` Wesley Cheng
1 sibling, 0 replies; 6+ messages in thread
From: Wesley Cheng @ 2021-02-17 0:07 UTC (permalink / raw)
To: eg Kroah-Hartman, Alan Stern
Cc: Felipe Balbi, open list:DESIGNWARE USB3 DRD IP DRIVER, open list
On 2/15/2021 5:30 PM, Jung Daehwan wrote:
> Hello, Alan
>
> On Mon, Feb 15, 2021 at 12:41:45PM -0500, Alan Stern wrote:
>> On Mon, Feb 15, 2021 at 11:38:58AM +0900, Daehwan Jung wrote:
>>> It seems pm_runtime_put calls runtime_idle callback not runtime_suspend callback.
>>
>> How is this fact related to your patch?
>
> I think we should cause dwc3_runtime_suspend at the time.
> That's why I use pm_runtime_put_sync_suspend.
>
>>
>>> It's better to use pm_runtime_put_sync_suspend to allow DWC3 runtime suspend.
>>
>> Why do you think it is better? The advantage of pm_runtime_put is that
>> it allows the suspend to occur at a later time in a workqueue thread, so
>> the caller doesn't have to wait for the device to go into suspend.
>>
>
> We can assume DWC3 was already in suspend state if pm_runtime_get_sync
> returns 0. DWC3 resumes due to pm_rumtime_get_sync but it doesn't
> re-enter runtime_suspend but runtime_idle. pm_runtime_put decreases
> usage_count but doesn't cause runtime_suspend.
>
> 1. USB disconnected
> 2. UDC unbinded
> 3. DWC3 runtime suspend
> 4. UDC binded unexpectedly
> 5. DWC3 runtime resume (pm_runtime_get_sync)
> 6. DWC3 runtime idle (pm_runtime_put)
> -> DWC3 runtime suspend again (pm_runtime_put_sync_suspend)
>
> I've talked with Wesley in other patch.
>
> usbb: dwc3: gadget: skip pullup and set_speed after suspend
> patchwork.kernel.org/project/linux-usb/patch/1611113968-102424-1-git-send-email-dh10.jung@samsung.com
>
> @ Wesley
>
> I think We should guarantee DWC3 enters suspend again at the time.
> How do you think?
>
Hi Daehwan,
Even if we call runtime idle versus suspend, if the device is still in
the disconnected state, it should call the runtime PM suspend routine
after the autosuspend timer expires. As Alan mentioned already, this
allows not blocking the caller for the entire DWC3 suspend sequence to
execute. (DWC3 core will suspend other components as well, such as PHYs)
Also, for legitimate cases where pullup is actually called to start
enumeration from a suspended state, I'm not sure if the short duration
between RS set and re-suspend (due to your patch) is enough time for the
host to actually detect the device connected.
Thanks
Wesley Cheng
> Best Regards,
> Jung Daehwan
>
>> Alan Stern
>>
>
>
>>> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
>>> ---
>>> drivers/usb/dwc3/gadget.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index aebcf8e..4a4b93b 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -2229,7 +2229,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>> */
>>> ret = pm_runtime_get_sync(dwc->dev);
>>> if (!ret || ret < 0) {
>>> - pm_runtime_put(dwc->dev);
>>> + pm_runtime_put_sync_suspend(dwc->dev);
>>> return 0;
>>> }
>>>
>>> --
>>> 2.7.4
>>>
>>
>>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-17 0:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20210215025057epcas2p205c3c283a8806d818d71f90c872c6e51@epcas2p2.samsung.com>
2021-02-15 2:38 ` usb: dwc3: gadget: Change runtime pm function for DWC3 runtime suspend Daehwan Jung
2021-02-15 7:54 ` Sergei Shtylyov
2021-02-15 17:41 ` Alan Stern
2021-02-16 1:30 ` Jung Daehwan
2021-02-16 15:31 ` Alan Stern
2021-02-17 0:07 ` Wesley Cheng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).