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
