Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

[Takashi Sakamoto]

Hi,

On Sep 2 2017 10:19, Takashi Sakamoto wrote:

Well, in sound subsystem, there're a few drivers which uses hrtimer:
  - snd-pcsp
  - snd-sh-dac-audio
  - snd-soc-imx-pcm-fiq

As a quick glance, 'snd-sh-dac-audio' includes the same bug, too. 
Additionally, 'snd-soc-imx-pcm-fiq' maintains hrtimer with loose manner 
in a point of state of PCM substream and it shall gain the same bug if 
improved. Later, I posted some patches for them.


After reading code thoroughly, I conclude that no need to fix these two
drivers. They're programmed with own protections. The former
(snd-sh-dac-audio) has 'struct snd_sh_dac.empty' and the latter
(snd-soc-imx-pcm-fiq) has 'struct imx_pcm_runtime_data.playing' and
'.capturing', to avoid cancellation of hrtimer on hrtimer callback.

These ways are not necessarily efficient but actually have no trouble.
I leave them as is.


Regards

Takashi Sakamoto

Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

[Takashi Sakamoto]

Hi,

On Sep 2 2017 10:19, Takashi Sakamoto wrote:

Well, in sound subsystem, there're a few drivers which uses hrtimer:
  - snd-pcsp
  - snd-sh-dac-audio
  - snd-soc-imx-pcm-fiq

As a quick glance, 'snd-sh-dac-audio' includes the same bug, too. 
Additionally, 'snd-soc-imx-pcm-fiq' maintains hrtimer with loose manner 
in a point of state of PCM substream and it shall gain the same bug if 
improved. Later, I posted some patches for them.


After reading code thoroughly, I conclude that no need to fix these two
drivers. They're programmed with own protections. The former
(snd-sh-dac-audio) has 'struct snd_sh_dac.empty' and the latter
(snd-soc-imx-pcm-fiq) has 'struct imx_pcm_runtime_data.playing' and
'.capturing', to avoid cancellation of hrtimer on hrtimer callback.

These ways are not necessarily efficient but actually have no trouble.
I leave them as is.


Regards

Takashi Sakamoto

Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

[Takashi Sakamoto]

On p 1 2017 20:58, Takashi Iwai wrote:

>From 07d61ba2a1c0e06e914443225e194d99f2d8c58d Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto 
Date: Fri, 1 Sep 2017 19:10:18 +0900
Subject: [PATCH] ALSA: dummy: avoid stall due to a call of hrtimer_cancel() on
  a callback of hrtimer

A call of 'htrimer_cancel()' on a callback of hrtimer brings endless loop
because 'struct hrtimer_clock_base.running' is not NULL on the callback.
In hrtimer subsystem, this member is used to indicate the instance of
hrtimer gets callbacks and there's a helper function,
'hrtimer_callback_running()' to check it.

ALSA dummy driver uses hrtimer to emulate hardware interrupt per period
of PCM buffer. When XRUN occurs on PCM substream, in a call of
'snd_pcm_period_elapsed()', 'struct snd_pcm_ops.stop()' is called to
stop the substream. In current implementation, 'hrtimer_cancel()' is
used to wait for cancellation of hrtimer. However, as described, this
brings endless loop.


It's not only about XRUN.  When the stream finishes the draining, it
stops the stream gracefully -- that is the very normal operation.


I overlooked it. Thanks for your indication.


For this problem, this commit uses 'hrtimer_callback_running()' to
detect whether to be on a callback of hrtimer or not, then skip
cancellation of hrtimer in hrtimer callbacks. Furthermore, at a case of
XRUN, hrtimer callback returns HRTIMER_NORESTART after a call of
'snd_pcm_period_elapsed()' to discontinue hrtimr because cancellation is
skipped.

Signed-off-by: Takashi Sakamoto 


It's better to fold the fix into the original patch instead of
introducing a bug and fixing it.


Yep. I request the authors to include this fix.


Well, in sound subsystem, there're a few drivers which uses hrtimer:
 - snd-pcsp
 - snd-sh-dac-audio
 - snd-soc-imx-pcm-fiq

As a quick glance, 'snd-sh-dac-audio' includes the same bug, too. 
Additionally, 'snd-soc-imx-pcm-fiq' maintains hrtimer with loose manner 
in a point of state of PCM substream and it shall gain the same bug if 
improved. Later, I posted some patches for them.



Thanks

Takashi Sakamoto

Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

[Takashi Sakamoto]

On p 1 2017 20:58, Takashi Iwai wrote:

>From 07d61ba2a1c0e06e914443225e194d99f2d8c58d Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto 
Date: Fri, 1 Sep 2017 19:10:18 +0900
Subject: [PATCH] ALSA: dummy: avoid stall due to a call of hrtimer_cancel() on
  a callback of hrtimer

A call of 'htrimer_cancel()' on a callback of hrtimer brings endless loop
because 'struct hrtimer_clock_base.running' is not NULL on the callback.
In hrtimer subsystem, this member is used to indicate the instance of
hrtimer gets callbacks and there's a helper function,
'hrtimer_callback_running()' to check it.

ALSA dummy driver uses hrtimer to emulate hardware interrupt per period
of PCM buffer. When XRUN occurs on PCM substream, in a call of
'snd_pcm_period_elapsed()', 'struct snd_pcm_ops.stop()' is called to
stop the substream. In current implementation, 'hrtimer_cancel()' is
used to wait for cancellation of hrtimer. However, as described, this
brings endless loop.


It's not only about XRUN.  When the stream finishes the draining, it
stops the stream gracefully -- that is the very normal operation.


I overlooked it. Thanks for your indication.


For this problem, this commit uses 'hrtimer_callback_running()' to
detect whether to be on a callback of hrtimer or not, then skip
cancellation of hrtimer in hrtimer callbacks. Furthermore, at a case of
XRUN, hrtimer callback returns HRTIMER_NORESTART after a call of
'snd_pcm_period_elapsed()' to discontinue hrtimr because cancellation is
skipped.

Signed-off-by: Takashi Sakamoto 


It's better to fold the fix into the original patch instead of
introducing a bug and fixing it.


Yep. I request the authors to include this fix.


Well, in sound subsystem, there're a few drivers which uses hrtimer:
 - snd-pcsp
 - snd-sh-dac-audio
 - snd-soc-imx-pcm-fiq

As a quick glance, 'snd-sh-dac-audio' includes the same bug, too. 
Additionally, 'snd-soc-imx-pcm-fiq' maintains hrtimer with loose manner 
in a point of state of PCM substream and it shall gain the same bug if 
improved. Later, I posted some patches for them.



Thanks

Takashi Sakamoto

Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

[Takashi Iwai]

On Fri, 01 Sep 2017 12:25:37 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Sep 1 2017 00:36, Takashi Iwai wrote:
> > I gave it at try, but it caused a kernel hang, unfortunately.
> > 
> > The reason is that snd_pcm_period_elapased() may stop the stream
> > (e.g. when reaching at the end).  With this patchset, it'll lead to
> > the call of hrtimer_cancel() from the hrtimer callback itself, thus it
> > stalls.
>  
> I can reproduce this bug.
> 
> > Below is the additional fix over your patch for working around it.
> > I believe it should cover most corner cases, and seems working fine
> > through quick tests, so far.
> 
> This patch looks good to me, too. But I have an alternative.
> 
> We can use 'hrtimer_callback_running()' to detect whether to be on hrtimer
> callback or not (please read '__run_hrtimer()' in 'kernel/time/hrtimer.c').

A good point, this is a better choice.

> Usage of this helper function on .stop callback to skip cancellation can
> avoid the stall. In this case, after stopping PCM substream, the hrtimer
> callback should return HRTIMER_NORESTART to avoid restarting, as well as
> your patch.  Please test a patch in this message.
> 
> > ---
> > diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
> > index 273d60c42125..b5dd64e3dab1 100644
> > --- a/sound/drivers/dummy.c
> > +++ b/sound/drivers/dummy.c
> > @@ -375,6 +375,7 @@ struct dummy_hrtimer_pcm {
> > ktime_t base_time;
> > ktime_t period_time;
> > atomic_t running;
> > +   atomic_t callback_running;
> > struct hrtimer timer;
> > struct snd_pcm_substream *substream;
> >   };
> > @@ -387,8 +388,15 @@ static enum hrtimer_restart 
> > dummy_hrtimer_callback(struct hrtimer *timer)
> > if (!atomic_read(>running))
> > return HRTIMER_NORESTART;
> >   
> > +   atomic_inc(>callback_running);
> > snd_pcm_period_elapsed(dpcm->substream);
> > +   atomic_dec(>callback_running);
> > +   /* may be flipped during snd_pcm_period_elapsed() */
> > +   if (!atomic_read(>running))
> > +   return HRTIMER_NORESTART;
> > +
> > hrtimer_forward_now(timer, dpcm->period_time);
> > +   atomic_dec(>callback_running);
> > return HRTIMER_RESTART;
> >   }
> >   
> > @@ -407,7 +415,9 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream 
> > *substream)
> > struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
> >   
> > atomic_set(>running, 0);
> > -   hrtimer_cancel(>timer);
> > +   /* issue hrtimer_cancel() only when called outside the callback */
> > +   if (!atomic_read(>callback_running))
> > +   hrtimer_cancel(>timer);
> > return 0;
> >   }
> >   
> > @@ -462,6 +472,7 @@ static int dummy_hrtimer_create(struct 
> > snd_pcm_substream *substream)
> > dpcm->timer.function = dummy_hrtimer_callback;
> > dpcm->substream = substream;
> > atomic_set(>running, 0);
> > +   atomic_set(>callback_running, 0);
> > return 0;
> >   }
> 
> >From 07d61ba2a1c0e06e914443225e194d99f2d8c58d Mon Sep 17 00:00:00 2001
> From: Takashi Sakamoto 
> Date: Fri, 1 Sep 2017 19:10:18 +0900
> Subject: [PATCH] ALSA: dummy: avoid stall due to a call of hrtimer_cancel() on
>  a callback of hrtimer
> 
> A call of 'htrimer_cancel()' on a callback of hrtimer brings endless loop
> because 'struct hrtimer_clock_base.running' is not NULL on the callback.
> In hrtimer subsystem, this member is used to indicate the instance of
> hrtimer gets callbacks and there's a helper function,
> 'hrtimer_callback_running()' to check it.
> 
> ALSA dummy driver uses hrtimer to emulate hardware interrupt per period
> of PCM buffer. When XRUN occurs on PCM substream, in a call of
> 'snd_pcm_period_elapsed()', 'struct snd_pcm_ops.stop()' is called to
> stop the substream. In current implementation, 'hrtimer_cancel()' is
> used to wait for cancellation of hrtimer. However, as described, this
> brings endless loop.

It's not only about XRUN.  When the stream finishes the draining, it
stops the stream gracefully -- that is the very normal operation.

> For this problem, this commit uses 'hrtimer_callback_running()' to
> detect whether to be on a callback of hrtimer or not, then skip
> cancellation of hrtimer in hrtimer callbacks. Furthermore, at a case of
> XRUN, hrtimer callback returns HRTIMER_NORESTART after a call of
> 'snd_pcm_period_elapsed()' to discontinue hrtimr because cancellation is
> skipped.
> 
> Signed-off-by: Takashi Sakamoto 

It's better to fold the fix into the original patch instead of
introducing a bug and fixing it.


Takashi

Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

[Takashi Iwai]

On Fri, 01 Sep 2017 12:25:37 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Sep 1 2017 00:36, Takashi Iwai wrote:
> > I gave it at try, but it caused a kernel hang, unfortunately.
> > 
> > The reason is that snd_pcm_period_elapased() may stop the stream
> > (e.g. when reaching at the end).  With this patchset, it'll lead to
> > the call of hrtimer_cancel() from the hrtimer callback itself, thus it
> > stalls.
>  
> I can reproduce this bug.
> 
> > Below is the additional fix over your patch for working around it.
> > I believe it should cover most corner cases, and seems working fine
> > through quick tests, so far.
> 
> This patch looks good to me, too. But I have an alternative.
> 
> We can use 'hrtimer_callback_running()' to detect whether to be on hrtimer
> callback or not (please read '__run_hrtimer()' in 'kernel/time/hrtimer.c').

A good point, this is a better choice.

> Usage of this helper function on .stop callback to skip cancellation can
> avoid the stall. In this case, after stopping PCM substream, the hrtimer
> callback should return HRTIMER_NORESTART to avoid restarting, as well as
> your patch.  Please test a patch in this message.
> 
> > ---
> > diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
> > index 273d60c42125..b5dd64e3dab1 100644
> > --- a/sound/drivers/dummy.c
> > +++ b/sound/drivers/dummy.c
> > @@ -375,6 +375,7 @@ struct dummy_hrtimer_pcm {
> > ktime_t base_time;
> > ktime_t period_time;
> > atomic_t running;
> > +   atomic_t callback_running;
> > struct hrtimer timer;
> > struct snd_pcm_substream *substream;
> >   };
> > @@ -387,8 +388,15 @@ static enum hrtimer_restart 
> > dummy_hrtimer_callback(struct hrtimer *timer)
> > if (!atomic_read(>running))
> > return HRTIMER_NORESTART;
> >   
> > +   atomic_inc(>callback_running);
> > snd_pcm_period_elapsed(dpcm->substream);
> > +   atomic_dec(>callback_running);
> > +   /* may be flipped during snd_pcm_period_elapsed() */
> > +   if (!atomic_read(>running))
> > +   return HRTIMER_NORESTART;
> > +
> > hrtimer_forward_now(timer, dpcm->period_time);
> > +   atomic_dec(>callback_running);
> > return HRTIMER_RESTART;
> >   }
> >   
> > @@ -407,7 +415,9 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream 
> > *substream)
> > struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
> >   
> > atomic_set(>running, 0);
> > -   hrtimer_cancel(>timer);
> > +   /* issue hrtimer_cancel() only when called outside the callback */
> > +   if (!atomic_read(>callback_running))
> > +   hrtimer_cancel(>timer);
> > return 0;
> >   }
> >   
> > @@ -462,6 +472,7 @@ static int dummy_hrtimer_create(struct 
> > snd_pcm_substream *substream)
> > dpcm->timer.function = dummy_hrtimer_callback;
> > dpcm->substream = substream;
> > atomic_set(>running, 0);
> > +   atomic_set(>callback_running, 0);
> > return 0;
> >   }
> 
> >From 07d61ba2a1c0e06e914443225e194d99f2d8c58d Mon Sep 17 00:00:00 2001
> From: Takashi Sakamoto 
> Date: Fri, 1 Sep 2017 19:10:18 +0900
> Subject: [PATCH] ALSA: dummy: avoid stall due to a call of hrtimer_cancel() on
>  a callback of hrtimer
> 
> A call of 'htrimer_cancel()' on a callback of hrtimer brings endless loop
> because 'struct hrtimer_clock_base.running' is not NULL on the callback.
> In hrtimer subsystem, this member is used to indicate the instance of
> hrtimer gets callbacks and there's a helper function,
> 'hrtimer_callback_running()' to check it.
> 
> ALSA dummy driver uses hrtimer to emulate hardware interrupt per period
> of PCM buffer. When XRUN occurs on PCM substream, in a call of
> 'snd_pcm_period_elapsed()', 'struct snd_pcm_ops.stop()' is called to
> stop the substream. In current implementation, 'hrtimer_cancel()' is
> used to wait for cancellation of hrtimer. However, as described, this
> brings endless loop.

It's not only about XRUN.  When the stream finishes the draining, it
stops the stream gracefully -- that is the very normal operation.

> For this problem, this commit uses 'hrtimer_callback_running()' to
> detect whether to be on a callback of hrtimer or not, then skip
> cancellation of hrtimer in hrtimer callbacks. Furthermore, at a case of
> XRUN, hrtimer callback returns HRTIMER_NORESTART after a call of
> 'snd_pcm_period_elapsed()' to discontinue hrtimr because cancellation is
> skipped.
> 
> Signed-off-by: Takashi Sakamoto 

It's better to fold the fix into the original patch instead of
introducing a bug and fixing it.


Takashi

Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

[Takashi Sakamoto]

Hi,

On Sep 1 2017 00:36, Takashi Iwai wrote:
> I gave it at try, but it caused a kernel hang, unfortunately.
> 
> The reason is that snd_pcm_period_elapased() may stop the stream
> (e.g. when reaching at the end).  With this patchset, it'll lead to
> the call of hrtimer_cancel() from the hrtimer callback itself, thus it
> stalls.
 
I can reproduce this bug.

> Below is the additional fix over your patch for working around it.
> I believe it should cover most corner cases, and seems working fine
> through quick tests, so far.

This patch looks good to me, too. But I have an alternative.

We can use 'hrtimer_callback_running()' to detect whether to be on hrtimer
callback or not (please read '__run_hrtimer()' in 'kernel/time/hrtimer.c').
Usage of this helper function on .stop callback to skip cancellation can
avoid the stall. In this case, after stopping PCM substream, the hrtimer
callback should return HRTIMER_NORESTART to avoid restarting, as well as
your patch.  Please test a patch in this message.

> ---
> diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
> index 273d60c42125..b5dd64e3dab1 100644
> --- a/sound/drivers/dummy.c
> +++ b/sound/drivers/dummy.c
> @@ -375,6 +375,7 @@ struct dummy_hrtimer_pcm {
>   ktime_t base_time;
>   ktime_t period_time;
>   atomic_t running;
> + atomic_t callback_running;
>   struct hrtimer timer;
>   struct snd_pcm_substream *substream;
>   };
> @@ -387,8 +388,15 @@ static enum hrtimer_restart 
> dummy_hrtimer_callback(struct hrtimer *timer)
>   if (!atomic_read(>running))
>   return HRTIMER_NORESTART;
>   
> + atomic_inc(>callback_running);
>   snd_pcm_period_elapsed(dpcm->substream);
> + atomic_dec(>callback_running);
> + /* may be flipped during snd_pcm_period_elapsed() */
> + if (!atomic_read(>running))
> + return HRTIMER_NORESTART;
> +
>   hrtimer_forward_now(timer, dpcm->period_time);
> + atomic_dec(>callback_running);
>   return HRTIMER_RESTART;
>   }
>   
> @@ -407,7 +415,9 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream 
> *substream)
>   struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
>   
>   atomic_set(>running, 0);
> - hrtimer_cancel(>timer);
> + /* issue hrtimer_cancel() only when called outside the callback */
> + if (!atomic_read(>callback_running))
> + hrtimer_cancel(>timer);
>   return 0;
>   }
>   
> @@ -462,6 +472,7 @@ static int dummy_hrtimer_create(struct snd_pcm_substream 
> *substream)
>   dpcm->timer.function = dummy_hrtimer_callback;
>   dpcm->substream = substream;
>   atomic_set(>running, 0);
> + atomic_set(>callback_running, 0);
>   return 0;
>   }

>From 07d61ba2a1c0e06e914443225e194d99f2d8c58d Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto 
Date: Fri, 1 Sep 2017 19:10:18 +0900
Subject: [PATCH] ALSA: dummy: avoid stall due to a call of hrtimer_cancel() on
 a callback of hrtimer

A call of 'htrimer_cancel()' on a callback of hrtimer brings endless loop
because 'struct hrtimer_clock_base.running' is not NULL on the callback.
In hrtimer subsystem, this member is used to indicate the instance of
hrtimer gets callbacks and there's a helper function,
'hrtimer_callback_running()' to check it.

ALSA dummy driver uses hrtimer to emulate hardware interrupt per period
of PCM buffer. When XRUN occurs on PCM substream, in a call of
'snd_pcm_period_elapsed()', 'struct snd_pcm_ops.stop()' is called to
stop the substream. In current implementation, 'hrtimer_cancel()' is
used to wait for cancellation of hrtimer. However, as described, this
brings endless loop.

For this problem, this commit uses 'hrtimer_callback_running()' to
detect whether to be on a callback of hrtimer or not, then skip
cancellation of hrtimer in hrtimer callbacks. Furthermore, at a case of
XRUN, hrtimer callback returns HRTIMER_NORESTART after a call of
'snd_pcm_period_elapsed()' to discontinue hrtimr because cancellation is
skipped.

Signed-off-by: Takashi Sakamoto 
---
 sound/drivers/dummy.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index 273d60c42125..9caf754c6135 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -387,7 +387,11 @@ static enum hrtimer_restart dummy_hrtimer_callback(struct 
hrtimer *timer)
if (!atomic_read(>running))
return HRTIMER_NORESTART;
 
+   /* In a case of XRUN, this calls .trigger to stop PCM substream. */
snd_pcm_period_elapsed(dpcm->substream);
+   if (!atomic_read(>running))
+   return HRTIMER_NORESTART;
+
hrtimer_forward_now(timer, dpcm->period_time);
return HRTIMER_RESTART;
 }
@@ -407,7 +411,8 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream 
*substream)
struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
 
atomic_set(>running, 0);
-  

Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

[Takashi Sakamoto]

Hi,

On Sep 1 2017 00:36, Takashi Iwai wrote:
> I gave it at try, but it caused a kernel hang, unfortunately.
> 
> The reason is that snd_pcm_period_elapased() may stop the stream
> (e.g. when reaching at the end).  With this patchset, it'll lead to
> the call of hrtimer_cancel() from the hrtimer callback itself, thus it
> stalls.
 
I can reproduce this bug.

> Below is the additional fix over your patch for working around it.
> I believe it should cover most corner cases, and seems working fine
> through quick tests, so far.

This patch looks good to me, too. But I have an alternative.

We can use 'hrtimer_callback_running()' to detect whether to be on hrtimer
callback or not (please read '__run_hrtimer()' in 'kernel/time/hrtimer.c').
Usage of this helper function on .stop callback to skip cancellation can
avoid the stall. In this case, after stopping PCM substream, the hrtimer
callback should return HRTIMER_NORESTART to avoid restarting, as well as
your patch.  Please test a patch in this message.

> ---
> diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
> index 273d60c42125..b5dd64e3dab1 100644
> --- a/sound/drivers/dummy.c
> +++ b/sound/drivers/dummy.c
> @@ -375,6 +375,7 @@ struct dummy_hrtimer_pcm {
>   ktime_t base_time;
>   ktime_t period_time;
>   atomic_t running;
> + atomic_t callback_running;
>   struct hrtimer timer;
>   struct snd_pcm_substream *substream;
>   };
> @@ -387,8 +388,15 @@ static enum hrtimer_restart 
> dummy_hrtimer_callback(struct hrtimer *timer)
>   if (!atomic_read(>running))
>   return HRTIMER_NORESTART;
>   
> + atomic_inc(>callback_running);
>   snd_pcm_period_elapsed(dpcm->substream);
> + atomic_dec(>callback_running);
> + /* may be flipped during snd_pcm_period_elapsed() */
> + if (!atomic_read(>running))
> + return HRTIMER_NORESTART;
> +
>   hrtimer_forward_now(timer, dpcm->period_time);
> + atomic_dec(>callback_running);
>   return HRTIMER_RESTART;
>   }
>   
> @@ -407,7 +415,9 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream 
> *substream)
>   struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
>   
>   atomic_set(>running, 0);
> - hrtimer_cancel(>timer);
> + /* issue hrtimer_cancel() only when called outside the callback */
> + if (!atomic_read(>callback_running))
> + hrtimer_cancel(>timer);
>   return 0;
>   }
>   
> @@ -462,6 +472,7 @@ static int dummy_hrtimer_create(struct snd_pcm_substream 
> *substream)
>   dpcm->timer.function = dummy_hrtimer_callback;
>   dpcm->substream = substream;
>   atomic_set(>running, 0);
> + atomic_set(>callback_running, 0);
>   return 0;
>   }

>From 07d61ba2a1c0e06e914443225e194d99f2d8c58d Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto 
Date: Fri, 1 Sep 2017 19:10:18 +0900
Subject: [PATCH] ALSA: dummy: avoid stall due to a call of hrtimer_cancel() on
 a callback of hrtimer

A call of 'htrimer_cancel()' on a callback of hrtimer brings endless loop
because 'struct hrtimer_clock_base.running' is not NULL on the callback.
In hrtimer subsystem, this member is used to indicate the instance of
hrtimer gets callbacks and there's a helper function,
'hrtimer_callback_running()' to check it.

ALSA dummy driver uses hrtimer to emulate hardware interrupt per period
of PCM buffer. When XRUN occurs on PCM substream, in a call of
'snd_pcm_period_elapsed()', 'struct snd_pcm_ops.stop()' is called to
stop the substream. In current implementation, 'hrtimer_cancel()' is
used to wait for cancellation of hrtimer. However, as described, this
brings endless loop.

For this problem, this commit uses 'hrtimer_callback_running()' to
detect whether to be on a callback of hrtimer or not, then skip
cancellation of hrtimer in hrtimer callbacks. Furthermore, at a case of
XRUN, hrtimer callback returns HRTIMER_NORESTART after a call of
'snd_pcm_period_elapsed()' to discontinue hrtimr because cancellation is
skipped.

Signed-off-by: Takashi Sakamoto 
---
 sound/drivers/dummy.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index 273d60c42125..9caf754c6135 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -387,7 +387,11 @@ static enum hrtimer_restart dummy_hrtimer_callback(struct 
hrtimer *timer)
if (!atomic_read(>running))
return HRTIMER_NORESTART;
 
+   /* In a case of XRUN, this calls .trigger to stop PCM substream. */
snd_pcm_period_elapsed(dpcm->substream);
+   if (!atomic_read(>running))
+   return HRTIMER_NORESTART;
+
hrtimer_forward_now(timer, dpcm->period_time);
return HRTIMER_RESTART;
 }
@@ -407,7 +411,8 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream 
*substream)
struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
 
atomic_set(>running, 0);
-   hrtimer_cancel(>timer);
+   if 

Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

[Takashi Iwai]

On Thu, 31 Aug 2017 14:23:45 +0200,
Anna-Maria Gleixner wrote:
> 
> From: Thomas Gleixner 
> 
> The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
> the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
> callback in softirq context as well which renders the tasklet useless.
> 
> Signed-off-by: Thomas Gleixner 
> Signed-off-by: Anna-Maria Gleixner 
> Cc: Jaroslav Kysela 
> Cc: Takashi Iwai 
> Cc: Takashi Sakamoto 
> Cc: alsa-de...@alsa-project.org

I gave it at try, but it caused a kernel hang, unfortunately.

The reason is that snd_pcm_period_elapased() may stop the stream
(e.g. when reaching at the end).  With this patchset, it'll lead to
the call of hrtimer_cancel() from the hrtimer callback itself, thus it
stalls.

Below is the additional fix over your patch for working around it.
I believe it should cover most corner cases, and seems working fine
through quick tests, so far.


thanks,

Takashi

---
diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index 273d60c42125..b5dd64e3dab1 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -375,6 +375,7 @@ struct dummy_hrtimer_pcm {
ktime_t base_time;
ktime_t period_time;
atomic_t running;
+   atomic_t callback_running;
struct hrtimer timer;
struct snd_pcm_substream *substream;
 };
@@ -387,8 +388,15 @@ static enum hrtimer_restart dummy_hrtimer_callback(struct 
hrtimer *timer)
if (!atomic_read(>running))
return HRTIMER_NORESTART;
 
+   atomic_inc(>callback_running);
snd_pcm_period_elapsed(dpcm->substream);
+   atomic_dec(>callback_running);
+   /* may be flipped during snd_pcm_period_elapsed() */
+   if (!atomic_read(>running))
+   return HRTIMER_NORESTART;
+
hrtimer_forward_now(timer, dpcm->period_time);
+   atomic_dec(>callback_running);
return HRTIMER_RESTART;
 }
 
@@ -407,7 +415,9 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream 
*substream)
struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
 
atomic_set(>running, 0);
-   hrtimer_cancel(>timer);
+   /* issue hrtimer_cancel() only when called outside the callback */
+   if (!atomic_read(>callback_running))
+   hrtimer_cancel(>timer);
return 0;
 }
 
@@ -462,6 +472,7 @@ static int dummy_hrtimer_create(struct snd_pcm_substream 
*substream)
dpcm->timer.function = dummy_hrtimer_callback;
dpcm->substream = substream;
atomic_set(>running, 0);
+   atomic_set(>callback_running, 0);
return 0;
 }
 

Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

[Takashi Iwai]

On Thu, 31 Aug 2017 14:23:45 +0200,
Anna-Maria Gleixner wrote:
> 
> From: Thomas Gleixner 
> 
> The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
> the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
> callback in softirq context as well which renders the tasklet useless.
> 
> Signed-off-by: Thomas Gleixner 
> Signed-off-by: Anna-Maria Gleixner 
> Cc: Jaroslav Kysela 
> Cc: Takashi Iwai 
> Cc: Takashi Sakamoto 
> Cc: alsa-de...@alsa-project.org

I gave it at try, but it caused a kernel hang, unfortunately.

The reason is that snd_pcm_period_elapased() may stop the stream
(e.g. when reaching at the end).  With this patchset, it'll lead to
the call of hrtimer_cancel() from the hrtimer callback itself, thus it
stalls.

Below is the additional fix over your patch for working around it.
I believe it should cover most corner cases, and seems working fine
through quick tests, so far.


thanks,

Takashi

---
diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index 273d60c42125..b5dd64e3dab1 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -375,6 +375,7 @@ struct dummy_hrtimer_pcm {
ktime_t base_time;
ktime_t period_time;
atomic_t running;
+   atomic_t callback_running;
struct hrtimer timer;
struct snd_pcm_substream *substream;
 };
@@ -387,8 +388,15 @@ static enum hrtimer_restart dummy_hrtimer_callback(struct 
hrtimer *timer)
if (!atomic_read(>running))
return HRTIMER_NORESTART;
 
+   atomic_inc(>callback_running);
snd_pcm_period_elapsed(dpcm->substream);
+   atomic_dec(>callback_running);
+   /* may be flipped during snd_pcm_period_elapsed() */
+   if (!atomic_read(>running))
+   return HRTIMER_NORESTART;
+
hrtimer_forward_now(timer, dpcm->period_time);
+   atomic_dec(>callback_running);
return HRTIMER_RESTART;
 }
 
@@ -407,7 +415,9 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream 
*substream)
struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
 
atomic_set(>running, 0);
-   hrtimer_cancel(>timer);
+   /* issue hrtimer_cancel() only when called outside the callback */
+   if (!atomic_read(>callback_running))
+   hrtimer_cancel(>timer);
return 0;
 }
 
@@ -462,6 +472,7 @@ static int dummy_hrtimer_create(struct snd_pcm_substream 
*substream)
dpcm->timer.function = dummy_hrtimer_callback;
dpcm->substream = substream;
atomic_set(>running, 0);
+   atomic_set(>callback_running, 0);
return 0;
 }
 

Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

[Takashi Iwai]

On Thu, 31 Aug 2017 16:21:17 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Aug 31 2017 21:23, Anna-Maria Gleixner wrote:
> > From: Thomas Gleixner 
> > 
> > The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
> > the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
> > callback in softirq context as well which renders the tasklet useless.
> > 
> > Signed-off-by: Thomas Gleixner 
> > Signed-off-by: Anna-Maria Gleixner 
> > Cc: Jaroslav Kysela 
> > Cc: Takashi Iwai 
> > Cc: Takashi Sakamoto 
> > Cc: alsa-de...@alsa-project.org
> > ---
> >  sound/drivers/dummy.c |   16 +++-
> >  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> I prefer this patch as long as this driver can still receive callbacks
> from hrtimer subsystem.
> 
> Reviewed-by: Takashi Sakamoto 
> 
> Unfortunately, I have too poor machine to compile whole kernel now, thus
> didn't do any tests, sorry.
> 
> I note that ALSA pcsp driver uses a combination of hrtimer/tasklet for the
> same purpose. I think we can simplify it, too. Please refer to a patch in
> the end of this message. (But not tested yet for the above reason...)

The pcsp is a bit special.  It's really high frequent irq calls for
controlling the beep on/off, thus offloading the whole isn't good.


thanks,

Takashi

Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

[Takashi Iwai]

On Thu, 31 Aug 2017 16:21:17 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Aug 31 2017 21:23, Anna-Maria Gleixner wrote:
> > From: Thomas Gleixner 
> > 
> > The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
> > the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
> > callback in softirq context as well which renders the tasklet useless.
> > 
> > Signed-off-by: Thomas Gleixner 
> > Signed-off-by: Anna-Maria Gleixner 
> > Cc: Jaroslav Kysela 
> > Cc: Takashi Iwai 
> > Cc: Takashi Sakamoto 
> > Cc: alsa-de...@alsa-project.org
> > ---
> >  sound/drivers/dummy.c |   16 +++-
> >  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> I prefer this patch as long as this driver can still receive callbacks
> from hrtimer subsystem.
> 
> Reviewed-by: Takashi Sakamoto 
> 
> Unfortunately, I have too poor machine to compile whole kernel now, thus
> didn't do any tests, sorry.
> 
> I note that ALSA pcsp driver uses a combination of hrtimer/tasklet for the
> same purpose. I think we can simplify it, too. Please refer to a patch in
> the end of this message. (But not tested yet for the above reason...)

The pcsp is a bit special.  It's really high frequent irq calls for
controlling the beep on/off, thus offloading the whole isn't good.


thanks,

Takashi

Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

[Takashi Sakamoto]

Hi,

On Aug 31 2017 21:23, Anna-Maria Gleixner wrote:
> From: Thomas Gleixner 
> 
> The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
> the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
> callback in softirq context as well which renders the tasklet useless.
> 
> Signed-off-by: Thomas Gleixner 
> Signed-off-by: Anna-Maria Gleixner 
> Cc: Jaroslav Kysela 
> Cc: Takashi Iwai 
> Cc: Takashi Sakamoto 
> Cc: alsa-de...@alsa-project.org
> ---
>  sound/drivers/dummy.c |   16 +++-
>  1 file changed, 3 insertions(+), 13 deletions(-)

I prefer this patch as long as this driver can still receive callbacks
from hrtimer subsystem.

Reviewed-by: Takashi Sakamoto 

Unfortunately, I have too poor machine to compile whole kernel now, thus
didn't do any tests, sorry.

I note that ALSA pcsp driver uses a combination of hrtimer/tasklet for the
same purpose. I think we can simplify it, too. Please refer to a patch in
the end of this message. (But not tested yet for the above reason...)

> --- a/sound/drivers/dummy.c
> +++ b/sound/drivers/dummy.c
> @@ -376,17 +376,9 @@ struct dummy_hrtimer_pcm {
>   ktime_t period_time;
>   atomic_t running;
>   struct hrtimer timer;
> - struct tasklet_struct tasklet;
>   struct snd_pcm_substream *substream;
>  };
>  
> -static void dummy_hrtimer_pcm_elapsed(unsigned long priv)
> -{
> - struct dummy_hrtimer_pcm *dpcm = (struct dummy_hrtimer_pcm *)priv;
> - if (atomic_read(>running))
> - snd_pcm_period_elapsed(dpcm->substream);
> -}
> -
>  static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
>  {
>   struct dummy_hrtimer_pcm *dpcm;
> @@ -394,7 +386,8 @@ static enum hrtimer_restart dummy_hrtime
>   dpcm = container_of(timer, struct dummy_hrtimer_pcm, timer);
>   if (!atomic_read(>running))
>   return HRTIMER_NORESTART;
> - tasklet_schedule(>tasklet);
> +
> + snd_pcm_period_elapsed(dpcm->substream);
>   hrtimer_forward_now(timer, dpcm->period_time);
>   return HRTIMER_RESTART;
>  }
> @@ -421,7 +414,6 @@ static int dummy_hrtimer_stop(struct snd
>  static inline void dummy_hrtimer_sync(struct dummy_hrtimer_pcm *dpcm)
>  {
>   hrtimer_cancel(>timer);
> - tasklet_kill(>tasklet);
>  }
>  
>  static snd_pcm_uframes_t
> @@ -466,12 +458,10 @@ static int dummy_hrtimer_create(struct s
>   if (!dpcm)
>   return -ENOMEM;
>   substream->runtime->private_data = dpcm;
> - hrtimer_init(>timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + hrtimer_init(>timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
>   dpcm->timer.function = dummy_hrtimer_callback;
>   dpcm->substream = substream;
>   atomic_set(>running, 0);
> - tasklet_init(>tasklet, dummy_hrtimer_pcm_elapsed,
> -  (unsigned long)dpcm);
>   return 0;
>  }

>From b2417f83e0ccbdfe1fc6870817ff0a1bd9243c77 Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto 
Date: Thu, 31 Aug 2017 22:52:33 +0900
Subject: [PATCH] ALSA: pcsp: code optimization for hrtimer in software IRQ
 context

In a development period for v4.14, code refactoring was done for hrtimer
subsystem. This enables to receive callbacks from hrtimer in software IRQ
context.

Currently, ALSA pcsp driver uses hrtimer callback to handle period elapse
of PCM buffer and actual calculation of pointer position is done in
software IRQ context of tasklet. This can be simplified to the introduced
hrtimer in software IRQ context.

This commit applies an optimization for the above reason.

Signed-off-by: Takashi Sakamoto 
---
 sound/drivers/pcsp/pcsp.c |  2 +-
 sound/drivers/pcsp/pcsp_lib.c | 24 +---
 2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/sound/drivers/pcsp/pcsp.c b/sound/drivers/pcsp/pcsp.c
index 0dd3f46eb03e..8fac38b81c4f 100644
--- a/sound/drivers/pcsp/pcsp.c
+++ b/sound/drivers/pcsp/pcsp.c
@@ -100,7 +100,7 @@ static int snd_card_pcsp_probe(int devnum, struct device 
*dev)
if (devnum != 0)
return -EINVAL;
 
-   hrtimer_init(_chip.timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+   hrtimer_init(_chip.timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
pcsp_chip.timer.function = pcsp_do_timer;
 
err = snd_card_new(dev, index, id, THIS_MODULE, 0, );
diff --git a/sound/drivers/pcsp/pcsp_lib.c b/sound/drivers/pcsp/pcsp_lib.c
index 2f5a35f38ce1..d2b67463ddd3 100644
--- a/sound/drivers/pcsp/pcsp_lib.c
+++ b/sound/drivers/pcsp/pcsp_lib.c
@@ -21,22 +21,6 @@ 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)
-{
- 

Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

[Takashi Sakamoto]

Hi,

On Aug 31 2017 21:23, Anna-Maria Gleixner wrote:
> From: Thomas Gleixner 
> 
> The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
> the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
> callback in softirq context as well which renders the tasklet useless.
> 
> Signed-off-by: Thomas Gleixner 
> Signed-off-by: Anna-Maria Gleixner 
> Cc: Jaroslav Kysela 
> Cc: Takashi Iwai 
> Cc: Takashi Sakamoto 
> Cc: alsa-de...@alsa-project.org
> ---
>  sound/drivers/dummy.c |   16 +++-
>  1 file changed, 3 insertions(+), 13 deletions(-)

I prefer this patch as long as this driver can still receive callbacks
from hrtimer subsystem.

Reviewed-by: Takashi Sakamoto 

Unfortunately, I have too poor machine to compile whole kernel now, thus
didn't do any tests, sorry.

I note that ALSA pcsp driver uses a combination of hrtimer/tasklet for the
same purpose. I think we can simplify it, too. Please refer to a patch in
the end of this message. (But not tested yet for the above reason...)

> --- a/sound/drivers/dummy.c
> +++ b/sound/drivers/dummy.c
> @@ -376,17 +376,9 @@ struct dummy_hrtimer_pcm {
>   ktime_t period_time;
>   atomic_t running;
>   struct hrtimer timer;
> - struct tasklet_struct tasklet;
>   struct snd_pcm_substream *substream;
>  };
>  
> -static void dummy_hrtimer_pcm_elapsed(unsigned long priv)
> -{
> - struct dummy_hrtimer_pcm *dpcm = (struct dummy_hrtimer_pcm *)priv;
> - if (atomic_read(>running))
> - snd_pcm_period_elapsed(dpcm->substream);
> -}
> -
>  static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
>  {
>   struct dummy_hrtimer_pcm *dpcm;
> @@ -394,7 +386,8 @@ static enum hrtimer_restart dummy_hrtime
>   dpcm = container_of(timer, struct dummy_hrtimer_pcm, timer);
>   if (!atomic_read(>running))
>   return HRTIMER_NORESTART;
> - tasklet_schedule(>tasklet);
> +
> + snd_pcm_period_elapsed(dpcm->substream);
>   hrtimer_forward_now(timer, dpcm->period_time);
>   return HRTIMER_RESTART;
>  }
> @@ -421,7 +414,6 @@ static int dummy_hrtimer_stop(struct snd
>  static inline void dummy_hrtimer_sync(struct dummy_hrtimer_pcm *dpcm)
>  {
>   hrtimer_cancel(>timer);
> - tasklet_kill(>tasklet);
>  }
>  
>  static snd_pcm_uframes_t
> @@ -466,12 +458,10 @@ static int dummy_hrtimer_create(struct s
>   if (!dpcm)
>   return -ENOMEM;
>   substream->runtime->private_data = dpcm;
> - hrtimer_init(>timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + hrtimer_init(>timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
>   dpcm->timer.function = dummy_hrtimer_callback;
>   dpcm->substream = substream;
>   atomic_set(>running, 0);
> - tasklet_init(>tasklet, dummy_hrtimer_pcm_elapsed,
> -  (unsigned long)dpcm);
>   return 0;
>  }

>From b2417f83e0ccbdfe1fc6870817ff0a1bd9243c77 Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto 
Date: Thu, 31 Aug 2017 22:52:33 +0900
Subject: [PATCH] ALSA: pcsp: code optimization for hrtimer in software IRQ
 context

In a development period for v4.14, code refactoring was done for hrtimer
subsystem. This enables to receive callbacks from hrtimer in software IRQ
context.

Currently, ALSA pcsp driver uses hrtimer callback to handle period elapse
of PCM buffer and actual calculation of pointer position is done in
software IRQ context of tasklet. This can be simplified to the introduced
hrtimer in software IRQ context.

This commit applies an optimization for the above reason.

Signed-off-by: Takashi Sakamoto 
---
 sound/drivers/pcsp/pcsp.c |  2 +-
 sound/drivers/pcsp/pcsp_lib.c | 24 +---
 2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/sound/drivers/pcsp/pcsp.c b/sound/drivers/pcsp/pcsp.c
index 0dd3f46eb03e..8fac38b81c4f 100644
--- a/sound/drivers/pcsp/pcsp.c
+++ b/sound/drivers/pcsp/pcsp.c
@@ -100,7 +100,7 @@ static int snd_card_pcsp_probe(int devnum, struct device 
*dev)
if (devnum != 0)
return -EINVAL;
 
-   hrtimer_init(_chip.timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+   hrtimer_init(_chip.timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
pcsp_chip.timer.function = pcsp_do_timer;
 
err = snd_card_new(dev, index, id, THIS_MODULE, 0, );
diff --git a/sound/drivers/pcsp/pcsp_lib.c b/sound/drivers/pcsp/pcsp_lib.c
index 2f5a35f38ce1..d2b67463ddd3 100644
--- a/sound/drivers/pcsp/pcsp_lib.c
+++ b/sound/drivers/pcsp/pcsp_lib.c
@@ -21,22 +21,6 @@ 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(_chip.timer_active)) {
-   struct snd_pcm_substream *substream;
-   substream = pcsp_chip.playback_substream;
-   if (substream)
-  

[PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

[Anna-Maria Gleixner]

From: Thomas Gleixner 

The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
callback in softirq context as well which renders the tasklet useless.

Signed-off-by: Thomas Gleixner 
Signed-off-by: Anna-Maria Gleixner 
Cc: Jaroslav Kysela 
Cc: Takashi Iwai 
Cc: Takashi Sakamoto 
Cc: alsa-de...@alsa-project.org
---
 sound/drivers/dummy.c |   16 +++-
 1 file changed, 3 insertions(+), 13 deletions(-)

--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -376,17 +376,9 @@ struct dummy_hrtimer_pcm {
ktime_t period_time;
atomic_t running;
struct hrtimer timer;
-   struct tasklet_struct tasklet;
struct snd_pcm_substream *substream;
 };
 
-static void dummy_hrtimer_pcm_elapsed(unsigned long priv)
-{
-   struct dummy_hrtimer_pcm *dpcm = (struct dummy_hrtimer_pcm *)priv;
-   if (atomic_read(>running))
-   snd_pcm_period_elapsed(dpcm->substream);
-}
-
 static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
 {
struct dummy_hrtimer_pcm *dpcm;
@@ -394,7 +386,8 @@ static enum hrtimer_restart dummy_hrtime
dpcm = container_of(timer, struct dummy_hrtimer_pcm, timer);
if (!atomic_read(>running))
return HRTIMER_NORESTART;
-   tasklet_schedule(>tasklet);
+
+   snd_pcm_period_elapsed(dpcm->substream);
hrtimer_forward_now(timer, dpcm->period_time);
return HRTIMER_RESTART;
 }
@@ -421,7 +414,6 @@ static int dummy_hrtimer_stop(struct snd
 static inline void dummy_hrtimer_sync(struct dummy_hrtimer_pcm *dpcm)
 {
hrtimer_cancel(>timer);
-   tasklet_kill(>tasklet);
 }
 
 static snd_pcm_uframes_t
@@ -466,12 +458,10 @@ static int dummy_hrtimer_create(struct s
if (!dpcm)
return -ENOMEM;
substream->runtime->private_data = dpcm;
-   hrtimer_init(>timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+   hrtimer_init(>timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
dpcm->timer.function = dummy_hrtimer_callback;
dpcm->substream = substream;
atomic_set(>running, 0);
-   tasklet_init(>tasklet, dummy_hrtimer_pcm_elapsed,
-(unsigned long)dpcm);
return 0;
 }
 

[PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

[Anna-Maria Gleixner]

From: Thomas Gleixner 

The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
callback in softirq context as well which renders the tasklet useless.

Signed-off-by: Thomas Gleixner 
Signed-off-by: Anna-Maria Gleixner 
Cc: Jaroslav Kysela 
Cc: Takashi Iwai 
Cc: Takashi Sakamoto 
Cc: alsa-de...@alsa-project.org
---
 sound/drivers/dummy.c |   16 +++-
 1 file changed, 3 insertions(+), 13 deletions(-)

--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -376,17 +376,9 @@ struct dummy_hrtimer_pcm {
ktime_t period_time;
atomic_t running;
struct hrtimer timer;
-   struct tasklet_struct tasklet;
struct snd_pcm_substream *substream;
 };
 
-static void dummy_hrtimer_pcm_elapsed(unsigned long priv)
-{
-   struct dummy_hrtimer_pcm *dpcm = (struct dummy_hrtimer_pcm *)priv;
-   if (atomic_read(>running))
-   snd_pcm_period_elapsed(dpcm->substream);
-}
-
 static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
 {
struct dummy_hrtimer_pcm *dpcm;
@@ -394,7 +386,8 @@ static enum hrtimer_restart dummy_hrtime
dpcm = container_of(timer, struct dummy_hrtimer_pcm, timer);
if (!atomic_read(>running))
return HRTIMER_NORESTART;
-   tasklet_schedule(>tasklet);
+
+   snd_pcm_period_elapsed(dpcm->substream);
hrtimer_forward_now(timer, dpcm->period_time);
return HRTIMER_RESTART;
 }
@@ -421,7 +414,6 @@ static int dummy_hrtimer_stop(struct snd
 static inline void dummy_hrtimer_sync(struct dummy_hrtimer_pcm *dpcm)
 {
hrtimer_cancel(>timer);
-   tasklet_kill(>tasklet);
 }
 
 static snd_pcm_uframes_t
@@ -466,12 +458,10 @@ static int dummy_hrtimer_create(struct s
if (!dpcm)
return -ENOMEM;
substream->runtime->private_data = dpcm;
-   hrtimer_init(>timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+   hrtimer_init(>timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
dpcm->timer.function = dummy_hrtimer_callback;
dpcm->substream = substream;
atomic_set(>running, 0);
-   tasklet_init(>tasklet, dummy_hrtimer_pcm_elapsed,
-(unsigned long)dpcm);
return 0;
 }