[PATCH] usb: dwc2: host: fix isoc urb actual length

[PATCH] usb: dwc2: host: fix isoc urb actual length

[William Wu]

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 &&
-- 
2.0.0

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

[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, ...."

Thanks,
Minas

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

[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, ...."

Thanks,
Minas

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

[wlf]

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.

>
> Thanks,
> Minas
>
>
>
>
>
>

-- 
吴良峰 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

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

[wlf]

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.

>
> Thanks,
> Minas
>
>
>
>
>
>

-- 
吴良峰 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

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

[Minas Harutyunyan]

Hi William,

On 11/6/2017 2:08 PM, 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://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_raspberrypi_linux_issues_903&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=6z9Al9FrHR_ZqbbtSAsD16pvOL2S3XHxQnSzq8kusyI&m=v5ECJPMqgVerj8G74QqsTJ7jt-qDVbCVIq8jq_t245s&s=ArX8DL6fZL4p3dsOY5AstGDRguIB_c1GDuuhf49-ev8&e=
> 
> 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.
Agree, should be no any side-effect.

> 
>>
>> Thanks,
>> Minas
>>
>>
>>
>>
>>
>>
> 

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

[Minas Harutyunyan]

Hi William,

On 11/6/2017 2:08 PM, 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://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_raspberrypi_linux_issues_903&d=DwIDaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=6z9Al9FrHR_ZqbbtSAsD16pvOL2S3XHxQnSzq8kusyI&m=v5ECJPMqgVerj8G74QqsTJ7jt-qDVbCVIq8jq_t245s&s=ArX8DL6fZL4p3dsOY5AstGDRguIB_c1GDuuhf49-ev8&e=
> 
> 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.
Agree, should be no any side-effect.

> 
>>
>> Thanks,
>> Minas
>>
>>
>>
>>
>>
>>
> 

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

[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;

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

[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;

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

[wlf]

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:

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);
+
         if (as->userbuffer && urb->actual_length) {
                 if (copy_urb_data_to_user(as->userbuffer, urb))
                         return -EFAULT;

>

-- 
吴良峰 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

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

[wlf]

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:

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);
+
         if (as->userbuffer && urb->actual_length) {
                 if (copy_urb_data_to_user(as->userbuffer, urb))
                         return -EFAULT;

>

-- 
吴良峰 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

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

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

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

Alan Stern

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

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

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

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[wlf]

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

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

[wlf]

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