[PATCH] ALSA: core - add more card sysfs entries

[PATCH] ALSA: core - add more card sysfs entries

[Jaroslav Kysela]

There are several strings which are describing the card. As time goes,
we have new universal drivers which probe components in a way, which
is disassociated from the card structure (ASoC). Also, some drivers
may require to select another firmware depending on the specific
platform using udev. The new firmware may change the sound card behaviour.

This patch allows flexible modifications of the card description
from the user space to handle the specific boot / plug-in settings.

Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Jaroslav Kysela <perex@perex.cz>
---
 sound/core/init.c | 166 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 155 insertions(+), 11 deletions(-)

diff --git a/sound/core/init.c b/sound/core/init.c
index ef41f5b3a240..01b26912a4d0 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -662,6 +662,33 @@ void snd_card_set_id(struct snd_card *card, const char *nid)
 }
 EXPORT_SYMBOL(snd_card_set_id);
 
+#define EXTRA_ID_CHARS		"_-"
+#define EXTRA_NAME_CHARS	" _-.:"
+
+static bool safe_attr_strcpy(char *dst, size_t dst_count,
+			     const char *src, size_t src_count,
+			     const char *extra_characters)
+{
+	size_t idx, copy;
+	int c;
+
+	copy = src_count >= dst_count ? dst_count - 1 : src_count;
+	for (idx = 0; idx < copy; idx++) {
+		c = src[idx];
+		if (c < ' ') {
+			copy = idx;
+			break;
+		}
+		if (!isalnum(c) && !strchr(extra_characters, c))
+			return false;
+	}
+	if (copy < 3)
+		return false;
+	memcpy(dst, src, copy);
+	dst[copy] = '\0';
+	return true;
+}
+
 static ssize_t
 card_id_show_attr(struct device *dev,
 		  struct device_attribute *attr, char *buf)
@@ -676,18 +703,10 @@ card_id_store_attr(struct device *dev, struct device_attribute *attr,
 {
 	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
 	char buf1[sizeof(card->id)];
-	size_t copy = count > sizeof(card->id) - 1 ?
-					sizeof(card->id) - 1 : count;
-	size_t idx;
-	int c;
 
-	for (idx = 0; idx < copy; idx++) {
-		c = buf[idx];
-		if (!isalnum(c) && c != '_' && c != '-')
-			return -EINVAL;
-	}
-	memcpy(buf1, buf, copy);
-	buf1[copy] = '\0';
+	if (!safe_attr_strcpy(buf1, sizeof(buf1), buf, count, EXTRA_ID_CHARS))
+		return -EINVAL;
+
 	mutex_lock(&snd_card_mutex);
 	if (!card_id_ok(NULL, buf1)) {
 		mutex_unlock(&snd_card_mutex);
@@ -712,9 +731,134 @@ card_number_show_attr(struct device *dev,
 
 static DEVICE_ATTR(number, 0444, card_number_show_attr, NULL);
 
+static ssize_t
+card_driver_show_attr(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
+	return scnprintf(buf, PAGE_SIZE, "%s\n", card->driver);
+}
+
+static ssize_t
+card_driver_store_attr(struct device *dev, struct device_attribute *attr,
+		       const char *buf, size_t count)
+{
+	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
+	char driver1[sizeof(card->driver)];
+
+	if (!safe_attr_strcpy(driver1, sizeof(driver1), buf, count, EXTRA_NAME_CHARS))
+		return -EINVAL;
+	mutex_lock(&snd_card_mutex);
+	strcpy(card->driver, driver1);
+	mutex_unlock(&snd_card_mutex);
+	return count;
+}
+
+static DEVICE_ATTR(driver, 0644, card_driver_show_attr, card_driver_store_attr);
+
+static ssize_t
+card_name_show_attr(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
+	return scnprintf(buf, PAGE_SIZE, "%s\n", card->shortname);
+}
+
+static ssize_t
+card_name_store_attr(struct device *dev, struct device_attribute *attr,
+		     const char *buf, size_t count)
+{
+	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
+	char name1[sizeof(card->shortname)];
+
+	if (!safe_attr_strcpy(name1, sizeof(name1), buf, count, EXTRA_NAME_CHARS))
+		return -EINVAL;
+	mutex_lock(&snd_card_mutex);
+	strcpy(card->shortname, name1);
+	mutex_unlock(&snd_card_mutex);
+	return count;
+}
+
+static DEVICE_ATTR(name, 0644, card_name_show_attr, card_name_store_attr);
+
+static ssize_t
+card_longname_show_attr(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
+	return scnprintf(buf, PAGE_SIZE, "%s\n", card->longname);
+}
+
+static ssize_t
+card_longname_store_attr(struct device *dev, struct device_attribute *attr,
+			 const char *buf, size_t count)
+{
+	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
+	char longname1[sizeof(card->longname)];
+
+	if (!safe_attr_strcpy(longname1, sizeof(longname1), buf, count, EXTRA_NAME_CHARS))
+		return -EINVAL;
+	mutex_lock(&snd_card_mutex);
+	strcpy(card->longname, longname1);
+	mutex_unlock(&snd_card_mutex);
+	return count;
+}
+
+static DEVICE_ATTR(longname, 0644, card_longname_show_attr, card_longname_store_attr);
+
+static ssize_t
+card_mixername_show_attr(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
+	return scnprintf(buf, PAGE_SIZE, "%s\n", card->mixername);
+}
+
+static ssize_t
+card_mixername_store_attr(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t count)
+{
+	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
+	char mixername1[sizeof(card->mixername)];
+
+	if (!safe_attr_strcpy(mixername1, sizeof(mixername1), buf, count, EXTRA_NAME_CHARS))
+		return -EINVAL;
+	mutex_lock(&snd_card_mutex);
+	strcpy(card->mixername, mixername1);
+	mutex_unlock(&snd_card_mutex);
+	return count;
+}
+
+static DEVICE_ATTR(mixername, 0644, card_mixername_show_attr, card_mixername_store_attr);
+
+static ssize_t
+card_components_show_attr(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
+	return scnprintf(buf, PAGE_SIZE, "%s\n", card->components);
+}
+
+static ssize_t
+card_components_store_attr(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
+	char components1[sizeof(card->components)];
+
+	if (!safe_attr_strcpy(components1, sizeof(components1), buf, count, EXTRA_NAME_CHARS))
+		return -EINVAL;
+	mutex_lock(&snd_card_mutex);
+	strcpy(card->components, components1);
+	mutex_unlock(&snd_card_mutex);
+	return count;
+}
+
+static DEVICE_ATTR(components, 0644, card_components_show_attr, card_components_store_attr);
+
 static struct attribute *card_dev_attrs[] = {
 	&dev_attr_id.attr,
 	&dev_attr_number.attr,
+	&dev_attr_driver.attr,
+	&dev_attr_name.attr,
+	&dev_attr_longname.attr,
+	&dev_attr_mixername.attr,
+	&dev_attr_components.attr,
 	NULL
 };
 
-- 
2.30.2

Re: [PATCH] ALSA: core - add more card sysfs entries

[Takashi Sakamoto]

On Thu, Apr 08, 2021 at 11:43:14AM +0200, Jaroslav Kysela wrote:
> There are several strings which are describing the card. As time goes,
> we have new universal drivers which probe components in a way, which
> is disassociated from the card structure (ASoC). Also, some drivers
> may require to select another firmware depending on the specific
> platform using udev. The new firmware may change the sound card behaviour.
> 
> This patch allows flexible modifications of the card description
> from the user space to handle the specific boot / plug-in settings.
> 
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Cc: Mark Brown <broonie@kernel.org>
> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
> ---
>  sound/core/init.c | 166 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 155 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/core/init.c b/sound/core/init.c
> index ef41f5b3a240..01b26912a4d0 100644
> --- a/sound/core/init.c
> +++ b/sound/core/init.c
> @@ -662,6 +662,33 @@ void snd_card_set_id(struct snd_card *card, const char *nid)
>  }
>  EXPORT_SYMBOL(snd_card_set_id);
>  
> +#define EXTRA_ID_CHARS		"_-"
> +#define EXTRA_NAME_CHARS	" _-.:"
> +
> +static bool safe_attr_strcpy(char *dst, size_t dst_count,
> +			     const char *src, size_t src_count,
> +			     const char *extra_characters)
> +{
> +	size_t idx, copy;
> +	int c;
> +
> +	copy = src_count >= dst_count ? dst_count - 1 : src_count;
> +	for (idx = 0; idx < copy; idx++) {
> +		c = src[idx];
> +		if (c < ' ') {
> +			copy = idx;
> +			break;
> +		}
> +		if (!isalnum(c) && !strchr(extra_characters, c))
> +			return false;
> +	}
> +	if (copy < 3)
> +		return false;
> +	memcpy(dst, src, copy);
> +	dst[copy] = '\0';
> +	return true;
> +}
> +
>  static ssize_t
>  card_id_show_attr(struct device *dev,
>  		  struct device_attribute *attr, char *buf)
> @@ -676,18 +703,10 @@ card_id_store_attr(struct device *dev, struct device_attribute *attr,
>  {
>  	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
>  	char buf1[sizeof(card->id)];
> -	size_t copy = count > sizeof(card->id) - 1 ?
> -					sizeof(card->id) - 1 : count;
> -	size_t idx;
> -	int c;
>  
> -	for (idx = 0; idx < copy; idx++) {
> -		c = buf[idx];
> -		if (!isalnum(c) && c != '_' && c != '-')
> -			return -EINVAL;
> -	}
> -	memcpy(buf1, buf, copy);
> -	buf1[copy] = '\0';
> +	if (!safe_attr_strcpy(buf1, sizeof(buf1), buf, count, EXTRA_ID_CHARS))
> +		return -EINVAL;
> +
>  	mutex_lock(&snd_card_mutex);
>  	if (!card_id_ok(NULL, buf1)) {
>  		mutex_unlock(&snd_card_mutex);
> @@ -712,9 +731,134 @@ card_number_show_attr(struct device *dev,
>  
>  static DEVICE_ATTR(number, 0444, card_number_show_attr, NULL);

Use DEVICE_ATTR_RO() instead.

> +static ssize_t
> +card_driver_show_attr(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
> +	return scnprintf(buf, PAGE_SIZE, "%s\n", card->driver);
> +}
> +
> +static ssize_t
> +card_driver_store_attr(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
> +	char driver1[sizeof(card->driver)];
> +
> +	if (!safe_attr_strcpy(driver1, sizeof(driver1), buf, count, EXTRA_NAME_CHARS))
> +		return -EINVAL;
> +	mutex_lock(&snd_card_mutex);
> +	strcpy(card->driver, driver1);
> +	mutex_unlock(&snd_card_mutex);
> +	return count;
> +}
> +
> +static DEVICE_ATTR(driver, 0644, card_driver_show_attr, card_driver_store_attr);

Use DEVICE_ATTR_RW() instead.

> +static ssize_t
> +card_name_show_attr(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
> +	return scnprintf(buf, PAGE_SIZE, "%s\n", card->shortname);
> +}
> +
> +static ssize_t
> +card_name_store_attr(struct device *dev, struct device_attribute *attr,
> +		     const char *buf, size_t count)
> +{
> +	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
> +	char name1[sizeof(card->shortname)];
> +
> +	if (!safe_attr_strcpy(name1, sizeof(name1), buf, count, EXTRA_NAME_CHARS))
> +		return -EINVAL;
> +	mutex_lock(&snd_card_mutex);
> +	strcpy(card->shortname, name1);
> +	mutex_unlock(&snd_card_mutex);
> +	return count;
> +}
> +
> +static DEVICE_ATTR(name, 0644, card_name_show_attr, card_name_store_attr);

Use DEVICE_ATTR_RW() instead.

> +static ssize_t
> +card_longname_show_attr(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
> +	return scnprintf(buf, PAGE_SIZE, "%s\n", card->longname);
> +}
> +
> +static ssize_t
> +card_longname_store_attr(struct device *dev, struct device_attribute *attr,
> +			 const char *buf, size_t count)
> +{
> +	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
> +	char longname1[sizeof(card->longname)];
> +
> +	if (!safe_attr_strcpy(longname1, sizeof(longname1), buf, count, EXTRA_NAME_CHARS))
> +		return -EINVAL;
> +	mutex_lock(&snd_card_mutex);
> +	strcpy(card->longname, longname1);
> +	mutex_unlock(&snd_card_mutex);
> +	return count;
> +}
> +
> +static DEVICE_ATTR(longname, 0644, card_longname_show_attr, card_longname_store_attr);

Use DEVICE_ATTR_RW() instead.

> +static ssize_t
> +card_mixername_show_attr(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
> +	return scnprintf(buf, PAGE_SIZE, "%s\n", card->mixername);
> +}
> +
> +static ssize_t
> +card_mixername_store_attr(struct device *dev, struct device_attribute *attr,
> +			  const char *buf, size_t count)
> +{
> +	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
> +	char mixername1[sizeof(card->mixername)];
> +
> +	if (!safe_attr_strcpy(mixername1, sizeof(mixername1), buf, count, EXTRA_NAME_CHARS))
> +		return -EINVAL;
> +	mutex_lock(&snd_card_mutex);
> +	strcpy(card->mixername, mixername1);
> +	mutex_unlock(&snd_card_mutex);
> +	return count;
> +}
> +
> +static DEVICE_ATTR(mixername, 0644, card_mixername_show_attr, card_mixername_store_attr);

Use DEVICE_ATTR_RW() instead.

> +static ssize_t
> +card_components_show_attr(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
> +	return scnprintf(buf, PAGE_SIZE, "%s\n", card->components);
> +}
> +
> +static ssize_t
> +card_components_store_attr(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	struct snd_card *card = container_of(dev, struct snd_card, card_dev);
> +	char components1[sizeof(card->components)];
> +
> +	if (!safe_attr_strcpy(components1, sizeof(components1), buf, count, EXTRA_NAME_CHARS))
> +		return -EINVAL;
> +	mutex_lock(&snd_card_mutex);
> +	strcpy(card->components, components1);
> +	mutex_unlock(&snd_card_mutex);
> +	return count;
> +}
> +
> +static DEVICE_ATTR(components, 0644, card_components_show_attr, card_components_store_attr);

Use DEVICE_ATTR_RW() instead.

>  static struct attribute *card_dev_attrs[] = {
>  	&dev_attr_id.attr,
>  	&dev_attr_number.attr,
> +	&dev_attr_driver.attr,
> +	&dev_attr_name.attr,
> +	&dev_attr_longname.attr,
> +	&dev_attr_mixername.attr,
> +	&dev_attr_components.attr,
>  	NULL
>  };
>  
> -- 
> 2.30.2

It should be done to emit snd_ctl_event when changing card parameters.
Silent change is the worst since many userspace applications can refer
to them in advance.


Regards

Takashi Sakamoto

Re: [PATCH] ALSA: core - add more card sysfs entries

[Takashi Iwai]

On Thu, 08 Apr 2021 12:38:19 +0200,
Takashi Sakamoto wrote:
> 
> It should be done to emit snd_ctl_event when changing card parameters.
> Silent change is the worst since many userspace applications can refer
> to them in advance.

Agreed.  The changes of those names during operation is fairly
intrusive, and can bring many side-effects.

Any reason that mixername and co have to be changeable inevitably?
If that's about UCM profile and its selection of the hardware
configuration, can it be another additional information instead of
overwriting the existing strings set by the driver?


thanks,

Takashi

Re: [PATCH] ALSA: core - add more card sysfs entries

[Jaroslav Kysela]

Dne 08. 04. 21 v 13:05 Takashi Iwai napsal(a):
> On Thu, 08 Apr 2021 12:38:19 +0200,
> Takashi Sakamoto wrote:
>>
>> It should be done to emit snd_ctl_event when changing card parameters.
>> Silent change is the worst since many userspace applications can refer
>> to them in advance.
> 
> Agreed.  The changes of those names during operation is fairly
> intrusive, and can bring many side-effects.

The event may be nice, indeed. The locking is also not so safe. But it should
be implemented separately.

> Any reason that mixername and co have to be changeable inevitably?
> If that's about UCM profile and its selection of the hardware
> configuration, can it be another additional information instead of
> overwriting the existing strings set by the driver?

Thanks for the discussion. I expected it. The ASoC drivers set all those
parameters in an inconsistent way (nobody verifies that) and I just propose to
fix things at boot in the user space. The goal is not to allow the random user
changes, but to modify this settings via udev in a persistent way. Also,
another firmware can really make the card different from the user space view.

Yes, it's for UCM, but even if we don't consider this purpose, the kernel API
should return some reasonable information rather than very generic (or empty)
strings which are used in the native ALSA utilities for example. So, I think
that we should allow to "fix" this info also from the user space rather than
to extend the existing API.

					Jaroslav

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

Re: [PATCH] ALSA: core - add more card sysfs entries

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 615 bytes --]

On Thu, Apr 08, 2021 at 01:21:52PM +0200, Jaroslav Kysela wrote:

> Yes, it's for UCM, but even if we don't consider this purpose, the kernel API
> should return some reasonable information rather than very generic (or empty)
> strings which are used in the native ALSA utilities for example. So, I think
> that we should allow to "fix" this info also from the user space rather than
> to extend the existing API.

Half the point with UCM was supposed to be to rewrite the control names
we get from the devices into standard things that are useful for
userspace, having to remap things for UCM doesn't sound right.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ALSA: core - add more card sysfs entries

[Takashi Iwai]

On Thu, 08 Apr 2021 14:05:02 +0200,
Mark Brown wrote:
> 
> On Thu, Apr 08, 2021 at 01:21:52PM +0200, Jaroslav Kysela wrote:
> 
> > Yes, it's for UCM, but even if we don't consider this purpose, the kernel API
> > should return some reasonable information rather than very generic (or empty)
> > strings which are used in the native ALSA utilities for example. So, I think
> > that we should allow to "fix" this info also from the user space rather than
> > to extend the existing API.
> 
> Half the point with UCM was supposed to be to rewrite the control names
> we get from the devices into standard things that are useful for
> userspace, having to remap things for UCM doesn't sound right.

I guess the question here is how to identify the proper profile for a
certain platform and how to get it passed over the whole system.
Theoretically, the rename of the card name or mixer name strings could
be done in user-space side, too (e.g. mapping in alsa-lib or
whatever), so I don't think it mandatory to make them variable via
sysfs, if it's meant only for the consistency reason.

Didn't we discuss in the past about the possibility to store the
profile name in the card component string?


thanks,

Takashi

Re: [PATCH] ALSA: core - add more card sysfs entries

[Pierre-Louis Bossart]

On 4/8/21 8:18 AM, Takashi Iwai wrote:
> On Thu, 08 Apr 2021 14:05:02 +0200,
> Mark Brown wrote:
>>
>> On Thu, Apr 08, 2021 at 01:21:52PM +0200, Jaroslav Kysela wrote:
>>
>>> Yes, it's for UCM, but even if we don't consider this purpose, the kernel API
>>> should return some reasonable information rather than very generic (or empty)
>>> strings which are used in the native ALSA utilities for example. So, I think
>>> that we should allow to "fix" this info also from the user space rather than
>>> to extend the existing API.
>>
>> Half the point with UCM was supposed to be to rewrite the control names
>> we get from the devices into standard things that are useful for
>> userspace, having to remap things for UCM doesn't sound right.
> 
> I guess the question here is how to identify the proper profile for a
> certain platform and how to get it passed over the whole system.
> Theoretically, the rename of the card name or mixer name strings could
> be done in user-space side, too (e.g. mapping in alsa-lib or
> whatever), so I don't think it mandatory to make them variable via
> sysfs, if it's meant only for the consistency reason.
> 
> Didn't we discuss in the past about the possibility to store the
> profile name in the card component string?

Here's a summary of an earlier discussion with Jaroslav, based on an 
initial ask from Curtis https://github.com/thesofproject/linux/issues/2766:

When a specific PCI ID is detected, we probe the SOF driver and will 
load a default firmware binary and topology.

Because of OEM or user customization, we will have multiple versions of 
firmware and topology that will have to be enabled in specific setting. 
The last thing we want is hard-coded rules in the kernel on which 
firmware customization to use for which platform.

The suggestion was made to use udev rules to modify the default path for 
the firmware and topology, so that e.g. on a specific Chromebook you 
could load firmware from /lib/firmware/google/<device name>/sof-tplg. 
The same can happen for other OEMs that support Linux distributions such 
as Dell and Lenovo.

If the users wipes the OEM image and installs a standard distribution on 
the same device, they would by default use the firmware generated from 
the SOF main branch, without any differentiation and 3rd party IP.

So the point is: how do we expose this information to UCM? In the 
machine driver where the card is created? There is zero information on 
what the firmware/topology does. The information can only be extracted 
when the topology is loaded when probing the SOF component driver.

I don't think the point was to rewrite the controls but make sure that 
UCM is aware that the card definition was changed by a different 
selection of firmware.

Jaroslav, please correct me if I misunderstood the intent of this patch!

Re: [PATCH] ALSA: core - add more card sysfs entries

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1796 bytes --]

On Thu, Apr 08, 2021 at 09:12:57AM -0500, Pierre-Louis Bossart wrote:
> On 4/8/21 8:18 AM, Takashi Iwai wrote:

> > I guess the question here is how to identify the proper profile for a
> > certain platform and how to get it passed over the whole system.
> > Theoretically, the rename of the card name or mixer name strings could
> > be done in user-space side, too (e.g. mapping in alsa-lib or
> > whatever), so I don't think it mandatory to make them variable via
> > sysfs, if it's meant only for the consistency reason.

> > Didn't we discuss in the past about the possibility to store the
> > profile name in the card component string?

> Because of OEM or user customization, we will have multiple versions of
> firmware and topology that will have to be enabled in specific setting. The
> last thing we want is hard-coded rules in the kernel on which firmware
> customization to use for which platform.

...

> If the users wipes the OEM image and installs a standard distribution on the
> same device, they would by default use the firmware generated from the SOF
> main branch, without any differentiation and 3rd party IP.

> So the point is: how do we expose this information to UCM? In the machine
> driver where the card is created? There is zero information on what the
> firmware/topology does. The information can only be extracted when the
> topology is loaded when probing the SOF component driver.

So what we're looking for here is a mechanism to tell userspace what
firmware has been loaded?
> 
> I don't think the point was to rewrite the controls but make sure that UCM
> is aware that the card definition was changed by a different selection of
> firmware.
> 
> Jaroslav, please correct me if I misunderstood the intent of this patch!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ALSA: core - add more card sysfs entries

[Jaroslav Kysela]

Dne 08. 04. 21 v 16:47 Mark Brown napsal(a):
> On Thu, Apr 08, 2021 at 09:12:57AM -0500, Pierre-Louis Bossart wrote:
>> On 4/8/21 8:18 AM, Takashi Iwai wrote:
> 
>>> I guess the question here is how to identify the proper profile for a
>>> certain platform and how to get it passed over the whole system.
>>> Theoretically, the rename of the card name or mixer name strings could
>>> be done in user-space side, too (e.g. mapping in alsa-lib or
>>> whatever), so I don't think it mandatory to make them variable via
>>> sysfs, if it's meant only for the consistency reason.
> 
>>> Didn't we discuss in the past about the possibility to store the
>>> profile name in the card component string?
> 
>> Because of OEM or user customization, we will have multiple versions of
>> firmware and topology that will have to be enabled in specific setting. The
>> last thing we want is hard-coded rules in the kernel on which firmware
>> customization to use for which platform.
> 
> ...
> 
>> If the users wipes the OEM image and installs a standard distribution on the
>> same device, they would by default use the firmware generated from the SOF
>> main branch, without any differentiation and 3rd party IP.
> 
>> So the point is: how do we expose this information to UCM? In the machine
>> driver where the card is created? There is zero information on what the
>> firmware/topology does. The information can only be extracted when the
>> topology is loaded when probing the SOF component driver.
> 
> So what we're looking for here is a mechanism to tell userspace what
> firmware has been loaded?

It's just a part of the issue with the proper runtime sound card
identification. But yes, the firmware can really change the created sound card
(controls, PCM devices, paramters for those devices). In this case, UCM should
pick another configuration.

I'm looking for a straight solution here.

					Jaroslav

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

Re: [PATCH] ALSA: core - add more card sysfs entries

[Amadeusz Sławiński]

On 4/8/2021 5:04 PM, Jaroslav Kysela wrote:
> Dne 08. 04. 21 v 16:47 Mark Brown napsal(a):
>> On Thu, Apr 08, 2021 at 09:12:57AM -0500, Pierre-Louis Bossart wrote:
>>> On 4/8/21 8:18 AM, Takashi Iwai wrote:
>>
>>>> I guess the question here is how to identify the proper profile for a
>>>> certain platform and how to get it passed over the whole system.
>>>> Theoretically, the rename of the card name or mixer name strings could
>>>> be done in user-space side, too (e.g. mapping in alsa-lib or
>>>> whatever), so I don't think it mandatory to make them variable via
>>>> sysfs, if it's meant only for the consistency reason.
>>
>>>> Didn't we discuss in the past about the possibility to store the
>>>> profile name in the card component string?
>>
>>> Because of OEM or user customization, we will have multiple versions of
>>> firmware and topology that will have to be enabled in specific setting. The
>>> last thing we want is hard-coded rules in the kernel on which firmware
>>> customization to use for which platform.
>>
>> ...
>>
>>> If the users wipes the OEM image and installs a standard distribution on the
>>> same device, they would by default use the firmware generated from the SOF
>>> main branch, without any differentiation and 3rd party IP.
>>
>>> So the point is: how do we expose this information to UCM? In the machine
>>> driver where the card is created? There is zero information on what the
>>> firmware/topology does. The information can only be extracted when the
>>> topology is loaded when probing the SOF component driver.
>>
>> So what we're looking for here is a mechanism to tell userspace what
>> firmware has been loaded?
> 
> It's just a part of the issue with the proper runtime sound card
> identification. But yes, the firmware can really change the created sound card
> (controls, PCM devices, paramters for those devices). In this case, UCM should
> pick another configuration.
> 
> I'm looking for a straight solution here.
> 
> 					Jaroslav
> 

Wouldn't dsp_status from snd_hwdep already work here?
The only thing which would need to be done is to connect it to ASoC card?

And in userspace ask using hwdep interface for what is the loaded FW?

Amadeusz

Re: [PATCH] ALSA: core - add more card sysfs entries

[Jaroslav Kysela]

Dne 08. 04. 21 v 15:18 Takashi Iwai napsal(a):
> On Thu, 08 Apr 2021 14:05:02 +0200,
> Mark Brown wrote:
>>
>> On Thu, Apr 08, 2021 at 01:21:52PM +0200, Jaroslav Kysela wrote:
>>
>>> Yes, it's for UCM, but even if we don't consider this purpose, the kernel API
>>> should return some reasonable information rather than very generic (or empty)
>>> strings which are used in the native ALSA utilities for example. So, I think
>>> that we should allow to "fix" this info also from the user space rather than
>>> to extend the existing API.
>>
>> Half the point with UCM was supposed to be to rewrite the control names
>> we get from the devices into standard things that are useful for
>> userspace, having to remap things for UCM doesn't sound right.
> 
> I guess the question here is how to identify the proper profile for a
> certain platform and how to get it passed over the whole system.
> Theoretically, the rename of the card name or mixer name strings could
> be done in user-space side, too (e.g. mapping in alsa-lib or
> whatever), so I don't think it mandatory to make them variable via
> sysfs, if it's meant only for the consistency reason.

There are two things to consider (please, don't concentrate to UCM here):

1) the card identification
2) the user experience

Actually, the generic ASoC drivers are too much generic and they didn't
provide a solid information about the hardware.

Two examples:

rpi with PCM5100A DAC (not hifiberry):

# cat /proc/asound/cards
 0 [sndrpihifiberry]: RPi-simple - snd_rpi_hifiberry_dac
                      snd_rpi_hifiberry_dac

SOF SoundWire driver:

# cat /proc/asound/cards
 0 [sofsoundwire   ]: sof-soundwire - sof-soundwire
                      Intel Soundwire SOF
# amixer -c 0 info
Card hw:0 'sofsoundwire'/'Intel Soundwire SOF'
  Mixer name	: 'Intel Kabylake HDMI'
  Components	: 'HDA:8086280b,80860101,00100000 cfg-spk:4 cfg-amp:2 hs:rt711
spk:rt1308 mic:rt715'


As you can see, the information for both drivers is quite inaccurate and users
(including me) may want some flexibility to change those strings to something
else. It may resolve some UCM problems, but it's just one small piece of the
issue.

Another issue is when the udev driver loader can change some parameters which
modifies the driver behaviour and sound device structure for the given card
(as discussed in another thread).

When we have a common standard layer for the plug-and-play handling (udev), we
should concentrate to allow changing / refining of this information there.
Those strings are not used for anything else than the user space. So from my
view, there's no reason to create another mechanism to handle the overrides.
It should be a safe, fast, flexible and _optional_ solution. The udev can
alter the sysfs attributes directly without any hassle with the file
modifications or looking for another way to pass / store this information
somewhere.

I evaluated the other possibilities like store the overwrites to a file, udev
environment (per device) and they are not so easy or create extra dependencies
(alsa-lib -> libudev).

> Didn't we discuss in the past about the possibility to store the
> profile name in the card component string?

It's already possible to extract any information from the components string.
The current UCM is very flexible, but it does not allow to use a missing
information.

The main questions is: Can we make those strings writable or not? What
prevents us to not allow that?

					Jaroslav

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

Re: [PATCH] ALSA: core - add more card sysfs entries

[Pierre-Louis Bossart]

On 4/8/21 10:01 AM, Jaroslav Kysela wrote:
> When we have a common standard layer for the plug-and-play handling (udev), we
> should concentrate to allow changing / refining of this information there.
> Those strings are not used for anything else than the user space. So from my
> view, there's no reason to create another mechanism to handle the overrides.
> It should be a safe, fast, flexible and_optional_  solution. The udev can
> alter the sysfs attributes directly without any hassle with the file
> modifications or looking for another way to pass / store this information
> somewhere.

There's one part where I am lost.

The initial idea of udev what to modify kernel parameters to pick a 
different path for firmware/topology before probing the PCI driver. At 
that point there is no card and no sysfs attributes just yet, they will 
be added at a later point during the probe itself.

So are we talking about a second set of rules that would be applied when 
the card is created?

Re: [PATCH] ALSA: core - add more card sysfs entries

[Jaroslav Kysela]

Dne 08. 04. 21 v 17:32 Pierre-Louis Bossart napsal(a):
> 
> 
> On 4/8/21 10:01 AM, Jaroslav Kysela wrote:
>> When we have a common standard layer for the plug-and-play handling (udev), we
>> should concentrate to allow changing / refining of this information there.
>> Those strings are not used for anything else than the user space. So from my
>> view, there's no reason to create another mechanism to handle the overrides.
>> It should be a safe, fast, flexible and_optional_  solution. The udev can
>> alter the sysfs attributes directly without any hassle with the file
>> modifications or looking for another way to pass / store this information
>> somewhere.
> 
> There's one part where I am lost.
> 
> The initial idea of udev what to modify kernel parameters to pick a 
> different path for firmware/topology before probing the PCI driver. At 

This may be a problematic point. The kernel cmdline cannot be modified from
udev (as far as I know). The module parameters can be set using modprobe's
config files or when loaded with sysfs attributes (/sys/module/*/parameters).
Eventually, you can call the modprobe command with custom module parameters
when the appropriate MODALIAS is probed.

Perhaps, I'm missing something here, too. Some example udev rules may help.

> that point there is no card and no sysfs attributes just yet, they will 
> be added at a later point during the probe itself.
> 
> So are we talking about a second set of rules that would be applied when 
> the card is created?

Yes, I'm talking about rules which depends on the sound driver specific sysfs
attributes (you can match the modified /sys/module/*/parameters here).

						Jaroslav

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

Re: [PATCH] ALSA: core - add more card sysfs entries

[Pierre-Louis Bossart]

>>> When we have a common standard layer for the plug-and-play handling (udev), we
>>> should concentrate to allow changing / refining of this information there.
>>> Those strings are not used for anything else than the user space. So from my
>>> view, there's no reason to create another mechanism to handle the overrides.
>>> It should be a safe, fast, flexible and_optional_  solution. The udev can
>>> alter the sysfs attributes directly without any hassle with the file
>>> modifications or looking for another way to pass / store this information
>>> somewhere.
>>
>> There's one part where I am lost.
>>
>> The initial idea of udev what to modify kernel parameters to pick a
>> different path for firmware/topology before probing the PCI driver. At
> 
> This may be a problematic point. The kernel cmdline cannot be modified from
> udev (as far as I know). The module parameters can be set using modprobe's
> config files or when loaded with sysfs attributes (/sys/module/*/parameters).
> Eventually, you can call the modprobe command with custom module parameters
> when the appropriate MODALIAS is probed.
> 
> Perhaps, I'm missing something here, too. Some example udev rules may help.

see the example shared by Curtis

SUBSYSTEM=="pci", ATTR{vendor}=="0x8086", ATTR{device}=="0xa0c8", 
ATTR{class}=="0x040100", ATTRS{[dmi/id]board_name}=="Eldrid", 
RUN+="/sbin/modprobe snd_sof_pci tplg_path=intel/sof-tplg/pdm1"

Those 'path' parameters would have to be set prior to creating the card, 
making them writable via sysfs would not work, the firmware and topology 
are already loaded and changing the paths would have no effect.

>> that point there is no card and no sysfs attributes just yet, they will
>> be added at a later point during the probe itself.
>>
>> So are we talking about a second set of rules that would be applied when
>> the card is created?
> 
> Yes, I'm talking about rules which depends on the sound driver specific sysfs
> attributes (you can match the modified /sys/module/*/parameters here).

you lost me with 'match the modified parameters' wording. who matches 
and who modifies those parameters?

Re: [PATCH] ALSA: core - add more card sysfs entries

[Jaroslav Kysela]

Dne 08. 04. 21 v 20:51 Pierre-Louis Bossart napsal(a):
> 
> 
>>>> When we have a common standard layer for the plug-and-play handling (udev), we
>>>> should concentrate to allow changing / refining of this information there.
>>>> Those strings are not used for anything else than the user space. So from my
>>>> view, there's no reason to create another mechanism to handle the overrides.
>>>> It should be a safe, fast, flexible and_optional_  solution. The udev can
>>>> alter the sysfs attributes directly without any hassle with the file
>>>> modifications or looking for another way to pass / store this information
>>>> somewhere.
>>>
>>> There's one part where I am lost.
>>>
>>> The initial idea of udev what to modify kernel parameters to pick a
>>> different path for firmware/topology before probing the PCI driver. At
>>
>> This may be a problematic point. The kernel cmdline cannot be modified from
>> udev (as far as I know). The module parameters can be set using modprobe's
>> config files or when loaded with sysfs attributes (/sys/module/*/parameters).
>> Eventually, you can call the modprobe command with custom module parameters
>> when the appropriate MODALIAS is probed.
>>
>> Perhaps, I'm missing something here, too. Some example udev rules may help.
> 
> see the example shared by Curtis
> 
> SUBSYSTEM=="pci", ATTR{vendor}=="0x8086", ATTR{device}=="0xa0c8", 
> ATTR{class}=="0x040100", ATTRS{[dmi/id]board_name}=="Eldrid", 
> RUN+="/sbin/modprobe snd_sof_pci tplg_path=intel/sof-tplg/pdm1"
> 
> Those 'path' parameters would have to be set prior to creating the card, 
> making them writable via sysfs would not work, the firmware and topology 
> are already loaded and changing the paths would have no effect.

Yes, it's something what I suggested. It's own modprobe call which modifies the module parameters.

>>> that point there is no card and no sysfs attributes just yet, they will
>>> be added at a later point during the probe itself.
>>>
>>> So are we talking about a second set of rules that would be applied when
>>> the card is created?
>>
>> Yes, I'm talking about rules which depends on the sound driver specific sysfs
>> attributes (you can match the modified /sys/module/*/parameters here).
> 
> you lost me with 'match the modified parameters' wording. who matches 
> and who modifies those parameters?

You can probably add something like this to the sound-card.rules:

SUBSYSTEMS=="pci",ATTR{device/driver/module}=="snd_soc_sof_sdw",
  ATTR{device/driver/module/../snd_sof_pci/parameters/tplg_path}=="intel/sof-tplg/pdm1",
  DO_SOMETHING_HERE

DO_SOMETHING_HERE may be ATTR{longname}="My Long Name" for example when my change is accepted.

				Jaroslav

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

Re: [PATCH] ALSA: core - add more card sysfs entries

[Pierre-Louis Bossart]

>>> Yes, I'm talking about rules which depends on the sound driver specific sysfs
>>> attributes (you can match the modified /sys/module/*/parameters here).
>>
>> you lost me with 'match the modified parameters' wording. who matches
>> and who modifies those parameters?
> 
> You can probably add something like this to the sound-card.rules:
> 
> SUBSYSTEMS=="pci",ATTR{device/driver/module}=="snd_soc_sof_sdw",
>    ATTR{device/driver/module/../snd_sof_pci/parameters/tplg_path}=="intel/sof-tplg/pdm1",
>    DO_SOMETHING_HERE
> 
> DO_SOMETHING_HERE may be ATTR{longname}="My Long Name" for example when my change is accepted.

Humm, not sure this can work due to dependencies.

The machine device is neither an ACPI nor PCI one. It's a platform device.

When the PCI device is detected, the PCI core will call the SOF driver 
probe, which will first try and boot the firmware, and then create the 
platform device. That results in the probe of the machine driver which 
creates the card, but that happens *after* booting the firmware.

the DSP firmware is setup starting here:

https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/core.c#L138

and the machine device is created almost last, after registering the 
ASoC components.

https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/core.c#L234

when the card is created, it's too late to change the firmware path or 
any firmware-related parameters.

Re: [PATCH] ALSA: core - add more card sysfs entries

[Curtis Malainey]

On Thu, Apr 8, 2021 at 12:43 PM Pierre-Louis Bossart <
pierre-louis.bossart@linux.intel.com> wrote:

>
>
>
> >>> Yes, I'm talking about rules which depends on the sound driver
> specific sysfs
> >>> attributes (you can match the modified /sys/module/*/parameters here).
> >>
> >> you lost me with 'match the modified parameters' wording. who matches
> >> and who modifies those parameters?
> >
> > You can probably add something like this to the sound-card.rules:
> >
> > SUBSYSTEMS=="pci",ATTR{device/driver/module}=="snd_soc_sof_sdw",
> >
> ATTR{device/driver/module/../snd_sof_pci/parameters/tplg_path}=="intel/sof-tplg/pdm1",
> >    DO_SOMETHING_HERE
> >
> > DO_SOMETHING_HERE may be ATTR{longname}="My Long Name" for example when
> my change is accepted.
>
> Humm, not sure this can work due to dependencies.
>
> The machine device is neither an ACPI nor PCI one. It's a platform device.
>
> When the PCI device is detected, the PCI core will call the SOF driver
> probe, which will first try and boot the firmware, and then create the
> platform device. That results in the probe of the machine driver which
> creates the card, but that happens *after* booting the firmware.
>
> the DSP firmware is setup starting here:
>
> https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/core.c#L138
>
> and the machine device is created almost last, after registering the
> ASoC components.
>
> https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/core.c#L234
>
> when the card is created, it's too late to change the firmware path or
> any firmware-related parameters.
>
>
>
Thanks for raising this to my attention Pierre, yes we are also
experiencing a similar issue since chromium builds will have default sof
tplg and fw, but chrome builds will not. Since our UCMs are in a single
location right now they will contain commands that will not execute on the
defaults and we have no way of indicating to our audio server how to handle
this. I am definitely interested in seeing a feature where we can pass
indicators to the UCM about what we have available.

Re: [PATCH] ALSA: core - add more card sysfs entries

[Jaroslav Kysela]

Dne 08. 04. 21 v 21:41 Pierre-Louis Bossart napsal(a):
> 
> 
> 
>>>> Yes, I'm talking about rules which depends on the sound driver specific sysfs
>>>> attributes (you can match the modified /sys/module/*/parameters here).
>>>
>>> you lost me with 'match the modified parameters' wording. who matches
>>> and who modifies those parameters?
>>
>> You can probably add something like this to the sound-card.rules:
>>
>> SUBSYSTEMS=="pci",ATTR{device/driver/module}=="snd_soc_sof_sdw",
>>    ATTR{device/driver/module/../snd_sof_pci/parameters/tplg_path}=="intel/sof-tplg/pdm1",
>>    DO_SOMETHING_HERE
>>
>> DO_SOMETHING_HERE may be ATTR{longname}="My Long Name" for example when my change is accepted.
> 
> Humm, not sure this can work due to dependencies.
> 
> The machine device is neither an ACPI nor PCI one. It's a platform device.
> 
> When the PCI device is detected, the PCI core will call the SOF driver 
> probe, which will first try and boot the firmware, and then create the 
> platform device. That results in the probe of the machine driver which 
> creates the card, but that happens *after* booting the firmware.
> 
> the DSP firmware is setup starting here:
> 
> https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/core.c#L138
> 
> and the machine device is created almost last, after registering the 
> ASoC components.
> 
> https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/core.c#L234
> 
> when the card is created, it's too late to change the firmware path or 
> any firmware-related parameters.

I just tried to describe the possible 2nd stage - modify the sysfs attributes
when the card with the modified firmware is created (all modules are loaded
and initialized). The 1st stage like from Curtis must be retained. It ensures
to load the right fw.

SYSTEMS=="pci" checks also parents and card0 links to pci device: card0 ->
../../devices/pci0000:00/0000:00:1f.3/sof_sdw/sound/card0 . You can modify
this matching anyway - the goal is to run commands for the specific driver and
module parameters when the card is loaded (avoid to change the card attributes
for other hw).

I'm not an udev expert, so there may be a bug in my suggestion. I also think
that the filter may be specified more elegantly (probably using the DRIVER
match or so).

					Jaroslav

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

Re: [PATCH] ALSA: core - add more card sysfs entries

[Jaroslav Kysela]

Dne 08. 04. 21 v 22:01 Jaroslav Kysela napsal(a):
> Dne 08. 04. 21 v 21:41 Pierre-Louis Bossart napsal(a):
>>
>>
>>
>>>>> Yes, I'm talking about rules which depends on the sound driver specific sysfs
>>>>> attributes (you can match the modified /sys/module/*/parameters here).
>>>>
>>>> you lost me with 'match the modified parameters' wording. who matches
>>>> and who modifies those parameters?
>>>
>>> You can probably add something like this to the sound-card.rules:
>>>
>>> SUBSYSTEMS=="pci",ATTR{device/driver/module}=="snd_soc_sof_sdw",
>>>    ATTR{device/driver/module/../snd_sof_pci/parameters/tplg_path}=="intel/sof-tplg/pdm1",
>>>    DO_SOMETHING_HERE
>>>
>>> DO_SOMETHING_HERE may be ATTR{longname}="My Long Name" for example when my change is accepted.
>>
>> Humm, not sure this can work due to dependencies.
>>
>> The machine device is neither an ACPI nor PCI one. It's a platform device.
>>
>> When the PCI device is detected, the PCI core will call the SOF driver 
>> probe, which will first try and boot the firmware, and then create the 
>> platform device. That results in the probe of the machine driver which 
>> creates the card, but that happens *after* booting the firmware.
>>
>> the DSP firmware is setup starting here:
>>
>> https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/core.c#L138
>>
>> and the machine device is created almost last, after registering the 
>> ASoC components.
>>
>> https://elixir.bootlin.com/linux/latest/source/sound/soc/sof/core.c#L234
>>
>> when the card is created, it's too late to change the firmware path or 
>> any firmware-related parameters.
> 
> I just tried to describe the possible 2nd stage - modify the sysfs attributes
> when the card with the modified firmware is created (all modules are loaded
> and initialized). The 1st stage like from Curtis must be retained. It ensures
> to load the right fw.
> 
> SYSTEMS=="pci" checks also parents and card0 links to pci device: card0 ->
> ../../devices/pci0000:00/0000:00:1f.3/sof_sdw/sound/card0 . You can modify
> this matching anyway - the goal is to run commands for the specific driver and
> module parameters when the card is loaded (avoid to change the card attributes
> for other hw).
> 
> I'm not an udev expert, so there may be a bug in my suggestion. I also think
> that the filter may be specified more elegantly (probably using the DRIVER
> match or so).

Another way to use two rules - use internal udev device environment variable
(it seems more straight):

PCI detection level:

SUBSYSTEM=="pci", ATTR{vendor}=="0x8086", ATTR{device}=="0xa0c8",
ATTR{class}=="0x040100",ATTRS{[dmi/id]board_name}=="Eldrid",
ENV{SOUND_SOF_PROFILE}="MyProfile",...module stuff...

Card instance level (sound-card.rules):

SUBSYSTEMS=="pci",ENV{SOUND_SOF_PROFILE}=="MyProfile",...attr setup...

					Jaroslav

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

Re: [PATCH] ALSA: core - add more card sysfs entries

[Takashi Iwai]

On Thu, 08 Apr 2021 20:51:41 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> 
> >>> When we have a common standard layer for the plug-and-play handling (udev), we
> >>> should concentrate to allow changing / refining of this information there.
> >>> Those strings are not used for anything else than the user space. So from my
> >>> view, there's no reason to create another mechanism to handle the overrides.
> >>> It should be a safe, fast, flexible and_optional_  solution. The udev can
> >>> alter the sysfs attributes directly without any hassle with the file
> >>> modifications or looking for another way to pass / store this information
> >>> somewhere.
> >>
> >> There's one part where I am lost.
> >>
> >> The initial idea of udev what to modify kernel parameters to pick a
> >> different path for firmware/topology before probing the PCI driver. At
> >
> > This may be a problematic point. The kernel cmdline cannot be modified from
> > udev (as far as I know). The module parameters can be set using modprobe's
> > config files or when loaded with sysfs attributes (/sys/module/*/parameters).
> > Eventually, you can call the modprobe command with custom module parameters
> > when the appropriate MODALIAS is probed.
> >
> > Perhaps, I'm missing something here, too. Some example udev rules may help.
> 
> see the example shared by Curtis
> 
> SUBSYSTEM=="pci", ATTR{vendor}=="0x8086", ATTR{device}=="0xa0c8",
> ATTR{class}=="0x040100", ATTRS{[dmi/id]board_name}=="Eldrid",
> RUN+="/sbin/modprobe snd_sof_pci tplg_path=intel/sof-tplg/pdm1"
> 
> Those 'path' parameters would have to be set prior to creating the
> card, making them writable via sysfs would not work, the firmware and
> topology are already loaded and changing the paths would have no
> effect.

Couldn't the driver probe the firmware files with some device-specific
string suffix at first?  e.g. the driver can issue request_firmware()
with $base_file-$dmi_board at first, then falls back to the generic
$base_file.  A similar method was already used in Broadcom WiFi
driver.

Also, the driver may do request_firmware() with a fixed path for the
custom firmware at first (e.g. "intel/sof-tplg-custom"); then a system
integrator may set up a specific configuration even that doesn't match
with DMI or whatever identifier.


Takashi

Re: [PATCH] ALSA: core - add more card sysfs entries

[Jaroslav Kysela]

Dne 09. 04. 21 v 9:39 Takashi Iwai napsal(a):
> On Thu, 08 Apr 2021 20:51:41 +0200,
> Pierre-Louis Bossart wrote:
>>
>>
>>
>>>>> When we have a common standard layer for the plug-and-play handling (udev), we
>>>>> should concentrate to allow changing / refining of this information there.
>>>>> Those strings are not used for anything else than the user space. So from my
>>>>> view, there's no reason to create another mechanism to handle the overrides.
>>>>> It should be a safe, fast, flexible and_optional_  solution. The udev can
>>>>> alter the sysfs attributes directly without any hassle with the file
>>>>> modifications or looking for another way to pass / store this information
>>>>> somewhere.
>>>>
>>>> There's one part where I am lost.
>>>>
>>>> The initial idea of udev what to modify kernel parameters to pick a
>>>> different path for firmware/topology before probing the PCI driver. At
>>>
>>> This may be a problematic point. The kernel cmdline cannot be modified from
>>> udev (as far as I know). The module parameters can be set using modprobe's
>>> config files or when loaded with sysfs attributes (/sys/module/*/parameters).
>>> Eventually, you can call the modprobe command with custom module parameters
>>> when the appropriate MODALIAS is probed.
>>>
>>> Perhaps, I'm missing something here, too. Some example udev rules may help.
>>
>> see the example shared by Curtis
>>
>> SUBSYSTEM=="pci", ATTR{vendor}=="0x8086", ATTR{device}=="0xa0c8",
>> ATTR{class}=="0x040100", ATTRS{[dmi/id]board_name}=="Eldrid",
>> RUN+="/sbin/modprobe snd_sof_pci tplg_path=intel/sof-tplg/pdm1"
>>
>> Those 'path' parameters would have to be set prior to creating the
>> card, making them writable via sysfs would not work, the firmware and
>> topology are already loaded and changing the paths would have no
>> effect.
> 
> Couldn't the driver probe the firmware files with some device-specific
> string suffix at first?  e.g. the driver can issue request_firmware()
> with $base_file-$dmi_board at first, then falls back to the generic
> $base_file.  A similar method was already used in Broadcom WiFi
> driver.
> 
> Also, the driver may do request_firmware() with a fixed path for the
> custom firmware at first (e.g. "intel/sof-tplg-custom"); then a system
> integrator may set up a specific configuration even that doesn't match
> with DMI or whatever identifier.

And when we have two firmware files which differs just by functionality
requested by user? Although your method will work, I won't close the
possibility to configure everything in udev rather using a hard coded fw load
scheme only.

						Jaroslav

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

Re: [PATCH] ALSA: core - add more card sysfs entries

[Takashi Iwai]

On Fri, 09 Apr 2021 10:34:03 +0200,
Jaroslav Kysela wrote:
> 
> Dne 09. 04. 21 v 9:39 Takashi Iwai napsal(a):
> > On Thu, 08 Apr 2021 20:51:41 +0200,
> > Pierre-Louis Bossart wrote:
> >>
> >>
> >>
> >>>>> When we have a common standard layer for the plug-and-play handling (udev), we
> >>>>> should concentrate to allow changing / refining of this information there.
> >>>>> Those strings are not used for anything else than the user space. So from my
> >>>>> view, there's no reason to create another mechanism to handle the overrides.
> >>>>> It should be a safe, fast, flexible and_optional_  solution. The udev can
> >>>>> alter the sysfs attributes directly without any hassle with the file
> >>>>> modifications or looking for another way to pass / store this information
> >>>>> somewhere.
> >>>>
> >>>> There's one part where I am lost.
> >>>>
> >>>> The initial idea of udev what to modify kernel parameters to pick a
> >>>> different path for firmware/topology before probing the PCI driver. At
> >>>
> >>> This may be a problematic point. The kernel cmdline cannot be modified from
> >>> udev (as far as I know). The module parameters can be set using modprobe's
> >>> config files or when loaded with sysfs attributes (/sys/module/*/parameters).
> >>> Eventually, you can call the modprobe command with custom module parameters
> >>> when the appropriate MODALIAS is probed.
> >>>
> >>> Perhaps, I'm missing something here, too. Some example udev rules may help.
> >>
> >> see the example shared by Curtis
> >>
> >> SUBSYSTEM=="pci", ATTR{vendor}=="0x8086", ATTR{device}=="0xa0c8",
> >> ATTR{class}=="0x040100", ATTRS{[dmi/id]board_name}=="Eldrid",
> >> RUN+="/sbin/modprobe snd_sof_pci tplg_path=intel/sof-tplg/pdm1"
> >>
> >> Those 'path' parameters would have to be set prior to creating the
> >> card, making them writable via sysfs would not work, the firmware and
> >> topology are already loaded and changing the paths would have no
> >> effect.
> > 
> > Couldn't the driver probe the firmware files with some device-specific
> > string suffix at first?  e.g. the driver can issue request_firmware()
> > with $base_file-$dmi_board at first, then falls back to the generic
> > $base_file.  A similar method was already used in Broadcom WiFi
> > driver.
> > 
> > Also, the driver may do request_firmware() with a fixed path for the
> > custom firmware at first (e.g. "intel/sof-tplg-custom"); then a system
> > integrator may set up a specific configuration even that doesn't match
> > with DMI or whatever identifier.
> 
> And when we have two firmware files which differs just by functionality
> requested by user? Although your method will work, I won't close the
> possibility to configure everything in udev rather using a hard coded fw load
> scheme only.

That can be achieved via a module option as Curtis's example, no?

That comes to a question at which stage you want to re-configure the
stuff.  I suppose that, if the system has been set up, then the
firmware cannot be changed on the fly; usually you have to rewind the
stuff and re-initialize from the beginning of the driver binding.
This corresponds to the device unbind and re-bind procedure
(equivalent with hot-unplug and hot-replug).  Then you can put your
change (e.g. set up module option or a dedicated firmware file)
between the unbind and rebind step.

If we make the driver setup as staged (i.e. waiting for the setup via
sysfs before moving to the actual firmware loading), a concern with
that is the fragility and complexity.  If the sysfs set up via sysfs
missed or went wrong, all goes south.


Takashi

Re: [PATCH] ALSA: core - add more card sysfs entries

[Amadeusz Sławiński]

On 4/9/2021 10:34 AM, Jaroslav Kysela wrote:
> Dne 09. 04. 21 v 9:39 Takashi Iwai napsal(a):
>> On Thu, 08 Apr 2021 20:51:41 +0200,
>> Pierre-Louis Bossart wrote:
>>>
>>>
>>>
>>>>>> When we have a common standard layer for the plug-and-play handling (udev), we
>>>>>> should concentrate to allow changing / refining of this information there.
>>>>>> Those strings are not used for anything else than the user space. So from my
>>>>>> view, there's no reason to create another mechanism to handle the overrides.
>>>>>> It should be a safe, fast, flexible and_optional_  solution. The udev can
>>>>>> alter the sysfs attributes directly without any hassle with the file
>>>>>> modifications or looking for another way to pass / store this information
>>>>>> somewhere.
>>>>>
>>>>> There's one part where I am lost.
>>>>>
>>>>> The initial idea of udev what to modify kernel parameters to pick a
>>>>> different path for firmware/topology before probing the PCI driver. At
>>>>
>>>> This may be a problematic point. The kernel cmdline cannot be modified from
>>>> udev (as far as I know). The module parameters can be set using modprobe's
>>>> config files or when loaded with sysfs attributes (/sys/module/*/parameters).
>>>> Eventually, you can call the modprobe command with custom module parameters
>>>> when the appropriate MODALIAS is probed.
>>>>
>>>> Perhaps, I'm missing something here, too. Some example udev rules may help.
>>>
>>> see the example shared by Curtis
>>>
>>> SUBSYSTEM=="pci", ATTR{vendor}=="0x8086", ATTR{device}=="0xa0c8",
>>> ATTR{class}=="0x040100", ATTRS{[dmi/id]board_name}=="Eldrid",
>>> RUN+="/sbin/modprobe snd_sof_pci tplg_path=intel/sof-tplg/pdm1"
>>>
>>> Those 'path' parameters would have to be set prior to creating the
>>> card, making them writable via sysfs would not work, the firmware and
>>> topology are already loaded and changing the paths would have no
>>> effect.
>>
>> Couldn't the driver probe the firmware files with some device-specific
>> string suffix at first?  e.g. the driver can issue request_firmware()
>> with $base_file-$dmi_board at first, then falls back to the generic
>> $base_file.  A similar method was already used in Broadcom WiFi
>> driver.
>>
>> Also, the driver may do request_firmware() with a fixed path for the
>> custom firmware at first (e.g. "intel/sof-tplg-custom"); then a system
>> integrator may set up a specific configuration even that doesn't match
>> with DMI or whatever identifier.
> 
> And when we have two firmware files which differs just by functionality
> requested by user? Although your method will work, I won't close the
> possibility to configure everything in udev rather using a hard coded fw load
> scheme only.
> 
> 						Jaroslav
> 

I've slept on it and now I think I see what you are trying to do.

1. Load FW dependent on platform/user settings
2. Load appropriate topology for FW
3. Have UCM for the FEs and controls exposed by driver


As for 1. I would say that FW should be loaded from one location
if there is some platform that requires special FW just add quirks, like 
it is done with every other driver, and if someone wants to build their 
own special FW, they just replace it. We can't possibly support hundreds 
of possible FW modifications if users want them by putting them in 
separate files. Alternatively allow override via kernel parameters.
Overriding FW files via udev would only make sense to me if it was 
possible to upload new FW at runtime.

I would say that same applies for 2.

This leaves number 3. which would require kernel exposing some kind of 
information about loaded topology, so user space can use proper UCM.
In topology manifest there are few reserved fields 
(https://elixir.bootlin.com/linux/latest/source/include/uapi/sound/asoc.h#L382), 
so we can add some information there which should be unique per topology 
and then expose it in userspace on topology load, it can be the name of 
UCM file topology wants to be loaded for example.

For example do something along those lines:

struct snd_soc_tplg_manifest {
	__le32 size;		/* in bytes of this structure */
	__le32 control_elems;	/* number of control elements */
	__le32 widget_elems;	/* number of widget elements */
	__le32 graph_elems;	/* number of graph elements */
	__le32 pcm_elems;	/* number of PCM elements */
	__le32 dai_link_elems;	/* number of DAI link elements */
	__le32 dai_elems;	/* number of physical DAI elements */
	__le32 ucm_files;	/* UCM files to use with topology */
	__le32 reserved[19];	/* reserved for new ABI element types */
	struct snd_soc_tplg_private priv;
} __attribute__((packed));

struct snd_soc_tplg_ucm_files {
	struct snd_soc_tplg_ctl_hdr hdr;
	__le32 size;	/* size of struct in bytes */
	__le32 count;	/* UCM entries */
	char ucms[SNDRV_CTL_ELEM_ID_NAME_MAXLEN][];
}

And then expose it somewhere under sysfs after parsing topology.

Re: [PATCH] ALSA: core - add more card sysfs entries

[Jaroslav Kysela]

Dne 09. 04. 21 v 11:09 Amadeusz Sławiński napsal(a):
> On 4/9/2021 10:34 AM, Jaroslav Kysela wrote:
>> Dne 09. 04. 21 v 9:39 Takashi Iwai napsal(a):
>>> On Thu, 08 Apr 2021 20:51:41 +0200,
>>> Pierre-Louis Bossart wrote:
>>>>
>>>>
>>>>
>>>>>>> When we have a common standard layer for the plug-and-play handling (udev), we
>>>>>>> should concentrate to allow changing / refining of this information there.
>>>>>>> Those strings are not used for anything else than the user space. So from my
>>>>>>> view, there's no reason to create another mechanism to handle the overrides.
>>>>>>> It should be a safe, fast, flexible and_optional_  solution. The udev can
>>>>>>> alter the sysfs attributes directly without any hassle with the file
>>>>>>> modifications or looking for another way to pass / store this information
>>>>>>> somewhere.
>>>>>>
>>>>>> There's one part where I am lost.
>>>>>>
>>>>>> The initial idea of udev what to modify kernel parameters to pick a
>>>>>> different path for firmware/topology before probing the PCI driver. At
>>>>>
>>>>> This may be a problematic point. The kernel cmdline cannot be modified from
>>>>> udev (as far as I know). The module parameters can be set using modprobe's
>>>>> config files or when loaded with sysfs attributes (/sys/module/*/parameters).
>>>>> Eventually, you can call the modprobe command with custom module parameters
>>>>> when the appropriate MODALIAS is probed.
>>>>>
>>>>> Perhaps, I'm missing something here, too. Some example udev rules may help.
>>>>
>>>> see the example shared by Curtis
>>>>
>>>> SUBSYSTEM=="pci", ATTR{vendor}=="0x8086", ATTR{device}=="0xa0c8",
>>>> ATTR{class}=="0x040100", ATTRS{[dmi/id]board_name}=="Eldrid",
>>>> RUN+="/sbin/modprobe snd_sof_pci tplg_path=intel/sof-tplg/pdm1"
>>>>
>>>> Those 'path' parameters would have to be set prior to creating the
>>>> card, making them writable via sysfs would not work, the firmware and
>>>> topology are already loaded and changing the paths would have no
>>>> effect.
>>>
>>> Couldn't the driver probe the firmware files with some device-specific
>>> string suffix at first?  e.g. the driver can issue request_firmware()
>>> with $base_file-$dmi_board at first, then falls back to the generic
>>> $base_file.  A similar method was already used in Broadcom WiFi
>>> driver.
>>>
>>> Also, the driver may do request_firmware() with a fixed path for the
>>> custom firmware at first (e.g. "intel/sof-tplg-custom"); then a system
>>> integrator may set up a specific configuration even that doesn't match
>>> with DMI or whatever identifier.
>>
>> And when we have two firmware files which differs just by functionality
>> requested by user? Although your method will work, I won't close the
>> possibility to configure everything in udev rather using a hard coded fw load
>> scheme only.
>>
>> 						Jaroslav
>>
> 
> I've slept on it and now I think I see what you are trying to do.
> 
> 1. Load FW dependent on platform/user settings
> 2. Load appropriate topology for FW
> 3. Have UCM for the FEs and controls exposed by driver
> 
> 
> As for 1. I would say that FW should be loaded from one location
> if there is some platform that requires special FW just add quirks, like 
> it is done with every other driver, and if someone wants to build their 
> own special FW, they just replace it. We can't possibly support hundreds 
> of possible FW modifications if users want them by putting them in 
> separate files. Alternatively allow override via kernel parameters.
> Overriding FW files via udev would only make sense to me if it was 
> possible to upload new FW at runtime.
> 
> I would say that same applies for 2.
> 
> This leaves number 3. which would require kernel exposing some kind of 
> information about loaded topology, so user space can use proper UCM.
> In topology manifest there are few reserved fields 
> (https://elixir.bootlin.com/linux/latest/source/include/uapi/sound/asoc.h#L382), 
> so we can add some information there which should be unique per topology 
> and then expose it in userspace on topology load, it can be the name of 
> UCM file topology wants to be loaded for example.
> 
> For example do something along those lines:
> 
> struct snd_soc_tplg_manifest {
> 	__le32 size;		/* in bytes of this structure */
> 	__le32 control_elems;	/* number of control elements */
> 	__le32 widget_elems;	/* number of widget elements */
> 	__le32 graph_elems;	/* number of graph elements */
> 	__le32 pcm_elems;	/* number of PCM elements */
> 	__le32 dai_link_elems;	/* number of DAI link elements */
> 	__le32 dai_elems;	/* number of physical DAI elements */
> 	__le32 ucm_files;	/* UCM files to use with topology */
> 	__le32 reserved[19];	/* reserved for new ABI element types */
> 	struct snd_soc_tplg_private priv;
> } __attribute__((packed));
> 
> struct snd_soc_tplg_ucm_files {
> 	struct snd_soc_tplg_ctl_hdr hdr;
> 	__le32 size;	/* size of struct in bytes */
> 	__le32 count;	/* UCM entries */
> 	char ucms[SNDRV_CTL_ELEM_ID_NAME_MAXLEN][];
> }
> 
> And then expose it somewhere under sysfs after parsing topology.

If the driver exports some extra information via sysfs, it can be already used
in the current UCM configs. I welcome any of this activity. It may ease
things. But it's a complement to my proposal to allow modify the sound card
identification in udev (not only for UCM). I believe that we can support
multiple ways and it's up to the maintainer / user of the specific distro to
select any.

					Jaroslav

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

Re: [PATCH] ALSA: core - add more card sysfs entries

[Pierre-Louis Bossart]

>>> Couldn't the driver probe the firmware files with some device-specific
>>> string suffix at first?  e.g. the driver can issue request_firmware()
>>> with $base_file-$dmi_board at first, then falls back to the generic
>>> $base_file.  A similar method was already used in Broadcom WiFi
>>> driver.
>>>
>>> Also, the driver may do request_firmware() with a fixed path for the
>>> custom firmware at first (e.g. "intel/sof-tplg-custom"); then a system
>>> integrator may set up a specific configuration even that doesn't match
>>> with DMI or whatever identifier.
>>
>> And when we have two firmware files which differs just by functionality
>> requested by user? Although your method will work, I won't close the
>> possibility to configure everything in udev rather using a hard coded 
>> fw load
>> scheme only.
>>
>>                         Jaroslav
>>
> 
> I've slept on it and now I think I see what you are trying to do.
> 
> 1. Load FW dependent on platform/user settings
> 2. Load appropriate topology for FW
> 3. Have UCM for the FEs and controls exposed by driver
> 
> 
> As for 1. I would say that FW should be loaded from one location
> if there is some platform that requires special FW just add quirks, like 
> it is done with every other driver, and if someone wants to build their 
> own special FW, they just replace it. We can't possibly support hundreds 
> of possible FW modifications if users want them by putting them in 
> separate files. Alternatively allow override via kernel parameters.
> Overriding FW files via udev would only make sense to me if it was 
> possible to upload new FW at runtime.

No, replacing firmware files is not viable.

Let me give you a practical example. In the course of SOF development, 
we routinely copy new test firmware+topology in the location of 
distribution-managed files. It's classic that when there is a 
distribution update in the background to install the latest SOF release, 
our test files are overwritten and it's not usual for developers to lose 
time trying to figure out why the functionality changed. We do need to 
have multiple paths and NEVER override what is provided by the 
distributions. it's the moral equivalent of /usr/bin v. /usr/local/bin.

Likewise, if an OEM has a custom version of firmware+topology, they may 
want to place it in their own packages that install *separately* from 
the default distribution. The udev rules provide a means to select the 
custom firmware, and if no rules are provided the default firmware with 
no customizations is used.

We are not talking about hundreds of configurations, but typically one 
directory per OEM. What they do internally is their problem, it's not 
for us to debate. The key point is that the custom files shall not 
interfere with the distribution baseline firmware, they have to be 
handled as separate packages that have their own release cycle and are 
possibly installed only in OEM custom images.

> I would say that same applies for 2.
> 
> This leaves number 3. which would require kernel exposing some kind of 
> information about loaded topology, so user space can use proper UCM.
> In topology manifest there are few reserved fields 
> (https://elixir.bootlin.com/linux/latest/source/include/uapi/sound/asoc.h#L382), 
> so we can add some information there which should be unique per topology 
> and then expose it in userspace on topology load, it can be the name of 
> UCM file topology wants to be loaded for example.
> 
> For example do something along those lines:
> 
> struct snd_soc_tplg_manifest {
>      __le32 size;        /* in bytes of this structure */
>      __le32 control_elems;    /* number of control elements */
>      __le32 widget_elems;    /* number of widget elements */
>      __le32 graph_elems;    /* number of graph elements */
>      __le32 pcm_elems;    /* number of PCM elements */
>      __le32 dai_link_elems;    /* number of DAI link elements */
>      __le32 dai_elems;    /* number of physical DAI elements */
>      __le32 ucm_files;    /* UCM files to use with topology */
>      __le32 reserved[19];    /* reserved for new ABI element types */
>      struct snd_soc_tplg_private priv;
> } __attribute__((packed));
> 
> struct snd_soc_tplg_ucm_files {
>      struct snd_soc_tplg_ctl_hdr hdr;
>      __le32 size;    /* size of struct in bytes */
>      __le32 count;    /* UCM entries */
>      char ucms[SNDRV_CTL_ELEM_ID_NAME_MAXLEN][];
> }
> 
> And then expose it somewhere under sysfs after parsing topology.

Yes I had a similar idea that we could expose in sysfs information taken 
from parsing the firmware (e.g. list of module UUIDs) and topology.

But it's not enough. The point is that you may use the same 
topology+firmware for different platforms, only the tuning varies. I 
think that's at that level that we should allow the OEMs to set a rule 
defining what the platform is and what tuning should be applied.

Re: [PATCH] ALSA: core - add more card sysfs entries

[Takashi Iwai]

On Fri, 09 Apr 2021 17:55:38 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> >>> Couldn't the driver probe the firmware files with some device-specific
> >>> string suffix at first?  e.g. the driver can issue request_firmware()
> >>> with $base_file-$dmi_board at first, then falls back to the generic
> >>> $base_file.  A similar method was already used in Broadcom WiFi
> >>> driver.
> >>>
> >>> Also, the driver may do request_firmware() with a fixed path for the
> >>> custom firmware at first (e.g. "intel/sof-tplg-custom"); then a system
> >>> integrator may set up a specific configuration even that doesn't match
> >>> with DMI or whatever identifier.
> >>
> >> And when we have two firmware files which differs just by functionality
> >> requested by user? Although your method will work, I won't close the
> >> possibility to configure everything in udev rather using a hard
> >> coded fw load
> >> scheme only.
> >>
> >>                         Jaroslav
> >>
> >
> > I've slept on it and now I think I see what you are trying to do.
> >
> > 1. Load FW dependent on platform/user settings
> > 2. Load appropriate topology for FW
> > 3. Have UCM for the FEs and controls exposed by driver
> >
> >
> > As for 1. I would say that FW should be loaded from one location
> > if there is some platform that requires special FW just add quirks,
> > like it is done with every other driver, and if someone wants to
> > build their own special FW, they just replace it. We can't possibly
> > support hundreds of possible FW modifications if users want them by
> > putting them in separate files. Alternatively allow override via
> > kernel parameters.
> > Overriding FW files via udev would only make sense to me if it was
> > possible to upload new FW at runtime.
> 
> No, replacing firmware files is not viable.
> 
> Let me give you a practical example. In the course of SOF development,
> we routinely copy new test firmware+topology in the location of
> distribution-managed files. It's classic that when there is a
> distribution update in the background to install the latest SOF
> release, our test files are overwritten and it's not usual for
> developers to lose time trying to figure out why the functionality
> changed. We do need to have multiple paths and NEVER override what is
> provided by the distributions. it's the moral equivalent of /usr/bin
> v. /usr/local/bin.

Use /lib/firmware/updates/*.  That precedes over the standard path
/lib/firmware/*.  (Also /lib/firmware/updates/$VERSION has even higher
priority.)


Takashi

Re: [PATCH] ALSA: core - add more card sysfs entries

[Pierre-Louis Bossart]

>> No, replacing firmware files is not viable.
>>
>> Let me give you a practical example. In the course of SOF development,
>> we routinely copy new test firmware+topology in the location of
>> distribution-managed files. It's classic that when there is a
>> distribution update in the background to install the latest SOF
>> release, our test files are overwritten and it's not usual for
>> developers to lose time trying to figure out why the functionality
>> changed. We do need to have multiple paths and NEVER override what is
>> provided by the distributions. it's the moral equivalent of /usr/bin
>> v. /usr/local/bin.
> 
> Use /lib/firmware/updates/*.  That precedes over the standard path
> /lib/firmware/*.  (Also /lib/firmware/updates/$VERSION has even higher
> priority.)

thanks for the feedback Takashi, I had no idea this even existed :-)

The documentation is here:
https://www.kernel.org/doc/html/latest/driver-api/firmware/fw_search_path.html

I guess that removes the need for udev rules to select the 
firmware+topology in simple cases, e.g. if you have only one custom 
version or an image overlay.

Re: [PATCH] ALSA: core - add more card sysfs entries

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1428 bytes --]

On Fri, Apr 09, 2021 at 10:55:38AM -0500, Pierre-Louis Bossart wrote:

> > For example do something along those lines:

> > struct snd_soc_tplg_manifest {
> >      __le32 size;        /* in bytes of this structure */
> >      __le32 control_elems;    /* number of control elements */

...

> > struct snd_soc_tplg_ucm_files {
> >      struct snd_soc_tplg_ctl_hdr hdr;
> >      __le32 size;    /* size of struct in bytes */
> >      __le32 count;    /* UCM entries */
> >      char ucms[SNDRV_CTL_ELEM_ID_NAME_MAXLEN][];
> > }

> > And then expose it somewhere under sysfs after parsing topology.

> Yes I had a similar idea that we could expose in sysfs information taken
> from parsing the firmware (e.g. list of module UUIDs) and topology.

> But it's not enough. The point is that you may use the same
> topology+firmware for different platforms, only the tuning varies. I think
> that's at that level that we should allow the OEMs to set a rule defining
> what the platform is and what tuning should be applied.

So the issue there is trying to put a list of UCM files in the kernel or
topology and exposing that (which I agree doesn't seem like something
that should come from the kernel), but exposing the information about
what's in the loaded topology file seems reasonable.  It's just more
information that userspace has available to key off when deciding what
it wants to do isn't it?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ALSA: core - add more card sysfs entries

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3362 bytes --]

On Thu, Apr 08, 2021 at 05:01:14PM +0200, Jaroslav Kysela wrote:

> There are two things to consider (please, don't concentrate to UCM here):

> 1) the card identification
> 2) the user experience

> Actually, the generic ASoC drivers are too much generic and they didn't
> provide a solid information about the hardware.

So if the information provided through the driver is too generic then we
should ideally be fixing those drivers/systems to do something sensible.
For the DT systems the generic cards have properties that let the system
just specify names directly so it will just be a case of setting them
properly and it should just be the x86 systems that are a problem.  ACPI
is a bit of a lost cause here, most of the systems aren't interested in
supporting Linux in the first place and the idioms there aren't great,
but for DT it's reasonably tractable to push back on people if there's
issues and it's much more scalable to do that than telling individual
users to do that.

> As you can see, the information for both drivers is quite inaccurate and users
> (including me) may want some flexibility to change those strings to something
> else. It may resolve some UCM problems, but it's just one small piece of the
> issue.

It feels like if we're conflating end user configuration and what we're
reporting to userspace for userspace to key off we're going to end up
causing confusion and/or tying ourselves in knots when people update
both places, for example if you're trying to figure out which
configuration was used but the values used to select the configuration
that was used have changed.

> Another issue is when the udev driver loader can change some parameters which
> modifies the driver behaviour and sound device structure for the given card
> (as discussed in another thread).

Not sure I've seen that thread, I've certainly not seen anything on
github.

> When we have a common standard layer for the plug-and-play handling (udev), we
> should concentrate to allow changing / refining of this information there.
> Those strings are not used for anything else than the user space. So from my
> view, there's no reason to create another mechanism to handle the overrides.
> It should be a safe, fast, flexible and _optional_ solution. The udev can
> alter the sysfs attributes directly without any hassle with the file
> modifications or looking for another way to pass / store this information
> somewhere.

We could add a new sysfs file for user overrides, or alternatively have
a new non-overridable file which contains whatever the kernel would set
by default so it's always available in case things start to get
confused (ie, you can always look at the original value even if it's
been overwritten later)?

> > Didn't we discuss in the past about the possibility to store the
> > profile name in the card component string?

> It's already possible to extract any information from the components string.
> The current UCM is very flexible, but it does not allow to use a missing
> information.

> The main questions is: Can we make those strings writable or not? What
> prevents us to not allow that?

Like I say I'm nervous about potential confusion if we allow people to
change things that were already set and used by the kernel, plus the
general desire to encourage better quality of implementation from
firmware where we can.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ALSA: core - add more card sysfs entries

[Pierre-Louis Bossart]

On 4/8/21 11:22 AM, Mark Brown wrote:
>> Actually, the generic ASoC drivers are too much generic and they didn't
>> provide a solid information about the hardware.
> So if the information provided through the driver is too generic then we
> should ideally be fixing those drivers/systems to do something sensible.
> For the DT systems the generic cards have properties that let the system
> just specify names directly so it will just be a case of setting them
> properly and it should just be the x86 systems that are a problem.  ACPI
> is a bit of a lost cause here, most of the systems aren't interested in
> supporting Linux in the first place and the idioms there aren't great,
> but for DT it's reasonably tractable to push back on people if there's
> issues and it's much more scalable to do that than telling individual
> users to do that.

Even in the DT case, you may be able to set a specific path for DSP 
firmware and topology but would you really have enough information to 
describe what the DSP firmware and topology actually do? That 
information is part of the DSP firmware manifest and topology.

In addition, the firmware/topology are typically located on the file 
system, it'd be a hassle to have to edit DT properties every time you 
have a new distribution update, wouldn't it?

Re: [PATCH] ALSA: core - add more card sysfs entries

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 846 bytes --]

On Thu, Apr 08, 2021 at 11:50:08AM -0500, Pierre-Louis Bossart wrote:

> Even in the DT case, you may be able to set a specific path for DSP firmware
> and topology but would you really have enough information to describe what
> the DSP firmware and topology actually do? That information is part of the
> DSP firmware manifest and topology.

I'd have hoped that for information on firmware we'd be keying off
descriptive information included in the firmware/topology itself,
ideally the kernel would be able to do that itself.

> In addition, the firmware/topology are typically located on the file system,
> it'd be a hassle to have to edit DT properties every time you have a new
> distribution update, wouldn't it?

You definitely shouldn't be doing that, but I'd also not expect us to
have to update anything that isn't the firmware itself.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ALSA: core - add more card sysfs entries

[Jaroslav Kysela]

Dne 08. 04. 21 v 18:22 Mark Brown napsal(a):
> On Thu, Apr 08, 2021 at 05:01:14PM +0200, Jaroslav Kysela wrote:
> 
>> There are two things to consider (please, don't concentrate to UCM here):
> 
>> 1) the card identification
>> 2) the user experience
> 
>> Actually, the generic ASoC drivers are too much generic and they didn't
>> provide a solid information about the hardware.
> 
> So if the information provided through the driver is too generic then we
> should ideally be fixing those drivers/systems to do something sensible.
> For the DT systems the generic cards have properties that let the system
> just specify names directly so it will just be a case of setting them
> properly and it should just be the x86 systems that are a problem.  ACPI
> is a bit of a lost cause here, most of the systems aren't interested in
> supporting Linux in the first place and the idioms there aren't great,
> but for DT it's reasonably tractable to push back on people if there's
> issues and it's much more scalable to do that than telling individual
> users to do that.

DT works only partially. Also, you need the DT compiler to change something,
it's easier to overwrite things in the booted system. The user experience is
lower with DT.

And as Pierre-louis noted, it may be possible to modify the firmware setup in
udev.

>> As you can see, the information for both drivers is quite inaccurate and users
>> (including me) may want some flexibility to change those strings to something
>> else. It may resolve some UCM problems, but it's just one small piece of the
>> issue.
> 
> It feels like if we're conflating end user configuration and what we're
> reporting to userspace for userspace to key off we're going to end up
> causing confusion and/or tying ourselves in knots when people update
> both places, for example if you're trying to figure out which
> configuration was used but the values used to select the configuration
> that was used have changed.
> 
>> Another issue is when the udev driver loader can change some parameters which
>> modifies the driver behaviour and sound device structure for the given card
>> (as discussed in another thread).
> 
> Not sure I've seen that thread, I've certainly not seen anything on
> github.

I meant the e-mail replies to this thread started by Pierre-louis about OEM
firmwares for the SOF driver. Sorry for the confusion.

>> When we have a common standard layer for the plug-and-play handling (udev), we
>> should concentrate to allow changing / refining of this information there.
>> Those strings are not used for anything else than the user space. So from my
>> view, there's no reason to create another mechanism to handle the overrides.
>> It should be a safe, fast, flexible and _optional_ solution. The udev can
>> alter the sysfs attributes directly without any hassle with the file
>> modifications or looking for another way to pass / store this information
>> somewhere.
> 
> We could add a new sysfs file for user overrides, or alternatively have
> a new non-overridable file which contains whatever the kernel would set
> by default so it's always available in case things start to get
> confused (ie, you can always look at the original value even if it's
> been overwritten later)?

It's an interesting idea. Preserving the original strings may be wanted.

					Jaroslav

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

Re: [PATCH] ALSA: core - add more card sysfs entries

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]

On Thu, Apr 08, 2021 at 07:20:39PM +0200, Jaroslav Kysela wrote:
> Dne 08. 04. 21 v 18:22 Mark Brown napsal(a):

> > So if the information provided through the driver is too generic then we
> > should ideally be fixing those drivers/systems to do something sensible.
> > For the DT systems the generic cards have properties that let the system
> > just specify names directly so it will just be a case of setting them
> > properly and it should just be the x86 systems that are a problem.  ACPI
> > is a bit of a lost cause here, most of the systems aren't interested in
> > supporting Linux in the first place and the idioms there aren't great,
> > but for DT it's reasonably tractable to push back on people if there's
> > issues and it's much more scalable to do that than telling individual
> > users to do that.

> DT works only partially. Also, you need the DT compiler to change something,
> it's easier to overwrite things in the booted system. The user experience is
> lower with DT.

TBH I think the ship sailed on user experience no matter what we do here :)
In any case no disagreement that it's useful to have some way to do this
at runtime for systems where it's not possible to update the firmware
for whatever reason.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ALSA: core - add more card sysfs entries

[Jaroslav Kysela]

Dne 08. 04. 21 v 12:38 Takashi Sakamoto napsal(a):

>>  static DEVICE_ATTR(number, 0444, card_number_show_attr, NULL);
> 
> Use DEVICE_ATTR_RO() instead.

My proposal is consistent with the existing code. The cleanups should go on top.

					Jaroslav

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

Re: [PATCH] ALSA: core - add more card sysfs entries

[Jaroslav Kysela]

Dne 08. 04. 21 v 11:43 Jaroslav Kysela napsal(a):
> There are several strings which are describing the card. As time goes,
> we have new universal drivers which probe components in a way, which
> is disassociated from the card structure (ASoC). Also, some drivers
> may require to select another firmware depending on the specific
> platform using udev. The new firmware may change the sound card behaviour.
> 
> This patch allows flexible modifications of the card description
> from the user space to handle the specific boot / plug-in settings.

The original discussion went to different forks, but I'd like summarize some
points:

1) those runtime changes may be intrusive
2) even if we allow to change those strings, we should preserve the original
3) generate change events

I hope that we all see the flexibility to change those strings via udev. The
nice thing is that we can write to those sysfs attributes before the _control
device file_ is created by udev (thus we are pretty sure, that no applications
are using this information (if I omit the additional proc and sysfs resources).

It seems to me, that we may just add a policy how to handle those card
identification changes. This policy may be controlled using a module parameter
(runtime), kernel configuration option (compiled default). The policies will
be 'allow changes' and 'disallow changes' so distributions or users may be
forced to explicitly enable this behaviour. When all sysfs changes are
finished, the udev rules may just set "do not allow other changes" via an
additional sysfs sound card attribute (write once) to prevent user errors. Or
we may apply the 'write once' mechanism for all strings separately. We should
disallow to change the card identification after this point.

Regarding the original value preservation - the udev can save the original
strings to it's device environment variables for the later usage. We may
handle this in the kernel. but I see the code reduction with this idea and the
use will be rate (alsa-info script may be extended to extract this info from
the udev database).

The change events are not necessary with this policy. The card identification
is supposed to be changed only via udev before any application is active.

					Jaroslav

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