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