[PATCH 1/3] sound: dummy, use setup_timer and mod_timer

[PATCH 1/3] sound: dummy, use setup_timer and mod_timer

[Jiri Slaby]

From: Roman Kollar <rkollar@mail.muni.cz>

Use setup_timer and mod_timer instead of structure assignments as it
is the preferred way to setup and set the timer.

Signed-off-by: Roman Kollar <rkollar@mail.muni.cz>
Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org
---
 sound/drivers/dummy.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index 5d0dfb787cec..d11baaf0f0b4 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -245,9 +245,8 @@ struct dummy_systimer_pcm {
 
 static void dummy_systimer_rearm(struct dummy_systimer_pcm *dpcm)
 {
-	dpcm->timer.expires = jiffies +
-		(dpcm->frac_period_rest + dpcm->rate - 1) / dpcm->rate;
-	add_timer(&dpcm->timer);
+	mod_timer(&dpcm->timer, jiffies +
+		(dpcm->frac_period_rest + dpcm->rate - 1) / dpcm->rate);
 }
 
 static void dummy_systimer_update(struct dummy_systimer_pcm *dpcm)
@@ -340,9 +339,8 @@ static int dummy_systimer_create(struct snd_pcm_substream *substream)
 	if (!dpcm)
 		return -ENOMEM;
 	substream->runtime->private_data = dpcm;
-	init_timer(&dpcm->timer);
-	dpcm->timer.data = (unsigned long) dpcm;
-	dpcm->timer.function = dummy_systimer_callback;
+	setup_timer(&dpcm->timer, dummy_systimer_callback,
+			(unsigned long) dpcm);
 	spin_lock_init(&dpcm->lock);
 	dpcm->substream = substream;
 	return 0;
-- 
2.2.1

[PATCH 2/3] sound: dummy, use del_timer_sync

[Jiri Slaby]

Wait for the timer to really quit, not only by ensuring it is not in
the critical section.

BTW the spin_lock in here does not disable BH, so that it could
deadlock.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org
---
 sound/drivers/dummy.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index d11baaf0f0b4..6d6bf4093583 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -281,9 +281,9 @@ static int dummy_systimer_start(struct snd_pcm_substream *substream)
 static int dummy_systimer_stop(struct snd_pcm_substream *substream)
 {
 	struct dummy_systimer_pcm *dpcm = substream->runtime->private_data;
-	spin_lock(&dpcm->lock);
-	del_timer(&dpcm->timer);
-	spin_unlock(&dpcm->lock);
+
+	del_timer_sync(&dpcm->timer);
+
 	return 0;
 }
 
-- 
2.2.1

Re: [PATCH 2/3] sound: dummy, use del_timer_sync

[Takashi Iwai]

At Mon, 19 Jan 2015 10:42:55 +0100,
Jiri Slaby wrote:
> 
> Wait for the timer to really quit, not only by ensuring it is not in
> the critical section.
> 
> BTW the spin_lock in here does not disable BH, so that it could
> deadlock.

The stop function is called from the PCM trigger callback which is
irq-disabled.  So, you can't use del_timer_sync() here.


thanks,

Takashi

> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: alsa-devel@alsa-project.org
> ---
>  sound/drivers/dummy.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
> index d11baaf0f0b4..6d6bf4093583 100644
> --- a/sound/drivers/dummy.c
> +++ b/sound/drivers/dummy.c
> @@ -281,9 +281,9 @@ static int dummy_systimer_start(struct snd_pcm_substream *substream)
>  static int dummy_systimer_stop(struct snd_pcm_substream *substream)
>  {
>  	struct dummy_systimer_pcm *dpcm = substream->runtime->private_data;
> -	spin_lock(&dpcm->lock);
> -	del_timer(&dpcm->timer);
> -	spin_unlock(&dpcm->lock);
> +
> +	del_timer_sync(&dpcm->timer);
> +
>  	return 0;
>  }
>  
> -- 
> 2.2.1
> 

Re: [PATCH 2/3] sound: dummy, use del_timer_sync

[Jiri Slaby]

On 01/19/2015, 10:52 AM, Takashi Iwai wrote:
> At Mon, 19 Jan 2015 10:42:55 +0100,
> Jiri Slaby wrote:
>>
>> Wait for the timer to really quit, not only by ensuring it is not in
>> the critical section.
>>
>> BTW the spin_lock in here does not disable BH, so that it could
>> deadlock.
> 
> The stop function is called from the PCM trigger callback which is
> irq-disabled.  So, you can't use del_timer_sync() here.

The same as 3/3 I was looking at compr* :(. Ignore this one, then.

thanks,
-- 
js
suse labs

[PATCH 3/3] sound: dummy, avoid races with timer

[Jiri Slaby]

There is a race between timer and process contexts. Process context
does not disable irqs, so when a timer ticks inside process' critical
section, the system can deadlock. Fix this by a traditional _irqsave
variant of spin_lock.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org
---
 sound/drivers/dummy.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index 6d6bf4093583..18a7cfc99078 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -271,10 +271,13 @@ static void dummy_systimer_update(struct dummy_systimer_pcm *dpcm)
 static int dummy_systimer_start(struct snd_pcm_substream *substream)
 {
 	struct dummy_systimer_pcm *dpcm = substream->runtime->private_data;
-	spin_lock(&dpcm->lock);
+	unsigned long flags;
+
+	spin_lock_irqsave(&dpcm->lock, flags);
 	dpcm->base_time = jiffies;
 	dummy_systimer_rearm(dpcm);
-	spin_unlock(&dpcm->lock);
+	spin_unlock_irqrestore(&dpcm->lock, flags);
+
 	return 0;
 }
 
@@ -322,12 +325,14 @@ static snd_pcm_uframes_t
 dummy_systimer_pointer(struct snd_pcm_substream *substream)
 {
 	struct dummy_systimer_pcm *dpcm = substream->runtime->private_data;
+	unsigned long flags;
 	snd_pcm_uframes_t pos;
 
-	spin_lock(&dpcm->lock);
+	spin_lock_irqsave(&dpcm->lock, flags);
 	dummy_systimer_update(dpcm);
 	pos = dpcm->frac_pos / HZ;
-	spin_unlock(&dpcm->lock);
+	spin_unlock_irqrestore(&dpcm->lock, flags);
+
 	return pos;
 }
 
-- 
2.2.1

Re: [PATCH 3/3] sound: dummy, avoid races with timer

[Takashi Iwai]

At Mon, 19 Jan 2015 10:42:56 +0100,
Jiri Slaby wrote:
> 
> There is a race between timer and process contexts. Process context
> does not disable irqs, so when a timer ticks inside process' critical
> section, the system can deadlock. Fix this by a traditional _irqsave
> variant of spin_lock.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: alsa-devel@alsa-project.org
> ---
>  sound/drivers/dummy.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
> index 6d6bf4093583..18a7cfc99078 100644
> --- a/sound/drivers/dummy.c
> +++ b/sound/drivers/dummy.c
> @@ -271,10 +271,13 @@ static void dummy_systimer_update(struct dummy_systimer_pcm *dpcm)
>  static int dummy_systimer_start(struct snd_pcm_substream *substream)
>  {
>  	struct dummy_systimer_pcm *dpcm = substream->runtime->private_data;
> -	spin_lock(&dpcm->lock);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dpcm->lock, flags);
>  	dpcm->base_time = jiffies;
>  	dummy_systimer_rearm(dpcm);
> -	spin_unlock(&dpcm->lock);
> +	spin_unlock_irqrestore(&dpcm->lock, flags);
> +

This looks correct, but...


>  	return 0;
>  }
>  
> @@ -322,12 +325,14 @@ static snd_pcm_uframes_t
>  dummy_systimer_pointer(struct snd_pcm_substream *substream)
>  {
>  	struct dummy_systimer_pcm *dpcm = substream->runtime->private_data;
> +	unsigned long flags;
>  	snd_pcm_uframes_t pos;
>  
> -	spin_lock(&dpcm->lock);
> +	spin_lock_irqsave(&dpcm->lock, flags);
>  	dummy_systimer_update(dpcm);
>  	pos = dpcm->frac_pos / HZ;
> -	spin_unlock(&dpcm->lock);
> +	spin_unlock_irqrestore(&dpcm->lock, flags);
> +

This chunk is superfluous.  The pointer callback is guaranteed to be
called in the irq-disabled context.


thanks,

Takashi

Re: [PATCH 3/3] sound: dummy, avoid races with timer

[Jiri Slaby]

On 01/19/2015, 10:49 AM, Takashi Iwai wrote:
>> @@ -322,12 +325,14 @@ static snd_pcm_uframes_t
>>  dummy_systimer_pointer(struct snd_pcm_substream *substream)
>>  {
>>  	struct dummy_systimer_pcm *dpcm = substream->runtime->private_data;
>> +	unsigned long flags;
>>  	snd_pcm_uframes_t pos;
>>  
>> -	spin_lock(&dpcm->lock);
>> +	spin_lock_irqsave(&dpcm->lock, flags);
>>  	dummy_systimer_update(dpcm);
>>  	pos = dpcm->frac_pos / HZ;
>> -	spin_unlock(&dpcm->lock);
>> +	spin_unlock_irqrestore(&dpcm->lock, flags);
>> +
> 
> This chunk is superfluous.  The pointer callback is guaranteed to be
> called in the irq-disabled context.

Oh, my bad, I was looking at snd_compr_ops->pointer which is not the case.

-- 
js
suse labs

[PATCH v2 2/2] sound: dummy, avoid race with timer

[Jiri Slaby]

There is a race between timer and process contexts. Process context
does not disable irqs, so when a timer ticks inside process' critical
section, the system can deadlock. Fix this by a traditional _irqsave
variant of spin_lock.

[v2] removed ->pointer change as it is called with BH off

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: alsa-devel@alsa-project.org
---
 sound/drivers/dummy.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index d11baaf0f0b4..0791210cb4e7 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -271,10 +271,13 @@ static void dummy_systimer_update(struct dummy_systimer_pcm *dpcm)
 static int dummy_systimer_start(struct snd_pcm_substream *substream)
 {
 	struct dummy_systimer_pcm *dpcm = substream->runtime->private_data;
-	spin_lock(&dpcm->lock);
+	unsigned long flags;
+
+	spin_lock_irqsave(&dpcm->lock, flags);
 	dpcm->base_time = jiffies;
 	dummy_systimer_rearm(dpcm);
-	spin_unlock(&dpcm->lock);
+	spin_unlock_irqrestore(&dpcm->lock, flags);
+
 	return 0;
 }
 
-- 
2.2.1

Re: [PATCH v2 2/2] sound: dummy, avoid race with timer

[Takashi Iwai]

At Mon, 19 Jan 2015 10:56:41 +0100,
Jiri Slaby wrote:
> 
> There is a race between timer and process contexts. Process context
> does not disable irqs, so when a timer ticks inside process' critical
> section, the system can deadlock. Fix this by a traditional _irqsave
> variant of spin_lock.
> 
> [v2] removed ->pointer change as it is called with BH off

Sorry, my previous comment was incorrect.  The start function here is
also called from PCM trigger callback that is already irq-disabled.
So, this is also superfluous.


thanks,

Takashi

> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: alsa-devel@alsa-project.org
> ---
>  sound/drivers/dummy.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
> index d11baaf0f0b4..0791210cb4e7 100644
> --- a/sound/drivers/dummy.c
> +++ b/sound/drivers/dummy.c
> @@ -271,10 +271,13 @@ static void dummy_systimer_update(struct dummy_systimer_pcm *dpcm)
>  static int dummy_systimer_start(struct snd_pcm_substream *substream)
>  {
>  	struct dummy_systimer_pcm *dpcm = substream->runtime->private_data;
> -	spin_lock(&dpcm->lock);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dpcm->lock, flags);
>  	dpcm->base_time = jiffies;
>  	dummy_systimer_rearm(dpcm);
> -	spin_unlock(&dpcm->lock);
> +	spin_unlock_irqrestore(&dpcm->lock, flags);
> +
>  	return 0;
>  }
>  
> -- 
> 2.2.1
> 

Re: [PATCH 1/3] sound: dummy, use setup_timer and mod_timer

[Takashi Iwai]

At Mon, 19 Jan 2015 10:42:54 +0100,
Jiri Slaby wrote:
> 
> From: Roman Kollar <rkollar@mail.muni.cz>
> 
> Use setup_timer and mod_timer instead of structure assignments as it
> is the preferred way to setup and set the timer.
> 
> Signed-off-by: Roman Kollar <rkollar@mail.muni.cz>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: alsa-devel@alsa-project.org

Applied this one, thanks.


Takashi

> ---
>  sound/drivers/dummy.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
> index 5d0dfb787cec..d11baaf0f0b4 100644
> --- a/sound/drivers/dummy.c
> +++ b/sound/drivers/dummy.c
> @@ -245,9 +245,8 @@ struct dummy_systimer_pcm {
>  
>  static void dummy_systimer_rearm(struct dummy_systimer_pcm *dpcm)
>  {
> -	dpcm->timer.expires = jiffies +
> -		(dpcm->frac_period_rest + dpcm->rate - 1) / dpcm->rate;
> -	add_timer(&dpcm->timer);
> +	mod_timer(&dpcm->timer, jiffies +
> +		(dpcm->frac_period_rest + dpcm->rate - 1) / dpcm->rate);
>  }
>  
>  static void dummy_systimer_update(struct dummy_systimer_pcm *dpcm)
> @@ -340,9 +339,8 @@ static int dummy_systimer_create(struct snd_pcm_substream *substream)
>  	if (!dpcm)
>  		return -ENOMEM;
>  	substream->runtime->private_data = dpcm;
> -	init_timer(&dpcm->timer);
> -	dpcm->timer.data = (unsigned long) dpcm;
> -	dpcm->timer.function = dummy_systimer_callback;
> +	setup_timer(&dpcm->timer, dummy_systimer_callback,
> +			(unsigned long) dpcm);
>  	spin_lock_init(&dpcm->lock);
>  	dpcm->substream = substream;
>  	return 0;
> -- 
> 2.2.1
>