[alsa-devel] snd_pcsp locking mess
Takashi Iwai
tiwai at suse.de
Tue May 27 17:50:44 CEST 2008
At Tue, 27 May 2008 17:47:19 +0400,
Stas Sergeev wrote:
>
> Hello.
>
> Takashi Iwai wrote:
> > No, close callback is always after the hw_free callback.
> > So, in hw_free callback, you just need to assure that hrtimer callback
> > is finished (and/or cancel it). After that, you are free to work
> > without considering hrtimer stuff.
> Here is my first attempt on that.
> The optimization you mention above
> is possible, but I haven't done it
> (yet). Because I think it will obscure
> things.
> I can't really say if this code is
> better than before - it looks cleaner
> but adds 3 locks.
Grrr, that's horrible.
> > We don't drop pcm substream lock here because we don't need to acquire
> > it.
> Why not? Because it is not recommended
> to use any more, or for some other
> reason? I mostly need the hrtimer
> callback to be protected from all
> the pcm callbacks, modulo the (IMO
> questionable) optimization. So why
> not would I just use the substream
> lock?
Well, let's take a look back how the callbacks are called.
The callbacks that are called during PCM running are the following:
- hrtimer callback gets called from hrtimer:
This updates the pcsp port and updates the PCM pointer.
- PCM pointer callback:
This returns the current PCM pointer
So, actually between these calls, we have only one thing to protect -
the PCM pointer. The DMA-buffer readiness is necessary only for
hrtimer callback.
And, in addition, we need to call somewhere snd_pcm_period_elapsed()
when the period is finished. This can be done in a tasklet. But,
note that this is no callback, and not what pcsp driver itself
actually cares.
Now, remember that hrtimer is started only via trigger start
callback. That is, hrtimer callback isn called first after PCM is
actually started; hence the DMA buffer is ready when it's called.
Then, if we assure that the DMA buffer isn't changed and the PCM
substream still exists during hrtimer callback (and/or its execution
finishes), hrtimer callback can safely run without *any* lock.
This is pretty easy. We just need to add appropriate calls to
synchronize the finish of hrtimer (and tasklet) at each place that can
change the DMA buffers.
Maybe codes tell better than words. The below is my untested patch.
Takashi
---
diff --git a/sound/drivers/pcsp/pcsp.c b/sound/drivers/pcsp/pcsp.c
index 1899cf0..87219bf 100644
--- a/sound/drivers/pcsp/pcsp.c
+++ b/sound/drivers/pcsp/pcsp.c
@@ -96,7 +96,7 @@ static int __devinit snd_card_pcsp_probe(int devnum, struct device *dev)
return -EINVAL;
hrtimer_init(&pcsp_chip.timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
- pcsp_chip.timer.cb_mode = HRTIMER_CB_SOFTIRQ;
+ pcsp_chip.timer.cb_mode = HRTIMER_CB_IRQSAFE;
pcsp_chip.timer.function = pcsp_do_timer;
card = snd_card_new(index, id, THIS_MODULE, 0);
@@ -188,10 +188,8 @@ static int __devexit pcsp_remove(struct platform_device *dev)
static void pcsp_stop_beep(struct snd_pcsp *chip)
{
- spin_lock_irq(&chip->substream_lock);
- if (!chip->playback_substream)
- pcspkr_stop_sound();
- spin_unlock_irq(&chip->substream_lock);
+ pcsp_sync_stop(chip);
+ pcspkr_stop_sound();
}
#ifdef CONFIG_PM
diff --git a/sound/drivers/pcsp/pcsp.h b/sound/drivers/pcsp/pcsp.h
index 1d661f7..70533a3 100644
--- a/sound/drivers/pcsp/pcsp.h
+++ b/sound/drivers/pcsp/pcsp.h
@@ -77,6 +77,7 @@ struct snd_pcsp {
extern struct snd_pcsp pcsp_chip;
extern enum hrtimer_restart pcsp_do_timer(struct hrtimer *handle);
+extern void pcsp_sync_stop(struct snd_pcsp *chip);
extern int snd_pcsp_new_pcm(struct snd_pcsp *chip);
extern int snd_pcsp_new_mixer(struct snd_pcsp *chip);
diff --git a/sound/drivers/pcsp/pcsp_lib.c b/sound/drivers/pcsp/pcsp_lib.c
index e341f3f..40f95f5 100644
--- a/sound/drivers/pcsp/pcsp_lib.c
+++ b/sound/drivers/pcsp/pcsp_lib.c
@@ -8,6 +8,7 @@
#include <linux/module.h>
#include <linux/moduleparam.h>
+#include <linux/interrupt.h>
#include <sound/pcm.h>
#include <asm/io.h>
#include "pcsp.h"
@@ -19,6 +20,22 @@ MODULE_PARM_DESC(nforce_wa, "Apply NForce chipset workaround "
#define DMIX_WANTS_S16 1
+/*
+ * Call snd_pcm_period_elapsed in a tasklet
+ * This avoids spinlock messes and long-running irq contexts
+ */
+static void pcsp_call_pcm_elapsed(unsigned long priv)
+{
+ if (atomic_read(&pcsp_chip.timer_active)) {
+ struct snd_pcm_substream *substream;
+ substream = pcsp_chip.playback_substream;
+ if (substream)
+ snd_pcm_period_elapsed(substream);
+ }
+}
+
+static DECLARE_TASKLET(pcsp_pcm_tasklet, pcsp_call_pcm_elapsed, 0);
+
enum hrtimer_restart pcsp_do_timer(struct hrtimer *handle)
{
unsigned char timer_cnt, val;
@@ -28,41 +45,23 @@ enum hrtimer_restart pcsp_do_timer(struct hrtimer *handle)
struct snd_pcm_substream *substream;
struct snd_pcm_runtime *runtime;
struct snd_pcsp *chip = container_of(handle, struct snd_pcsp, timer);
+ unsigned long flags;
if (chip->thalf) {
outb(chip->val61, 0x61);
chip->thalf = 0;
if (!atomic_read(&chip->timer_active))
- return HRTIMER_NORESTART;
+ goto stop;
hrtimer_forward(&chip->timer, chip->timer.expires,
ktime_set(0, chip->ns_rem));
return HRTIMER_RESTART;
}
- spin_lock_irq(&chip->substream_lock);
- /* Takashi Iwai says regarding this extra lock:
-
- If the irq handler handles some data on the DMA buffer, it should
- do snd_pcm_stream_lock().
- That protects basically against all races among PCM callbacks, yes.
- However, there are two remaining issues:
- 1. The substream pointer you try to lock isn't protected _before_
- this lock yet.
- 2. snd_pcm_period_elapsed() itself acquires the lock.
- The requirement of another lock is because of 1. When you get
- chip->playback_substream, it's not protected.
- Keeping this lock while snd_pcm_period_elapsed() assures the substream
- is still protected (at least, not released). And the other status is
- handled properly inside snd_pcm_stream_lock() in
- snd_pcm_period_elapsed().
-
- */
- if (!chip->playback_substream)
- goto exit_nr_unlock1;
- substream = chip->playback_substream;
- snd_pcm_stream_lock(substream);
if (!atomic_read(&chip->timer_active))
- goto exit_nr_unlock2;
+ goto stop;
+ substream = chip->playback_substream;
+ if (!substream)
+ goto stop;
runtime = substream->runtime;
fmt_size = snd_pcm_format_physical_width(runtime->format) >> 3;
@@ -87,6 +86,8 @@ enum hrtimer_restart pcsp_do_timer(struct hrtimer *handle)
period_bytes = snd_pcm_lib_period_bytes(substream);
buffer_bytes = snd_pcm_lib_buffer_bytes(substream);
+
+ spin_lock_irqsave(&chip->substream_lock, flags);
chip->playback_ptr += PCSP_INDEX_INC() * fmt_size;
periods_elapsed = chip->playback_ptr - chip->period_ptr;
if (periods_elapsed < 0) {
@@ -102,18 +103,15 @@ enum hrtimer_restart pcsp_do_timer(struct hrtimer *handle)
* or ALSA will BUG on us. */
chip->playback_ptr %= buffer_bytes;
- snd_pcm_stream_unlock(substream);
-
if (periods_elapsed) {
- snd_pcm_period_elapsed(substream);
chip->period_ptr += periods_elapsed * period_bytes;
chip->period_ptr %= buffer_bytes;
+ tasklet_schedule(&pcsp_pcm_tasklet);
}
-
- spin_unlock_irq(&chip->substream_lock);
+ spin_unlock_irqrestore(&chip->substream_lock, flags);
if (!atomic_read(&chip->timer_active))
- return HRTIMER_NORESTART;
+ goto stop;
chip->ns_rem = PCSP_PERIOD_NS();
ns = (chip->thalf ? PCSP_CALC_NS(timer_cnt) : chip->ns_rem);
@@ -121,10 +119,7 @@ enum hrtimer_restart pcsp_do_timer(struct hrtimer *handle)
hrtimer_forward(&chip->timer, chip->timer.expires, ktime_set(0, ns));
return HRTIMER_RESTART;
-exit_nr_unlock2:
- snd_pcm_stream_unlock(substream);
-exit_nr_unlock1:
- spin_unlock_irq(&chip->substream_lock);
+ stop:
return HRTIMER_NORESTART;
}
@@ -164,26 +159,35 @@ static void pcsp_stop_playing(struct snd_pcsp *chip)
spin_unlock(&i8253_lock);
}
+/*
+ * Force to stop and sync the stream
+ */
+void pcsp_sync_stop(struct snd_pcsp *chip)
+{
+ local_irq_disable();
+ pcsp_stop_playing(chip);
+ local_irq_enable();
+ hrtimer_cancel(&chip->timer);
+ tasklet_kill(&pcsp_pcm_tasklet);
+}
+
static int snd_pcsp_playback_close(struct snd_pcm_substream *substream)
{
struct snd_pcsp *chip = snd_pcm_substream_chip(substream);
#if PCSP_DEBUG
printk(KERN_INFO "PCSP: close called\n");
#endif
- if (atomic_read(&chip->timer_active)) {
- printk(KERN_ERR "PCSP: timer still active\n");
- pcsp_stop_playing(chip);
- }
- spin_lock_irq(&chip->substream_lock);
+ pcsp_sync_stop(chip);
chip->playback_substream = NULL;
- spin_unlock_irq(&chip->substream_lock);
return 0;
}
static int snd_pcsp_playback_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *hw_params)
{
+ struct snd_pcsp *chip = snd_pcm_substream_chip(substream);
int err;
+ pcsp_sync_stop(chip);
err = snd_pcm_lib_malloc_pages(substream,
params_buffer_bytes(hw_params));
if (err < 0)
@@ -193,9 +197,11 @@ static int snd_pcsp_playback_hw_params(struct snd_pcm_substream *substream,
static int snd_pcsp_playback_hw_free(struct snd_pcm_substream *substream)
{
+ struct snd_pcsp *chip = snd_pcm_substream_chip(substream);
#if PCSP_DEBUG
printk(KERN_INFO "PCSP: hw_free called\n");
#endif
+ pcsp_sync_stop(chip);
return snd_pcm_lib_free_pages(substream);
}
@@ -211,6 +217,7 @@ static int snd_pcsp_playback_prepare(struct snd_pcm_substream *substream)
snd_pcm_lib_period_bytes(substream),
substream->runtime->periods);
#endif
+ pcsp_sync_stop(chip);
chip->playback_ptr = 0;
chip->period_ptr = 0;
return 0;
@@ -241,7 +248,11 @@ static snd_pcm_uframes_t snd_pcsp_playback_pointer(struct snd_pcm_substream
*substream)
{
struct snd_pcsp *chip = snd_pcm_substream_chip(substream);
- return bytes_to_frames(substream->runtime, chip->playback_ptr);
+ unsigned int pos;
+ spin_lock(&chip->substream_lock);
+ pos = chip->playback_ptr;
+ spin_unlock(&chip->substream_lock);
+ return bytes_to_frames(substream->runtime, pos);
}
static struct snd_pcm_hardware snd_pcsp_playback = {
@@ -278,9 +289,7 @@ static int snd_pcsp_playback_open(struct snd_pcm_substream *substream)
return -EBUSY;
}
runtime->hw = snd_pcsp_playback;
- spin_lock_irq(&chip->substream_lock);
chip->playback_substream = substream;
- spin_unlock_irq(&chip->substream_lock);
return 0;
}
More information about the Alsa-devel
mailing list