[alsa-devel] [PATCH 08/16] ALSA: line6: Drop superfluous spinlock for trigger

[Takashi Iwai]

At Tue, 27 Jan 2015 00:22:48 -0600,
Chris Rorvick wrote:
> 
> On Fri, Jan 23, 2015 at 11:13 AM, Takashi Iwai <tiwai at suse.de> wrote:
> > The trigger callback is already spinlocked, so we need no more lock
> > here (even for the linked substreams).  Let's drop it.
> 
> Are they linked?  It doesn't look like they are for my TonePort UX2.

They can be linked when specified by application via snd_pcm_link().
It's not always linked.

> I assumed this is why the trigger lock existed, to synchronize the
> trigger callbacks between playback and capture because the group lock
> was *not* being acquired.

For the trigger part, both LINE6_BITS_PLAYBACK_STREAM and
LINE6_BITS_CAPTURE_STREAM can be dealt really independently.  Thus
there is no reason to protect the parallel triggers for both
directions.

And, a stream lock is always held for the trigger callback (in
addition to a group lock if linked).

> And the purpose of this synchronization I assumed was to really
> synchronize the calls to line6_pcm_acquire/release().  But then
> hw_params() and hw_free() are calling into these seemingly without any
> lock.

Right.  And, reading the code more carefully, you'll find that a lock
can't help there at all.  The current line6_pcm_acquire() and
line6_pcm_release() are already racy.  At the beginning of each
function, it tries to be consistent about the bit flags.  But that's
all.  It sets to the new bits there, and checks the diff between the
old and the new.  This doesn't protect against  the renewal of the
same bit while the bit is being handled.

For example, suppose a task A calls line6_pcm_release(*_BUFFER).  It
clears the *_BUFFER bits and then tries to free a buffer via kfree().
During it, suppose a task B calls line6_pcm_acquire() with the same
*_BUFFER flag.  The bits was already cleared, so it sets again and
processes the *_BUFFER part, where it allocates a buffer via
kmalloc().  If the task B is executed before kfree() call of task A,
kmalloc() may be performed and replace line6pcm->buffer_out.  Then
task B calls kfree() with the newly allocated one.  Ouch.

> And line6_pcm_acquire/release() are updating line6pcm->flags
> atomically, but elsewhere we're just using test_and_set_bit() and
> such.
>
> All of this looks racy to me.  Am I missing something?

Yes, the code *is* already racy :)
And the lock doesn't anything there.  It protects the parallel call of
playback and capture directions, but these parallel triggers are
actually no problem, so it just gives the unnecessary protection.

> Finally, what is the point of dispatching through this rather than to
> the respective playback/capture callback directly?  Would this come
> into play if I was seeing linked substreams?

I can only guess: the reason is likely to simplify the call for
IMPULSE or MONITOR cases.  In the current form, they can be a simple
line6_pcm_acquire() or _release() call with the combined bit flags.
If the handling of streams are separated, it'll need multiple calls.

I don't mean that the current form is good, though.  Rather I find
it makes thing unnecessarily too complex, and as already mentioned,
it's racy, doesn't work as expected.  So I think we should straighten
the code later, in anyway.

> Sorry, this is way beyond the scope of this patch.  But I've been
> spending some time trying to sort this out and haven't answered the
> above questions.  If I'm missing anything, a pointer to the relevant
> source would be greatly appreciated!

HTH,

Takashi