Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Felipe Balbi <balbi@kernel.org>
To:     Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>,
        Roger Quadros <rogerq@ti.com>
Cc:     "linux-usb\@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume
In-Reply-To: <410670D7E743164D87FA6160E7907A560113ABB478@am04wembxa.internal.synopsys.com>
References: <1519730526-22274-1-git-send-email-rogerq@ti.com> <a04fafee-7992-ce8b-55a9-afd4c9935462@ti.com> <69517684-bd39-e945-0c9e-ccd52b8050d5@ti.com> <87y3isffog.fsf@linux.intel.com> <5ea0ad17-d538-72ec-ed59-004242c4cd26@ti.com> <410670D7E743164D87FA6160E7907A560113ABB478@am04wembxa.internal.synopsys.com>
Date:   Fri, 16 Mar 2018 14:25:02 +0200
Message-ID: <87zi38438h.fsf@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


Hi,

Minas Harutyunyan <Minas.Harutyunyan@synopsys.com> writes:
>>>> On 09/03/18 14:47, Roger Quadros wrote:
>>>>> 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()
>>>>>
>>>>> To fix this instead of waiting indefinitely with wait_event_lock_irq()
>>>>> we use wait_event_interruptible_lock_irq_timeout() and print
>>>>> and error message if there was a timeout.
>>>>>
>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>
>>>> Thanks for picking this for -next.
>>>> Is it better to have this in v4.16-rc fixes?
>>>> and also stable? v4.12+
>>>
>>> Well, there was no "Fixes: foobar" or "Cc: stable" lines in the commit
>>> log ;-)
>>>
>>> The best we can do now, is wait for -rc1 and manually send the commit to
>>> stable.
>>>
>> 
>> That's fine. Thanks.
>> 
>
> Same issue seen in dwc3_gadget_ep_dequeue() function where also used 
> wait_event_lock_irq() - as result infinite loop.

how did this happen? During rmmod dwc3? Or, perhaps, after you unloaded
a gadget driver?

> Actually to fix this issue I updated condition of wait function
> from:
> !(dep->flags & DWC3_EP_END_TRANSFER_PENDING)
> to:
> !(dep->flags & DWC3_EP_END_TRANSFER_PENDING & DWC3_EP_ENABLED)

you're not fixing anything. You're, essentially, removing the entire
end transfer pending logic. The whole idea of this is that we can
disable the endpoint and wait for the End Transfer interrupt. When you
add a check for the endpoint being enabled, then that code will never
run and, thus, never wait for the End Transfer IRQ.

If you manage to find a more reliable way of reproducing this, then make
sure to capture dwc3 tracepoints (see the documentation for details) and
let's start trying to figure out what's going on.

cheers

-- 
balbi