2021-04-15 05:41:58

by Artur Petrosyan

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

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 <[email protected]>
---
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");
+ }
+ }
+
if (hsotg->in_ppd) {
retval = dwc2_exit_partial_power_down(hsotg, 0, true);
if (retval)
--
2.25.1

2021-04-15 09:13:44

by Sergei Shtylyov

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

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 <[email protected]>
> ---
> 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*?


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

MBR, Sergei

2021-04-16 06:02:12

by Artur Petrosyan

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

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 <[email protected]>
>> ---
>> 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

2021-04-16 08:05:55

by Artur Petrosyan

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

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 <[email protected]>
>>> ---
>>> 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
>