From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 82756C4320A for ; Tue, 10 Aug 2021 07:11:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5CE4A61019 for ; Tue, 10 Aug 2021 07:11:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229721AbhHJHLj (ORCPT ); Tue, 10 Aug 2021 03:11:39 -0400 Received: from mail.kernel.org ([198.145.29.99]:47152 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229470AbhHJHLi (ORCPT ); Tue, 10 Aug 2021 03:11:38 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 42DC26101E; Tue, 10 Aug 2021 07:11:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1628579477; bh=IwchdthcW4P+bBMYFrtyi8Xq04VaMOj01WPs6GFnGFI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=PKLZrQMm1p+pZvggy5YfdVVIkuJ49ljD3TffLfk+xOs4fqjWBFFWEN9Y/x4lFigjG hfHYFM3FGe5RAiTbyKgGsFBKQS3BvjnNIukChaXieJNgrSiSzAV1ym/SG5P/fECCO9 52sHs91VqLOZR55LCIHtmLb6+p8V7aj0WuWm/9e4= Date: Tue, 10 Aug 2021 09:11:14 +0200 From: Greg Kroah-Hartman To: Thinh Nguyen Cc: John Stultz , lkml , Wesley Cheng , Felipe Balbi , Alan Stern , Jack Pham , Todd Kjos , Amit Pundir , YongQin Liu , Sumit Semwal , Petri Gynther , "linux-usb@vger.kernel.org" Subject: Re: [RFC][PATCH] dwc3: gadget: Fix losing list items in dwc3_gadget_ep_cleanup_completed_requests() Message-ID: References: <20210809223159.2342385-1-john.stultz@linaro.org> <4e1bef57-8520-36b9-f5cb-bbc925626a19@synopsys.com> <0dfa8cd6-99b6-55c7-8099-0f6f1187b7fd@synopsys.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Aug 10, 2021 at 08:05:49AM +0200, Greg Kroah-Hartman wrote: > On Mon, Aug 09, 2021 at 10:57:27PM +0000, Thinh Nguyen wrote: > > John Stultz wrote: > > > On Mon, Aug 9, 2021 at 3:44 PM Thinh Nguyen wrote: > > >> > > >> John Stultz wrote: > > >>> In commit d25d85061bd8 ("usb: dwc3: gadget: Use > > >>> list_replace_init() before traversing lists"), a local list_head > > >>> was introduced to process the started_list items to avoid races. > > >>> > > >>> However, in dwc3_gadget_ep_cleanup_completed_requests() if > > >>> dwc3_gadget_ep_cleanup_completed_request() fails, we break early, > > >>> causing the items on the local list_head to be lost. > > >>> > > >>> This issue showed up as problems on the db845c/RB3 board, where > > >>> adb connetions would fail, showing the device as "offline". > > >>> > > >>> This patch tries to fix the issue by if we are returning early > > >>> we splice in the local list head back into the started_list > > >>> and return (avoiding an infinite loop, as the started_list is > > >>> now non-null). > > >>> > > >>> Not sure if this is fully correct, but seems to work for me so I > > >>> wanted to share for feedback. > > >>> > > >>> Cc: Wesley Cheng > > >>> Cc: Felipe Balbi > > >>> Cc: Greg Kroah-Hartman > > >>> Cc: Alan Stern > > >>> Cc: Jack Pham > > >>> Cc: Thinh Nguyen > > >>> Cc: Todd Kjos > > >>> Cc: Amit Pundir > > >>> Cc: YongQin Liu > > >>> Cc: Sumit Semwal > > >>> Cc: Petri Gynther > > >>> Cc: linux-usb@vger.kernel.org > > >>> Fixes: d25d85061bd8 ("usb: dwc3: gadget: Use list_replace_init() before traversing lists") > > >>> Signed-off-by: John Stultz > > >>> --- > > >>> drivers/usb/dwc3/gadget.c | 6 ++++++ > > >>> 1 file changed, 6 insertions(+) > > >>> > > >>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > >>> index b8d4b2d327b23..a73ebe8e75024 100644 > > >>> --- a/drivers/usb/dwc3/gadget.c > > >>> +++ b/drivers/usb/dwc3/gadget.c > > >>> @@ -2990,6 +2990,12 @@ static void dwc3_gadget_ep_cleanup_completed_requests(struct dwc3_ep *dep, > > >>> break; > > >>> } > > >>> > > >>> + if (!list_empty(&local)) { > > >>> + list_splice_tail(&local, &dep->started_list); > > >>> + /* Return so we don't hit the restart case and loop forever */ > > >>> + return; > > >>> + } > > >>> + > > >>> if (!list_empty(&dep->started_list)) > > >>> goto restart; > > >>> } > > >>> > > >> > > >> No, we should revert the change for > > >> dwc3_gadget_ep_cleaup_completed_requests(). As I mentioned previously, > > >> we don't cleanup the entire started_list. If the original problem is due > > >> to disconnection in the middle of request completion, then we can just > > >> check for pullup_connected and exit the loop and let the > > >> dwc3_remove_requests() do the cleanup. > > > > > > Ok, sorry, I didn't read your mail in depth until I had this patch > > > sent out. If a revert of d25d85061bd8 is the better fix, I'm fine with > > > that too. > > > > > > thanks > > > -john > > > > > > > IMO, we should revert this patch for now since it will cause regression. > > We can review and test a proper fix at a later time. > > Ok, can someone send me a revert please? That will go faster than me > having to create it myself... I'll go do this now...