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