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 -- Regards, Laurent Pinchart -- 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
