Message ID | 20230323171931.4085496-1-m.felsch@pengutronix.de |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:604a:0:0:0:0:0 with SMTP id j10csp3045565wrt; Thu, 23 Mar 2023 10:37:23 -0700 (PDT) X-Google-Smtp-Source: AK7set+lIHylZ5MHLEP7xl9Wid+02UtdFjf/+b+BtAj+Ul7KI8oxGa0roKRclBmq1uRHQhj6DKit X-Received: by 2002:a05:6a20:c525:b0:d4:b5dc:2909 with SMTP id gm37-20020a056a20c52500b000d4b5dc2909mr353525pzb.28.1679593043424; Thu, 23 Mar 2023 10:37:23 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1679593043; cv=none; d=google.com; s=arc-20160816; b=BkpC+BRKELQYpDtNb40xv2+j0b5eoP3qiEzjtGiFX4AF5TwELTfbduhzOrPD8KqINe McT1QUy6tOGzFBfnCKOuR41gBGlXumjXfwkcCHGqxWkI/wcRfQEv8cCOGgR/2kFtZSAf M7C4KT7PPQPR0hiuDniLnazAps5A87HDLP1fd9yUD0B4/637didpMnfDyELlsDJH7IuR kgeD3TM4FCGwcWy4ACCAWXXu2q6dpvF0BzPAUvKGZiPx28ZXxbuN0+VFscDla1spiSmL w+uEqMIuZRNtDdqha12XjOKVXJVD/8bdYCOeq3ZwOG6uA3wTOamAVuwTjE0+DCHaDWbC MSng== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=qjrGXgoB4dRZdN6YjKtiLFTi8llQ3OSZ6lTUsFitDTs=; b=M4x50DtxMThZmy2dgN1mNpKLejOf48jbPS9n+D6R6sKVIhJlZ72/ei1qsH2iwDnKqP ydetOtDmkWyI8LtS5SYm994POdo82UBUvUbUT+3oZsnWWbTZ2K542/DWvrjIOQMa4nUX VhGheC1MrqO3D29S60tkZ8JfWT0GBom1RCdKKl82wp9jX32pJvSxCOPHutda7gn8cmO2 9u7Tg6swMJmBSnsguWgM1k37VajZu7cDTPbqfAjSamOm3wZLP0KSZS0JKFtcMkMzZ61D cbJXUCDupWGXdh2urXDavne1e0wgWdoiUxwvBcwoDwqNgmginkhBlYnUGEDIyAVnyTwe 26rQ== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id m17-20020a056a00081100b0058dc4c4e238si14379323pfk.360.2023.03.23.10.37.10; Thu, 23 Mar 2023 10:37:23 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231250AbjCWRTr (ORCPT <rfc822;ezelljr.billy@gmail.com> + 99 others); Thu, 23 Mar 2023 13:19:47 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50598 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229508AbjCWRTp (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 23 Mar 2023 13:19:45 -0400 Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [IPv6:2001:67c:670:201:290:27ff:fe1d:cc33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6619B26872 for <linux-kernel@vger.kernel.org>; Thu, 23 Mar 2023 10:19:43 -0700 (PDT) Received: from drehscheibe.grey.stw.pengutronix.de ([2a0a:edc0:0:c01:1d::a2]) by metis.ext.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from <mfe@pengutronix.de>) id 1pfOb9-0005wZ-5s; Thu, 23 Mar 2023 18:19:35 +0100 Received: from [2a0a:edc0:0:1101:1d::28] (helo=dude02.red.stw.pengutronix.de) by drehscheibe.grey.stw.pengutronix.de with esmtp (Exim 4.94.2) (envelope-from <mfe@pengutronix.de>) id 1pfOb7-006CZa-9o; Thu, 23 Mar 2023 18:19:33 +0100 Received: from mfe by dude02.red.stw.pengutronix.de with local (Exim 4.94.2) (envelope-from <mfe@pengutronix.de>) id 1pfOb6-00H8q2-NS; Thu, 23 Mar 2023 18:19:32 +0100 From: Marco Felsch <m.felsch@pengutronix.de> To: Thinh.Nguyen@synopsys.com, gregkh@linuxfoundation.org, balbi@kernel.org Cc: linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, kernel@pengutronix.de Subject: [PATCH] usb: dwc3: gadget: lower informal user notifaction dequeue operation Date: Thu, 23 Mar 2023 18:19:31 +0100 Message-Id: <20230323171931.4085496-1-m.felsch@pengutronix.de> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SA-Exim-Connect-IP: 2a0a:edc0:0:c01:1d::a2 X-SA-Exim-Mail-From: mfe@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org X-Spam-Status: No, score=-2.3 required=5.0 tests=RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1761180954901404929?= X-GMAIL-MSGID: =?utf-8?q?1761180954901404929?= |
Series |
usb: dwc3: gadget: lower informal user notifaction dequeue operation
|
|
Commit Message
Marco Felsch
March 23, 2023, 5:19 p.m. UTC
Printing an error message during usb_ep_dequeue() is more confusing than
helpful since the usb_ep_dequeue() could be call during unbind() just
in case that everything is canceld before unbinding the driver. Lower
the dev_err() message to dev_dbg() to keep the message for developers.
Fixes: fcd2def66392 ("usb: dwc3: gadget: Refactor dwc3_gadget_ep_dequeue")
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
drivers/usb/dwc3/gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
Hi, On Thu, Mar 23, 2023, Marco Felsch wrote: > Printing an error message during usb_ep_dequeue() is more confusing than > helpful since the usb_ep_dequeue() could be call during unbind() just > in case that everything is canceld before unbinding the driver. Lower > the dev_err() message to dev_dbg() to keep the message for developers. > > Fixes: fcd2def66392 ("usb: dwc3: gadget: Refactor dwc3_gadget_ep_dequeue") > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/usb/dwc3/gadget.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 89dcfac01235f..6699db26cc7b5 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -2106,7 +2106,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep, > } > } > > - dev_err(dwc->dev, "request %pK was not queued to %s\n", > + dev_dbg(dwc->dev, "request %pK was not queued to %s\n", > request, ep->name); > ret = -EINVAL; > out: > -- > 2.30.2 > How were you able to reproduce this error message? During unbind(), the function driver would typically call to usb_ep_disable(). Before the call usb_ep_disable() completes, all queued and incompleted requests are expected to be returned with -ESHUTDOWN. For you to see this error, this means that the function driver issued usb_ep_dequeue() to an already disabled endpoint, and the request was probably already given back. Even though this error message is not critical and shouldn't affect the driver's behavior, it's better to fix the function driver to handle this race. Thanks, Thinh
Hi, On 23-03-23, Thinh Nguyen wrote: > Hi, > > On Thu, Mar 23, 2023, Marco Felsch wrote: > > Printing an error message during usb_ep_dequeue() is more confusing than > > helpful since the usb_ep_dequeue() could be call during unbind() just > > in case that everything is canceld before unbinding the driver. Lower > > the dev_err() message to dev_dbg() to keep the message for developers. > > > > Fixes: fcd2def66392 ("usb: dwc3: gadget: Refactor dwc3_gadget_ep_dequeue") > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/usb/dwc3/gadget.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > index 89dcfac01235f..6699db26cc7b5 100644 > > --- a/drivers/usb/dwc3/gadget.c > > +++ b/drivers/usb/dwc3/gadget.c > > @@ -2106,7 +2106,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep, > > } > > } > > > > - dev_err(dwc->dev, "request %pK was not queued to %s\n", > > + dev_dbg(dwc->dev, "request %pK was not queued to %s\n", > > request, ep->name); > > ret = -EINVAL; > > out: > > -- > > 2.30.2 > > > > How were you able to reproduce this error message? We use the driver within barebox where we do have support for fastboot. During the driver unbind usb_ep_dequeue() is called which throw this error. > During unbind(), the function driver would typically call to > usb_ep_disable(). Before the call usb_ep_disable() completes, all queued > and incompleted requests are expected to be returned with -ESHUTDOWN. So the unbind() function driver should use usb_ep_disable() instead of usb_ep_dequeue()? > For you to see this error, this means that the function driver issued > usb_ep_dequeue() to an already disabled endpoint, and the request was > probably already given back. The unbind() just calls usb_ep_dequeue() which isn't forbidden according the API doc. We just want to ensure that the request is cancled if any. > Even though this error message is not critical and shouldn't affect the > driver's behavior, it's better to fix the function driver to handle this > race. As you have pointed out: 'it is not criticial' and therefore we shouldn't use dev_err() for non crictical information since this can cause user-space confusion. Regards, Marco > > Thanks, > Thinh
On Fri, Mar 24, 2023, Marco Felsch wrote: > Hi, > > On 23-03-23, Thinh Nguyen wrote: > > Hi, > > > > On Thu, Mar 23, 2023, Marco Felsch wrote: > > > Printing an error message during usb_ep_dequeue() is more confusing than > > > helpful since the usb_ep_dequeue() could be call during unbind() just > > > in case that everything is canceld before unbinding the driver. Lower > > > the dev_err() message to dev_dbg() to keep the message for developers. > > > > > > Fixes: fcd2def66392 ("usb: dwc3: gadget: Refactor dwc3_gadget_ep_dequeue") > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > --- > > > drivers/usb/dwc3/gadget.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > > index 89dcfac01235f..6699db26cc7b5 100644 > > > --- a/drivers/usb/dwc3/gadget.c > > > +++ b/drivers/usb/dwc3/gadget.c > > > @@ -2106,7 +2106,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep, > > > } > > > } > > > > > > - dev_err(dwc->dev, "request %pK was not queued to %s\n", > > > + dev_dbg(dwc->dev, "request %pK was not queued to %s\n", > > > request, ep->name); > > > ret = -EINVAL; > > > out: > > > -- > > > 2.30.2 > > > > > > > How were you able to reproduce this error message? > > We use the driver within barebox where we do have support for fastboot. > During the driver unbind usb_ep_dequeue() is called which throw this > error. I mean which gadget/function driver did you use. > > > During unbind(), the function driver would typically call to > > usb_ep_disable(). Before the call usb_ep_disable() completes, all queued > > and incompleted requests are expected to be returned with -ESHUTDOWN. > > So the unbind() function driver should use usb_ep_disable() instead of > usb_ep_dequeue()? No, it can do whatever it wants. I'm just pointing out the typical behavior when this case happens during unbind(). > > > For you to see this error, this means that the function driver issued > > usb_ep_dequeue() to an already disabled endpoint, and the request was > > probably already given back. > > The unbind() just calls usb_ep_dequeue() which isn't forbidden according > the API doc. We just want to ensure that the request is cancled if any. It's not forbidden, and it's not unexpected for this message to be generated if usb_ep_dequeue() is called after usb_ep_disable(). However, knowing the behavior of usb_ep_disable(), does it make sense to call usb_ep_dequeue() after usb_ep_disable() completes? (I'm assuming this is what happened in your case from the commit description). > > > Even though this error message is not critical and shouldn't affect the > > driver's behavior, it's better to fix the function driver to handle this > > race. > > As you have pointed out: 'it is not criticial' and therefore we shouldn't > use dev_err() for non crictical information since this can cause > user-space confusion. I noted this particular case that it's not critical because we know where/when it happened because you pointed out that it occurs during unbind(). However, in any case, we want to notify that the usb_ep_dequeue() was used on a wrong request, allowing the user to review and fix this if needed. Thanks, Thinh
On 23-03-24, Thinh Nguyen wrote: > On Fri, Mar 24, 2023, Marco Felsch wrote: > > Hi, > > > > On 23-03-23, Thinh Nguyen wrote: > > > Hi, > > > > > > On Thu, Mar 23, 2023, Marco Felsch wrote: > > > > Printing an error message during usb_ep_dequeue() is more confusing than > > > > helpful since the usb_ep_dequeue() could be call during unbind() just > > > > in case that everything is canceld before unbinding the driver. Lower > > > > the dev_err() message to dev_dbg() to keep the message for developers. > > > > > > > > Fixes: fcd2def66392 ("usb: dwc3: gadget: Refactor dwc3_gadget_ep_dequeue") > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > --- > > > > drivers/usb/dwc3/gadget.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > > > index 89dcfac01235f..6699db26cc7b5 100644 > > > > --- a/drivers/usb/dwc3/gadget.c > > > > +++ b/drivers/usb/dwc3/gadget.c > > > > @@ -2106,7 +2106,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep, > > > > } > > > > } > > > > > > > > - dev_err(dwc->dev, "request %pK was not queued to %s\n", > > > > + dev_dbg(dwc->dev, "request %pK was not queued to %s\n", > > > > request, ep->name); > > > > ret = -EINVAL; > > > > out: > > > > -- > > > > 2.30.2 > > > > > > > > > > How were you able to reproduce this error message? > > > > We use the driver within barebox where we do have support for fastboot. > > During the driver unbind usb_ep_dequeue() is called which throw this > > error. > > I mean which gadget/function driver did you use. As I have written, the fastboot driver within barebox. > > > During unbind(), the function driver would typically call to > > > usb_ep_disable(). Before the call usb_ep_disable() completes, all queued > > > and incompleted requests are expected to be returned with -ESHUTDOWN. > > > > So the unbind() function driver should use usb_ep_disable() instead of > > usb_ep_dequeue()? > > No, it can do whatever it wants. I'm just pointing out the typical > behavior when this case happens during unbind(). Okay. > > > For you to see this error, this means that the function driver issued > > > usb_ep_dequeue() to an already disabled endpoint, and the request was > > > probably already given back. > > > > The unbind() just calls usb_ep_dequeue() which isn't forbidden according > > the API doc. We just want to ensure that the request is cancled if any. > > It's not forbidden, and it's not unexpected for this message to be > generated if usb_ep_dequeue() is called after usb_ep_disable(). Exactly that happened: usb_ep_disable() called in front of the usb_ep_dequeue(). Thanks to your first response which explained the behaviour, since I'm not that familiar with the gadget stack. > However, knowing the behavior of usb_ep_disable(), does it make sense > to call usb_ep_dequeue() after usb_ep_disable() completes? (I'm > assuming this is what happened in your case from the commit > description). Nope and therefore we removed it. > > > Even though this error message is not critical and shouldn't affect the > > > driver's behavior, it's better to fix the function driver to handle this > > > race. > > > > As you have pointed out: 'it is not criticial' and therefore we shouldn't > > use dev_err() for non crictical information since this can cause > > user-space confusion. > > I noted this particular case that it's not critical because we know > where/when it happened because you pointed out that it occurs during > unbind(). However, in any case, we want to notify that the > usb_ep_dequeue() was used on a wrong request, allowing the user to > review and fix this if needed. Right, thanks for your input. Please ignore this patch. Regards, Marco > > Thanks, > Thinh
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 89dcfac01235f..6699db26cc7b5 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2106,7 +2106,7 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep, } } - dev_err(dwc->dev, "request %pK was not queued to %s\n", + dev_dbg(dwc->dev, "request %pK was not queued to %s\n", request, ep->name); ret = -EINVAL; out: