Re: [PATCH] usb: dwc2: host: fix isoc urb actual length
From: wlf
Date: Tue Nov 07 2017 - 04:59:04 EST
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@xxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxx