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

[Takashi Iwai]

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