Re: [PATCH 26/29] ALSA: oxfw: Add support AMDTP in-stream

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

On Nov 15 2014 21:04, Clemens Ladisch wrote:
> Takashi Sakamoto wrote:
>> If I rewrite your pseudo code, like this:
>>
>>>    void start_some_substream(...)
>>>    {
>>>        if (atomic_inc_and_test(substreams)) {
>>>            mutex_lock();
>>>            // lots of stuff to start DMA ...
>>
>>              if (wait_for_first_packet() == ETIMEDOUT) {
>>                  stop_dma();
>>                  release_streams();
>>                  mutex_unlock();
>>                  return ETIMEDOUT;
>>              }
>>>            mutex_unlock();
>>>        }
>>>        ...
>>>    }
> 
> This is an even better example of why atomic_t is incorrect:  when the
> startup fails, the second call to start_some_substream() at the same
> time, which just sees that substreams>0, cannot notice the error, and
> thinks its own substream has been started successfully.

Yes. The reference counter is invalid in this pseudo code which I
showrf. I left your insistence about 'atomic_inc_and_test()' what it is.

More precisely, current implementations (bebob/fireworks/dice) are (or
should be for oxfw) like:

void start_some_substream(...)
{
    mutex_lock();

    if (atomic_read(substreams) == 0)
        goto end;

    if (streams_are_running())
        goto end;

    if (!start_isochronous_context_and_dma()) {
        release_isochronous_context_and_dma();
        release_streams();
        goto end;
    }

    if (wait_for_first_packet() == ETIMEDOUT) {
        release_isochronous_context_and_dma();
        release_streams();
    }
end:
    mutex_unlock();
    return err;
}

I believe that the reference counter with atomic_t is valid regardless
of inner/outer the lock (mutex). For this critical section, it's just a
importance that some substreams already subscribe the stream or not,
instead of the number of substreams.

If the reference counter is non-zero, it means that 'a stream is
requested to run for some substream, even if the substreams are not for
current caller'.

I don't show in the pseudo code; when the streams are already running
but the sampling transfer frequency is not the same as the one which the
caller requested, then the streams are stopped once and set sampling
transfer frequency and restart them.


Regards

Takashi Sakamoto
o-takashi@sakamocchi.jp