[alsa-devel] Questions on locking, snd-usb-caiaq
I fixed some race issues in snd-usb-caiaq, but have some questions on the interface between the driver and ALSA.
First fix is a lock in pointer(), which caused snd_pcm_update_hw_ptr_pos() to report an out of range buffer position (even though it was sucessfully corrected to zero).
Second is a lock as part of trigger(). On testing, this appears to fix an issue where white noise could sometimes be transferred by the driver.
According to the documentation, trigger() and pointer() are called with local interrupts disabled. This means they should really be non-irqsave locks?
(http://www.alsa-project.org/~tiwai/writing-an-alsa-driver/ch05s08.html)
Even for callbacks documented as atomic, if read_completed() and write_completed() can be called at any time interrupts are enabled (eg. on another CPU) it seems that additional locking my be required. Candidates are:
snd_usb_caiaq_substream_open snd_usb_caiaq_substream_close snd_usb_caiaq_pcm_hw_free snd_usb_caiaq_pcm_prepare
Are there any guarantees on these functions which mean the lock is not required?
To lock in prepare() would (I think) require demoting a spin_lock_irqsave() to a spin_lock() to send initialisation commands. If so, what is the best design for this?
Thanks,
At Sun, 20 Sep 2009 13:07:05 +0100 (BST), Mark Hills wrote:
I fixed some race issues in snd-usb-caiaq, but have some questions on the interface between the driver and ALSA.
First fix is a lock in pointer(), which caused snd_pcm_update_hw_ptr_pos() to report an out of range buffer position (even though it was sucessfully corrected to zero).
Second is a lock as part of trigger(). On testing, this appears to fix an issue where white noise could sometimes be transferred by the driver.
Both look fine to me.
According to the documentation, trigger() and pointer() are called with local interrupts disabled. This means they should really be non-irqsave locks?
They can be non-irqsave one, but not necessarily to be.
(http://www.alsa-project.org/~tiwai/writing-an-alsa-driver/ch05s08.html)
Even for callbacks documented as atomic, if read_completed() and write_completed() can be called at any time interrupts are enabled (eg. on another CPU) it seems that additional locking my be required. Candidates are:
snd_usb_caiaq_substream_open snd_usb_caiaq_substream_close snd_usb_caiaq_pcm_hw_free snd_usb_caiaq_pcm_prepare
Are there any guarantees on these functions which mean the lock is not required?
They are all protected via PCM substream lock, thus they aren't racy.
To lock in prepare() would (I think) require demoting a spin_lock_irqsave() to a spin_lock() to send initialisation commands. If so, what is the best design for this?
The best is to sync the deactivation. Right now, activation / deactivation are done in asynchronous way in caiaq driver. At the beginning of prepare callback, you should be sure that all URBs are killed, then proceed the rest preparation job.
Takashi
On Mon, Sep 21, 2009 at 03:35:06PM +0200, Takashi Iwai wrote:
At Sun, 20 Sep 2009 13:07:05 +0100 (BST), Mark Hills wrote:
I fixed some race issues in snd-usb-caiaq, but have some questions on the interface between the driver and ALSA.
First fix is a lock in pointer(), which caused snd_pcm_update_hw_ptr_pos() to report an out of range buffer position (even though it was sucessfully corrected to zero).
Second is a lock as part of trigger(). On testing, this appears to fix an issue where white noise could sometimes be transferred by the driver.
Both look fine to me.
To me as well, but I'm currently unable to test them. But I trust Mark - he's been tracking that issue for a long time already and was the only one so far observing this bug. If those patches fix it for him, they should definitely go in. So assume my Acked-by: on them :)
Thanks, Daniel
On Tue, 22 Sep 2009, Daniel Mack wrote:
On Mon, Sep 21, 2009 at 03:35:06PM +0200, Takashi Iwai wrote:
At Sun, 20 Sep 2009 13:07:05 +0100 (BST), Mark Hills wrote:
I fixed some race issues in snd-usb-caiaq, but have some questions on the interface between the driver and ALSA.
First fix is a lock in pointer(), which caused snd_pcm_update_hw_ptr_pos() to report an out of range buffer position (even though it was sucessfully corrected to zero).
Second is a lock as part of trigger(). On testing, this appears to fix an issue where white noise could sometimes be transferred by the driver.
Both look fine to me.
To me as well, but I'm currently unable to test them. But I trust Mark - he's been tracking that issue for a long time already and was the only one so far observing this bug. If those patches fix it for him, they should definitely go in. So assume my Acked-by: on them :)
Thanks, once I've done a little more widespread testing for visible bugs, I'll follow up with proper patches. I also made the change to spin_lock() where appropriate.
The driver documentation states that local interrupts are already disabled when trigger() is called, so only a spinlock is required. --- sound/usb/caiaq/audio.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c index 121af06..ce193a5 100644 --- a/sound/usb/caiaq/audio.c +++ b/sound/usb/caiaq/audio.c @@ -72,15 +72,14 @@ static void deactivate_substream(struct snd_usb_caiaqdev *dev, struct snd_pcm_substream *sub) { - unsigned long flags; - spin_lock_irqsave(&dev->spinlock, flags); + spin_lock(&dev->spinlock);
if (sub->stream == SNDRV_PCM_STREAM_PLAYBACK) dev->sub_playback[sub->number] = NULL; else dev->sub_capture[sub->number] = NULL;
- spin_unlock_irqrestore(&dev->spinlock, flags); + spin_unlock(&dev->spinlock); }
static int
Fix a race which causes snd_pcm_update_hw_ptr_pos() to report a bug. --- sound/usb/caiaq/audio.c | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c index ce193a5..9856c00 100644 --- a/sound/usb/caiaq/audio.c +++ b/sound/usb/caiaq/audio.c @@ -268,16 +268,22 @@ snd_usb_caiaq_pcm_pointer(struct snd_pcm_substream *sub) { int index = sub->number; struct snd_usb_caiaqdev *dev = snd_pcm_substream_chip(sub); + snd_pcm_uframes_t ptr; + + spin_lock(&dev->spinlock);
if (dev->input_panic || dev->output_panic) - return SNDRV_PCM_POS_XRUN; + ptr = SNDRV_PCM_POS_XRUN;
if (sub->stream == SNDRV_PCM_STREAM_PLAYBACK) - return bytes_to_frames(sub->runtime, + ptr = bytes_to_frames(sub->runtime, dev->audio_out_buf_pos[index]); else - return bytes_to_frames(sub->runtime, + ptr = bytes_to_frames(sub->runtime, dev->audio_in_buf_pos[index]); + + spin_unlock(&dev->spinlock); + return ptr; }
/* operators for both playback and capture */
Fix a bug which can result in white noise from the driver after stream start or unpause. --- sound/usb/caiaq/audio.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c index 9856c00..de9cf1e 100644 --- a/sound/usb/caiaq/audio.c +++ b/sound/usb/caiaq/audio.c @@ -62,10 +62,14 @@ static void activate_substream(struct snd_usb_caiaqdev *dev, struct snd_pcm_substream *sub) { + spin_lock(&dev->spinlock); + if (sub->stream == SNDRV_PCM_STREAM_PLAYBACK) dev->sub_playback[sub->number] = sub; else dev->sub_capture[sub->number] = sub; + + spin_unlock(&dev->spinlock); }
static void
--- sound/usb/caiaq/device.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/sound/usb/caiaq/device.c b/sound/usb/caiaq/device.c index 83e6c13..a3f02dd 100644 --- a/sound/usb/caiaq/device.c +++ b/sound/usb/caiaq/device.c @@ -35,7 +35,7 @@ #include "input.h"
MODULE_AUTHOR("Daniel Mack daniel@caiaq.de"); -MODULE_DESCRIPTION("caiaq USB audio, version 1.3.19"); +MODULE_DESCRIPTION("caiaq USB audio, version 1.3.20"); MODULE_LICENSE("GPL"); MODULE_SUPPORTED_DEVICE("{{Native Instruments, RigKontrol2}," "{Native Instruments, RigKontrol3},"
On Fri, Sep 25, 2009 at 04:22:15PM +0100, Mark Hills wrote:
The driver documentation states that local interrupts are already disabled when trigger() is called, so only a spinlock is required.
Hmm. I assume you tested them well. But I remember to have had quite some trouble at this point on SMP machines and ended up with that irqsave code. However, if you're sure that fixes a bug or is not necessary, let's take the patches and I'll test them once I'm back home around mid Oct.
Btw - your Signed-off-by: is missing on all of them :)
Thanks, Daniel
sound/usb/caiaq/audio.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c index 121af06..ce193a5 100644 --- a/sound/usb/caiaq/audio.c +++ b/sound/usb/caiaq/audio.c @@ -72,15 +72,14 @@ static void deactivate_substream(struct snd_usb_caiaqdev *dev, struct snd_pcm_substream *sub) {
- unsigned long flags;
- spin_lock_irqsave(&dev->spinlock, flags);
spin_lock(&dev->spinlock);
if (sub->stream == SNDRV_PCM_STREAM_PLAYBACK) dev->sub_playback[sub->number] = NULL; else dev->sub_capture[sub->number] = NULL;
- spin_unlock_irqrestore(&dev->spinlock, flags);
- spin_unlock(&dev->spinlock);
}
static int
1.6.4.4
On Fri, 25 Sep 2009, Daniel Mack wrote:
On Fri, Sep 25, 2009 at 04:22:15PM +0100, Mark Hills wrote:
The driver documentation states that local interrupts are already disabled when trigger() is called, so only a spinlock is required.
Hmm. I assume you tested them well. But I remember to have had quite some trouble at this point on SMP machines and ended up with that irqsave code. However, if you're sure that fixes a bug or is not necessary, let's take the patches and I'll test them once I'm back home around mid Oct.
I did check both non and SMP machines, and it corresponds to the docs. I also removed the patches to verify I could reproduce the original problems.
As I missed off the the Signed-off-by, I'll not rush to resubmit them; let's wait until you have also tested them.
Btw - your Signed-off-by: is missing on all of them :)
Thanks, Daniel
sound/usb/caiaq/audio.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c index 121af06..ce193a5 100644 --- a/sound/usb/caiaq/audio.c +++ b/sound/usb/caiaq/audio.c @@ -72,15 +72,14 @@ static void deactivate_substream(struct snd_usb_caiaqdev *dev, struct snd_pcm_substream *sub) {
- unsigned long flags;
- spin_lock_irqsave(&dev->spinlock, flags);
spin_lock(&dev->spinlock);
if (sub->stream == SNDRV_PCM_STREAM_PLAYBACK) dev->sub_playback[sub->number] = NULL; else dev->sub_capture[sub->number] = NULL;
- spin_unlock_irqrestore(&dev->spinlock, flags);
- spin_unlock(&dev->spinlock);
}
static int
1.6.4.4
At Sat, 26 Sep 2009 17:06:23 +0100 (BST), Mark Hills wrote:
On Fri, 25 Sep 2009, Daniel Mack wrote:
On Fri, Sep 25, 2009 at 04:22:15PM +0100, Mark Hills wrote:
The driver documentation states that local interrupts are already disabled when trigger() is called, so only a spinlock is required.
Hmm. I assume you tested them well. But I remember to have had quite some trouble at this point on SMP machines and ended up with that irqsave code. However, if you're sure that fixes a bug or is not necessary, let's take the patches and I'll test them once I'm back home around mid Oct.
I did check both non and SMP machines, and it corresponds to the docs. I also removed the patches to verify I could reproduce the original problems.
The irqsave is needed for deactivate_substream() because it's called also in your hw_free callback, which is non-atomic. In the real case, it's hard to hit this codepath, though.
Meanwhile, activate_substream() is called only from the trigger callback, and thus it's safe to use without irqsave version.
thanks,
Takashi
On Mon, Sep 28, 2009 at 10:33:35AM +0200, Takashi Iwai wrote:
At Sat, 26 Sep 2009 17:06:23 +0100 (BST), Mark Hills wrote:
On Fri, 25 Sep 2009, Daniel Mack wrote:
On Fri, Sep 25, 2009 at 04:22:15PM +0100, Mark Hills wrote:
The driver documentation states that local interrupts are already disabled when trigger() is called, so only a spinlock is required.
Hmm. I assume you tested them well. But I remember to have had quite some trouble at this point on SMP machines and ended up with that irqsave code. However, if you're sure that fixes a bug or is not necessary, let's take the patches and I'll test them once I'm back home around mid Oct.
I did check both non and SMP machines, and it corresponds to the docs. I also removed the patches to verify I could reproduce the original problems.
The irqsave is needed for deactivate_substream() because it's called also in your hw_free callback, which is non-atomic. In the real case, it's hard to hit this codepath, though.
Well, the nature of race conditions is that they are hard to hit, right? ;)
However, I tested the patches now, and couldn't reproduce anything like the things I remember I had.
So from my side, they can go in then. Sorry for the long delay on this.
Thanks, Daniel
On Sat, 17 Oct 2009, Daniel Mack wrote:
On Mon, Sep 28, 2009 at 10:33:35AM +0200, Takashi Iwai wrote:
At Sat, 26 Sep 2009 17:06:23 +0100 (BST), Mark Hills wrote:
On Fri, 25 Sep 2009, Daniel Mack wrote:
On Fri, Sep 25, 2009 at 04:22:15PM +0100, Mark Hills wrote:
The driver documentation states that local interrupts are already disabled when trigger() is called, so only a spinlock is required.
Hmm. I assume you tested them well. But I remember to have had quite some trouble at this point on SMP machines and ended up with that irqsave code. However, if you're sure that fixes a bug or is not necessary, let's take the patches and I'll test them once I'm back home around mid Oct.
I did check both non and SMP machines, and it corresponds to the docs. I also removed the patches to verify I could reproduce the original problems.
The irqsave is needed for deactivate_substream() because it's called also in your hw_free callback, which is non-atomic. In the real case, it's hard to hit this codepath, though.
Well, the nature of race conditions is that they are hard to hit, right? ;)
However, I tested the patches now, and couldn't reproduce anything like the things I remember I had.
So from my side, they can go in then. Sorry for the long delay on this.
No, Takashi is correct -- deactivate_substream() is also used in hw_free. It means that "[PATCH 1/4] Do not disable local interrupts" should not be applied. It was meant to be a cleanup only but introduces a new problem.
The other patches (2-4) are good and should be applied. I'll resubmit with my Signed-off-by, but this will be in a week or so.
participants (3)
-
Daniel Mack -
Mark Hills -
Takashi Iwai