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

[Artur Petrosyan]

Hi Sergei,

On 4/16/2021 09:43, Artur Petrosyan wrote:
> 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.
> 

Sorry I got your point wrong there. You meant the ifs for dev_err().
Thank you for the review I will update them.

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