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

[Takashi Iwai <tiwai@suse.de>]

At Sun, 28 Apr 2013 23:09:59 +0200,
Antonio Ospite wrote:
> 
> On Fri, 22 Feb 2013 13:53:06 +0100
> Takashi Iwai <tiwai@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().

Hmm, it doesn't sound good.


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

OK, then I read it wrongly.  In general, we don't want to do such
conversions in the driver code but it's too exotic, and maybe not
worth to add the new format in the common definition.


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

Should be, yes.

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

ALSA state check should be enough for XRUN.

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

Yes, something like that.


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

What does stream_mutex protect?  I don't have any glance over the
whole code, and the code was so old, thus I don't remember at all...

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

Which one?  The common usb-audio driver has the check beforehand.

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

A huge difference.  Imagine allocating a 1MB buffer.


thanks,

Takashi