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

[Mikel Astiz]

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).

Cheers,
Mikel