[alsa-devel] [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 at perex.cz>
> Cc: Takashi Iwai <tiwai at suse.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy at 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
> 
>