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.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS 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 8236DC4320A for ; Sun, 15 Aug 2021 06:11:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 688FA6103A for ; Sun, 15 Aug 2021 06:11:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232727AbhHOGMB (ORCPT ); Sun, 15 Aug 2021 02:12:01 -0400 Received: from mail.kernel.org ([198.145.29.99]:38092 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229633AbhHOGL6 (ORCPT ); Sun, 15 Aug 2021 02:11:58 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id ED35D6103A; Sun, 15 Aug 2021 06:11:27 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1629007889; bh=+IX5+SWQLDPBDNMa10/JtaQDgNFjaGUxPCZVn7OWT5s=; h=References:From:To:Cc:Subject:Date:In-reply-to:From; b=UhWGtgrv0I80no6wslohSzVx0fNAkNPYt+u+0uIWkAAfQzKI+qAmXvnjGgX3+A0Gu cCsoXXiLY6HFhc1ZnXBYA9vatgnnEOlHwpbxQWbNbdNBz5MHAww2uw8bENtfXlnE0f S34hkI7XefzeH7jEVAZIomrvbF0MJND55fyqUpxaFzEoBBcuSFaY8T46hAsQ7LD9Z0 PNhaBOYQ1mZag+BFCcjlkxZWBqkaOYGNQj8R7UuYt0RC+Xw7Wzo5F/v8HjDjmmAG4K z/X3lkDAXCyzT4Ngtc7w+k8Uibv3Z3UQLhg+6PyJ36idv/lge2kxlK9Ddj9V3VhLgT AmkFonmvRG3fA== References: <1628648608-15239-1-git-send-email-wcheng@codeaurora.org> <3edf74ba-d167-0589-a7ab-827b57aa5d9c@codeaurora.org> <5be881a9-c79d-3f21-9e2e-173307fef734@codeaurora.org> <00952bdc-acc2-f373-9286-6a8380e0b7d1@synopsys.com> User-agent: mu4e 1.6.2; emacs 27.2 From: Felipe Balbi To: Thinh Nguyen Cc: Wesley Cheng , "gregkh@linuxfoundation.org" , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "jackp@codeauora.org" Subject: Re: [RFC][PATCH] usb: dwc3: usb: dwc3: Force stop EP0 transfers during pullup disable Date: Sun, 15 Aug 2021 09:06:00 +0300 In-reply-to: <00952bdc-acc2-f373-9286-6a8380e0b7d1@synopsys.com> Message-ID: <875yw7jkz6.fsf@kernel.org> MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, Thinh Nguyen writes: >>>>>>>>> If this occurs, then the entire pullup disable routine is skipped and >>>>>>>>> proper cleanup and halting of the controller does not complete. >>>>>>>>> Instead of returning an error (which is ignored from the UDC >>>>>>>>> perspective), do what is mentioned in the comments and force the >>>>>>>>> transaction to complete and put the ep0state back to the SETUP phase. >>>>>>>>> >>>>>>>>> Signed-off-by: Wesley Cheng >>>>>>>>> --- >>>>>>>>> drivers/usb/dwc3/ep0.c | 4 ++-- >>>>>>>>> drivers/usb/dwc3/gadget.c | 6 +++++- >>>>>>>>> drivers/usb/dwc3/gadget.h | 3 +++ >>>>>>>>> 3 files changed, 10 insertions(+), 3 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c >>>>>>>>> index 6587394..abfc42b 100644 >>>>>>>>> --- a/drivers/usb/dwc3/ep0.c >>>>>>>>> +++ b/drivers/usb/dwc3/ep0.c >>>>>>>>> @@ -218,7 +218,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request, >>>>>>>>> return ret; >>>>>>>>> } >>>>>>>>> >>>>>>>>> -static void dwc3_ep0_stall_and_restart(struct dwc3 *dwc) >>>>>>>>> +void dwc3_ep0_stall_and_restart(struct dwc3 *dwc) >>>>>>>>> { >>>>>>>>> struct dwc3_ep *dep; >>>>>>>>> >>>>>>>>> @@ -1073,7 +1073,7 @@ void dwc3_ep0_send_delayed_status(struct dwc3 *dwc) >>>>>>>>> __dwc3_ep0_do_control_status(dwc, dwc->eps[direction]); >>>>>>>>> } >>>>>>>>> >>>>>>>>> -static void dwc3_ep0_end_control_data(struct dwc3 *dwc, struct dwc3_ep *dep) >>>>>>>>> +void dwc3_ep0_end_control_data(struct dwc3 *dwc, struct dwc3_ep *dep) >>>>>>>>> { >>>>>>>>> struct dwc3_gadget_ep_cmd_params params; >>>>>>>>> u32 cmd; >>>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>>>>>>> index 54c5a08..a0e2e4d 100644 >>>>>>>>> --- a/drivers/usb/dwc3/gadget.c >>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c >>>>>>>>> @@ -2437,7 +2437,11 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) >>>>>>>>> msecs_to_jiffies(DWC3_PULL_UP_TIMEOUT)); >>>>>>>>> if (ret == 0) { >>>>>>>>> dev_err(dwc->dev, "timed out waiting for SETUP phase\n"); >>>>>>>>> - return -ETIMEDOUT; >>>>>>>>> + spin_lock_irqsave(&dwc->lock, flags); >>>>>>>>> + dwc3_ep0_end_control_data(dwc, dwc->eps[0]); >>>>>>>>> + dwc3_ep0_end_control_data(dwc, dwc->eps[1]); >>>>>>>> >>>>>>>> End transfer command takes time, need to wait for it to complete before >>>>>>>> issuing Start transfer again. Also, why restart again when it's about to >>>>>>>> be disconnected. >>>>>>> >>>>>>> I can try without restarting it again, and see if that works. Instead >>>>>>> of waiting for the command complete event, can we set the ForceRM bit, >>>>>>> similar to what we do for dwc3_remove_requests()? >>>>>>> >>>>>> >>>>>> ForceRM=1 means that the controller will ignore updating the TRBs >>>>>> (including not clearing the HWO and remain transfer size). The driver >>>>>> still needs to wait for the command to complete before issuing Start >>>>>> Transfer command. Otherwise Start Transfer won't go through. If we know >>>>>> that we're not going to issue Start Transfer any time soon, then we may >>>>>> be able to get away with ignoring End Transfer command completion. >>>>>> >>>>> >>>>> I see. Currently, in the place that we do use >>>>> dwc3_ep0_end_control_data(), its followed by >>>>> dwc3_ep0_stall_and_restart() which would execute start transfer. For >>>> >>>> That doesn't look right. You can try to see if it can recover from a >>>> control write request. Often time we do control read and not write. >>>> (i.e. try to End Transfer and immediately Start Transfer on the same >>>> direction control endpoint). >>>> >>> OK, I can try, but just to clarify, I was referring to how it was being >>> done in: >>> >>> static void dwc3_ep0_xfernotready(struct dwc3 *dwc, >>> const struct dwc3_event_depevt *event) >>> { >>> ... >>> if (dwc->ep0_expect_in != event->endpoint_number) { >>> struct dwc3_ep *dep = dwc->eps[dwc->ep0_expect_in]; >>> >>> dev_err(dwc->dev, "unexpected direction for Data Phase\n"); >>> dwc3_ep0_end_control_data(dwc, dep); >>> dwc3_ep0_stall_and_restart(dwc); >>> return; >>> } >>> > > Looking at this snippet again, it looks wrong. For control write > unexpected direction, if the driver hasn't setup and started the DATA > phase yet, then it's fine, but there is a problem if it did. > > Since dwc3_ep0_end_control_data() doesn't issue End Transfer command to > ep0 due to the resource_index check, it doesn't follow the control IIRC resource_index is always non-zero, so the command should be triggered. If you have access to a Lecroy USB Trainer, could you script this very scenario for verification? > transfer flow model in the programming guide. This may cause > dwc3_ep0_stall_and_restart() to overwrite the TRBs for the DATA phase > with SETUP stage. Also, if the ep0 is already started, the driver won't > issue Start Transfer command again. > This issue is unlikely to occur unless we see a misbehave host for > control write request. Regardless, we need to fix this. I may need some right, it would be a misbehaving host, however databook called it out as something that _can_ happen. Moreover, I have vague memories of this being one of the test cases in Lecroy's USB Certification Suite. > time before I can create a patch and test it. If you or anyone is up to > take this on, it'd be highly appreciated. Before we go ahead writing a patch for this, I'd really like to see traces showing this failure and a minimal reproducer. The reproducer would probably have to be a script for Lecroy's USB Trainer. Keep in mind this entire ep0 stack used to pass USBCV on every -rc and major release (before I lost access to all my USB gear heh). -- balbi