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

[Takashi Iwai]

At Sun, 01 Feb 2015 10:29:24 +0100,
Takashi Iwai wrote:
> 
> 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.

After reading through this patch, I now wonder whether the standard
usb-audio would work with a simple quirk for the PCM of this device.
If so, it'd be easier to merge bcd2000 stuff into usb-audio, as a
quirk for MIDI and additional controls, where already some devices do.

Writing the whole stuff from the scratch is error-prone.  With the
merger, you'll get the suspend/resume, auto PM, and the proper
disconnection with gratis.


thanks,

Takashi