| Message ID | 20180716225357.v25f6rurz56q4yes@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Sebastian Andrzej Siewior wrote: > sound/ is (almost) the only part where struct usb_iso_packet_descriptor > init does not fit. It looks like it is done just before urb_submit() and > could be avoided there (moved to the init funtcion instead). For playback, the packet lengths change dynamically, because the nominal sample rate (let alone the actual sample rate) often is not an integer multiple of the 1 kHz/8 kHz USB frame rate. Regards, Clemens
Hi Sebastian, On Tuesday, 17 July 2018 01:53:57 EEST Sebastian Andrzej Siewior wrote: > On 2018-07-13 09:47:28 [+0200], To Greg Kroah-Hartman wrote: > > sure. Let me refresh my old usb_fill_int_urb() series with this instead. > > The series is at > > https://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/staging.git/log/?h= > usb-iso > > and needs double checking before it can be posted (and addressing the > few comments I had so far). Do you plan to send it in the near future ? I know this all started with simple patches and grew to a more complex patch series, but please don't give up, your work is valuable. > Here are just the highlights: > - usb_fill_iso_urb() itself: > > +static inline void usb_fill_iso_urb(struct urb *urb, > + struct usb_device *dev, > + unsigned int pipe, > + void *transfer_buffer, > + int buffer_length, > + usb_complete_t complete_fn, > + void *context, > + int interval, > + unsigned int packets, > + unsigned int packet_size) > +{ > + unsigned int i; > + > + urb->dev = dev; > + urb->pipe = pipe; > + urb->transfer_buffer = transfer_buffer; > + urb->transfer_buffer_length = buffer_length; > + urb->complete = complete_fn; > + urb->context = context; > + > + interval = clamp(interval, 1, 16); > + urb->interval = 1 << (interval - 1); > + urb->start_frame = -1; > + > + if (packets) > + urb->number_of_packets = packets; > + > + if (packet_size) { > + for (i = 0; i < packets; i++) { > + urb->iso_frame_desc[i].offset = packet_size * i; > + urb->iso_frame_desc[i].length = packet_size; > + } > + } > +} > > My understanding is that ->start_frame is only a return parameter. The > value is either implicit zero (via kzalloc()/memset()) or explicit > assignment to 0 or -1. So since it is a return value an init to 0 would > not be required and an initialisation to -1 (like for INT) would be > okay. Am I wrong? > > sound/ is (almost) the only part where struct usb_iso_packet_descriptor > init does not fit. It looks like it is done just before urb_submit() and > could be avoided there (moved to the init funtcion instead). However I > avoided changing it and added a zero check for `packet_size' so it can > be skipped. I also need to check if the `interval' value here to see if > it works as expected. Two examples: > > diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c > index c90607ebe155..f1d4e90e1d23 100644 > --- a/sound/usb/endpoint.c > +++ b/sound/usb/endpoint.c > @@ -772,6 +772,8 @@ static int data_ep_set_params(struct snd_usb_endpoint > *ep, /* allocate and initialize data urbs */ > for (i = 0; i < ep->nurbs; i++) { > struct snd_urb_ctx *u = &ep->urb[i]; > + void *buf; > + > u->index = i; > u->ep = ep; > u->packets = urb_packs; > @@ -783,16 +785,14 @@ static int data_ep_set_params(struct snd_usb_endpoint > *ep, if (!u->urb) > goto out_of_memory; > > - u->urb->transfer_buffer = > - usb_alloc_coherent(ep->chip->dev, u->buffer_size, > - GFP_KERNEL, &u->urb->transfer_dma); > - if (!u->urb->transfer_buffer) > + buf = usb_alloc_coherent(ep->chip->dev, u->buffer_size, > + GFP_KERNEL, &u->urb->transfer_dma); > + if (!buf) > goto out_of_memory; > - u->urb->pipe = ep->pipe; > + usb_fill_iso_urb(u->urb, NULL, ep->pipe, buf, u->buffer_size, > + snd_complete_urb, u, ep->datainterval + 1, 0, > + 0); > u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP; > - u->urb->interval = 1 << ep->datainterval; > - u->urb->context = u; > - u->urb->complete = snd_complete_urb; > INIT_LIST_HEAD(&u->ready_list); > } > > @@ -823,15 +823,12 @@ static int sync_ep_set_params(struct snd_usb_endpoint > *ep) u->urb = usb_alloc_urb(1, GFP_KERNEL); > if (!u->urb) > goto out_of_memory; > - u->urb->transfer_buffer = ep->syncbuf + i * 4; > + usb_fill_iso_urb(u->urb, NULL, ep->pipe, ep->syncbuf + i * 4, 4, > + snd_complete_urb, u, ep->syncinterval + 1, 1, > + 0); > + > u->urb->transfer_dma = ep->sync_dma + i * 4; > - u->urb->transfer_buffer_length = 4; > - u->urb->pipe = ep->pipe; > u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP; > - u->urb->number_of_packets = 1; > - u->urb->interval = 1 << ep->syncinterval; > - u->urb->context = u; > - u->urb->complete = snd_complete_urb; > } > > ep->nurbs = SYNC_URBS; > > diff --git a/sound/usb/usx2y/usx2yhwdeppcm.c > b/sound/usb/usx2y/usx2yhwdeppcm.c index 4fd9276b8e50..3928d0d50028 100644 > --- a/sound/usb/usx2y/usx2yhwdeppcm.c > +++ b/sound/usb/usx2y/usx2yhwdeppcm.c > @@ -325,6 +325,8 @@ static int usX2Y_usbpcm_urbs_allocate(struct > snd_usX2Y_substream *subs) /* allocate and initialize data urbs */ > for (i = 0; i < NRURBS; i++) { > struct urb **purb = subs->urb + i; > + void *buf; > + > if (*purb) { > usb_kill_urb(*purb); > continue; > @@ -334,18 +336,18 @@ static int usX2Y_usbpcm_urbs_allocate(struct > snd_usX2Y_substream *subs) usX2Y_usbpcm_urbs_release(subs); > return -ENOMEM; > } > - (*purb)->transfer_buffer = is_playback ? > - subs->usX2Y->hwdep_pcm_shm->playback : ( > - subs->endpoint == 0x8 ? > - subs->usX2Y->hwdep_pcm_shm->capture0x8 : > - subs->usX2Y->hwdep_pcm_shm->capture0xA); > - > - (*purb)->dev = dev; > - (*purb)->pipe = pipe; > - (*purb)->number_of_packets = nr_of_packs(); > - (*purb)->context = subs; > - (*purb)->interval = 1; > - (*purb)->complete = i_usX2Y_usbpcm_subs_startup; > + if (is_playback) { > + buf = subs->usX2Y->hwdep_pcm_shm->playback; > + } else { > + if (subs->endpoint == 0x8) > + buf = subs->usX2Y->hwdep_pcm_shm->capture0x8; > + else > + buf = subs->usX2Y->hwdep_pcm_shm->capture0xA; > + } > + usb_fill_iso_urb(*purb, dev, pipe, buf, > + subs->maxpacksize * nr_of_packs(), > + i_usX2Y_usbpcm_subs_startup, subs, 1, > + nr_of_packs(), 0); > } > return 0; > } > > The users in media/ look almost always the same, a random one: > > diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c > index 54b036d39c5b..2e60d6257596 100644 > --- a/drivers/media/usb/pwc/pwc-if.c > +++ b/drivers/media/usb/pwc/pwc-if.c > @@ -358,7 +358,7 @@ static int pwc_isoc_init(struct pwc_device *pdev) > { > struct usb_device *udev; > struct urb *urb; > - int i, j, ret; > + int i, ret; > struct usb_interface *intf; > struct usb_host_interface *idesc = NULL; > int compression = 0; /* 0..3 = uncompressed..high */ > @@ -409,6 +409,8 @@ static int pwc_isoc_init(struct pwc_device *pdev) > > /* Allocate and init Isochronuous urbs */ > for (i = 0; i < MAX_ISO_BUFS; i++) { > + void *buf; > + > urb = usb_alloc_urb(ISO_FRAMES_PER_DESC, GFP_KERNEL); > if (urb == NULL) { > pwc_isoc_cleanup(pdev); > @@ -416,29 +418,19 @@ static int pwc_isoc_init(struct pwc_device *pdev) > } > pdev->urbs[i] = urb; > PWC_DEBUG_MEMORY("Allocated URB at 0x%p\n", urb); > - > - urb->interval = 1; // devik > - urb->dev = udev; > - urb->pipe = usb_rcvisocpipe(udev, pdev->vendpoint); > urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP; > - urb->transfer_buffer = usb_alloc_coherent(udev, > - ISO_BUFFER_SIZE, > - GFP_KERNEL, > - &urb->transfer_dma); > - if (urb->transfer_buffer == NULL) { > + buf = usb_alloc_coherent(udev, ISO_BUFFER_SIZE, GFP_KERNEL, > + &urb->transfer_dma); > + if (buf == NULL) { > PWC_ERROR("Failed to allocate urb buffer %d\n", i); > pwc_isoc_cleanup(pdev); > return -ENOMEM; > } > - urb->transfer_buffer_length = ISO_BUFFER_SIZE; > - urb->complete = pwc_isoc_handler; > - urb->context = pdev; > - urb->start_frame = 0; > - urb->number_of_packets = ISO_FRAMES_PER_DESC; > - for (j = 0; j < ISO_FRAMES_PER_DESC; j++) { > - urb->iso_frame_desc[j].offset = j * ISO_MAX_FRAME_SIZE; > - urb->iso_frame_desc[j].length = pdev->vmax_packet_size; > - } > + usb_fill_iso_urb(urb, udev, > + usb_rcvisocpipe(udev, pdev->vendpoint), > + buf, ISO_BUFFER_SIZE, pwc_isoc_handler, pdev, > + 1, ISO_FRAMES_PER_DESC, > + pdev->vmax_packet_size); > } > > /* link */ > > I remember Alan asked to mention that the `.length' value is always set > to the same value by the proposed function while it could have different > values (like [0].offset = 0, [0].length = 8, [1].offset = 8, [1].length > = 16, [2].offset = 24, …). Unless I missed something, everyone using the > same value for length. Except for usbfs which uses what userland passes: > > diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c > index 476dcc5f2da3..54294f1a6ce5 100644 > --- a/drivers/usb/core/devio.c > +++ b/drivers/usb/core/devio.c > @@ -1436,7 +1436,8 @@ static int proc_do_submiturb(struct usb_dev_state *ps, > struct usbdevfs_urb *uurb int i, ret, is_in, num_sgs = 0, ifnum = -1; > int number_of_packets = 0; > unsigned int stream_id = 0; > - void *buf; > + int pipe; > + void *buf = NULL; > unsigned long mask = USBDEVFS_URB_SHORT_NOT_OK | > USBDEVFS_URB_BULK_CONTINUATION | > USBDEVFS_URB_NO_FSBR | > @@ -1631,22 +1632,20 @@ static int proc_do_submiturb(struct usb_dev_state > *ps, struct usbdevfs_urb *uurb } > totlen -= u; > } > + buf = NULL; > } else if (uurb->buffer_length > 0) { > if (as->usbm) { > unsigned long uurb_start = (unsigned long)uurb->buffer; > > - as->urb->transfer_buffer = as->usbm->mem + > - (uurb_start - as->usbm->vm_start); > + buf = as->usbm->mem + (uurb_start - as->usbm->vm_start); > } else { > - as->urb->transfer_buffer = kmalloc(uurb->buffer_length, > - GFP_KERNEL); > - if (!as->urb->transfer_buffer) { > + buf = kmalloc(uurb->buffer_length, GFP_KERNEL); > + if (!buf) { > ret = -ENOMEM; > goto error; > } > if (!is_in) { > - if (copy_from_user(as->urb->transfer_buffer, > - uurb->buffer, > + if (copy_from_user(buf, uurb->buffer, > uurb->buffer_length)) { > ret = -EFAULT; > goto error; > @@ -1658,16 +1657,10 @@ static int proc_do_submiturb(struct usb_dev_state > *ps, struct usbdevfs_urb *uurb * short. Clear the buffer so that the gaps > * don't leak kernel data to userspace. > */ > - memset(as->urb->transfer_buffer, 0, > - uurb->buffer_length); > + memset(buf, 0, uurb->buffer_length); > } > } > } > - as->urb->dev = ps->dev; > - as->urb->pipe = (uurb->type << 30) | > - __create_pipe(ps->dev, uurb->endpoint & 0xf) | > - (uurb->endpoint & USB_DIR_IN); > - > /* This tedious sequence is necessary because the URB_* flags > * are internal to the kernel and subject to change, whereas > * the USBDEVFS_URB_* flags are a user API and must not be changed. > @@ -1683,30 +1676,42 @@ static int proc_do_submiturb(struct usb_dev_state > *ps, struct usbdevfs_urb *uurb u |= URB_NO_INTERRUPT; > as->urb->transfer_flags = u; > > - as->urb->transfer_buffer_length = uurb->buffer_length; > - as->urb->setup_packet = (unsigned char *)dr; > - dr = NULL; > + pipe = (uurb->type << 30) | (uurb->endpoint & USB_DIR_IN) | > + __create_pipe(ps->dev, uurb->endpoint & 0xf); > + switch (uurb->type) { > + case USBDEVFS_URB_TYPE_CONTROL: > + usb_fill_control_urb(as->urb, ps->dev, pipe, (u8 *)dr, buf, > + uurb->buffer_length, async_completed, as); > + dr = NULL; > + break; > + > + case USBDEVFS_URB_TYPE_BULK: > + usb_fill_bulk_urb(as->urb, ps->dev, pipe, buf, > + uurb->buffer_length, async_completed, as); > + break; > + > + case USBDEVFS_URB_TYPE_INTERRUPT: > + usb_fill_int_urb(as->urb, ps->dev, pipe, buf, > + uurb->buffer_length, async_completed, as, > + ep->desc.bInterval); > + break; > + > + case USBDEVFS_URB_TYPE_ISO: > + usb_fill_iso_urb(as->urb, ps->dev, pipe, buf, > + uurb->buffer_length, async_completed, as, > + ep->desc.bInterval, number_of_packets, 0); > + for (totlen = u = 0; u < number_of_packets; u++) { > + as->urb->iso_frame_desc[u].offset = totlen; > + as->urb->iso_frame_desc[u].length = isopkt[u].length; > + totlen += isopkt[u].length; > + } > + break; > + > + } > + buf = NULL; > as->urb->start_frame = uurb->start_frame; > - as->urb->number_of_packets = number_of_packets; > as->urb->stream_id = stream_id; > > - if (ep->desc.bInterval) { > - if (uurb->type == USBDEVFS_URB_TYPE_ISO || > - ps->dev->speed == USB_SPEED_HIGH || > - ps->dev->speed >= USB_SPEED_SUPER) > - as->urb->interval = 1 << > - min(15, ep->desc.bInterval - 1); > - else > - as->urb->interval = ep->desc.bInterval; > - } > - > - as->urb->context = as; > - as->urb->complete = async_completed; > - for (totlen = u = 0; u < number_of_packets; u++) { > - as->urb->iso_frame_desc[u].offset = totlen; > - as->urb->iso_frame_desc[u].length = isopkt[u].length; > - totlen += isopkt[u].length; > - } > kfree(isopkt); > isopkt = NULL; > as->ps = ps; > @@ -1777,6 +1782,7 @@ static int proc_do_submiturb(struct usb_dev_state *ps, > struct usbdevfs_urb *uurb dec_usb_memory_use_count(as->usbm, > &as->usbm->urb_use_count); > kfree(isopkt); > kfree(dr); > + kfree(buf); > if (as) > free_async(as); > return ret; > > > > thanks, > > > > > > greg k-h > > Sebastian
On 2018-08-07 00:21:26 [+0300], Laurent Pinchart wrote: > Hi Sebastian, Hi Laurent, > Do you plan to send it in the near future ? I know this all started with > simple patches and grew to a more complex patch series, but please don't give > up, your work is valuable. Well, I could say that I waited for feedback from Greg after he asked for some examples :) The truth is that I tried to address the little review I got and the current heatwave made that impossible. So thanks for asking, I will try to continue this. Sebastian
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index c90607ebe155..f1d4e90e1d23 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -772,6 +772,8 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep, /* allocate and initialize data urbs */ for (i = 0; i < ep->nurbs; i++) { struct snd_urb_ctx *u = &ep->urb[i]; + void *buf; + u->index = i; u->ep = ep; u->packets = urb_packs; @@ -783,16 +785,14 @@ static int data_ep_set_params(struct snd_usb_endpoint *ep, if (!u->urb) goto out_of_memory; - u->urb->transfer_buffer = - usb_alloc_coherent(ep->chip->dev, u->buffer_size, - GFP_KERNEL, &u->urb->transfer_dma); - if (!u->urb->transfer_buffer) + buf = usb_alloc_coherent(ep->chip->dev, u->buffer_size, + GFP_KERNEL, &u->urb->transfer_dma); + if (!buf) goto out_of_memory; - u->urb->pipe = ep->pipe; + usb_fill_iso_urb(u->urb, NULL, ep->pipe, buf, u->buffer_size, + snd_complete_urb, u, ep->datainterval + 1, 0, + 0); u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP; - u->urb->interval = 1 << ep->datainterval; - u->urb->context = u; - u->urb->complete = snd_complete_urb; INIT_LIST_HEAD(&u->ready_list); } @@ -823,15 +823,12 @@ static int sync_ep_set_params(struct snd_usb_endpoint *ep) u->urb = usb_alloc_urb(1, GFP_KERNEL); if (!u->urb) goto out_of_memory; - u->urb->transfer_buffer = ep->syncbuf + i * 4; + usb_fill_iso_urb(u->urb, NULL, ep->pipe, ep->syncbuf + i * 4, 4, + snd_complete_urb, u, ep->syncinterval + 1, 1, + 0); + u->urb->transfer_dma = ep->sync_dma + i * 4; - u->urb->transfer_buffer_length = 4; - u->urb->pipe = ep->pipe; u->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP; - u->urb->number_of_packets = 1; - u->urb->interval = 1 << ep->syncinterval; - u->urb->context = u; - u->urb->complete = snd_complete_urb; } ep->nurbs = SYNC_URBS; diff --git a/sound/usb/usx2y/usx2yhwdeppcm.c b/sound/usb/usx2y/usx2yhwdeppcm.c index 4fd9276b8e50..3928d0d50028 100644 --- a/sound/usb/usx2y/usx2yhwdeppcm.c +++ b/sound/usb/usx2y/usx2yhwdeppcm.c @@ -325,6 +325,8 @@ static int usX2Y_usbpcm_urbs_allocate(struct snd_usX2Y_substream *subs) /* allocate and initialize data urbs */ for (i = 0; i < NRURBS; i++) { struct urb **purb = subs->urb + i; + void *buf; + if (*purb) { usb_kill_urb(*purb); continue; @@ -334,18 +336,18 @@ static int usX2Y_usbpcm_urbs_allocate(struct snd_usX2Y_substream *subs) usX2Y_usbpcm_urbs_release(subs); return -ENOMEM; } - (*purb)->transfer_buffer = is_playback ? - subs->usX2Y->hwdep_pcm_shm->playback : ( - subs->endpoint == 0x8 ? - subs->usX2Y->hwdep_pcm_shm->capture0x8 : - subs->usX2Y->hwdep_pcm_shm->capture0xA); - - (*purb)->dev = dev; - (*purb)->pipe = pipe; - (*purb)->number_of_packets = nr_of_packs(); - (*purb)->context = subs; - (*purb)->interval = 1; - (*purb)->complete = i_usX2Y_usbpcm_subs_startup; + if (is_playback) { + buf = subs->usX2Y->hwdep_pcm_shm->playback; + } else { + if (subs->endpoint == 0x8) + buf = subs->usX2Y->hwdep_pcm_shm->capture0x8; + else + buf = subs->usX2Y->hwdep_pcm_shm->capture0xA; + } + usb_fill_iso_urb(*purb, dev, pipe, buf, + subs->maxpacksize * nr_of_packs(), + i_usX2Y_usbpcm_subs_startup, subs, 1, + nr_of_packs(), 0); } return 0; } The users in media/ look almost always the same, a random one: diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c index 54b036d39c5b..2e60d6257596 100644 --- a/drivers/media/usb/pwc/pwc-if.c +++ b/drivers/media/usb/pwc/pwc-if.c @@ -358,7 +358,7 @@ static int pwc_isoc_init(struct pwc_device *pdev) { struct usb_device *udev; struct urb *urb; - int i, j, ret; + int i, ret; struct usb_interface *intf; struct usb_host_interface *idesc = NULL; int compression = 0; /* 0..3 = uncompressed..high */ @@ -409,6 +409,8 @@ static int pwc_isoc_init(struct pwc_device *pdev) /* Allocate and init Isochronuous urbs */ for (i = 0; i < MAX_ISO_BUFS; i++) { + void *buf; + urb = usb_alloc_urb(ISO_FRAMES_PER_DESC, GFP_KERNEL); if (urb == NULL) { pwc_isoc_cleanup(pdev); @@ -416,29 +418,19 @@ static int pwc_isoc_init(struct pwc_device *pdev) } pdev->urbs[i] = urb; PWC_DEBUG_MEMORY("Allocated URB at 0x%p\n", urb); - - urb->interval = 1; // devik - urb->dev = udev; - urb->pipe = usb_rcvisocpipe(udev, pdev->vendpoint); urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP; - urb->transfer_buffer = usb_alloc_coherent(udev, - ISO_BUFFER_SIZE, - GFP_KERNEL, - &urb->transfer_dma); - if (urb->transfer_buffer == NULL) { + buf = usb_alloc_coherent(udev, ISO_BUFFER_SIZE, GFP_KERNEL, + &urb->transfer_dma); + if (buf == NULL) { PWC_ERROR("Failed to allocate urb buffer %d\n", i); pwc_isoc_cleanup(pdev); return -ENOMEM; } - urb->transfer_buffer_length = ISO_BUFFER_SIZE; - urb->complete = pwc_isoc_handler; - urb->context = pdev; - urb->start_frame = 0; - urb->number_of_packets = ISO_FRAMES_PER_DESC; - for (j = 0; j < ISO_FRAMES_PER_DESC; j++) { - urb->iso_frame_desc[j].offset = j * ISO_MAX_FRAME_SIZE; - urb->iso_frame_desc[j].length = pdev->vmax_packet_size; - } + usb_fill_iso_urb(urb, udev, + usb_rcvisocpipe(udev, pdev->vendpoint), + buf, ISO_BUFFER_SIZE, pwc_isoc_handler, pdev, + 1, ISO_FRAMES_PER_DESC, + pdev->vmax_packet_size); } /* link */ I remember Alan asked to mention that the `.length' value is always set to the same value by the proposed function while it could have different values (like [0].offset = 0, [0].length = 8, [1].offset = 8, [1].length = 16, [2].offset = 24, …). Unless I missed something, everyone using the same value for length. Except for usbfs which uses what userland passes: diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index 476dcc5f2da3..54294f1a6ce5 100644 --- a/drivers/usb/core/devio.c +++ b/drivers/usb/core/devio.c @@ -1436,7 +1436,8 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb int i, ret, is_in, num_sgs = 0, ifnum = -1; int number_of_packets = 0; unsigned int stream_id = 0; - void *buf; + int pipe; + void *buf = NULL; unsigned long mask = USBDEVFS_URB_SHORT_NOT_OK | USBDEVFS_URB_BULK_CONTINUATION | USBDEVFS_URB_NO_FSBR | @@ -1631,22 +1632,20 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb } totlen -= u; } + buf = NULL; } else if (uurb->buffer_length > 0) { if (as->usbm) { unsigned long uurb_start = (unsigned long)uurb->buffer; - as->urb->transfer_buffer = as->usbm->mem + - (uurb_start - as->usbm->vm_start); + buf = as->usbm->mem + (uurb_start - as->usbm->vm_start); } else { - as->urb->transfer_buffer = kmalloc(uurb->buffer_length, - GFP_KERNEL); - if (!as->urb->transfer_buffer) { + buf = kmalloc(uurb->buffer_length, GFP_KERNEL); + if (!buf) { ret = -ENOMEM; goto error; } if (!is_in) { - if (copy_from_user(as->urb->transfer_buffer, - uurb->buffer, + if (copy_from_user(buf, uurb->buffer, uurb->buffer_length)) { ret = -EFAULT; goto error; @@ -1658,16 +1657,10 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb * short. Clear the buffer so that the gaps * don't leak kernel data to userspace. */ - memset(as->urb->transfer_buffer, 0, - uurb->buffer_length); + memset(buf, 0, uurb->buffer_length); } } } - as->urb->dev = ps->dev; - as->urb->pipe = (uurb->type << 30) | - __create_pipe(ps->dev, uurb->endpoint & 0xf) | - (uurb->endpoint & USB_DIR_IN); - /* This tedious sequence is necessary because the URB_* flags * are internal to the kernel and subject to change, whereas * the USBDEVFS_URB_* flags are a user API and must not be changed. @@ -1683,30 +1676,42 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb u |= URB_NO_INTERRUPT; as->urb->transfer_flags = u; - as->urb->transfer_buffer_length = uurb->buffer_length; - as->urb->setup_packet = (unsigned char *)dr; - dr = NULL; + pipe = (uurb->type << 30) | (uurb->endpoint & USB_DIR_IN) | + __create_pipe(ps->dev, uurb->endpoint & 0xf); + switch (uurb->type) { + case USBDEVFS_URB_TYPE_CONTROL: + usb_fill_control_urb(as->urb, ps->dev, pipe, (u8 *)dr, buf, + uurb->buffer_length, async_completed, as); + dr = NULL; + break; + + case USBDEVFS_URB_TYPE_BULK: + usb_fill_bulk_urb(as->urb, ps->dev, pipe, buf, + uurb->buffer_length, async_completed, as); + break; + + case USBDEVFS_URB_TYPE_INTERRUPT: + usb_fill_int_urb(as->urb, ps->dev, pipe, buf, + uurb->buffer_length, async_completed, as, + ep->desc.bInterval); + break; + + case USBDEVFS_URB_TYPE_ISO: + usb_fill_iso_urb(as->urb, ps->dev, pipe, buf, + uurb->buffer_length, async_completed, as, + ep->desc.bInterval, number_of_packets, 0); + for (totlen = u = 0; u < number_of_packets; u++) { + as->urb->iso_frame_desc[u].offset = totlen; + as->urb->iso_frame_desc[u].length = isopkt[u].length; + totlen += isopkt[u].length; + } + break; + + } + buf = NULL; as->urb->start_frame = uurb->start_frame; - as->urb->number_of_packets = number_of_packets; as->urb->stream_id = stream_id; - if (ep->desc.bInterval) { - if (uurb->type == USBDEVFS_URB_TYPE_ISO || - ps->dev->speed == USB_SPEED_HIGH || - ps->dev->speed >= USB_SPEED_SUPER) - as->urb->interval = 1 << - min(15, ep->desc.bInterval - 1); - else - as->urb->interval = ep->desc.bInterval; - } - - as->urb->context = as; - as->urb->complete = async_completed; - for (totlen = u = 0; u < number_of_packets; u++) { - as->urb->iso_frame_desc[u].offset = totlen; - as->urb->iso_frame_desc[u].length = isopkt[u].length; - totlen += isopkt[u].length; - } kfree(isopkt); isopkt = NULL; as->ps = ps; @@ -1777,6 +1782,7 @@ static int proc_do_submiturb(struct usb_dev_state *ps, struct usbdevfs_urb *uurb dec_usb_memory_use_count(as->usbm, &as->usbm->urb_use_count); kfree(isopkt); kfree(dr); + kfree(buf); if (as) free_async(as); return ret;