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

Artur Petrosyan
2021-04-16T05:43:31Z

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@synopsys.com>
>> ---
>> 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