Hi Tanu, On Sun, Sep 30, 2012 at 9:55 AM, Tanu Kaskinen <tanuk at iki.fi> wrote: > On Fri, 2012-09-28 at 17:45 +0200, Mikel Astiz wrote: >> From: Mikel Astiz <mikel.astiz at bmw-carit.de> >> >> Sinks and sources hold a pointer to asyncmsgq, so they should use the >> reference counting mechanism to make sure this queue is not freed. > > I don't really want to apply this. This shouldn't be needed, so if this > makes some bug unreproducible, it's a case of hiding a programming > error, not really fixing it. As a side comment, note that the rest of the patches in this patchset do not depend on this one, as explained in the cover letter. > > Explanation why this shouldn't be needed: > > The pa_sink.asyncmsgq variable should only be used by the main thread, > and only when the sink is linked. pa_sink.asyncmsgq always points to > pa_thread_mq's inq. pa_thread_mq.inq is created in pa_thread_mq_init() > and unreffed in pa_thread_mq_done(). pa_thread_mq_done() is only called > after the "hardware sink" (the one implementing the IO thread) has been > unlinked. Therefore, at least the hardware sink is fine without any This was exactly the condition that was not met during my testing with the bluetooth modules. As checked in patch v0 11/16, a potential error in start_thread() could lead to this inconsistent state where the sinks/sources were not unlinked (until I realized patch v0 07/16 was needed). This was obviously a bug, but such reference counting would have come handy to debug the problem. > extra pa_asyncmsgq_ref() calls. What about filter sinks then? Can they > be linked while the hardware sink is not, therefore maybe allowing the > filter sink's asyncmsgq to be used while the underlying pa_thread_mq is > gone? No. The filter sinks are unlinked at the same time when the > hardware sink is unlinked. So, pa_sink.asyncmsgq will always have a > reference when it's needed. No need to call pa_asyncmsgq_ref() in > pa_sink_set_asyncmsgq(). What about checking the reference counter pa_thread_mq_done()? If I got this right, we would expect that the reference count is 1 at that point, so we could add an assertion. If there is some sink/source that hasn't been unlinked, the assertion would fail. Would this check be valid for the rest of the code using pa_asyncmsgq_ref()? Cheers, Mikel
