medial: use irqsave() in URB completion + usb_fill_int_urb

medial: use irqsave() in URB completion + usb_fill_int_urb

[Sebastian Andrzej Siewior]

This series is mostly about using _irqsave() primitives in the
completion callback in order to get rid of local_irq_save() in
__usb_hcd_giveback_urb(). While at it, I also tried to move drivers to
use usb_fill_int_urb() otherwise it is hard find users of a certain API.
 
Sebastian

[PATCH 01/27] media: b2c2: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an URB. A
grep for members of the struct (like ->complete) reveal lots of other
things, too.

Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: linux-media@vger.kernel.org
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/b2c2/flexcop-usb.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/media/usb/b2c2/flexcop-usb.c b/drivers/media/usb/b2c2/flexcop-usb.c
index a8f3169e30b3..d5296f643a29 100644
--- a/drivers/media/usb/b2c2/flexcop-usb.c
+++ b/drivers/media/usb/b2c2/flexcop-usb.c
@@ -461,15 +461,13 @@ static int flexcop_usb_transfer_init(struct flexcop_usb *fc_usb)
 		deb_ts("initializing and submitting urb no. %d (buf_offset: %d).\n",
 		       i, buffer_offset);
 
-		urb->dev = fc_usb->udev;
-		urb->context = fc_usb;
-		urb->complete = flexcop_usb_urb_complete;
-		urb->pipe = B2C2_USB_DATA_PIPE;
+		usb_fill_int_urb(urb, fc_usb->udev, B2C2_USB_DATA_PIPE,
+				 fc_usb->iso_buffer + buffer_offset,
+				 frame_size * B2C2_USB_FRAMES_PER_ISO,
+				 flexcop_usb_urb_complete, fc_usb, 1);
+
 		urb->transfer_flags = URB_ISO_ASAP;
-		urb->interval = 1;
 		urb->number_of_packets = B2C2_USB_FRAMES_PER_ISO;
-		urb->transfer_buffer_length = frame_size * B2C2_USB_FRAMES_PER_ISO;
-		urb->transfer_buffer = fc_usb->iso_buffer + buffer_offset;
 
 		buffer_offset += frame_size * B2C2_USB_FRAMES_PER_ISO;
 		for (j = 0; j < B2C2_USB_FRAMES_PER_ISO; j++) {
-- 
2.17.1

[PATCH 02/27] media: cpia2_usb: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.

Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/cpia2/cpia2_usb.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/cpia2/cpia2_usb.c b/drivers/media/usb/cpia2/cpia2_usb.c
index a771e0a52610..6f94bdcc4111 100644
--- a/drivers/media/usb/cpia2/cpia2_usb.c
+++ b/drivers/media/usb/cpia2/cpia2_usb.c
@@ -689,16 +689,11 @@ static int submit_urbs(struct camera_data *cam)
 		}
 
 		cam->sbuf[i].urb = urb;
-		urb->dev = cam->dev;
+		usb_fill_int_urb(urb, cam->dev, usb_rcvisocpipe(cam->dev, 1),
+				 cam->sbuf[i].data, FRAME_SIZE_PER_DESC *
+				 FRAMES_PER_DESC, cpia2_usb_complete, cam, 1);
 		urb->context = cam;
-		urb->pipe = usb_rcvisocpipe(cam->dev, 1 /*ISOC endpoint*/);
 		urb->transfer_flags = URB_ISO_ASAP;
-		urb->transfer_buffer = cam->sbuf[i].data;
-		urb->complete = cpia2_usb_complete;
-		urb->number_of_packets = FRAMES_PER_DESC;
-		urb->interval = 1;
-		urb->transfer_buffer_length =
-			FRAME_SIZE_PER_DESC * FRAMES_PER_DESC;
 
 		for (fx = 0; fx < FRAMES_PER_DESC; fx++) {
 			urb->iso_frame_desc[fx].offset =
-- 
2.17.1

[PATCH 03/27] media: cx231xx: use usb_fill_XXX_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_XXX_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.

Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/cx231xx/cx231xx-audio.c | 30 ++++++++---------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/media/usb/cx231xx/cx231xx-audio.c b/drivers/media/usb/cx231xx/cx231xx-audio.c
index c4a84fb930b6..5d7a0c085902 100644
--- a/drivers/media/usb/cx231xx/cx231xx-audio.c
+++ b/drivers/media/usb/cx231xx/cx231xx-audio.c
@@ -281,6 +281,7 @@ static int cx231xx_init_audio_isoc(struct cx231xx *dev)
 	for (i = 0; i < CX231XX_AUDIO_BUFS; i++) {
 		struct urb *urb;
 		int j, k;
+		unsigned int pipe;
 
 		dev->adev.transfer_buffer[i] = kmalloc(sb_size, GFP_ATOMIC);
 		if (!dev->adev.transfer_buffer[i])
@@ -295,17 +296,12 @@ static int cx231xx_init_audio_isoc(struct cx231xx *dev)
 			}
 			return -ENOMEM;
 		}
-
-		urb->dev = dev->udev;
-		urb->context = dev;
-		urb->pipe = usb_rcvisocpipe(dev->udev,
-						dev->adev.end_point_addr);
+		pipe = usb_rcvisocpipe(dev->udev, dev->adev.end_point_addr);
+		usb_fill_int_urb(urb, dev->udev, pipe,
+				 dev->adev.transfer_buffer[i], sb_size,
+				 cx231xx_audio_isocirq, dev, 1);
 		urb->transfer_flags = URB_ISO_ASAP;
-		urb->transfer_buffer = dev->adev.transfer_buffer[i];
-		urb->interval = 1;
-		urb->complete = cx231xx_audio_isocirq;
 		urb->number_of_packets = CX231XX_ISO_NUM_AUDIO_PACKETS;
-		urb->transfer_buffer_length = sb_size;
 
 		for (j = k = 0; j < CX231XX_ISO_NUM_AUDIO_PACKETS;
 			j++, k += dev->adev.max_pkt_size) {
@@ -356,18 +352,12 @@ static int cx231xx_init_audio_bulk(struct cx231xx *dev)
 			}
 			return -ENOMEM;
 		}
-
-		urb->dev = dev->udev;
-		urb->context = dev;
-		urb->pipe = usb_rcvbulkpipe(dev->udev,
-						dev->adev.end_point_addr);
-		urb->transfer_flags = 0;
-		urb->transfer_buffer = dev->adev.transfer_buffer[i];
-		urb->complete = cx231xx_audio_bulkirq;
-		urb->transfer_buffer_length = sb_size;
-
+		usb_fill_bulk_urb(urb, dev->udev,
+				  usb_rcvbulkpipe(dev->udev,
+						  dev->adev.end_point_addr),
+				  dev->adev.transfer_buffer[i], sb_size,
+				  cx231xx_audio_bulkirq, dev);
 		dev->adev.urb[i] = urb;
-
 	}
 
 	for (i = 0; i < CX231XX_AUDIO_BUFS; i++) {
-- 
2.17.1

[PATCH 04/27] media: cx231xx: use irqsave() in USB's complete callback

[Sebastian Andrzej Siewior]

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/cx231xx/cx231xx-audio.c | 10 ++++++----
 drivers/media/usb/cx231xx/cx231xx-core.c  | 10 ++++++----
 drivers/media/usb/cx231xx/cx231xx-vbi.c   |  5 +++--
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/media/usb/cx231xx/cx231xx-audio.c b/drivers/media/usb/cx231xx/cx231xx-audio.c
index 5d7a0c085902..79a368d5f9dd 100644
--- a/drivers/media/usb/cx231xx/cx231xx-audio.c
+++ b/drivers/media/usb/cx231xx/cx231xx-audio.c
@@ -126,6 +126,7 @@ static void cx231xx_audio_isocirq(struct urb *urb)
 		stride = runtime->frame_bits >> 3;
 
 		for (i = 0; i < urb->number_of_packets; i++) {
+			unsigned long flags;
 			int length = urb->iso_frame_desc[i].actual_length /
 				     stride;
 			cp = (unsigned char *)urb->transfer_buffer +
@@ -148,7 +149,7 @@ static void cx231xx_audio_isocirq(struct urb *urb)
 				       length * stride);
 			}
 
-			snd_pcm_stream_lock(substream);
+			snd_pcm_stream_lock_irqsave(substream, flags);
 
 			dev->adev.hwptr_done_capture += length;
 			if (dev->adev.hwptr_done_capture >=
@@ -163,7 +164,7 @@ static void cx231xx_audio_isocirq(struct urb *urb)
 						runtime->period_size;
 				period_elapsed = 1;
 			}
-			snd_pcm_stream_unlock(substream);
+			snd_pcm_stream_unlock_irqrestore(substream, flags);
 		}
 		if (period_elapsed)
 			snd_pcm_period_elapsed(substream);
@@ -216,6 +217,7 @@ static void cx231xx_audio_bulkirq(struct urb *urb)
 		stride = runtime->frame_bits >> 3;
 
 		if (1) {
+			unsigned long flags;
 			int length = urb->actual_length /
 				     stride;
 			cp = (unsigned char *)urb->transfer_buffer;
@@ -234,7 +236,7 @@ static void cx231xx_audio_bulkirq(struct urb *urb)
 				       length * stride);
 			}
 
-			snd_pcm_stream_lock(substream);
+			snd_pcm_stream_lock_irqsave(substream, flags);
 
 			dev->adev.hwptr_done_capture += length;
 			if (dev->adev.hwptr_done_capture >=
@@ -249,7 +251,7 @@ static void cx231xx_audio_bulkirq(struct urb *urb)
 						runtime->period_size;
 				period_elapsed = 1;
 			}
-			snd_pcm_stream_unlock(substream);
+			snd_pcm_stream_unlock_irqrestore(substream, flags);
 		}
 		if (period_elapsed)
 			snd_pcm_period_elapsed(substream);
diff --git a/drivers/media/usb/cx231xx/cx231xx-core.c b/drivers/media/usb/cx231xx/cx231xx-core.c
index 53d846dea3d2..493c2dca6244 100644
--- a/drivers/media/usb/cx231xx/cx231xx-core.c
+++ b/drivers/media/usb/cx231xx/cx231xx-core.c
@@ -799,6 +799,7 @@ static void cx231xx_isoc_irq_callback(struct urb *urb)
 	struct cx231xx_video_mode *vmode =
 	    container_of(dma_q, struct cx231xx_video_mode, vidq);
 	struct cx231xx *dev = container_of(vmode, struct cx231xx, video_mode);
+	unsigned long flags;
 	int i;
 
 	switch (urb->status) {
@@ -815,9 +816,9 @@ static void cx231xx_isoc_irq_callback(struct urb *urb)
 	}
 
 	/* Copy data from URB */
-	spin_lock(&dev->video_mode.slock);
+	spin_lock_irqsave(&dev->video_mode.slock, flags);
 	dev->video_mode.isoc_ctl.isoc_copy(dev, urb);
-	spin_unlock(&dev->video_mode.slock);
+	spin_unlock_irqrestore(&dev->video_mode.slock, flags);
 
 	/* Reset urb buffers */
 	for (i = 0; i < urb->number_of_packets; i++) {
@@ -844,6 +845,7 @@ static void cx231xx_bulk_irq_callback(struct urb *urb)
 	struct cx231xx_video_mode *vmode =
 	    container_of(dma_q, struct cx231xx_video_mode, vidq);
 	struct cx231xx *dev = container_of(vmode, struct cx231xx, video_mode);
+	unsigned long flags;
 
 	switch (urb->status) {
 	case 0:		/* success */
@@ -862,9 +864,9 @@ static void cx231xx_bulk_irq_callback(struct urb *urb)
 	}
 
 	/* Copy data from URB */
-	spin_lock(&dev->video_mode.slock);
+	spin_lock_irqsave(&dev->video_mode.slock, flags);
 	dev->video_mode.bulk_ctl.bulk_copy(dev, urb);
-	spin_unlock(&dev->video_mode.slock);
+	spin_unlock_irqrestore(&dev->video_mode.slock, flags);
 
 	/* Reset urb buffers */
 	urb->status = usb_submit_urb(urb, GFP_ATOMIC);
diff --git a/drivers/media/usb/cx231xx/cx231xx-vbi.c b/drivers/media/usb/cx231xx/cx231xx-vbi.c
index b621cf1aa96b..920417baf893 100644
--- a/drivers/media/usb/cx231xx/cx231xx-vbi.c
+++ b/drivers/media/usb/cx231xx/cx231xx-vbi.c
@@ -305,6 +305,7 @@ static void cx231xx_irq_vbi_callback(struct urb *urb)
 	struct cx231xx_video_mode *vmode =
 	    container_of(dma_q, struct cx231xx_video_mode, vidq);
 	struct cx231xx *dev = container_of(vmode, struct cx231xx, vbi_mode);
+	unsigned long flags;
 
 	switch (urb->status) {
 	case 0:		/* success */
@@ -321,9 +322,9 @@ static void cx231xx_irq_vbi_callback(struct urb *urb)
 	}
 
 	/* Copy data from URB */
-	spin_lock(&dev->vbi_mode.slock);
+	spin_lock_irqsave(&dev->vbi_mode.slock, flags);
 	dev->vbi_mode.bulk_ctl.bulk_copy(dev, urb);
-	spin_unlock(&dev->vbi_mode.slock);
+	spin_unlock_irqrestore(&dev->vbi_mode.slock, flags);
 
 	/* Reset status */
 	urb->status = 0;
-- 
2.17.1

[PATCH 05/27] media: dvb-usb: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.

Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/dvb-usb/usb-urb.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/dvb-usb/usb-urb.c b/drivers/media/usb/dvb-usb/usb-urb.c
index 5e05963f4220..3c6b88e0a437 100644
--- a/drivers/media/usb/dvb-usb/usb-urb.c
+++ b/drivers/media/usb/dvb-usb/usb-urb.c
@@ -187,16 +187,14 @@ static int usb_isoc_urb_init(struct usb_data_stream *stream)
 		}
 
 		urb = stream->urb_list[i];
-
-		urb->dev = stream->udev;
-		urb->context = stream;
-		urb->complete = usb_urb_complete;
-		urb->pipe = usb_rcvisocpipe(stream->udev,stream->props.endpoint);
+		usb_fill_int_urb(urb, stream->udev,
+				 usb_rcvisocpipe(stream->udev,
+						 stream->props.endpoint),
+				 stream->buf_list[i], stream->buf_size,
+				 usb_urb_complete, stream,
+				 stream->props.u.isoc.interval);
 		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
-		urb->interval = stream->props.u.isoc.interval;
 		urb->number_of_packets = stream->props.u.isoc.framesperurb;
-		urb->transfer_buffer_length = stream->buf_size;
-		urb->transfer_buffer = stream->buf_list[i];
 		urb->transfer_dma = stream->dma_addr[i];
 
 		for (j = 0; j < stream->props.u.isoc.framesperurb; j++) {
-- 
2.17.1

[PATCH 06/27] media: dvb_usb_v2: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.

Cc: Antti Palosaari <crope@iki.fi>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/dvb-usb-v2/usb_urb.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/media/usb/dvb-usb-v2/usb_urb.c b/drivers/media/usb/dvb-usb-v2/usb_urb.c
index b0499f95ec45..e5e0bf96bad2 100644
--- a/drivers/media/usb/dvb-usb-v2/usb_urb.c
+++ b/drivers/media/usb/dvb-usb-v2/usb_urb.c
@@ -180,18 +180,17 @@ static int usb_urb_alloc_isoc_urbs(struct usb_data_stream *stream)
 		}
 
 		urb = stream->urb_list[i];
+		usb_fill_int_urb(urb, stream->udev,
+				 usb_rcvisocpipe(stream->udev,
+						 stream->props.endpoint),
+				 stream->buf_list[i],
+				 stream->props.u.isoc.framesize *
+				 stream->props.u.isoc.framesperurb,
+				 usb_urb_complete, stream,
+				 stream->props.u.isoc.interval);
 
-		urb->dev = stream->udev;
-		urb->context = stream;
-		urb->complete = usb_urb_complete;
-		urb->pipe = usb_rcvisocpipe(stream->udev,
-				stream->props.endpoint);
 		urb->transfer_flags = URB_ISO_ASAP | URB_FREE_BUFFER;
-		urb->interval = stream->props.u.isoc.interval;
 		urb->number_of_packets = stream->props.u.isoc.framesperurb;
-		urb->transfer_buffer_length = stream->props.u.isoc.framesize *
-				stream->props.u.isoc.framesperurb;
-		urb->transfer_buffer = stream->buf_list[i];
 
 		for (j = 0; j < stream->props.u.isoc.framesperurb; j++) {
 			urb->iso_frame_desc[j].offset = frame_offset;
-- 
2.17.1

[PATCH 07/27] media: em28xx-audio: use GFP_KERNEL for memory allocation during init

[Sebastian Andrzej Siewior]

As far as I can tell em28xx_audio_urb_init() is called once during
initialization from non atomic context. Memory allocation from
non atomic context should use GFP_KERNEL to avoid using emergency pool
for memory allocation.
Use GFP_KERNEL for memory allocation.

Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/em28xx/em28xx-audio.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
index 8e799ae1df69..bb510cf8fbbb 100644
--- a/drivers/media/usb/em28xx/em28xx-audio.c
+++ b/drivers/media/usb/em28xx/em28xx-audio.c
@@ -842,11 +842,11 @@ static int em28xx_audio_urb_init(struct em28xx *dev)
 
 	dev->adev.transfer_buffer = kcalloc(num_urb,
 					    sizeof(*dev->adev.transfer_buffer),
-					    GFP_ATOMIC);
+					    GFP_KERNEL);
 	if (!dev->adev.transfer_buffer)
 		return -ENOMEM;
 
-	dev->adev.urb = kcalloc(num_urb, sizeof(*dev->adev.urb), GFP_ATOMIC);
+	dev->adev.urb = kcalloc(num_urb, sizeof(*dev->adev.urb), GFP_KERNEL);
 	if (!dev->adev.urb) {
 		kfree(dev->adev.transfer_buffer);
 		return -ENOMEM;
@@ -859,14 +859,14 @@ static int em28xx_audio_urb_init(struct em28xx *dev)
 		int j, k;
 		void *buf;
 
-		urb = usb_alloc_urb(npackets, GFP_ATOMIC);
+		urb = usb_alloc_urb(npackets, GFP_KERNEL);
 		if (!urb) {
 			em28xx_audio_free_urb(dev);
 			return -ENOMEM;
 		}
 		dev->adev.urb[i] = urb;
 
-		buf = usb_alloc_coherent(udev, npackets * ep_size, GFP_ATOMIC,
+		buf = usb_alloc_coherent(udev, npackets * ep_size, GFP_KERNEL,
 					 &urb->transfer_dma);
 		if (!buf) {
 			dev_err(&dev->intf->dev,
-- 
2.17.1

[PATCH 08/27] media: em28xx-audio: use irqsave() in USB's complete callback

[Sebastian Andrzej Siewior]

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/em28xx/em28xx-audio.c | 5 +++--
 drivers/media/usb/em28xx/em28xx-core.c  | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
index bb510cf8fbbb..7f75002927a1 100644
--- a/drivers/media/usb/em28xx/em28xx-audio.c
+++ b/drivers/media/usb/em28xx/em28xx-audio.c
@@ -116,6 +116,7 @@ static void em28xx_audio_isocirq(struct urb *urb)
 		stride = runtime->frame_bits >> 3;
 
 		for (i = 0; i < urb->number_of_packets; i++) {
+			unsigned long flags;
 			int length =
 			    urb->iso_frame_desc[i].actual_length / stride;
 			cp = (unsigned char *)urb->transfer_buffer +
@@ -137,7 +138,7 @@ static void em28xx_audio_isocirq(struct urb *urb)
 				       length * stride);
 			}
 
-			snd_pcm_stream_lock(substream);
+			snd_pcm_stream_lock_irqsave(substream, flags);
 
 			dev->adev.hwptr_done_capture += length;
 			if (dev->adev.hwptr_done_capture >=
@@ -153,7 +154,7 @@ static void em28xx_audio_isocirq(struct urb *urb)
 				period_elapsed = 1;
 			}
 
-			snd_pcm_stream_unlock(substream);
+			snd_pcm_stream_unlock_irqrestore(substream, flags);
 		}
 		if (period_elapsed)
 			snd_pcm_period_elapsed(substream);
diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index f70845e7d8c6..ec6e5152afd9 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -777,6 +777,7 @@ EXPORT_SYMBOL_GPL(em28xx_set_mode);
 static void em28xx_irq_callback(struct urb *urb)
 {
 	struct em28xx *dev = urb->context;
+	unsigned long flags;
 	int i;
 
 	switch (urb->status) {
@@ -793,9 +794,9 @@ static void em28xx_irq_callback(struct urb *urb)
 	}
 
 	/* Copy data from URB */
-	spin_lock(&dev->slock);
+	spin_lock_irqsave(&dev->slock, flags);
 	dev->usb_ctl.urb_data_copy(dev, urb);
-	spin_unlock(&dev->slock);
+	spin_unlock_irqrestore(&dev->slock, flags);
 
 	/* Reset urb buffers */
 	for (i = 0; i < urb->number_of_packets; i++) {
-- 
2.17.1

[PATCH 09/27] media: em28xx-audio: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an URB. A
grep for members of the struct (like ->complete) reveal lots of other
things, too.

Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/em28xx/em28xx-audio.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
index 7f75002927a1..4251c9f8e07a 100644
--- a/drivers/media/usb/em28xx/em28xx-audio.c
+++ b/drivers/media/usb/em28xx/em28xx-audio.c
@@ -877,15 +877,13 @@ static int em28xx_audio_urb_init(struct em28xx *dev)
 		}
 		dev->adev.transfer_buffer[i] = buf;
 
-		urb->dev = udev;
-		urb->context = dev;
-		urb->pipe = usb_rcvisocpipe(udev, EM28XX_EP_AUDIO);
+		usb_fill_int_urb(urb, udev,
+				 usb_rcvisocpipe(udev, EM28XX_EP_AUDIO),
+				 buf, ep_size * npackets, em28xx_audio_isocirq,
+				 dev, ep->bInterval);
+
 		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
-		urb->transfer_buffer = buf;
-		urb->interval = interval;
-		urb->complete = em28xx_audio_isocirq;
 		urb->number_of_packets = npackets;
-		urb->transfer_buffer_length = ep_size * npackets;
 
 		for (j = k = 0; j < npackets; j++, k += ep_size) {
 			urb->iso_frame_desc[j].offset = k;
-- 
2.17.1

[PATCH 10/27] media: go7007: use irqsave() in USB's complete callback

[Sebastian Andrzej Siewior]

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/go7007/go7007-driver.c |  9 +++++----
 drivers/media/usb/go7007/snd-go7007.c    | 11 ++++++-----
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/media/usb/go7007/go7007-driver.c b/drivers/media/usb/go7007/go7007-driver.c
index 05b1126f263e..62aeebcdd7f7 100644
--- a/drivers/media/usb/go7007/go7007-driver.c
+++ b/drivers/media/usb/go7007/go7007-driver.c
@@ -448,13 +448,14 @@ static struct go7007_buffer *frame_boundary(struct go7007 *go, struct go7007_buf
 {
 	u32 *bytesused;
 	struct go7007_buffer *vb_tmp = NULL;
+	unsigned long flags;
 
 	if (vb == NULL) {
-		spin_lock(&go->spinlock);
+		spin_lock_irqsave(&go->spinlock, flags);
 		if (!list_empty(&go->vidq_active))
 			vb = go->active_buf =
 				list_first_entry(&go->vidq_active, struct go7007_buffer, list);
-		spin_unlock(&go->spinlock);
+		spin_unlock_irqrestore(&go->spinlock, flags);
 		go->next_seq++;
 		return vb;
 	}
@@ -468,7 +469,7 @@ static struct go7007_buffer *frame_boundary(struct go7007 *go, struct go7007_buf
 
 	vb->vb.vb2_buf.timestamp = ktime_get_ns();
 	vb_tmp = vb;
-	spin_lock(&go->spinlock);
+	spin_lock_irqsave(&go->spinlock, flags);
 	list_del(&vb->list);
 	if (list_empty(&go->vidq_active))
 		vb = NULL;
@@ -476,7 +477,7 @@ static struct go7007_buffer *frame_boundary(struct go7007 *go, struct go7007_buf
 		vb = list_first_entry(&go->vidq_active,
 				struct go7007_buffer, list);
 	go->active_buf = vb;
-	spin_unlock(&go->spinlock);
+	spin_unlock_irqrestore(&go->spinlock, flags);
 	vb2_buffer_done(&vb_tmp->vb.vb2_buf, VB2_BUF_STATE_DONE);
 	return vb;
 }
diff --git a/drivers/media/usb/go7007/snd-go7007.c b/drivers/media/usb/go7007/snd-go7007.c
index f84a2130f033..137fc253b122 100644
--- a/drivers/media/usb/go7007/snd-go7007.c
+++ b/drivers/media/usb/go7007/snd-go7007.c
@@ -75,13 +75,14 @@ static void parse_audio_stream_data(struct go7007 *go, u8 *buf, int length)
 	struct go7007_snd *gosnd = go->snd_context;
 	struct snd_pcm_runtime *runtime = gosnd->substream->runtime;
 	int frames = bytes_to_frames(runtime, length);
+	unsigned long flags;
 
-	spin_lock(&gosnd->lock);
+	spin_lock_irqsave(&gosnd->lock, flags);
 	gosnd->hw_ptr += frames;
 	if (gosnd->hw_ptr >= runtime->buffer_size)
 		gosnd->hw_ptr -= runtime->buffer_size;
 	gosnd->avail += frames;
-	spin_unlock(&gosnd->lock);
+	spin_unlock_irqrestore(&gosnd->lock, flags);
 	if (gosnd->w_idx + length > runtime->dma_bytes) {
 		int cpy = runtime->dma_bytes - gosnd->w_idx;
 
@@ -92,13 +93,13 @@ static void parse_audio_stream_data(struct go7007 *go, u8 *buf, int length)
 	}
 	memcpy(runtime->dma_area + gosnd->w_idx, buf, length);
 	gosnd->w_idx += length;
-	spin_lock(&gosnd->lock);
+	spin_lock_irqsave(&gosnd->lock, flags);
 	if (gosnd->avail < runtime->period_size) {
-		spin_unlock(&gosnd->lock);
+		spin_unlock_irqrestore(&gosnd->lock, flags);
 		return;
 	}
 	gosnd->avail -= runtime->period_size;
-	spin_unlock(&gosnd->lock);
+	spin_unlock_irqrestore(&gosnd->lock, flags);
 	if (gosnd->capturing)
 		snd_pcm_period_elapsed(gosnd->substream);
 }
-- 
2.17.1

[PATCH 11/27] media: gspca: benq: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an URB. A
grep for members of the struct (like ->complete) reveal lots of other
things, too.

Cc: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/gspca/benq.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/media/usb/gspca/benq.c b/drivers/media/usb/gspca/benq.c
index 8a8db5eb6d5f..a59c92cb6a3b 100644
--- a/drivers/media/usb/gspca/benq.c
+++ b/drivers/media/usb/gspca/benq.c
@@ -90,28 +90,27 @@ static int sd_start(struct gspca_dev *gspca_dev)
 #define SD_PKT_SZ 64
 #define SD_NPKT 32
 	for (n = 0; n < 4; n++) {
+		void *buf;
+
 		urb = usb_alloc_urb(SD_NPKT, GFP_KERNEL);
 		if (!urb)
 			return -ENOMEM;
 		gspca_dev->urb[n] = urb;
-		urb->transfer_buffer = usb_alloc_coherent(gspca_dev->dev,
-						SD_PKT_SZ * SD_NPKT,
-						GFP_KERNEL,
-						&urb->transfer_dma);
+		buf = usb_alloc_coherent(gspca_dev->dev, SD_PKT_SZ * SD_NPKT,
+					 GFP_KERNEL, &urb->transfer_dma);
 
-		if (urb->transfer_buffer == NULL) {
+		if (buf == NULL) {
 			pr_err("usb_alloc_coherent failed\n");
 			return -ENOMEM;
 		}
-		urb->dev = gspca_dev->dev;
-		urb->context = gspca_dev;
-		urb->transfer_buffer_length = SD_PKT_SZ * SD_NPKT;
-		urb->pipe = usb_rcvisocpipe(gspca_dev->dev,
-					n & 1 ? 0x82 : 0x83);
+		usb_fill_int_urb(urb, gspca_dev->dev,
+				 usb_rcvisocpipe(gspca_dev->dev,
+						 n & 1 ? 0x82 : 0x83),
+				 buf, SD_PKT_SZ * SD_NPKT, sd_isoc_irq,
+				 gspca_dev, 1);
+
 		urb->transfer_flags = URB_ISO_ASAP
 					| URB_NO_TRANSFER_DMA_MAP;
-		urb->interval = 1;
-		urb->complete = sd_isoc_irq;
 		urb->number_of_packets = SD_NPKT;
 		for (i = 0; i < SD_NPKT; i++) {
 			urb->iso_frame_desc[i].length = SD_PKT_SZ;
-- 
2.17.1

[PATCH 12/27] media: gspca: gspca: use usb_fill_XXX_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an URB. A
grep for members of the struct (like ->complete) reveal lots of other
things, too.

Cc: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/gspca/gspca.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/media/usb/gspca/gspca.c b/drivers/media/usb/gspca/gspca.c
index 57aa521e16b1..4cc9135829df 100644
--- a/drivers/media/usb/gspca/gspca.c
+++ b/drivers/media/usb/gspca/gspca.c
@@ -698,39 +698,40 @@ static int create_urbs(struct gspca_dev *gspca_dev,
 	}
 
 	for (n = 0; n < nurbs; n++) {
+		void *buf;
+
 		urb = usb_alloc_urb(npkt, GFP_KERNEL);
 		if (!urb)
 			return -ENOMEM;
 		gspca_dev->urb[n] = urb;
-		urb->transfer_buffer = usb_alloc_coherent(gspca_dev->dev,
+		buf = usb_alloc_coherent(gspca_dev->dev,
 						bsize,
 						GFP_KERNEL,
 						&urb->transfer_dma);
 
-		if (urb->transfer_buffer == NULL) {
+		if (buf == NULL) {
 			pr_err("usb_alloc_coherent failed\n");
 			return -ENOMEM;
 		}
-		urb->dev = gspca_dev->dev;
-		urb->context = gspca_dev;
-		urb->transfer_buffer_length = bsize;
 		if (npkt != 0) {		/* ISOC */
-			urb->pipe = usb_rcvisocpipe(gspca_dev->dev,
-						    ep->desc.bEndpointAddress);
+			usb_fill_int_urb(urb, gspca_dev->dev,
+					 usb_rcvisocpipe(gspca_dev->dev, ep->desc.bEndpointAddress),
+					 buf, bsize, isoc_irq, gspca_dev,
+					 ep->desc.bInterval);
+
 			urb->transfer_flags = URB_ISO_ASAP
 					| URB_NO_TRANSFER_DMA_MAP;
-			urb->interval = 1 << (ep->desc.bInterval - 1);
-			urb->complete = isoc_irq;
 			urb->number_of_packets = npkt;
 			for (i = 0; i < npkt; i++) {
 				urb->iso_frame_desc[i].length = psize;
 				urb->iso_frame_desc[i].offset = psize * i;
 			}
 		} else {		/* bulk */
-			urb->pipe = usb_rcvbulkpipe(gspca_dev->dev,
-						ep->desc.bEndpointAddress);
+			usb_fill_bulk_urb(urb, gspca_dev->dev,
+					 usb_rcvbulkpipe(gspca_dev->dev, ep->desc.bEndpointAddress),
+					 buf, bsize, bulk_irq, gspca_dev);
+
 			urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
-			urb->complete = bulk_irq;
 		}
 	}
 	return 0;
-- 
2.17.1

[PATCH 13/27] media: gspca: konica: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an URB. A
grep for members of the struct (like ->complete) reveal lots of other
things, too.

Cc: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/gspca/konica.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/media/usb/gspca/konica.c b/drivers/media/usb/gspca/konica.c
index 989ae997f66d..6ab96bcb338f 100644
--- a/drivers/media/usb/gspca/konica.c
+++ b/drivers/media/usb/gspca/konica.c
@@ -203,6 +203,8 @@ static int sd_start(struct gspca_dev *gspca_dev)
 #endif
 #define SD_NPKT 32
 	for (n = 0; n < 4; n++) {
+		void *buf;
+
 		i = n & 1 ? 0 : 1;
 		packet_size =
 			le16_to_cpu(alt->endpoint[i].desc.wMaxPacketSize);
@@ -210,24 +212,21 @@ static int sd_start(struct gspca_dev *gspca_dev)
 		if (!urb)
 			return -ENOMEM;
 		gspca_dev->urb[n] = urb;
-		urb->transfer_buffer = usb_alloc_coherent(gspca_dev->dev,
-						packet_size * SD_NPKT,
-						GFP_KERNEL,
-						&urb->transfer_dma);
-		if (urb->transfer_buffer == NULL) {
+		buf = usb_alloc_coherent(gspca_dev->dev, packet_size * SD_NPKT,
+					 GFP_KERNEL, &urb->transfer_dma);
+		if (buf == NULL) {
 			pr_err("usb_buffer_alloc failed\n");
 			return -ENOMEM;
 		}
 
-		urb->dev = gspca_dev->dev;
-		urb->context = gspca_dev;
-		urb->transfer_buffer_length = packet_size * SD_NPKT;
-		urb->pipe = usb_rcvisocpipe(gspca_dev->dev,
-					n & 1 ? 0x81 : 0x82);
+		usb_fill_int_urb(urb, gspca_dev->dev,
+				 usb_rcvisocpipe(gspca_dev->dev,
+						 n & 1 ? 0x81 : 0x82),
+				 buf, packet_size * SD_NPKT, sd_isoc_irq,
+				 gspca_dev, 1);
+
 		urb->transfer_flags = URB_ISO_ASAP
 					| URB_NO_TRANSFER_DMA_MAP;
-		urb->interval = 1;
-		urb->complete = sd_isoc_irq;
 		urb->number_of_packets = SD_NPKT;
 		for (i = 0; i < SD_NPKT; i++) {
 			urb->iso_frame_desc[i].length = packet_size;
-- 
2.17.1

[PATCH 14/27] media: gspca: sq930x: use GFP_KERNEL in sd_dq_callback()

[Sebastian Andrzej Siewior]

The context in which sd_dq_callback() is non atomic, there is even
msleep() at the end of the function. There is no need to use GFP_ATOMIC
here - use GFP_KERNEL instead.

Cc: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/gspca/sq930x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/gspca/sq930x.c b/drivers/media/usb/gspca/sq930x.c
index d7cbcf2b3947..e15b45f022e1 100644
--- a/drivers/media/usb/gspca/sq930x.c
+++ b/drivers/media/usb/gspca/sq930x.c
@@ -1044,7 +1044,7 @@ static void sd_dq_callback(struct gspca_dev *gspca_dev)
 			v4l2_ctrl_g_ctrl(sd->gain));
 
 	gspca_dev->cam.bulk_nurbs = 1;
-	ret = usb_submit_urb(gspca_dev->urb[0], GFP_ATOMIC);
+	ret = usb_submit_urb(gspca_dev->urb[0], GFP_KERNEL);
 	if (ret < 0)
 		pr_err("sd_dq_callback() err %d\n", ret);
 
-- 
2.17.1

[PATCH 15/27] media: msi2500: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an URB. A
grep for members of the struct (like ->complete) reveal lots of other
things, too.

Cc: Antti Palosaari <crope@iki.fi>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/msi2500/msi2500.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/media/usb/msi2500/msi2500.c b/drivers/media/usb/msi2500/msi2500.c
index 65ef755adfdc..7ac6284248ce 100644
--- a/drivers/media/usb/msi2500/msi2500.c
+++ b/drivers/media/usb/msi2500/msi2500.c
@@ -507,6 +507,8 @@ static int msi2500_isoc_init(struct msi2500_dev *dev)
 
 	/* 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) {
 			msi2500_isoc_cleanup(dev);
@@ -515,22 +517,19 @@ static int msi2500_isoc_init(struct msi2500_dev *dev)
 		dev->urbs[i] = urb;
 		dev_dbg(dev->dev, "Allocated URB at 0x%p\n", urb);
 
-		urb->interval = 1;
-		urb->dev = dev->udev;
-		urb->pipe = usb_rcvisocpipe(dev->udev, 0x81);
-		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
-		urb->transfer_buffer = usb_alloc_coherent(dev->udev,
-				ISO_BUFFER_SIZE,
-				GFP_KERNEL, &urb->transfer_dma);
-		if (urb->transfer_buffer == NULL) {
+
+		buf = usb_alloc_coherent(dev->udev, ISO_BUFFER_SIZE, GFP_KERNEL,
+					 &urb->transfer_dma);
+		if (buf == NULL) {
 			dev_err(dev->dev,
 				"Failed to allocate urb buffer %d\n", i);
 			msi2500_isoc_cleanup(dev);
 			return -ENOMEM;
 		}
-		urb->transfer_buffer_length = ISO_BUFFER_SIZE;
-		urb->complete = msi2500_isoc_handler;
-		urb->context = dev;
+		usb_fill_int_urb(urb, dev->udev,
+				 usb_rcvisocpipe(dev->udev, 0x81), buf,
+				 ISO_BUFFER_SIZE, msi2500_isoc_handler, dev, 1);
+		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
 		urb->start_frame = 0;
 		urb->number_of_packets = ISO_FRAMES_PER_DESC;
 		for (j = 0; j < ISO_FRAMES_PER_DESC; j++) {
-- 
2.17.1

[PATCH 16/27] media: pwc: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an URB. A
grep for members of the struct (like ->complete) reveal lots of other
things, too.

Cc: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/pwc/pwc-if.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/media/usb/pwc/pwc-if.c b/drivers/media/usb/pwc/pwc-if.c
index 54b036d39c5b..8af72b0a607e 100644
--- a/drivers/media/usb/pwc/pwc-if.c
+++ b/drivers/media/usb/pwc/pwc-if.c
@@ -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,23 +418,18 @@ 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;
+		usb_fill_int_urb(urb, udev,
+			usb_rcvisocpipe(udev, pdev->vendpoint),
+			buf, ISO_BUFFER_SIZE, pwc_isoc_handler, pdev, 1);
+
 		urb->start_frame = 0;
 		urb->number_of_packets = ISO_FRAMES_PER_DESC;
 		for (j = 0; j < ISO_FRAMES_PER_DESC; j++) {
-- 
2.17.1

[PATCH 17/27] media: stk1160: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an URB. A
grep for members of the struct (like ->complete) reveal lots of other
things, too.

Cc: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/stk1160/stk1160-video.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/media/usb/stk1160/stk1160-video.c b/drivers/media/usb/stk1160/stk1160-video.c
index 2811f612820f..2dd2cb9079d7 100644
--- a/drivers/media/usb/stk1160/stk1160-video.c
+++ b/drivers/media/usb/stk1160/stk1160-video.c
@@ -481,13 +481,10 @@ int stk1160_alloc_isoc(struct stk1160 *dev)
 		/*
 		 * FIXME: Where can I get the endpoint?
 		 */
-		urb->dev = dev->udev;
-		urb->pipe = usb_rcvisocpipe(dev->udev, STK1160_EP_VIDEO);
-		urb->transfer_buffer = dev->isoc_ctl.transfer_buffer[i];
-		urb->transfer_buffer_length = sb_size;
-		urb->complete = stk1160_isoc_irq;
-		urb->context = dev;
-		urb->interval = 1;
+		usb_fill_int_urb(urb, dev->udev,
+				 usb_rcvisocpipe(dev->udev, STK1160_EP_VIDEO),
+				 dev->isoc_ctl.transfer_buffer[i], sb_size,
+				 stk1160_isoc_irq, dev, 1);
 		urb->start_frame = 0;
 		urb->number_of_packets = max_packets;
 #ifndef CONFIG_DMA_NONCOHERENT
-- 
2.17.1

[PATCH 18/27] media: stkwebcam: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an URB. A
grep for members of the struct (like ->complete) reveal lots of other
things, too.

Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/stkwebcam/stk-webcam.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/media/usb/stkwebcam/stk-webcam.c b/drivers/media/usb/stkwebcam/stk-webcam.c
index 5accb5241072..0cdf23180095 100644
--- a/drivers/media/usb/stkwebcam/stk-webcam.c
+++ b/drivers/media/usb/stkwebcam/stk-webcam.c
@@ -460,14 +460,11 @@ static int stk_prepare_iso(struct stk_camera *dev)
 			usb_kill_urb(dev->isobufs[i].urb);
 			urb = dev->isobufs[i].urb;
 		}
-		urb->interval = 1;
-		urb->dev = udev;
-		urb->pipe = usb_rcvisocpipe(udev, dev->isoc_ep);
+		usb_fill_int_urb(urb, udev, usb_rcvisocpipe(udev, dev->isoc_ep),
+				 dev->isobufs[i].data, ISO_BUFFER_SIZE,
+				 stk_isoc_handler, dev, 1);
+
 		urb->transfer_flags = URB_ISO_ASAP;
-		urb->transfer_buffer = dev->isobufs[i].data;
-		urb->transfer_buffer_length = ISO_BUFFER_SIZE;
-		urb->complete = stk_isoc_handler;
-		urb->context = dev;
 		urb->start_frame = 0;
 		urb->number_of_packets = ISO_FRAMES_PER_DESC;
 
-- 
2.17.1

[PATCH 19/27] media: tm6000: use irqsave() in USB's complete callback

[Sebastian Andrzej Siewior]

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/tm6000/tm6000-video.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/tm6000/tm6000-video.c b/drivers/media/usb/tm6000/tm6000-video.c
index 96055de6e8ce..7d268f2404e1 100644
--- a/drivers/media/usb/tm6000/tm6000-video.c
+++ b/drivers/media/usb/tm6000/tm6000-video.c
@@ -419,6 +419,7 @@ static void tm6000_irq_callback(struct urb *urb)
 {
 	struct tm6000_dmaqueue  *dma_q = urb->context;
 	struct tm6000_core *dev = container_of(dma_q, struct tm6000_core, vidq);
+	unsigned long flags;
 	int i;
 
 	switch (urb->status) {
@@ -436,9 +437,9 @@ static void tm6000_irq_callback(struct urb *urb)
 		break;
 	}
 
-	spin_lock(&dev->slock);
+	spin_lock_irqsave(&dev->slock, flags);
 	tm6000_isoc_copy(urb);
-	spin_unlock(&dev->slock);
+	spin_unlock_irqrestore(&dev->slock, flags);
 
 	/* Reset urb buffers */
 	for (i = 0; i < urb->number_of_packets; i++) {
-- 
2.17.1

[PATCH 20/27] media: ttusb-budget: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an URB. A
grep for members of the struct (like ->complete) reveal lots of other
things, too.

Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c b/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
index eed56895c2b9..493fb44586e9 100644
--- a/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
+++ b/drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c
@@ -846,16 +846,12 @@ static int ttusb_start_iso_xfer(struct ttusb *ttusb)
 		int frame_offset = 0;
 		struct urb *urb = ttusb->iso_urb[i];
 
-		urb->dev = ttusb->dev;
-		urb->context = ttusb;
-		urb->complete = ttusb_iso_irq;
-		urb->pipe = ttusb->isoc_in_pipe;
+		usb_fill_int_urb(urb, ttusb->dev, ttusb->isoc_in_pipe,
+				 ttusb->iso_buffer + buffer_offset,
+				 ISO_FRAME_SIZE * FRAMES_PER_ISO_BUF,
+				 ttusb_iso_irq, ttusb, 1);
 		urb->transfer_flags = URB_ISO_ASAP;
-		urb->interval = 1;
 		urb->number_of_packets = FRAMES_PER_ISO_BUF;
-		urb->transfer_buffer_length =
-		    ISO_FRAME_SIZE * FRAMES_PER_ISO_BUF;
-		urb->transfer_buffer = ttusb->iso_buffer + buffer_offset;
 		buffer_offset += ISO_FRAME_SIZE * FRAMES_PER_ISO_BUF;
 
 		for (j = 0; j < FRAMES_PER_ISO_BUF; j++) {
-- 
2.17.1

[PATCH 21/27] media: ttusb-dec: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an URB. A
grep for members of the struct (like ->complete) reveal lots of other
things, too.

Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/ttusb-dec/ttusb_dec.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/ttusb-dec/ttusb_dec.c b/drivers/media/usb/ttusb-dec/ttusb_dec.c
index 44ca66cb9b8f..4c094364873d 100644
--- a/drivers/media/usb/ttusb-dec/ttusb_dec.c
+++ b/drivers/media/usb/ttusb-dec/ttusb_dec.c
@@ -858,16 +858,13 @@ static void ttusb_dec_setup_urbs(struct ttusb_dec *dec)
 		int frame_offset = 0;
 		struct urb *urb = dec->iso_urb[i];
 
-		urb->dev = dec->udev;
-		urb->context = dec;
-		urb->complete = ttusb_dec_process_urb;
-		urb->pipe = dec->in_pipe;
+		usb_fill_int_urb(urb, dec->udev, dec->in_pipe,
+				 dec->iso_buffer + buffer_offset,
+				 ISO_FRAME_SIZE * FRAMES_PER_ISO_BUF,
+				 ttusb_dec_process_urb, dec, 1);
+
 		urb->transfer_flags = URB_ISO_ASAP;
-		urb->interval = 1;
 		urb->number_of_packets = FRAMES_PER_ISO_BUF;
-		urb->transfer_buffer_length = ISO_FRAME_SIZE *
-					      FRAMES_PER_ISO_BUF;
-		urb->transfer_buffer = dec->iso_buffer + buffer_offset;
 		buffer_offset += ISO_FRAME_SIZE * FRAMES_PER_ISO_BUF;
 
 		for (j = 0; j < FRAMES_PER_ISO_BUF; j++) {
-- 
2.17.1

[PATCH 22/27] media: ttusbir: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.

Cc: Sean Young <sean@mess.org>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/rc/ttusbir.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/media/rc/ttusbir.c b/drivers/media/rc/ttusbir.c
index aafea3c5170b..6a7c9b50ff5a 100644
--- a/drivers/media/rc/ttusbir.c
+++ b/drivers/media/rc/ttusbir.c
@@ -257,10 +257,6 @@ static int ttusbir_probe(struct usb_interface *intf,
 			goto out;
 		}
 
-		urb->dev = tt->udev;
-		urb->context = tt;
-		urb->pipe = usb_rcvisocpipe(tt->udev, tt->iso_in_endp);
-		urb->interval = 1;
 		buffer = usb_alloc_coherent(tt->udev, 128, GFP_KERNEL,
 						&urb->transfer_dma);
 		if (!buffer) {
@@ -268,11 +264,11 @@ static int ttusbir_probe(struct usb_interface *intf,
 			ret = -ENOMEM;
 			goto out;
 		}
+		usb_fill_int_urb(urb, tt->udev,
+				 usb_rcvisocpipe(tt->udev, tt->iso_in_endp),
+				 buffer, 128, ttusbir_urb_complete, tt, 1);
 		urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP | URB_ISO_ASAP;
-		urb->transfer_buffer = buffer;
-		urb->complete = ttusbir_urb_complete;
 		urb->number_of_packets = 8;
-		urb->transfer_buffer_length = 128;
 
 		for (j = 0; j < 8; j++) {
 			urb->iso_frame_desc[j].offset = j * 16;
-- 
2.17.1

Re: [PATCH 22/27] media: ttusbir: use usb_fill_int_urb()

[Sean Young]

On Wed, Jun 20, 2018 at 01:01:00PM +0200, Sebastian Andrzej Siewior wrote:
> Using usb_fill_int_urb() helps to find code which initializes an
> URB. A grep for members of the struct (like ->complete) reveal lots
> of other things, too.

The reason I didn't use usb_fill_int_urb() is that is not an interrupt
urb, it's a iso urb. I'm not sure what affect the interval handling
in usb_fill_int_urb() will have on this.


Sean

> 
> Cc: Sean Young <sean@mess.org>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/media/rc/ttusbir.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/rc/ttusbir.c b/drivers/media/rc/ttusbir.c
> index aafea3c5170b..6a7c9b50ff5a 100644
> --- a/drivers/media/rc/ttusbir.c
> +++ b/drivers/media/rc/ttusbir.c
> @@ -257,10 +257,6 @@ static int ttusbir_probe(struct usb_interface *intf,
>  			goto out;
>  		}
>  
> -		urb->dev = tt->udev;
> -		urb->context = tt;
> -		urb->pipe = usb_rcvisocpipe(tt->udev, tt->iso_in_endp);
> -		urb->interval = 1;
>  		buffer = usb_alloc_coherent(tt->udev, 128, GFP_KERNEL,
>  						&urb->transfer_dma);
>  		if (!buffer) {
> @@ -268,11 +264,11 @@ static int ttusbir_probe(struct usb_interface *intf,
>  			ret = -ENOMEM;
>  			goto out;
>  		}
> +		usb_fill_int_urb(urb, tt->udev,
> +				 usb_rcvisocpipe(tt->udev, tt->iso_in_endp),
> +				 buffer, 128, ttusbir_urb_complete, tt, 1);
>  		urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP | URB_ISO_ASAP;
> -		urb->transfer_buffer = buffer;
> -		urb->complete = ttusbir_urb_complete;
>  		urb->number_of_packets = 8;
> -		urb->transfer_buffer_length = 128;
>  
>  		for (j = 0; j < 8; j++) {
>  			urb->iso_frame_desc[j].offset = j * 16;
> -- 
> 2.17.1

Re: [PATCH 22/27] media: ttusbir: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

On 2018-06-20 21:50:49 [+0100], Sean Young wrote:
> On Wed, Jun 20, 2018 at 01:01:00PM +0200, Sebastian Andrzej Siewior wrote:
> > Using usb_fill_int_urb() helps to find code which initializes an
> > URB. A grep for members of the struct (like ->complete) reveal lots
> > of other things, too.
> 
> The reason I didn't use usb_fill_int_urb() is that is not an interrupt
> urb, it's a iso urb. I'm not sure what affect the interval handling
> in usb_fill_int_urb() will have on this.

It is wrong, I had false memory on regarding ISO/FS. There will be a
respin a of the usb_fill* patches once we have usb_fill_iso_urb().

Mauro, could you please consider only the !fill patches from the series:
  [PATCH 04/27] media: cx231xx: use irqsave() in USB's complete callback
  [PATCH 07/27] media: em28xx-audio: use GFP_KERNEL for memory allocation during init
  [PATCH 08/27] media: em28xx-audio: use irqsave() in USB's complete callback
  [PATCH 10/27] media: go7007: use irqsave() in USB's complete callback
  [PATCH 14/27] media: gspca: sq930x: use GFP_KERNEL in sd_dq_callback()
  [PATCH 19/27] media: tm6000: use irqsave() in USB's complete callback
  [PATCH 23/27] media: usbtv: use irqsave() in USB's complete callback
  [PATCH 25/27] media: usbvision: remove time_in_irq

> Sean

Sebastian

[PATCH 23/27] media: usbtv: use irqsave() in USB's complete callback

[Sebastian Andrzej Siewior]

The USB completion callback does not disable interrupts while acquiring
the lock. We want to remove the local_irq_disable() invocation from
__usb_hcd_giveback_urb() and therefore it is required for the callback
handler to disable the interrupts while acquiring the lock.
The callback may be invoked either in IRQ or BH context depending on the
USB host controller.
Use the _irqsave() variant of the locking primitives.

Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/usbtv/usbtv-audio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/usbtv/usbtv-audio.c b/drivers/media/usb/usbtv/usbtv-audio.c
index 2c2ca77fa01f..4ce38246ed64 100644
--- a/drivers/media/usb/usbtv/usbtv-audio.c
+++ b/drivers/media/usb/usbtv/usbtv-audio.c
@@ -126,6 +126,7 @@ static void usbtv_audio_urb_received(struct urb *urb)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	size_t i, frame_bytes, chunk_length, buffer_pos, period_pos;
 	int period_elapsed;
+	unsigned long flags;
 	void *urb_current;
 
 	switch (urb->status) {
@@ -179,12 +180,12 @@ static void usbtv_audio_urb_received(struct urb *urb)
 		}
 	}
 
-	snd_pcm_stream_lock(substream);
+	snd_pcm_stream_lock_irqsave(substream, flags);
 
 	chip->snd_buffer_pos = buffer_pos;
 	chip->snd_period_pos = period_pos;
 
-	snd_pcm_stream_unlock(substream);
+	snd_pcm_stream_unlock_irqrestore(substream, flags);
 
 	if (period_elapsed)
 		snd_pcm_period_elapsed(substream);
-- 
2.17.1

[PATCH 24/27] media: usbtv: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an URB. A
grep for members of the struct (like ->complete) reveal lots of other
things, too.

Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/usbtv/usbtv-video.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/media/usb/usbtv/usbtv-video.c b/drivers/media/usb/usbtv/usbtv-video.c
index 36a9a4017185..2be4935b7afe 100644
--- a/drivers/media/usb/usbtv/usbtv-video.c
+++ b/drivers/media/usb/usbtv/usbtv-video.c
@@ -496,26 +496,24 @@ static struct urb *usbtv_setup_iso_transfer(struct usbtv *usbtv)
 {
 	struct urb *ip;
 	int size = usbtv->iso_size;
+	void *buf;
 	int i;
 
 	ip = usb_alloc_urb(USBTV_ISOC_PACKETS, GFP_KERNEL);
 	if (ip == NULL)
 		return NULL;
 
-	ip->dev = usbtv->udev;
-	ip->context = usbtv;
-	ip->pipe = usb_rcvisocpipe(usbtv->udev, USBTV_VIDEO_ENDP);
-	ip->interval = 1;
-	ip->transfer_flags = URB_ISO_ASAP;
-	ip->transfer_buffer = kcalloc(USBTV_ISOC_PACKETS, size,
-						GFP_KERNEL);
-	if (!ip->transfer_buffer) {
+	buf = kcalloc(USBTV_ISOC_PACKETS, size, GFP_KERNEL);
+	if (!buf) {
 		usb_free_urb(ip);
 		return NULL;
 	}
-	ip->complete = usbtv_iso_cb;
+	usb_fill_int_urb(ip, usbtv->udev,
+			 usb_rcvisocpipe(usbtv->udev, USBTV_VIDEO_ENDP),
+			 buf, size * USBTV_ISOC_PACKETS, usbtv_iso_cb,
+			 usbtv, 1);
+	ip->transfer_flags = URB_ISO_ASAP;
 	ip->number_of_packets = USBTV_ISOC_PACKETS;
-	ip->transfer_buffer_length = size * USBTV_ISOC_PACKETS;
 	for (i = 0; i < USBTV_ISOC_PACKETS; i++) {
 		ip->iso_frame_desc[i].offset = size * i;
 		ip->iso_frame_desc[i].length = size;
-- 
2.17.1

[PATCH 25/27] media: usbvision: remove time_in_irq

[Sebastian Andrzej Siewior]

Time "in interrupt" accounting with the help of `jiffies' is a pointless
exercise. This variable isn't even used.
Remove time_in_irq.

Cc: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/usbvision/usbvision-core.c | 3 ---
 drivers/media/usb/usbvision/usbvision.h      | 1 -
 2 files changed, 4 deletions(-)

diff --git a/drivers/media/usb/usbvision/usbvision-core.c b/drivers/media/usb/usbvision/usbvision-core.c
index 7138c2b606cc..31e0e98d6daf 100644
--- a/drivers/media/usb/usbvision/usbvision-core.c
+++ b/drivers/media/usb/usbvision/usbvision-core.c
@@ -1272,7 +1272,6 @@ static void usbvision_isoc_irq(struct urb *urb)
 	int len;
 	struct usb_usbvision *usbvision = urb->context;
 	int i;
-	unsigned long start_time = jiffies;
 	struct usbvision_frame **f;
 
 	/* We don't want to do anything if we are about to be removed! */
@@ -1324,8 +1323,6 @@ static void usbvision_isoc_irq(struct urb *urb)
 		scratch_reset(usbvision);
 	}
 
-	usbvision->time_in_irq += jiffies - start_time;
-
 	for (i = 0; i < USBVISION_URB_FRAMES; i++) {
 		urb->iso_frame_desc[i].status = 0;
 		urb->iso_frame_desc[i].actual_length = 0;
diff --git a/drivers/media/usb/usbvision/usbvision.h b/drivers/media/usb/usbvision/usbvision.h
index 6ecdcd58248f..017e7baf5747 100644
--- a/drivers/media/usb/usbvision/usbvision.h
+++ b/drivers/media/usb/usbvision/usbvision.h
@@ -447,7 +447,6 @@ struct usb_usbvision {
 	unsigned long isoc_skip_count;			/* How many empty ISO packets received */
 	unsigned long isoc_err_count;			/* How many bad ISO packets received */
 	unsigned long isoc_packet_count;		/* How many packets we totally got */
-	unsigned long time_in_irq;			/* How long do we need for interrupt */
 	int isoc_measure_bandwidth_count;
 	int frame_num;					/* How many video frames we send to user */
 	int max_strip_len;				/* How big is the biggest strip */
-- 
2.17.1

[PATCH 26/27] media: usbvision: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.

Cc: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/usbvision/usbvision-core.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/usbvision/usbvision-core.c b/drivers/media/usb/usbvision/usbvision-core.c
index 31e0e98d6daf..52f21ba2a368 100644
--- a/drivers/media/usb/usbvision/usbvision-core.c
+++ b/drivers/media/usb/usbvision/usbvision-core.c
@@ -2302,16 +2302,14 @@ int usbvision_init_isoc(struct usb_usbvision *usbvision)
 					   sb_size,
 					   GFP_KERNEL,
 					   &urb->transfer_dma);
-		urb->dev = dev;
-		urb->context = usbvision;
-		urb->pipe = usb_rcvisocpipe(dev, usbvision->video_endp);
+		usb_fill_int_urb(urb, dev,
+				 usb_rcvisocpipe(dev, usbvision->video_endp),
+				 usbvision->sbuf[buf_idx].data,
+				 usbvision->isoc_packet_size * USBVISION_URB_FRAMES,
+				 usbvision_isoc_irq, usbvision, 1);
+
 		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
-		urb->interval = 1;
-		urb->transfer_buffer = usbvision->sbuf[buf_idx].data;
-		urb->complete = usbvision_isoc_irq;
 		urb->number_of_packets = USBVISION_URB_FRAMES;
-		urb->transfer_buffer_length =
-		    usbvision->isoc_packet_size * USBVISION_URB_FRAMES;
 		for (j = k = 0; j < USBVISION_URB_FRAMES; j++,
 		     k += usbvision->isoc_packet_size) {
 			urb->iso_frame_desc[j].offset = k;
-- 
2.17.1

[PATCH 27/27] media: uvcvideo: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

Using usb_fill_int_urb() helps to find code which initializes an
URB. A grep for members of the struct (like ->complete) reveal lots
of other things, too.
usb_fill_int_urb() also checks bInterval to be in the 1…16 range on
HS/SS.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/uvc/uvc_video.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index a88b2e51a666..79e7a827ed44 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1619,21 +1619,19 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream,
 			return -ENOMEM;
 		}
 
-		urb->dev = stream->dev->udev;
-		urb->context = stream;
-		urb->pipe = usb_rcvisocpipe(stream->dev->udev,
-				ep->desc.bEndpointAddress);
+		usb_fill_int_urb(urb, stream->dev->udev,
+				 usb_rcvisocpipe(stream->dev->udev,
+						 ep->desc.bEndpointAddress),
+				 stream->urb_buffer[i], size,
+				 uvc_video_complete, stream,
+				 ep->desc.bInterval);
 #ifndef CONFIG_DMA_NONCOHERENT
 		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
 		urb->transfer_dma = stream->urb_dma[i];
 #else
 		urb->transfer_flags = URB_ISO_ASAP;
 #endif
-		urb->interval = ep->desc.bInterval;
-		urb->transfer_buffer = stream->urb_buffer[i];
-		urb->complete = uvc_video_complete;
 		urb->number_of_packets = npackets;
-		urb->transfer_buffer_length = size;
 
 		for (j = 0; j < npackets; ++j) {
 			urb->iso_frame_desc[j].offset = j * psize;
-- 
2.17.1

Re: [PATCH 27/27] media: uvcvideo: use usb_fill_int_urb()

[Laurent Pinchart]

Hi Sebastian,

Thank you for the patch.

On Wednesday, 20 June 2018 14:01:05 EEST Sebastian Andrzej Siewior wrote:
> Using usb_fill_int_urb() helps to find code which initializes an
> URB. A grep for members of the struct (like ->complete) reveal lots
> of other things, too.
> usb_fill_int_urb() also checks bInterval to be in the 1…16 range on
> HS/SS.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index a88b2e51a666..79e7a827ed44 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1619,21 +1619,19 @@ static int uvc_init_video_isoc(struct uvc_streaming
> *stream, return -ENOMEM;
>  		}
> 
> -		urb->dev = stream->dev->udev;
> -		urb->context = stream;
> -		urb->pipe = usb_rcvisocpipe(stream->dev->udev,
> -				ep->desc.bEndpointAddress);
> +		usb_fill_int_urb(urb, stream->dev->udev,
> +				 usb_rcvisocpipe(stream->dev->udev,
> +						 ep->desc.bEndpointAddress),
> +				 stream->urb_buffer[i], size,
> +				 uvc_video_complete, stream,
> +				 ep->desc.bInterval);

You're filling an isoc URB with usb_fill_int_urb(), which is explicitly 
documented as usable to fill an interrupt URB. Shouldn't we create a 
usb_fill_isoc_urb() function ? It could just be an alias for 
usb_fill_int_urb() if isoc and interrupt URBs don't need to be treated 
differently. Alternatively, I'd be fine using usb_fill_int_urb() if the 
function documentation's was updated to mention isoc URBs as well.

>  #ifndef CONFIG_DMA_NONCOHERENT
>  		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
>  		urb->transfer_dma = stream->urb_dma[i];
>  #else
>  		urb->transfer_flags = URB_ISO_ASAP;
>  #endif
> -		urb->interval = ep->desc.bInterval;

Unless I'm mistaken this introduces a change in behaviour for HS and SS, and 
should thus be documented in the commit message. I suspect that this is the 
real reason for this patch. I'd thus update the subject line to describe this 
fix, and the body of the message to explain why using usb_fill_int_urb() is 
the proper fix.

> -		urb->transfer_buffer = stream->urb_buffer[i];
> -		urb->complete = uvc_video_complete;
>  		urb->number_of_packets = npackets;
> -		urb->transfer_buffer_length = size;

usb_fill_int_urb() sets urb->start_frame to -1. Does that impact us in any way 
?

>  		for (j = 0; j < npackets; ++j) {
>  			urb->iso_frame_desc[j].offset = j * psize;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 27/27] media: uvcvideo: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

On 2018-06-20 14:55:23 [+0300], Laurent Pinchart wrote:
> Hi Sebastian,
Hi Laurent,

> Thank you for the patch.
> 
> On Wednesday, 20 June 2018 14:01:05 EEST Sebastian Andrzej Siewior wrote:
> > Using usb_fill_int_urb() helps to find code which initializes an
> > URB. A grep for members of the struct (like ->complete) reveal lots
> > of other things, too.
> > usb_fill_int_urb() also checks bInterval to be in the 1…16 range on
> > HS/SS.
> > 
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> >  drivers/media/usb/uvc/uvc_video.c | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > b/drivers/media/usb/uvc/uvc_video.c index a88b2e51a666..79e7a827ed44 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -1619,21 +1619,19 @@ static int uvc_init_video_isoc(struct uvc_streaming
> > *stream, return -ENOMEM;
> >  		}
> > 
> > -		urb->dev = stream->dev->udev;
> > -		urb->context = stream;
> > -		urb->pipe = usb_rcvisocpipe(stream->dev->udev,
> > -				ep->desc.bEndpointAddress);
> > +		usb_fill_int_urb(urb, stream->dev->udev,
> > +				 usb_rcvisocpipe(stream->dev->udev,
> > +						 ep->desc.bEndpointAddress),
> > +				 stream->urb_buffer[i], size,
> > +				 uvc_video_complete, stream,
> > +				 ep->desc.bInterval);
> 
> You're filling an isoc URB with usb_fill_int_urb(), which is explicitly 
> documented as usable to fill an interrupt URB. Shouldn't we create a 
> usb_fill_isoc_urb() function ? It could just be an alias for 
> usb_fill_int_urb() if isoc and interrupt URBs don't need to be treated 
> differently. Alternatively, I'd be fine using usb_fill_int_urb() if the 
> function documentation's was updated to mention isoc URBs as well.

I thought I read it there but I couldn't find it. And then I found it in
Documentation/driver-api/usb/URB.rst:
| you specify. You can use the :c:func:`usb_fill_int_urb` macro to fill
| most ISO transfer fields.

So you simply asking that the kerneldoc of usb_fill_int_urb() is
extended to mention isoc, too?

> >  #ifndef CONFIG_DMA_NONCOHERENT
> >  		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
> >  		urb->transfer_dma = stream->urb_dma[i];
> >  #else
> >  		urb->transfer_flags = URB_ISO_ASAP;
> >  #endif
> > -		urb->interval = ep->desc.bInterval;
> 
> Unless I'm mistaken this introduces a change in behaviour for HS and SS, and 
> should thus be documented in the commit message. 
I did:

| usb_fill_int_urb() also checks bInterval to be in the 1…16 range on
| HS/SS.

so this wasn't enough?

> I suspect that this is the 
> real reason for this patch. I'd thus update the subject line to describe this 
> fix, and the body of the message to explain why using usb_fill_int_urb() is 
> the proper fix.

Actually no. I was looking for all the ->complete handlers and most of
them used usb_fill_… except a few. And while moving to the function I
was checking if everything stays the same (and mentioned ->interval
since ->start_frame is documented as a return parameter).

So here you are asking for a description update which explicit says
bug-fix?

> > -		urb->transfer_buffer = stream->urb_buffer[i];
> > -		urb->complete = uvc_video_complete;
> >  		urb->number_of_packets = npackets;
> > -		urb->transfer_buffer_length = size;
> 
> usb_fill_int_urb() sets urb->start_frame to -1. Does that impact us in any way 
> ?

It should not. The documentation says:
|  * @start_frame: Returns the initial frame for isochronous transfers.

> >  		for (j = 0; j < npackets; ++j) {
> >  			urb->iso_frame_desc[j].offset = j * psize;

Sebastian

Re: [PATCH 27/27] media: uvcvideo: use usb_fill_int_urb()

[Laurent Pinchart]

Hi Sebastian,

On Wednesday, 20 June 2018 16:21:44 EEST Sebastian Andrzej Siewior wrote:
> On 2018-06-20 14:55:23 [+0300], Laurent Pinchart wrote:
> > Hi Sebastian,
> 
> Hi Laurent,
> 
> > Thank you for the patch.
> > 
> > On Wednesday, 20 June 2018 14:01:05 EEST Sebastian Andrzej Siewior wrote:
> > > Using usb_fill_int_urb() helps to find code which initializes an
> > > URB. A grep for members of the struct (like ->complete) reveal lots
> > > of other things, too.
> > > usb_fill_int_urb() also checks bInterval to be in the 1…16 range on
> > > HS/SS.
> > > 
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > > ---
> > > 
> > >  drivers/media/usb/uvc/uvc_video.c | 14 ++++++--------
> > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > > b/drivers/media/usb/uvc/uvc_video.c index a88b2e51a666..79e7a827ed44
> > > 100644
> > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > @@ -1619,21 +1619,19 @@ static int uvc_init_video_isoc(struct
> > > uvc_streaming *stream,
> > >  			return -ENOMEM;
> > >  		}
> > > 
> > > -		urb->dev = stream->dev->udev;
> > > -		urb->context = stream;
> > > -		urb->pipe = usb_rcvisocpipe(stream->dev->udev,
> > > -				ep->desc.bEndpointAddress);
> > > +		usb_fill_int_urb(urb, stream->dev->udev,
> > > +				 usb_rcvisocpipe(stream->dev->udev,
> > > +						 ep->desc.bEndpointAddress),
> > > +				 stream->urb_buffer[i], size,
> > > +				 uvc_video_complete, stream,
> > > +				 ep->desc.bInterval);
> > 
> > You're filling an isoc URB with usb_fill_int_urb(), which is explicitly
> > documented as usable to fill an interrupt URB. Shouldn't we create a
> > usb_fill_isoc_urb() function ? It could just be an alias for
> > usb_fill_int_urb() if isoc and interrupt URBs don't need to be treated
> > differently. Alternatively, I'd be fine using usb_fill_int_urb() if the
> > function documentation's was updated to mention isoc URBs as well.
> 
> I thought I read it there but I couldn't find it. And then I found it in
> 
> Documentation/driver-api/usb/URB.rst:
> | you specify. You can use the :c:func:`usb_fill_int_urb` macro to fill
> | most ISO transfer fields.
> 
> So you simply asking that the kerneldoc of usb_fill_int_urb() is
> extended to mention isoc, too?

That would be nice I think.

> > >  #ifndef CONFIG_DMA_NONCOHERENT
> > >  		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
> > >  		urb->transfer_dma = stream->urb_dma[i];
> > >  #else
> > >  
> > >  #endif
> > > -		urb->interval = ep->desc.bInterval;
> > 
> > Unless I'm mistaken this introduces a change in behaviour for HS and SS,
> > and should thus be documented in the commit message.
> 
> I did:
> | usb_fill_int_urb() also checks bInterval to be in the 1…16 range on
> | HS/SS.
> 
> so this wasn't enough?

This sounds to me like a sanity check, while the patch effectively changes the 
code as follows for HS and SS:

-		urb->interval = ep->desc.bInterval;
+		urb->interval = 1 << (ep->desc.bInterval - 1);

I believe the change is correct, although it would be nice if you could 
double-check, as the documentation of the function states:

 * @interval: what to set the urb interval to, encoded like
 *      the endpoint descriptor's bInterval value.

> > I suspect that this is the real reason for this patch. I'd thus update the
> > subject line to describe this fix, and the body of the message to explain
> > why using usb_fill_int_urb() is the proper fix.
> 
> Actually no. I was looking for all the ->complete handlers and most of
> them used usb_fill_… except a few. And while moving to the function I
> was checking if everything stays the same (and mentioned ->interval
> since ->start_frame is documented as a return parameter).
> 
> So here you are asking for a description update which explicit says
> bug-fix?

I'd like that, as it seems to be a bugfix, not just a code cleanup without any 
behavioural change.

> > > -		urb->transfer_buffer = stream->urb_buffer[i];
> > > -		urb->complete = uvc_video_complete;
> > > 
> > >  		urb->number_of_packets = npackets;
> > > 
> > > -		urb->transfer_buffer_length = size;
> > 
> > usb_fill_int_urb() sets urb->start_frame to -1. Does that impact us in any
> > way ?
> 
> It should not. The documentation says:
> |  * @start_frame: Returns the initial frame for isochronous transfers.

Thanks for checking.

> > >  		for (j = 0; j < npackets; ++j) {
> > >  		
> > >  			urb->iso_frame_desc[j].offset = j * psize;

-- 
Regards,

Laurent Pinchart

[PATCH] USB: note that usb_fill_int_urb() can be used used for ISOC urbs.

[Sebastian Andrzej Siewior]

Laurent suggested that the kerneldoc documentation could state that
usb_fill_int_urb() can also be used for the initialisation of an
isochronous urb. The USB documentation in
Documentation/driver-api/usb/URB.rst already mentions this, some drivers
do so and there is no explicit usb_fill_iso_urb().

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
On 2018-06-20 17:14:53 [+0300], Laurent Pinchart wrote:
> > So you simply asking that the kerneldoc of usb_fill_int_urb() is
> > extended to mention isoc, too?
> 
> That would be nice I think.

here it is.

 include/linux/usb.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/usb.h b/include/linux/usb.h
index 4cdd515a4385..c3a8bd586121 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1657,7 +1657,8 @@ static inline void usb_fill_bulk_urb(struct urb *urb,
  *	the endpoint descriptor's bInterval value.
  *
  * Initializes a interrupt urb with the proper information needed to submit
- * it to a device.
+ * it to a device. This function can also be used to initialize an isochronous
+ * urb.
  *
  * Note that High Speed and SuperSpeed(+) interrupt endpoints use a logarithmic
  * encoding of the endpoint interval, and express polling intervals in
-- 
2.17.1

Re: [PATCH] USB: note that usb_fill_int_urb() can be used used for ISOC urbs.

[Alan Stern]

On Wed, 20 Jun 2018, Sebastian Andrzej Siewior wrote:

> Laurent suggested that the kerneldoc documentation could state that
> usb_fill_int_urb() can also be used for the initialisation of an
> isochronous urb. The USB documentation in
> Documentation/driver-api/usb/URB.rst already mentions this, some drivers
> do so and there is no explicit usb_fill_iso_urb().
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> On 2018-06-20 17:14:53 [+0300], Laurent Pinchart wrote:
> > > So you simply asking that the kerneldoc of usb_fill_int_urb() is
> > > extended to mention isoc, too?
> > 
> > That would be nice I think.
> 
> here it is.
> 
>  include/linux/usb.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 4cdd515a4385..c3a8bd586121 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1657,7 +1657,8 @@ static inline void usb_fill_bulk_urb(struct urb *urb,
>   *	the endpoint descriptor's bInterval value.
>   *
>   * Initializes a interrupt urb with the proper information needed to submit
> - * it to a device.
> + * it to a device. This function can also be used to initialize an isochronous
> + * urb.

No, no!  This function can _help_ initialize an isochronous URB, but
that's all.  It would be better to create an explicit 
usb_fill_isoc_urb() routine, and even that would have to be incomplete.

There are two problems with using usb_fill_int_urb() to initialize an 
isochronous URB:

	The calculation of the interval value is wrong for full-speed
	devices.

	The routine does not set urb->number_of_packets or
	urb->iso_frame_desc[].

Alan Stern

>   *
>   * Note that High Speed and SuperSpeed(+) interrupt endpoints use a logarithmic
>   * encoding of the endpoint interval, and express polling intervals in
> 

Re: [PATCH] USB: note that usb_fill_int_urb() can be used used for ISOC urbs.

[Sebastian Andrzej Siewior]

On 2018-06-20 11:35:05 [-0400], Alan Stern wrote:
> > diff --git a/include/linux/usb.h b/include/linux/usb.h
> > index 4cdd515a4385..c3a8bd586121 100644
> > --- a/include/linux/usb.h
> > +++ b/include/linux/usb.h
> > @@ -1657,7 +1657,8 @@ static inline void usb_fill_bulk_urb(struct urb *urb,
> >   *	the endpoint descriptor's bInterval value.
> >   *
> >   * Initializes a interrupt urb with the proper information needed to submit
> > - * it to a device.
> > + * it to a device. This function can also be used to initialize an isochronous
> > + * urb.
> 
> No, no!  This function can _help_ initialize an isochronous URB, but
> that's all.  It would be better to create an explicit 
> usb_fill_isoc_urb() routine, and even that would have to be incomplete.

Yes, incomplete. I read incomplete as some additional fields have to be
set which are not set yet. But for FS you write…

> There are two problems with using usb_fill_int_urb() to initialize an 
> isochronous URB:
> 
> 	The calculation of the interval value is wrong for full-speed
> 	devices.

Why wrong?

> 	The routine does not set urb->number_of_packets or
> 	urb->iso_frame_desc[].

Yes.

> Alan Stern

Sebastian

Re: [PATCH] USB: note that usb_fill_int_urb() can be used used for ISOC urbs.

[Alan Stern]

On Wed, 20 Jun 2018, Sebastian Andrzej Siewior wrote:

> On 2018-06-20 11:35:05 [-0400], Alan Stern wrote:
> > > diff --git a/include/linux/usb.h b/include/linux/usb.h
> > > index 4cdd515a4385..c3a8bd586121 100644
> > > --- a/include/linux/usb.h
> > > +++ b/include/linux/usb.h
> > > @@ -1657,7 +1657,8 @@ static inline void usb_fill_bulk_urb(struct urb *urb,
> > >   *	the endpoint descriptor's bInterval value.
> > >   *
> > >   * Initializes a interrupt urb with the proper information needed to submit
> > > - * it to a device.
> > > + * it to a device. This function can also be used to initialize an isochronous
> > > + * urb.
> > 
> > No, no!  This function can _help_ initialize an isochronous URB, but
> > that's all.  It would be better to create an explicit 
> > usb_fill_isoc_urb() routine, and even that would have to be incomplete.
> 
> Yes, incomplete. I read incomplete as some additional fields have to be
> set which are not set yet. But for FS you write…

There's also the cognitive dissonance of using a routine named 
"usb_fill_int_urb" to initialize an isochronous URB.  That should set 
off alarm bells in the mind of anyone reading the code, no matter what 
the documentation says.

> > There are two problems with using usb_fill_int_urb() to initialize an 
> > isochronous URB:
> > 
> > 	The calculation of the interval value is wrong for full-speed
> > 	devices.
> 
> Why wrong?

Because the interval value in the FS endpoint descriptor uses a
logarithmic encoding, whereas the value stored in the URB uses a linear
encoding.  Simply copying one value to the other will store an
incorrect number, except in the lucky cases where the interval is
either 1 or 2.

Alan Stern

> > 	The routine does not set urb->number_of_packets or
> > 	urb->iso_frame_desc[].
> 
> Yes.
> 
> > Alan Stern
> 
> Sebastian

Re: [PATCH] USB: note that usb_fill_int_urb() can be used used for ISOC urbs.

[Sebastian Andrzej Siewior]

On 2018-06-20 12:21:33 [-0400], Alan Stern wrote:
> > > There are two problems with using usb_fill_int_urb() to initialize an 
> > > isochronous URB:
> > > 
> > > 	The calculation of the interval value is wrong for full-speed
> > > 	devices.
> > 
> > Why wrong?
> 
> Because the interval value in the FS endpoint descriptor uses a
> logarithmic encoding, whereas the value stored in the URB uses a linear
> encoding.  Simply copying one value to the other will store an
> incorrect number, except in the lucky cases where the interval is
> either 1 or 2.

Hmmm. Now that I looked into USB 2.0 specification it really says
logarithmic encoding for ISOC endpoints on every speed and not just HS.
And INTR endpoints have logarithmic encoding only for HS. I remembered
it differently…

But based on this, we should really introduce usb_fill_iso_urb() which
handles this correctly. Also the documentation should be updated because
the suggestion for ubs_fill_intr_urb() is misleading (especially if you
have wrong memory about the encoding on FS).

> Alan Stern

Sebastian

Re: [PATCH] USB: note that usb_fill_int_urb() can be used used for ISOC urbs.

[Alan Stern]

On Wed, 20 Jun 2018, Sebastian Andrzej Siewior wrote:

> On 2018-06-20 12:21:33 [-0400], Alan Stern wrote:
> > > > There are two problems with using usb_fill_int_urb() to initialize an 
> > > > isochronous URB:
> > > > 
> > > > 	The calculation of the interval value is wrong for full-speed
> > > > 	devices.
> > > 
> > > Why wrong?
> > 
> > Because the interval value in the FS endpoint descriptor uses a
> > logarithmic encoding, whereas the value stored in the URB uses a linear
> > encoding.  Simply copying one value to the other will store an
> > incorrect number, except in the lucky cases where the interval is
> > either 1 or 2.
> 
> Hmmm. Now that I looked into USB 2.0 specification it really says
> logarithmic encoding for ISOC endpoints on every speed and not just HS.
> And INTR endpoints have logarithmic encoding only for HS. I remembered
> it differently…
> 
> But based on this, we should really introduce usb_fill_iso_urb() which
> handles this correctly. Also the documentation should be updated because
> the suggestion for ubs_fill_intr_urb() is misleading (especially if you
> have wrong memory about the encoding on FS).

That sounds like a good thing to do.

Alan Stern

[PATCH 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);
-- 
2.18.0

Re: [PATCH 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

Re: [PATCH 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

Re: [PATCH 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:

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

Re: [PATCH 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

Re: [PATCH 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


-- 
Regards,

Laurent Pinchart

Re: [PATCH 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

Re: [PATCH 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);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 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

[PATCH 27/27 v2] media: uvcvideo: use usb_fill_int_urb() for the ->intarval value

[Sebastian Andrzej Siewior]

uvc_init_video_isoc() assigns
	urb->interval = p->desc.bInterval;

for the interval. This is correct for FS/LS. For HS/SS the bInterval
value is using a logarithmic encoding. The usb_fill_int_urb() function
takes this into account while settings the ->interval member.
->start_frame is set to -1 on init and should be filled by the HC on
completion of the URB.

Use usb_fill_int_urb() to fill the members of the struct urb.

Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/media/usb/uvc/uvc_video.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index a88b2e51a666..79e7a827ed44 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1619,21 +1619,19 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream,
 			return -ENOMEM;
 		}
 
-		urb->dev = stream->dev->udev;
-		urb->context = stream;
-		urb->pipe = usb_rcvisocpipe(stream->dev->udev,
-				ep->desc.bEndpointAddress);
+		usb_fill_int_urb(urb, stream->dev->udev,
+				 usb_rcvisocpipe(stream->dev->udev,
+						 ep->desc.bEndpointAddress),
+				 stream->urb_buffer[i], size,
+				 uvc_video_complete, stream,
+				 ep->desc.bInterval);
 #ifndef CONFIG_DMA_NONCOHERENT
 		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
 		urb->transfer_dma = stream->urb_dma[i];
 #else
 		urb->transfer_flags = URB_ISO_ASAP;
 #endif
-		urb->interval = ep->desc.bInterval;
-		urb->transfer_buffer = stream->urb_buffer[i];
-		urb->complete = uvc_video_complete;
 		urb->number_of_packets = npackets;
-		urb->transfer_buffer_length = size;
 
 		for (j = 0; j < npackets; ++j) {
 			urb->iso_frame_desc[j].offset = j * psize;
-- 
2.17.1

Re: [PATCH 27/27 v2] media: uvcvideo: use usb_fill_int_urb() for the ->intarval value

[Alan Stern]

On Wed, 20 Jun 2018, Sebastian Andrzej Siewior wrote:

> uvc_init_video_isoc() assigns
> 	urb->interval = p->desc.bInterval;
> 
> for the interval. This is correct for FS/LS.

That's a strange thing to say.  For one thing, LS devices don't support 
isochronous transfers at all.  And while this assignment would be 
correct for FS interrupt URBs, it is wrong for FS isochronous URBs.

> For HS/SS the bInterval
> value is using a logarithmic encoding. The usb_fill_int_urb() function
> takes this into account while settings the ->interval member.
> ->start_frame is set to -1 on init and should be filled by the HC on
> completion of the URB.
> 
> Use usb_fill_int_urb() to fill the members of the struct urb.

Please don't do this.  If you instead on using an inline routine to 
save on source code, create an explicit usb_fill_isoc_urb() function.

Alan Stern

> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index a88b2e51a666..79e7a827ed44 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1619,21 +1619,19 @@ static int uvc_init_video_isoc(struct uvc_streaming *stream,
>  			return -ENOMEM;
>  		}
>  
> -		urb->dev = stream->dev->udev;
> -		urb->context = stream;
> -		urb->pipe = usb_rcvisocpipe(stream->dev->udev,
> -				ep->desc.bEndpointAddress);
> +		usb_fill_int_urb(urb, stream->dev->udev,
> +				 usb_rcvisocpipe(stream->dev->udev,
> +						 ep->desc.bEndpointAddress),
> +				 stream->urb_buffer[i], size,
> +				 uvc_video_complete, stream,
> +				 ep->desc.bInterval);
>  #ifndef CONFIG_DMA_NONCOHERENT
>  		urb->transfer_flags = URB_ISO_ASAP | URB_NO_TRANSFER_DMA_MAP;
>  		urb->transfer_dma = stream->urb_dma[i];
>  #else
>  		urb->transfer_flags = URB_ISO_ASAP;
>  #endif
> -		urb->interval = ep->desc.bInterval;
> -		urb->transfer_buffer = stream->urb_buffer[i];
> -		urb->complete = uvc_video_complete;
>  		urb->number_of_packets = npackets;
> -		urb->transfer_buffer_length = size;
>  
>  		for (j = 0; j < npackets; ++j) {
>  			urb->iso_frame_desc[j].offset = j * psize;
>