diff mbox

usb: dwc2: host: fix isoc urb actual length

Message ID f2b7e952-8d0f-25dd-4777-efc1e2b9be8a@rock-chips.com (mailing list archive)
State New, archived
Headers show

Commit Message

wuliangfeng Nov. 7, 2017, 9:58 a.m. UTC 
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 <william.wu@rock-chips.com>
>>>> ---
>>>>     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;

>

Comments

Alan Stern Nov. 7, 2017, 3:18 p.m. UTC | #1 
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.)

However, nobody has answered my original question: Does the patch 
actually fix the problem?

Alan Stern
wuliangfeng Nov. 8, 2017, 2:58 a.m. UTC | #2 
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
>
>
>
>
diff mbox

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))