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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <54b8ce81-f308-d3dc-c1b5-357b62594c6c@synopsys.com>
Date:   Fri, 16 Apr 2021 07:05:31 +0000
From:   Artur Petrosyan <Arthur.Petrosyan@...opsys.com>
To:     Artur Petrosyan <Arthur.Petrosyan@...opsys.com>,
        Sergei Shtylyov <sergei.shtylyov@...il.com>,
        Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Minas Harutyunyan <Minas.Harutyunyan@...opsys.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     John Youn <John.Youn@...opsys.com>
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 <Arthur.Petrosyan@...opsys.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.
> 

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
>