* [8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
@ 2018-06-19 21:55 Sebastian Andrzej Siewior
0 siblings, 0 replies; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-06-19 21:55 UTC (permalink / raw)
To: alsa-devel
Cc: linux-usb, tglx, Takashi Iwai, Jaroslav Kysela,
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;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
@ 2018-06-20 12:51 Takashi Iwai
0 siblings, 0 replies; 7+ messages in thread
From: Takashi Iwai @ 2018-06-20 12:51 UTC (permalink / raw)
To: Sebastian Andrzej Siewior; +Cc: alsa-devel, tglx, Jaroslav Kysela, linux-usb
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
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* [8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
@ 2018-06-20 14:39 Sebastian Andrzej Siewior
0 siblings, 0 replies; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-06-20 14:39 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel, tglx, Jaroslav Kysela, linux-usb
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;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
@ 2018-06-20 14:52 Takashi Iwai
0 siblings, 0 replies; 7+ messages in thread
From: Takashi Iwai @ 2018-06-20 14:52 UTC (permalink / raw)
To: Sebastian Andrzej Siewior; +Cc: alsa-devel, tglx, Jaroslav Kysela, linux-usb
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
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
* [8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
@ 2018-06-20 15:38 Sebastian Andrzej Siewior
0 siblings, 0 replies; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-06-20 15:38 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel, tglx, Jaroslav Kysela, linux-usb
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;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
@ 2018-06-20 15:47 Sebastian Andrzej Siewior
0 siblings, 0 replies; 7+ messages in thread
From: Sebastian Andrzej Siewior @ 2018-06-20 15:47 UTC (permalink / raw)
To: Takashi Iwai; +Cc: alsa-devel, tglx, Jaroslav Kysela, linux-usb
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;
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb()
@ 2018-06-20 16:20 Takashi Iwai
0 siblings, 0 replies; 7+ messages in thread
From: Takashi Iwai @ 2018-06-20 16:20 UTC (permalink / raw)
To: Sebastian Andrzej Siewior; +Cc: alsa-devel, tglx, Jaroslav Kysela, linux-usb
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
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-06-20 16:20 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-19 21:55 [8/9] ALSA: usx2y: usbusx2yaudio: use usb_fill_int_urb() Sebastian Andrzej Siewior
-- strict thread matches above, loose matches on Subject: below --
2018-06-20 12:51 Takashi Iwai
2018-06-20 14:39 Sebastian Andrzej Siewior
2018-06-20 14:52 Takashi Iwai
2018-06-20 15:38 Sebastian Andrzej Siewior
2018-06-20 15:47 Sebastian Andrzej Siewior
2018-06-20 16:20 Takashi Iwai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).