* [PATCH 1/3] sound: dummy, use setup_timer and mod_timer
@ 2015-01-19 9:42 Jiri Slaby
2015-01-19 9:42 ` [PATCH 2/3] sound: dummy, use del_timer_sync Jiri Slaby
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Jiri Slaby @ 2015-01-19 9:42 UTC (permalink / raw)
To: tiwai; +Cc: linux-kernel, Roman Kollar, Jiri Slaby, Jaroslav Kysela, alsa-devel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] sound: dummy, use del_timer_sync
2015-01-19 9:42 [PATCH 1/3] sound: dummy, use setup_timer and mod_timer Jiri Slaby
@ 2015-01-19 9:42 ` Jiri Slaby
2015-01-19 9:52 ` Takashi Iwai
2015-01-19 9:42 ` [PATCH 3/3] sound: dummy, avoid races with timer Jiri Slaby
2015-01-19 9:53 ` [PATCH 1/3] sound: dummy, use setup_timer and mod_timer Takashi Iwai
2 siblings, 1 reply; 10+ messages in thread
From: Jiri Slaby @ 2015-01-19 9:42 UTC (permalink / raw)
To: tiwai; +Cc: linux-kernel, Jiri Slaby, Jaroslav Kysela, alsa-devel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] sound: dummy, avoid races with timer
2015-01-19 9:42 [PATCH 1/3] sound: dummy, use setup_timer and mod_timer Jiri Slaby
2015-01-19 9:42 ` [PATCH 2/3] sound: dummy, use del_timer_sync Jiri Slaby
@ 2015-01-19 9:42 ` Jiri Slaby
2015-01-19 9:49 ` Takashi Iwai
2015-01-19 9:53 ` [PATCH 1/3] sound: dummy, use setup_timer and mod_timer Takashi Iwai
2 siblings, 1 reply; 10+ messages in thread
From: Jiri Slaby @ 2015-01-19 9:42 UTC (permalink / raw)
To: tiwai; +Cc: linux-kernel, Jiri Slaby, Jaroslav Kysela, alsa-devel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] sound: dummy, avoid races with timer
2015-01-19 9:42 ` [PATCH 3/3] sound: dummy, avoid races with timer Jiri Slaby
@ 2015-01-19 9:49 ` Takashi Iwai
2015-01-19 9:53 ` Jiri Slaby
2015-01-19 9:56 ` [PATCH v2 2/2] sound: dummy, avoid race " Jiri Slaby
0 siblings, 2 replies; 10+ messages in thread
From: Takashi Iwai @ 2015-01-19 9:49 UTC (permalink / raw)
To: Jiri Slaby; +Cc: linux-kernel, Jaroslav Kysela, alsa-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] sound: dummy, use del_timer_sync
2015-01-19 9:42 ` [PATCH 2/3] sound: dummy, use del_timer_sync Jiri Slaby
@ 2015-01-19 9:52 ` Takashi Iwai
2015-01-19 9:59 ` Jiri Slaby
0 siblings, 1 reply; 10+ messages in thread
From: Takashi Iwai @ 2015-01-19 9:52 UTC (permalink / raw)
To: Jiri Slaby; +Cc: linux-kernel, Jaroslav Kysela, alsa-devel
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] sound: dummy, use setup_timer and mod_timer
2015-01-19 9:42 [PATCH 1/3] sound: dummy, use setup_timer and mod_timer Jiri Slaby
2015-01-19 9:42 ` [PATCH 2/3] sound: dummy, use del_timer_sync Jiri Slaby
2015-01-19 9:42 ` [PATCH 3/3] sound: dummy, avoid races with timer Jiri Slaby
@ 2015-01-19 9:53 ` Takashi Iwai
2 siblings, 0 replies; 10+ messages in thread
From: Takashi Iwai @ 2015-01-19 9:53 UTC (permalink / raw)
To: Jiri Slaby; +Cc: linux-kernel, Roman Kollar, Jaroslav Kysela, alsa-devel
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] sound: dummy, avoid races with timer
2015-01-19 9:49 ` Takashi Iwai
@ 2015-01-19 9:53 ` Jiri Slaby
2015-01-19 9:56 ` [PATCH v2 2/2] sound: dummy, avoid race " Jiri Slaby
1 sibling, 0 replies; 10+ messages in thread
From: Jiri Slaby @ 2015-01-19 9:53 UTC (permalink / raw)
To: Takashi Iwai; +Cc: linux-kernel, Jaroslav Kysela, alsa-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] sound: dummy, avoid race with timer
2015-01-19 9:49 ` Takashi Iwai
2015-01-19 9:53 ` Jiri Slaby
@ 2015-01-19 9:56 ` Jiri Slaby
2015-01-19 9:59 ` Takashi Iwai
1 sibling, 1 reply; 10+ messages in thread
From: Jiri Slaby @ 2015-01-19 9:56 UTC (permalink / raw)
To: tiwai; +Cc: linux-kernel, Jiri Slaby, Jaroslav Kysela, alsa-devel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] sound: dummy, use del_timer_sync
2015-01-19 9:52 ` Takashi Iwai
@ 2015-01-19 9:59 ` Jiri Slaby
0 siblings, 0 replies; 10+ messages in thread
From: Jiri Slaby @ 2015-01-19 9:59 UTC (permalink / raw)
To: Takashi Iwai; +Cc: linux-kernel, Jaroslav Kysela, alsa-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] sound: dummy, avoid race with timer
2015-01-19 9:56 ` [PATCH v2 2/2] sound: dummy, avoid race " Jiri Slaby
@ 2015-01-19 9:59 ` Takashi Iwai
0 siblings, 0 replies; 10+ messages in thread
From: Takashi Iwai @ 2015-01-19 9:59 UTC (permalink / raw)
To: Jiri Slaby; +Cc: linux-kernel, Jaroslav Kysela, alsa-devel
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-01-19 9:59 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-19 9:42 [PATCH 1/3] sound: dummy, use setup_timer and mod_timer Jiri Slaby
2015-01-19 9:42 ` [PATCH 2/3] sound: dummy, use del_timer_sync Jiri Slaby
2015-01-19 9:52 ` Takashi Iwai
2015-01-19 9:59 ` Jiri Slaby
2015-01-19 9:42 ` [PATCH 3/3] sound: dummy, avoid races with timer Jiri Slaby
2015-01-19 9:49 ` Takashi Iwai
2015-01-19 9:53 ` Jiri Slaby
2015-01-19 9:56 ` [PATCH v2 2/2] sound: dummy, avoid race " Jiri Slaby
2015-01-19 9:59 ` Takashi Iwai
2015-01-19 9:53 ` [PATCH 1/3] sound: dummy, use setup_timer and mod_timer Takashi Iwai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).