Re: snd_pcsp locking mess

[Stas Sergeev <stsp@aknet.ru>]

Hello.

Takashi Iwai wrote:
>>> Anyway, what I meant is that the part touching I/O ports could be a faster path
>>> than softirq.  And the hrtimer code can use a bit simpler locks.  The hrtimer
>>> callback needs just one thing about DMA buffer - the buffer is allocated and
>>> the address doesn't change during the callback.  So, we'd need the lock for the
>>> hr callback and for the PCM prepare, hwparams and free callbacks, i.e. to
>>> assure that the hrtimer callback is finished when these PCM callbacks may
>>> change the DMA buffer.  Thus this lock doesn't have to be PCM substream lock.
>> I wonder if it would be better to introduce
>> such a lock in an alsa core rather than in a
>> driver. The same way as snd_pcm_stream_lock().
> I don't think it's good to introduce yet another lock in the PCM
> core.  More lock, more complex.  Even now the lock handling is too
> complex (and might be buggy in some cases) in ALSA PCM core.
Then I might be missing something.
Right now I touch runtime->dma_area[]
in a callback. You say it have to be
protected, but not with alsa lock, but
with my own lock. But I do not touch
it ever in any other place. Everything
else happens in an alsa core. So how
would I possibly protect it with some
lock private to snd-pcsp?

> I think, however, it'd be worth to consider snd_pcm_period_elapsed()
> to be softirq in PCM core so that the driver doesn't need
> initializations.  This will also reduce the spin unlock/re-lock
> procedure in IRQ handler around snd_pcm_perild_elapsed().
I guess it will help, but such a solution
has a tendency of being classified as a
"stupid hack", which you've already seen
recently. :)
There really must be other ways, that do
not carry such a risk.

>> Wouldn't it be better to just make
>> snd_pcm_period_elapsed() to not take
>> the stream lock? There is a requirement
>> right now to drop the stream lock before
>> calling this. I still beleive this is
>> the root of all the headache.
> No.  The work done by snd_pcm_period_elapsed() touches critical
> sections and is delicated enough to protect by a lock.
I didn't mean it should run without a lock.
But it shouldn't take it itself, allowing
the caller to _not drop_ it before calling.
Is this acceptable?

> The root of all the headache is the order of the spinlock.  It's just
> one lock in the PCM; and you must take it somewhere to protect.
Certainly, I take it. But the unfortunate
policy is that I have to drop it before
calling snd_pcm_period_elapsed(). Can we
change that?
Of course, that will mean more changes
than just to not take the lock in
snd_pcm_period_elapsed(). Most likely
the snd_pcm_stream_lock() and all its
users will have to be slightly adjusted,
but I think its pretty much a trivial
changes, which will lead to a much cleaner
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.