Why doesn't mixer control (values) have some kind of locking mechanism? (mutex?)

Why doesn't mixer control (values) have some kind of locking mechanism? (mutex?)

[Tom Yan]

Hi all,

I just wonder if it's a "no one cares" or a "no one was aware of it"
issue (or maybe both?).

When you change (integer) values (e.g. volume) of a mixer control, it
usually (if not always) involves calling two functions/methods of a
snd_kcontrol_new, which are get and put, in order to do relative
volume adjustments. (Apparently it is often done relatively even if we
have absolute values, for reasons.)

While these two "actions" can be and probably are mostly "atomic"
(with the help of mutex) in the kernel drivers *respectively*, they
are not and cannot be atomic as a whole.

This won't really be an issue when the actions (either for one or
multiple channels) are done "synchronously" in *one* program run (e.g.
amixer -c STX set Master 1+). However, if such a program run is issued
multiple times "asynchronously" (e.g. binding it to some
XF86Audio{Raise,Lower}Volume scroll wheel), volume adjustment becomes
a total mess / failure.

If it isn't obvious enough. it could happen like the following:
get1(100 100)
set1(101 100)
get2(101 100)
set2(102 100)
...

Or worse:
get1(100 100)
get2(100 100)
set1(101 100)
set2(100 101)
...

Not only that it may/will not finish the first set of adjustments for
all channels before the second, get() from the second set could happen
before set() from the first, reverting the effect of the earlier
one(s).

Certainly one can use something like `flock` with amixer to make sure
the atomicity of each issue/run, but not only that it looks silly and
primitive, we don't always manipulate the mixer control with an
"executable". For example, this weird issue in pulseaudio is probably
related: https://bugs.freedesktop.org/show_bug.cgi?id=92717

So I wonder, is there a particular reason that mixer control doesn't
possess some form of lock, which allows any form of userspace
manipulation to lock it until what should be / is considered atomic is
finished?

Re: Why doesn't mixer control (values) have some kind of locking mechanism? (mutex?)

[Pierre-Louis Bossart]

[Adding Mark, Takashi and Jaroslav in CC: to make sure they see this thread]

On 8/5/20 12:31 PM, Tom Yan wrote:
> Hi all,
> 
> I just wonder if it's a "no one cares" or a "no one was aware of it"
> issue (or maybe both?).

none of the above, see below

> When you change (integer) values (e.g. volume) of a mixer control, it
> usually (if not always) involves calling two functions/methods of a
> snd_kcontrol_new, which are get and put, in order to do relative
> volume adjustments. (Apparently it is often done relatively even if we
> have absolute values, for reasons.)
> 
> While these two "actions" can be and probably are mostly "atomic"
> (with the help of mutex) in the kernel drivers *respectively*, they
> are not and cannot be atomic as a whole.
> 
> This won't really be an issue when the actions (either for one or
> multiple channels) are done "synchronously" in *one* program run (e.g.
> amixer -c STX set Master 1+). However, if such a program run is issued
> multiple times "asynchronously" (e.g. binding it to some
> XF86Audio{Raise,Lower}Volume scroll wheel), volume adjustment becomes
> a total mess / failure.
> 
> If it isn't obvious enough. it could happen like the following:
> get1(100 100)
> set1(101 100)
> get2(101 100)
> set2(102 100)
> ...
> 
> Or worse:
> get1(100 100)
> get2(100 100)
> set1(101 100)
> set2(100 101)
> ...
> 
> Not only that it may/will not finish the first set of adjustments for
> all channels before the second, get() from the second set could happen
> before set() from the first, reverting the effect of the earlier
> one(s).
> 
> Certainly one can use something like `flock` with amixer to make sure
> the atomicity of each issue/run, but not only that it looks silly and
> primitive, we don't always manipulate the mixer control with an
> "executable". For example, this weird issue in pulseaudio is probably
> related: https://bugs.freedesktop.org/show_bug.cgi?id=92717
> 
> So I wonder, is there a particular reason that mixer control doesn't
> possess some form of lock, which allows any form of userspace
> manipulation to lock it until what should be / is considered atomic is
> finished?

In the past on the Intel side we had a need for a control 'commit' 
operation, where a set of control values where passed all the way to DSP 
firmware, but were applied at the same time to avoid transients and 
inconsistent states. This was typically required when changing the 
orientation or changing the routing. We solved the problem by having a 
dedicated 'commit' control which gated all others, but there was no 
framework support for this.

These 'commit' operations are quite common in a number of 
specifications, e.g. OpenSL ES, and required when you update filter 
coefficients through independent transactions.

SoundWire 1.2 added support for dual-ranked registers which contains the 
'Current' and 'Next' value, and a commit mechanism to flip the two in 
synchronized ways. This will likely trickle in other hardware standards, 
and indeed I wonder if we should update the control/mixer framework - 
and possibly regmap as well?

It'd be a rather large plumbing exercise but it's worth it IMHO.
-Pierre

Re: Why doesn't mixer control (values) have some kind of locking mechanism? (mutex?)

[Takashi Sakamoto]

Hi,

On Thu, Aug 06, 2020 at 01:31:03AM +0800, Tom Yan wrote:
> Hi all,
> 
> I just wonder if it's a "no one cares" or a "no one was aware of it"
> issue (or maybe both?).
> 
> When you change (integer) values (e.g. volume) of a mixer control, it
> usually (if not always) involves calling two functions/methods of a
> snd_kcontrol_new, which are get and put, in order to do relative
> volume adjustments. (Apparently it is often done relatively even if we
> have absolute values, for reasons.)
> 
> While these two "actions" can be and probably are mostly "atomic"
> (with the help of mutex) in the kernel drivers *respectively*, they
> are not and cannot be atomic as a whole.
> 
> This won't really be an issue when the actions (either for one or
> multiple channels) are done "synchronously" in *one* program run (e.g.
> amixer -c STX set Master 1+). However, if such a program run is issued
> multiple times "asynchronously" (e.g. binding it to some
> XF86Audio{Raise,Lower}Volume scroll wheel), volume adjustment becomes
> a total mess / failure.
> 
> If it isn't obvious enough. it could happen like the following:
> get1(100 100)
> set1(101 100)
> get2(101 100)
> set2(102 100)
> ...
> 
> Or worse:
> get1(100 100)
> get2(100 100)
> set1(101 100)
> set2(100 101)
> ...
> 
> Not only that it may/will not finish the first set of adjustments for
> all channels before the second, get() from the second set could happen
> before set() from the first, reverting the effect of the earlier
> one(s).
> 
> Certainly one can use something like `flock` with amixer to make sure
> the atomicity of each issue/run, but not only that it looks silly and
> primitive, we don't always manipulate the mixer control with an
> "executable". For example, this weird issue in pulseaudio is probably
> related: https://bugs.freedesktop.org/show_bug.cgi?id=92717
> 
> So I wonder, is there a particular reason that mixer control doesn't
> possess some form of lock, which allows any form of userspace
> manipulation to lock it until what should be / is considered atomic is
> finished?

ALSA control core allows applications to lock/unlock a control element
so that any write opreation to the control element fails for processes
except for owner process.

When a process requests `SNDRV_CTL_IOCTL_ELEM_LOCK`[1] against a
control element. After operating the request, the control element is
under 'owned by the process' state. In this state, any request of
`SNDRV_CTL_IOCTL_ELEM_WRITE` from the other processes fails with
`-EPERM`[2]. The write operation from the owner process is successful
only. When the owner process is going to finish, the state is
released[3].

ALSA userspace library, a.k.a alsa-lib, has a pair of
`snd_ctl_elem_lock()` and `snd_ctl_elem_unlock()` as its exported
API[4].

If application developers would like to bring failure to
requests of `SNDRV_CTL_IOCTL_ELEM_WRITE` from the other processes in
the period that the process requests `SNDRV_CTL_IOCTL_ELEM_READ` and
`SNDRV_CTL_IOCTL_ELEM_WRITE` as a transaction, the lock/unlock
mechanism is available. However, as long as I know, it's not used
popularly.


This is a simple demonstration about the above mechanism. PyGObject and
alsa-gobject[5] is required to install:

```
#!/usr/bin/env python3

import gi
gi.require_version('ALSACtl', '0.0')
from gi.repository import ALSACtl

import subprocess

def run_amixer(should_err):
  cmd = ('amixer', '-c', str(card_id),
    'cset',
    'iface={},name="{}",index={},device={},subdevice={},numid={}'.format(
        eid.get_iface().value_nick, eid.get_name(),
        eid.get_index(), eid.get_device_id(),
        eid.get_subdevice_id(), eid.get_numid()),
    '0,0',
  )

  result = subprocess.run(cmd, capture_output=True)
  if result.stderr:
    err = result.stderr.decode('UTF-8').rstrip()
    print(' ', 'expected' if should_err else 'unexpected')
    print('   ', err)
  if result.stdout:
    output = result.stdout.decode('UTF-8').rstrip().split('\n')
    print(' ', 'expected' if not should_err else 'unexpected')
    print('   ', output[-2])

card_id = 0
card = ALSACtl.Card.new()
card.open(card_id, 0)

for eid in card.get_elem_id_list():
  prev_info = card.get_elem_info(eid)
  if (prev_info.get_property('type') != ALSACtl.ElemType.INTEGER or
      'write' not in prev_info.get_property('access').value_nicks or
      'lock' in prev_info.get_property('access').value_nicks):
      continue

  card.lock_elem(eid, True)
  print('  my program locks: "{}"'.format(eid.get_name()))
  run_amixer_subprocess(True)

  card.lock_elem(eid, False)
  print('  my program unlocks: "{}"'.format(eid.get_name()))
  run_amixer_subprocess(False)
```

You can see the result of amixer execution is different in the cases of
locked and unlocked, like:

```
$ /tmp/lock-demo
  ...
  my program locks: "Headphone Playback Volume"
  expected
    amixer: Control hw:1 element write error: Operation not permitted
  my program unlocks: "Headphone Playback Volume"
  expected
      : values=0,0
  ...
```


[1] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/asound.h#n1083
[2] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/sound/core/control.c#n1108
[3] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/sound/core/control.c#n122
[4] https://www.alsa-project.org/alsa-doc/alsa-lib/group___control.html#ga1fba1f7e08ab11505a617af5d54f4580
[5] https://github.com/alsa-project/alsa-gobject


Regards

Takashi Sakamoto

Re: Why doesn't mixer control (values) have some kind of locking mechanism? (mutex?)

[Tom Yan]

Hi,

On Thu, 6 Aug 2020 at 10:06, Takashi Sakamoto <o-takashi@sakamocchi.jp> wrote:
>
> Hi,
>
> ALSA control core allows applications to lock/unlock a control element
> so that any write opreation to the control element fails for processes
> except for owner process.
>
> When a process requests `SNDRV_CTL_IOCTL_ELEM_LOCK`[1] against a
> control element. After operating the request, the control element is
> under 'owned by the process' state. In this state, any request of
> `SNDRV_CTL_IOCTL_ELEM_WRITE` from the other processes fails with
> `-EPERM`[2]. The write operation from the owner process is successful
> only. When the owner process is going to finish, the state is
> released[3].

That doesn't really address the problem anyway. As I've mentioned,
implementation of volume put() in kernel drivers often / can easily be
made atomic anyway (that might be the reason why this write lock isn't
popular). The problem/race I am trying to point out is, one process
can get()/read before another finishing its get()+put() pair (which is
required by volume setting/adjusting), so something like
get1()->get2()->put1()->put2() could easily happen (where each put()
relies on / is "configured" with volumes of their respective get()).
The lock will need to intentionally stall further get()/read as well.

If we for some reason want to avoid using locks, put() needs to be
atomic by design (like, "embed" get() in itself and use arrays for
volume values, instead of requiring those to be implemented in the
userspace manually / with a loop). Unfortunately that isn't the case
in ALSA.

>
> ALSA userspace library, a.k.a alsa-lib, has a pair of
> `snd_ctl_elem_lock()` and `snd_ctl_elem_unlock()` as its exported
> API[4].
>
> If application developers would like to bring failure to
> requests of `SNDRV_CTL_IOCTL_ELEM_WRITE` from the other processes in
> the period that the process requests `SNDRV_CTL_IOCTL_ELEM_READ` and
> `SNDRV_CTL_IOCTL_ELEM_WRITE` as a transaction, the lock/unlock
> mechanism is available. However, as long as I know, it's not used
> popularly.
>
>
> This is a simple demonstration about the above mechanism. PyGObject and
> alsa-gobject[5] is required to install:
>
> ```
> #!/usr/bin/env python3
>
> import gi
> gi.require_version('ALSACtl', '0.0')
> from gi.repository import ALSACtl
>
> import subprocess
>
> def run_amixer(should_err):
>   cmd = ('amixer', '-c', str(card_id),
>     'cset',
>     'iface={},name="{}",index={},device={},subdevice={},numid={}'.format(
>         eid.get_iface().value_nick, eid.get_name(),
>         eid.get_index(), eid.get_device_id(),
>         eid.get_subdevice_id(), eid.get_numid()),
>     '0,0',
>   )
>
>   result = subprocess.run(cmd, capture_output=True)
>   if result.stderr:
>     err = result.stderr.decode('UTF-8').rstrip()
>     print(' ', 'expected' if should_err else 'unexpected')
>     print('   ', err)
>   if result.stdout:
>     output = result.stdout.decode('UTF-8').rstrip().split('\n')
>     print(' ', 'expected' if not should_err else 'unexpected')
>     print('   ', output[-2])
>
> card_id = 0
> card = ALSACtl.Card.new()
> card.open(card_id, 0)
>
> for eid in card.get_elem_id_list():
>   prev_info = card.get_elem_info(eid)
>   if (prev_info.get_property('type') != ALSACtl.ElemType.INTEGER or
>       'write' not in prev_info.get_property('access').value_nicks or
>       'lock' in prev_info.get_property('access').value_nicks):
>       continue
>
>   card.lock_elem(eid, True)
>   print('  my program locks: "{}"'.format(eid.get_name()))
>   run_amixer_subprocess(True)
>
>   card.lock_elem(eid, False)
>   print('  my program unlocks: "{}"'.format(eid.get_name()))
>   run_amixer_subprocess(False)
> ```
>
> You can see the result of amixer execution is different in the cases of
> locked and unlocked, like:
>
> ```
> $ /tmp/lock-demo
>   ...
>   my program locks: "Headphone Playback Volume"
>   expected
>     amixer: Control hw:1 element write error: Operation not permitted
>   my program unlocks: "Headphone Playback Volume"
>   expected
>       : values=0,0
>   ...
> ```
>
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/include/uapi/sound/asound.h#n1083
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/sound/core/control.c#n1108
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/sound/core/control.c#n122
> [4] https://www.alsa-project.org/alsa-doc/alsa-lib/group___control.html#ga1fba1f7e08ab11505a617af5d54f4580
> [5] https://github.com/alsa-project/alsa-gobject
>
>
> Regards
>
> Takashi Sakamoto

Re: Why doesn't mixer control (values) have some kind of locking mechanism? (mutex?)

[Takashi Sakamoto]

Hi,

On Thu, Aug 06, 2020 at 04:57:02PM +0800, Tom Yan wrote:
> The problem/race I am trying to point out is, one process can
> get()/read before another finishing its get()+put() pair (which is
> required by volume setting/adjusting), so something like
> get1()->get2()->put1()->put2() could easily happen (where each put()
> relies on / is "configured" with volumes of their respective get()).
> The lock will need to intentionally stall further get()/read as well.

In my opinion, in the above case, it's possible to serialize each
transaction which consists of get/put (read/write in userspace
application) with lock/unlock mechanism.

+-----------+-----------+
| process A | process B |
+-----------+-----------+
|   lock    |           |
|   get     |           |
|           |lock(EPERM)| reschedule lock/get/set/unlock actions
|   set     |           |
|           |lock(EPERM)| reschedule lock/get/set/unlock actions
|  unlock   |           |
|           |   lock    |
|           |   get     |
|           |   set     |
|           |  unlock   |
+-----------+-----------+

(Of course, the above is achieved when the series of operations is done
by userspace applications. For simplicity, I don't mention about
in-kernel initiators of the get/set actions. In this point, I don't
address to the message Pierre posted.)

> If we for some reason want to avoid using locks, put() needs to be
> atomic by design (like, "embed" get() in itself and use arrays for
> volume values, instead of requiring those to be implemented in the
> userspace manually / with a loop). Unfortunately that isn't the case
> in ALSA.
 
I get your intension is something like compare-and-swap[1]. At present,
ALSA control core has no functionality like it, but it's worth to
investigate. The ioctl request should includes a pair of 'struct
snd_ctl_elem_value' in its argument. In a design of ALSA control
core, the pair should be processed in each driver since ALSA control
core has no 'cache' of the content of 'struct snd_ctl_elem_value' except
for user-defined control element set.

Here, would I ask your opinion to the lock/get/set/unlock actions
from userspace? It can really not be one of solution for the issue?

[1] https://en.wikipedia.org/wiki/Compare-and-swap


Regards

Takashi Sakamoto

Re: Why doesn't mixer control (values) have some kind of locking mechanism? (mutex?)

[Tom Yan]

Yeah I suppose a "full" lock would do. (That was what I was trying to
point out. I don't really understand Pierre's message. I merely
suppose you need some facility in the kernel anyway so that you can
lock from userspace.) I hope that amixer the utility will at least
have the capability to reschedule/wait by then though (instead of just
"failing" like in your python demo).

As for the compare-and-swap part, it's just a plus. Not that
"double-looping" for *each* channel doesn't work. It just again seems
silly and primitive (and was once confusing to me).

On Thu, 6 Aug 2020 at 17:15, Takashi Sakamoto <o-takashi@sakamocchi.jp> wrote:
>
> Hi,
>
> On Thu, Aug 06, 2020 at 04:57:02PM +0800, Tom Yan wrote:
> > The problem/race I am trying to point out is, one process can
> > get()/read before another finishing its get()+put() pair (which is
> > required by volume setting/adjusting), so something like
> > get1()->get2()->put1()->put2() could easily happen (where each put()
> > relies on / is "configured" with volumes of their respective get()).
> > The lock will need to intentionally stall further get()/read as well.
>
> In my opinion, in the above case, it's possible to serialize each
> transaction which consists of get/put (read/write in userspace
> application) with lock/unlock mechanism.
>
> +-----------+-----------+
> | process A | process B |
> +-----------+-----------+
> |   lock    |           |
> |   get     |           |
> |           |lock(EPERM)| reschedule lock/get/set/unlock actions
> |   set     |           |
> |           |lock(EPERM)| reschedule lock/get/set/unlock actions
> |  unlock   |           |
> |           |   lock    |
> |           |   get     |
> |           |   set     |
> |           |  unlock   |
> +-----------+-----------+
>
> (Of course, the above is achieved when the series of operations is done
> by userspace applications. For simplicity, I don't mention about
> in-kernel initiators of the get/set actions. In this point, I don't
> address to the message Pierre posted.)
>
> > If we for some reason want to avoid using locks, put() needs to be
> > atomic by design (like, "embed" get() in itself and use arrays for
> > volume values, instead of requiring those to be implemented in the
> > userspace manually / with a loop). Unfortunately that isn't the case
> > in ALSA.
>
> I get your intension is something like compare-and-swap[1]. At present,
> ALSA control core has no functionality like it, but it's worth to
> investigate. The ioctl request should includes a pair of 'struct
> snd_ctl_elem_value' in its argument. In a design of ALSA control
> core, the pair should be processed in each driver since ALSA control
> core has no 'cache' of the content of 'struct snd_ctl_elem_value' except
> for user-defined control element set.
>
> Here, would I ask your opinion to the lock/get/set/unlock actions
> from userspace? It can really not be one of solution for the issue?
>
> [1] https://en.wikipedia.org/wiki/Compare-and-swap
>
>
> Regards
>
> Takashi Sakamoto

Re: Why doesn't mixer control (values) have some kind of locking mechanism? (mutex?)

[Takashi Sakamoto]

Hi,

On Thu, Aug 06, 2020 at 08:31:51PM +0800, Tom Yan wrote:
> Yeah I suppose a "full" lock would do. (That was what I was trying to
> point out. I don't really understand Pierre's message. I merely
> suppose you need some facility in the kernel anyway so that you can
> lock from userspace.) I hope that amixer the utility will at least have
> the capability to reschedule/wait by then though (instead of just
> "failing" like in your python demo).

As long as I know, neither tools in alsa-utils/alsa-tools nor pulseaudio
use the lock mechanism. In short, you are the first person to address
to the issue. Thanks for your patience since the first post in 2015.

> As for the compare-and-swap part, it's just a plus. Not that
> "double-looping" for *each* channel doesn't work. It just again seems
> silly and primitive (and was once confusing to me).

I prepare a rough kernel patch abount the compare-and-swap idea for
our discussion. The compare-and-swap is done under lock acquisition of
'struct snd_card.controls_rwsem', therefore many types of operations
to control element (e.g. read as well) get affects. This idea works
well at first when alsa-lib supports corresponding API and userspace
applications uses it. Therefore we need more work than changing just
in userspace.

But in my opinion, if things can be solved just in userspace, it should
be done with no change in kernel space.

======== 8< --------

From 54832d11b9056da2883d6edfdccaab76d8b08a5c Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Date: Thu, 6 Aug 2020 19:34:55 +0900
Subject: [PATCH] ALSA: control: add new ioctl request for compare_and_swap
 operation to element value

This is a rough implementation as a solution for an issue below. This is
not tested yet. The aim of this patch is for further discussion.

Typical userspace applications decide write value to control element
according to value read preliminarily. In the case, if multiple
applications begin a pair of read and write operations simultaneously,
the result is not deterministic without any lock mechanism. Although
ALSA control core has lock/unlock mechanism to a control element for
the case, the mechanism is not so popular. The mechanism neither not
used by tools in alsa-utils, alsa-tools, nor PulseAudio, at least.

This commit is an attempt to solve the case by introducing new ioctl
request. The request is a part of 'compare and swap' mechanism. The
applications should pass ioctl argument with a pair of old and new value
of the control element. ALSA control core read current value and compare
it to the old value under acquisition of lock. If they are the same,
the new value is going to be written at last.

Reported-by: Tom Yan <tom.ty89@gmail.com>
Reference: https://lore.kernel.org/alsa-devel/CAGnHSEkV9cpWoQKP1mT7RyqyTvGrZu045k=3W45Jm=mBidqDnw@mail.gmail.com/T/
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 include/uapi/sound/asound.h |  6 ++++
 sound/core/control.c        | 56 +++++++++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 535a7229e1d9..ff8d5416458d 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -1074,6 +1074,11 @@ struct snd_ctl_tlv {
 	unsigned int tlv[0];	/* first TLV */
 };
 
+struct snd_ctl_elem_compare_and_swap {
+	struct snd_ctl_elem_value old;
+	struct snd_ctl_elem_value new;
+};
+
 #define SNDRV_CTL_IOCTL_PVERSION	_IOR('U', 0x00, int)
 #define SNDRV_CTL_IOCTL_CARD_INFO	_IOR('U', 0x01, struct snd_ctl_card_info)
 #define SNDRV_CTL_IOCTL_ELEM_LIST	_IOWR('U', 0x10, struct snd_ctl_elem_list)
@@ -1089,6 +1094,7 @@ struct snd_ctl_tlv {
 #define SNDRV_CTL_IOCTL_TLV_READ	_IOWR('U', 0x1a, struct snd_ctl_tlv)
 #define SNDRV_CTL_IOCTL_TLV_WRITE	_IOWR('U', 0x1b, struct snd_ctl_tlv)
 #define SNDRV_CTL_IOCTL_TLV_COMMAND	_IOWR('U', 0x1c, struct snd_ctl_tlv)
+#define SNDRV_CTL_IOCTL_ELEM_COPARE_AND_SWAP	_IOWR('U', 0x1d, struct snd_ctl_elem_compare_and_swap)
 #define SNDRV_CTL_IOCTL_HWDEP_NEXT_DEVICE _IOWR('U', 0x20, int)
 #define SNDRV_CTL_IOCTL_HWDEP_INFO	_IOR('U', 0x21, struct snd_hwdep_info)
 #define SNDRV_CTL_IOCTL_PCM_NEXT_DEVICE	_IOR('U', 0x30, int)
diff --git a/sound/core/control.c b/sound/core/control.c
index aa0c0cf182af..0ac1f7c489be 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1684,6 +1684,60 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
 	return -ENXIO;
 }
 
+static int snd_ctl_elem_compare_and_swap(struct snd_ctl_file *ctl_file,
+					 struct snd_ctl_elem_compare_and_swap *cas)
+{
+	struct snd_card *card = ctl_file->card;
+	// TODO: too much use on kernel stack...
+	struct snd_ctl_elem_value curr;
+	struct snd_ctl_elem_info info;
+	unsigned int unit_size;
+	int err;
+
+	info.id = cas->old.id;
+	err = snd_ctl_elem_info(ctl_file, &info);
+	if (err < 0)
+		return err;
+	if (info.type < SNDRV_CTL_ELEM_TYPE_BOOLEAN || info.type > SNDRV_CTL_ELEM_TYPE_INTEGER64)
+		return -ENXIO;
+	unit_size = value_sizes[info.type];
+
+	curr.id = cas->old.id;
+	err = snd_ctl_elem_read(card, &curr);
+	if (err < 0)
+		return err;
+
+	// Compare.
+	if (memcmp(&cas->old.value, &curr.value, unit_size * info.count) != 0)
+		return -EAGAIN;
+
+	// Swap.
+	return snd_ctl_elem_write(card, ctl_file, &cas->new);
+}
+
+static int snd_ctl_elem_compare_and_swap_user(struct snd_ctl_file *ctl_file,
+					      struct snd_ctl_elem_compare_and_swap __user *argp)
+{
+	struct snd_card *card = ctl_file->card;
+	struct snd_ctl_elem_compare_and_swap *cas;
+	int err;
+
+	cas = memdup_user(argp, sizeof(*cas));
+	if (IS_ERR(cas))
+		return PTR_ERR(cas);
+
+	err = snd_power_wait(card, SNDRV_CTL_POWER_D0);
+	if (err < 0)
+		goto end;
+
+	down_read(&card->controls_rwsem);
+	err = snd_ctl_elem_compare_and_swap(ctl_file, cas);
+	up_read(&card->controls_rwsem);
+end:
+	kfree(cas);
+	return err;
+}
+
 static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	struct snd_ctl_file *ctl;
@@ -1737,6 +1791,8 @@ static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg
 		err = snd_ctl_tlv_ioctl(ctl, argp, SNDRV_CTL_TLV_OP_CMD);
 		up_write(&ctl->card->controls_rwsem);
 		return err;
+	case SNDRV_CTL_IOCTL_ELEM_COPARE_AND_SWAP:
+		return snd_ctl_elem_compare_and_swap_user(ctl, argp);
 	case SNDRV_CTL_IOCTL_POWER:
 		return -ENOPROTOOPT;
 	case SNDRV_CTL_IOCTL_POWER_STATE:
-- 
2.25.1

======== 8< --------

Thanks

Takashi Sakamoto

Re: Why doesn't mixer control (values) have some kind of locking mechanism? (mutex?)

[Tom Yan]

Hi,

On Thu, 6 Aug 2020 at 22:47, Takashi Sakamoto <o-takashi@sakamocchi.jp> wrote:
>
> Hi,
>
> As long as I know, neither tools in alsa-utils/alsa-tools nor pulseaudio
> use the lock mechanism. In short, you are the first person to address
> to the issue. Thanks for your patience since the first post in 2015.
>
> > As for the compare-and-swap part, it's just a plus. Not that
> > "double-looping" for *each* channel doesn't work. It just again seems
> > silly and primitive (and was once confusing to me).
>
> I prepare a rough kernel patch abount the compare-and-swap idea for
> our discussion. The compare-and-swap is done under lock acquisition of
> 'struct snd_card.controls_rwsem', therefore many types of operations
> to control element (e.g. read as well) get affects. This idea works
> well at first when alsa-lib supports corresponding API and userspace
> applications uses it. Therefore we need more work than changing just
> in userspace.
>
> But in my opinion, if things can be solved just in userspace, it should
> be done with no change in kernel space.

I didn't know much about programming or so back then (even by now I
know only a little) when I first noticed the problem, so I just
avoided using amixer / my volume wheel / parts of pulse and resorted
to alsamixer. For some reason the race didn't *appear to* exist with
onboard or USB audio but only my Xonar STX (maybe because volume
adjustments take a bit more time with it), so for a long time I
thought it's some delicate bug in the oxygen/xonar driver that I
failed to identify. Only until very recently it gets back to my head
and becomes clear to me that it's a general design flaw in ALSA.

Just to confirm, does SNDRV_CTL_IOCTL_ELEM_LOCK currently prevent
get()/read? Or is it just a write lock as I thought? If that's the
case, and if the compare-and-swap approach doesn't involve a lot of
changes (in all the kernel drivers, for example), I'd say we better
start moving to something neat than using something which is less so
and wasn't really ever adopted anyway.

>
> ======== 8< --------
>
> From 54832d11b9056da2883d6edfdccaab76d8b08a5c Mon Sep 17 00:00:00 2001
> From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Date: Thu, 6 Aug 2020 19:34:55 +0900
> Subject: [PATCH] ALSA: control: add new ioctl request for compare_and_swap
>  operation to element value
>
> This is a rough implementation as a solution for an issue below. This is
> not tested yet. The aim of this patch is for further discussion.
>
> Typical userspace applications decide write value to control element
> according to value read preliminarily. In the case, if multiple
> applications begin a pair of read and write operations simultaneously,
> the result is not deterministic without any lock mechanism. Although
> ALSA control core has lock/unlock mechanism to a control element for
> the case, the mechanism is not so popular. The mechanism neither not
> used by tools in alsa-utils, alsa-tools, nor PulseAudio, at least.
>
> This commit is an attempt to solve the case by introducing new ioctl
> request. The request is a part of 'compare and swap' mechanism. The
> applications should pass ioctl argument with a pair of old and new value
> of the control element. ALSA control core read current value and compare
> it to the old value under acquisition of lock. If they are the same,
> the new value is going to be written at last.
>
> Reported-by: Tom Yan <tom.ty89@gmail.com>
> Reference: https://lore.kernel.org/alsa-devel/CAGnHSEkV9cpWoQKP1mT7RyqyTvGrZu045k=3W45Jm=mBidqDnw@mail.gmail.com/T/
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---
>  include/uapi/sound/asound.h |  6 ++++
>  sound/core/control.c        | 56 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
>
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 535a7229e1d9..ff8d5416458d 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -1074,6 +1074,11 @@ struct snd_ctl_tlv {
>         unsigned int tlv[0];    /* first TLV */
>  };
>
> +struct snd_ctl_elem_compare_and_swap {
> +       struct snd_ctl_elem_value old;
> +       struct snd_ctl_elem_value new;
> +};
> +
>  #define SNDRV_CTL_IOCTL_PVERSION       _IOR('U', 0x00, int)
>  #define SNDRV_CTL_IOCTL_CARD_INFO      _IOR('U', 0x01, struct snd_ctl_card_info)
>  #define SNDRV_CTL_IOCTL_ELEM_LIST      _IOWR('U', 0x10, struct snd_ctl_elem_list)
> @@ -1089,6 +1094,7 @@ struct snd_ctl_tlv {
>  #define SNDRV_CTL_IOCTL_TLV_READ       _IOWR('U', 0x1a, struct snd_ctl_tlv)
>  #define SNDRV_CTL_IOCTL_TLV_WRITE      _IOWR('U', 0x1b, struct snd_ctl_tlv)
>  #define SNDRV_CTL_IOCTL_TLV_COMMAND    _IOWR('U', 0x1c, struct snd_ctl_tlv)
> +#define SNDRV_CTL_IOCTL_ELEM_COPARE_AND_SWAP   _IOWR('U', 0x1d, struct snd_ctl_elem_compare_and_swap)
>  #define SNDRV_CTL_IOCTL_HWDEP_NEXT_DEVICE _IOWR('U', 0x20, int)
>  #define SNDRV_CTL_IOCTL_HWDEP_INFO     _IOR('U', 0x21, struct snd_hwdep_info)
>  #define SNDRV_CTL_IOCTL_PCM_NEXT_DEVICE        _IOR('U', 0x30, int)
> diff --git a/sound/core/control.c b/sound/core/control.c
> index aa0c0cf182af..0ac1f7c489be 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -1684,6 +1684,60 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
>         return -ENXIO;
>  }
>
> +static int snd_ctl_elem_compare_and_swap(struct snd_ctl_file *ctl_file,
> +                                        struct snd_ctl_elem_compare_and_swap *cas)
> +{
> +       struct snd_card *card = ctl_file->card;
> +       // TODO: too much use on kernel stack...
> +       struct snd_ctl_elem_value curr;
> +       struct snd_ctl_elem_info info;
> +       unsigned int unit_size;
> +       int err;
> +
> +       info.id = cas->old.id;
> +       err = snd_ctl_elem_info(ctl_file, &info);
> +       if (err < 0)
> +               return err;
> +       if (info.type < SNDRV_CTL_ELEM_TYPE_BOOLEAN || info.type > SNDRV_CTL_ELEM_TYPE_INTEGER64)
> +               return -ENXIO;
> +       unit_size = value_sizes[info.type];
> +
> +       curr.id = cas->old.id;
> +       err = snd_ctl_elem_read(card, &curr);
> +       if (err < 0)
> +               return err;
> +
> +       // Compare.
> +       if (memcmp(&cas->old.value, &curr.value, unit_size * info.count) != 0)
> +               return -EAGAIN;
> +
> +       // Swap.
> +       return snd_ctl_elem_write(card, ctl_file, &cas->new);
> +}
> +
> +static int snd_ctl_elem_compare_and_swap_user(struct snd_ctl_file *ctl_file,
> +                                             struct snd_ctl_elem_compare_and_swap __user *argp)
> +{
> +       struct snd_card *card = ctl_file->card;
> +       struct snd_ctl_elem_compare_and_swap *cas;
> +       int err;
> +
> +       cas = memdup_user(argp, sizeof(*cas));
> +       if (IS_ERR(cas))
> +               return PTR_ERR(cas);
> +
> +       err = snd_power_wait(card, SNDRV_CTL_POWER_D0);
> +       if (err < 0)
> +               goto end;
> +
> +       down_read(&card->controls_rwsem);
> +       err = snd_ctl_elem_compare_and_swap(ctl_file, cas);
> +       up_read(&card->controls_rwsem);
> +end:
> +       kfree(cas);
> +       return err;
> +}
> +
>  static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  {
>         struct snd_ctl_file *ctl;
> @@ -1737,6 +1791,8 @@ static long snd_ctl_ioctl(struct file *file, unsigned int cmd, unsigned long arg
>                 err = snd_ctl_tlv_ioctl(ctl, argp, SNDRV_CTL_TLV_OP_CMD);
>                 up_write(&ctl->card->controls_rwsem);
>                 return err;
> +       case SNDRV_CTL_IOCTL_ELEM_COPARE_AND_SWAP:
> +               return snd_ctl_elem_compare_and_swap_user(ctl, argp);
>         case SNDRV_CTL_IOCTL_POWER:
>                 return -ENOPROTOOPT;
>         case SNDRV_CTL_IOCTL_POWER_STATE:
> --
> 2.25.1
>
> ======== 8< --------
>
> Thanks
>
> Takashi Sakamoto

Re: Why doesn't mixer control (values) have some kind of locking mechanism? (mutex?)

[Takashi Sakamoto]

On Thu, Aug 06, 2020 at 11:34:12PM +0800, Tom Yan wrote:
> On Thu, 6 Aug 2020 at 22:47, Takashi Sakamoto <o-takashi@sakamocchi.jp> wrote:
> > As long as I know, neither tools in alsa-utils/alsa-tools nor pulseaudio
> > use the lock mechanism. In short, you are the first person to address
> > to the issue. Thanks for your patience since the first post in 2015.
> >
> > > As for the compare-and-swap part, it's just a plus. Not that
> > > "double-looping" for *each* channel doesn't work. It just again seems
> > > silly and primitive (and was once confusing to me).
> >
> > I prepare a rough kernel patch abount the compare-and-swap idea for
> > our discussion. The compare-and-swap is done under lock acquisition of
> > 'struct snd_card.controls_rwsem', therefore many types of operations
> > to control element (e.g. read as well) get affects. This idea works
> > well at first when alsa-lib supports corresponding API and userspace
> > applications uses it. Therefore we need more work than changing just
> > in userspace.
> >
> > But in my opinion, if things can be solved just in userspace, it should
> > be done with no change in kernel space.
> 
> I didn't know much about programming or so back then (even by now I
> know only a little) when I first noticed the problem, so I just
> avoided using amixer / my volume wheel / parts of pulse and resorted
> to alsamixer. For some reason the race didn't *appear to* exist with
> onboard or USB audio but only my Xonar STX (maybe because volume
> adjustments take a bit more time with it), so for a long time I
> thought it's some delicate bug in the oxygen/xonar driver that I
> failed to identify. Only until very recently it gets back to my head
> and becomes clear to me that it's a general design flaw in ALSA.
> 
> Just to confirm, does SNDRV_CTL_IOCTL_ELEM_LOCK currently prevent
> get()/read? Or is it just a write lock as I thought? If that's the
> case, and if the compare-and-swap approach doesn't involve a lot of
> changes (in all the kernel drivers, for example), I'd say we better
> start moving to something neat than using something which is less so
> and wasn't really ever adopted anyway.

In your case, SNDRV_CTL_IOCTL_ELEM_LOCK looks 'write-lock', therefore
any userspace applications can do read operation to the control element
locked by the other processes.

To solve the issue, the pair of read/write operations should be done
between lock/unlock operations. I can consider about two cases.

A case is that all of applications implements the above. This is
already proposed. The case is that the world is universe.

+-----------+-----------+
| process A | process B |
+-----------+-----------+
|   lock    |           |
|   read    |           |
|           |lock(EPERM)| reschedule lock/get/put/unlock actions
|   write   |           |
|           |lock(EPERM)| reschedule lock/get/put/unlock actions
|  unlock   |           |
|           |   lock    |
|           |   read    | calculate for new value
|           |   write   |
|           |  unlock   |
+-----------+-----------+

Another case is that a part of application implements the above. Let
me drill down the case into two cases. These cases are realistic
(sign...):

+-----------+------------+
| process A | process B  |
+-----------+------------+
|   lock    |            |
|   read    |            |
|   write   |            |
|           |    read    | calculate for new value 
|           |write(EPERM)|
|  unlock   |            |
|           |   write    | <- expected value
+-----------+------------+

+-----------+------------+
| process A | process B  |
+-----------+------------+
|   lock    |            |
|   read    |            |
|           |   read     | calculate for new value
|   write   |            |
|           |write(EPERM)|
|  unlock   |            |
|           |   write    | <- unexpected value
+-----------+------------+

The lock/unlock mechanism is not perfect solution as long as any
applications perform like the process.

If we can expect the most of applications to be back to read operation
at failure of write operation, thing goes well.

+-----------+------------+
| process A | process B  |
+-----------+------------+
|   lock    |            |
|   read    |            |
|           |   read     | calculate for new value
|   write   |            |
|           |write(EPERM)|
|  unlock   |            |
|           |   read     | calculate for new value
|           |   write    | <- expected value
+-----------+------------+


Thanks

Takashi Sakamoto

Re: Why doesn't mixer control (values) have some kind of locking mechanism? (mutex?)

[Tom Yan]

On Fri, 7 Aug 2020 at 01:19, Takashi Sakamoto <o-takashi@sakamocchi.jp> wrote:
>
>
> In your case, SNDRV_CTL_IOCTL_ELEM_LOCK looks 'write-lock', therefore
> any userspace applications can do read operation to the control element
> locked by the other processes.
>
> To solve the issue, the pair of read/write operations should be done
> between lock/unlock operations. I can consider about two cases.
>
> A case is that all of applications implements the above. This is
> already proposed. The case is that the world is universe.
>
> +-----------+-----------+
> | process A | process B |
> +-----------+-----------+
> |   lock    |           |
> |   read    |           |
> |           |lock(EPERM)| reschedule lock/get/put/unlock actions
> |   write   |           |
> |           |lock(EPERM)| reschedule lock/get/put/unlock actions
> |  unlock   |           |
> |           |   lock    |
> |           |   read    | calculate for new value
> |           |   write   |
> |           |  unlock   |
> +-----------+-----------+
>
> Another case is that a part of application implements the above. Let
> me drill down the case into two cases. These cases are realistic
> (sign...):
>
> +-----------+------------+
> | process A | process B  |
> +-----------+------------+
> |   lock    |            |
> |   read    |            |
> |   write   |            |
> |           |    read    | calculate for new value
> |           |write(EPERM)|
> |  unlock   |            |
> |           |   write    | <- expected value
> +-----------+------------+
>
> +-----------+------------+
> | process A | process B  |
> +-----------+------------+
> |   lock    |            |
> |   read    |            |
> |           |   read     | calculate for new value
> |   write   |            |
> |           |write(EPERM)|
> |  unlock   |            |
> |           |   write    | <- unexpected value
> +-----------+------------+
>
> The lock/unlock mechanism is not perfect solution as long as any
> applications perform like the process.
>
> If we can expect the most of applications to be back to read operation
> at failure of write operation, thing goes well.
>
> +-----------+------------+
> | process A | process B  |
> +-----------+------------+
> |   lock    |            |
> |   read    |            |
> |           |   read     | calculate for new value
> |   write   |            |
> |           |write(EPERM)|
> |  unlock   |            |
> |           |   read     | calculate for new value
> |           |   write    | <- expected value
> +-----------+------------+
>

Oh you were saying, while it is a "write lock in nature", if all the
processes considered/involved make use of it (properly, by attempting
to lock before *reading*), it would work like a "full" lock. Sorry I
wasn't thinking straight.

And I guess there's no point in changing it into a "real" full lock in
the kernel anyway as that won't prevent whatever that doesn't make use
of it race with each other.

Okay so I suppose that means we can/should at least fix amixer with it
for now and see if we can/want to get something better into the
kernel. (I am in no position to comment on whether we should do
compare-and-swap as I don't know if it's a good idea programmatically
speaking, or if there's a huge price in any aspect; all I know is that
it *looks* more neat.)

Thanks a lot! Should I file an issue additionally in the alsa-utils
github repo btw?

>
> Thanks
>
> Takashi Sakamoto

Re: Why doesn't mixer control (values) have some kind of locking mechanism? (mutex?)

[Jaroslav Kysela]

Dne 06. 08. 20 v 16:47 Takashi Sakamoto napsal(a):
> Hi,
> 
> On Thu, Aug 06, 2020 at 08:31:51PM +0800, Tom Yan wrote:
>> Yeah I suppose a "full" lock would do. (That was what I was trying to
>> point out. I don't really understand Pierre's message. I merely
>> suppose you need some facility in the kernel anyway so that you can
>> lock from userspace.) I hope that amixer the utility will at least have
>> the capability to reschedule/wait by then though (instead of just
>> "failing" like in your python demo).
> 
> As long as I know, neither tools in alsa-utils/alsa-tools nor pulseaudio
> use the lock mechanism. In short, you are the first person to address
> to the issue. Thanks for your patience since the first post in 2015.
> 
>> As for the compare-and-swap part, it's just a plus. Not that
>> "double-looping" for *each* channel doesn't work. It just again seems
>> silly and primitive (and was once confusing to me).
> 
> I prepare a rough kernel patch abount the compare-and-swap idea for
> our discussion. The compare-and-swap is done under lock acquisition of
> 'struct snd_card.controls_rwsem', therefore many types of operations
> to control element (e.g. read as well) get affects. This idea works
> well at first when alsa-lib supports corresponding API and userspace
> applications uses it. Therefore we need more work than changing just
> in userspace.
> 
> But in my opinion, if things can be solved just in userspace, it should
> be done with no change in kernel space.

The compare-and-swap is just a limited mechanism which does not resolve all
possible usage cases (mainly the operation grouping).

I read the whole story and I basically don't think that we need to handle this
in the kernel space. The arbitration should be easily implementable in the
user space. We can agree that we need to add a new extension which will grant
to the user space application the exclusive access to change one or more
control elements exclusively possibly based on the previous contents.

I would propose a new alsa-lib API extension (which may be shared with other
ALSA user space APIs): System V semaphore (man 2 semop). We can use the named
semaphore. Two functions can be added like snd_ctl_transaction_begin() and
snd_ctl_transaction_end() to alsa-lib. The wait condition can be handled
through an argument to snd_ctl_transaction_begin(). It might be probably also
nice to consider the poll multiplexing implementation for the wait condition
(use a message queue to notify other tasks when the semaphore was released).

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [pulseaudio-discuss] Why doesn't mixer control (values) have some kind of locking mechanism? (mutex?)

[Pierre-Louis Bossart]

> ALSA control core allows applications to lock/unlock a control element
> so that any write opreation to the control element fails for processes
> except for owner process.
> 
> When a process requests `SNDRV_CTL_IOCTL_ELEM_LOCK`[1] against a
> control element. After operating the request, the control element is
> under 'owned by the process' state. In this state, any request of
> `SNDRV_CTL_IOCTL_ELEM_WRITE` from the other processes fails with
> `-EPERM`[2]. The write operation from the owner process is successful
> only. When the owner process is going to finish, the state is
> released[3].
> 
> ALSA userspace library, a.k.a alsa-lib, has a pair of
> `snd_ctl_elem_lock()` and `snd_ctl_elem_unlock()` as its exported
> API[4].

Thank you Sakamoto-san for this explanation, I wasn't even aware this 
existed.

What I was trying to describe in my earlier answer is a different need 
to have an atomic update of *multiple* controls.

If e.g. a DSP or hardware engine exposes two separate filters for left 
and right channels, and the coefficients for those filters are modified 
with separate controls, it would be really nice to have the capability 
of writing these controls separately, but have a 'commit' mechanism so 
that these updated coefficients are used at the same time by the left 
and right filters.

Re: [pulseaudio-discuss] Why doesn't mixer control (values) have some kind of locking mechanism? (mutex?)

[Takashi Sakamoto]

Hi,

On Thu, Aug 06, 2020 at 10:30:36AM -0500, Pierre-Louis Bossart wrote:
> What I was trying to describe in my earlier answer is a different need to
> have an atomic update of *multiple* controls.
> 
> If e.g. a DSP or hardware engine exposes two separate filters for left and
> right channels, and the coefficients for those filters are modified with
> separate controls, it would be really nice to have the capability of writing
> these controls separately, but have a 'commit' mechanism so that these
> updated coefficients are used at the same time by the left and right
> filters.

For the pair of left and right filters, the simplest solution is to unify
the two control elements into single one, as you know. The array of
two values can be passed to your driver by single system call and
ALSA control core surely calls driver code under lock acquisition
against any operation for control element.

The other options are case-by-case. At least, you need to describe each
detail for better discussion.


Regards

Takashi Sakamoto

Re: [pulseaudio-discuss] Why doesn't mixer control (values) have some kind of locking mechanism? (mutex?)

[Pierre-Louis Bossart]

> On Thu, Aug 06, 2020 at 10:30:36AM -0500, Pierre-Louis Bossart wrote:
>> What I was trying to describe in my earlier answer is a different need to
>> have an atomic update of *multiple* controls.
>>
>> If e.g. a DSP or hardware engine exposes two separate filters for left and
>> right channels, and the coefficients for those filters are modified with
>> separate controls, it would be really nice to have the capability of writing
>> these controls separately, but have a 'commit' mechanism so that these
>> updated coefficients are used at the same time by the left and right
>> filters.
> 
> For the pair of left and right filters, the simplest solution is to unify
> the two control elements into single one, as you know. The array of
> two values can be passed to your driver by single system call and
> ALSA control core surely calls driver code under lock acquisition
> against any operation for control element.

I am not worried about other applications, the issue is that a 
transaction on a bus or IPC is assumed to have an immediate effect. In 
the case of multiple values, it'd really be desirable to defer the 
effect of write transactions until they are all complete.
I am not making this up, this sort of capabilities is described in 
standards and I am not aware of any support from the ALSA control 
framework for a global commit operation. We have mechanisms to 
synchronize triggers on PCM devices with the snd_pcm_link(), 
synchronization of control changes is a miss IMHO.

Re: Why doesn't mixer control (values) have some kind of locking mechanism? (mutex?)

[Takashi Sakamoto]

Hi,

On Thu, Aug 06, 2020 at 07:12:14PM -0500, Pierre-Louis Bossart wrote:
> > On Thu, Aug 06, 2020 at 10:30:36AM -0500, Pierre-Louis Bossart wrote:
> > > What I was trying to describe in my earlier answer is a different need to
> > > have an atomic update of *multiple* controls.
> > > 
> > > If e.g. a DSP or hardware engine exposes two separate filters for left and
> > > right channels, and the coefficients for those filters are modified with
> > > separate controls, it would be really nice to have the capability of writing
> > > these controls separately, but have a 'commit' mechanism so that these
> > > updated coefficients are used at the same time by the left and right
> > > filters.
> > 
> > For the pair of left and right filters, the simplest solution is to unify
> > the two control elements into single one, as you know. The array of
> > two values can be passed to your driver by single system call and
> > ALSA control core surely calls driver code under lock acquisition
> > against any operation for control element.
 
(After posting the above message, I realized that the above method is
not good in the case since the coefficients data is array type of data.
The aggregation seems not to be well...)

> I am not worried about other applications, the issue is that a transaction
> on a bus or IPC is assumed to have an immediate effect. In the case of
> multiple values, it'd really be desirable to defer the effect of write
> transactions until they are all complete.
> I am not making this up, this sort of capabilities is described in standards
> and I am not aware of any support from the ALSA control framework for a
> global commit operation. We have mechanisms to synchronize triggers on PCM
> devices with the snd_pcm_link(), synchronization of control changes is a
> miss IMHO.

I attempt to arrange several points in your messages:

1. passing vendor-dependent data blob or metadata via ALSA control
   interface without any data abstraction
2. control ioctl request to handle multiple 'struct snd_ctl_elem_value'
   to corresponding control elements at once.
3. introduce traditional 'asynchronous I/O' operation to element write
   operation in system call level.

I'm in conservative place in a point of changes in kernel land. At present,
my answers for the above is:

1. It's impossible for standard ALSA control applications such as
   amixer(1) to handle the vendor-dependent data blob or metadata.
   Therefore the functionality is not necessarily to be implemented with
   ALSA control interface. The functionality is itself unfriendly to
   the existent userspace applications.
2. Any kernel patch is welcome. But I'm happy if you have enough care
   of my proposal of limitation removal for the number of members in
   value array[1].
3. It seems to be problematic for both of ALSA control core and
   userspace applications since any attempt for asynchronous I/O in
   system call level is not necessarily successful in history of Linux
   kernel development (or standardization by Austin). I don't know the
   future.

Summary, I recommend you to use ALSA hwdep interface for the
functionality in your device, instead of ALSA control interface.
You can see some drivers have implementation for userspace applications
to execute request and wait for response; e.g. snd-fireworks.

But I note that the summary comes from my conservative place and
there is space for further discussion.

[1] https://github.com/takaswie/presentations/blob/master/20181021/contents.md#limitation-on-a-container-for-value-array-to-an-element


Thanks

Takashi Sakamoto