Questions on locking, snd-usb-caiaq

Questions on locking, snd-usb-caiaq

[Mark Hills]

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,

-- 
Mark

diff --git a/sound/usb/caiaq/audio.c b/sound/usb/caiaq/audio.c
index 121af06..c659f81 100644
--- a/sound/usb/caiaq/audio.c
+++ b/sound/usb/caiaq/audio.c
@@ -62,10 +62,15 @@ static void
  activate_substream(struct snd_usb_caiaqdev *dev,
  	           struct snd_pcm_substream *sub)
  {
+	unsigned long flags;
+	spin_lock_irqsave(&dev->spinlock, flags);
+
  	if (sub->stream == SNDRV_PCM_STREAM_PLAYBACK)
  		dev->sub_playback[sub->number] = sub;
  	else
  		dev->sub_capture[sub->number] = sub;
+
+	spin_unlock_irqrestore(&dev->spinlock, flags);
  }

  static void
@@ -269,16 +274,23 @@ snd_usb_caiaq_pcm_pointer(struct snd_pcm_substream *sub)
  {
  	int index = sub->number;
  	struct snd_usb_caiaqdev *dev = snd_pcm_substream_chip(sub);
+	unsigned long flags;
+	snd_pcm_uframes_t ptr;
+
+	spin_lock_irqsave(&dev->spinlock, flags);

  	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_irqrestore(&dev->spinlock, flags);
+	return ptr;
  }

  /* operators for both playback and capture */

Re: Questions on locking, snd-usb-caiaq

[Takashi Iwai]

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

Re: Questions on locking, snd-usb-caiaq

[Daniel Mack]

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

Re: Questions on locking, snd-usb-caiaq

[Mark Hills]

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.

-- 
Mark

[PATCH 1/4] snd-usb-caiaq: Do not disable local interrupts

[Mark Hills]

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

[PATCH 2/4] snd-usb-caiaq: Missing lock around use of buffer positions

[Mark Hills]

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 */
-- 
1.6.4.4

[PATCH 3/4] snd-usb-caiaq: Lock on stream start/unpause

[Mark Hills]

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

[PATCH 4/4] snd-usb-caiaq: Bump version number to 1.3.20

[Mark Hills]

---
 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},"
-- 
1.6.4.4

Re: [PATCH 1/4] snd-usb-caiaq: Do not disable local interrupts

[Daniel Mack]

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
> 

Re: [PATCH 1/4] snd-usb-caiaq: Do not disable local interrupts

[Mark Hills]

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

-- 
Mark

Re: [PATCH 1/4] snd-usb-caiaq: Do not disable local interrupts

[Takashi Iwai]

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

Re: [PATCH 1/4] snd-usb-caiaq: Do not disable local interrupts

[Daniel Mack]

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

Re: [PATCH 1/4] snd-usb-caiaq: Do not disable local interrupts

[Mark Hills]

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.

-- 
Mark