[alsa-devel] snd_pcsp locking mess

[Stas Sergeev]

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.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: as_stop.diff
Type: text/x-patch
Size: 1301 bytes
Desc: not available
Url : http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20080821/34dabccd/attachment.diff