[RFC] usb: add usb_fill_iso_urb()

[RFC] usb: add usb_fill_iso_urb()

[Sebastian Andrzej Siewior]

Provide usb_fill_iso_urb() for the initialisation of isochronous URBs.
We already have one of this helpers for control, bulk and interruptible
URB types. This helps to keep the initialisation of the URB members in
one place.
Update the documentation by adding this to the available init functions
and remove the suggestion to use the `_int_' helper which might provide
wrong encoding for the `interval' member.

This looks like it would cover most users nicely. The sound subsystem
initialises the ->iso_frame_desc[].offset + length member (often) at a
different location and I'm not sure ->interval will work always as
expected. So we might need to overwrite those two in worst case.

Some users also initialise ->iso_frame_desc[].actual_length but I don't
this is required since it is the return value.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 Documentation/driver-api/usb/URB.rst | 12 +++----
 include/linux/usb.h                  | 53 ++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/Documentation/driver-api/usb/URB.rst b/Documentation/driver-api/usb/URB.rst
index 61a54da9fce9..20030b781519 100644
--- a/Documentation/driver-api/usb/URB.rst
+++ b/Documentation/driver-api/usb/URB.rst
@@ -116,11 +116,11 @@ What has to be filled in?
 
 Depending on the type of transaction, there are some inline functions
 defined in ``linux/usb.h`` to simplify the initialization, such as
-:c:func:`usb_fill_control_urb`, :c:func:`usb_fill_bulk_urb` and
-:c:func:`usb_fill_int_urb`.  In general, they need the usb device pointer,
-the pipe (usual format from usb.h), the transfer buffer, the desired transfer
-length, the completion handler, and its context. Take a look at the some
-existing drivers to see how they're used.
+:c:func:`usb_fill_control_urb`, :c:func:`usb_fill_bulk_urb`,
+:c:func:`usb_fill_int_urb` and :c:func:`usb_fill_iso_urb`.  In general, they
+need the usb device pointer, the pipe (usual format from usb.h), the transfer
+buffer, the desired transfer length, the completion handler, and its context.
+Take a look at the some existing drivers to see how they're used.
 
 Flags:
 
@@ -243,7 +243,7 @@ Besides the fields present on a bulk transfer, for ISO, you also
 also have to set ``urb->interval`` to say how often to make transfers; it's
 often one per frame (which is once every microframe for highspeed devices).
 The actual interval used will be a power of two that's no bigger than what
-you specify. You can use the :c:func:`usb_fill_int_urb` macro to fill
+you specify. You can use the :c:func:`usb_fill_iso_urb` macro to fill
 most ISO transfer fields.
 
 For ISO transfers you also have to fill a :c:type:`usb_iso_packet_descriptor`
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 4cdd515a4385..74a3339041d6 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1697,6 +1697,59 @@ static inline void usb_fill_int_urb(struct urb *urb,
 	urb->start_frame = -1;
 }
 
+/**
+ * usb_fill_iso_urb - macro to help initialize an isochronous urb
+ * @urb: pointer to the urb to initialize.
+ * @dev: pointer to the struct usb_device for this urb.
+ * @pipe: the endpoint pipe
+ * @transfer_buffer: pointer to the transfer buffer
+ * @buffer_length: length of the transfer buffer
+ * @complete_fn: pointer to the usb_complete_t function
+ * @context: what to set the urb context to.
+ * @interval: what to set the urb interval to, encoded like
+ *	the endpoint descriptor's bInterval value.
+ * @packets: number of ISO packets.
+ * @packet_size: size of each ISO packet.
+ *
+ * Initializes an isochronous urb with the proper information needed to submit
+ * it to a device.
+ *
+ * Note that isochronous endpoints use a logarithmic encoding of the endpoint
+ * interval, and express polling intervals in microframes (eight per
+ * millisecond) rather than in frames (one per millisecond).
+ */
+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;
+
+	urb->number_of_packets = packets;
+
+	for (i = 0; i < packets; i++) {
+		urb->iso_frame_desc[i].offset = packet_size * i;
+		urb->iso_frame_desc[i].length = packet_size;
+	}
+}
+
 extern void usb_init_urb(struct urb *urb);
 extern struct urb *usb_alloc_urb(int iso_packets, gfp_t mem_flags);
 extern void usb_free_urb(struct urb *urb);

[RFC] usb: add usb_fill_iso_urb()

[Greg Kroah-Hartman]

On Fri, Jul 13, 2018 at 12:35:27AM +0200, Sebastian Andrzej Siewior wrote:
> Provide usb_fill_iso_urb() for the initialisation of isochronous URBs.
> We already have one of this helpers for control, bulk and interruptible
> URB types. This helps to keep the initialisation of the URB members in
> one place.
> Update the documentation by adding this to the available init functions
> and remove the suggestion to use the `_int_' helper which might provide
> wrong encoding for the `interval' member.
> 
> This looks like it would cover most users nicely. The sound subsystem
> initialises the ->iso_frame_desc[].offset + length member (often) at a
> different location and I'm not sure ->interval will work always as
> expected. So we might need to overwrite those two in worst case.
> 
> Some users also initialise ->iso_frame_desc[].actual_length but I don't
> this is required since it is the return value.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  Documentation/driver-api/usb/URB.rst | 12 +++----
>  include/linux/usb.h                  | 53 ++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+), 6 deletions(-)

Do you have a few example patches of using this new function?  Many many
years ago we tried to create this function, but we gave up as it just
didn't seem to work for the majority of the users of ISO packets.  Maybe
things have changed since then and people do it all more in a "standard"
way and we can take advantage of this.  But it would be nice to see
proof it can be used before taking this patch.

thanks,

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] usb: add usb_fill_iso_urb()

[Sebastian Andrzej Siewior]

On 2018-07-13 09:29:23 [+0200], Greg Kroah-Hartman wrote:
> Do you have a few example patches of using this new function?  Many many
> years ago we tried to create this function, but we gave up as it just
> didn't seem to work for the majority of the users of ISO packets.  Maybe
> things have changed since then and people do it all more in a "standard"
> way and we can take advantage of this.  But it would be nice to see
> proof it can be used before taking this patch.

sure. Let me refresh my old usb_fill_int_urb() series with this instead.

> thanks,
> 
> greg k-h

Sebastian
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] usb: add usb_fill_iso_urb()

[Laurent Pinchart]

Hi Sebastian,

Thank you for the patch.

On Friday, 13 July 2018 01:35:27 EEST Sebastian Andrzej Siewior wrote:
> Provide usb_fill_iso_urb() for the initialisation of isochronous URBs.
> We already have one of this helpers for control, bulk and interruptible
> URB types. This helps to keep the initialisation of the URB members in
> one place.
> Update the documentation by adding this to the available init functions
> and remove the suggestion to use the `_int_' helper which might provide
> wrong encoding for the `interval' member.
> 
> This looks like it would cover most users nicely. The sound subsystem
> initialises the ->iso_frame_desc[].offset + length member (often) at a
> different location and I'm not sure ->interval will work always as
> expected. So we might need to overwrite those two in worst case.
> 
> Some users also initialise ->iso_frame_desc[].actual_length but I don't

s/I don't/I don't think/ ?

> this is required since it is the return value.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  Documentation/driver-api/usb/URB.rst | 12 +++----
>  include/linux/usb.h                  | 53 ++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/driver-api/usb/URB.rst
> b/Documentation/driver-api/usb/URB.rst index 61a54da9fce9..20030b781519
> 100644
> --- a/Documentation/driver-api/usb/URB.rst
> +++ b/Documentation/driver-api/usb/URB.rst
> @@ -116,11 +116,11 @@ What has to be filled in?
> 
>  Depending on the type of transaction, there are some inline functions
>  defined in ``linux/usb.h`` to simplify the initialization, such as
> -:c:func:`usb_fill_control_urb`, :c:func:`usb_fill_bulk_urb` and
> -:c:func:`usb_fill_int_urb`.  In general, they need the usb device pointer,
> -the pipe (usual format from usb.h), the transfer buffer, the desired
> transfer -length, the completion handler, and its context. Take a look at
> the some -existing drivers to see how they're used.
> +:c:func:`usb_fill_control_urb`, :c:func:`usb_fill_bulk_urb`,
> +:c:func:`usb_fill_int_urb` and :c:func:`usb_fill_iso_urb`.  In general,
> they +need the usb device pointer, the pipe (usual format from usb.h), the
> transfer +buffer, the desired transfer length, the completion handler, and
> its context. +Take a look at the some existing drivers to see how they're
> used.
> 
>  Flags:
> 
> @@ -243,7 +243,7 @@ Besides the fields present on a bulk transfer, for ISO,
> you also also have to set ``urb->interval`` to say how often to make
> transfers; it's often one per frame (which is once every microframe for
> highspeed devices). The actual interval used will be a power of two that's
> no bigger than what -you specify. You can use the
> :c:func:`usb_fill_int_urb` macro to fill +you specify. You can use the
> :c:func:`usb_fill_iso_urb` macro to fill most ISO transfer fields.
> 
>  For ISO transfers you also have to fill a
> :c:type:`usb_iso_packet_descriptor` diff --git a/include/linux/usb.h
> b/include/linux/usb.h
> index 4cdd515a4385..74a3339041d6 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1697,6 +1697,59 @@ static inline void usb_fill_int_urb(struct urb *urb,
>  	urb->start_frame = -1;
>  }
> 
> +/**
> + * usb_fill_iso_urb - macro to help initialize an isochronous urb

Strictly speaking this isn't a macro, so I'd write "initializes an isochronous 
urb"

> + * @urb: pointer to the urb to initialize.
> + * @dev: pointer to the struct usb_device for this urb.
> + * @pipe: the endpoint pipe
> + * @transfer_buffer: pointer to the transfer buffer
> + * @buffer_length: length of the transfer buffer
> + * @complete_fn: pointer to the usb_complete_t function
> + * @context: what to set the urb context to.
> + * @interval: what to set the urb interval to, encoded like
> + *	the endpoint descriptor's bInterval value.
> + * @packets: number of ISO packets.
> + * @packet_size: size of each ISO packet.
> + *
> + * Initializes an isochronous urb with the proper information needed to
> submit
> + * it to a device.
> + *
> + * Note that isochronous endpoints use a logarithmic encoding of the
> endpoint
> + * interval, and express polling intervals in microframes (eight per
> + * millisecond) rather than in frames (one per millisecond).
> + */
> +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;
> +
> +	urb->number_of_packets = packets;
> +
> +	for (i = 0; i < packets; i++) {
> +		urb->iso_frame_desc[i].offset = packet_size * i;
> +		urb->iso_frame_desc[i].length = packet_size;
> +	}
> +}

I think this should be moved to a .c file as the function is growing big, but 
that's true of the other URB helpers as well, so it can be done later in a 
separate patch.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  extern void usb_init_urb(struct urb *urb);
>  extern struct urb *usb_alloc_urb(int iso_packets, gfp_t mem_flags);
>  extern void usb_free_urb(struct urb *urb);

[RFC] usb: add usb_fill_iso_urb()

[Alan Stern]

On Fri, 13 Jul 2018, Sebastian Andrzej Siewior wrote:

> Provide usb_fill_iso_urb() for the initialisation of isochronous URBs.
> We already have one of this helpers for control, bulk and interruptible
> URB types. This helps to keep the initialisation of the URB members in
> one place.
> Update the documentation by adding this to the available init functions
> and remove the suggestion to use the `_int_' helper which might provide
> wrong encoding for the `interval' member.
> 
> This looks like it would cover most users nicely. The sound subsystem
> initialises the ->iso_frame_desc[].offset + length member (often) at a
> different location and I'm not sure ->interval will work always as
> expected. So we might need to overwrite those two in worst case.
> 
> Some users also initialise ->iso_frame_desc[].actual_length but I don't
> this is required since it is the return value.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---


> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1697,6 +1697,59 @@ static inline void usb_fill_int_urb(struct urb *urb,
>  	urb->start_frame = -1;
>  }
>  
> +/**
> + * usb_fill_iso_urb - macro to help initialize an isochronous urb
> + * @urb: pointer to the urb to initialize.
> + * @dev: pointer to the struct usb_device for this urb.
> + * @pipe: the endpoint pipe
> + * @transfer_buffer: pointer to the transfer buffer
> + * @buffer_length: length of the transfer buffer
> + * @complete_fn: pointer to the usb_complete_t function
> + * @context: what to set the urb context to.
> + * @interval: what to set the urb interval to, encoded like
> + *	the endpoint descriptor's bInterval value.
> + * @packets: number of ISO packets.
> + * @packet_size: size of each ISO packet.
> + *
> + * Initializes an isochronous urb with the proper information needed to submit
> + * it to a device.
> + *
> + * Note that isochronous endpoints use a logarithmic encoding of the endpoint
> + * interval, and express polling intervals in microframes (eight per
> + * millisecond) rather than in frames (one per millisecond).

Full-speed devices express polling intervals in frames (1 per ms); 
high-speed and SuperSpeed devices express polling intervals in 
microframes (8 per ms).

> + */
> +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;
> +
> +	urb->number_of_packets = packets;
> +
> +	for (i = 0; i < packets; i++) {
> +		urb->iso_frame_desc[i].offset = packet_size * i;
> +		urb->iso_frame_desc[i].length = packet_size;
> +	}
> +}

This initialization of the iso_frame_desc[] elements assumes that all
the packets are the same size.  The kerneldoc should mention that this
is just an assumption; if it is wrong then the driver code will need to
rewrite the elements after calling this function.

One possibility is to allow the caller to set packets to 0; in that 
case you could avoid setting either urb->number_of_packets or the 
iso_frame_desc[] elements.

Alan Stern
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] usb: add usb_fill_iso_urb()

[Sebastian Andrzej Siewior]

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:


> > thanks,
> > 
> > greg k-h

Sebastian
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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;

[RFC] usb: add usb_fill_iso_urb()

[Clemens Ladisch]

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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] usb: add usb_fill_iso_urb()

[Laurent Pinchart]

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

[RFC] usb: add usb_fill_iso_urb()

[Sebastian Andrzej Siewior]

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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html