[pulseaudio-discuss] [PATCH v0 1/2] bluetooth: Do not setup stream before thread starts

[Tanu Kaskinen]

On Sun, 2012-12-02 at 10:02 +0100, Mikel Astiz wrote:
> Hi Tanu,
> 
> On Sun, Dec 2, 2012 at 12:45 AM, Tanu Kaskinen <tanuk at iki.fi> wrote:
> > On Thu, 2012-11-29 at 14:28 +0100, Mikel Astiz wrote:
> >> From: Mikel Astiz <mikel.astiz at bmw-carit.de>
> >>
> >> bt_transport_acquire() might get called from the main thread, in case
> >> the IO thread hasn't been started yet. In this case, we should not call
> >> setup_stream() since this is going to be called in the beginning of
> >> thread_func().
> >> ---
> >>  src/modules/bluetooth/module-bluetooth-device.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
> >> index 136c7da..d1c386f 100644
> >> --- a/src/modules/bluetooth/module-bluetooth-device.c
> >> +++ b/src/modules/bluetooth/module-bluetooth-device.c
> >> @@ -424,6 +424,10 @@ static int bt_transport_acquire(struct userdata *u, pa_bool_t start) {
> >>          return 0;
> >>
> >>  done:
> >> +    /* If thread is still about to start, the stream will be set up in the beginning of thread_func() */
> >> +    if (u->thread == NULL)
> >> +        return 0;
> >
> > Otherwise fine, but isn't it random whether u->thread == NULL when
> > bt_transport_acquire() is called in the beginning of thread_func()? It
> 
> Yes, you're right. I haven't seen any issue during testing but this
> code is racy.
> 
> Alternatively we could use u->rtpoll instead of u->thread. But even
> then, I'm not 100% sure of how compiler instruction reordering could
> affect it.
> 
> A safer choice could be to just use the pa_thread_mq_get(), exactly as
> pa_assert_ctl_context() does.
> 
> > looks like a race to me. Would it be better to call setup_stream()
> > directly in thread_func() instead of calling bt_transport_acquire()?
> 
> If what you're proposing is to modify thread_func() by replacing
> bt_transport_acquire() with a call to setup_stream(), I can't think
> how that would be helping. You still need to avoid the call to
> setup_stream() from the main thread, so it doesn't get called in case
> the main thread calls pa_transport_acquire(u, TRUE), which is
> basically the issue that this patch was trying to solve (since such a
> call is proposed in patch v0 2/2).

My proposal is that we keep the "if (u->thread == NULL)" check as it is.
The only problem with that is the bt_transport_acquire() call in the
beginning of thread_func(), and that can be fixed by calling
setup_stream() directly.

-- 
Tanu