usb: dwc3: gadget: skip pullup and set_speed after suspend

usb: dwc3: gadget: skip pullup and set_speed after suspend

[Daehwan Jung]

Sometimes dwc3_gadget_pullup and dwc3_gadget_set_speed are called after
entering suspend. That's why it needs to check whether suspend

1. dwc3 sends disconnect uevent and turn off. (suspend)
2. Platform side causes pullup or set_speed(e.g., adbd closes ffs node)
3. It causes unexpected behavior like ITMON error.

Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
---
 drivers/usb/dwc3/gadget.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index ee44321..d7d4202 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2093,6 +2093,9 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
 	unsigned long		flags;
 	int			ret;
 
+	if (pm_runtime_suspended(dwc->dev))
+		return 0;
+
 	is_on = !!is_on;
 
 	/*
@@ -2403,6 +2406,9 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
 	unsigned long		flags;
 	u32			reg;
 
+	if (pm_runtime_suspended(dwc->dev))
+		return;
+
 	spin_lock_irqsave(&dwc->lock, flags);
 	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
 	reg &= ~(DWC3_DCFG_SPEED_MASK);
-- 
2.7.4

Re: usb: dwc3: gadget: skip pullup and set_speed after suspend

[Thinh Nguyen]

Hi,

Daehwan Jung wrote:
> Sometimes dwc3_gadget_pullup and dwc3_gadget_set_speed are called after
> entering suspend. That's why it needs to check whether suspend
>
> 1. dwc3 sends disconnect uevent and turn off. (suspend)
> 2. Platform side causes pullup or set_speed(e.g., adbd closes ffs node)
> 3. It causes unexpected behavior like ITMON error.
>
> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> ---
>  drivers/usb/dwc3/gadget.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index ee44321..d7d4202 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2093,6 +2093,9 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>  	unsigned long		flags;
>  	int			ret;
>  
> +	if (pm_runtime_suspended(dwc->dev))
> +		return 0;
> +
>  	is_on = !!is_on;
>  
>  	/*
> @@ -2403,6 +2406,9 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
>  	unsigned long		flags;
>  	u32			reg;
>  
> +	if (pm_runtime_suspended(dwc->dev))
> +		return;
> +
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
>  	reg &= ~(DWC3_DCFG_SPEED_MASK);

This is already addressed in Wesley Cheng's patches. Can you try the
latest changes of DWC3 in Greg's usb-next branch?

Thanks,
Thinh

Re: usb: dwc3: gadget: skip pullup and set_speed after suspend

[Jung Daehwan]

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

Hi,

On Thu, Jan 21, 2021 at 01:00:32AM +0000, Thinh Nguyen wrote:
> Hi,
> 
> Daehwan Jung wrote:
> > Sometimes dwc3_gadget_pullup and dwc3_gadget_set_speed are called after
> > entering suspend. That's why it needs to check whether suspend
> >
> > 1. dwc3 sends disconnect uevent and turn off. (suspend)
> > 2. Platform side causes pullup or set_speed(e.g., adbd closes ffs node)
> > 3. It causes unexpected behavior like ITMON error.
> >
> > Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> > ---
> >  drivers/usb/dwc3/gadget.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index ee44321..d7d4202 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -2093,6 +2093,9 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> >  	unsigned long		flags;
> >  	int			ret;
> >  
> > +	if (pm_runtime_suspended(dwc->dev))
> > +		return 0;
> > +
> >  	is_on = !!is_on;
> >  
> >  	/*
> > @@ -2403,6 +2406,9 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
> >  	unsigned long		flags;
> >  	u32			reg;
> >  
> > +	if (pm_runtime_suspended(dwc->dev))
> > +		return;
> > +
> >  	spin_lock_irqsave(&dwc->lock, flags);
> >  	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
> >  	reg &= ~(DWC3_DCFG_SPEED_MASK);
> 
> This is already addressed in Wesley Cheng's patches. Can you try the
> latest changes of DWC3 in Greg's usb-next branch?
> 
> Thanks,
> Thinh

I checked Wesly Cheng's pathces but it's not same.
What I want to do for this patch is to avoid pullup from platform side.
(android in my case)

It's possible that platform side tries to pullup by UDC_Store after usb is already disconnected.
It can finally run controller and enable irq.

I think we have to avoid it and other possible things related to platform side.


[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: usb: dwc3: gadget: skip pullup and set_speed after suspend

[Wesley Cheng]

On 1/20/2021 10:49 PM, Jung Daehwan wrote:
> Hi,
> 
> On Thu, Jan 21, 2021 at 01:00:32AM +0000, Thinh Nguyen wrote:
>> Hi,
>>
>> Daehwan Jung wrote:
>>> Sometimes dwc3_gadget_pullup and dwc3_gadget_set_speed are called after
>>> entering suspend. That's why it needs to check whether suspend
>>>
>>> 1. dwc3 sends disconnect uevent and turn off. (suspend)
>>> 2. Platform side causes pullup or set_speed(e.g., adbd closes ffs node)
>>> 3. It causes unexpected behavior like ITMON error.
>>>
>>> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
>>> ---
>>>  drivers/usb/dwc3/gadget.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index ee44321..d7d4202 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -2093,6 +2093,9 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>>  	unsigned long		flags;
>>>  	int			ret;
>>>  
>>> +	if (pm_runtime_suspended(dwc->dev))
>>> +		return 0;
>>> +
>>>  	is_on = !!is_on;
>>>  
>>>  	/*
>>> @@ -2403,6 +2406,9 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
>>>  	unsigned long		flags;
>>>  	u32			reg;
>>>  
>>> +	if (pm_runtime_suspended(dwc->dev))
>>> +		return;
>>> +
>>>  	spin_lock_irqsave(&dwc->lock, flags);
>>>  	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
>>>  	reg &= ~(DWC3_DCFG_SPEED_MASK);
>>
>> This is already addressed in Wesley Cheng's patches. Can you try the
>> latest changes of DWC3 in Greg's usb-next branch?
>>
>> Thanks,
>> Thinh
> 
> I checked Wesly Cheng's pathces but it's not same.
> What I want to do for this patch is to avoid pullup from platform side.
> (android in my case)
> 
> It's possible that platform side tries to pullup by UDC_Store after usb is already disconnected.
> It can finally run controller and enable irq.
> 
> I think we have to avoid it and other possible things related to platform side.
> 
> 

Hi Daehwan,

I think what you're trying to do is to avoid the unnecessary runtime
resume if the cable is disconnected and userspace attempts to
bind/unbind the UDC.

I'm not exactly sure what patches you've pulled in, but assuming you
didn't pull in any of the recent suspend changes:

usb: dwc3: gadget: Allow runtime suspend if UDC unbinded
usb: dwc3: gadget: Preserve UDC max speed setting

Please consider the following scenario:
1.  USB connected
2.  UDC unbinded
3.  DWC3 will continue to stay in runtime active, since dwc->connected =
true

In this scenario, we should allow the DWC3 to enter runtime suspend,
since runstop has been disabled.

If you have pulled in the above changes, and adding your changes on top
of it, then consider the following:
1.  USB connected
2.  UDC unbinded
3.  DWC enters runtime suspend (due to the above changes)
4.  UDC binded

The check for pm_runtime_suspended() will block step#4 from re-enabling
the runstop bit even if the USB cable is connected.

Thanks
Wesley Cheng

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: usb: dwc3: gadget: skip pullup and set_speed after suspend

[Jung Daehwan]

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

On Wed, Jan 20, 2021 at 11:44:05PM -0800, Wesley Cheng wrote:
> 
> 
> On 1/20/2021 10:49 PM, Jung Daehwan wrote:
> > Hi,
> > 
> > On Thu, Jan 21, 2021 at 01:00:32AM +0000, Thinh Nguyen wrote:
> >> Hi,
> >>
> >> Daehwan Jung wrote:
> >>> Sometimes dwc3_gadget_pullup and dwc3_gadget_set_speed are called after
> >>> entering suspend. That's why it needs to check whether suspend
> >>>
> >>> 1. dwc3 sends disconnect uevent and turn off. (suspend)
> >>> 2. Platform side causes pullup or set_speed(e.g., adbd closes ffs node)
> >>> 3. It causes unexpected behavior like ITMON error.
> >>>
> >>> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> >>> ---
> >>>  drivers/usb/dwc3/gadget.c | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> >>> index ee44321..d7d4202 100644
> >>> --- a/drivers/usb/dwc3/gadget.c
> >>> +++ b/drivers/usb/dwc3/gadget.c
> >>> @@ -2093,6 +2093,9 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> >>>  	unsigned long		flags;
> >>>  	int			ret;
> >>>  
> >>> +	if (pm_runtime_suspended(dwc->dev))
> >>> +		return 0;
> >>> +
> >>>  	is_on = !!is_on;
> >>>  
> >>>  	/*
> >>> @@ -2403,6 +2406,9 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
> >>>  	unsigned long		flags;
> >>>  	u32			reg;
> >>>  
> >>> +	if (pm_runtime_suspended(dwc->dev))
> >>> +		return;
> >>> +
> >>>  	spin_lock_irqsave(&dwc->lock, flags);
> >>>  	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
> >>>  	reg &= ~(DWC3_DCFG_SPEED_MASK);
> >>
> >> This is already addressed in Wesley Cheng's patches. Can you try the
> >> latest changes of DWC3 in Greg's usb-next branch?
> >>
> >> Thanks,
> >> Thinh
> > 
> > I checked Wesly Cheng's pathces but it's not same.
> > What I want to do for this patch is to avoid pullup from platform side.
> > (android in my case)
> > 
> > It's possible that platform side tries to pullup by UDC_Store after usb is already disconnected.
> > It can finally run controller and enable irq.
> > 
> > I think we have to avoid it and other possible things related to platform side.
> > 
> > 
> 
> Hi Daehwan,
> 
> I think what you're trying to do is to avoid the unnecessary runtime
> resume if the cable is disconnected and userspace attempts to
> bind/unbind the UDC.
> 
> I'm not exactly sure what patches you've pulled in, but assuming you
> didn't pull in any of the recent suspend changes:
> 
> usb: dwc3: gadget: Allow runtime suspend if UDC unbinded
> usb: dwc3: gadget: Preserve UDC max speed setting
> 
> Please consider the following scenario:
> 1.  USB connected
> 2.  UDC unbinded
> 3.  DWC3 will continue to stay in runtime active, since dwc->connected =
> true
> 
> In this scenario, we should allow the DWC3 to enter runtime suspend,
> since runstop has been disabled.
> 
> If you have pulled in the above changes, and adding your changes on top
> of it, then consider the following:
> 1.  USB connected
> 2.  UDC unbinded
> 3.  DWC enters runtime suspend (due to the above changes)
> 4.  UDC binded
> 
> The check for pm_runtime_suspended() will block step#4 from re-enabling
> the runstop bit even if the USB cable is connected.
> 
> Thanks
> Wesley Cheng
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Hi Wesley

The check for runtime_suspended() will block re-enabling the runstop bit as
you said after pulling your patches in.

UDC is contolled by userspace and it's possible UDC can be binded
unexpectedly. That's why I think it needs to handle it even if the
problem is from userspace.

Below is an example in my environment.

1. USB disconnected
2. UDC unbinded
3. DWC enters runtime suspend
4. UDC binded unexpectedly
5. Gadget start and enable irq


Line 9823: [   36.024428][ T2889] dwc3 10e00000.dwc3: Turn off gadget dwc3-gadget
Line 9827: [   36.024572][ T2889] __dwc3_gadget_stop called
Line 9828: [   36.025083][ T2603] android_work: sent uevent USB_STATE=DISCONNECTED
Line 9842: [   36.200896][  T554] usb_gadget_disconnect
Line 9843: [   36.200916][  T554] dwc3_gadget_pullup : 0
Line 9844: [   36.201165][  T554] dwc3_gadget_pullup: get_sync fail
Line 9845: [   36.201197][  T554] dwc3_gadget_stop called
Line 9846: [   36.201250][  T451] android_work: did not send uevent (0 0 0000000000000000)
Line 9849: [   36.202343][    T1] init: processing action (sys.usb.config=adb && sys.usb.configfs=1..
Line 9851: [   36.203622][    T1] dwc3_gadget_start called
Line 9852: [   36.204079][    T1] usb_gadget_connect
Line 9853: [   36.204086][    T1] dwc3_gadget_pullup : 1
Line 9854: [   36.204091][    T1] __dwc3_gadget_start called

Best Regrars,
Jung Daehwan

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: usb: dwc3: gadget: skip pullup and set_speed after suspend

[Wesley Cheng]

On 1/21/2021 12:13 AM, Jung Daehwan wrote:
> On Wed, Jan 20, 2021 at 11:44:05PM -0800, Wesley Cheng wrote:
>>
>>
>> On 1/20/2021 10:49 PM, Jung Daehwan wrote:
>>> Hi,
>>>
>>> On Thu, Jan 21, 2021 at 01:00:32AM +0000, Thinh Nguyen wrote:
>>>> Hi,
>>>>
>>>> Daehwan Jung wrote:
>>>>> Sometimes dwc3_gadget_pullup and dwc3_gadget_set_speed are called after
>>>>> entering suspend. That's why it needs to check whether suspend
>>>>>
>>>>> 1. dwc3 sends disconnect uevent and turn off. (suspend)
>>>>> 2. Platform side causes pullup or set_speed(e.g., adbd closes ffs node)
>>>>> 3. It causes unexpected behavior like ITMON error.
>>>>>
>>>>> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
>>>>> ---
>>>>>  drivers/usb/dwc3/gadget.c | 6 ++++++
>>>>>  1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>> index ee44321..d7d4202 100644
>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>> @@ -2093,6 +2093,9 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>>>>  	unsigned long		flags;
>>>>>  	int			ret;
>>>>>  
>>>>> +	if (pm_runtime_suspended(dwc->dev))
>>>>> +		return 0;
>>>>> +
>>>>>  	is_on = !!is_on;
>>>>>  
>>>>>  	/*
>>>>> @@ -2403,6 +2406,9 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
>>>>>  	unsigned long		flags;
>>>>>  	u32			reg;
>>>>>  
>>>>> +	if (pm_runtime_suspended(dwc->dev))
>>>>> +		return;
>>>>> +
>>>>>  	spin_lock_irqsave(&dwc->lock, flags);
>>>>>  	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
>>>>>  	reg &= ~(DWC3_DCFG_SPEED_MASK);
>>>>
>>>> This is already addressed in Wesley Cheng's patches. Can you try the
>>>> latest changes of DWC3 in Greg's usb-next branch?
>>>>
>>>> Thanks,
>>>> Thinh
>>>
>>> I checked Wesly Cheng's pathces but it's not same.
>>> What I want to do for this patch is to avoid pullup from platform side.
>>> (android in my case)
>>>
>>> It's possible that platform side tries to pullup by UDC_Store after usb is already disconnected.
>>> It can finally run controller and enable irq.
>>>
>>> I think we have to avoid it and other possible things related to platform side.
>>>
>>>
>>
>> Hi Daehwan,
>>
>> I think what you're trying to do is to avoid the unnecessary runtime
>> resume if the cable is disconnected and userspace attempts to
>> bind/unbind the UDC.
>>
>> I'm not exactly sure what patches you've pulled in, but assuming you
>> didn't pull in any of the recent suspend changes:
>>
>> usb: dwc3: gadget: Allow runtime suspend if UDC unbinded
>> usb: dwc3: gadget: Preserve UDC max speed setting
>>
>> Please consider the following scenario:
>> 1.  USB connected
>> 2.  UDC unbinded
>> 3.  DWC3 will continue to stay in runtime active, since dwc->connected =
>> true
>>
>> In this scenario, we should allow the DWC3 to enter runtime suspend,
>> since runstop has been disabled.
>>
>> If you have pulled in the above changes, and adding your changes on top
>> of it, then consider the following:
>> 1.  USB connected
>> 2.  UDC unbinded
>> 3.  DWC enters runtime suspend (due to the above changes)
>> 4.  UDC binded
>>
>> The check for pm_runtime_suspended() will block step#4 from re-enabling
>> the runstop bit even if the USB cable is connected.
>>
>> Thanks
>> Wesley Cheng
>>
>> -- 
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
> 
> Hi Wesley
> 
> The check for runtime_suspended() will block re-enabling the runstop bit as
> you said after pulling your patches in.
> 
> UDC is contolled by userspace and it's possible UDC can be binded
> unexpectedly. That's why I think it needs to handle it even if the
> problem is from userspace.
> 
> Below is an example in my environment.
> 
> 1. USB disconnected
> 2. UDC unbinded
> 3. DWC enters runtime suspend
> 4. UDC binded unexpectedly
> 5. Gadget start and enable irq
> 
Hi Daehwan,

If this is an unexpected event where userspace initiates the UDC bind
sequence, then after the above sequence occurs, the DWC3 device should
still be able to re-enter runtime suspend after the autosuspend timer
expires.  Since the cable is disconnected, the dwc->connected flag would
still be false.  Is this not happening in your situation?

I'm just trying to understand what issue you're seeing other than the
momentary transition from runtime suspend (due to cable disconnect)
-->runtime resume (due to unexpected UDC bind) --> runtime  suspend (due
to nothing connected).

Thanks
Wesley Cheng

> 
> Line 9823: [   36.024428][ T2889] dwc3 10e00000.dwc3: Turn off gadget dwc3-gadget
> Line 9827: [   36.024572][ T2889] __dwc3_gadget_stop called
> Line 9828: [   36.025083][ T2603] android_work: sent uevent USB_STATE=DISCONNECTED
> Line 9842: [   36.200896][  T554] usb_gadget_disconnect
> Line 9843: [   36.200916][  T554] dwc3_gadget_pullup : 0
> Line 9844: [   36.201165][  T554] dwc3_gadget_pullup: get_sync fail
> Line 9845: [   36.201197][  T554] dwc3_gadget_stop called
> Line 9846: [   36.201250][  T451] android_work: did not send uevent (0 0 0000000000000000)
> Line 9849: [   36.202343][    T1] init: processing action (sys.usb.config=adb && sys.usb.configfs=1..
> Line 9851: [   36.203622][    T1] dwc3_gadget_start called
> Line 9852: [   36.204079][    T1] usb_gadget_connect
> Line 9853: [   36.204086][    T1] dwc3_gadget_pullup : 1
> Line 9854: [   36.204091][    T1] __dwc3_gadget_start called
> 
> Best Regrars,
> Jung Daehwan
> 
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: usb: dwc3: gadget: skip pullup and set_speed after suspend

[Jung Daehwan]

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

Hi,

On Thu, Jan 21, 2021 at 01:00:32AM +0000, Thinh Nguyen wrote:
> Hi,
> 
> Daehwan Jung wrote:
> > Sometimes dwc3_gadget_pullup and dwc3_gadget_set_speed are called after
> > entering suspend. That's why it needs to check whether suspend
> >
> > 1. dwc3 sends disconnect uevent and turn off. (suspend)
> > 2. Platform side causes pullup or set_speed(e.g., adbd closes ffs node)
> > 3. It causes unexpected behavior like ITMON error.
> >
> > Signed-off-by: Daehwan Jung <dh10.jung@samsung.com>
> > ---
> >  drivers/usb/dwc3/gadget.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index ee44321..d7d4202 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -2093,6 +2093,9 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> >  	unsigned long		flags;
> >  	int			ret;
> >  
> > +	if (pm_runtime_suspended(dwc->dev))
> > +		return 0;
> > +
> >  	is_on = !!is_on;
> >  
> >  	/*
> > @@ -2403,6 +2406,9 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
> >  	unsigned long		flags;
> >  	u32			reg;
> >  
> > +	if (pm_runtime_suspended(dwc->dev))
> > +		return;
> > +
> >  	spin_lock_irqsave(&dwc->lock, flags);
> >  	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
> >  	reg &= ~(DWC3_DCFG_SPEED_MASK);
> 
> This is already addressed in Wesley Cheng's patches. Can you try the
> latest changes of DWC3 in Greg's usb-next branch?
> 
> Thanks,
> Thinh

I checked Wesly Cheng's pathces but it's not same.
What I want to do for this patch is to avoid pullup from platform side.
(android in my case)

It's possible that platform side tries to pullup by UDC_Store after usb is already disconnected.
It can finally run controller and enable irq.

I think we have to avoid it and other possible things related to platform side.

BR,
Jung Daehwan

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: usb: dwc3: gadget: skip pullup and set_speed after suspend

[Felipe Balbi]

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


Hi,

Daehwan Jung <dh10.jung@samsung.com> writes:
> Sometimes dwc3_gadget_pullup and dwc3_gadget_set_speed are called after
> entering suspend. That's why it needs to check whether suspend
>
> 1. dwc3 sends disconnect uevent and turn off. (suspend)
> 2. Platform side causes pullup or set_speed(e.g., adbd closes ffs node)
> 3. It causes unexpected behavior like ITMON error.

please collect dwc3 trace events showing this problem.

-- 
balbi

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

Re: usb: dwc3: gadget: skip pullup and set_speed after suspend

[Jung Daehwan]

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

On Sun, Jan 24, 2021 at 05:31:57PM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> Daehwan Jung <dh10.jung@samsung.com> writes:
> > Sometimes dwc3_gadget_pullup and dwc3_gadget_set_speed are called after
> > entering suspend. That's why it needs to check whether suspend
> >
> > 1. dwc3 sends disconnect uevent and turn off. (suspend)
> > 2. Platform side causes pullup or set_speed(e.g., adbd closes ffs node)
> > 3. It causes unexpected behavior like ITMON error.
> 
> please collect dwc3 trace events showing this problem.
> 
> -- 
> balbi

Hi, balbi

I'm sorry for late reply. I pulled in belo patch and that issue didn't occur

usb: dwc3: gadget: Restart DWC3 gadget when enabling pullup

@ Wesley cheng
But, I think it needs to modify pm_rumtime_put to pm_runtime_put_sync_suspend
for syncronization. pm_rumtime_put calls rumtime_idle not runtime_suspend
Please check it.

usb: dwc3: gadget: Allow runtime suspend if UDC unbinded

Best Regards,
Jung Daehwan

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: usb: dwc3: gadget: skip pullup and set_speed after suspend

[Jung Daehwan]

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

On Fri, Jan 22, 2021 03:32, Wesley cheng wrote:
> Hi Daehwan,
> 
> If this is an unexpected event where userspace initiates the UDC bind
> sequence, then after the above sequence occurs, the DWC3 device should
> still be able to re-enter runtime suspend after the autosuspend timer
> expires.  Since the cable is disconnected, the dwc->connected flag would
> still be false.  Is this not happening in your situation?
> 
> I'm just trying to understand what issue you're seeing other than the
> momentary transition from runtime suspend (due to cable disconnect)
> -->runtime resume (due to unexpected UDC bind) --> runtime  suspend (due
> to nothing connected).
> 
> Thanks
> Wesley cheng

Hi Wesley,

I don't know why but DWC3 device is not re-entering runtime-suspend in
my situation. I'm still debugging it.
Even if DWC3 re-enter runtime-suspend but it doesn't mean stopping gadget.
Are you stopping gadget manually in this case?

Best Regards,
Jung Daehwan

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: usb: dwc3: gadget: skip pullup and set_speed after suspend

[Wesley Cheng]

On 1/21/2021 11:15 PM, Jung Daehwan wrote:
> On Fri, Jan 22, 2021 03:32, Wesley cheng wrote:
>> Hi Daehwan,
>>
>> If this is an unexpected event where userspace initiates the UDC bind
>> sequence, then after the above sequence occurs, the DWC3 device should
>> still be able to re-enter runtime suspend after the autosuspend timer
>> expires.  Since the cable is disconnected, the dwc->connected flag would
>> still be false.  Is this not happening in your situation?
>>
>> I'm just trying to understand what issue you're seeing other than the
>> momentary transition from runtime suspend (due to cable disconnect)
>> -->runtime resume (due to unexpected UDC bind) --> runtime  suspend (due
>> to nothing connected).
>>
>> Thanks
>> Wesley cheng
> 
> Hi Wesley,
> 
> I don't know why but DWC3 device is not re-entering runtime-suspend in
> my situation. I'm still debugging it.
> Even if DWC3 re-enter runtime-suspend but it doesn't mean stopping gadget.
> Are you stopping gadget manually in this case?

Hi Daehwan,

Sorry for the late response.  So during the DWC3 runtime suspend path,
we will execute dwc3_gadget_suspend() which should disable the gadget
events and disable ep0 then clear RS bit.  Then on runtime resume, the
DWC3 will be re-enabled, and the RS bit set again.

Thanks
Wesley Cheng

> 
> Best Regards,
> Jung Daehwan
> 
> 

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project