diff mbox

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

Message ID 20180620143922.nkcazvbcrmkyvm2c@linutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Sebastian Andrzej Siewior June 20, 2018, 2:39 p.m. UTC
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(-)

Comments

Takashi Iwai June 20, 2018, 2:52 p.m. UTC | #1
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
>
diff mbox

Patch

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;