[RFC][PATCH 0/2] ALSA: control: limit life time of user-defined element set

[RFC][PATCH 0/2] ALSA: control: limit life time of user-defined element set

[Takashi Sakamoto]

Hi,

This patchset is an example to implement an idea mentioned in this thread:

[alsa-devel] [PATCH] Softvol: Allow per process volume control.
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-July/095026.html

ALSA control core allows applications to add arbitrary element sets to an
instance of control device, while removal of the element sets is voluntarily
done by some applications. This is not convenient to certain situations.
For example, an application adds some element sets, then aborted due to
programming mistakes, and restart. Needless elements can be added one after
another.

This patchset limits life time of user-defined element set. Basically, the life
time is corresponding to a file descriptor opened by a process to add element
sets. However, elements can be locked by the other processes. For this case,
this patchset counts locked elements, then judge to remove an element set
including these elements.

My large concern is whether there's any applications to open a file descriptor
just to add any element sets. In this case, added element sets are removed
immediately at closing the file descriptor unexpectedly to the application, and
unavailable to the other processes anymore. In this point, this patchset is
intrusive.

(But I'm a bit optimistic because use-defined element set is long abandoned not
to work well in several items, and there may be probably few consumers in user
space. Perhaps, this change is acceptable, I hope.)

To reviewers, my aim for this patchset is to show attempts to synchronize life
time of user-defined element set to file descriptors. Any objections are
welcome. Then, I'd like to receive alternative ideas, especially for definition
of the life time.

Takashi Sakamoto (2):
  ALSA: control: limit life time of user-defined element set
  ALSA: control: bump up protocol version to 2.0.8

 include/uapi/sound/asound.h |   2 +-
 sound/core/control.c        | 101 +++++++++++++++++++++++++++++++++++++-------
 2 files changed, 87 insertions(+), 16 deletions(-)

-- 
2.7.4

[PATCH 1/2] ALSA: control: limit life time of user-defined element set

[Takashi Sakamoto]

ALSA control core allows applications to add arbitrary element sets to
an instance of control device. This is performed by ioctl(2) with
SNDRV_CTL_IOCTL_ELEM_ADD request. Elements in added element set can be
handled by the same way as the one added by kernel drivers. The elements
can be removed by ioctl(2) with SNDRV_CTL_IOCTL_ELEM_REMOVE for an
including element set. As of 2016, the main user of this feature is PCM
softvol plugin in alsa-lib.

There's an issue of removal. If a process adds any element sets and never
does removal, the element set still remain in the control device; e.g.
aborted by some reasons. Although the other processes can perform it,
this is unlikely because the process which is the most interested in the
element set is a process to add them.

This commit attempts to limit life time of the element set. When a process
adds some element sets, they're basically removed corresponding to closing
a file descriptor used for the addition. This is convenient because
operating system promises to close the file descriptor when the process
aborts. If the file descriptor is duplicated for several processes, the
removal is done when one of the processes finally closes the descriptor.

There's an exception; lock operation. ALSA control core allows applications
to lock favorite elements by ioctl(2) via SNDRV_CTL_IOCTL_ELEM_LOCK
request. In this case, the process becomes 'owner' of the element and
disallow the other processes to read/write any data or TLV information to
it. When the removal of an element set is executed corresponding to a file
descriptor of a process, during some elements in the element set are locked
via the other processes, it's logically wrong.

For this situation, this commit counts locked elements in an element set,
then judge to remove or not.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 101 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 86 insertions(+), 15 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index fb096cb..24fca5d 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -34,6 +34,27 @@
 #define MAX_USER_CONTROLS	32
 #define MAX_CONTROL_COUNT	1028
 
+struct user_element {
+	/*
+	 * This member is corresponding to a file descriptor to add this
+	 * element set. This is used to maintain a life time of the element set.
+	 */
+	struct snd_ctl_file *originator;
+
+	struct snd_ctl_elem_info info;
+	struct snd_card *card;
+	/* element data */
+	char *elem_data;
+	/* size of element data in bytes */
+	unsigned long elem_data_size;
+	/* TLV data */
+	void *tlv_data;
+	/* TLV data size */
+	unsigned long tlv_data_size;
+	/* private data (like strings for enumerated type) */
+	void *priv_data;
+};
+
 struct snd_kctl_ioctl {
 	struct list_head list;		/* list of all ioctls */
 	snd_kctl_ioctl_func_t fioctl;
@@ -118,20 +139,79 @@ static int snd_ctl_release(struct inode *inode, struct file *file)
 	unsigned long flags;
 	struct snd_card *card;
 	struct snd_ctl_file *ctl;
-	struct snd_kcontrol *control;
+	struct snd_kcontrol *control, *tmp;
 	unsigned int idx;
+	struct snd_kcontrol_volatile *vd;
+	struct user_element *ue;
+	unsigned int count;
 
 	ctl = file->private_data;
 	file->private_data = NULL;
 	card = ctl->card;
+
 	write_lock_irqsave(&card->ctl_files_rwlock, flags);
 	list_del(&ctl->list);
 	write_unlock_irqrestore(&card->ctl_files_rwlock, flags);
+
 	down_write(&card->controls_rwsem);
-	list_for_each_entry(control, &card->controls, list)
-		for (idx = 0; idx < control->count; idx++)
-			if (control->vd[idx].owner == ctl)
-				control->vd[idx].owner = NULL;
+	list_for_each_entry_safe(control, tmp, &card->controls, list) {
+		vd = control->vd;
+
+		/*
+		 * All of elements in an user-defined element set has the same
+		 * access information in their volatile data, therefore it's
+		 * enough to check the first one.
+		 */
+		if (vd[0].access & SNDRV_CTL_ELEM_ACCESS_USER) {
+			ue = control->private_data;
+
+			/*
+			 * Lock the element temporarily for future removal if
+			 * still unlocked.
+			 */
+			if (ue->originator == ctl) {
+				for (idx = 0; idx < control->count; ++idx) {
+					if (vd[idx].owner == NULL)
+						vd[idx].owner = ctl;
+				}
+
+				ue->originator = NULL;
+			}
+		}
+
+		/*
+		 * Unlock each of elements in the element set if it was locked
+		 * via this file descriptor. Then count unlocked elements.
+		 */
+		count = 0;
+		for (idx = 0; idx < control->count; idx++) {
+			if (vd[idx].owner == ctl)
+				vd[idx].owner = NULL;
+			if (vd[idx].owner == NULL)
+				count++;
+		}
+
+		/*
+		 * None of elements in the element set are locked via any file
+		 * descriptors. As long as it's an user-defined element set and
+		 * originator of it is going to be closed or was already closed,
+		 * let's remove it from this control instance.
+		 */
+		if ((vd[0].access & SNDRV_CTL_ELEM_ACCESS_USER) &&
+		    count == control->count) {
+			ue = control->private_data;
+
+			/*
+			 * Originator was already closed or is going to be
+			 * closed.
+			 */
+			if (ue->originator == NULL) {
+				/* No worry of failure. */
+				snd_ctl_remove(card, control);
+				card->user_ctl_count--;
+			}
+		}
+	}
 	up_write(&card->controls_rwsem);
 	snd_ctl_empty_read_queue(ctl);
 	put_pid(ctl->pid);
@@ -1058,16 +1138,6 @@ static int snd_ctl_elem_unlock(struct snd_ctl_file *file,
 	return result;
 }
 
-struct user_element {
-	struct snd_ctl_elem_info info;
-	struct snd_card *card;
-	char *elem_data;		/* element data */
-	unsigned long elem_data_size;	/* size of element data in bytes */
-	void *tlv_data;			/* TLV data */
-	unsigned long tlv_data_size;	/* TLV data size */
-	void *priv_data;		/* private data (like strings for enumerated type) */
-};
-
 static int snd_ctl_elem_user_info(struct snd_kcontrol *kcontrol,
 				  struct snd_ctl_elem_info *uinfo)
 {
@@ -1329,6 +1399,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file,
 	/* Set private data for this userspace control. */
 	ue = (struct user_element *)kctl->private_data;
 	ue->card = card;
+	ue->originator = file;
 	ue->info = *info;
 	ue->info.access = 0;
 	ue->elem_data = (char *)ue + sizeof(*ue);
-- 
2.7.4

[PATCH 2/2] ALSA: control: bump up protocol version to 2.0.8

[Takashi Sakamoto]

In former commit, life time of user-defined element set is limited.
This is a change of protocol between kernel/userland and tell it to
applications so that they can judge to remove added element sets
voluntarily or not.

This commit bumps up the version of protocol.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 include/uapi/sound/asound.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 609cadb..34c6f3e 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -805,7 +805,7 @@ struct snd_timer_tread {
  *                                                                          *
  ****************************************************************************/
 
-#define SNDRV_CTL_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 7)
+#define SNDRV_CTL_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 8)
 
 struct snd_ctl_card_info {
 	int card;			/* card number */
-- 
2.7.4

Re: [RFC][PATCH 0/2] ALSA: control: limit life time of user-defined element set

[Clemens Ladisch]

Takashi Sakamoto wrote:
> ALSA control core allows applications to add arbitrary element sets to an
> instance of control device, while removal of the element sets is voluntarily
> done by some applications. This is not convenient to certain situations.

This was originally designed for softvol, where the user-defined element
should behave as much as possible as a hardware control.

> My large concern is whether there's any applications to open a file descriptor
> just to add any element sets.

alsactl restore


Regards,
Clemens

Re: [RFC][PATCH 0/2] ALSA: control: limit life time of user-defined element set

[Takashi Iwai]

On Sun, 04 Sep 2016 17:38:20 +0200,
Clemens Ladisch wrote:
> 
> Takashi Sakamoto wrote:
> > ALSA control core allows applications to add arbitrary element sets to an
> > instance of control device, while removal of the element sets is voluntarily
> > done by some applications. This is not convenient to certain situations.
> 
> This was originally designed for softvol, where the user-defined element
> should behave as much as possible as a hardware control.
> 
> > My large concern is whether there's any applications to open a file descriptor
> > just to add any element sets.
> 
> alsactl restore

Yes, and this is actually 99% of use cases for now: restoring the
persistent softvol element at the boot time.

A similar idea to implement a process-bound user-space kctl has popped
up a few times in the past, but it's never realized, since no one
could find it really useful / demanded.  You may say that it can be
used for some application-specific volume or such, but what would be
it exactly?

Once when we have a real use case, I'm not against adding such a
change.  But of course as long as it doesn't break the current way of
softvol usage with the persistent user kctl.


thanks,

Takashi

Re: [RFC][PATCH 0/2] ALSA: control: limit life time of user-defined element set

[Takashi Sakamoto]

Hi,

On Sep 5 2016 01:37, Takashi Iwai wrote:
> On Sun, 04 Sep 2016 17:38:20 +0200,
> Clemens Ladisch wrote:
>>
>> Takashi Sakamoto wrote:
>>> ALSA control core allows applications to add arbitrary element sets to an
>>> instance of control device, while removal of the element sets is voluntarily
>>> done by some applications. This is not convenient to certain situations.
>>
>> This was originally designed for softvol, where the user-defined element
>> should behave as much as possible as a hardware control.
>>
>>> My large concern is whether there's any applications to open a file descriptor
>>> just to add any element sets.
>>
>> alsactl restore
>
> Yes, and this is actually 99% of use cases for now: restoring the
> persistent softvol element at the boot time.

Aha. I didn't realize alsactl calls library APIs to add control element 
sets. I was just aware of PCM softvol plugin. Thanks for your notification.

> A similar idea to implement a process-bound user-space kctl has popped
> up a few times in the past, but it's never realized, since no one
> could find it really useful / demanded.

For my information, if possible, would you please show pointers to the 
proposals?

> You may say that it can be used for some application-specific volume or
> such, but what would be it exactly?

I have a plan for a daemon program in user land. It allows ALSA 
applications to control DSP configurations of audio and music units on 
IEEE 1394 bus, via ALSA control character device.

Currently, drivers for the units are in ALSA firewire stack[1], while 
they support no control elements, just support packet streaming 
functionality, because we can achieve it in user land. For example, 
there're already some Python 3 scripts in hinawa-utils[2] to configure 
the units from user space via firewire character device, with a help of 
libhinawa[3].

When detecting some audio and music units on IEEE 1394 bus, the daemon 
program adds some element sets and start listening to them. If ALSA 
control applications changes state of elements in the element sets, the 
daemon catches the event and configure these units by hardware-specific 
ways.

Currently, this idea is just for audio and music units on IEEE 1394 bus. 
But I think we can utilize it for USB audio devices with vendor-specific 
features, without adding more vendor-dependent codes to kernel land.

> Once when we have a real use case, I'm not against adding such a
> change.  But of course as long as it doesn't break the current way of
> softvol usage with the persistent user kctl.

Apparently, this patch breaks the combination of 'PCM softvol plugin' 
and 'alsactl restore'. I think it better to add a new flag such as 
'SNDRV_CTL_ELEM_ACCESS_USER_BOND_TO_FD' to perform automatic removal.

[1] 
http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/sound/firewire
[2] https://github.com/takaswie/hinawa-utils/
[3] https://github.com/takaswie/libhinawa/


Regards

Takashi Sakamoto

Re: [RFC][PATCH 0/2] ALSA: control: limit life time of user-defined element set

[Clemens Ladisch]

Takashi Sakamoto wrote:
> When detecting some audio and music units on IEEE 1394 bus, the daemon
> program adds some element sets and start listening to them. If ALSA
> control applications changes state of elements in the element sets,
> the daemon catches the event and configure these units by hardware-
> specific ways.

If these controls are removed when the daemon exits, the normal alsactl
mechanism of saving/restoring does not work.

I guess the reason for removing the controls is to avoid the case that
they are non-functional if the daemon is not running?


Regards,
Clemens

Re: [RFC][PATCH 0/2] ALSA: control: limit life time of user-defined element set

[Takashi Sakamoto]

Hi Clemens,

On Sep 6 2016 15:28, Clemens Ladisch wrote:
> Takashi Sakamoto wrote:
>> When detecting some audio and music units on IEEE 1394 bus, the daemon
>> program adds some element sets and start listening to them. If ALSA
>> control applications changes state of elements in the element sets,
>> the daemon catches the event and configure these units by hardware-
>> specific ways.
> 
> If these controls are removed when the daemon exits, the normal alsactl
> mechanism of saving/restoring does not work.

Yes, because many of supported audio and music units on IEEE 1394 have
on-board non-volatile memory to restore current state every at
rebooting. I have a plan to utilize it in the daemon. In this case,
restoring from alsactl is conflicts to the feature.

Besides, the rest of units mostly have no feature corresponding to
control elements.

In detail:
 - With control elements and on-board non-volatile memory
   - DM1000/1100/1500 with BeBoB firmware:
   - Dice:
   - Fireworks board module:
- With control elements and non on-board memory
 - OXFW
- No control elements (controlled via MIDI messages)
 - firewire-tascam:
- Unknown
 - firewire-digi00x (certainly have some control elements)
 - firewire-motu (not yet)
 - firewire-rme (not yet)

> I guess the reason for removing the controls is to avoid the case that
> they are non-functional if the daemon is not running?

Yes. For the case, I plan to restart the daemon by 'Restart' directive
of systemd service unit file. Then, in current ALSA control
implementation, elements remains.

In ALSA control core, the maximum number of user-defined element sets is
limited up to 32. So left elements causes an issue that the daemon
cannot add more element sets in a half of way to start.

Of course, I can program the daemon to check the name of adding/added
elements. But to avoid the cumbersome, I simply hope to constrain life
time of element set to file descriptors.

However I have a hesitation for this direction; a daemon for control
service. It might be more huge than what I require, and larger work
against rest of my free time. In short, most of what I need are
satisfied with the combination of libhinawa/hinawa-utils, just to
control the units. I don't prefer over-specification[0]. But I hear
users seem to hope to control their units via ALSA control interface.

Well, this patchset is for more generic issues discussed longer term.
I'd get your comments regardless of an idea of the server.

[0] ffado-dbus-server/ffado-mixer is this kind of software.


Regards

Takashi Sakamoto