From patchwork Tue Nov 7 09:58:47 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: wuliangfeng X-Patchwork-Id: 10046135 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 5D3526031B for ; Tue, 7 Nov 2017 09:59:24 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4B1482913F for ; Tue, 7 Nov 2017 09:59:24 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 3E5F129D94; Tue, 7 Nov 2017 09:59:24 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.7 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, RCVD_IN_DNSWL_MED, RCVD_IN_SORBS_WEB autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 943242913F for ; Tue, 7 Nov 2017 09:59:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=leHU5+T6TwwgdgTquBbl6qmdPr1mT6+pzGPhiSvw7Q8=; b=pF4qccpfhVjmUk7EpbwaeKBsw FwRCFpb8L1ypXePo/rINly51jbsOalww+KnznkFtlABLZyUvIo3MFzsv+GCfrSmDN5yUPqhKTVPCn RnobdgJq90KiEY45UdGaAfqfXffW8d2yIWvQoEdIu1muWJ28eGNF/POfK67yNv30WklvoP6zMXwbf FdQxz1PQI0c6AWmQ/i27nLv9EtjnV47WZ0q8q2mn68DxIBt7s0QtHW9VzbnHCXuIyEtEM9f1P/Di3 uoXe3BogRFemca80TqVLMO7HeDCqIKEX3zd2XOd9T9TAu20sMcgIAipx7ksWaYIzPT45ElHiNQ2Eh iE8ab3EZw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1eC0fD-0001Gg-0P; Tue, 07 Nov 2017 09:59:23 +0000 Received: from regular1.263xmail.com ([211.150.99.136]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1eC0f8-0001Ax-AR for linux-rockchip@lists.infradead.org; Tue, 07 Nov 2017 09:59:21 +0000 Received: from wulf?rock-chips.com (unknown [192.168.167.235]) by regular1.263xmail.com (Postfix) with ESMTP id 0F8C440; Tue, 7 Nov 2017 17:58:51 +0800 (CST) X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 Received: from [172.16.12.36] (localhost [127.0.0.1]) by smtp.263.net (Postfix) with ESMTPA id 9A3DE3D2; Tue, 7 Nov 2017 17:58:47 +0800 (CST) X-RL-SENDER: wulf@rock-chips.com X-FST-TO: fml@rock-chips.com X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: wulf@rock-chips.com X-UNIQUE-TAG: <24e2c061774a359709e392132f6d6cd7> X-ATTACHMENT-NUM: 0 X-SENDER: wulf@rock-chips.com X-DNS-TYPE: 0 Received: from [172.16.12.36] (unknown [58.22.7.114]) by smtp.263.net (Postfix) whith ESMTP id 14389BKH5CR; Tue, 07 Nov 2017 17:58:50 +0800 (CST) Subject: Re: [PATCH] usb: dwc2: host: fix isoc urb actual length To: Alan Stern References: From: wlf Message-ID: Date: Tue, 7 Nov 2017 17:58:47 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20171107_015919_036742_8C3E4601 X-CRM114-Status: GOOD ( 20.31 ) X-BeenThere: linux-rockchip@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: Upstream kernel work for Rockchip platforms List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "huangtao@rock-chips.com" , "balbi@kernel.org" , "heiko@sntech.de" , "frank.wang@rock-chips.com" , "gregkh@linuxfoundation.org" , "linux-usb@vger.kernel.org" , Minas Harutyunyan , "John.Youn@synopsys.com" , "linux-rockchip@lists.infradead.org" , "fml@rock-chips.com" , William Wu , "dianders@google.com" , "daniel.meng@rock-chips.com" , "linux-kernel@vger.kernel.org" Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+patchwork-linux-rockchip=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Alan, 在 2017年11月07日 03:17, Alan Stern 写道: > On Mon, 6 Nov 2017, wlf wrote: > >> Hi Minas, >> >> 在 2017年11月06日 17:28, Minas Harutyunyan 写道: >>> Hi, >>> >>> On 11/6/2017 12:46 PM, William Wu wrote: >>>> The actual_length in dwc2_hcd_urb structure is used >>>> to indicate the total data length transferred so far, >>>> but in dwc2_update_isoc_urb_state(), it just updates >>>> the actual_length of isoc frame, and don't update the >>>> urb actual_length at the same time, this will cause >>>> device drivers working error which depend on the urb >>>> actual_length. >>>> >>>> we can easily find this issue if use an USB camera， >>>> the userspace use libusb to get USB data from kernel >>>> via devio driver.In usb devio driver, processcompl() >>>> function will process urb complete and copy data to >>>> userspace depending on urb actual_length. >>>> >>>> Let's update the urb actual_length if the isoc frame >>>> is valid. >>>> >>>> Signed-off-by: William Wu >>>> --- >>>> drivers/usb/dwc2/hcd_intr.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c >>>> index 28a8210..01b1e13 100644 >>>> --- a/drivers/usb/dwc2/hcd_intr.c >>>> +++ b/drivers/usb/dwc2/hcd_intr.c >>>> @@ -580,6 +580,7 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state( >>>> frame_desc->status = 0; >>>> frame_desc->actual_length = dwc2_get_actual_xfer_length(hsotg, >>>> chan, chnum, qtd, halt_status, NULL); >>>> + urb->actual_length += frame_desc->actual_length; >>>> break; >>>> case DWC2_HC_XFER_FRAME_OVERRUN: >>>> urb->error_count++; >>>> @@ -599,6 +600,7 @@ static enum dwc2_halt_status dwc2_update_isoc_urb_state( >>>> frame_desc->status = -EPROTO; >>>> frame_desc->actual_length = dwc2_get_actual_xfer_length(hsotg, >>>> chan, chnum, qtd, halt_status, NULL); >>>> + urb->actual_length += frame_desc->actual_length; >>>> >>>> /* Skip whole frame */ >>>> if (chan->qh->do_split && >>>> >>> According urb description in usb.h urb->actual_length used for non-iso >>> transfers: >>> >>> "@actual_length: This is read in non-iso completion functions, and ... >>> >>> * ISO transfer status is reported in the status and actual_length fields >>> * of the iso_frame_desc array, ...." >> Yes, urb->actual_length is used for non-iso transfers. And for ISO >> transfer, the most >> usb class drivers can only use iso frame status and actual_length to >> handle usb data, >> like: kernel v4.4 drivers/media/usb/uvc/uvc_video.c >> >> But the usb devio driver really need the urb actual_length in the >> processcompl() if >> handle ISO transfer, just as I mentioned in the commit message. >> >> And I also found the same issue on raspberrypi board: >> https://github.com/raspberrypi/linux/issues/903 >> >> So do you think we need to fix the usb devio driver, rather than fix dwc2? >> I think maybe we can use urb actual length for ISO transfer, it seems that >> don't cause any side-effect. > That sounds like a good idea. Minas, does the following patch fix your > problem? > > In theory we could do this calculation for every isochronous URB, not > just those coming from usbfs. But I don't think there's any point, > since the USB class drivers don't use it. > > Alan Stern > > > > Index: usb-4.x/drivers/usb/core/devio.c > =================================================================== > --- usb-4.x.orig/drivers/usb/core/devio.c > +++ usb-4.x/drivers/usb/core/devio.c > @@ -1828,6 +1828,18 @@ static int proc_unlinkurb(struct usb_dev > return 0; > } > > +static void compute_isochronous_actual_length(struct urb *urb) > +{ > + unsigned i; > + > + if (urb->number_of_packets > 0) { > + urb->actual_length = 0; > + for (i = 0; i < urb->number_of_packets; i++) > + urb->actual_length += > + urb->iso_frame_desc[i].actual_length; > + } > +} > + > static int processcompl(struct async *as, void __user * __user *arg) > { > struct urb *urb = as->urb; > @@ -1835,6 +1847,8 @@ static int processcompl(struct async *as > void __user *addr = as->userurb; > unsigned int i; > > + compute_isochronous_actual_length(urb); > + > if (as->userbuffer && urb->actual_length) { > if (copy_urb_data_to_user(as->userbuffer, urb)) > goto err_out; > @@ -2003,6 +2017,8 @@ static int processcompl_compat(struct as > void __user *addr = as->userurb; > unsigned int i; > > + compute_isochronous_actual_length(urb); > + > if (as->userbuffer && urb->actual_length) { > if (copy_urb_data_to_user(as->userbuffer, urb)) > return -EFAULT; > > Yeah, it's a good idea to calculate the urb actual length in the devio driver. Your patch seems good, and I think we can do a small optimization base your patch, like the following patch: goto err_out; @@ -1833,6 +1836,9 @@ static int processcompl_compat(struct async *as, void __user * __user *arg) void __user *addr = as->userurb; unsigned int i; + if (usb_endpoint_xfer_isoc(&urb->ep->desc)) + compute_isochronous_actual_length(urb); + if (as->userbuffer && urb->actual_length) { if (copy_urb_data_to_user(as->userbuffer, urb)) return -EFAULT; > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index bd94192..a2e7b97 100644 --- a/drivers/usb/core/devio.c +++ b/drivers/usb/core/devio.c @@ -1664,6 +1664,9 @@ static int processcompl(struct async *as, void __user * __user *arg) void __user *addr = as->userurb; unsigned int i; + if (usb_endpoint_xfer_isoc(&urb->ep->desc)) + compute_isochronous_actual_length(urb); + if (as->userbuffer && urb->actual_length) { if (copy_urb_data_to_user(as->userbuffer, urb))