ALSA: use irqsave() in URB completion + usb_fill_int_urb

ALSA: 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 1/9] ALSA: 6fire: 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.
usb6fire_comm_init_urb() passes no transfer length.

Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 sound/usb/6fire/comm.c | 20 +++++++-------------
 sound/usb/6fire/pcm.c  | 25 +++++++++++--------------
 2 files changed, 18 insertions(+), 27 deletions(-)

diff --git a/sound/usb/6fire/comm.c b/sound/usb/6fire/comm.c
index 161215d78d95..6adc09a0c6fa 100644
--- a/sound/usb/6fire/comm.c
+++ b/sound/usb/6fire/comm.c
@@ -26,12 +26,9 @@ static void usb6fire_comm_init_urb(struct comm_runtime *rt, struct urb *urb,
 		u8 *buffer, void *context, void(*handler)(struct urb *urb))
 {
 	usb_init_urb(urb);
-	urb->transfer_buffer = buffer;
-	urb->pipe = usb_sndintpipe(rt->chip->dev, COMM_EP);
-	urb->complete = handler;
-	urb->context = context;
-	urb->interval = 1;
-	urb->dev = rt->chip->dev;
+	usb_fill_int_urb(urb, rt->chip->dev,
+			 usb_sndintpipe(rt->chip->dev, COMM_EP),
+			 buffer, 0, handler, context, 1);
 }
 
 static void usb6fire_comm_receiver_handler(struct urb *urb)
@@ -168,13 +165,10 @@ int usb6fire_comm_init(struct sfire_chip *chip)
 	rt->write16 = usb6fire_comm_write16;
 
 	/* submit an urb that receives communication data from device */
-	urb->transfer_buffer = rt->receiver_buffer;
-	urb->transfer_buffer_length = COMM_RECEIVER_BUFSIZE;
-	urb->pipe = usb_rcvintpipe(chip->dev, COMM_EP);
-	urb->dev = chip->dev;
-	urb->complete = usb6fire_comm_receiver_handler;
-	urb->context = rt;
-	urb->interval = 1;
+	usb_fill_int_urb(urb, chip->dev, usb_rcvintpipe(chip->dev, COMM_EP),
+			 rt->receiver_buffer, COMM_RECEIVER_BUFSIZE,
+			 usb6fire_comm_receiver_handler, rt, 1);
+
 	ret = usb_submit_urb(urb, GFP_KERNEL);
 	if (ret < 0) {
 		kfree(rt->receiver_buffer);
diff --git a/sound/usb/6fire/pcm.c b/sound/usb/6fire/pcm.c
index 2dd2518a71d3..e32cca0f3c2a 100644
--- a/sound/usb/6fire/pcm.c
+++ b/sound/usb/6fire/pcm.c
@@ -569,20 +569,14 @@ static const struct snd_pcm_ops pcm_ops = {
 };
 
 static void usb6fire_pcm_init_urb(struct pcm_urb *urb,
-				  struct sfire_chip *chip, bool in, int ep,
+				  struct sfire_chip *chip, unsigned int pipe,
 				  void (*handler)(struct urb *))
 {
 	urb->chip = chip;
 	usb_init_urb(&urb->instance);
-	urb->instance.transfer_buffer = urb->buffer;
-	urb->instance.transfer_buffer_length =
-			PCM_N_PACKETS_PER_URB * PCM_MAX_PACKET_SIZE;
-	urb->instance.dev = chip->dev;
-	urb->instance.pipe = in ? usb_rcvisocpipe(chip->dev, ep)
-			: usb_sndisocpipe(chip->dev, ep);
-	urb->instance.interval = 1;
-	urb->instance.complete = handler;
-	urb->instance.context = urb;
+	usb_fill_int_urb(&urb->instance, chip->dev, pipe, urb->buffer,
+			 PCM_N_PACKETS_PER_URB * PCM_MAX_PACKET_SIZE,
+			 handler, urb, 1);
 	urb->instance.number_of_packets = PCM_N_PACKETS_PER_URB;
 }
 
@@ -643,10 +637,13 @@ int usb6fire_pcm_init(struct sfire_chip *chip)
 	spin_lock_init(&rt->capture.lock);
 
 	for (i = 0; i < PCM_N_URBS; i++) {
-		usb6fire_pcm_init_urb(&rt->in_urbs[i], chip, true, IN_EP,
-				usb6fire_pcm_in_urb_handler);
-		usb6fire_pcm_init_urb(&rt->out_urbs[i], chip, false, OUT_EP,
-				usb6fire_pcm_out_urb_handler);
+		usb6fire_pcm_init_urb(&rt->in_urbs[i], chip,
+				      usb_rcvisocpipe(chip->dev, IN_EP),
+				      usb6fire_pcm_in_urb_handler);
+
+		usb6fire_pcm_init_urb(&rt->out_urbs[i], chip,
+				      usb_sndisocpipe(chip->dev, OUT_EP),
+				      usb6fire_pcm_out_urb_handler);
 
 		rt->in_urbs[i].peer = &rt->out_urbs[i];
 		rt->out_urbs[i].peer = &rt->in_urbs[i];
-- 
2.17.1

[PATCH 2/9] ALSA: line6: 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.
Data pointer & size is filled out later so they are not considered.

Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 sound/usb/line6/capture.c  | 16 ++++++----------
 sound/usb/line6/playback.c | 16 ++++++----------
 2 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/sound/usb/line6/capture.c b/sound/usb/line6/capture.c
index d8a14d769f48..4ee54ba436df 100644
--- a/sound/usb/line6/capture.c
+++ b/sound/usb/line6/capture.c
@@ -52,7 +52,6 @@ static int submit_audio_in_urb(struct snd_line6_pcm *line6pcm)
 	    line6pcm->in.buffer +
 	    index * LINE6_ISO_PACKETS * line6pcm->max_packet_size_in;
 	urb_in->transfer_buffer_length = urb_size;
-	urb_in->context = line6pcm;
 
 	ret = usb_submit_urb(urb_in, GFP_ATOMIC);
 
@@ -280,17 +279,14 @@ int line6_create_audio_in_urbs(struct snd_line6_pcm *line6pcm)
 		if (urb == NULL)
 			return -ENOMEM;
 
-		urb->dev = line6->usbdev;
-		urb->pipe =
-		    usb_rcvisocpipe(line6->usbdev,
-				    line6->properties->ep_audio_r &
-				    USB_ENDPOINT_NUMBER_MASK);
+		usb_fill_int_urb(urb, line6->usbdev,
+				 usb_rcvisocpipe(line6->usbdev,
+						 line6->properties->ep_audio_r &
+						 USB_ENDPOINT_NUMBER_MASK),
+				 NULL, 0, audio_in_callback, line6pcm,
+				 LINE6_ISO_INTERVAL);
 		urb->transfer_flags = URB_ISO_ASAP;
-		urb->start_frame = -1;
 		urb->number_of_packets = LINE6_ISO_PACKETS;
-		urb->interval = LINE6_ISO_INTERVAL;
-		urb->error_count = 0;
-		urb->complete = audio_in_callback;
 	}
 
 	return 0;
diff --git a/sound/usb/line6/playback.c b/sound/usb/line6/playback.c
index dec89d2beb57..af1ca09c260e 100644
--- a/sound/usb/line6/playback.c
+++ b/sound/usb/line6/playback.c
@@ -202,7 +202,6 @@ static int submit_audio_out_urb(struct snd_line6_pcm *line6pcm)
 	    line6pcm->out.buffer +
 	    index * LINE6_ISO_PACKETS * line6pcm->max_packet_size_out;
 	urb_out->transfer_buffer_length = urb_size;
-	urb_out->context = line6pcm;
 
 	if (test_bit(LINE6_STREAM_PCM, &line6pcm->out.running) &&
 	    !test_bit(LINE6_FLAG_PAUSE_PLAYBACK, &line6pcm->flags)) {
@@ -425,17 +424,14 @@ int line6_create_audio_out_urbs(struct snd_line6_pcm *line6pcm)
 		if (urb == NULL)
 			return -ENOMEM;
 
-		urb->dev = line6->usbdev;
-		urb->pipe =
-		    usb_sndisocpipe(line6->usbdev,
-				    line6->properties->ep_audio_w &
-				    USB_ENDPOINT_NUMBER_MASK);
+		usb_fill_int_urb(urb, line6->usbdev,
+				 usb_sndisocpipe(line6->usbdev,
+						 line6->properties->ep_audio_w &
+						 USB_ENDPOINT_NUMBER_MASK),
+				 NULL, 0, audio_out_callback, line6pcm,
+				 LINE6_ISO_INTERVAL);
 		urb->transfer_flags = URB_ISO_ASAP;
-		urb->start_frame = -1;
 		urb->number_of_packets = LINE6_ISO_PACKETS;
-		urb->interval = LINE6_ISO_INTERVAL;
-		urb->error_count = 0;
-		urb->complete = audio_out_callback;
 	}
 
 	return 0;
-- 
2.17.1

[PATCH 3/9] ALSA: ua101: 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: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Clemens Ladisch <clemens@ladisch.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 sound/usb/misc/ua101.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/sound/usb/misc/ua101.c b/sound/usb/misc/ua101.c
index 386fbfd5c617..7002fb4c1bce 100644
--- a/sound/usb/misc/ua101.c
+++ b/sound/usb/misc/ua101.c
@@ -1118,16 +1118,12 @@ static int alloc_stream_urbs(struct ua101 *ua, struct ua101_stream *stream,
 			if (!urb)
 				return -ENOMEM;
 			usb_init_urb(&urb->urb);
-			urb->urb.dev = ua->dev;
-			urb->urb.pipe = stream->usb_pipe;
+			usb_fill_int_urb(&urb->urb, ua->dev, stream->usb_pipe,
+					 addr, max_packet_size, urb_complete,
+					 ua, 1);
 			urb->urb.transfer_flags = URB_NO_TRANSFER_DMA_MAP;
-			urb->urb.transfer_buffer = addr;
 			urb->urb.transfer_dma = dma;
-			urb->urb.transfer_buffer_length = max_packet_size;
 			urb->urb.number_of_packets = 1;
-			urb->urb.interval = 1;
-			urb->urb.context = ua;
-			urb->urb.complete = urb_complete;
 			urb->urb.iso_frame_desc[0].offset = 0;
 			urb->urb.iso_frame_desc[0].length = max_packet_size;
 			stream->urbs[u++] = urb;
-- 
2.17.1

[PATCH 4/9] ALSA: usb-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.

data_ep_set_params() additionally sets urb->transfer_buffer_length which
was not the case earlier.
data_ep_set_params() and data_ep_set_params() ensure that syncinterval is
within the allowed range on HS/SS. The interval value seems to come from
snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage 1 … 4.
So in order to keep the magic wokring I pass datainterval + 1.

Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 sound/usb/endpoint.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index c90607ebe155..bbc02db5b417 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,13 @@ 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_int_urb(u->urb, NULL, ep->pipe, buf, u->buffer_size,
+				 snd_complete_urb, u, ep->datainterval + 1);
 		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 +822,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_int_urb(u->urb, NULL, ep->pipe, ep->syncbuf + i * 4, 4,
+				 snd_complete_urb, u, ep->syncinterval + 1);
+
 		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;
-- 
2.17.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 4/9] ALSA: usb-audio: use usb_fill_int_urb()

[Sergei Shtylyov]

Hello!

On 6/20/2018 12:55 AM, 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.
> 
> data_ep_set_params() additionally sets urb->transfer_buffer_length which
> was not the case earlier.
> data_ep_set_params() and data_ep_set_params()

    These 2 are the same function?

> ensure that syncinterval is
> within the allowed range on HS/SS. The interval value seems to come from
> snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage 1 … 4.
> So in order to keep the magic wokring I pass datainterval + 1.

    Working.

> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
[...]

MBR, Sergei
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 4/9 v2] ALSA: usb-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.

data_ep_set_params() additionally sets urb->transfer_buffer_length which
was not the case earlier.
usb_fill_int_urb() ensures that syncinterval is within the allowed range
on HS/SS. The interval value seems to come from
snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage
1 … 4. So in order to keep the magic working I pass datainterval + 1.

Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
v1…v2: description changes (spelling + usb_fill_int_urb() ensures
       "syncinterval" not data_ep_set_params())

 sound/usb/endpoint.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index c90607ebe155..bbc02db5b417 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,13 @@ 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_int_urb(u->urb, NULL, ep->pipe, buf, u->buffer_size,
+				 snd_complete_urb, u, ep->datainterval + 1);
 		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 +822,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_int_urb(u->urb, NULL, ep->pipe, ep->syncbuf + i * 4, 4,
+				 snd_complete_urb, u, ep->syncinterval + 1);
+
 		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;
-- 
2.17.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 4/9 v2] ALSA: usb-audio: use usb_fill_int_urb()

[Takashi Iwai]

On Wed, 20 Jun 2018 11:38:27 +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.
> 
> data_ep_set_params() additionally sets urb->transfer_buffer_length which
> was not the case earlier.
> usb_fill_int_urb() ensures that syncinterval is within the allowed range
> on HS/SS. The interval value seems to come from
> snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage
> 1 … 4. So in order to keep the magic working I pass datainterval + 1.

This needs more explanation.  By this conversion, the interval
computation becomes really tricky.

There are two interval calculations.  The first one is
fp->datainterval and it's from snd_usb_parse_datainterval() as you
mentioned.  And a tricky part is that fp->datainterval is 0 on
FULL_SPEED.  Casually, fp->datainterval+1 will be translated to the
correct value since (0 + 1) == (1 << 0).

OTOH, for the sync EP, it's taken from ep->syncinterval, which is set
in snd_usb_add_endpoint().  Luckily, again, ep->syncinterval + 1 works
for FULL_SPEED as well, as (1 + 1) == (1 << 1).


thanks,

Takashi

> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> v1…v2: description changes (spelling + usb_fill_int_urb() ensures
>        "syncinterval" not data_ep_set_params())
> 
>  sound/usb/endpoint.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
> index c90607ebe155..bbc02db5b417 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,13 @@ 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_int_urb(u->urb, NULL, ep->pipe, buf, u->buffer_size,
> +				 snd_complete_urb, u, ep->datainterval + 1);
>  		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 +822,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_int_urb(u->urb, NULL, ep->pipe, ep->syncbuf + i * 4, 4,
> +				 snd_complete_urb, u, ep->syncinterval + 1);
> +
>  		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;
> -- 
> 2.17.1
> 
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 4/9 v2] ALSA: usb-audio: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

On 2018-06-20 15:21:49 [+0200], Takashi Iwai wrote:
> > usb_fill_int_urb() ensures that syncinterval is within the allowed range
> > on HS/SS. The interval value seems to come from
> > snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage
> > 1 … 4. So in order to keep the magic working I pass datainterval + 1.
> 
> This needs more explanation.  By this conversion, the interval
> computation becomes really tricky.
> 
> There are two interval calculations.  The first one is
> fp->datainterval and it's from snd_usb_parse_datainterval() as you
> mentioned.  And a tricky part is that fp->datainterval is 0 on
> FULL_SPEED.  Casually, fp->datainterval+1 will be translated to the
> correct value since (0 + 1) == (1 << 0).
> 
> OTOH, for the sync EP, it's taken from ep->syncinterval, which is set
> in snd_usb_add_endpoint().  Luckily, again, ep->syncinterval + 1 works
> for FULL_SPEED as well, as (1 + 1) == (1 << 1).

Do you want me to add your additional explanation to the description?
 
> thanks,
> 
> Takashi

Sebastian
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 4/9 v2] ALSA: usb-audio: use usb_fill_int_urb()

[Takashi Iwai]

On Wed, 20 Jun 2018 15:52:49 +0200,
Sebastian Andrzej Siewior wrote:
> 
> On 2018-06-20 15:21:49 [+0200], Takashi Iwai wrote:
> > > usb_fill_int_urb() ensures that syncinterval is within the allowed range
> > > on HS/SS. The interval value seems to come from
> > > snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage
> > > 1 … 4. So in order to keep the magic working I pass datainterval + 1.
> > 
> > This needs more explanation.  By this conversion, the interval
> > computation becomes really tricky.
> > 
> > There are two interval calculations.  The first one is
> > fp->datainterval and it's from snd_usb_parse_datainterval() as you
> > mentioned.  And a tricky part is that fp->datainterval is 0 on
> > FULL_SPEED.  Casually, fp->datainterval+1 will be translated to the
> > correct value since (0 + 1) == (1 << 0).
> > 
> > OTOH, for the sync EP, it's taken from ep->syncinterval, which is set
> > in snd_usb_add_endpoint().  Luckily, again, ep->syncinterval + 1 works
> > for FULL_SPEED as well, as (1 + 1) == (1 << 1).
> 
> Do you want me to add your additional explanation to the description?

Yes.  It's a very implicit assumption, and needs clarification.
In addition, the comment 1...4 is only for fp->datainterval, not about
ep->syncinterval.

Alternatively, give some comments in the places where putting
fp->datainterval and ep->syncinterval.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 4/9 v2] ALSA: usb-audio: use usb_fill_int_urb()

[Takashi Iwai]

On Wed, 20 Jun 2018 15:56:53 +0200,
Takashi Iwai wrote:
> 
> On Wed, 20 Jun 2018 15:52:49 +0200,
> Sebastian Andrzej Siewior wrote:
> > 
> > On 2018-06-20 15:21:49 [+0200], Takashi Iwai wrote:
> > > > usb_fill_int_urb() ensures that syncinterval is within the allowed range
> > > > on HS/SS. The interval value seems to come from
> > > > snd_usb_parse_datainterval() which is bInterval - 1 and only in the rage
> > > > 1 … 4. So in order to keep the magic working I pass datainterval + 1.
> > > 
> > > This needs more explanation.  By this conversion, the interval
> > > computation becomes really tricky.
> > > 
> > > There are two interval calculations.  The first one is
> > > fp->datainterval and it's from snd_usb_parse_datainterval() as you
> > > mentioned.  And a tricky part is that fp->datainterval is 0 on
> > > FULL_SPEED.  Casually, fp->datainterval+1 will be translated to the
> > > correct value since (0 + 1) == (1 << 0).
> > > 
> > > OTOH, for the sync EP, it's taken from ep->syncinterval, which is set
> > > in snd_usb_add_endpoint().  Luckily, again, ep->syncinterval + 1 works
> > > for FULL_SPEED as well, as (1 + 1) == (1 << 1).
> > 
> > Do you want me to add your additional explanation to the description?
> 
> Yes.  It's a very implicit assumption, and needs clarification.
> In addition, the comment 1...4 is only for fp->datainterval, not about
> ep->syncinterval.
> 
> Alternatively, give some comments in the places where putting
> fp->datainterval and ep->syncinterval.

Oh, and for other patches, something about interval could be
mentioned in the change log.  You pass 1 to the macro as if it were
identical as the original code (urb->interval = 1), but this isn't
evaluated equally.

Effectively it'll result in the same, but better to be clarified.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH 5/9] ALSA: usb: caiaq: audio: use irqsave() in USB's complete callback

[Sebastian Andrzej Siewior]

From: John Ogness <john.ogness@linutronix.de>

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: Daniel Mack <zonque@gmail.com>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 sound/usb/caiaq/audio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
index f35d29f49ffe..15344d39a6cd 100644
--- a/sound/usb/caiaq/audio.c
+++ b/sound/usb/caiaq/audio.c
@@ -636,6 +636,7 @@ static void read_completed(struct urb *urb)
 	struct device *dev;
 	struct urb *out = NULL;
 	int i, frame, len, send_it = 0, outframe = 0;
+	unsigned long flags;
 	size_t offset = 0;
 
 	if (urb->status || !info)
@@ -672,10 +673,10 @@ static void read_completed(struct urb *urb)
 		offset += len;
 
 		if (len > 0) {
-			spin_lock(&cdev->spinlock);
+			spin_lock_irqsave(&cdev->spinlock, flags);
 			fill_out_urb(cdev, out, &out->iso_frame_desc[outframe]);
 			read_in_urb(cdev, urb, &urb->iso_frame_desc[frame]);
-			spin_unlock(&cdev->spinlock);
+			spin_unlock_irqrestore(&cdev->spinlock, flags);
 			check_for_elapsed_periods(cdev, cdev->sub_playback);
 			check_for_elapsed_periods(cdev, cdev->sub_capture);
 			send_it = 1;
-- 
2.17.1

Re: [PATCH 5/9] ALSA: usb: caiaq: audio: use irqsave() in USB's complete callback

[Daniel Mack]

On Tuesday, June 19, 2018 11:55 PM, Sebastian Andrzej Siewior wrote:
> From: John Ogness <john.ogness@linutronix.de>
> 
> 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.
Acked-by: Daniel Mack <zonque@gmail.com>


> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>   sound/usb/caiaq/audio.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
> index f35d29f49ffe..15344d39a6cd 100644
> --- a/sound/usb/caiaq/audio.c
> +++ b/sound/usb/caiaq/audio.c
> @@ -636,6 +636,7 @@ static void read_completed(struct urb *urb)
>   	struct device *dev;
>   	struct urb *out = NULL;
>   	int i, frame, len, send_it = 0, outframe = 0;
> +	unsigned long flags;
>   	size_t offset = 0;
>   
>   	if (urb->status || !info)
> @@ -672,10 +673,10 @@ static void read_completed(struct urb *urb)
>   		offset += len;
>   
>   		if (len > 0) {
> -			spin_lock(&cdev->spinlock);
> +			spin_lock_irqsave(&cdev->spinlock, flags);
>   			fill_out_urb(cdev, out, &out->iso_frame_desc[outframe]);
>   			read_in_urb(cdev, urb, &urb->iso_frame_desc[frame]);
> -			spin_unlock(&cdev->spinlock);
> +			spin_unlock_irqrestore(&cdev->spinlock, flags);
>   			check_for_elapsed_periods(cdev, cdev->sub_playback);
>   			check_for_elapsed_periods(cdev, cdev->sub_capture);
>   			send_it = 1;
> 

[PATCH 6/9] ALSA: usb: caiaq: 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: Daniel Mack <zonque@gmail.com>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 sound/usb/caiaq/audio.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
index 15344d39a6cd..e10d5790099f 100644
--- a/sound/usb/caiaq/audio.c
+++ b/sound/usb/caiaq/audio.c
@@ -736,16 +736,17 @@ static struct urb **alloc_urbs(struct snd_usb_caiaqdev *cdev, int dir, int *ret)
 	}
 
 	for (i = 0; i < N_URBS; i++) {
+		void *buf;
+
 		urbs[i] = usb_alloc_urb(FRAMES_PER_URB, GFP_KERNEL);
 		if (!urbs[i]) {
 			*ret = -ENOMEM;
 			return urbs;
 		}
 
-		urbs[i]->transfer_buffer =
-			kmalloc_array(BYTES_PER_FRAME, FRAMES_PER_URB,
-				      GFP_KERNEL);
-		if (!urbs[i]->transfer_buffer) {
+		buf = kmalloc_array(BYTES_PER_FRAME, FRAMES_PER_URB,
+				    GFP_KERNEL);
+		if (!buf) {
 			*ret = -ENOMEM;
 			return urbs;
 		}
@@ -758,15 +759,13 @@ static struct urb **alloc_urbs(struct snd_usb_caiaqdev *cdev, int dir, int *ret)
 			iso->length = BYTES_PER_FRAME;
 		}
 
-		urbs[i]->dev = usb_dev;
-		urbs[i]->pipe = pipe;
-		urbs[i]->transfer_buffer_length = FRAMES_PER_URB
-						* BYTES_PER_FRAME;
-		urbs[i]->context = &cdev->data_cb_info[i];
-		urbs[i]->interval = 1;
+		usb_fill_int_urb(urbs[i], usb_dev, pipe, buf,
+				 FRAMES_PER_URB * BYTES_PER_FRAME,
+				 (dir == SNDRV_PCM_STREAM_CAPTURE) ?
+				 read_completed : write_completed,
+				 &cdev->data_cb_info[i], 1);
+
 		urbs[i]->number_of_packets = FRAMES_PER_URB;
-		urbs[i]->complete = (dir == SNDRV_PCM_STREAM_CAPTURE) ?
-					read_completed : write_completed;
 	}
 
 	*ret = 0;
-- 
2.17.1

Re: [PATCH 6/9] ALSA: usb: caiaq: use usb_fill_int_urb()

[Daniel Mack]

On Tuesday, June 19, 2018 11:55 PM, 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.

Acked-by: Daniel Mack <zonque@gmail.com>

> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>   sound/usb/caiaq/audio.c | 23 +++++++++++------------
>   1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
> index 15344d39a6cd..e10d5790099f 100644
> --- a/sound/usb/caiaq/audio.c
> +++ b/sound/usb/caiaq/audio.c
> @@ -736,16 +736,17 @@ static struct urb **alloc_urbs(struct snd_usb_caiaqdev *cdev, int dir, int *ret)
>   	}
>   
>   	for (i = 0; i < N_URBS; i++) {
> +		void *buf;
> +
>   		urbs[i] = usb_alloc_urb(FRAMES_PER_URB, GFP_KERNEL);
>   		if (!urbs[i]) {
>   			*ret = -ENOMEM;
>   			return urbs;
>   		}
>   
> -		urbs[i]->transfer_buffer =
> -			kmalloc_array(BYTES_PER_FRAME, FRAMES_PER_URB,
> -				      GFP_KERNEL);
> -		if (!urbs[i]->transfer_buffer) {
> +		buf = kmalloc_array(BYTES_PER_FRAME, FRAMES_PER_URB,
> +				    GFP_KERNEL);
> +		if (!buf) {
>   			*ret = -ENOMEM;
>   			return urbs;
>   		}
> @@ -758,15 +759,13 @@ static struct urb **alloc_urbs(struct snd_usb_caiaqdev *cdev, int dir, int *ret)
>   			iso->length = BYTES_PER_FRAME;
>   		}
>   
> -		urbs[i]->dev = usb_dev;
> -		urbs[i]->pipe = pipe;
> -		urbs[i]->transfer_buffer_length = FRAMES_PER_URB
> -						* BYTES_PER_FRAME;
> -		urbs[i]->context = &cdev->data_cb_info[i];
> -		urbs[i]->interval = 1;
> +		usb_fill_int_urb(urbs[i], usb_dev, pipe, buf,
> +				 FRAMES_PER_URB * BYTES_PER_FRAME,
> +				 (dir == SNDRV_PCM_STREAM_CAPTURE) ?
> +				 read_completed : write_completed,
> +				 &cdev->data_cb_info[i], 1);
> +
>   		urbs[i]->number_of_packets = FRAMES_PER_URB;
> -		urbs[i]->complete = (dir == SNDRV_PCM_STREAM_CAPTURE) ?
> -					read_completed : write_completed;
>   	}
>   
>   	*ret = 0;
> 

Re: [PATCH 6/9] ALSA: usb: caiaq: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

On 2018-06-21 22:19:32 [+0200], Daniel Mack wrote:
> On Tuesday, June 19, 2018 11:55 PM, 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.
> 
> Acked-by: Daniel Mack <zonque@gmail.com>

nope, please don't.
Takashi, please ignore the usb_fill_.* patches. I will be doing another
spin with usb_fill_iso_urb() instead.

Sebastian

Re: [PATCH 6/9] ALSA: usb: caiaq: use usb_fill_int_urb()

[Takashi Iwai]

On Thu, 21 Jun 2018 23:16:39 +0200,
Sebastian Andrzej Siewior wrote:
> 
> On 2018-06-21 22:19:32 [+0200], Daniel Mack wrote:
> > On Tuesday, June 19, 2018 11:55 PM, 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.
> > 
> > Acked-by: Daniel Mack <zonque@gmail.com>
> 
> nope, please don't.
> Takashi, please ignore the usb_fill_.* patches. I will be doing another
> spin with usb_fill_iso_urb() instead.

This sounds like a better approach, indeed.


thanks,

Takashi

Re: [PATCH 6/9] ALSA: usb: caiaq: use usb_fill_int_urb()

[Daniel Mack]

On Thursday, June 21, 2018 11:16 PM, Sebastian Andrzej Siewior wrote:
> On 2018-06-21 22:19:32 [+0200], Daniel Mack wrote:
>> On Tuesday, June 19, 2018 11:55 PM, 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.
>>
>> Acked-by: Daniel Mack <zonque@gmail.com>
> 
> nope, please don't.
> Takashi, please ignore the usb_fill_.* patches. I will be doing another
> spin with usb_fill_iso_urb() instead.

Hmm, there is no such thing as usb_fill_iso_urb() in my tree, are you 
going to add that?

The only part that needs attention is the interval parameter, which is 
passed to usb_fill_int_urb() as 1 now, and hence urb->interval will also 
be 1, just like the open-coded version before. Unless I miss anything, 
your conversion should work, but I haven't tested it yet.

But I agree the function name is misleading, so we should probably get a 
usb_fill_iso_urb() and use it where appropriate. AFAICS, it will be 
identical to usb_fill_bulk_urb(), as the endpoint type is encoded in the 
pipe. Maybe it's worth adding a check?


Thanks,
Daniel

Re: [PATCH 6/9] ALSA: usb: caiaq: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

On 2018-06-22 12:01:13 [+0200], Daniel Mack wrote:
> Hmm, there is no such thing as usb_fill_iso_urb() in my tree, are you going
> to add that?

Yes.

> The only part that needs attention is the interval parameter, which is
> passed to usb_fill_int_urb() as 1 now, and hence urb->interval will also be
> 1, just like the open-coded version before. Unless I miss anything, your
> conversion should work, but I haven't tested it yet.

It should work in most cases. The point is that the argument expects
bInterval (from the endpoint) which has a different encoding on FS vs
HS/SS for INTR endpoints but not for ISOC endpoints and I got this wrong
initially.

> But I agree the function name is misleading, so we should probably get a
> usb_fill_iso_urb() and use it where appropriate. AFAICS, it will be
> identical to usb_fill_bulk_urb(), as the endpoint type is encoded in the
> pipe. Maybe it's worth adding a check?

What check?
it should be identical to INTR without the speed check (always the HS
version should be used). I need to check if it makes sense to extend the
parameters to cover ->number_of_packets and so on.
Any way, I plan to first RFC the function, land it upstream and then
convert the users.

> Thanks,
> Daniel

Sebastian

[PATCH 7/9] ALSA: usb-midi: use irqsave() in USB's complete callback

[Sebastian Andrzej Siewior]

From: John Ogness <john.ogness@linutronix.de>

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: Clemens Ladisch <clemens@ladisch.de>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 sound/usb/midi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/usb/midi.c b/sound/usb/midi.c
index 2c1aaa3292bf..dcfc546d81b9 100644
--- a/sound/usb/midi.c
+++ b/sound/usb/midi.c
@@ -281,15 +281,16 @@ static void snd_usbmidi_out_urb_complete(struct urb *urb)
 	struct out_urb_context *context = urb->context;
 	struct snd_usb_midi_out_endpoint *ep = context->ep;
 	unsigned int urb_index;
+	unsigned long flags;
 
-	spin_lock(&ep->buffer_lock);
+	spin_lock_irqsave(&ep->buffer_lock, flags);
 	urb_index = context - ep->urbs;
 	ep->active_urbs &= ~(1 << urb_index);
 	if (unlikely(ep->drain_urbs)) {
 		ep->drain_urbs &= ~(1 << urb_index);
 		wake_up(&ep->drain_wait);
 	}
-	spin_unlock(&ep->buffer_lock);
+	spin_unlock_irqrestore(&ep->buffer_lock, flags);
 	if (urb->status < 0) {
 		int err = snd_usbmidi_urb_error(urb);
 		if (err < 0) {
-- 
2.17.1

[PATCH 8/9] ALSA: usx2y: usbusx2yaudio: 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.

The "&& !(*purb)->transfer_buffer" check has been removed because the
URB has been freshly allocated a few lines above so ->transfer_buffer
has to be NULL here.
The `dev' and `transfer_size' assignments have been moved from
usX2Y_urbs_start() to usX2Y_urbs_allocate() because they don't change
overtime.

Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 sound/usb/usx2y/usbusx2yaudio.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
index 2b833054e3b0..9a49bdb07508 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -425,6 +425,9 @@ static int usX2Y_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 = NULL;
+		unsigned int len = 0;
+
 		if (*purb) {
 			usb_kill_urb(*purb);
 			continue;
@@ -434,22 +437,18 @@ static int usX2Y_urbs_allocate(struct snd_usX2Y_substream *subs)
 			usX2Y_urbs_release(subs);
 			return -ENOMEM;
 		}
-		if (!is_playback && !(*purb)->transfer_buffer) {
+		if (!is_playback) {
 			/* allocate a capture buffer per urb */
-			(*purb)->transfer_buffer =
-				kmalloc_array(subs->maxpacksize,
-					      nr_of_packs(), GFP_KERNEL);
-			if (NULL == (*purb)->transfer_buffer) {
+			len = subs->maxpacksize * nr_of_packs();
+			buf = kmalloc(len, GFP_KERNEL);
+			if (!buf) {
 				usX2Y_urbs_release(subs);
 				return -ENOMEM;
 			}
 		}
-		(*purb)->dev = dev;
-		(*purb)->pipe = pipe;
+		usb_fill_int_urb(*purb, dev, pipe, buf, len,
+				 i_usX2Y_subs_startup, subs, 1);
 		(*purb)->number_of_packets = nr_of_packs();
-		(*purb)->context = subs;
-		(*purb)->interval = 1;
-		(*purb)->complete = i_usX2Y_subs_startup;
 	}
 	return 0;
 }
@@ -485,12 +484,10 @@ static int usX2Y_urbs_start(struct snd_usX2Y_substream *subs)
 			unsigned long pack;
 			if (0 == i)
 				atomic_set(&subs->state, state_STARTING3);
-			urb->dev = usX2Y->dev;
 			for (pack = 0; pack < nr_of_packs(); pack++) {
 				urb->iso_frame_desc[pack].offset = subs->maxpacksize * pack;
 				urb->iso_frame_desc[pack].length = subs->maxpacksize;
 			}
-			urb->transfer_buffer_length = subs->maxpacksize * nr_of_packs(); 
 			if ((err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
 				snd_printk (KERN_ERR "cannot submit datapipe for urb %d, err = %d\n", i, err);
 				err = -EPIPE;
-- 
2.17.1

Re: [PATCH 8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()

[Takashi Iwai]

On Tue, 19 Jun 2018 23:55:20 +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 "&& !(*purb)->transfer_buffer" check has been removed because the
> URB has been freshly allocated a few lines above so ->transfer_buffer
> has to be NULL here.
> The `dev' and `transfer_size' assignments have been moved from
> usX2Y_urbs_start() to usX2Y_urbs_allocate() because they don't change
> overtime.
> 
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  sound/usb/usx2y/usbusx2yaudio.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
> index 2b833054e3b0..9a49bdb07508 100644
> --- a/sound/usb/usx2y/usbusx2yaudio.c
> +++ b/sound/usb/usx2y/usbusx2yaudio.c
> @@ -425,6 +425,9 @@ static int usX2Y_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 = NULL;
> +		unsigned int len = 0;
> +
>  		if (*purb) {
>  			usb_kill_urb(*purb);
>  			continue;
> @@ -434,22 +437,18 @@ static int usX2Y_urbs_allocate(struct snd_usX2Y_substream *subs)
>  			usX2Y_urbs_release(subs);
>  			return -ENOMEM;
>  		}
> -		if (!is_playback && !(*purb)->transfer_buffer) {
> +		if (!is_playback) {
>  			/* allocate a capture buffer per urb */
> -			(*purb)->transfer_buffer =
> -				kmalloc_array(subs->maxpacksize,
> -					      nr_of_packs(), GFP_KERNEL);
> -			if (NULL == (*purb)->transfer_buffer) {
> +			len = subs->maxpacksize * nr_of_packs();
> +			buf = kmalloc(len, GFP_KERNEL);

I'd keep kmalloc_array() as is, and just put subs->maxpacksize *
nr_of_packs() in usb_fill_int_urb().  Otherwise it's a step backward.


thanks,

Takashi

Re: [PATCH 8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

On 2018-06-20 14:51:12 [+0200], Takashi Iwai wrote:
> On Tue, 19 Jun 2018 23:55:20 +0200,
> Sebastian Andrzej Siewior wrote:
> > -			(*purb)->transfer_buffer =
> > -				kmalloc_array(subs->maxpacksize,
> > -					      nr_of_packs(), GFP_KERNEL);
> > -			if (NULL == (*purb)->transfer_buffer) {
> > +			len = subs->maxpacksize * nr_of_packs();
> > +			buf = kmalloc(len, GFP_KERNEL);
> 
> I'd keep kmalloc_array() as is, and just put subs->maxpacksize *
> nr_of_packs() in usb_fill_int_urb().  Otherwise it's a step backward.

but then we end up with two multiplications. What about pulling the
overflow-mul macro out of malloc_array() and doing this instead:

----------- >8

Subject: [PATCH 8/9 v2] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()

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 "&& !(*purb)->transfer_buffer" check has been removed because the
URB has been freshly allocated a few lines above so ->transfer_buffer
has to be NULL here.
The `dev' and `transfer_size' assignments have been moved from
usX2Y_urbs_start() to usX2Y_urbs_allocate() because they don't change
overtime.
The multiplication via check_mul_overflow() has been extracted from
kmalloc_array() to avoid two multiplication (one with overflow check and
one without for the length argument). This requires to change the type
`maxpacksize' to int so there is only one type involved in the
multiplication.

Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 sound/usb/usx2y/usbusx2y.h      |  2 +-
 sound/usb/usx2y/usbusx2yaudio.c | 38 ++++++++++++++++-----------------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/sound/usb/usx2y/usbusx2y.h b/sound/usb/usx2y/usbusx2y.h
index e0f77172ce8f..2bec412589b4 100644
--- a/sound/usb/usx2y/usbusx2y.h
+++ b/sound/usb/usx2y/usbusx2y.h
@@ -56,7 +56,7 @@ struct snd_usX2Y_substream {
 	struct snd_pcm_substream *pcm_substream;
 
 	int			endpoint;		
-	unsigned int		maxpacksize;		/* max packet size in bytes */
+	int			maxpacksize;		/* max packet size in bytes */
 
 	atomic_t		state;
 #define state_STOPPED	0
diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
index 2b833054e3b0..e5580cb59858 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -425,33 +425,35 @@ static int usX2Y_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 = NULL;
+		int len = 0;
+
 		if (*purb) {
 			usb_kill_urb(*purb);
 			continue;
 		}
 		*purb = usb_alloc_urb(nr_of_packs(), GFP_KERNEL);
-		if (NULL == *purb) {
-			usX2Y_urbs_release(subs);
-			return -ENOMEM;
-		}
-		if (!is_playback && !(*purb)->transfer_buffer) {
+		if (NULL == *purb)
+			goto err_free;
+
+		if (!is_playback) {
 			/* allocate a capture buffer per urb */
-			(*purb)->transfer_buffer =
-				kmalloc_array(subs->maxpacksize,
-					      nr_of_packs(), GFP_KERNEL);
-			if (NULL == (*purb)->transfer_buffer) {
-				usX2Y_urbs_release(subs);
-				return -ENOMEM;
-			}
+			if (check_mul_overflow(subs->maxpacksize,
+					       nr_of_packs(),
+					       &len))
+				goto err_free;
+			buf = kmalloc(len, GFP_KERNEL);
+			if (!buf)
+				goto err_free;
 		}
-		(*purb)->dev = dev;
-		(*purb)->pipe = pipe;
+		usb_fill_int_urb(*purb, dev, pipe, buf, len,
+				 i_usX2Y_subs_startup, subs, 1);
 		(*purb)->number_of_packets = nr_of_packs();
-		(*purb)->context = subs;
-		(*purb)->interval = 1;
-		(*purb)->complete = i_usX2Y_subs_startup;
 	}
 	return 0;
+err_free:
+	usX2Y_urbs_release(subs);
+	return -ENOMEM;
 }
 
 static void usX2Y_subs_startup(struct snd_usX2Y_substream *subs)
@@ -485,12 +487,10 @@ static int usX2Y_urbs_start(struct snd_usX2Y_substream *subs)
 			unsigned long pack;
 			if (0 == i)
 				atomic_set(&subs->state, state_STARTING3);
-			urb->dev = usX2Y->dev;
 			for (pack = 0; pack < nr_of_packs(); pack++) {
 				urb->iso_frame_desc[pack].offset = subs->maxpacksize * pack;
 				urb->iso_frame_desc[pack].length = subs->maxpacksize;
 			}
-			urb->transfer_buffer_length = subs->maxpacksize * nr_of_packs(); 
 			if ((err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
 				snd_printk (KERN_ERR "cannot submit datapipe for urb %d, err = %d\n", i, err);
 				err = -EPIPE;
-- 
2.17.1

Re: [PATCH 8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()

[Takashi Iwai]

On Wed, 20 Jun 2018 16:39:22 +0200,
Sebastian Andrzej Siewior wrote:
> 
> On 2018-06-20 14:51:12 [+0200], Takashi Iwai wrote:
> > On Tue, 19 Jun 2018 23:55:20 +0200,
> > Sebastian Andrzej Siewior wrote:
> > > -			(*purb)->transfer_buffer =
> > > -				kmalloc_array(subs->maxpacksize,
> > > -					      nr_of_packs(), GFP_KERNEL);
> > > -			if (NULL == (*purb)->transfer_buffer) {
> > > +			len = subs->maxpacksize * nr_of_packs();
> > > +			buf = kmalloc(len, GFP_KERNEL);
> > 
> > I'd keep kmalloc_array() as is, and just put subs->maxpacksize *
> > nr_of_packs() in usb_fill_int_urb().  Otherwise it's a step backward.
> 
> but then we end up with two multiplications.

Yes, but it's no hot path, and the code won't bigger.
And I bet you won't notice any performance lost by that :)

> What about pulling the
> overflow-mul macro out of malloc_array() and doing this instead:

Well, it's neither smaller nor more readable...


thanks,

Takashi

> 
> ----------- >8
> 
> Subject: [PATCH 8/9 v2] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
> 
> 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 "&& !(*purb)->transfer_buffer" check has been removed because the
> URB has been freshly allocated a few lines above so ->transfer_buffer
> has to be NULL here.
> The `dev' and `transfer_size' assignments have been moved from
> usX2Y_urbs_start() to usX2Y_urbs_allocate() because they don't change
> overtime.
> The multiplication via check_mul_overflow() has been extracted from
> kmalloc_array() to avoid two multiplication (one with overflow check and
> one without for the length argument). This requires to change the type
> `maxpacksize' to int so there is only one type involved in the
> multiplication.
> 
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  sound/usb/usx2y/usbusx2y.h      |  2 +-
>  sound/usb/usx2y/usbusx2yaudio.c | 38 ++++++++++++++++-----------------
>  2 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/sound/usb/usx2y/usbusx2y.h b/sound/usb/usx2y/usbusx2y.h
> index e0f77172ce8f..2bec412589b4 100644
> --- a/sound/usb/usx2y/usbusx2y.h
> +++ b/sound/usb/usx2y/usbusx2y.h
> @@ -56,7 +56,7 @@ struct snd_usX2Y_substream {
>  	struct snd_pcm_substream *pcm_substream;
>  
>  	int			endpoint;		
> -	unsigned int		maxpacksize;		/* max packet size in bytes */
> +	int			maxpacksize;		/* max packet size in bytes */
>  
>  	atomic_t		state;
>  #define state_STOPPED	0
> diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
> index 2b833054e3b0..e5580cb59858 100644
> --- a/sound/usb/usx2y/usbusx2yaudio.c
> +++ b/sound/usb/usx2y/usbusx2yaudio.c
> @@ -425,33 +425,35 @@ static int usX2Y_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 = NULL;
> +		int len = 0;
> +
>  		if (*purb) {
>  			usb_kill_urb(*purb);
>  			continue;
>  		}
>  		*purb = usb_alloc_urb(nr_of_packs(), GFP_KERNEL);
> -		if (NULL == *purb) {
> -			usX2Y_urbs_release(subs);
> -			return -ENOMEM;
> -		}
> -		if (!is_playback && !(*purb)->transfer_buffer) {
> +		if (NULL == *purb)
> +			goto err_free;
> +
> +		if (!is_playback) {
>  			/* allocate a capture buffer per urb */
> -			(*purb)->transfer_buffer =
> -				kmalloc_array(subs->maxpacksize,
> -					      nr_of_packs(), GFP_KERNEL);
> -			if (NULL == (*purb)->transfer_buffer) {
> -				usX2Y_urbs_release(subs);
> -				return -ENOMEM;
> -			}
> +			if (check_mul_overflow(subs->maxpacksize,
> +					       nr_of_packs(),
> +					       &len))
> +				goto err_free;
> +			buf = kmalloc(len, GFP_KERNEL);
> +			if (!buf)
> +				goto err_free;
>  		}
> -		(*purb)->dev = dev;
> -		(*purb)->pipe = pipe;
> +		usb_fill_int_urb(*purb, dev, pipe, buf, len,
> +				 i_usX2Y_subs_startup, subs, 1);
>  		(*purb)->number_of_packets = nr_of_packs();
> -		(*purb)->context = subs;
> -		(*purb)->interval = 1;
> -		(*purb)->complete = i_usX2Y_subs_startup;
>  	}
>  	return 0;
> +err_free:
> +	usX2Y_urbs_release(subs);
> +	return -ENOMEM;
>  }
>  
>  static void usX2Y_subs_startup(struct snd_usX2Y_substream *subs)
> @@ -485,12 +487,10 @@ static int usX2Y_urbs_start(struct snd_usX2Y_substream *subs)
>  			unsigned long pack;
>  			if (0 == i)
>  				atomic_set(&subs->state, state_STARTING3);
> -			urb->dev = usX2Y->dev;
>  			for (pack = 0; pack < nr_of_packs(); pack++) {
>  				urb->iso_frame_desc[pack].offset = subs->maxpacksize * pack;
>  				urb->iso_frame_desc[pack].length = subs->maxpacksize;
>  			}
> -			urb->transfer_buffer_length = subs->maxpacksize * nr_of_packs(); 
>  			if ((err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
>  				snd_printk (KERN_ERR "cannot submit datapipe for urb %d, err = %d\n", i, err);
>  				err = -EPIPE;
> -- 
> 2.17.1
> 

Re: [PATCH 8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

On 2018-06-20 16:52:45 [+0200], Takashi Iwai wrote:
> > but then we end up with two multiplications.
> 
> Yes, but it's no hot path, and the code won't bigger.
> And I bet you won't notice any performance lost by that :)
> 
> > What about pulling the
> > overflow-mul macro out of malloc_array() and doing this instead:
> 
> Well, it's neither smaller nor more readable...

okay, as you wish:

> thanks,
> 
> Takashi

----------- >8

Subject: [PATCH 8/9 v3] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()

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 "&& !(*purb)->transfer_buffer" check has been removed because the
URB has been freshly allocated a few lines above so ->transfer_buffer
has to be NULL here.
The `dev' and `transfer_size' assignments have been moved from
usX2Y_urbs_start() to usX2Y_urbs_allocate() because they don't change
overtime.
The multiplication via check_mul_overflow() has been extracted from
kmalloc_array() to avoid two multiplication (one with overflow check and
one without for the length argument). This requires to change the type
`maxpacksize' to int so there is only one type involved in the
multiplication.

Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 sound/usb/usx2y/usbusx2yaudio.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
index 2b833054e3b0..ee8d21a784fd 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -425,6 +425,9 @@ static int usX2Y_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 = NULL;
+		int len = 0;
+
 		if (*purb) {
 			usb_kill_urb(*purb);
 			continue;
@@ -434,22 +437,20 @@ static int usX2Y_urbs_allocate(struct snd_usX2Y_substream *subs)
 			usX2Y_urbs_release(subs);
 			return -ENOMEM;
 		}
-		if (!is_playback && !(*purb)->transfer_buffer) {
+
+		if (!is_playback) {
 			/* allocate a capture buffer per urb */
-			(*purb)->transfer_buffer =
-				kmalloc_array(subs->maxpacksize,
-					      nr_of_packs(), GFP_KERNEL);
-			if (NULL == (*purb)->transfer_buffer) {
+			buf = kmalloc_array(subs->maxpacksize, nr_of_packs,
+					    GFP_KERNEL);
+			if (!buf) {
 				usX2Y_urbs_release(subs);
 				return -ENOMEM;
 			}
+			len = subs->maxpacksize * nr_of_packs;
 		}
-		(*purb)->dev = dev;
-		(*purb)->pipe = pipe;
+		usb_fill_int_urb(*purb, dev, pipe, buf, len,
+				 i_usX2Y_subs_startup, subs, 1);
 		(*purb)->number_of_packets = nr_of_packs();
-		(*purb)->context = subs;
-		(*purb)->interval = 1;
-		(*purb)->complete = i_usX2Y_subs_startup;
 	}
 	return 0;
 }
@@ -485,12 +486,10 @@ static int usX2Y_urbs_start(struct snd_usX2Y_substream *subs)
 			unsigned long pack;
 			if (0 == i)
 				atomic_set(&subs->state, state_STARTING3);
-			urb->dev = usX2Y->dev;
 			for (pack = 0; pack < nr_of_packs(); pack++) {
 				urb->iso_frame_desc[pack].offset = subs->maxpacksize * pack;
 				urb->iso_frame_desc[pack].length = subs->maxpacksize;
 			}
-			urb->transfer_buffer_length = subs->maxpacksize * nr_of_packs(); 
 			if ((err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
 				snd_printk (KERN_ERR "cannot submit datapipe for urb %d, err = %d\n", i, err);
 				err = -EPIPE;
-- 
2.17.1

Re: [PATCH 8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()

[Sebastian Andrzej Siewior]

On 2018-06-20 17:38:44 [+0200], To Takashi Iwai wrote:
> okay, as you wish:

I'm sorry, I compiled one patch and send the other. Here is the fixed
one.

> > thanks,
> > 
> > Takashi
> 
----------- >8
Subject: [PATCH 8/9 v4] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()

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 "&& !(*purb)->transfer_buffer" check has been removed because the
URB has been freshly allocated a few lines above so ->transfer_buffer
has to be NULL here.
The `dev' and `transfer_size' assignments have been moved from
usX2Y_urbs_start() to usX2Y_urbs_allocate() because they don't change
overtime.
The multiplication via check_mul_overflow() has been extracted from
kmalloc_array() to avoid two multiplication (one with overflow check and
one without for the length argument). This requires to change the type
`maxpacksize' to int so there is only one type involved in the
multiplication.

Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 sound/usb/usx2y/usbusx2yaudio.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
index 2b833054e3b0..de3a21444db3 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -425,6 +425,9 @@ static int usX2Y_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 = NULL;
+		int len = 0;
+
 		if (*purb) {
 			usb_kill_urb(*purb);
 			continue;
@@ -434,22 +437,20 @@ static int usX2Y_urbs_allocate(struct snd_usX2Y_substream *subs)
 			usX2Y_urbs_release(subs);
 			return -ENOMEM;
 		}
-		if (!is_playback && !(*purb)->transfer_buffer) {
+
+		if (!is_playback) {
 			/* allocate a capture buffer per urb */
-			(*purb)->transfer_buffer =
-				kmalloc_array(subs->maxpacksize,
-					      nr_of_packs(), GFP_KERNEL);
-			if (NULL == (*purb)->transfer_buffer) {
+			buf = kmalloc_array(subs->maxpacksize, nr_of_packs(),
+					    GFP_KERNEL);
+			if (!buf) {
 				usX2Y_urbs_release(subs);
 				return -ENOMEM;
 			}
+			len = subs->maxpacksize * nr_of_packs();
 		}
-		(*purb)->dev = dev;
-		(*purb)->pipe = pipe;
+		usb_fill_int_urb(*purb, dev, pipe, buf, len,
+				 i_usX2Y_subs_startup, subs, 1);
 		(*purb)->number_of_packets = nr_of_packs();
-		(*purb)->context = subs;
-		(*purb)->interval = 1;
-		(*purb)->complete = i_usX2Y_subs_startup;
 	}
 	return 0;
 }
@@ -485,12 +486,10 @@ static int usX2Y_urbs_start(struct snd_usX2Y_substream *subs)
 			unsigned long pack;
 			if (0 == i)
 				atomic_set(&subs->state, state_STARTING3);
-			urb->dev = usX2Y->dev;
 			for (pack = 0; pack < nr_of_packs(); pack++) {
 				urb->iso_frame_desc[pack].offset = subs->maxpacksize * pack;
 				urb->iso_frame_desc[pack].length = subs->maxpacksize;
 			}
-			urb->transfer_buffer_length = subs->maxpacksize * nr_of_packs(); 
 			if ((err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
 				snd_printk (KERN_ERR "cannot submit datapipe for urb %d, err = %d\n", i, err);
 				err = -EPIPE;
-- 
2.17.1

Re: [PATCH 8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()

[Takashi Iwai]

On Wed, 20 Jun 2018 17:47:01 +0200,
Sebastian Andrzej Siewior wrote:
> 
> On 2018-06-20 17:38:44 [+0200], To Takashi Iwai wrote:
> > okay, as you wish:
> 
> I'm sorry, I compiled one patch and send the other. Here is the fixed
> one.

Well, you seem to have forgotten to update the changelog...

Don't need to rush, it's a change for 4.19.


thanks,

Takashi

> 
> > > thanks,
> > > 
> > > Takashi
> > 
> ----------- >8
> Subject: [PATCH 8/9 v4] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
> 
> 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 "&& !(*purb)->transfer_buffer" check has been removed because the
> URB has been freshly allocated a few lines above so ->transfer_buffer
> has to be NULL here.
> The `dev' and `transfer_size' assignments have been moved from
> usX2Y_urbs_start() to usX2Y_urbs_allocate() because they don't change
> overtime.
> The multiplication via check_mul_overflow() has been extracted from
> kmalloc_array() to avoid two multiplication (one with overflow check and
> one without for the length argument). This requires to change the type
> `maxpacksize' to int so there is only one type involved in the
> multiplication.
> 
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  sound/usb/usx2y/usbusx2yaudio.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
> index 2b833054e3b0..de3a21444db3 100644
> --- a/sound/usb/usx2y/usbusx2yaudio.c
> +++ b/sound/usb/usx2y/usbusx2yaudio.c
> @@ -425,6 +425,9 @@ static int usX2Y_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 = NULL;
> +		int len = 0;
> +
>  		if (*purb) {
>  			usb_kill_urb(*purb);
>  			continue;
> @@ -434,22 +437,20 @@ static int usX2Y_urbs_allocate(struct snd_usX2Y_substream *subs)
>  			usX2Y_urbs_release(subs);
>  			return -ENOMEM;
>  		}
> -		if (!is_playback && !(*purb)->transfer_buffer) {
> +
> +		if (!is_playback) {
>  			/* allocate a capture buffer per urb */
> -			(*purb)->transfer_buffer =
> -				kmalloc_array(subs->maxpacksize,
> -					      nr_of_packs(), GFP_KERNEL);
> -			if (NULL == (*purb)->transfer_buffer) {
> +			buf = kmalloc_array(subs->maxpacksize, nr_of_packs(),
> +					    GFP_KERNEL);
> +			if (!buf) {
>  				usX2Y_urbs_release(subs);
>  				return -ENOMEM;
>  			}
> +			len = subs->maxpacksize * nr_of_packs();
>  		}
> -		(*purb)->dev = dev;
> -		(*purb)->pipe = pipe;
> +		usb_fill_int_urb(*purb, dev, pipe, buf, len,
> +				 i_usX2Y_subs_startup, subs, 1);
>  		(*purb)->number_of_packets = nr_of_packs();
> -		(*purb)->context = subs;
> -		(*purb)->interval = 1;
> -		(*purb)->complete = i_usX2Y_subs_startup;
>  	}
>  	return 0;
>  }
> @@ -485,12 +486,10 @@ static int usX2Y_urbs_start(struct snd_usX2Y_substream *subs)
>  			unsigned long pack;
>  			if (0 == i)
>  				atomic_set(&subs->state, state_STARTING3);
> -			urb->dev = usX2Y->dev;
>  			for (pack = 0; pack < nr_of_packs(); pack++) {
>  				urb->iso_frame_desc[pack].offset = subs->maxpacksize * pack;
>  				urb->iso_frame_desc[pack].length = subs->maxpacksize;
>  			}
> -			urb->transfer_buffer_length = subs->maxpacksize * nr_of_packs(); 
>  			if ((err = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
>  				snd_printk (KERN_ERR "cannot submit datapipe for urb %d, err = %d\n", i, err);
>  				err = -EPIPE;
> -- 
> 2.17.1
> 

[PATCH 9/9] ALSA: usx2y: usx2yhwdeppcm: 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.
I'm keeping the transfer-length initialisation in
usX2Y_usbpcm_urbs_start() because I am not certain if this does not
change over time.

Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 sound/usb/usx2y/usx2yhwdeppcm.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/sound/usb/usx2y/usx2yhwdeppcm.c b/sound/usb/usx2y/usx2yhwdeppcm.c
index 4fd9276b8e50..0a14612f2178 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,19 @@ 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);
+		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_int_urb(*purb, dev, pipe, buf,
+				 subs->maxpacksize * nr_of_packs(),
+				 i_usX2Y_usbpcm_subs_startup, subs, 1);
 
-		(*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;
 	}
 	return 0;
 }
-- 
2.17.1

Re: ALSA: use irqsave() in URB completion + usb_fill_int_urb

[Takashi Iwai]

On Tue, 19 Jun 2018 23:55:12 +0200,
Sebastian Andrzej Siewior wrote:
> 
> 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.

At the next time, please split to two different patchsets in such a
case.  The conversions to usb_fill_int_urb() and the change to
_irqsave() are completely different things; the former is merely a
code cleanup that doesn't give any functional change, while the latter
is the followup for the USB core change.


thanks,

Takashi