From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753073AbdKHC6b (ORCPT ); Tue, 7 Nov 2017 21:58:31 -0500 Received: from regular1.263xmail.com ([211.150.99.133]:58685 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752375AbdKHC63 (ORCPT ); Tue, 7 Nov 2017 21:58:29 -0500 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 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: X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Subject: Re: [PATCH] usb: dwc2: host: fix isoc urb actual length To: Alan Stern Cc: Minas Harutyunyan , William Wu , "John.Youn@synopsys.com" , "balbi@kernel.org" , "gregkh@linuxfoundation.org" , "heiko@sntech.de" , "linux-kernel@vger.kernel.org" , "linux-usb@vger.kernel.org" , "linux-rockchip@lists.infradead.org" , "frank.wang@rock-chips.com" , "huangtao@rock-chips.com" , "dianders@google.com" , "daniel.meng@rock-chips.com" , "fml@rock-chips.com" References: From: wlf Message-ID: <8eeae89c-70a2-3c64-66dc-4053f2012e1a@rock-chips.com> Date: Wed, 8 Nov 2017 10:58:08 +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-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Alan, 在 2017年11月07日 23:18, Alan Stern 写道: > On Tue, 7 Nov 2017, wlf wrote: > >>> 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: >> >> 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)) >> 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); >> + > Well, this depends on whether you want to optimize for space or for > speed. I've been going for space. And since usbfs is inherently > rather slow, I don't think this will make any significant speed > difference. So I don't think adding the extra tests is worthwhile. > > (Besides, if you really wanted to do it this way, you should have moved > the test for "urb->number_of_packets > 0" into the callers instead of > adding an additional test of the endpoint type.) Yes, agree with you. > > However, nobody has answered my original question: Does the patch > actually fix the problem? I test your patch on Rockchip RK3188 platform, use UsbWebCamera APP by Serenegiant (libusb + devio) with usb camera, it work well. > > Alan Stern > > > > -- 吴良峰 William.Wu 福建省福州市铜盘路软件大道89号软件园A区21号楼 No.21 Building, A District, No.89,software Boulevard Fuzhou,Fujian, PRC 手机: 13685012275 座机: 0591-83991906-8520 邮件:wulf@rock-chips.com