Re: snd_pcsp locking mess

[Stas Sergeev <stsp@aknet.ru>]

Hello.

Takashi Iwai wrote:
> The substream lock can be removed from hrtimer callback gracefully.
> So, you have no longer the issue with this lock/unlock procedure.
> In hrtimer callback, you just need to have the DMA-buffer.  That can
> be protected without the substream lock, too, I believe.
OK. But still, the substream have to
be protected against the close callback.
So I still need 2 locks. One for the
substream, another for the DMA buffer.
That makes things better but not much.
I don't want 2 locks in a softirq callback,
even if they are independant.
Or am I still missing your point?

> In this scenario, the trigger-stop could be asynchronous.  Thus, the
> prepare PCM callback should sync also with the hrtimer callback to
> assure that the pending callback is finished.
OK, so I need 3 locks after all - adding
one for the position pointers. Now that
may be not even better than using the
pcm_stream_lock() after all...
Can there be a more advanced version
of a pcm stream lock? The one that will
take something persistant as an argument
(chip->pcm perhaps), take some lock from
there, then check if the substream is
not freed, and then take the substream
lock. And if substream was freed - it
unlocks and returns an error. Effectively
similar to the current snd_pcm_stream_lock(),
but without a need for the second lock
to protect the substream itself.

>> code. And I remember the last time we
>> discussed this, you admitted (some of)
>> the other drivers do get this part wrong
>> too. So fixing it properly once and for
>> all might be a good idea as you are at
>> it again.
> I don't agree here unless any code is shown :)
OK, lets try. My assumption is that all
drivers do have the locking wrong because
of that policy, but please show me what
am I missing. :)
So lets see hda_intel.c.
What it does is to release some reg_lock
before calling snd_pcm_period_elapsed().
It takes that lock in azx_pcm_close().
If it spins on that lock in azx_pcm_close()
on one CPU and drops that lock in an IRQ
handler on another CPU, then, I beleive,
it will crash in snd_pcm_period_elapsed(),
as the substream is freed before it takes
the lock again.
What am I missing? (that can't happen with
a single CPU perhaps, as the lock is IRQ-safe,
but IIRC this doesn't help on SMP at all)
Or, what nm256.c does... hmm, it doesn't
even do "substream = NULL" in a close
callback - why?