[alsa-devel] [PATCH v2] Add M2Tech hiFace USB-SPDIF driver
Antonio Ospite
ospite at studenti.unina.it
Mon Jun 3 23:40:00 CEST 2013
On Wed, 29 May 2013 14:24:50 +0200
Takashi Iwai <tiwai at suse.de> wrote:
Hi again,
> At Sun, 28 Apr 2013 23:09:59 +0200,
> Antonio Ospite wrote:
> >
> > 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().
>
> Hmm, it doesn't sound good.
>
In some other drivers like caiaq the "panic" is a condition independent
from the USB communication itself.
In the 6fire driver instead the panic is triggered by the URB status
itself, so the scheme is more or less the same as in this hiFace driver,
which was based on the 6fire one.
I may very well skip the panic logic altogether if the assumption is
that the USB communication works, and if it does not it is
responsibility of the USB stack to report that.
> > > > +/* 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.
>
Probably not worth it, I'll leave this as 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?
>
> Should be, yes.
>
Done, thanks.
> > > > +
> > > > + 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.
>
Mmh, checking the ALSA state in the URB complete callback looks a bit
off, I don't see the other drives doing it, and if I add a check like
the above then I cannot resume from pause.
Is the driver supposed to keep sending URBs even when the stream is in
SNDRV_PCM_STATE_PAUSED? I'll check with the other drivers.
> > > > +
> > > > + 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...
>
OK, now I get it, the check must go away and the hiface_pcm_stream_stop
() must be called, otherwise the driver will keep sending URBs even
after a .close(). Thanks again.
> > 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.
I was referring to ua101 and to 6fire, but I see that the caiaq driver
and the usb generic one do not call snd_pcm_stop() at all so I am going
to drop it 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?
>
> A huge difference. Imagine allocating a 1MB buffer.
I think I get what you meant now: for USB devices
snd_pcm_lib_alloc_vmalloc_buffer() can be used because an USB device
does not have the DMA and contiguousness requirements that a PCI device
has, right?
https://github.com/panicking/snd-usb-asyncaudio/commit/7cc042dda89e3705dbca6edb6744ea068e01a115
Daniel, maybe you want to change this in the caiaq driver too?
Should I send a patch for it?
The doubts about the panic condition and about checking the ALSA state
before resubmitting the URBs are the only two issue which are
keeping me from sending a v3 targeted for inclusion.
Michael I'll drop all the pr_debug("%s: called!", __func__) too in the
final version.
Thanks again,
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?
More information about the Alsa-devel
mailing list