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