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;

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;
  
  

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;

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;

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;

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