[alsa-devel] [PATCH 2/3] snd-bcd2000: add playback and capture support

[Takashi Iwai]

At Sat, 31 Jan 2015 22:34:24 +0100,
Mario Kicherer wrote:
> 
> +/* handle incoming URB with captured data */
> +static void bcd2000_pcm_in_urb_handler(struct urb *usb_urb)
> +{
> +	struct bcd2000_urb *bcd2k_urb = usb_urb->context;
> +	struct bcd2000_pcm *pcm = &bcd2k_urb->bcd2k->pcm;
> +	struct bcd2000_substream *stream = bcd2k_urb->stream;
> +	unsigned long flags;
> +	int ret, k, period_bytes;
> +	struct usb_iso_packet_descriptor *packet;
> +
> +	if (pcm->panic || stream->state == STREAM_STOPPING)
> +		return;

Don't you need protection for these flags and state variables?

> +
> +	if (unlikely(usb_urb->status == -ENOENT ||	/* unlinked */
> +		usb_urb->status == -ENODEV ||		/* device removed */
> +		usb_urb->status == -ECONNRESET ||	/* unlinked */
> +		usb_urb->status == -ESHUTDOWN))		/* device disabled */
> +	{
> +		goto out_fail;

This isn't always an error, e.g. active URBS at disconnection.

> +	}
> +
> +	if (stream->state == STREAM_STARTING) {
> +		stream->wait_cond = true;
> +		wake_up(&stream->wait_queue);
> +	}
> +
> +	if (stream->active) {
> +		spin_lock_irqsave(&stream->lock, flags);
> +
> +		/* copy captured data into ALSA buffer */
> +		bcd2000_pcm_capture(stream, bcd2k_urb);
> +
> +		period_bytes = snd_pcm_lib_period_bytes(stream->instance);
> +
> +		/* do we have enough data for one period? */
> +		if (stream->period_off > period_bytes) {
> +			stream->period_off %= period_bytes;
> +
> +			spin_unlock_irqrestore(&stream->lock, flags);
> +
> +			/*
> +			 * call this only once even if multiple periods
> +			 * are ready
> +			 */
> +			snd_pcm_period_elapsed(stream->instance);
> +
> +			memset(bcd2k_urb->buffer, 0, USB_BUFFER_SIZE);
> +		} else {
> +			spin_unlock_irqrestore(&stream->lock, flags);
> +			memset(bcd2k_urb->buffer, 0, USB_BUFFER_SIZE);
> +		}
> +	} else {
> +		memset(bcd2k_urb->buffer, 0, USB_BUFFER_SIZE);

We see three same lines of memset...  It shows something to improve.


> +	}
> +
> +	/* reset URB data */
> +	for (k = 0; k < USB_N_PACKETS_PER_URB; k++) {
> +		packet = &bcd2k_urb->packets[k];
> +		packet->offset = k * USB_PACKET_SIZE;
> +		packet->length = USB_PACKET_SIZE;
> +		packet->actual_length = 0;
> +		packet->status = 0;
> +	}
> +	bcd2k_urb->instance.number_of_packets = USB_N_PACKETS_PER_URB;
> +
> +	/* send the URB back to the BCD2000 */
> +	ret = usb_submit_urb(&bcd2k_urb->instance, GFP_ATOMIC);
> +	if (ret < 0)
> +		goto out_fail;
> +
> +	return;
> +
> +out_fail:
> +	dev_info(&bcd2k_urb->bcd2k->dev->dev, PREFIX "error in in_urb handler");
> +	pcm->panic = true;

How is pcm->panic recovered?
I see it's cleared in bcd2000_pcm_stream_start().  But then some
doubts:

- who will refill / resubmit urbs at restart?

- the flag is common to both playback and capture, and it's cleared in
  the trigger for one direction?

Also, does the disconnection while playing/recording works reliably?
snd_card_disconnect() doesn't close the PCM stream by itself, it
merely triggers STOP and sets the state to DISCONNECTED.  That is,
with your code, the URBs are still pending, and it's killed after
disconnection callback returns.

BTW, a similar (but basically more severe) problem exists for the
MIDI.  You shouldn't release the resources right after
snd_card_disconnect().  The streams might be still alive.  Usually the
resources have to be released in the free callback, not at
disconnection.


Takashi