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

[Clemens Ladisch]

Takashi Sakamoto wrote:
> On Nov 17 2014 06:21, Clemens Ladisch wrote:
>> And why are the substreams counters atomic?
>> Can't they just be normal variables accessed from inside the mutex?
>
> Just for my convinience. I didn't want to write many
> mutex_lock()/mutex_unlock() because wrong coding of them causes-dead
> lock

Just use a kernel compiled with CONFIG_PROVE_LOCKING.

> An attached patch achieves your idea over my patchset. Handling
> reference counter is easy to understand (because it's arithmetric
> operation) but I don't like much mutex_lock()/mutex_unlock().
>
> Can I request you to explain about the advantages of your idea?

It makes the driver work correctly.

There's nothing wrong with atomic_t itself, but in this case, none of
the accesses would be valid outside of the lock, so it is not necessary
for them to be atomic.

In theory, it would be possible to use atomic_t to synchronize multiple
streams, but this requires that
1. two CPUs that start a stream at the same time do not both think they
   are the first, so you *must* use something like atomic_inc_and_test(),
   and
2. when one CPU is still starting a stream, code on all other CPUs must
   be able to work correctly, without synchronization.  This is not
   possible if the code requires to know that the DMA actually has
   started; consider the following code:

   void start_some_substream(...)
   {
       if (atomic_inc_and_test(substreams)) {
           mutex_lock();
           // lots of stuff to start DMA ...
           mutex_unlock();
       } else {
           // wait for the DMA to be started
       }
       ...
   }

   How could the 'else' branch ensure that the DMA engine is started?
   Just taking the mutex is not enough, because it could happen before
   the mutex_lock() of the first stream.  This is not possible without
   moving the inc_and_test into the locked region.


Regards,
Clemens