Re: [PATCH 10/15] usb: dwc2: Allow exit hibernation in urb enqueue

[Artur Petrosyan]

Hi Sergei,

On 4/15/2021 13:12, Sergei Shtylyov wrote:
> On 15.04.2021 8:40, Artur Petrosyan wrote:
> 
>> When core is in hibernation state and an external
>> hub is connected, upper layer sends URB enqueue request,
>> which results in port reset issue.
>>
>> - Added exit from hibernation state to avoid port
>> reset issue and process upper layer request properly.
>>
>> Signed-off-by: Artur Petrosyan <Arthur.Petrosyan@xxxxxxxxxxxx>
>> ---
>>    drivers/usb/dwc2/hcd.c | 17 +++++++++++++++++
>>    1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index cc9ad6cf02d9..3b03b2d73aaa 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -4631,12 +4631,29 @@ static int _dwc2_hcd_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
>>    	struct dwc2_qh *qh;
>>    	bool qh_allocated = false;
>>    	struct dwc2_qtd *qtd;
>> +	struct dwc2_gregs_backup *gr;
>> +
>> +	gr = &hsotg->gr_backup;
>>    
>>    	if (dbg_urb(urb)) {
>>    		dev_vdbg(hsotg->dev, "DWC OTG HCD URB Enqueue\n");
>>    		dwc2_dump_urb_info(hcd, urb, "urb_enqueue");
>>    	}
>>    
>> +	if (hsotg->hibernated) {
>> +		if (gr->gotgctl & GOTGCTL_CURMODE_HOST) {
>> +			retval = dwc2_exit_hibernation(hsotg, 0, 0, 1);
>> +			if (retval)
>> +				dev_err(hsotg->dev,
>> +					"exit hibernation failed.\n");
>> +		} else {
>> +			retval = dwc2_exit_hibernation(hsotg, 0, 0, 0);
>> +			if (retval)
>> +				dev_err(hsotg->dev,
>> +					"exit hibernation failed.\n");
> 
>      Why not put these identical *if*s outside the the outer *if*?
> 
Well the reason that the conditions are implemented like they are, is 
that the inner if checks whether core operates in host mode or device 
mode and the outside if checks if the core is hibernated or not.

So now imagine that the ifs are combined and core is not hibernated but 
the operational mode of the driver is let's say gadget. If the case is 
such then the driver will try to exit from gadget hibernation because of 
the else condition as the if condition will be false again because core 
is not hibernated. As a result if we combine the outside and inner ifs 
then it will try to exit from gadget hibernation but core is not 
hibernated and that would be an issue.


> 
>> +		}
>> +	}
>> +
>>    	if (hsotg->in_ppd) {
>>    		retval = dwc2_exit_partial_power_down(hsotg, 0, true);
>>    		if (retval)
> 
> MBR, Sergei
> 

Regards,
Artur