At Tue, 27 Jan 2015 00:22:48 -0600, Chris Rorvick wrote:
On Fri, Jan 23, 2015 at 11:13 AM, Takashi Iwai tiwai@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