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:     Roger Quadros <rogerq@ti.com>,
        Baolin Wang <baolin.wang@linaro.org>,
        John Youn <John.Youn@synopsys.com>
Cc:     USB <linux-usb@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume
In-Reply-To: <8ec0485e-89af-568b-e34a-b0cd490817d0@ti.com>
References: <1519730526-22274-1-git-send-email-rogerq@ti.com> <87sh9l5z4l.fsf@linux.intel.com> <94cd6377-1327-2309-8d69-6ab0de2bdfd4@ti.com> <87po4i3o1v.fsf@linux.intel.com> <a907787a-dea2-5f15-3d25-693680a42b71@ti.com> <CAMz4kuKOnAPckX1=nqVPyF7O5u-0YrekTfMddYwe0WC2-0AFDw@mail.gmail.com> <87k1uq3ho6.fsf@linux.intel.com> <8ec0485e-89af-568b-e34a-b0cd490817d0@ti.com>
Date:   Mon, 05 Mar 2018 13:25:36 +0200
Message-ID: <87k1uqwyq7.fsf@linux.intel.com>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
        micalg=pgp-sha512; protocol="application/pgp-signature"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable


Hi,

Roger Quadros <rogerq@ti.com> writes:
> On 05/03/18 13:06, Felipe Balbi wrote:
>>=20
>> Hi,
>>=20
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>> Roger Quadros <rogerq@ti.com> writes:
>>>>>>> Roger Quadros <rogerq@ti.com> writes:
>>>>>>>> In the following test we get stuck by sleeping forever in _dwc3_se=
t_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 driv=
e 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@ti.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 =3D 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
>>>>>>>> +          struct dwc3_ep  *dep =3D dwc->eps[epnum];
>>>>>>>> +
>>>>>>>> +          if (!dep)
>>>>>>>> +                  continue;
>>>>>>>> +
>>>>>>>> +          dep->flags &=3D ~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 fr=
ee
>>>>>>> the memory anyway. Might as well clear all flags to 0 there.
>>>>>>>
>>>>>>
>>>>>> But it won't solve the deadlock issue. Since dwc3_gadget_free_endpoi=
nts()
>>>>>> is called after usb_del_gadget_udc() and the deadlock happens when
>>>>>>
>>>>>> usb_del_gadget_udc()->udc_stop()->dwc3_gadget_stop()->wait_event_loc=
k_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 f=
lag
>>>>> before calling ->udc_stop(), this means the loop in dwc3_gadget_stop()
>>>>> will do nothing. Might as well remove it.
>>>>>
>>>>
>>>> This means that we will never wait for DWC3_EP_END_TRANSFER_PENDING to=
 clear
>>>> in dwc3_gadget_stop() like we used to. This is perfectly fine, right?
>>>>
>>>> It makes sense to me as dwc3_gadget_stop() calls __dwc3_gadget_stop() =
which
>>>> masks all interrupts and nobody will ever clear that flag if it was se=
t.
>>>
>>> I don't think so. It can not mask the endpoint events, please check
>>> the events which will be masked in DEVTEN register. The reason why we
>>> should wait for DWC3_EP_END_TRANSFER_PENDING to clear is that,
>>> sometimes the DWC3_DEPEVT_EPCMDCMPLT event will be triggered later
>>> than 100us, but now we may have freed the gadget irq which will cause
>>> crash.
>>=20
>> We could mask command complete events as soon as ->udc_stop() is called,
>> right? Hmm, actually, __dwc3_gadget_stop() already clears DEVTEN
>> completely.
>
> But which bit in DEVTEN says Endpoint events are disabled?
>
>>=20
>> /me goes check databook
>>=20
>> At least on revision 2.60a of the databook, bit 10 is reserved. I wonder
>> if that's the start of all the problems. Anybody has access to older and
>> newer databook revisions so we can cross-check?
>>=20
>
> I can access v2.40 and v3.10 books.
>
> bit 10 is reserved on both
>
> Differences in v2.4 vs v3.10 are:
>
> bit 8	reserved	vs	L1SUSPEN
> bit 13	reserved	vs	StopOnDisconnectEn
> bit 14	reserved	vs	L1WKUPEVTEN

odd, at some point we lost command complete interrupt :-(

That line exists since first commit (see below), so that would mean it
existed in 1.73a (the revision the original was written against), but
vanished on 2.40a. Perhaps 2.00a still had it.

Hey John, do you know, off the top of your head, when we lost DEVTEN[10]
as mask/unmask bit for EP Command Completion IRQs?


commit 72246da40f3719af3bfd104a2365b32537c27d83
Author: Felipe Balbi <balbi@ti.com>
Date:   Fri Aug 19 18:10:58 2011 +0300

    usb: Introduce DesignWare USB3 DRD Driver

=2D-=20
balbi

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAlqdKTAACgkQzL64meEa
mQYJhhAAiQsN3snKihl8xB8ZMi7OGxwt0PTz/r8xSsjabyxl4HBw1++Z4Wd4VLIz
FWGMpx8KLqljmCybFOWQ9jq7P3hMH1mzAvio6n378oR5vHKv00FOe/ftFwNQRIrv
mAJ0BevKg/Afz60atVbu76fQyY55/iHTLY2Edq1ZUUQt9AZq60P4ZF3XBI/dmYJ6
oS5C3GVuVOh8L4H7WW7Ssl5zjuWV19L9jz8+gZ4S0kbwwb8GEyxYgfg20tuTwULZ
sLWU3IukER+15y/iJt95evVMo87ibr9hPGIjDPG+EzmezLZ+gqyn6O7v64H2Wipw
1OQ5j05Zms4Bt/vVDYoHUduD20MOeNLOLeujnaQSFTguAym79ZZB92c75gRSH94h
HrrNIAwBEuc9XjcNGeZh7YLCeyIji78pHE+mQXEEHdSt0gKnUHLrXqo7hGdCE2me
IuN2kP7QOnyYGUcRQnvhOMwdKtQiHt8SXKSgFRGSZWcpl7KeymH+R7JtF8P8fozM
dZDPoLTAUjU1x/wfaCEavRcPSXSWp4jfPBf3azzlr7eFoN8oyaZNhrV3Ta/m/mVX
nB72kFDfplLzMOcmoMbq6LZml/5wzy8BrAc6Qiv+5IJtuxCo254JpcaAWP5Y+uLj
GUTuv/AJh9etfkY9q5mehiiXoDDAsijUrTP0W8E8+xH3J5aN7nc=
=gb26
-----END PGP SIGNATURE-----
--=-=-=--