linux-kernel - Re: [PATCH] usb: dwc2: host: fix isoc urb actual length

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <8eeae89c-70a2-3c64-66dc-4053f2012e1a@rock-chips.com>
Date:   Wed, 8 Nov 2017 10:58:08 +0800
From:   wlf <wulf@...k-chips.com>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Minas Harutyunyan <Minas.Harutyunyan@...opsys.com>,
        William Wu <william.wu@...k-chips.com>,
        "John.Youn@...opsys.com" <John.Youn@...opsys.com>,
        "balbi@...nel.org" <balbi@...nel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "heiko@...ech.de" <heiko@...ech.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-rockchip@...ts.infradead.org" 
        <linux-rockchip@...ts.infradead.org>,
        "frank.wang@...k-chips.com" <frank.wang@...k-chips.com>,
        "huangtao@...k-chips.com" <huangtao@...k-chips.com>,
        "dianders@...gle.com" <dianders@...gle.com>,
        "daniel.meng@...k-chips.com" <daniel.meng@...k-chips.com>,
        "fml@...k-chips.com" <fml@...k-chips.com>
Subject: Re: [PATCH] usb: dwc2: host: fix isoc urb actual length

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@...k-chips.com