[PATCH] ALSA: core: sysfs: show components string

[PATCH] ALSA: core: sysfs: show components string

[Pierre-Louis Bossart]

Add attribute and show the components string. This is useful to see
what is provided to userspace and e.g. used by UCM to understand the
card configuration:

root@plb:~# more /sys/class/sound/card0/components
HDA:8086280b,80860101,00100000 cfg-spk:2 hs:rt711 spk:rt1308 mic:rt715

Note that the style uses what's recommended by checkpatch.pl and is
slightly different from other sysfs attributes.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/core/init.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/sound/core/init.c b/sound/core/init.c
index b02a99766351..decaf944a8ad 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -695,9 +695,21 @@ card_number_show_attr(struct device *dev,
 
 static DEVICE_ATTR(number, 0444, card_number_show_attr, NULL);
 
+static ssize_t
+components_show(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 DEVICE_ATTR_RO(components);
+
 static struct attribute *card_dev_attrs[] = {
 	&dev_attr_id.attr,
 	&dev_attr_number.attr,
+	&dev_attr_components.attr,
 	NULL
 };
 
-- 
2.20.1

Re: [PATCH] ALSA: core: sysfs: show components string

[Mark Brown]

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

On Mon, Mar 23, 2020 at 02:36:23PM -0500, Pierre-Louis Bossart wrote:
> Add attribute and show the components string. This is useful to see
> what is provided to userspace and e.g. used by UCM to understand the
> card configuration:
> 
> root@plb:~# more /sys/class/sound/card0/components
> HDA:8086280b,80860101,00100000 cfg-spk:2 hs:rt711 spk:rt1308 mic:rt715

Sysfs is supposed to be one value per file so this should be a directory
with a file for each component I guess.

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

Re: [PATCH] ALSA: core: sysfs: show components string

[Pierre-Louis Bossart]

On 3/23/20 2:41 PM, Mark Brown wrote:
> On Mon, Mar 23, 2020 at 02:36:23PM -0500, Pierre-Louis Bossart wrote:
>> Add attribute and show the components string. This is useful to see
>> what is provided to userspace and e.g. used by UCM to understand the
>> card configuration:
>>
>> root@plb:~# more /sys/class/sound/card0/components
>> HDA:8086280b,80860101,00100000 cfg-spk:2 hs:rt711 spk:rt1308 mic:rt715
> 
> Sysfs is supposed to be one value per file so this should be a directory
> with a file for each component I guess.

that's fair, but the 'value' is already a string here. There's no syntax 
or grammar that would define what the contents of the string would be, 
so it'd be difficult to define any sort of ABI. Are you saying sysfs 
can't be used here?

Re: [PATCH] ALSA: core: sysfs: show components string

[Takashi Iwai]

On Mon, 23 Mar 2020 20:41:42 +0100,
Mark Brown wrote:
> 
> On Mon, Mar 23, 2020 at 02:36:23PM -0500, Pierre-Louis Bossart wrote:
> > Add attribute and show the components string. This is useful to see
> > what is provided to userspace and e.g. used by UCM to understand the
> > card configuration:
> > 
> > root@plb:~# more /sys/class/sound/card0/components
> > HDA:8086280b,80860101,00100000 cfg-spk:2 hs:rt711 spk:rt1308 mic:rt715
> 
> Sysfs is supposed to be one value per file so this should be a directory
> with a file for each component I guess.

Well, the actual representation of components itself is CSV, so this
can be OK, IMO.


Takashi

Re: [PATCH] ALSA: core: sysfs: show components string

[Takashi Sakamoto]

Hi,

On Mon, Mar 23, 2020 at 02:36:23PM -0500, Pierre-Louis Bossart wrote:
> Add attribute and show the components string. This is useful to see
> what is provided to userspace and e.g. used by UCM to understand the
> card configuration:
> 
> root@plb:~# more /sys/class/sound/card0/components
> HDA:8086280b,80860101,00100000 cfg-spk:2 hs:rt711 spk:rt1308 mic:rt715
> 
> Note that the style uses what's recommended by checkpatch.pl and is
> slightly different from other sysfs attributes.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  sound/core/init.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
 
I have a concern about this patch in a point of userspace interface.

The 'component' field of 'struct snd_ctl_card_info' is just defined to
have strings with space-separated chunks, and its actual value is not
so fine-documented, thus it's largely different depending of developers
of each driver.

$ cat include/uapi/sound/asound.h
     ...
 941 struct snd_ctl_card_info {
         ...
 950     unsigned char components[128];  /* card components / fine identification, delimited with one space (AC97 etc..) */
 951 };

On the other hand, the node of sysfs is quite common in Linux because
it's tightly coupled to kernel objects. Let you see files under
'Documentation/ABI/'. We can find efforts to maintain sysfs node so
safe and stable. Due to this reason, it's better to take more care when
adding new node.

Would I request you the reason to add this node and the reason that
current ALSA control interface doesn't satisfy your requirement?

> diff --git a/sound/core/init.c b/sound/core/init.c
> index b02a99766351..decaf944a8ad 100644
> --- a/sound/core/init.c
> +++ b/sound/core/init.c
> @@ -695,9 +695,21 @@ card_number_show_attr(struct device *dev,
>  
>  static DEVICE_ATTR(number, 0444, card_number_show_attr, NULL);
>  
> +static ssize_t
> +components_show(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 DEVICE_ATTR_RO(components);
> +
>  static struct attribute *card_dev_attrs[] = {
>  	&dev_attr_id.attr,
>  	&dev_attr_number.attr,
> +	&dev_attr_components.attr,
>  	NULL
>  };
>  
> -- 
> 2.20.1
 

Regards

Takashi Sakamoto

Re: [PATCH] ALSA: core: sysfs: show components string

[Pierre-Louis Bossart]

On 3/23/20 8:53 PM, Takashi Sakamoto wrote:
> Hi,
> 
> On Mon, Mar 23, 2020 at 02:36:23PM -0500, Pierre-Louis Bossart wrote:
>> Add attribute and show the components string. This is useful to see
>> what is provided to userspace and e.g. used by UCM to understand the
>> card configuration:
>>
>> root@plb:~# more /sys/class/sound/card0/components
>> HDA:8086280b,80860101,00100000 cfg-spk:2 hs:rt711 spk:rt1308 mic:rt715
>>
>> Note that the style uses what's recommended by checkpatch.pl and is
>> slightly different from other sysfs attributes.
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   sound/core/init.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>   
> I have a concern about this patch in a point of userspace interface.
> 
> The 'component' field of 'struct snd_ctl_card_info' is just defined to
> have strings with space-separated chunks, and its actual value is not
> so fine-documented, thus it's largely different depending of developers
> of each driver.

In case you missed it, the components are used by machine drivers to 
report e.g. number of speakers, mics, etc, so that UCM can find the 
right configuration. For a given family of products, the syntax will be 
fixed, e.g. hs stands for headset codec, etc.

> $ cat include/uapi/sound/asound.h
>       ...
>   941 struct snd_ctl_card_info {
>           ...
>   950     unsigned char components[128];  /* card components / fine identification, delimited with one space (AC97 etc..) */
>   951 };
> 
> On the other hand, the node of sysfs is quite common in Linux because
> it's tightly coupled to kernel objects. Let you see files under
> 'Documentation/ABI/'. We can find efforts to maintain sysfs node so
> safe and stable. Due to this reason, it's better to take more care when
> adding new node.
> 
> Would I request you the reason to add this node and the reason that
> current ALSA control interface doesn't satisfy your requirement?

simplicity for user support. Anyone can peak at a sysfs file, not 
everyone is familiar with the alsa control interface.

We get lots of bug reports from people who are asking for configuration 
help, and the simpler the command is the better.

>> diff --git a/sound/core/init.c b/sound/core/init.c
>> index b02a99766351..decaf944a8ad 100644
>> --- a/sound/core/init.c
>> +++ b/sound/core/init.c
>> @@ -695,9 +695,21 @@ card_number_show_attr(struct device *dev,
>>   
>>   static DEVICE_ATTR(number, 0444, card_number_show_attr, NULL);
>>   
>> +static ssize_t
>> +components_show(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 DEVICE_ATTR_RO(components);
>> +
>>   static struct attribute *card_dev_attrs[] = {
>>   	&dev_attr_id.attr,
>>   	&dev_attr_number.attr,
>> +	&dev_attr_components.attr,
>>   	NULL
>>   };
>>   
>> -- 
>> 2.20.1
>   
> 
> Regards
> 
> Takashi Sakamoto
> 

Re: [PATCH] ALSA: core: sysfs: show components string

[Takashi Sakamoto]

On Mon, Mar 23, 2020 at 10:34:23PM -0500, Pierre-Louis Bossart wrote:
> On 3/23/20 8:53 PM, Takashi Sakamoto wrote:
> > Hi,
> > 
> > On Mon, Mar 23, 2020 at 02:36:23PM -0500, Pierre-Louis Bossart wrote:
> > > Add attribute and show the components string. This is useful to see
> > > what is provided to userspace and e.g. used by UCM to understand the
> > > card configuration:
> > > 
> > > root@plb:~# more /sys/class/sound/card0/components
> > > HDA:8086280b,80860101,00100000 cfg-spk:2 hs:rt711 spk:rt1308 mic:rt715
> > > 
> > > Note that the style uses what's recommended by checkpatch.pl and is
> > > slightly different from other sysfs attributes.
> > > 
> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > ---
> > >   sound/core/init.c | 12 ++++++++++++
> > >   1 file changed, 12 insertions(+)
> > I have a concern about this patch in a point of userspace interface.
> > 
> > The 'component' field of 'struct snd_ctl_card_info' is just defined to
> > have strings with space-separated chunks, and its actual value is not
> > so fine-documented, thus it's largely different depending of developers
> > of each driver.
> 
> In case you missed it, the components are used by machine drivers to report
> e.g. number of speakers, mics, etc, so that UCM can find the right
> configuration. For a given family of products, the syntax will be fixed,
> e.g. hs stands for headset codec, etc.
 
Actually the syntax is just fixed to devices which a part of ALSA
in-kernel driver supports. There's no specification widely used to all of
ALSA in-kernel drivers.

(Of cource, it covers many of actual devices which assumed users own.)

> > $ cat include/uapi/sound/asound.h
> >       ...
> >   941 struct snd_ctl_card_info {
> >           ...
> >   950     unsigned char components[128];  /* card components / fine identification, delimited with one space (AC97 etc..) */
> >   951 };
> > 
> > On the other hand, the node of sysfs is quite common in Linux because
> > it's tightly coupled to kernel objects. Let you see files under
> > 'Documentation/ABI/'. We can find efforts to maintain sysfs node so
> > safe and stable. Due to this reason, it's better to take more care when
> > adding new node.
> > 
> > Would I request you the reason to add this node and the reason that
> > current ALSA control interface doesn't satisfy your requirement?
> 
> simplicity for user support. Anyone can peak at a sysfs file, not everyone
> is familiar with the alsa control interface.
> 
> We get lots of bug reports from people who are asking for configuration
> help, and the simpler the command is the better.

For my information, would I request you to disclose the part of such reports?

I need supplemental information about the reason to add the alternative
path to expose it, especially the reason that no developers work to
improve existent tool relevant to UCM and are going to wish to add the
alternative without utilizing ALSA control character device.


Regards

Takashi Sakamoto

Re: [PATCH] ALSA: core: sysfs: show components string

[Pierre-Louis Bossart]

>>> On the other hand, the node of sysfs is quite common in Linux because
>>> it's tightly coupled to kernel objects. Let you see files under
>>> 'Documentation/ABI/'. We can find efforts to maintain sysfs node so
>>> safe and stable. Due to this reason, it's better to take more care when
>>> adding new node.
>>>
>>> Would I request you the reason to add this node and the reason that
>>> current ALSA control interface doesn't satisfy your requirement?
>>
>> simplicity for user support. Anyone can peak at a sysfs file, not everyone
>> is familiar with the alsa control interface.
>>
>> We get lots of bug reports from people who are asking for configuration
>> help, and the simpler the command is the better.
> 
> For my information, would I request you to disclose the part of such reports?
> 
> I need supplemental information about the reason to add the alternative
> path to expose it, especially the reason that no developers work to
> improve existent tool relevant to UCM and are going to wish to add the
> alternative without utilizing ALSA control character device.

I don't understand your question, sorry. UCM already uses the control 
interface, it's not a matter of adding a new interface but making it 
easier to visualize the contents reported by the machine driver.

See for example 
https://github.com/alsa-project/alsa-ucm-conf/blob/4722f5b3859903521ba0f92a64d86af31083ca50/ucm2/sof-hda-dsp/HiFi.conf#L145

when people report that their microphone is not reported by 
PulseAudio/UCM, it's very helpful to know what UCM was supposed to use 
in the first place. We don't have a debugger or step-by-step mechanisms 
to figure out what the configurations are.

There is zero intent to advertise this sysfs node as a basis for 
applications to bypass the control interface, if that was what you 
thought I was promoting.

Re: [PATCH] ALSA: core: sysfs: show components string

[Takashi Sakamoto]

On Tue, Mar 24, 2020 at 12:12:15AM -0500, Pierre-Louis Bossart wrote:
> when people report that their microphone is not reported by PulseAudio/UCM,
> it's very helpful to know what UCM was supposed to use in the first place.
> We don't have a debugger or step-by-step mechanisms to figure out what the
> configurations are.

If I get your intension correctly, the addition of sysfs node is just to
investigate which use-case configuration is applied in cases that people
get issues. If so, it's really exaggerative in a point of the concept of
sysfs.

I have two alternatives. If it's possible to focus on ALSA SoC part only,
addition of node to debugfs is reasonable for this purpose.

Another alternative is to change output of 'cards' node of procfs. The
latter is commonly available for all cases. For example:

diff --git a/sound/core/init.c b/sound/core/init.c
index b02a99766351..9a04974c1d19 100644
--- a/sound/core/init.c
+++ b/sound/core/init.c
@@ -805,6 +805,8 @@ static void snd_card_info_read(struct snd_info_entry *entry,
                                        card->shortname);
                        snd_iprintf(buffer, "                      %s\n",
                                        card->longname);
+                       snd_iprintf(buffer, "                      %s\n",
+                                       card->component);
                }
                mutex_unlock(&snd_card_mutex);
        }

(If you're investigating to use rules of udevd for automated application
of UCM-based operation, there's a space to investigate the merit to expose
information via sysfs node. But actually you're not...)


Regards

Takashi Sakamoto

Re: [PATCH] ALSA: core: sysfs: show components string

[Pierre-Louis Bossart]

On 3/24/20 4:01 AM, Takashi Sakamoto wrote:
> On Tue, Mar 24, 2020 at 12:12:15AM -0500, Pierre-Louis Bossart wrote:
>> when people report that their microphone is not reported by PulseAudio/UCM,
>> it's very helpful to know what UCM was supposed to use in the first place.
>> We don't have a debugger or step-by-step mechanisms to figure out what the
>> configurations are.
> 
> If I get your intension correctly, the addition of sysfs node is just to
> investigate which use-case configuration is applied in cases that people
> get issues. If so, it's really exaggerative in a point of the concept of
> sysfs.
> 
> I have two alternatives. If it's possible to focus on ALSA SoC part only,
> addition of node to debugfs is reasonable for this purpose.
> 
> Another alternative is to change output of 'cards' node of procfs. The
> latter is commonly available for all cases. For example:

I initially wanted to use /proc but thought it was a thing from the past 
so I looked at sysfs. If this is the recommendation I don't mind using it.

debugsfs is not something the average user is familiar with, and it's 
not available in all cases. I'd like to extend existing pieces of 
information than add new things.

Re: [PATCH] ALSA: core: sysfs: show components string

[Jaroslav Kysela]

Dne 24. 03. 20 v 14:15 Pierre-Louis Bossart napsal(a):
> 
> 
> On 3/24/20 4:01 AM, Takashi Sakamoto wrote:
>> On Tue, Mar 24, 2020 at 12:12:15AM -0500, Pierre-Louis Bossart wrote:
>>> when people report that their microphone is not reported by PulseAudio/UCM,
>>> it's very helpful to know what UCM was supposed to use in the first place.
>>> We don't have a debugger or step-by-step mechanisms to figure out what the
>>> configurations are.
>>
>> If I get your intension correctly, the addition of sysfs node is just to
>> investigate which use-case configuration is applied in cases that people
>> get issues. If so, it's really exaggerative in a point of the concept of
>> sysfs.
>>
>> I have two alternatives. If it's possible to focus on ALSA SoC part only,
>> addition of node to debugfs is reasonable for this purpose.
>>
>> Another alternative is to change output of 'cards' node of procfs. The
>> latter is commonly available for all cases. For example:
> 
> I initially wanted to use /proc but thought it was a thing from the past
> so I looked at sysfs. If this is the recommendation I don't mind using it.
> 
> debugsfs is not something the average user is familiar with, and it's
> not available in all cases. I'd like to extend existing pieces of
> information than add new things.

I won't modify /proc/asound/cards in this case, but create a per card file 
like /proc/asound/card#/components .

				Thanks,
					Jaroslav

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

Re: [PATCH] ALSA: core: sysfs: show components string

[Takashi Iwai]

On Tue, 24 Mar 2020 14:15:44 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> 
> On 3/24/20 4:01 AM, Takashi Sakamoto wrote:
> > On Tue, Mar 24, 2020 at 12:12:15AM -0500, Pierre-Louis Bossart wrote:
> >> when people report that their microphone is not reported by PulseAudio/UCM,
> >> it's very helpful to know what UCM was supposed to use in the first place.
> >> We don't have a debugger or step-by-step mechanisms to figure out what the
> >> configurations are.
> >
> > If I get your intension correctly, the addition of sysfs node is just to
> > investigate which use-case configuration is applied in cases that people
> > get issues. If so, it's really exaggerative in a point of the concept of
> > sysfs.
> >
> > I have two alternatives. If it's possible to focus on ALSA SoC part only,
> > addition of node to debugfs is reasonable for this purpose.
> >
> > Another alternative is to change output of 'cards' node of procfs. The
> > latter is commonly available for all cases. For example:
> 
> I initially wanted to use /proc but thought it was a thing from the
> past so I looked at sysfs. If this is the recommendation I don't mind
> using it.

procfs will practically never die, and it's already there, so I'm fine
with that path, too, supposing that the primary purpose is for help
debugging / analyzing.  If it's used by UCM or whatever configuration
tool, sysfs is the better choice, OTOH.

> debugsfs is not something the average user is familiar with, and it's
> not available in all cases. I'd like to extend existing pieces of
> information than add new things.

Right, debugfs isn't available per card as default, so it's no good
option.


Takashi

Re: [PATCH] ALSA: core: sysfs: show components string

[Pierre-Louis Bossart]

>>>> when people report that their microphone is not reported by PulseAudio/UCM,
>>>> it's very helpful to know what UCM was supposed to use in the first place.
>>>> We don't have a debugger or step-by-step mechanisms to figure out what the
>>>> configurations are.
>>>
>>> If I get your intension correctly, the addition of sysfs node is just to
>>> investigate which use-case configuration is applied in cases that people
>>> get issues. If so, it's really exaggerative in a point of the concept of
>>> sysfs.
>>>
>>> I have two alternatives. If it's possible to focus on ALSA SoC part only,
>>> addition of node to debugfs is reasonable for this purpose.
>>>
>>> Another alternative is to change output of 'cards' node of procfs. The
>>> latter is commonly available for all cases. For example:
>>
>> I initially wanted to use /proc but thought it was a thing from the
>> past so I looked at sysfs. If this is the recommendation I don't mind
>> using it.
> 
> procfs will practically never die, and it's already there, so I'm fine
> with that path, too, supposing that the primary purpose is for help
> debugging / analyzing.  If it's used by UCM or whatever configuration
> tool, sysfs is the better choice, OTOH.
> 
>> debugsfs is not something the average user is familiar with, and it's
>> not available in all cases. I'd like to extend existing pieces of
>> information than add new things.
> 
> Right, debugfs isn't available per card as default, so it's no good
> option.

ok, let's go with procfs then, thanks for the feedback. I'll work on an 
update and resubmit.
-Pierre

Re: [PATCH] ALSA: core: sysfs: show components string

[Mark Brown]

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

On Mon, Mar 23, 2020 at 10:34:23PM -0500, Pierre-Louis Bossart wrote:

> In case you missed it, the components are used by machine drivers to report
> e.g. number of speakers, mics, etc, so that UCM can find the right
> configuration. For a given family of products, the syntax will be fixed,
> e.g. hs stands for headset codec, etc.

If that's what you're looking for it sounds like a richer sysfs ABI
which has these things in it more directly would be a more idiomatic fit
(or like Sakamoto-san says adding controls, though that is a barrier to
things like udev and so on like you say).

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