[PATCH v2 0/2] Handle core soft reset failure in pullup

[PATCH v2 0/2] Handle core soft reset failure in pullup

[Krishna Kurapati]

When core soft reset timeout happens, pullup doesn't check for the
return value and proceeds setting up of event buffers and starts the
controller.

In this scneario, it is observed sometimes that the GEVTADDR LO/HI
registers read zero while we are setting the run stop bit and we end
up accessing address 0x00 leading to a crash. This series tries to
address this issue by handling the timeout and return back appropriate
error code to configfs for it to retry enumeration if it chooses to.

Link to v1: https://lore.kernel.org/all/20230322092740.28491-1-quic_kriskura@quicinc.com/

changes in v2:
Fixed comments addressing incomplete error handling in udc core

Krishna Kurapati (2):
  usb: dwc3: gadget: Bail out in pullup if soft reset timeout happens
  usb: gadget: udc: Handle gadget_connect failure during bind operation

 drivers/usb/dwc3/gadget.c     |  5 ++++-
 drivers/usb/gadget/udc/core.c | 20 ++++++++++++++++----
 2 files changed, 20 insertions(+), 5 deletions(-)

-- 
2.40.0

[PATCH v2 1/2] usb: dwc3: gadget: Bail out in pullup if soft reset timeout happens

[Krishna Kurapati]

If the core soft reset timeout happens, avoid setting up event
buffers and starting gadget as the writes to these registers
may not reflect when in reset and setting the run stop bit
can lead the controller to access wrong event buffer address
resulting in a crash.

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 drivers/usb/dwc3/gadget.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 3c63fa97a680..f0472801d9a5 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2620,13 +2620,16 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
 		 * device-initiated disconnect requires a core soft reset
 		 * (DCTL.CSftRst) before enabling the run/stop bit.
 		 */
-		dwc3_core_soft_reset(dwc);
+		ret = dwc3_core_soft_reset(dwc);
+		if (ret)
+			goto done;
 
 		dwc3_event_buffers_setup(dwc);
 		__dwc3_gadget_start(dwc);
 		ret = dwc3_gadget_run_stop(dwc, true, false);
 	}
 
+done:
 	pm_runtime_put(dwc->dev);
 
 	return ret;
-- 
2.40.0

Re: [PATCH v2 1/2] usb: dwc3: gadget: Bail out in pullup if soft reset timeout happens

[Thinh Nguyen]

Hi,

On Tue, Mar 28, 2023, Krishna Kurapati wrote:
> If the core soft reset timeout happens, avoid setting up event
> buffers and starting gadget as the writes to these registers
> may not reflect when in reset and setting the run stop bit
> can lead the controller to access wrong event buffer address
> resulting in a crash.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  drivers/usb/dwc3/gadget.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3c63fa97a680..f0472801d9a5 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2620,13 +2620,16 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>  		 * device-initiated disconnect requires a core soft reset
>  		 * (DCTL.CSftRst) before enabling the run/stop bit.
>  		 */
> -		dwc3_core_soft_reset(dwc);
> +		ret = dwc3_core_soft_reset(dwc);
> +		if (ret)
> +			goto done;
>  
>  		dwc3_event_buffers_setup(dwc);
>  		__dwc3_gadget_start(dwc);
>  		ret = dwc3_gadget_run_stop(dwc, true, false);
>  	}
>  
> +done:
>  	pm_runtime_put(dwc->dev);
>  
>  	return ret;
> -- 
> 2.40.0
> 

I think there's one more place that may needs this check. Can you also
add this check in __dwc3_set_mode()?

Thanks,
Thinh

Re: [PATCH v2 1/2] usb: dwc3: gadget: Bail out in pullup if soft reset timeout happens

[Krishna Kurapati PSSNV]

On 3/29/2023 2:50 AM, Thinh Nguyen wrote:
> Hi,
> 
> On Tue, Mar 28, 2023, Krishna Kurapati wrote:
>> If the core soft reset timeout happens, avoid setting up event
>> buffers and starting gadget as the writes to these registers
>> may not reflect when in reset and setting the run stop bit
>> can lead the controller to access wrong event buffer address
>> resulting in a crash.
>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>>   drivers/usb/dwc3/gadget.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 3c63fa97a680..f0472801d9a5 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2620,13 +2620,16 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>   		 * device-initiated disconnect requires a core soft reset
>>   		 * (DCTL.CSftRst) before enabling the run/stop bit.
>>   		 */
>> -		dwc3_core_soft_reset(dwc);
>> +		ret = dwc3_core_soft_reset(dwc);
>> +		if (ret)
>> +			goto done;
>>   
>>   		dwc3_event_buffers_setup(dwc);
>>   		__dwc3_gadget_start(dwc);
>>   		ret = dwc3_gadget_run_stop(dwc, true, false);
>>   	}
>>   
>> +done:
>>   	pm_runtime_put(dwc->dev);
>>   
>>   	return ret;
>> -- 
>> 2.40.0
>>
> 
> I think there's one more place that may needs this check. Can you also
> add this check in __dwc3_set_mode()?

Hi Thinh,

   Sure. Will do it.
Will the below be good enough ? Or would it be good to add an error/warn 
log there>

kriskura@hu-kriskura-hyd:/local/mnt/workspace/krishna/skales2/skales/kernel$ 
git diff drivers/usb/
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 476b63618511..8d1d213d1dcd 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -210,7 +210,9 @@ static void __dwc3_set_mode(struct work_struct *work)
                 }
                 break;
         case DWC3_GCTL_PRTCAP_DEVICE:
-               dwc3_core_soft_reset(dwc);
+               ret = dwc3_core_soft_reset(dwc);
+               if (ret)
+                       goto out;

                 dwc3_event_buffers_setup(dwc);

Regards,
Krishna,

Re: [PATCH v2 1/2] usb: dwc3: gadget: Bail out in pullup if soft reset timeout happens

[Thinh Nguyen]

On Wed, Mar 29, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 3/29/2023 2:50 AM, Thinh Nguyen wrote:
> > Hi,
> > 
> > On Tue, Mar 28, 2023, Krishna Kurapati wrote:
> > > If the core soft reset timeout happens, avoid setting up event
> > > buffers and starting gadget as the writes to these registers
> > > may not reflect when in reset and setting the run stop bit
> > > can lead the controller to access wrong event buffer address
> > > resulting in a crash.
> > > 
> > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > ---
> > >   drivers/usb/dwc3/gadget.c | 5 ++++-
> > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > > index 3c63fa97a680..f0472801d9a5 100644
> > > --- a/drivers/usb/dwc3/gadget.c
> > > +++ b/drivers/usb/dwc3/gadget.c
> > > @@ -2620,13 +2620,16 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> > >   		 * device-initiated disconnect requires a core soft reset
> > >   		 * (DCTL.CSftRst) before enabling the run/stop bit.
> > >   		 */
> > > -		dwc3_core_soft_reset(dwc);
> > > +		ret = dwc3_core_soft_reset(dwc);
> > > +		if (ret)
> > > +			goto done;
> > >   		dwc3_event_buffers_setup(dwc);
> > >   		__dwc3_gadget_start(dwc);
> > >   		ret = dwc3_gadget_run_stop(dwc, true, false);
> > >   	}
> > > +done:
> > >   	pm_runtime_put(dwc->dev);
> > >   	return ret;
> > > -- 
> > > 2.40.0
> > > 
> > 
> > I think there's one more place that may needs this check. Can you also
> > add this check in __dwc3_set_mode()?
> 
> Hi Thinh,
> 
>   Sure. Will do it.
> Will the below be good enough ? Or would it be good to add an error/warn log
> there>

There's already a warning message in dwc3_core_soft_reset() if it fails.

> 
> kriskura@hu-kriskura-hyd:/local/mnt/workspace/krishna/skales2/skales/kernel$
> git diff drivers/usb/
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 476b63618511..8d1d213d1dcd 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -210,7 +210,9 @@ static void __dwc3_set_mode(struct work_struct *work)
>                 }
>                 break;
>         case DWC3_GCTL_PRTCAP_DEVICE:
> -               dwc3_core_soft_reset(dwc);
> +               ret = dwc3_core_soft_reset(dwc);
> +               if (ret)
> +                       goto out;
> 
>                 dwc3_event_buffers_setup(dwc);
> 

If soft-reset failed, the controller is in a bad state. We should not
perform any further operation until the next hard reset. We should flag
the controller as dead. I don't think we have the equivalent of the
host's HCD_FLAG_DEAD. It may require some work in the UDC core. Perhaps
we can flag within dwc3 for now and prevent any further operation for a
simpler fix.

Thanks,
Thinh

Re: [PATCH v2 1/2] usb: dwc3: gadget: Bail out in pullup if soft reset timeout happens

[Krishna Kurapati PSSNV]

On 3/30/2023 5:40 AM, Thinh Nguyen wrote:
> On Wed, Mar 29, 2023, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 3/29/2023 2:50 AM, Thinh Nguyen wrote:
>>> Hi,
>>>
>>> On Tue, Mar 28, 2023, Krishna Kurapati wrote:
>>>> If the core soft reset timeout happens, avoid setting up event
>>>> buffers and starting gadget as the writes to these registers
>>>> may not reflect when in reset and setting the run stop bit
>>>> can lead the controller to access wrong event buffer address
>>>> resulting in a crash.
>>>>
>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>> ---
>>>>    drivers/usb/dwc3/gadget.c | 5 ++++-
>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>> index 3c63fa97a680..f0472801d9a5 100644
>>>> --- a/drivers/usb/dwc3/gadget.c
>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>> @@ -2620,13 +2620,16 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>>>    		 * device-initiated disconnect requires a core soft reset
>>>>    		 * (DCTL.CSftRst) before enabling the run/stop bit.
>>>>    		 */
>>>> -		dwc3_core_soft_reset(dwc);
>>>> +		ret = dwc3_core_soft_reset(dwc);
>>>> +		if (ret)
>>>> +			goto done;
>>>>    		dwc3_event_buffers_setup(dwc);
>>>>    		__dwc3_gadget_start(dwc);
>>>>    		ret = dwc3_gadget_run_stop(dwc, true, false);
>>>>    	}
>>>> +done:
>>>>    	pm_runtime_put(dwc->dev);
>>>>    	return ret;
>>>> -- 
>>>> 2.40.0
>>>>
>>>
>>> I think there's one more place that may needs this check. Can you also
>>> add this check in __dwc3_set_mode()?
>>
>> Hi Thinh,
>>
>>    Sure. Will do it.
>> Will the below be good enough ? Or would it be good to add an error/warn log
>> there>
> 
> There's already a warning message in dwc3_core_soft_reset() if it fails.
> 
>>
>> kriskura@hu-kriskura-hyd:/local/mnt/workspace/krishna/skales2/skales/kernel$
>> git diff drivers/usb/
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 476b63618511..8d1d213d1dcd 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -210,7 +210,9 @@ static void __dwc3_set_mode(struct work_struct *work)
>>                  }
>>                  break;
>>          case DWC3_GCTL_PRTCAP_DEVICE:
>> -               dwc3_core_soft_reset(dwc);
>> +               ret = dwc3_core_soft_reset(dwc);
>> +               if (ret)
>> +                       goto out;
>>
>>                  dwc3_event_buffers_setup(dwc);
>>
> 
> If soft-reset failed, the controller is in a bad state. We should not
> perform any further operation until the next hard reset. We should flag
> the controller as dead. I don't think we have the equivalent of the
> host's HCD_FLAG_DEAD. It may require some work in the UDC core. Perhaps
> we can flag within dwc3 for now and prevent any further operation for a
> simpler fix.
> 
Hi Thinh,

  Are you referring that if __dwc3_set_mode failed with core soft reset 
timing out, the caller i.e., dwc3_set_mode who queues the work need to 
know that the operation actually failed. So we can add a flag to 
indicate that gadget is dead and the caller of dwc3_set_mode can check 
the flag to see if the operation is successful or not.

Or am I misunderstanding your comment ?

Regards,
Krishna,

Re: [PATCH v2 1/2] usb: dwc3: gadget: Bail out in pullup if soft reset timeout happens

[Thinh Nguyen]

On Thu, Mar 30, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 3/30/2023 5:40 AM, Thinh Nguyen wrote:
> > On Wed, Mar 29, 2023, Krishna Kurapati PSSNV wrote:
> > > 
> > > 
> > > On 3/29/2023 2:50 AM, Thinh Nguyen wrote:
> > > > Hi,
> > > > 
> > > > On Tue, Mar 28, 2023, Krishna Kurapati wrote:
> > > > > If the core soft reset timeout happens, avoid setting up event
> > > > > buffers and starting gadget as the writes to these registers
> > > > > may not reflect when in reset and setting the run stop bit
> > > > > can lead the controller to access wrong event buffer address
> > > > > resulting in a crash.
> > > > > 
> > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > > ---
> > > > >    drivers/usb/dwc3/gadget.c | 5 ++++-
> > > > >    1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > > > > index 3c63fa97a680..f0472801d9a5 100644
> > > > > --- a/drivers/usb/dwc3/gadget.c
> > > > > +++ b/drivers/usb/dwc3/gadget.c
> > > > > @@ -2620,13 +2620,16 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> > > > >    		 * device-initiated disconnect requires a core soft reset
> > > > >    		 * (DCTL.CSftRst) before enabling the run/stop bit.
> > > > >    		 */
> > > > > -		dwc3_core_soft_reset(dwc);
> > > > > +		ret = dwc3_core_soft_reset(dwc);
> > > > > +		if (ret)
> > > > > +			goto done;
> > > > >    		dwc3_event_buffers_setup(dwc);
> > > > >    		__dwc3_gadget_start(dwc);
> > > > >    		ret = dwc3_gadget_run_stop(dwc, true, false);
> > > > >    	}
> > > > > +done:
> > > > >    	pm_runtime_put(dwc->dev);
> > > > >    	return ret;
> > > > > -- 
> > > > > 2.40.0
> > > > > 
> > > > 
> > > > I think there's one more place that may needs this check. Can you also
> > > > add this check in __dwc3_set_mode()?
> > > 
> > > Hi Thinh,
> > > 
> > >    Sure. Will do it.
> > > Will the below be good enough ? Or would it be good to add an error/warn log
> > > there>
> > 
> > There's already a warning message in dwc3_core_soft_reset() if it fails.
> > 
> > > 
> > > kriskura@hu-kriskura-hyd:/local/mnt/workspace/krishna/skales2/skales/kernel$
> > > git diff drivers/usb/
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index 476b63618511..8d1d213d1dcd 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -210,7 +210,9 @@ static void __dwc3_set_mode(struct work_struct *work)
> > >                  }
> > >                  break;
> > >          case DWC3_GCTL_PRTCAP_DEVICE:
> > > -               dwc3_core_soft_reset(dwc);
> > > +               ret = dwc3_core_soft_reset(dwc);
> > > +               if (ret)
> > > +                       goto out;
> > > 
> > >                  dwc3_event_buffers_setup(dwc);
> > > 
> > 
> > If soft-reset failed, the controller is in a bad state. We should not
> > perform any further operation until the next hard reset. We should flag
> > the controller as dead. I don't think we have the equivalent of the
> > host's HCD_FLAG_DEAD. It may require some work in the UDC core. Perhaps
> > we can flag within dwc3 for now and prevent any further operation for a
> > simpler fix.
> > 
> Hi Thinh,
> 
>  Are you referring that if __dwc3_set_mode failed with core soft reset
> timing out, the caller i.e., dwc3_set_mode who queues the work need to know
> that the operation actually failed. So we can add a flag to indicate that
> gadget is dead and the caller of dwc3_set_mode can check the flag to see if
> the operation is successful or not.
> 
> Or am I misunderstanding your comment ?
> 

Not just in __dwc3_set_mode(). I mean any time dwc3_core_soft_reset
fails, then we set this flag. So that it can prevent the user calling
any gadget ops causing more crashes/invalid behavior. The
dwc->softconnect is already wrong on pullup() on failure.

So that we can have a check in different gadget ops. For pullup():

static int dwc3_gadget_pullup() {
	if (dwc->udc_is_dead) {
		dev_err(dev, "reset me. x_x \n");
		return;
	}

	abc();
}

Perhaps the effort is probably the same if we enhance the UDC core for
this? In any case, I'm fine either way.

Thanks,
Thinh

Re: [PATCH v2 1/2] usb: dwc3: gadget: Bail out in pullup if soft reset timeout happens

[Krishna Kurapati PSSNV]

On 4/4/2023 5:19 AM, Thinh Nguyen wrote:
> On Thu, Mar 30, 2023, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 3/30/2023 5:40 AM, Thinh Nguyen wrote:
>>> On Wed, Mar 29, 2023, Krishna Kurapati PSSNV wrote:
>>>>
>>>>
>>>> On 3/29/2023 2:50 AM, Thinh Nguyen wrote:
>>>>> Hi,
>>>>>
>>>>> On Tue, Mar 28, 2023, Krishna Kurapati wrote:
>>>>>> If the core soft reset timeout happens, avoid setting up event
>>>>>> buffers and starting gadget as the writes to these registers
>>>>>> may not reflect when in reset and setting the run stop bit
>>>>>> can lead the controller to access wrong event buffer address
>>>>>> resulting in a crash.
>>>>>>
>>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>>>> ---
>>>>>>     drivers/usb/dwc3/gadget.c | 5 ++++-
>>>>>>     1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>>> index 3c63fa97a680..f0472801d9a5 100644
>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>> @@ -2620,13 +2620,16 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>>>>>     		 * device-initiated disconnect requires a core soft reset
>>>>>>     		 * (DCTL.CSftRst) before enabling the run/stop bit.
>>>>>>     		 */
>>>>>> -		dwc3_core_soft_reset(dwc);
>>>>>> +		ret = dwc3_core_soft_reset(dwc);
>>>>>> +		if (ret)
>>>>>> +			goto done;
>>>>>>     		dwc3_event_buffers_setup(dwc);
>>>>>>     		__dwc3_gadget_start(dwc);
>>>>>>     		ret = dwc3_gadget_run_stop(dwc, true, false);
>>>>>>     	}
>>>>>> +done:
>>>>>>     	pm_runtime_put(dwc->dev);
>>>>>>     	return ret;
>>>>>> -- 
>>>>>> 2.40.0
>>>>>>
>>>>>
>>>>> I think there's one more place that may needs this check. Can you also
>>>>> add this check in __dwc3_set_mode()?
>>>>
>>>> Hi Thinh,
>>>>
>>>>     Sure. Will do it.
>>>> Will the below be good enough ? Or would it be good to add an error/warn log
>>>> there>
>>>
>>> There's already a warning message in dwc3_core_soft_reset() if it fails.
>>>
>>>>
>>>> kriskura@hu-kriskura-hyd:/local/mnt/workspace/krishna/skales2/skales/kernel$
>>>> git diff drivers/usb/
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 476b63618511..8d1d213d1dcd 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -210,7 +210,9 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>>                   }
>>>>                   break;
>>>>           case DWC3_GCTL_PRTCAP_DEVICE:
>>>> -               dwc3_core_soft_reset(dwc);
>>>> +               ret = dwc3_core_soft_reset(dwc);
>>>> +               if (ret)
>>>> +                       goto out;
>>>>
>>>>                   dwc3_event_buffers_setup(dwc);
>>>>
>>>
>>> If soft-reset failed, the controller is in a bad state. We should not
>>> perform any further operation until the next hard reset. We should flag
>>> the controller as dead. I don't think we have the equivalent of the
>>> host's HCD_FLAG_DEAD. It may require some work in the UDC core. Perhaps
>>> we can flag within dwc3 for now and prevent any further operation for a
>>> simpler fix.
>>>
>> Hi Thinh,
>>
>>   Are you referring that if __dwc3_set_mode failed with core soft reset
>> timing out, the caller i.e., dwc3_set_mode who queues the work need to know
>> that the operation actually failed. So we can add a flag to indicate that
>> gadget is dead and the caller of dwc3_set_mode can check the flag to see if
>> the operation is successful or not.
>>
>> Or am I misunderstanding your comment ?
>>
> 
> Not just in __dwc3_set_mode(). I mean any time dwc3_core_soft_reset
> fails, then we set this flag. So that it can prevent the user calling
> any gadget ops causing more crashes/invalid behavior. The
> dwc->softconnect is already wrong on pullup() on failure.
> 
> So that we can have a check in different gadget ops. For pullup():
> 
> static int dwc3_gadget_pullup() {
> 	if (dwc->udc_is_dead) {
> 		dev_err(dev, "reset me. x_x \n");
> 		return;
> 	}
> 
> 	abc();
> }
> 
> Perhaps the effort is probably the same if we enhance the UDC core for
> this? In any case, I'm fine either way.
> 
> Thanks,
> Thinh

Hi Thinh,

  So you don't want UDC to retry pullup if it fails the first time ? As 
per patch-2 of this series, I was trying to propagate the EITMEDOUT to 
UDC so that the caller (most probably configfs) can take appropriate 
action as to whether it must retry pullup or not.

Regards,
Krishna,

Re: [PATCH v2 1/2] usb: dwc3: gadget: Bail out in pullup if soft reset timeout happens

[Thinh Nguyen]

On Tue, Apr 04, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 4/4/2023 5:19 AM, Thinh Nguyen wrote:
> > On Thu, Mar 30, 2023, Krishna Kurapati PSSNV wrote:
> > > 
> > > 
> > > On 3/30/2023 5:40 AM, Thinh Nguyen wrote:
> > > > On Wed, Mar 29, 2023, Krishna Kurapati PSSNV wrote:
> > > > > 
> > > > > 
> > > > > On 3/29/2023 2:50 AM, Thinh Nguyen wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Tue, Mar 28, 2023, Krishna Kurapati wrote:
> > > > > > > If the core soft reset timeout happens, avoid setting up event
> > > > > > > buffers and starting gadget as the writes to these registers
> > > > > > > may not reflect when in reset and setting the run stop bit
> > > > > > > can lead the controller to access wrong event buffer address
> > > > > > > resulting in a crash.
> > > > > > > 
> > > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > > > > ---
> > > > > > >     drivers/usb/dwc3/gadget.c | 5 ++++-
> > > > > > >     1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > > > > > > index 3c63fa97a680..f0472801d9a5 100644
> > > > > > > --- a/drivers/usb/dwc3/gadget.c
> > > > > > > +++ b/drivers/usb/dwc3/gadget.c
> > > > > > > @@ -2620,13 +2620,16 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> > > > > > >     		 * device-initiated disconnect requires a core soft reset
> > > > > > >     		 * (DCTL.CSftRst) before enabling the run/stop bit.
> > > > > > >     		 */
> > > > > > > -		dwc3_core_soft_reset(dwc);
> > > > > > > +		ret = dwc3_core_soft_reset(dwc);
> > > > > > > +		if (ret)
> > > > > > > +			goto done;
> > > > > > >     		dwc3_event_buffers_setup(dwc);
> > > > > > >     		__dwc3_gadget_start(dwc);
> > > > > > >     		ret = dwc3_gadget_run_stop(dwc, true, false);
> > > > > > >     	}
> > > > > > > +done:
> > > > > > >     	pm_runtime_put(dwc->dev);
> > > > > > >     	return ret;
> > > > > > > -- 
> > > > > > > 2.40.0
> > > > > > > 
> > > > > > 
> > > > > > I think there's one more place that may needs this check. Can you also
> > > > > > add this check in __dwc3_set_mode()?
> > > > > 
> > > > > Hi Thinh,
> > > > > 
> > > > >     Sure. Will do it.
> > > > > Will the below be good enough ? Or would it be good to add an error/warn log
> > > > > there>
> > > > 
> > > > There's already a warning message in dwc3_core_soft_reset() if it fails.
> > > > 
> > > > > 
> > > > > kriskura@hu-kriskura-hyd:/local/mnt/workspace/krishna/skales2/skales/kernel$
> > > > > git diff drivers/usb/
> > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > > index 476b63618511..8d1d213d1dcd 100644
> > > > > --- a/drivers/usb/dwc3/core.c
> > > > > +++ b/drivers/usb/dwc3/core.c
> > > > > @@ -210,7 +210,9 @@ static void __dwc3_set_mode(struct work_struct *work)
> > > > >                   }
> > > > >                   break;
> > > > >           case DWC3_GCTL_PRTCAP_DEVICE:
> > > > > -               dwc3_core_soft_reset(dwc);
> > > > > +               ret = dwc3_core_soft_reset(dwc);
> > > > > +               if (ret)
> > > > > +                       goto out;
> > > > > 
> > > > >                   dwc3_event_buffers_setup(dwc);
> > > > > 
> > > > 
> > > > If soft-reset failed, the controller is in a bad state. We should not
> > > > perform any further operation until the next hard reset. We should flag
> > > > the controller as dead. I don't think we have the equivalent of the
> > > > host's HCD_FLAG_DEAD. It may require some work in the UDC core. Perhaps
> > > > we can flag within dwc3 for now and prevent any further operation for a
> > > > simpler fix.
> > > > 
> > > Hi Thinh,
> > > 
> > >   Are you referring that if __dwc3_set_mode failed with core soft reset
> > > timing out, the caller i.e., dwc3_set_mode who queues the work need to know
> > > that the operation actually failed. So we can add a flag to indicate that
> > > gadget is dead and the caller of dwc3_set_mode can check the flag to see if
> > > the operation is successful or not.
> > > 
> > > Or am I misunderstanding your comment ?
> > > 
> > 
> > Not just in __dwc3_set_mode(). I mean any time dwc3_core_soft_reset
> > fails, then we set this flag. So that it can prevent the user calling
> > any gadget ops causing more crashes/invalid behavior. The
> > dwc->softconnect is already wrong on pullup() on failure.
> > 
> > So that we can have a check in different gadget ops. For pullup():
> > 
> > static int dwc3_gadget_pullup() {
> > 	if (dwc->udc_is_dead) {
> > 		dev_err(dev, "reset me. x_x \n");
> > 		return;
> > 	}
> > 
> > 	abc();
> > }
> > 
> > Perhaps the effort is probably the same if we enhance the UDC core for
> > this? In any case, I'm fine either way.
> > 
> > Thanks,
> > Thinh
> 
> Hi Thinh,
> 
>  So you don't want UDC to retry pullup if it fails the first time ? As per
> patch-2 of this series, I was trying to propagate the EITMEDOUT to UDC so
> that the caller (most probably configfs) can take appropriate action as to
> whether it must retry pullup or not.
> 

Now I'm confused. If the soft-reset times out, that means that the
soft-reset (self-clearing) bit isn't cleared. How can we retry if it's
stuck in this state? My impression is that soft-reset would not complete
at all. Is that not the case for you, or it's simply because we need a
longer soft-reset timeout?

Thanks,
Thinh

Re: [PATCH v2 1/2] usb: dwc3: gadget: Bail out in pullup if soft reset timeout happens

[Krishna Kurapati PSSNV]

On 4/5/2023 3:13 AM, Thinh Nguyen wrote:
> On Tue, Apr 04, 2023, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 4/4/2023 5:19 AM, Thinh Nguyen wrote:
>>> On Thu, Mar 30, 2023, Krishna Kurapati PSSNV wrote:
>>>>
>>>>
>>>> On 3/30/2023 5:40 AM, Thinh Nguyen wrote:
>>>>> On Wed, Mar 29, 2023, Krishna Kurapati PSSNV wrote:
>>>>>>
>>>>>>
>>>>>> On 3/29/2023 2:50 AM, Thinh Nguyen wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On Tue, Mar 28, 2023, Krishna Kurapati wrote:
>>>>>>>> If the core soft reset timeout happens, avoid setting up event
>>>>>>>> buffers and starting gadget as the writes to these registers
>>>>>>>> may not reflect when in reset and setting the run stop bit
>>>>>>>> can lead the controller to access wrong event buffer address
>>>>>>>> resulting in a crash.
>>>>>>>>
>>>>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>>>>>> ---
>>>>>>>>      drivers/usb/dwc3/gadget.c | 5 ++++-
>>>>>>>>      1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>>>>> index 3c63fa97a680..f0472801d9a5 100644
>>>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>>>> @@ -2620,13 +2620,16 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>>>>>>>      		 * device-initiated disconnect requires a core soft reset
>>>>>>>>      		 * (DCTL.CSftRst) before enabling the run/stop bit.
>>>>>>>>      		 */
>>>>>>>> -		dwc3_core_soft_reset(dwc);
>>>>>>>> +		ret = dwc3_core_soft_reset(dwc);
>>>>>>>> +		if (ret)
>>>>>>>> +			goto done;
>>>>>>>>      		dwc3_event_buffers_setup(dwc);
>>>>>>>>      		__dwc3_gadget_start(dwc);
>>>>>>>>      		ret = dwc3_gadget_run_stop(dwc, true, false);
>>>>>>>>      	}
>>>>>>>> +done:
>>>>>>>>      	pm_runtime_put(dwc->dev);
>>>>>>>>      	return ret;
>>>>>>>> -- 
>>>>>>>> 2.40.0
>>>>>>>>
>>>>>>>
>>>>>>> I think there's one more place that may needs this check. Can you also
>>>>>>> add this check in __dwc3_set_mode()?
>>>>>>
>>>>>> Hi Thinh,
>>>>>>
>>>>>>      Sure. Will do it.
>>>>>> Will the below be good enough ? Or would it be good to add an error/warn log
>>>>>> there>
>>>>>
>>>>> There's already a warning message in dwc3_core_soft_reset() if it fails.
>>>>>
>>>>>>
>>>>>> kriskura@hu-kriskura-hyd:/local/mnt/workspace/krishna/skales2/skales/kernel$
>>>>>> git diff drivers/usb/
>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>>> index 476b63618511..8d1d213d1dcd 100644
>>>>>> --- a/drivers/usb/dwc3/core.c
>>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>>> @@ -210,7 +210,9 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>>>>                    }
>>>>>>                    break;
>>>>>>            case DWC3_GCTL_PRTCAP_DEVICE:
>>>>>> -               dwc3_core_soft_reset(dwc);
>>>>>> +               ret = dwc3_core_soft_reset(dwc);
>>>>>> +               if (ret)
>>>>>> +                       goto out;
>>>>>>
>>>>>>                    dwc3_event_buffers_setup(dwc);
>>>>>>
>>>>>
>>>>> If soft-reset failed, the controller is in a bad state. We should not
>>>>> perform any further operation until the next hard reset. We should flag
>>>>> the controller as dead. I don't think we have the equivalent of the
>>>>> host's HCD_FLAG_DEAD. It may require some work in the UDC core. Perhaps
>>>>> we can flag within dwc3 for now and prevent any further operation for a
>>>>> simpler fix.
>>>>>
>>>> Hi Thinh,
>>>>
>>>>    Are you referring that if __dwc3_set_mode failed with core soft reset
>>>> timing out, the caller i.e., dwc3_set_mode who queues the work need to know
>>>> that the operation actually failed. So we can add a flag to indicate that
>>>> gadget is dead and the caller of dwc3_set_mode can check the flag to see if
>>>> the operation is successful or not.
>>>>
>>>> Or am I misunderstanding your comment ?
>>>>
>>>
>>> Not just in __dwc3_set_mode(). I mean any time dwc3_core_soft_reset
>>> fails, then we set this flag. So that it can prevent the user calling
>>> any gadget ops causing more crashes/invalid behavior. The
>>> dwc->softconnect is already wrong on pullup() on failure.
>>>
>>> So that we can have a check in different gadget ops. For pullup():
>>>
>>> static int dwc3_gadget_pullup() {
>>> 	if (dwc->udc_is_dead) {
>>> 		dev_err(dev, "reset me. x_x \n");
>>> 		return;
>>> 	}
>>>
>>> 	abc();
>>> }
>>>
>>> Perhaps the effort is probably the same if we enhance the UDC core for
>>> this? In any case, I'm fine either way.
>>>
>>> Thanks,
>>> Thinh
>>
>> Hi Thinh,
>>
>>   So you don't want UDC to retry pullup if it fails the first time ? As per
>> patch-2 of this series, I was trying to propagate the EITMEDOUT to UDC so
>> that the caller (most probably configfs) can take appropriate action as to
>> whether it must retry pullup or not.
>>
> 
> Now I'm confused. If the soft-reset times out, that means that the
> soft-reset (self-clearing) bit isn't cleared. How can we retry if it's
> stuck in this state? My impression is that soft-reset would not complete
> at all. Is that not the case for you, or it's simply because we need a
> longer soft-reset timeout?
> 
> Thanks,
> Thinh

Hi Thinh,

   Sorry for not being clear. The intention of patch-1 was to ensure we 
don't start the controller if reset times out and patch-2 was to ensure 
that UDC is in sync with controller by understanding that gadget_connect 
has failed and necessary cleanup has to be done in gadget_bind_driver.

But usually since the UDC_store is the one that is causing pullup to be 
called, the error value is propagated back to UDC_store. If it sees a 
failure, it does a retry to pullup.

I didn't check  whether subsequent retries by UDC to pullup are helping 
clear the reset bit or not. But I thought retrying pullup won't be of 
any harm.

I now get that my patches are incomplete w.r.t handling the timeout.

IIRC one of the following is what you are suggesting we need to do:

Option-1:
Set a flag when reset times out and clear it upon core_exit / core_init. 
If the flag is set, block calls to all the gadget_ops in dwc3. Basically 
even if retry happens from configfs/UDC, we bail out in pullup/udc_start 
even without trying the requested gadget operation.

Option-2:
If gadget_connect fails with -ETIMEDOUT in UDC, handle the failure and 
implement the same flag in UDC and don't even call any gadget_ops.

Regards,
Krishna,

Re: [PATCH v2 1/2] usb: dwc3: gadget: Bail out in pullup if soft reset timeout happens

[Thinh Nguyen]

On Wed, Apr 05, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 4/5/2023 3:13 AM, Thinh Nguyen wrote:
> > On Tue, Apr 04, 2023, Krishna Kurapati PSSNV wrote:
> > > 
> > > 
> > > On 4/4/2023 5:19 AM, Thinh Nguyen wrote:
> > > > On Thu, Mar 30, 2023, Krishna Kurapati PSSNV wrote:
> > > > > 
> > > > > 
> > > > > On 3/30/2023 5:40 AM, Thinh Nguyen wrote:
> > > > > > On Wed, Mar 29, 2023, Krishna Kurapati PSSNV wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 3/29/2023 2:50 AM, Thinh Nguyen wrote:
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > On Tue, Mar 28, 2023, Krishna Kurapati wrote:
> > > > > > > > > If the core soft reset timeout happens, avoid setting up event
> > > > > > > > > buffers and starting gadget as the writes to these registers
> > > > > > > > > may not reflect when in reset and setting the run stop bit
> > > > > > > > > can lead the controller to access wrong event buffer address
> > > > > > > > > resulting in a crash.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > > > > > > ---
> > > > > > > > >      drivers/usb/dwc3/gadget.c | 5 ++++-
> > > > > > > > >      1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > > > > > > > > index 3c63fa97a680..f0472801d9a5 100644
> > > > > > > > > --- a/drivers/usb/dwc3/gadget.c
> > > > > > > > > +++ b/drivers/usb/dwc3/gadget.c
> > > > > > > > > @@ -2620,13 +2620,16 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> > > > > > > > >      		 * device-initiated disconnect requires a core soft reset
> > > > > > > > >      		 * (DCTL.CSftRst) before enabling the run/stop bit.
> > > > > > > > >      		 */
> > > > > > > > > -		dwc3_core_soft_reset(dwc);
> > > > > > > > > +		ret = dwc3_core_soft_reset(dwc);
> > > > > > > > > +		if (ret)
> > > > > > > > > +			goto done;
> > > > > > > > >      		dwc3_event_buffers_setup(dwc);
> > > > > > > > >      		__dwc3_gadget_start(dwc);
> > > > > > > > >      		ret = dwc3_gadget_run_stop(dwc, true, false);
> > > > > > > > >      	}
> > > > > > > > > +done:
> > > > > > > > >      	pm_runtime_put(dwc->dev);
> > > > > > > > >      	return ret;
> > > > > > > > > -- 
> > > > > > > > > 2.40.0
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > I think there's one more place that may needs this check. Can you also
> > > > > > > > add this check in __dwc3_set_mode()?
> > > > > > > 
> > > > > > > Hi Thinh,
> > > > > > > 
> > > > > > >      Sure. Will do it.
> > > > > > > Will the below be good enough ? Or would it be good to add an error/warn log
> > > > > > > there>
> > > > > > 
> > > > > > There's already a warning message in dwc3_core_soft_reset() if it fails.
> > > > > > 
> > > > > > > 
> > > > > > > kriskura@hu-kriskura-hyd:/local/mnt/workspace/krishna/skales2/skales/kernel$
> > > > > > > git diff drivers/usb/
> > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > > > > index 476b63618511..8d1d213d1dcd 100644
> > > > > > > --- a/drivers/usb/dwc3/core.c
> > > > > > > +++ b/drivers/usb/dwc3/core.c
> > > > > > > @@ -210,7 +210,9 @@ static void __dwc3_set_mode(struct work_struct *work)
> > > > > > >                    }
> > > > > > >                    break;
> > > > > > >            case DWC3_GCTL_PRTCAP_DEVICE:
> > > > > > > -               dwc3_core_soft_reset(dwc);
> > > > > > > +               ret = dwc3_core_soft_reset(dwc);
> > > > > > > +               if (ret)
> > > > > > > +                       goto out;
> > > > > > > 
> > > > > > >                    dwc3_event_buffers_setup(dwc);
> > > > > > > 
> > > > > > 
> > > > > > If soft-reset failed, the controller is in a bad state. We should not
> > > > > > perform any further operation until the next hard reset. We should flag
> > > > > > the controller as dead. I don't think we have the equivalent of the
> > > > > > host's HCD_FLAG_DEAD. It may require some work in the UDC core. Perhaps
> > > > > > we can flag within dwc3 for now and prevent any further operation for a
> > > > > > simpler fix.
> > > > > > 
> > > > > Hi Thinh,
> > > > > 
> > > > >    Are you referring that if __dwc3_set_mode failed with core soft reset
> > > > > timing out, the caller i.e., dwc3_set_mode who queues the work need to know
> > > > > that the operation actually failed. So we can add a flag to indicate that
> > > > > gadget is dead and the caller of dwc3_set_mode can check the flag to see if
> > > > > the operation is successful or not.
> > > > > 
> > > > > Or am I misunderstanding your comment ?
> > > > > 
> > > > 
> > > > Not just in __dwc3_set_mode(). I mean any time dwc3_core_soft_reset
> > > > fails, then we set this flag. So that it can prevent the user calling
> > > > any gadget ops causing more crashes/invalid behavior. The
> > > > dwc->softconnect is already wrong on pullup() on failure.
> > > > 
> > > > So that we can have a check in different gadget ops. For pullup():
> > > > 
> > > > static int dwc3_gadget_pullup() {
> > > > 	if (dwc->udc_is_dead) {
> > > > 		dev_err(dev, "reset me. x_x \n");
> > > > 		return;
> > > > 	}
> > > > 
> > > > 	abc();
> > > > }
> > > > 
> > > > Perhaps the effort is probably the same if we enhance the UDC core for
> > > > this? In any case, I'm fine either way.
> > > > 
> > > > Thanks,
> > > > Thinh
> > > 
> > > Hi Thinh,
> > > 
> > >   So you don't want UDC to retry pullup if it fails the first time ? As per
> > > patch-2 of this series, I was trying to propagate the EITMEDOUT to UDC so
> > > that the caller (most probably configfs) can take appropriate action as to
> > > whether it must retry pullup or not.
> > > 
> > 
> > Now I'm confused. If the soft-reset times out, that means that the
> > soft-reset (self-clearing) bit isn't cleared. How can we retry if it's
> > stuck in this state? My impression is that soft-reset would not complete
> > at all. Is that not the case for you, or it's simply because we need a
> > longer soft-reset timeout?
> > 
> > Thanks,
> > Thinh
> 
> Hi Thinh,
> 
>   Sorry for not being clear. The intention of patch-1 was to ensure we don't
> start the controller if reset times out and patch-2 was to ensure that UDC
> is in sync with controller by understanding that gadget_connect has failed
> and necessary cleanup has to be done in gadget_bind_driver.

That should still be there.

> 
> But usually since the UDC_store is the one that is causing pullup to be
> called, the error value is propagated back to UDC_store. If it sees a
> failure, it does a retry to pullup.
> 
> I didn't check  whether subsequent retries by UDC to pullup are helping
> clear the reset bit or not. But I thought retrying pullup won't be of any
> harm.

It's fine to retry. I'm thinking that the controller is in a bad state
at this point that we can't recover (hopefully that's not the case).

> 
> I now get that my patches are incomplete w.r.t handling the timeout.
> 
> IIRC one of the following is what you are suggesting we need to do:
> 
> Option-1:
> Set a flag when reset times out and clear it upon core_exit / core_init. If
> the flag is set, block calls to all the gadget_ops in dwc3. Basically even
> if retry happens from configfs/UDC, we bail out in pullup/udc_start even
> without trying the requested gadget operation.
> 
> Option-2:
> If gadget_connect fails with -ETIMEDOUT in UDC, handle the failure and
> implement the same flag in UDC and don't even call any gadget_ops.
> 

I'm thinking of option-1. For option-2, we can't control if the
gadget_ops will be called. We only have control how we will respond in
case they get called again.

But now I'm thinking again, I think it may be ok without adding the
flag. The UDC core and gadget driver won't do anything else before
pullup(1) is successful. Calling other gadget_ops should be harmless
(ie. it won't crash/break the system)?

Sorry for the noise, but I think it may be ok without marking the
controller dead. I wonder if we can confirm my suspiction on retry? I
believe this is not easy to reproduce on your setup? If not, I think we
can take your change as is.

Thanks,
Thinh

Re: [PATCH v2 1/2] usb: dwc3: gadget: Bail out in pullup if soft reset timeout happens

[Krishna Kurapati PSSNV]

On 4/6/2023 6:15 AM, Thinh Nguyen wrote:
> On Wed, Apr 05, 2023, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 4/5/2023 3:13 AM, Thinh Nguyen wrote:
>>> On Tue, Apr 04, 2023, Krishna Kurapati PSSNV wrote:
>>>>
>>>>
>>>> On 4/4/2023 5:19 AM, Thinh Nguyen wrote:
>>>>> On Thu, Mar 30, 2023, Krishna Kurapati PSSNV wrote:
>>>>>>
>>>>>>
>>>>>> On 3/30/2023 5:40 AM, Thinh Nguyen wrote:
>>>>>>> On Wed, Mar 29, 2023, Krishna Kurapati PSSNV wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 3/29/2023 2:50 AM, Thinh Nguyen wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On Tue, Mar 28, 2023, Krishna Kurapati wrote:
>>>>>>>>>> If the core soft reset timeout happens, avoid setting up event
>>>>>>>>>> buffers and starting gadget as the writes to these registers
>>>>>>>>>> may not reflect when in reset and setting the run stop bit
>>>>>>>>>> can lead the controller to access wrong event buffer address
>>>>>>>>>> resulting in a crash.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>>>>>>>> ---
>>>>>>>>>>       drivers/usb/dwc3/gadget.c | 5 ++++-
>>>>>>>>>>       1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>>>>>>>>> index 3c63fa97a680..f0472801d9a5 100644
>>>>>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>>>>>> @@ -2620,13 +2620,16 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>>>>>>>>>>       		 * device-initiated disconnect requires a core soft reset
>>>>>>>>>>       		 * (DCTL.CSftRst) before enabling the run/stop bit.
>>>>>>>>>>       		 */
>>>>>>>>>> -		dwc3_core_soft_reset(dwc);
>>>>>>>>>> +		ret = dwc3_core_soft_reset(dwc);
>>>>>>>>>> +		if (ret)
>>>>>>>>>> +			goto done;
>>>>>>>>>>       		dwc3_event_buffers_setup(dwc);
>>>>>>>>>>       		__dwc3_gadget_start(dwc);
>>>>>>>>>>       		ret = dwc3_gadget_run_stop(dwc, true, false);
>>>>>>>>>>       	}
>>>>>>>>>> +done:
>>>>>>>>>>       	pm_runtime_put(dwc->dev);
>>>>>>>>>>       	return ret;
>>>>>>>>>> -- 
>>>>>>>>>> 2.40.0
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I think there's one more place that may needs this check. Can you also
>>>>>>>>> add this check in __dwc3_set_mode()?
>>>>>>>>
>>>>>>>> Hi Thinh,
>>>>>>>>
>>>>>>>>       Sure. Will do it.
>>>>>>>> Will the below be good enough ? Or would it be good to add an error/warn log
>>>>>>>> there>
>>>>>>>
>>>>>>> There's already a warning message in dwc3_core_soft_reset() if it fails.
>>>>>>>
>>>>>>>>
>>>>>>>> kriskura@hu-kriskura-hyd:/local/mnt/workspace/krishna/skales2/skales/kernel$
>>>>>>>> git diff drivers/usb/
>>>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>>>>> index 476b63618511..8d1d213d1dcd 100644
>>>>>>>> --- a/drivers/usb/dwc3/core.c
>>>>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>>>>> @@ -210,7 +210,9 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>>>>>>                     }
>>>>>>>>                     break;
>>>>>>>>             case DWC3_GCTL_PRTCAP_DEVICE:
>>>>>>>> -               dwc3_core_soft_reset(dwc);
>>>>>>>> +               ret = dwc3_core_soft_reset(dwc);
>>>>>>>> +               if (ret)
>>>>>>>> +                       goto out;
>>>>>>>>
>>>>>>>>                     dwc3_event_buffers_setup(dwc);
>>>>>>>>
>>>>>>>
>>>>>>> If soft-reset failed, the controller is in a bad state. We should not
>>>>>>> perform any further operation until the next hard reset. We should flag
>>>>>>> the controller as dead. I don't think we have the equivalent of the
>>>>>>> host's HCD_FLAG_DEAD. It may require some work in the UDC core. Perhaps
>>>>>>> we can flag within dwc3 for now and prevent any further operation for a
>>>>>>> simpler fix.
>>>>>>>
>>>>>> Hi Thinh,
>>>>>>
>>>>>>     Are you referring that if __dwc3_set_mode failed with core soft reset
>>>>>> timing out, the caller i.e., dwc3_set_mode who queues the work need to know
>>>>>> that the operation actually failed. So we can add a flag to indicate that
>>>>>> gadget is dead and the caller of dwc3_set_mode can check the flag to see if
>>>>>> the operation is successful or not.
>>>>>>
>>>>>> Or am I misunderstanding your comment ?
>>>>>>
>>>>>
>>>>> Not just in __dwc3_set_mode(). I mean any time dwc3_core_soft_reset
>>>>> fails, then we set this flag. So that it can prevent the user calling
>>>>> any gadget ops causing more crashes/invalid behavior. The
>>>>> dwc->softconnect is already wrong on pullup() on failure.
>>>>>
>>>>> So that we can have a check in different gadget ops. For pullup():
>>>>>
>>>>> static int dwc3_gadget_pullup() {
>>>>> 	if (dwc->udc_is_dead) {
>>>>> 		dev_err(dev, "reset me. x_x \n");
>>>>> 		return;
>>>>> 	}
>>>>>
>>>>> 	abc();
>>>>> }
>>>>>
>>>>> Perhaps the effort is probably the same if we enhance the UDC core for
>>>>> this? In any case, I'm fine either way.
>>>>>
>>>>> Thanks,
>>>>> Thinh
>>>>
>>>> Hi Thinh,
>>>>
>>>>    So you don't want UDC to retry pullup if it fails the first time ? As per
>>>> patch-2 of this series, I was trying to propagate the EITMEDOUT to UDC so
>>>> that the caller (most probably configfs) can take appropriate action as to
>>>> whether it must retry pullup or not.
>>>>
>>>
>>> Now I'm confused. If the soft-reset times out, that means that the
>>> soft-reset (self-clearing) bit isn't cleared. How can we retry if it's
>>> stuck in this state? My impression is that soft-reset would not complete
>>> at all. Is that not the case for you, or it's simply because we need a
>>> longer soft-reset timeout?
>>>
>>> Thanks,
>>> Thinh
>>
>> Hi Thinh,
>>
>>    Sorry for not being clear. The intention of patch-1 was to ensure we don't
>> start the controller if reset times out and patch-2 was to ensure that UDC
>> is in sync with controller by understanding that gadget_connect has failed
>> and necessary cleanup has to be done in gadget_bind_driver.
> 
> That should still be there.
> 
>>
>> But usually since the UDC_store is the one that is causing pullup to be
>> called, the error value is propagated back to UDC_store. If it sees a
>> failure, it does a retry to pullup.
>>
>> I didn't check  whether subsequent retries by UDC to pullup are helping
>> clear the reset bit or not. But I thought retrying pullup won't be of any
>> harm.
> 
> It's fine to retry. I'm thinking that the controller is in a bad state
> at this point that we can't recover (hopefully that's not the case).
> 
>>
>> I now get that my patches are incomplete w.r.t handling the timeout.
>>
>> IIRC one of the following is what you are suggesting we need to do:
>>
>> Option-1:
>> Set a flag when reset times out and clear it upon core_exit / core_init. If
>> the flag is set, block calls to all the gadget_ops in dwc3. Basically even
>> if retry happens from configfs/UDC, we bail out in pullup/udc_start even
>> without trying the requested gadget operation.
>>
>> Option-2:
>> If gadget_connect fails with -ETIMEDOUT in UDC, handle the failure and
>> implement the same flag in UDC and don't even call any gadget_ops.
>>
> 
Hi Thinh,

Thanks for the review.

> I'm thinking of option-1. For option-2, we can't control if the
> gadget_ops will be called. We only have control how we will respond in
> case they get called again.
> 
> But now I'm thinking again, I think it may be ok without adding the
> flag. The UDC core and gadget driver won't do anything else before
> pullup(1) is successful. Calling other gadget_ops should be harmless
> (ie. it won't crash/break the system)?
> 
I can give this a try in long run testing (For 7-14 days) and see if 
anything else is breaking.

Most probably we do a composition switch / PIPO in between which can 
call usb_gadget_unregister_driver which might invoke a pullup(0) 
followed by udc_stop() and like you mentioned must not be a problem.

> Sorry for the noise, but I think it may be ok without marking the
> controller dead. I wonder if we can confirm my suspiction on retry? I
> believe this is not easy to reproduce on your setup? If not, I think we
> can take your change as is.
> 
Regards,
Krishna,

Re: [PATCH v2 1/2] usb: dwc3: gadget: Bail out in pullup if soft reset timeout happens

[Krishna Kurapati PSSNV]

On 4/6/2023 7:44 AM, Krishna Kurapati PSSNV wrote:
> 
> 
> On 4/6/2023 6:15 AM, Thinh Nguyen wrote:
>> On Wed, Apr 05, 2023, Krishna Kurapati PSSNV wrote:
>>>
>>>
>>> On 4/5/2023 3:13 AM, Thinh Nguyen wrote:
>>>> On Tue, Apr 04, 2023, Krishna Kurapati PSSNV wrote:
>>>>>
>>>>>
>>>>> On 4/4/2023 5:19 AM, Thinh Nguyen wrote:
>>>>>> On Thu, Mar 30, 2023, Krishna Kurapati PSSNV wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 3/30/2023 5:40 AM, Thinh Nguyen wrote:
>>>>>>>> On Wed, Mar 29, 2023, Krishna Kurapati PSSNV wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 3/29/2023 2:50 AM, Thinh Nguyen wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> On Tue, Mar 28, 2023, Krishna Kurapati wrote:
>>>>>>>>>>> If the core soft reset timeout happens, avoid setting up event
>>>>>>>>>>> buffers and starting gadget as the writes to these registers
>>>>>>>>>>> may not reflect when in reset and setting the run stop bit
>>>>>>>>>>> can lead the controller to access wrong event buffer address
>>>>>>>>>>> resulting in a crash.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>>>>>>>>> ---
>>>>>>>>>>>       drivers/usb/dwc3/gadget.c | 5 ++++-
>>>>>>>>>>>       1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c 
>>>>>>>>>>> b/drivers/usb/dwc3/gadget.c
>>>>>>>>>>> index 3c63fa97a680..f0472801d9a5 100644
>>>>>>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>>>>>>> @@ -2620,13 +2620,16 @@ static int dwc3_gadget_pullup(struct 
>>>>>>>>>>> usb_gadget *g, int is_on)
>>>>>>>>>>>                * device-initiated disconnect requires a core 
>>>>>>>>>>> soft reset
>>>>>>>>>>>                * (DCTL.CSftRst) before enabling the run/stop 
>>>>>>>>>>> bit.
>>>>>>>>>>>                */
>>>>>>>>>>> -        dwc3_core_soft_reset(dwc);
>>>>>>>>>>> +        ret = dwc3_core_soft_reset(dwc);
>>>>>>>>>>> +        if (ret)
>>>>>>>>>>> +            goto done;
>>>>>>>>>>>               dwc3_event_buffers_setup(dwc);
>>>>>>>>>>>               __dwc3_gadget_start(dwc);
>>>>>>>>>>>               ret = dwc3_gadget_run_stop(dwc, true, false);
>>>>>>>>>>>           }
>>>>>>>>>>> +done:
>>>>>>>>>>>           pm_runtime_put(dwc->dev);
>>>>>>>>>>>           return ret;
>>>>>>>>>>> -- 
>>>>>>>>>>> 2.40.0
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I think there's one more place that may needs this check. Can 
>>>>>>>>>> you also
>>>>>>>>>> add this check in __dwc3_set_mode()?
>>>>>>>>>
>>>>>>>>> Hi Thinh,
>>>>>>>>>
>>>>>>>>>       Sure. Will do it.
>>>>>>>>> Will the below be good enough ? Or would it be good to add an 
>>>>>>>>> error/warn log
>>>>>>>>> there>
>>>>>>>>
>>>>>>>> There's already a warning message in dwc3_core_soft_reset() if 
>>>>>>>> it fails.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> kriskura@hu-kriskura-hyd:/local/mnt/workspace/krishna/skales2/skales/kernel$
>>>>>>>>> git diff drivers/usb/
>>>>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>>>>>> index 476b63618511..8d1d213d1dcd 100644
>>>>>>>>> --- a/drivers/usb/dwc3/core.c
>>>>>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>>>>>> @@ -210,7 +210,9 @@ static void __dwc3_set_mode(struct 
>>>>>>>>> work_struct *work)
>>>>>>>>>                     }
>>>>>>>>>                     break;
>>>>>>>>>             case DWC3_GCTL_PRTCAP_DEVICE:
>>>>>>>>> -               dwc3_core_soft_reset(dwc);
>>>>>>>>> +               ret = dwc3_core_soft_reset(dwc);
>>>>>>>>> +               if (ret)
>>>>>>>>> +                       goto out;
>>>>>>>>>
>>>>>>>>>                     dwc3_event_buffers_setup(dwc);
>>>>>>>>>
>>>>>>>>
>>>>>>>> If soft-reset failed, the controller is in a bad state. We 
>>>>>>>> should not
>>>>>>>> perform any further operation until the next hard reset. We 
>>>>>>>> should flag
>>>>>>>> the controller as dead. I don't think we have the equivalent of the
>>>>>>>> host's HCD_FLAG_DEAD. It may require some work in the UDC core. 
>>>>>>>> Perhaps
>>>>>>>> we can flag within dwc3 for now and prevent any further 
>>>>>>>> operation for a
>>>>>>>> simpler fix.
>>>>>>>>
>>>>>>> Hi Thinh,
>>>>>>>
>>>>>>>     Are you referring that if __dwc3_set_mode failed with core 
>>>>>>> soft reset
>>>>>>> timing out, the caller i.e., dwc3_set_mode who queues the work 
>>>>>>> need to know
>>>>>>> that the operation actually failed. So we can add a flag to 
>>>>>>> indicate that
>>>>>>> gadget is dead and the caller of dwc3_set_mode can check the flag 
>>>>>>> to see if
>>>>>>> the operation is successful or not.
>>>>>>>
>>>>>>> Or am I misunderstanding your comment ?
>>>>>>>
>>>>>>
>>>>>> Not just in __dwc3_set_mode(). I mean any time dwc3_core_soft_reset
>>>>>> fails, then we set this flag. So that it can prevent the user calling
>>>>>> any gadget ops causing more crashes/invalid behavior. The
>>>>>> dwc->softconnect is already wrong on pullup() on failure.
>>>>>>
>>>>>> So that we can have a check in different gadget ops. For pullup():
>>>>>>
>>>>>> static int dwc3_gadget_pullup() {
>>>>>>     if (dwc->udc_is_dead) {
>>>>>>         dev_err(dev, "reset me. x_x \n");
>>>>>>         return;
>>>>>>     }
>>>>>>
>>>>>>     abc();
>>>>>> }
>>>>>>
>>>>>> Perhaps the effort is probably the same if we enhance the UDC core 
>>>>>> for
>>>>>> this? In any case, I'm fine either way.
>>>>>>
>>>>>> Thanks,
>>>>>> Thinh
>>>>>
>>>>> Hi Thinh,
>>>>>
>>>>>    So you don't want UDC to retry pullup if it fails the first time 
>>>>> ? As per
>>>>> patch-2 of this series, I was trying to propagate the EITMEDOUT to 
>>>>> UDC so
>>>>> that the caller (most probably configfs) can take appropriate 
>>>>> action as to
>>>>> whether it must retry pullup or not.
>>>>>
>>>>
>>>> Now I'm confused. If the soft-reset times out, that means that the
>>>> soft-reset (self-clearing) bit isn't cleared. How can we retry if it's
>>>> stuck in this state? My impression is that soft-reset would not 
>>>> complete
>>>> at all. Is that not the case for you, or it's simply because we need a
>>>> longer soft-reset timeout?
>>>>
>>>> Thanks,
>>>> Thinh
>>>
>>> Hi Thinh,
>>>
>>>    Sorry for not being clear. The intention of patch-1 was to ensure 
>>> we don't
>>> start the controller if reset times out and patch-2 was to ensure 
>>> that UDC
>>> is in sync with controller by understanding that gadget_connect has 
>>> failed
>>> and necessary cleanup has to be done in gadget_bind_driver.
>>
>> That should still be there.
>>
>>>
>>> But usually since the UDC_store is the one that is causing pullup to be
>>> called, the error value is propagated back to UDC_store. If it sees a
>>> failure, it does a retry to pullup.
>>>
>>> I didn't check  whether subsequent retries by UDC to pullup are helping
>>> clear the reset bit or not. But I thought retrying pullup won't be of 
>>> any
>>> harm.
>>
>> It's fine to retry. I'm thinking that the controller is in a bad state
>> at this point that we can't recover (hopefully that's not the case).
>>
>>>
>>> I now get that my patches are incomplete w.r.t handling the timeout.
>>>
>>> IIRC one of the following is what you are suggesting we need to do:
>>>
>>> Option-1:
>>> Set a flag when reset times out and clear it upon core_exit / 
>>> core_init. If
>>> the flag is set, block calls to all the gadget_ops in dwc3. Basically 
>>> even
>>> if retry happens from configfs/UDC, we bail out in pullup/udc_start even
>>> without trying the requested gadget operation.
>>>
>>> Option-2:
>>> If gadget_connect fails with -ETIMEDOUT in UDC, handle the failure and
>>> implement the same flag in UDC and don't even call any gadget_ops.
>>>
>>
> Hi Thinh,
> 
> Thanks for the review.
> 
>> I'm thinking of option-1. For option-2, we can't control if the
>> gadget_ops will be called. We only have control how we will respond in
>> case they get called again.
>>
>> But now I'm thinking again, I think it may be ok without adding the
>> flag. The UDC core and gadget driver won't do anything else before
>> pullup(1) is successful. Calling other gadget_ops should be harmless
>> (ie. it won't crash/break the system)?
>>
> I can give this a try in long run testing (For 7-14 days) and see if 
> anything else is breaking.
> 
> Most probably we do a composition switch / PIPO in between which can 
> call usb_gadget_unregister_driver which might invoke a pullup(0) 
> followed by udc_stop() and like you mentioned must not be a problem.
> 
>> Sorry for the noise, but I think it may be ok without marking the
>> controller dead. I wonder if we can confirm my suspiction on retry? I
>> believe this is not easy to reproduce on your setup? If not, I think we
>> can take your change as is.

Hi Thinh,

   I got this patch tested on two diff Gen-2 targets for around 10 days 
and no issues were seen. (No SMMU fault seen on a 10 day run). Let me 
know of any other concerns that might come up with this patch. Else I 
can rebase it to get merged.

Regards,
Krishna,

Re: [PATCH v2 1/2] usb: dwc3: gadget: Bail out in pullup if soft reset timeout happens

[Thinh Nguyen]

On Tue, Apr 25, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 4/6/2023 7:44 AM, Krishna Kurapati PSSNV wrote:
> > 
> > 
> > On 4/6/2023 6:15 AM, Thinh Nguyen wrote:
> > > On Wed, Apr 05, 2023, Krishna Kurapati PSSNV wrote:
> > > > 
> > > > 
> > > > On 4/5/2023 3:13 AM, Thinh Nguyen wrote:
> > > > > On Tue, Apr 04, 2023, Krishna Kurapati PSSNV wrote:
> > > > > > 
> > > > > > 
> > > > > > On 4/4/2023 5:19 AM, Thinh Nguyen wrote:
> > > > > > > On Thu, Mar 30, 2023, Krishna Kurapati PSSNV wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On 3/30/2023 5:40 AM, Thinh Nguyen wrote:
> > > > > > > > > On Wed, Mar 29, 2023, Krishna Kurapati PSSNV wrote:
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > On 3/29/2023 2:50 AM, Thinh Nguyen wrote:
> > > > > > > > > > > Hi,
> > > > > > > > > > > 
> > > > > > > > > > > On Tue, Mar 28, 2023, Krishna Kurapati wrote:
> > > > > > > > > > > > If the core soft reset timeout happens, avoid setting up event
> > > > > > > > > > > > buffers and starting gadget as the writes to these registers
> > > > > > > > > > > > may not reflect when in reset and setting the run stop bit
> > > > > > > > > > > > can lead the controller to access wrong event buffer address
> > > > > > > > > > > > resulting in a crash.
> > > > > > > > > > > > 
> > > > > > > > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > > > > > > > > > ---
> > > > > > > > > > > >       drivers/usb/dwc3/gadget.c | 5 ++++-
> > > > > > > > > > > >       1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > > > > > > > > 
> > > > > > > > > > > > diff --git
> > > > > > > > > > > > a/drivers/usb/dwc3/gadget.c
> > > > > > > > > > > > b/drivers/usb/dwc3/gadget.c
> > > > > > > > > > > > index 3c63fa97a680..f0472801d9a5 100644
> > > > > > > > > > > > --- a/drivers/usb/dwc3/gadget.c
> > > > > > > > > > > > +++ b/drivers/usb/dwc3/gadget.c
> > > > > > > > > > > > @@ -2620,13 +2620,16 @@ static
> > > > > > > > > > > > int dwc3_gadget_pullup(struct
> > > > > > > > > > > > usb_gadget *g, int is_on)
> > > > > > > > > > > >                *
> > > > > > > > > > > > device-initiated disconnect
> > > > > > > > > > > > requires a core soft reset
> > > > > > > > > > > >                * (DCTL.CSftRst)
> > > > > > > > > > > > before enabling the run/stop
> > > > > > > > > > > > bit.
> > > > > > > > > > > >                */
> > > > > > > > > > > > -        dwc3_core_soft_reset(dwc);
> > > > > > > > > > > > +        ret = dwc3_core_soft_reset(dwc);
> > > > > > > > > > > > +        if (ret)
> > > > > > > > > > > > +            goto done;
> > > > > > > > > > > >               dwc3_event_buffers_setup(dwc);
> > > > > > > > > > > >               __dwc3_gadget_start(dwc);
> > > > > > > > > > > >               ret = dwc3_gadget_run_stop(dwc, true, false);
> > > > > > > > > > > >           }
> > > > > > > > > > > > +done:
> > > > > > > > > > > >           pm_runtime_put(dwc->dev);
> > > > > > > > > > > >           return ret;
> > > > > > > > > > > > -- 
> > > > > > > > > > > > 2.40.0
> > > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > I think there's one more place that
> > > > > > > > > > > may needs this check. Can you also
> > > > > > > > > > > add this check in __dwc3_set_mode()?
> > > > > > > > > > 
> > > > > > > > > > Hi Thinh,
> > > > > > > > > > 
> > > > > > > > > >       Sure. Will do it.
> > > > > > > > > > Will the below be good enough ? Or would
> > > > > > > > > > it be good to add an error/warn log
> > > > > > > > > > there>
> > > > > > > > > 
> > > > > > > > > There's already a warning message in
> > > > > > > > > dwc3_core_soft_reset() if it fails.
> > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > kriskura@hu-kriskura-hyd:/local/mnt/workspace/krishna/skales2/skales/kernel$
> > > > > > > > > > git diff drivers/usb/
> > > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > > > > > > > index 476b63618511..8d1d213d1dcd 100644
> > > > > > > > > > --- a/drivers/usb/dwc3/core.c
> > > > > > > > > > +++ b/drivers/usb/dwc3/core.c
> > > > > > > > > > @@ -210,7 +210,9 @@ static void
> > > > > > > > > > __dwc3_set_mode(struct work_struct
> > > > > > > > > > *work)
> > > > > > > > > >                     }
> > > > > > > > > >                     break;
> > > > > > > > > >             case DWC3_GCTL_PRTCAP_DEVICE:
> > > > > > > > > > -               dwc3_core_soft_reset(dwc);
> > > > > > > > > > +               ret = dwc3_core_soft_reset(dwc);
> > > > > > > > > > +               if (ret)
> > > > > > > > > > +                       goto out;
> > > > > > > > > > 
> > > > > > > > > >                     dwc3_event_buffers_setup(dwc);
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > If soft-reset failed, the controller is in a
> > > > > > > > > bad state. We should not
> > > > > > > > > perform any further operation until the next
> > > > > > > > > hard reset. We should flag
> > > > > > > > > the controller as dead. I don't think we have the equivalent of the
> > > > > > > > > host's HCD_FLAG_DEAD. It may require some
> > > > > > > > > work in the UDC core. Perhaps
> > > > > > > > > we can flag within dwc3 for now and prevent
> > > > > > > > > any further operation for a
> > > > > > > > > simpler fix.
> > > > > > > > > 
> > > > > > > > Hi Thinh,
> > > > > > > > 
> > > > > > > >     Are you referring that if __dwc3_set_mode
> > > > > > > > failed with core soft reset
> > > > > > > > timing out, the caller i.e., dwc3_set_mode who
> > > > > > > > queues the work need to know
> > > > > > > > that the operation actually failed. So we can
> > > > > > > > add a flag to indicate that
> > > > > > > > gadget is dead and the caller of dwc3_set_mode
> > > > > > > > can check the flag to see if
> > > > > > > > the operation is successful or not.
> > > > > > > > 
> > > > > > > > Or am I misunderstanding your comment ?
> > > > > > > > 
> > > > > > > 
> > > > > > > Not just in __dwc3_set_mode(). I mean any time dwc3_core_soft_reset
> > > > > > > fails, then we set this flag. So that it can prevent the user calling
> > > > > > > any gadget ops causing more crashes/invalid behavior. The
> > > > > > > dwc->softconnect is already wrong on pullup() on failure.
> > > > > > > 
> > > > > > > So that we can have a check in different gadget ops. For pullup():
> > > > > > > 
> > > > > > > static int dwc3_gadget_pullup() {
> > > > > > >     if (dwc->udc_is_dead) {
> > > > > > >         dev_err(dev, "reset me. x_x \n");
> > > > > > >         return;
> > > > > > >     }
> > > > > > > 
> > > > > > >     abc();
> > > > > > > }
> > > > > > > 
> > > > > > > Perhaps the effort is probably the same if we
> > > > > > > enhance the UDC core for
> > > > > > > this? In any case, I'm fine either way.
> > > > > > > 
> > > > > > > Thanks,
> > > > > > > Thinh
> > > > > > 
> > > > > > Hi Thinh,
> > > > > > 
> > > > > >    So you don't want UDC to retry pullup if it fails the
> > > > > > first time ? As per
> > > > > > patch-2 of this series, I was trying to propagate the
> > > > > > EITMEDOUT to UDC so
> > > > > > that the caller (most probably configfs) can take
> > > > > > appropriate action as to
> > > > > > whether it must retry pullup or not.
> > > > > > 
> > > > > 
> > > > > Now I'm confused. If the soft-reset times out, that means that the
> > > > > soft-reset (self-clearing) bit isn't cleared. How can we retry if it's
> > > > > stuck in this state? My impression is that soft-reset would
> > > > > not complete
> > > > > at all. Is that not the case for you, or it's simply because we need a
> > > > > longer soft-reset timeout?
> > > > > 
> > > > > Thanks,
> > > > > Thinh
> > > > 
> > > > Hi Thinh,
> > > > 
> > > >    Sorry for not being clear. The intention of patch-1 was to
> > > > ensure we don't
> > > > start the controller if reset times out and patch-2 was to
> > > > ensure that UDC
> > > > is in sync with controller by understanding that gadget_connect
> > > > has failed
> > > > and necessary cleanup has to be done in gadget_bind_driver.
> > > 
> > > That should still be there.
> > > 
> > > > 
> > > > But usually since the UDC_store is the one that is causing pullup to be
> > > > called, the error value is propagated back to UDC_store. If it sees a
> > > > failure, it does a retry to pullup.
> > > > 
> > > > I didn't check  whether subsequent retries by UDC to pullup are helping
> > > > clear the reset bit or not. But I thought retrying pullup won't
> > > > be of any
> > > > harm.
> > > 
> > > It's fine to retry. I'm thinking that the controller is in a bad state
> > > at this point that we can't recover (hopefully that's not the case).
> > > 
> > > > 
> > > > I now get that my patches are incomplete w.r.t handling the timeout.
> > > > 
> > > > IIRC one of the following is what you are suggesting we need to do:
> > > > 
> > > > Option-1:
> > > > Set a flag when reset times out and clear it upon core_exit /
> > > > core_init. If
> > > > the flag is set, block calls to all the gadget_ops in dwc3.
> > > > Basically even
> > > > if retry happens from configfs/UDC, we bail out in pullup/udc_start even
> > > > without trying the requested gadget operation.
> > > > 
> > > > Option-2:
> > > > If gadget_connect fails with -ETIMEDOUT in UDC, handle the failure and
> > > > implement the same flag in UDC and don't even call any gadget_ops.
> > > > 
> > > 
> > Hi Thinh,
> > 
> > Thanks for the review.
> > 
> > > I'm thinking of option-1. For option-2, we can't control if the
> > > gadget_ops will be called. We only have control how we will respond in
> > > case they get called again.
> > > 
> > > But now I'm thinking again, I think it may be ok without adding the
> > > flag. The UDC core and gadget driver won't do anything else before
> > > pullup(1) is successful. Calling other gadget_ops should be harmless
> > > (ie. it won't crash/break the system)?
> > > 
> > I can give this a try in long run testing (For 7-14 days) and see if
> > anything else is breaking.
> > 
> > Most probably we do a composition switch / PIPO in between which can
> > call usb_gadget_unregister_driver which might invoke a pullup(0)
> > followed by udc_stop() and like you mentioned must not be a problem.
> > 
> > > Sorry for the noise, but I think it may be ok without marking the
> > > controller dead. I wonder if we can confirm my suspiction on retry? I
> > > believe this is not easy to reproduce on your setup? If not, I think we
> > > can take your change as is.
> 
> Hi Thinh,
> 
>   I got this patch tested on two diff Gen-2 targets for around 10 days and
> no issues were seen. (No SMMU fault seen on a 10 day run). Let me know of
> any other concerns that might come up with this patch. Else I can rebase it
> to get merged.
> 
> Regards,
> Krishna,

Thanks for the tests. So you were able to observe the failure and able
to recover from it without SMMU fault right?

Thanks,
Thinh

Re: [PATCH v2 1/2] usb: dwc3: gadget: Bail out in pullup if soft reset timeout happens

[Krishna Kurapati PSSNV]

On 4/26/2023 5:52 AM, Thinh Nguyen wrote:
> On Tue, Apr 25, 2023, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 4/6/2023 7:44 AM, Krishna Kurapati PSSNV wrote:
>>>
>>>
>>> On 4/6/2023 6:15 AM, Thinh Nguyen wrote:
>>>> On Wed, Apr 05, 2023, Krishna Kurapati PSSNV wrote:
>>>>>
>>>>>
>>>>> On 4/5/2023 3:13 AM, Thinh Nguyen wrote:
>>>>>> On Tue, Apr 04, 2023, Krishna Kurapati PSSNV wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 4/4/2023 5:19 AM, Thinh Nguyen wrote:
>>>>>>>> On Thu, Mar 30, 2023, Krishna Kurapati PSSNV wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 3/30/2023 5:40 AM, Thinh Nguyen wrote:
>>>>>>>>>> On Wed, Mar 29, 2023, Krishna Kurapati PSSNV wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 3/29/2023 2:50 AM, Thinh Nguyen wrote:
>>>>>>>>>>>> Hi,
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Mar 28, 2023, Krishna Kurapati wrote:
>>>>>>>>>>>>> If the core soft reset timeout happens, avoid setting up event
>>>>>>>>>>>>> buffers and starting gadget as the writes to these registers
>>>>>>>>>>>>> may not reflect when in reset and setting the run stop bit
>>>>>>>>>>>>> can lead the controller to access wrong event buffer address
>>>>>>>>>>>>> resulting in a crash.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>        drivers/usb/dwc3/gadget.c | 5 ++++-
>>>>>>>>>>>>>        1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git
>>>>>>>>>>>>> a/drivers/usb/dwc3/gadget.c
>>>>>>>>>>>>> b/drivers/usb/dwc3/gadget.c
>>>>>>>>>>>>> index 3c63fa97a680..f0472801d9a5 100644
>>>>>>>>>>>>> --- a/drivers/usb/dwc3/gadget.c
>>>>>>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c
>>>>>>>>>>>>> @@ -2620,13 +2620,16 @@ static
>>>>>>>>>>>>> int dwc3_gadget_pullup(struct
>>>>>>>>>>>>> usb_gadget *g, int is_on)
>>>>>>>>>>>>>                 *
>>>>>>>>>>>>> device-initiated disconnect
>>>>>>>>>>>>> requires a core soft reset
>>>>>>>>>>>>>                 * (DCTL.CSftRst)
>>>>>>>>>>>>> before enabling the run/stop
>>>>>>>>>>>>> bit.
>>>>>>>>>>>>>                 */
>>>>>>>>>>>>> -        dwc3_core_soft_reset(dwc);
>>>>>>>>>>>>> +        ret = dwc3_core_soft_reset(dwc);
>>>>>>>>>>>>> +        if (ret)
>>>>>>>>>>>>> +            goto done;
>>>>>>>>>>>>>                dwc3_event_buffers_setup(dwc);
>>>>>>>>>>>>>                __dwc3_gadget_start(dwc);
>>>>>>>>>>>>>                ret = dwc3_gadget_run_stop(dwc, true, false);
>>>>>>>>>>>>>            }
>>>>>>>>>>>>> +done:
>>>>>>>>>>>>>            pm_runtime_put(dwc->dev);
>>>>>>>>>>>>>            return ret;
>>>>>>>>>>>>> -- 
>>>>>>>>>>>>> 2.40.0
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I think there's one more place that
>>>>>>>>>>>> may needs this check. Can you also
>>>>>>>>>>>> add this check in __dwc3_set_mode()?
>>>>>>>>>>>
>>>>>>>>>>> Hi Thinh,
>>>>>>>>>>>
>>>>>>>>>>>        Sure. Will do it.
>>>>>>>>>>> Will the below be good enough ? Or would
>>>>>>>>>>> it be good to add an error/warn log
>>>>>>>>>>> there>
>>>>>>>>>>
>>>>>>>>>> There's already a warning message in
>>>>>>>>>> dwc3_core_soft_reset() if it fails.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> kriskura@hu-kriskura-hyd:/local/mnt/workspace/krishna/skales2/skales/kernel$
>>>>>>>>>>> git diff drivers/usb/
>>>>>>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>>>>>>>> index 476b63618511..8d1d213d1dcd 100644
>>>>>>>>>>> --- a/drivers/usb/dwc3/core.c
>>>>>>>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>>>>>>>> @@ -210,7 +210,9 @@ static void
>>>>>>>>>>> __dwc3_set_mode(struct work_struct
>>>>>>>>>>> *work)
>>>>>>>>>>>                      }
>>>>>>>>>>>                      break;
>>>>>>>>>>>              case DWC3_GCTL_PRTCAP_DEVICE:
>>>>>>>>>>> -               dwc3_core_soft_reset(dwc);
>>>>>>>>>>> +               ret = dwc3_core_soft_reset(dwc);
>>>>>>>>>>> +               if (ret)
>>>>>>>>>>> +                       goto out;
>>>>>>>>>>>
>>>>>>>>>>>                      dwc3_event_buffers_setup(dwc);
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> If soft-reset failed, the controller is in a
>>>>>>>>>> bad state. We should not
>>>>>>>>>> perform any further operation until the next
>>>>>>>>>> hard reset. We should flag
>>>>>>>>>> the controller as dead. I don't think we have the equivalent of the
>>>>>>>>>> host's HCD_FLAG_DEAD. It may require some
>>>>>>>>>> work in the UDC core. Perhaps
>>>>>>>>>> we can flag within dwc3 for now and prevent
>>>>>>>>>> any further operation for a
>>>>>>>>>> simpler fix.
>>>>>>>>>>
>>>>>>>>> Hi Thinh,
>>>>>>>>>
>>>>>>>>>      Are you referring that if __dwc3_set_mode
>>>>>>>>> failed with core soft reset
>>>>>>>>> timing out, the caller i.e., dwc3_set_mode who
>>>>>>>>> queues the work need to know
>>>>>>>>> that the operation actually failed. So we can
>>>>>>>>> add a flag to indicate that
>>>>>>>>> gadget is dead and the caller of dwc3_set_mode
>>>>>>>>> can check the flag to see if
>>>>>>>>> the operation is successful or not.
>>>>>>>>>
>>>>>>>>> Or am I misunderstanding your comment ?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Not just in __dwc3_set_mode(). I mean any time dwc3_core_soft_reset
>>>>>>>> fails, then we set this flag. So that it can prevent the user calling
>>>>>>>> any gadget ops causing more crashes/invalid behavior. The
>>>>>>>> dwc->softconnect is already wrong on pullup() on failure.
>>>>>>>>
>>>>>>>> So that we can have a check in different gadget ops. For pullup():
>>>>>>>>
>>>>>>>> static int dwc3_gadget_pullup() {
>>>>>>>>      if (dwc->udc_is_dead) {
>>>>>>>>          dev_err(dev, "reset me. x_x \n");
>>>>>>>>          return;
>>>>>>>>      }
>>>>>>>>
>>>>>>>>      abc();
>>>>>>>> }
>>>>>>>>
>>>>>>>> Perhaps the effort is probably the same if we
>>>>>>>> enhance the UDC core for
>>>>>>>> this? In any case, I'm fine either way.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Thinh
>>>>>>>
>>>>>>> Hi Thinh,
>>>>>>>
>>>>>>>     So you don't want UDC to retry pullup if it fails the
>>>>>>> first time ? As per
>>>>>>> patch-2 of this series, I was trying to propagate the
>>>>>>> EITMEDOUT to UDC so
>>>>>>> that the caller (most probably configfs) can take
>>>>>>> appropriate action as to
>>>>>>> whether it must retry pullup or not.
>>>>>>>
>>>>>>
>>>>>> Now I'm confused. If the soft-reset times out, that means that the
>>>>>> soft-reset (self-clearing) bit isn't cleared. How can we retry if it's
>>>>>> stuck in this state? My impression is that soft-reset would
>>>>>> not complete
>>>>>> at all. Is that not the case for you, or it's simply because we need a
>>>>>> longer soft-reset timeout?
>>>>>>
>>>>>> Thanks,
>>>>>> Thinh
>>>>>
>>>>> Hi Thinh,
>>>>>
>>>>>     Sorry for not being clear. The intention of patch-1 was to
>>>>> ensure we don't
>>>>> start the controller if reset times out and patch-2 was to
>>>>> ensure that UDC
>>>>> is in sync with controller by understanding that gadget_connect
>>>>> has failed
>>>>> and necessary cleanup has to be done in gadget_bind_driver.
>>>>
>>>> That should still be there.
>>>>
>>>>>
>>>>> But usually since the UDC_store is the one that is causing pullup to be
>>>>> called, the error value is propagated back to UDC_store. If it sees a
>>>>> failure, it does a retry to pullup.
>>>>>
>>>>> I didn't check  whether subsequent retries by UDC to pullup are helping
>>>>> clear the reset bit or not. But I thought retrying pullup won't
>>>>> be of any
>>>>> harm.
>>>>
>>>> It's fine to retry. I'm thinking that the controller is in a bad state
>>>> at this point that we can't recover (hopefully that's not the case).
>>>>
>>>>>
>>>>> I now get that my patches are incomplete w.r.t handling the timeout.
>>>>>
>>>>> IIRC one of the following is what you are suggesting we need to do:
>>>>>
>>>>> Option-1:
>>>>> Set a flag when reset times out and clear it upon core_exit /
>>>>> core_init. If
>>>>> the flag is set, block calls to all the gadget_ops in dwc3.
>>>>> Basically even
>>>>> if retry happens from configfs/UDC, we bail out in pullup/udc_start even
>>>>> without trying the requested gadget operation.
>>>>>
>>>>> Option-2:
>>>>> If gadget_connect fails with -ETIMEDOUT in UDC, handle the failure and
>>>>> implement the same flag in UDC and don't even call any gadget_ops.
>>>>>
>>>>
>>> Hi Thinh,
>>>
>>> Thanks for the review.
>>>
>>>> I'm thinking of option-1. For option-2, we can't control if the
>>>> gadget_ops will be called. We only have control how we will respond in
>>>> case they get called again.
>>>>
>>>> But now I'm thinking again, I think it may be ok without adding the
>>>> flag. The UDC core and gadget driver won't do anything else before
>>>> pullup(1) is successful. Calling other gadget_ops should be harmless
>>>> (ie. it won't crash/break the system)?
>>>>
>>> I can give this a try in long run testing (For 7-14 days) and see if
>>> anything else is breaking.
>>>
>>> Most probably we do a composition switch / PIPO in between which can
>>> call usb_gadget_unregister_driver which might invoke a pullup(0)
>>> followed by udc_stop() and like you mentioned must not be a problem.
>>>
>>>> Sorry for the noise, but I think it may be ok without marking the
>>>> controller dead. I wonder if we can confirm my suspiction on retry? I
>>>> believe this is not easy to reproduce on your setup? If not, I think we
>>>> can take your change as is.
>>
>> Hi Thinh,
>>
>>    I got this patch tested on two diff Gen-2 targets for around 10 days and
>> no issues were seen. (No SMMU fault seen on a 10 day run). Let me know of
>> any other concerns that might come up with this patch. Else I can rebase it
>> to get merged.
>>
>> Regards,
>> Krishna,
> 
> Thanks for the tests. So you were able to observe the failure and able
> to recover from it without SMMU fault right?
> 
Hi Thinh,

   Ideally if the issue didn't come up for more than a week straight, I 
believe that we are able to recover from the issue.

Regards,
Krishna,

Re: [PATCH v2 1/2] usb: dwc3: gadget: Bail out in pullup if soft reset timeout happens

[Thinh Nguyen]

On Wed, Apr 26, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 4/26/2023 5:52 AM, Thinh Nguyen wrote:
> > On Tue, Apr 25, 2023, Krishna Kurapati PSSNV wrote:
> > > 
> > > 
> > > On 4/6/2023 7:44 AM, Krishna Kurapati PSSNV wrote:
> > > > 
> > > > 
> > > > On 4/6/2023 6:15 AM, Thinh Nguyen wrote:
> > > > > On Wed, Apr 05, 2023, Krishna Kurapati PSSNV wrote:
> > > > > > 
> > > > > > 
> > > > > > On 4/5/2023 3:13 AM, Thinh Nguyen wrote:
> > > > > > > On Tue, Apr 04, 2023, Krishna Kurapati PSSNV wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On 4/4/2023 5:19 AM, Thinh Nguyen wrote:
> > > > > > > > > On Thu, Mar 30, 2023, Krishna Kurapati PSSNV wrote:
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > On 3/30/2023 5:40 AM, Thinh Nguyen wrote:
> > > > > > > > > > > On Wed, Mar 29, 2023, Krishna Kurapati PSSNV wrote:
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > On 3/29/2023 2:50 AM, Thinh Nguyen wrote:
> > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > 
> > > > > > > > > > > > > On Tue, Mar 28, 2023, Krishna Kurapati wrote:
> > > > > > > > > > > > > > If the core soft reset timeout happens, avoid setting up event
> > > > > > > > > > > > > > buffers and starting gadget as the writes to these registers
> > > > > > > > > > > > > > may not reflect when in reset and setting the run stop bit
> > > > > > > > > > > > > > can lead the controller to access wrong event buffer address
> > > > > > > > > > > > > > resulting in a crash.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > > > > > > > > > > > ---
> > > > > > > > > > > > > >        drivers/usb/dwc3/gadget.c | 5 ++++-
> > > > > > > > > > > > > >        1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > diff --git
> > > > > > > > > > > > > > a/drivers/usb/dwc3/gadget.c
> > > > > > > > > > > > > > b/drivers/usb/dwc3/gadget.c
> > > > > > > > > > > > > > index 3c63fa97a680..f0472801d9a5 100644
> > > > > > > > > > > > > > --- a/drivers/usb/dwc3/gadget.c
> > > > > > > > > > > > > > +++ b/drivers/usb/dwc3/gadget.c
> > > > > > > > > > > > > > @@ -2620,13 +2620,16 @@ static
> > > > > > > > > > > > > > int dwc3_gadget_pullup(struct
> > > > > > > > > > > > > > usb_gadget *g, int is_on)
> > > > > > > > > > > > > >                 *
> > > > > > > > > > > > > > device-initiated disconnect
> > > > > > > > > > > > > > requires a core soft reset
> > > > > > > > > > > > > >                 * (DCTL.CSftRst)
> > > > > > > > > > > > > > before enabling the run/stop
> > > > > > > > > > > > > > bit.
> > > > > > > > > > > > > >                 */
> > > > > > > > > > > > > > -        dwc3_core_soft_reset(dwc);
> > > > > > > > > > > > > > +        ret = dwc3_core_soft_reset(dwc);
> > > > > > > > > > > > > > +        if (ret)
> > > > > > > > > > > > > > +            goto done;
> > > > > > > > > > > > > >                dwc3_event_buffers_setup(dwc);
> > > > > > > > > > > > > >                __dwc3_gadget_start(dwc);
> > > > > > > > > > > > > >                ret = dwc3_gadget_run_stop(dwc, true, false);
> > > > > > > > > > > > > >            }
> > > > > > > > > > > > > > +done:
> > > > > > > > > > > > > >            pm_runtime_put(dwc->dev);
> > > > > > > > > > > > > >            return ret;
> > > > > > > > > > > > > > -- 
> > > > > > > > > > > > > > 2.40.0
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I think there's one more place that
> > > > > > > > > > > > > may needs this check. Can you also
> > > > > > > > > > > > > add this check in __dwc3_set_mode()?
> > > > > > > > > > > > 
> > > > > > > > > > > > Hi Thinh,
> > > > > > > > > > > > 
> > > > > > > > > > > >        Sure. Will do it.
> > > > > > > > > > > > Will the below be good enough ? Or would
> > > > > > > > > > > > it be good to add an error/warn log
> > > > > > > > > > > > there>
> > > > > > > > > > > 
> > > > > > > > > > > There's already a warning message in
> > > > > > > > > > > dwc3_core_soft_reset() if it fails.
> > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > kriskura@hu-kriskura-hyd:/local/mnt/workspace/krishna/skales2/skales/kernel$
> > > > > > > > > > > > git diff drivers/usb/
> > > > > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > > > > > > > > > index 476b63618511..8d1d213d1dcd 100644
> > > > > > > > > > > > --- a/drivers/usb/dwc3/core.c
> > > > > > > > > > > > +++ b/drivers/usb/dwc3/core.c
> > > > > > > > > > > > @@ -210,7 +210,9 @@ static void
> > > > > > > > > > > > __dwc3_set_mode(struct work_struct
> > > > > > > > > > > > *work)
> > > > > > > > > > > >                      }
> > > > > > > > > > > >                      break;
> > > > > > > > > > > >              case DWC3_GCTL_PRTCAP_DEVICE:
> > > > > > > > > > > > -               dwc3_core_soft_reset(dwc);
> > > > > > > > > > > > +               ret = dwc3_core_soft_reset(dwc);
> > > > > > > > > > > > +               if (ret)
> > > > > > > > > > > > +                       goto out;
> > > > > > > > > > > > 
> > > > > > > > > > > >                      dwc3_event_buffers_setup(dwc);
> > > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > If soft-reset failed, the controller is in a
> > > > > > > > > > > bad state. We should not
> > > > > > > > > > > perform any further operation until the next
> > > > > > > > > > > hard reset. We should flag
> > > > > > > > > > > the controller as dead. I don't think we have the equivalent of the
> > > > > > > > > > > host's HCD_FLAG_DEAD. It may require some
> > > > > > > > > > > work in the UDC core. Perhaps
> > > > > > > > > > > we can flag within dwc3 for now and prevent
> > > > > > > > > > > any further operation for a
> > > > > > > > > > > simpler fix.
> > > > > > > > > > > 
> > > > > > > > > > Hi Thinh,
> > > > > > > > > > 
> > > > > > > > > >      Are you referring that if __dwc3_set_mode
> > > > > > > > > > failed with core soft reset
> > > > > > > > > > timing out, the caller i.e., dwc3_set_mode who
> > > > > > > > > > queues the work need to know
> > > > > > > > > > that the operation actually failed. So we can
> > > > > > > > > > add a flag to indicate that
> > > > > > > > > > gadget is dead and the caller of dwc3_set_mode
> > > > > > > > > > can check the flag to see if
> > > > > > > > > > the operation is successful or not.
> > > > > > > > > > 
> > > > > > > > > > Or am I misunderstanding your comment ?
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Not just in __dwc3_set_mode(). I mean any time dwc3_core_soft_reset
> > > > > > > > > fails, then we set this flag. So that it can prevent the user calling
> > > > > > > > > any gadget ops causing more crashes/invalid behavior. The
> > > > > > > > > dwc->softconnect is already wrong on pullup() on failure.
> > > > > > > > > 
> > > > > > > > > So that we can have a check in different gadget ops. For pullup():
> > > > > > > > > 
> > > > > > > > > static int dwc3_gadget_pullup() {
> > > > > > > > >      if (dwc->udc_is_dead) {
> > > > > > > > >          dev_err(dev, "reset me. x_x \n");
> > > > > > > > >          return;
> > > > > > > > >      }
> > > > > > > > > 
> > > > > > > > >      abc();
> > > > > > > > > }
> > > > > > > > > 
> > > > > > > > > Perhaps the effort is probably the same if we
> > > > > > > > > enhance the UDC core for
> > > > > > > > > this? In any case, I'm fine either way.
> > > > > > > > > 
> > > > > > > > > Thanks,
> > > > > > > > > Thinh
> > > > > > > > 
> > > > > > > > Hi Thinh,
> > > > > > > > 
> > > > > > > >     So you don't want UDC to retry pullup if it fails the
> > > > > > > > first time ? As per
> > > > > > > > patch-2 of this series, I was trying to propagate the
> > > > > > > > EITMEDOUT to UDC so
> > > > > > > > that the caller (most probably configfs) can take
> > > > > > > > appropriate action as to
> > > > > > > > whether it must retry pullup or not.
> > > > > > > > 
> > > > > > > 
> > > > > > > Now I'm confused. If the soft-reset times out, that means that the
> > > > > > > soft-reset (self-clearing) bit isn't cleared. How can we retry if it's
> > > > > > > stuck in this state? My impression is that soft-reset would
> > > > > > > not complete
> > > > > > > at all. Is that not the case for you, or it's simply because we need a
> > > > > > > longer soft-reset timeout?
> > > > > > > 
> > > > > > > Thanks,
> > > > > > > Thinh
> > > > > > 
> > > > > > Hi Thinh,
> > > > > > 
> > > > > >     Sorry for not being clear. The intention of patch-1 was to
> > > > > > ensure we don't
> > > > > > start the controller if reset times out and patch-2 was to
> > > > > > ensure that UDC
> > > > > > is in sync with controller by understanding that gadget_connect
> > > > > > has failed
> > > > > > and necessary cleanup has to be done in gadget_bind_driver.
> > > > > 
> > > > > That should still be there.
> > > > > 
> > > > > > 
> > > > > > But usually since the UDC_store is the one that is causing pullup to be
> > > > > > called, the error value is propagated back to UDC_store. If it sees a
> > > > > > failure, it does a retry to pullup.
> > > > > > 
> > > > > > I didn't check  whether subsequent retries by UDC to pullup are helping
> > > > > > clear the reset bit or not. But I thought retrying pullup won't
> > > > > > be of any
> > > > > > harm.
> > > > > 
> > > > > It's fine to retry. I'm thinking that the controller is in a bad state
> > > > > at this point that we can't recover (hopefully that's not the case).
> > > > > 
> > > > > > 
> > > > > > I now get that my patches are incomplete w.r.t handling the timeout.
> > > > > > 
> > > > > > IIRC one of the following is what you are suggesting we need to do:
> > > > > > 
> > > > > > Option-1:
> > > > > > Set a flag when reset times out and clear it upon core_exit /
> > > > > > core_init. If
> > > > > > the flag is set, block calls to all the gadget_ops in dwc3.
> > > > > > Basically even
> > > > > > if retry happens from configfs/UDC, we bail out in pullup/udc_start even
> > > > > > without trying the requested gadget operation.
> > > > > > 
> > > > > > Option-2:
> > > > > > If gadget_connect fails with -ETIMEDOUT in UDC, handle the failure and
> > > > > > implement the same flag in UDC and don't even call any gadget_ops.
> > > > > > 
> > > > > 
> > > > Hi Thinh,
> > > > 
> > > > Thanks for the review.
> > > > 
> > > > > I'm thinking of option-1. For option-2, we can't control if the
> > > > > gadget_ops will be called. We only have control how we will respond in
> > > > > case they get called again.
> > > > > 
> > > > > But now I'm thinking again, I think it may be ok without adding the
> > > > > flag. The UDC core and gadget driver won't do anything else before
> > > > > pullup(1) is successful. Calling other gadget_ops should be harmless
> > > > > (ie. it won't crash/break the system)?
> > > > > 
> > > > I can give this a try in long run testing (For 7-14 days) and see if
> > > > anything else is breaking.
> > > > 
> > > > Most probably we do a composition switch / PIPO in between which can
> > > > call usb_gadget_unregister_driver which might invoke a pullup(0)
> > > > followed by udc_stop() and like you mentioned must not be a problem.
> > > > 
> > > > > Sorry for the noise, but I think it may be ok without marking the
> > > > > controller dead. I wonder if we can confirm my suspiction on retry? I
> > > > > believe this is not easy to reproduce on your setup? If not, I think we
> > > > > can take your change as is.
> > > 
> > > Hi Thinh,
> > > 
> > >    I got this patch tested on two diff Gen-2 targets for around 10 days and
> > > no issues were seen. (No SMMU fault seen on a 10 day run). Let me know of
> > > any other concerns that might come up with this patch. Else I can rebase it
> > > to get merged.
> > > 
> > > Regards,
> > > Krishna,
> > 
> > Thanks for the tests. So you were able to observe the failure and able
> > to recover from it without SMMU fault right?
> > 
> Hi Thinh,
> 
>   Ideally if the issue didn't come up for more than a week straight, I
> believe that we are able to recover from the issue.
> 
> Regards,
> Krishna,

Hi Krishna,

I wanted to confirm whether the controller is dead and whether any
attempt to access the controller at this point can be fatal to the
system (such as SMMU fault). I believe your patches prevents SMMU fault
on first failure. I'm not sure what will happen after if we attempt a
retry.

Regardless, your changes are good for now. If we need to make further
enhancement in the future, we'll do that later.

Many thanks for the tests,
Thinh

Re: [PATCH v2 1/2] usb: dwc3: gadget: Bail out in pullup if soft reset timeout happens

[Thinh Nguyen]

On Tue, Mar 28, 2023, Krishna Kurapati wrote:
> If the core soft reset timeout happens, avoid setting up event
> buffers and starting gadget as the writes to these registers
> may not reflect when in reset and setting the run stop bit
> can lead the controller to access wrong event buffer address
> resulting in a crash.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  drivers/usb/dwc3/gadget.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3c63fa97a680..f0472801d9a5 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2620,13 +2620,16 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>  		 * device-initiated disconnect requires a core soft reset
>  		 * (DCTL.CSftRst) before enabling the run/stop bit.
>  		 */
> -		dwc3_core_soft_reset(dwc);
> +		ret = dwc3_core_soft_reset(dwc);
> +		if (ret)
> +			goto done;
>  
>  		dwc3_event_buffers_setup(dwc);
>  		__dwc3_gadget_start(dwc);
>  		ret = dwc3_gadget_run_stop(dwc, true, false);
>  	}
>  
> +done:
>  	pm_runtime_put(dwc->dev);
>  
>  	return ret;
> -- 
> 2.40.0
> 

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh

[PATCH v2 2/2] usb: gadget: udc: Handle gadget_connect failure during bind operation

[Krishna Kurapati]

In the event, gadget_connect call (which invokes pullup) fails,
propagate the error to udc bind operation which inturn sends the
error to configfs. The userspace can then retry enumeartion if
it chooses to.

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 drivers/usb/gadget/udc/core.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 23b0629a8774..975205a1853f 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1051,12 +1051,16 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state);
 
 /* ------------------------------------------------------------------------- */
 
-static void usb_udc_connect_control(struct usb_udc *udc)
+static int usb_udc_connect_control(struct usb_udc *udc)
 {
+	int ret;
+
 	if (udc->vbus)
-		usb_gadget_connect(udc->gadget);
+		ret = usb_gadget_connect(udc->gadget);
 	else
-		usb_gadget_disconnect(udc->gadget);
+		ret = usb_gadget_disconnect(udc->gadget);
+
+	return ret;
 }
 
 /**
@@ -1500,11 +1504,19 @@ static int gadget_bind_driver(struct device *dev)
 	if (ret)
 		goto err_start;
 	usb_gadget_enable_async_callbacks(udc);
-	usb_udc_connect_control(udc);
+	ret = usb_udc_connect_control(udc);
+	if (ret)
+		goto err_connect_control;
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 	return 0;
 
+ err_connect_control:
+	usb_gadget_disable_async_callbacks(udc);
+	if (gadget->irq)
+		synchronize_irq(gadget->irq);
+	usb_gadget_udc_stop(udc);
+
  err_start:
 	driver->unbind(udc->gadget);
 
-- 
2.40.0

Re: [PATCH v2 2/2] usb: gadget: udc: Handle gadget_connect failure during bind operation

[Krishna Kurapati PSSNV]

Hi Alan, Geert,

  Can you help review and provide comments/approval on the following patch.

Regards,
Krishna,

On 3/28/2023 9:37 PM, Krishna Kurapati wrote:
> In the event, gadget_connect call (which invokes pullup) fails,
> propagate the error to udc bind operation which inturn sends the
> error to configfs. The userspace can then retry enumeartion if
> it chooses to.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>   drivers/usb/gadget/udc/core.c | 20 ++++++++++++++++----
>   1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index 23b0629a8774..975205a1853f 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -1051,12 +1051,16 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state);
>   
>   /* ------------------------------------------------------------------------- */
>   
> -static void usb_udc_connect_control(struct usb_udc *udc)
> +static int usb_udc_connect_control(struct usb_udc *udc)
>   {
> +	int ret;
> +
>   	if (udc->vbus)
> -		usb_gadget_connect(udc->gadget);
> +		ret = usb_gadget_connect(udc->gadget);
>   	else
> -		usb_gadget_disconnect(udc->gadget);
> +		ret = usb_gadget_disconnect(udc->gadget);
> +
> +	return ret;
>   }
>   
>   /**
> @@ -1500,11 +1504,19 @@ static int gadget_bind_driver(struct device *dev)
>   	if (ret)
>   		goto err_start;
>   	usb_gadget_enable_async_callbacks(udc);
> -	usb_udc_connect_control(udc);
> +	ret = usb_udc_connect_control(udc);
> +	if (ret)
> +		goto err_connect_control;
>   
>   	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>   	return 0;
>   
> + err_connect_control:
> +	usb_gadget_disable_async_callbacks(udc);
> +	if (gadget->irq)
> +		synchronize_irq(gadget->irq);
> +	usb_gadget_udc_stop(udc);
> +
>    err_start:
>   	driver->unbind(udc->gadget);
>   

Re: [PATCH v2 2/2] usb: gadget: udc: Handle gadget_connect failure during bind operation

[Geert Uytterhoeven]

Hi Krishna,

On Wed, Apr 26, 2023 at 3:17 AM Krishna Kurapati PSSNV
<quic_kriskura@quicinc.com> wrote:
> Hi Alan, Geert,
>
>   Can you help review and provide comments/approval on the following patch.

I don't know why you are addressing me, as I never touched the affected
file, am not listed as its maintainer, and don't know much about USB UDC.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

get_maintainer.pl wrong and undeterministic? (was: Re: [PATCH v2 2/2] usb: gadget: udc: Handle gadget_connect failure during bind operation)

[Geert Uytterhoeven]

Hi Krishna,

CC Joe and lkml

On Thu, Apr 27, 2023 at 10:44 AM Krishna Kurapati PSSNV
<quic_kriskura@quicinc.com> wrote:
> On 4/27/2023 1:23 PM, Geert Uytterhoeven wrote:
> > On Thu, Apr 27, 2023 at 5:49 AM Krishna Kurapati PSSNV
> > <quic_kriskura@quicinc.com> wrote:
> >> On 4/26/2023 2:48 PM, Geert Uytterhoeven wrote:
> >>> On Wed, Apr 26, 2023 at 3:17 AM Krishna Kurapati PSSNV
> >>> <quic_kriskura@quicinc.com> wrote:
> >>>> Hi Alan, Geert,
> >>>>
> >>>>     Can you help review and provide comments/approval on the following patch.
> >>>
> >>> I don't know why you are addressing me, as I never touched the affected
> >>> file, am not listed as its maintainer, and don't know much about USB UDC.
> >
> >>    Apologies. I must have caused some confusion because of same name. I
> >> must have specified clearly whom I was referring to.
> >>
> >> I CC'd and was referring to Geert Uytterhoeven <geert+renesas@glider.be>
> >> for comments.
> >
> > That's actually me, too ;-)
> >
> >> As per the output of get_maintainer.pl
> >>
> >> ./scripts/get_maintainer.pl drivers/usb/gadget/udc/core.c
> >>
> >> Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB
> >> SUBSYSTEM,commit_signer:6/6=100%,authored:1/6=17%,removed_lines:2/26=8%)
> >> Alan Stern <stern@rowland.harvard.edu>
> >> (commit_signer:4/6=67%,authored:3/6=50%,added_lines:36/45=80%,removed_lines:17/26=65%)
> >> TaoXue <xuetao09@huawei.com> (commit_signer:1/6=17%)
> >> "Rafael J. Wysocki" <rafael@kernel.org> (commit_signer:1/6=17%)
> >> Geert Uytterhoeven <geert+renesas@glider.be> (commit_signer:1/6=17%)
> >> Colin Ian King <colin.i.king@gmail.com> (authored:1/6=17%)
> >> Jiantao Zhang <water.zhangjiantao@huawei.com>
> >> (authored:1/6=17%,added_lines:6/45=13%,removed_lines:6/26=23%)
> >
> > Interesting, I don't see me listed when running that command (on v6.3 and
> > next-20230425), and I never authored any change to that file.
> > What is the tree (commit sha1) you are running ./scripts/get_maintainer.pl on?
>
> I checked it on linux-next a couple of weeks back and it showed me this.
> But when I synced latest linux kernel, it didn't show it today 😅
> Not sure, what is the diff here.

Interesting:

    $ git checkout next-20230425
    Updating files: 100% (7386/7386), done.
    Previous HEAD position was 198925fae644b009 Add linux-next
specific files for 20230329
    HEAD is now at f600e0bbde8562a0 Add linux-next specific files for 20230425
    $ scripts/get_maintainer.pl drivers/usb/gadget/udc/core.c
    Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB
SUBSYSTEM,commit_signer:11/11=100%,authored:3/11=27%,removed_lines:5/73=7%)
    Alan Stern <stern@rowland.harvard.edu>
(commit_signer:4/11=36%,authored:3/11=27%,added_lines:36/182=20%,removed_lines:17/73=23%)
    Badhri Jagan Sridharan <badhri@google.com>
(commit_signer:2/11=18%,authored:2/11=18%,added_lines:107/182=59%,removed_lines:44/73=60%)
    Elson Roy Serrao <quic_eserrao@quicinc.com>
(commit_signer:1/11=9%,added_lines:27/182=15%)
    Sebastian Reichel <sre@kernel.org> (commit_signer:1/11=9%)
    Colin Ian King <colin.i.king@gmail.com> (authored:1/11=9%)
    Jiantao Zhang <water.zhangjiantao@huawei.com>
(authored:1/11=9%,removed_lines:6/73=8%)
    linux-usb@vger.kernel.org (open list:USB SUBSYSTEM)
    linux-kernel@vger.kernel.org (open list)
    $ git checkout next-20230329
    Updating files: 100% (7386/7386), done.
    Previous HEAD position was f600e0bbde8562a0 Add linux-next
specific files for 20230425
    HEAD is now at 198925fae644b009 Add linux-next specific files for 20230329
    $ scripts/get_maintainer.pl drivers/usb/gadget/udc/core.c
    Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB
SUBSYSTEM,commit_signer:8/8=100%,authored:3/8=38%,added_lines:5/48=10%,removed_lines:5/29=17%)
    Alan Stern <stern@rowland.harvard.edu>
(commit_signer:3/8=50%,authored:3/8=38%,added_lines:36/48=75%,removed_lines:17/29=59%)
    Geert Uytterhoeven <geert+renesas@glider.be> (commit_signer:1/8=12%)
    Sebastian Reichel <sre@kernel.org> (commit_signer:1/8=12%)
    Heikki Krogerus <heikki.krogerus@linux.intel.com> (commit_signer:1/8=12%)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I, Sebastian, and Heikki never touched this file...

     Jiantao Zhang <water.zhangjiantao@huawei.com>
(authored:1/8=12%,added_lines:6/48=12%,removed_lines:6/29=21%)
    Colin Ian King <colin.i.king@gmail.com> (authored:1/8=12%)
    linux-usb@vger.kernel.org (open list:USB SUBSYSTEM)
    linux-kernel@vger.kernel.org (open list)
    $ scripts/get_maintainer.pl drivers/usb/gadget/udc/core.c
    Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB
SUBSYSTEM,commit_signer:8/8=100%,authored:3/8=38%,added_lines:5/48=10%,removed_lines:5/29=17%)
    Alan Stern <stern@rowland.harvard.edu>
(commit_signer:4/8=50%,authored:3/8=38%,added_lines:36/48=75%,removed_lines:17/29=59%)
    "Rafael J. Wysocki" <rafael@kernel.org> (commit_signer:1/8=12%)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Heikki and I are gone, but Rafael is new and also hasn't touched this file?

    Colin Ian King <colin.i.king@gmail.com>
(commit_signer:1/8=12%,authored:1/8=12%)
    Sebastian Reichel <sre@kernel.org> (commit_signer:1/8=12%)
    Jiantao Zhang <water.zhangjiantao@huawei.com>
(authored:1/8=12%,added_lines:6/48=12%,removed_lines:6/29=21%)
    linux-usb@vger.kernel.org (open list:USB SUBSYSTEM)
    linux-kernel@vger.kernel.org (open list)

You can see the differences when running the following multiple times:

    $ diff <(scripts/get_maintainer.pl drivers/usb/gadget/udc/core.c)
<(scripts/get_maintainer.pl drivers/usb/gadget/udc/core.c)

Looks like scripts/get_maintainer.pl (a) shows wrong committers (they
did provide other e.g. Reviewed-by tags), and (b) is not deterministic?

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: get_maintainer.pl wrong and undeterministic?

[Andreas Schwab]

On Apr 27 2023, Geert Uytterhoeven wrote:

>     $ scripts/get_maintainer.pl drivers/usb/gadget/udc/core.c
>     Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB
> SUBSYSTEM,commit_signer:8/8=100%,authored:3/8=38%,added_lines:5/48=10%,removed_lines:5/29=17%)
>     Alan Stern <stern@rowland.harvard.edu>
> (commit_signer:3/8=50%,authored:3/8=38%,added_lines:36/48=75%,removed_lines:17/29=59%)
>     Geert Uytterhoeven <geert+renesas@glider.be> (commit_signer:1/8=12%)
>     Sebastian Reichel <sre@kernel.org> (commit_signer:1/8=12%)
>     Heikki Krogerus <heikki.krogerus@linux.intel.com> (commit_signer:1/8=12%)
>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> I, Sebastian, and Heikki never touched this file...

If you look at 'git log drivers/usb/gadget/udc/core.c', you see commits
touching scripts/get_maintainer.pl.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: get_maintainer.pl wrong and undeterministic?

[Andreas Schwab]

Please ignore that, typo on my side.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: get_maintainer.pl wrong and undeterministic? (was: Re: [PATCH v2 2/2] usb: gadget: udc: Handle gadget_connect failure during bind operation)

[Joe Perches]

On Thu, 2023-04-27 at 11:09 +0200, Geert Uytterhoeven wrote:
> Hi Krishna,
> 
> CC Joe and lkml

get_maintainer is sometimes non deterministic.

When adding maintainers from git commit logs (and btw, it's
not just nominal maintainers, it's any signers), if the
same number of signature names are read from the commit logs
the selection of the listed entries _is_ random.

see: https://lore.kernel.org/lkml/1499984554.4457.64.camel@perches.com/

> 
> On Thu, Apr 27, 2023 at 10:44 AM Krishna Kurapati PSSNV
> <quic_kriskura@quicinc.com> wrote:
> > On 4/27/2023 1:23 PM, Geert Uytterhoeven wrote:
> > > On Thu, Apr 27, 2023 at 5:49 AM Krishna Kurapati PSSNV
> > > <quic_kriskura@quicinc.com> wrote:
> > > > On 4/26/2023 2:48 PM, Geert Uytterhoeven wrote:
> > > > > On Wed, Apr 26, 2023 at 3:17 AM Krishna Kurapati PSSNV to l
> > > > > <quic_kriskura@quicinc.com> wrote:
> > > > > > Hi Alan, Geert,
> > > > > > 
> > > > > >     Can you help review and provide comments/approval on the following patch.
> > > > > 
> > > > > I don't know why you are addressing me, as I never touched the affected
> > > > > file, am not listed as its maintainer, and don't know much about USB UDC.
> > > 
> > > >    Apologies. I must have caused some confusion because of same name. I
> > > > must have specified clearly whom I was referring to.
> > > > 
> > > > I CC'd and was referring to Geert Uytterhoeven <geert+renesas@glider.be>
> > > > for comments.
> > > 
> > > That's actually me, too ;-)
> > > 
> > > > As per the output of get_maintainer.pl
> > > > 
> > > > ./scripts/get_maintainer.pl drivers/usb/gadget/udc/core.c
> > > > 
> > > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB
> > > > SUBSYSTEM,commit_signer:6/6=100%,authored:1/6=17%,removed_lines:2/26=8%)
> > > > Alan Stern <stern@rowland.harvard.edu>
> > > > (commit_signer:4/6=67%,authored:3/6=50%,added_lines:36/45=80%,removed_lines:17/26=65%)
> > > > TaoXue <xuetao09@huawei.com> (commit_signer:1/6=17%)
> > > > "Rafael J. Wysocki" <rafael@kernel.org> (commit_signer:1/6=17%)
> > > > Geert Uytterhoeven <geert+renesas@glider.be> (commit_signer:1/6=17%)
> > > > Colin Ian King <colin.i.king@gmail.com> (authored:1/6=17%)
> > > > Jiantao Zhang <water.zhangjiantao@huawei.com>
> > > > (authored:1/6=17%,added_lines:6/45=13%,removed_lines:6/26=23%)
> > > 
> > > Interesting, I don't see me listed when running that command (on v6.3 and
> > > next-20230425), and I never authored any change to that file.
> > > What is the tree (commit sha1) you are running ./scripts/get_maintainer.pl on?
> > 
> > I checked it on linux-next a couple of weeks back and it showed me this.
> > But when I synced latest linux kernel, it didn't show it today 😅
> > Not sure, what is the diff here.
> 
> Interesting:
> 
>     $ git checkout next-20230425
>     Updating files: 100% (7386/7386), done.
>     Previous HEAD position was 198925fae644b009 Add linux-next
> specific files for 20230329
>     HEAD is now at f600e0bbde8562a0 Add linux-next specific files for 20230425
>     $ scripts/get_maintainer.pl drivers/usb/gadget/udc/core.c
>     Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB
> SUBSYSTEM,commit_signer:11/11=100%,authored:3/11=27%,removed_lines:5/73=7%)
>     Alan Stern <stern@rowland.harvard.edu>
> (commit_signer:4/11=36%,authored:3/11=27%,added_lines:36/182=20%,removed_lines:17/73=23%)
>     Badhri Jagan Sridharan <badhri@google.com>
> (commit_signer:2/11=18%,authored:2/11=18%,added_lines:107/182=59%,removed_lines:44/73=60%)
>     Elson Roy Serrao <quic_eserrao@quicinc.com>
> (commit_signer:1/11=9%,added_lines:27/182=15%)
>     Sebastian Reichel <sre@kernel.org> (commit_signer:1/11=9%)
>     Colin Ian King <colin.i.king@gmail.com> (authored:1/11=9%)
>     Jiantao Zhang <water.zhangjiantao@huawei.com>
> (authored:1/11=9%,removed_lines:6/73=8%)
>     linux-usb@vger.kernel.org (open list:USB SUBSYSTEM)
>     linux-kernel@vger.kernel.org (open list)
>     $ git checkout next-20230329
>     Updating files: 100% (7386/7386), done.
>     Previous HEAD position was f600e0bbde8562a0 Add linux-next
> specific files for 20230425
>     HEAD is now at 198925fae644b009 Add linux-next specific files for 20230329
>     $ scripts/get_maintainer.pl drivers/usb/gadget/udc/core.c
>     Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB
> SUBSYSTEM,commit_signer:8/8=100%,authored:3/8=38%,added_lines:5/48=10%,removed_lines:5/29=17%)
>     Alan Stern <stern@rowland.harvard.edu>
> (commit_signer:3/8=50%,authored:3/8=38%,added_lines:36/48=75%,removed_lines:17/29=59%)
>     Geert Uytterhoeven <geert+renesas@glider.be> (commit_signer:1/8=12%)
>     Sebastian Reichel <sre@kernel.org> (commit_signer:1/8=12%)
>     Heikki Krogerus <heikki.krogerus@linux.intel.com> (commit_signer:1/8=12%)
>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> I, Sebastian, and Heikki never touched this file...
> 
>      Jiantao Zhang <water.zhangjiantao@huawei.com>
> (authored:1/8=12%,added_lines:6/48=12%,removed_lines:6/29=21%)
>     Colin Ian King <colin.i.king@gmail.com> (authored:1/8=12%)
>     linux-usb@vger.kernel.org (open list:USB SUBSYSTEM)
>     linux-kernel@vger.kernel.org (open list)
>     $ scripts/get_maintainer.pl drivers/usb/gadget/udc/core.c
>     Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB
> SUBSYSTEM,commit_signer:8/8=100%,authored:3/8=38%,added_lines:5/48=10%,removed_lines:5/29=17%)
>     Alan Stern <stern@rowland.harvard.edu>
> (commit_signer:4/8=50%,authored:3/8=38%,added_lines:36/48=75%,removed_lines:17/29=59%)
>     "Rafael J. Wysocki" <rafael@kernel.org> (commit_signer:1/8=12%)
>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Heikki and I are gone, but Rafael is new and also hasn't touched this file?
> 
>     Colin Ian King <colin.i.king@gmail.com>
> (commit_signer:1/8=12%,authored:1/8=12%)
>     Sebastian Reichel <sre@kernel.org> (commit_signer:1/8=12%)
>     Jiantao Zhang <water.zhangjiantao@huawei.com>
> (authored:1/8=12%,added_lines:6/48=12%,removed_lines:6/29=21%)
>     linux-usb@vger.kernel.org (open list:USB SUBSYSTEM)
>     linux-kernel@vger.kernel.org (open list)
> 
> You can see the differences when running the following multiple times:
> 
>     $ diff <(scripts/get_maintainer.pl drivers/usb/gadget/udc/core.c)
> <(scripts/get_maintainer.pl drivers/usb/gadget/udc/core.c)
> 
> Looks like scripts/get_maintainer.pl (a) shows wrong committers (they
> did provide other e.g. Reviewed-by tags), and (b) is not deterministic?
> 
> Thanks!
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH v2 2/2] usb: gadget: udc: Handle gadget_connect failure during bind operation

[Alan Stern]

On Wed, Apr 26, 2023 at 06:47:13AM +0530, Krishna Kurapati PSSNV wrote:
> Hi Alan, Geert,
> 
>  Can you help review and provide comments/approval on the following patch.
> 
> Regards,
> Krishna,

Sorry this took so long to review; I've been quite busy.

The patch is good except for one stylistic thing...

> On 3/28/2023 9:37 PM, Krishna Kurapati wrote:
> > In the event, gadget_connect call (which invokes pullup) fails,
> > propagate the error to udc bind operation which inturn sends the
> > error to configfs. The userspace can then retry enumeartion if
> > it chooses to.
> > 
> > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > ---
> >   drivers/usb/gadget/udc/core.c | 20 ++++++++++++++++----
> >   1 file changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > index 23b0629a8774..975205a1853f 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -1051,12 +1051,16 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state);
> >   /* ------------------------------------------------------------------------- */
> > -static void usb_udc_connect_control(struct usb_udc *udc)
> > +static int usb_udc_connect_control(struct usb_udc *udc)
> >   {
> > +	int ret;
> > +
> >   	if (udc->vbus)
> > -		usb_gadget_connect(udc->gadget);
> > +		ret = usb_gadget_connect(udc->gadget);
> >   	else
> > -		usb_gadget_disconnect(udc->gadget);
> > +		ret = usb_gadget_disconnect(udc->gadget);
> > +
> > +	return ret;
> >   }
> >   /**
> > @@ -1500,11 +1504,19 @@ static int gadget_bind_driver(struct device *dev)
> >   	if (ret)
> >   		goto err_start;
> >   	usb_gadget_enable_async_callbacks(udc);
> > -	usb_udc_connect_control(udc);
> > +	ret = usb_udc_connect_control(udc);
> > +	if (ret)
> > +		goto err_connect_control;
> >   	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
> >   	return 0;
> > + err_connect_control:

For consistency with the other error-handling statement labels in this 
routine, there should be a blank line immediately before this label.

> > +	usb_gadget_disable_async_callbacks(udc);
> > +	if (gadget->irq)
> > +		synchronize_irq(gadget->irq);
> > +	usb_gadget_udc_stop(udc);
> > +
> >    err_start:
> >   	driver->unbind(udc->gadget);

Everything else looks okay.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern