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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
