Re: snd_pcsp locking mess

[Takashi Iwai <tiwai@suse.de>]

Hi,

whipping this old horse again...

At Thu, 21 Aug 2008 14:25:36 +0400,
Stas Sergeev wrote:
> 
> Hello.
> 
> Takashi Iwai wrote:
> > I don't remember exactly, but adding a new callback doesn't sound so
> > perfect.  Well, it's just one pointer, but it's added all over
> > places.
> Well, it can be a concern only as long
> as the callback is entirely useless. :)
> If it does the right thing, then it
> certainly worth a pointer.
> 
> > Anyway, could you repost it?  Then we can discuss about it more
> > concretely.
> Attached, and the entire message is at
> the bottom.
> 
> > A known problem with PCM substream lock is that it's to be used for
> > multiple bound streams as well.  My concern is whether this can be
> > still avoided well by your changes.
> That's why I haven't made the patch
> for the PCM locking change. Someone
> else should know better how to make
> it the safe way.

Indeed, the async trigger is nice to have in the common place.
However, the change wouldn't be as trivial as it sounds, as you
mentioned.  By async nature, there can be a transition phase between
the XRUN and STOP, which can cause races.

A possible solution is to introduce an intermediate sound state
(e.g. SOUND_STATE_TRIGGER_PENDING), but this anyway requires a major
rewrite in the PCM core code.

So, I first applied my patch for 2.6.29 (not 28) for further testing.
Let's consider more on async trigger later...


thanks,

Takashi


> Below is an old message with the patch.
> ---
> Hi.
> 
> I was trying to get the locking right
> in my pcsp driver, and I have the following
> problem.
> I am using the chip->playback_substream in
> the IRQ handler context. To prevent the chance
> of closing the substream on another CPU while
> the IRQ handler still messes with it, I
> decided to protect it with the spinlock.
> So I acquire the same lock both in an IRQ
> handler and in the pcsp_stop_playing() routine.
> That way I can be sure they never step on
> each other's feet, even on SMP.
> The problem is though that ALSA calls the
> trigger() function to stop the playback both
> within the task context and within the IRQ
> context (later is via snd_pcm_period_elapsed(),
> which is called from an IRQ context).
> So the above locking scheme does not work,
> because trigger() being called from an IRQ
> context, takes the already taken lock.
> I can drop the lock before calling snd_pcm_period_elapsed(),
> but I beleive this opens a (very small) race
> condition.
> What can be a solution to this problem?
> I think it is never too good to call the same
> callbacks from both the task and IRQ contexts,
> but the ALSA does this. In other words, can
> something like the attached patch ever be applied,
> or am I misunderstanding the problem completely?
> The patch adds the separate callback, which is
> intended to be called from an IRQ context only.
> Does this look like the right solution?
> ---
> 
> [2 as_stop.diff <text/x-patch (7bit)>]
> --- linux-2.6.21/sound/core/pcm_native.c.old	2007-05-12 11:30:06.000000000 +0400
> +++ linux-2.6.21/sound/core/pcm_native.c	2007-05-20 01:22:45.000000000 +0400
> @@ -922,8 +922,14 @@
>  static int snd_pcm_do_stop(struct snd_pcm_substream *substream, int state)
>  {
>  	if (substream->runtime->trigger_master == substream &&
> -	    snd_pcm_running(substream))
> -		substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP);
> +	    snd_pcm_running(substream)) {
> +		if (substream->ops->async_stop)
> +			/* the driver provides a separate callback
> +			 * for the IRQ context */
> +			substream->ops->async_stop(substream);
> +		else
> +			substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP);
> +	}
>  	return 0; /* unconditonally stop all substreams */
>  }
>  
> --- linux-2.6.21/include/sound/pcm.h.old	2007-05-12 11:30:01.000000000 +0400
> +++ linux-2.6.21/include/sound/pcm.h	2007-05-20 01:21:29.000000000 +0400
> @@ -68,6 +68,7 @@
>  	int (*hw_free)(struct snd_pcm_substream *substream);
>  	int (*prepare)(struct snd_pcm_substream *substream);
>  	int (*trigger)(struct snd_pcm_substream *substream, int cmd);
> +	int (*async_stop)(struct snd_pcm_substream *substream);
>  	snd_pcm_uframes_t (*pointer)(struct snd_pcm_substream *substream);
>  	int (*copy)(struct snd_pcm_substream *substream, int channel,
>  		    snd_pcm_uframes_t pos,
>