linux-kernel - Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set

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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 05 Mar 2018 10:49:00 +0200
From:   Felipe Balbi <balbi@...nel.org>
To:     Roger Quadros <rogerq@...com>
Cc:     linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume


Hi,

Roger Quadros <rogerq@...com> writes:
>> Roger Quadros <rogerq@...com> writes:
>>> In the following test we get stuck by sleeping forever in _dwc3_set_mode()
>>> after which dual-role switching doesn't work.
>>>
>>> On dra7-evm's dual-role port,
>>> - Load g_zero gadget driver and enumerate to host
>>> - suspend to mem
>>> - disconnect USB cable to host and connect otg cable with Pen drive in it.
>>> - resume system
>>> - we sleep indefinitely in _dwc3_set_mode due to.
>>>   dwc3_gadget_exit()->usb_del_gadget_udc()->udc_stop()->
>>> 	dwc3_gadget_stop()->wait_event_lock_irq()
>>>
>>> Let's clear the DWC3_EP_END_TRANSFER_PENDING flag on all endpoints
>>> so we don't wait in dwc3_gadget_stop().
>>>
>>> Signed-off-by: Roger Quadros <rogerq@...com>
>>> ---
>>>  drivers/usb/dwc3/gadget.c | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 2bda4eb..0a360da 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -3273,6 +3273,20 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>>>  
>>>  void dwc3_gadget_exit(struct dwc3 *dwc)
>>>  {
>>> +	int epnum;
>>> +	unsigned long flags;
>>> +
>>> +	spin_lock_irqsave(&dwc->lock, flags);
>>> +	for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
>>> +		struct dwc3_ep  *dep = dwc->eps[epnum];
>>> +
>>> +		if (!dep)
>>> +			continue;
>>> +
>>> +		dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
>>> +	}
>>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>>> +
>>>  	usb_del_gadget_udc(&dwc->gadget);
>>>  	dwc3_gadget_free_endpoints(dwc);
>> 
>> free endpoints is a better place for this. It's already going to free
>> the memory anyway. Might as well clear all flags to 0 there.
>> 
>
> But it won't solve the deadlock issue. Since dwc3_gadget_free_endpoints()
> is called after usb_del_gadget_udc() and the deadlock happens when
>
> usb_del_gadget_udc()->udc_stop()->dwc3_gadget_stop()->wait_event_lock_irq()
>
> and DWC3_EP_END_TRANSFER_PENDING flag is set.

indeed. Iterating twice over the entire endpoint list seems
wasteful. Perhaps we just shouldn't wait when removing the UDC since
that's essentially what this patch will do, right? If you clear the flag
before calling ->udc_stop(), this means the loop in dwc3_gadget_stop()
will do nothing. Might as well remove it.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (833 bytes)