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

[Antonio Ospite]

On Fri, 22 Feb 2013 13:53:06 +0100
Takashi Iwai <tiwai at suse.de> wrote:

Hi Takashi, I know, it's a huge delay.

> 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 logic behind this is that rt->panic is set to true if there was some
USB errors (or a disconnect), so a USB replug is expected anyways;
rt->panic will be initialized in the hiface_pcm_init() to 0 with a
kzalloc().

> > +/* 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?

The code above calls swahw32() it swaps words, it's some kind of mixed
endian format which the device expects, maybe it's because it handles
16 bits at time internally, we don't know for sure, but this is how it
is.

> 
> > +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?
>

Let's take a look at hiface_pcm_close():
 * sub->instance is set to NULL
 * the URBs are killed _after_ sub->instance is set to NULL

So if a URB is completed between these actions
hiface_pcm_out_urb_handler() gets called and there could be a problem.

If in hiface_pcm_close() I kill the URBs first and then set
sub->instance to NULL the race window gets closed, right?

> > +
> > +	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.
>

I am not sure I understand what you mean here, should I check the
state of ALSA PCM stream or should I keep another state to avoid
transmissions on overruns?

Maybe you mean that I need to check that:
 sub->instance->runtime->status->state == SNDRV_PCM_STATE_RUNNING
?

> > +
> > +	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...?
>

You mean that if we got to the .close() callback then the .open() one
succeeded and so we have a valid snd_pcm_substream? I guess that's
right and I can skip the check.

The driver was based on a driver which handled multiple streams so
something must have slipped during the cleanup.

BTW, I think that cleaning up this hiFace driver further could be useful
even after mainline inclusion, it's a fairly simple driver which can be
used as an example driver, but my inexperience with ALSA, and audio in
general, was holding me back a bit to revolutionize it's initial
structure. Help is always appreciated by more experienced people.

> > +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().
>

OK, will do, thanks.

> > +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.
>

Mmh, some other USB drivers do that too in their abort, _after_
snd_card_disconnect() so they must be wrong too.
Can I just skip calling snd_pcm_stop() altogether since the PCM stream
state is in DISCONNECTED state already?

> > +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?
> 

I don't know, I just shamelessly copied from other drivers here, what'd
be the difference?

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?