Re: [alsa-devel] [PATCH 26/29] ALSA: oxfw: Add support AMDTP in-stream
Takashi Sakamoto wrote:
On Nov 24 2014 22:54, Clemens Ladisch wrote:
In theory, it would be possible to use atomic_t to synchronize multiple streams, but this requires that
- two CPUs that start a stream at the same time do not both think they are the first, ... and
- when one CPU is still starting a stream, code on all other CPUs must be able to work correctly, without synchronization. [...]
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.
Regards, Clemens
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
participants (2)
-
Clemens Ladisch
-
Takashi Sakamoto