Re: [alsa-devel] [PATCH v2] Add M2Tech hiFace USB-SPDIF driver

At Wed, 13 Feb 2013 18:11:45 +0100, Antonio Ospite wrote:

new file mode 100644 index 0000000..d0c3c58 --- /dev/null +++ b/sound/usb/hiface/pcm.c @@ -0,0 +1,638 @@

....

+struct pcm_runtime {

• struct hiface_chip *chip;
• struct snd_pcm *instance;
• struct pcm_substream playback;
• bool panic; /* if set driver won't do anymore pcm on device */

Once when rt->panic is set, how can you recover? I see no code resetting rt->panic.

+/* The hardware wants word-swapped 32-bit values */ +static void memcpy_swahw32(u8 *dest, u8 *src, unsigned int n) +{

• unsigned int i;
• for (i = 0; i < n / 4; i++)

• ((u32 *)dest)[i] = swahw32(((u32 *)src)[i]);
  

+}

+/* call with substream locked */ +/* returns true if a period elapsed */ +static bool hiface_pcm_playback(struct pcm_substream *sub,

• struct pcm_urb *urb)
  

+{

• struct snd_pcm_runtime *alsa_rt = sub->instance->runtime;
• u8 *source;
• unsigned int pcm_buffer_size;
• WARN_ON(alsa_rt->format != SNDRV_PCM_FORMAT_S32_LE);

Can't we use simply SNDRV_PCM_FORMAT_S32_BE without the byte swapping above?

+static void hiface_pcm_out_urb_handler(struct urb *usb_urb) +{

• struct pcm_urb *out_urb = usb_urb->context;
• struct pcm_runtime *rt = out_urb->chip->pcm;
• struct pcm_substream *sub;
• bool do_period_elapsed = false;
• unsigned long flags;
• int ret;
• pr_debug("%s: called.\n", __func__);
• if (rt->panic || rt->stream_state == STREAM_STOPPING)

• return;
  

• 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;
  

• }
• if (rt->stream_state == STREAM_STARTING) {

• rt->stream_wait_cond = true;
  

• wake_up(&rt->stream_wait_queue);
  

• }
• /* now send our playback data (if a free out urb was found) */
• sub = &rt->playback;
• spin_lock_irqsave(&sub->lock, flags);
• if (sub->active)

• do_period_elapsed = hiface_pcm_playback(sub, out_urb);
  

• else

• memset(out_urb->buffer, 0, PCM_PACKET_SIZE);
  

• spin_unlock_irqrestore(&sub->lock, flags);
• if (do_period_elapsed)

• snd_pcm_period_elapsed(sub->instance);
  

This looks like a small open race. sub->instance might be set to NULL after the unlock above?

• ret = usb_submit_urb(&out_urb->instance, GFP_ATOMIC);
• if (ret < 0)

• goto out_fail;
  

Maybe better to check the state again before resubmission, e.g. when XRUN is detected in the pointer callback.

• return;

+out_fail:

• rt->panic = true;

+}

+static int hiface_pcm_open(struct snd_pcm_substream *alsa_sub) +{

• struct pcm_runtime *rt = snd_pcm_substream_chip(alsa_sub);
• struct pcm_substream *sub = NULL;
• struct snd_pcm_runtime *alsa_rt = alsa_sub->runtime;
• int ret;
• pr_debug("%s: called.\n", __func__);
• if (rt->panic)

• return -EPIPE;
  

• mutex_lock(&rt->stream_mutex);
• alsa_rt->hw = pcm_hw;
• if (alsa_sub->stream == SNDRV_PCM_STREAM_PLAYBACK)

• sub = &rt->playback;
  

• if (!sub) {

• mutex_unlock(&rt->stream_mutex);
  

• pr_err("Invalid stream type\n");
  

• return -EINVAL;
  

• }
• if (rt->extra_freq) {

• alsa_rt->hw.rates |= SNDRV_PCM_RATE_KNOT;
  

• alsa_rt->hw.rate_max = 384000;
  

• /* explicit constraints needed as we added SNDRV_PCM_RATE_KNOT */
  

• ret = snd_pcm_hw_constraint_list(alsa_sub->runtime, 0,
  

• 				 SNDRV_PCM_HW_PARAM_RATE,
  

• 				 &constraints_extra_rates);
  

• if (ret < 0) {
  

• 	mutex_unlock(&rt->stream_mutex);
  

• 	return ret;
  

• }
  

• }
• sub->instance = alsa_sub;
• sub->active = false;
• mutex_unlock(&rt->stream_mutex);
• return 0;

+}

+static int hiface_pcm_close(struct snd_pcm_substream *alsa_sub) +{

• struct pcm_runtime *rt = snd_pcm_substream_chip(alsa_sub);
• struct pcm_substream *sub = hiface_pcm_get_substream(alsa_sub);
• unsigned long flags;
• if (rt->panic)

• return 0;
  

• pr_debug("%s: called.\n", __func__);
• mutex_lock(&rt->stream_mutex);
• if (sub) {

• /* deactivate substream */
  

• spin_lock_irqsave(&sub->lock, flags);
  

• sub->instance = NULL;
  

• sub->active = false;
  

• spin_unlock_irqrestore(&sub->lock, flags);
  

• /* all substreams closed? if so, stop streaming */
  

• if (!rt->playback.instance)
  

• 	hiface_pcm_stream_stop(rt);
  

Can this condition be ever false...?

+static int hiface_pcm_trigger(struct snd_pcm_substream *alsa_sub, int cmd) +{

• struct pcm_substream *sub = hiface_pcm_get_substream(alsa_sub);
• struct pcm_runtime *rt = snd_pcm_substream_chip(alsa_sub);
• unsigned long flags;

For trigger callback, you need no irqsave but simply use spin_lock_irq().

+void hiface_pcm_abort(struct hiface_chip *chip) +{

• struct pcm_runtime *rt = chip->pcm;
• if (rt) {

• rt->panic = true;
  

• if (rt->playback.instance) {
  

• 	snd_pcm_stream_lock_irq(rt->playback.instance);
  

• 	snd_pcm_stop(rt->playback.instance,
  

• 			SNDRV_PCM_STATE_XRUN);
  

• 	snd_pcm_stream_unlock_irq(rt->playback.instance);
  

• }
  

Hmm... this is called after snd_card_disconnect(), thus it will reset the PCM stream state from DISCONNECTED to XRUN. It doesn't sound right.

+int hiface_pcm_init(struct hiface_chip *chip, u8 extra_freq) +{

....

• snd_pcm_lib_preallocate_pages_for_all(pcm,

• 			SNDRV_DMA_TYPE_CONTINUOUS,
  

• 			snd_dma_continuous_data(GFP_KERNEL),
  

• 			PCM_BUFFER_SIZE, PCM_BUFFER_SIZE);
  

Any reason not to use vmalloc buffer?

thanks,

Takashi