| Message ID | 1422033203-23254-9-git-send-email-tiwai@suse.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f2a76225b962f00642002fb109aee2e5b0dc4259 |
| Headers | show |
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. 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. 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. 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? 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? 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! Regards, Chris
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
On Tue, Jan 27, 2015 at 4:27 AM, Takashi Iwai <tiwai@suse.de> wrote: > At Tue, 27 Jan 2015 00:22:48 -0600, Chris Rorvick wrote: >> All of this looks racy to me. Am I missing something? > > Yes, the code *is* already racy :) OK, good to know I'm not seeing ghosts. :-) >> 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, Yes, very much. Thanks! Regards, Chris
At Tue, 27 Jan 2015 06:48:22 -0600, Chris Rorvick wrote: > > On Tue, Jan 27, 2015 at 4:27 AM, Takashi Iwai <tiwai@suse.de> wrote: > > At Tue, 27 Jan 2015 00:22:48 -0600, Chris Rorvick wrote: > >> All of this looks racy to me. Am I missing something? > > > > Yes, the code *is* already racy :) > > OK, good to know I'm not seeing ghosts. :-) Hehe. I wrote a few more patches to cover the races in a bit better way (although it's not perfect yet). I'm going to send to ML soon. Could you give tested-by tag if the latest test/line6 branch works -- at least, no serious regression is seen? thanks, Takashi
On Tue, Jan 27, 2015 at 10:10 AM, Takashi Iwai <tiwai@suse.de> wrote: > Could you give tested-by tag if the latest test/line6 branch works -- > at least, no serious regression is seen? The "Fix racy loopback handling" is broken due to the typo, but that drops out later and does not affect the final tree. So, very briefly ... Tested-by: Chris Rorvick <chris@rorvick.com>
At Wed, 28 Jan 2015 00:03:17 -0600, Chris Rorvick wrote: > > On Tue, Jan 27, 2015 at 10:10 AM, Takashi Iwai <tiwai@suse.de> wrote: > > Could you give tested-by tag if the latest test/line6 branch works -- > > at least, no serious regression is seen? > > The "Fix racy loopback handling" is broken due to the typo, but that > drops out later and does not affect the final tree. So, very briefly > ... > > Tested-by: Chris Rorvick <chris@rorvick.com> Thanks! I updated the branch and now merged to for-next. Takashi
On Wed, Jan 28, 2015 at 12:26 AM, Takashi Iwai <tiwai@suse.de> wrote: > At Wed, 28 Jan 2015 00:03:17 -0600, > Chris Rorvick wrote: >> >> On Tue, Jan 27, 2015 at 10:10 AM, Takashi Iwai <tiwai@suse.de> wrote: >> > Could you give tested-by tag if the latest test/line6 branch works -- >> > at least, no serious regression is seen? >> >> The "Fix racy loopback handling" is broken due to the typo, but that >> drops out later and does not affect the final tree. So, very briefly >> ... >> >> Tested-by: Chris Rorvick <chris@rorvick.com> > > Thanks! I updated the branch and now merged to for-next. You're welcome, looks good! Nice to see the playback/capture buffer/stream flags stuff cleaned up. Chris
diff --git a/sound/usb/line6/pcm.c b/sound/usb/line6/pcm.c index 9a2a15f4c4e4..adbcac46b785 100644 --- a/sound/usb/line6/pcm.c +++ b/sound/usb/line6/pcm.c @@ -226,9 +226,8 @@ int snd_line6_trigger(struct snd_pcm_substream *substream, int cmd) { struct snd_line6_pcm *line6pcm = snd_pcm_substream_chip(substream); struct snd_pcm_substream *s; - int err; + int err = 0; - spin_lock(&line6pcm->lock_trigger); clear_bit(LINE6_INDEX_PREPARED, &line6pcm->flags); snd_pcm_group_for_each_entry(s, substream) { @@ -237,32 +236,23 @@ int snd_line6_trigger(struct snd_pcm_substream *substream, int cmd) switch (s->stream) { case SNDRV_PCM_STREAM_PLAYBACK: err = snd_line6_playback_trigger(line6pcm, cmd); - - if (err < 0) { - spin_unlock(&line6pcm->lock_trigger); - return err; - } - break; case SNDRV_PCM_STREAM_CAPTURE: err = snd_line6_capture_trigger(line6pcm, cmd); - - if (err < 0) { - spin_unlock(&line6pcm->lock_trigger); - return err; - } - break; default: dev_err(line6pcm->line6->ifcdev, "Unknown stream direction %d\n", s->stream); + err = -EINVAL; + break; } + if (err < 0) + break; } - spin_unlock(&line6pcm->lock_trigger); - return 0; + return err; } /* control info callback */ @@ -427,7 +417,6 @@ int line6_init_pcm(struct usb_line6 *line6, spin_lock_init(&line6pcm->lock_audio_out); spin_lock_init(&line6pcm->lock_audio_in); - spin_lock_init(&line6pcm->lock_trigger); line6pcm->impulse_period = LINE6_IMPULSE_DEFAULT_PERIOD; line6->line6pcm = line6pcm; diff --git a/sound/usb/line6/pcm.h b/sound/usb/line6/pcm.h index c742b33666eb..a84753ee0fa2 100644 --- a/sound/usb/line6/pcm.h +++ b/sound/usb/line6/pcm.h @@ -308,11 +308,6 @@ struct snd_line6_pcm { spinlock_t lock_audio_in; /** - Spin lock to protect trigger. - */ - spinlock_t lock_trigger; - - /** PCM playback volume (left and right). */ int volume_playback[2];
The trigger callback is already spinlocked, so we need no more lock here (even for the linked substreams). Let's drop it. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/usb/line6/pcm.c | 23 ++++++----------------- sound/usb/line6/pcm.h | 5 ----- 2 files changed, 6 insertions(+), 22 deletions(-)