[PATCH v2] ASoC: core: clarify the driver name initialization

[PATCH v2] ASoC: core: clarify the driver name initialization

[Jaroslav Kysela]

The driver field in the struct snd_ctl_card_info is a valid
user space identifier. Actually, many ASoC drivers do not care
and let to initialize this field using a standard wrapping method.
Unfortunately, in this way, this field becomes unusable and
unreadable for the drivers with longer card names. Also,
there is a possibility to have clashes (driver field has
only limit of 15 characters).

This change will print an error when the wrapping is used.
The developers of the affected drivers should fix the problem.

Signed-off-by: Jaroslav Kysela <perex@perex.cz>

v1..v2:
  - remove the wrong DMI condition per Mark's review
---
 sound/soc/soc-core.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index e824ff1a9fc0..590143ebf6df 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1840,21 +1840,22 @@ static void soc_check_tplg_fes(struct snd_soc_card *card)
 	}
 }
 
-#define soc_setup_card_name(name, name1, name2, norm)		\
-	__soc_setup_card_name(name, sizeof(name), name1, name2, norm)
-static void __soc_setup_card_name(char *name, int len,
-				  const char *name1, const char *name2,
-				  int normalization)
+#define soc_setup_card_name(card, name, name1, name2) \
+	__soc_setup_card_name(card, name, sizeof(name), name1, name2)
+static void __soc_setup_card_name(struct snd_soc_card *card,
+				  char *name, int len,
+				  const char *name1, const char *name2)
 {
+	const char *src = name1 ? name1 : name2;
 	int i;
 
-	snprintf(name, len, "%s", name1 ? name1 : name2);
+	snprintf(name, len, "%s", src);
 
-	if (!normalization)
+	if (name != card->snd_card->driver)
 		return;
 
 	/*
-	 * Name normalization
+	 * Name normalization (driver field)
 	 *
 	 * The driver name is somewhat special, as it's used as a key for
 	 * searches in the user-space.
@@ -1874,6 +1875,14 @@ static void __soc_setup_card_name(char *name, int len,
 			break;
 		}
 	}
+
+	/*
+	 * The driver field should contain a valid string from the user view.
+	 * The wrapping usually does not work so well here. Set a smaller string
+	 * in the specific ASoC driver.
+	 */
+	if (strlen(src) > len - 1)
+		dev_err(card->dev, "ASoC: driver name too long '%s' -> '%s'\n", src, name);
 }
 
 static void soc_cleanup_card_resources(struct snd_soc_card *card)
@@ -2041,12 +2050,12 @@ static int snd_soc_bind_card(struct snd_soc_card *card)
 	/* try to set some sane longname if DMI is available */
 	snd_soc_set_dmi_name(card, NULL);
 
-	soc_setup_card_name(card->snd_card->shortname,
-			    card->name, NULL, 0);
-	soc_setup_card_name(card->snd_card->longname,
-			    card->long_name, card->name, 0);
-	soc_setup_card_name(card->snd_card->driver,
-			    card->driver_name, card->name, 1);
+	soc_setup_card_name(card, card->snd_card->shortname,
+			    card->name, NULL);
+	soc_setup_card_name(card, card->snd_card->longname,
+			    card->long_name, card->name);
+	soc_setup_card_name(card, card->snd_card->driver,
+			    card->driver_name, card->name);
 
 	if (card->components) {
 		/* the current implementation of snd_component_add() accepts */
-- 
2.35.3

Re: [PATCH v2] ASoC: core: clarify the driver name initialization

[Nícolas F. R. A. Prado]

Hi,

On Thu, Sep 29, 2022 at 04:37:54PM +0200, Jaroslav Kysela wrote:
> The driver field in the struct snd_ctl_card_info is a valid
> user space identifier. Actually, many ASoC drivers do not care
> and let to initialize this field using a standard wrapping method.
> Unfortunately, in this way, this field becomes unusable and
> unreadable for the drivers with longer card names. Also,
> there is a possibility to have clashes (driver field has
> only limit of 15 characters).
> 
> This change will print an error when the wrapping is used.
> The developers of the affected drivers should fix the problem.
> 
> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
> 
> v1..v2:
>   - remove the wrong DMI condition per Mark's review
> ---
>  sound/soc/soc-core.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index e824ff1a9fc0..590143ebf6df 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -1840,21 +1840,22 @@ static void soc_check_tplg_fes(struct snd_soc_card *card)
>  	}
>  }
>  
> -#define soc_setup_card_name(name, name1, name2, norm)		\
> -	__soc_setup_card_name(name, sizeof(name), name1, name2, norm)
> -static void __soc_setup_card_name(char *name, int len,
> -				  const char *name1, const char *name2,
> -				  int normalization)
> +#define soc_setup_card_name(card, name, name1, name2) \
> +	__soc_setup_card_name(card, name, sizeof(name), name1, name2)
> +static void __soc_setup_card_name(struct snd_soc_card *card,
> +				  char *name, int len,
> +				  const char *name1, const char *name2)
>  {
> +	const char *src = name1 ? name1 : name2;
>  	int i;
>  
> -	snprintf(name, len, "%s", name1 ? name1 : name2);
> +	snprintf(name, len, "%s", src);
>  
> -	if (!normalization)
> +	if (name != card->snd_card->driver)
>  		return;

The changes do more than the commit message implies by also reworking the need
for the normalization flag, and now that you're special casing most of the
function based on a particular pointer, I wonder if splitting the below logic to
a separate function would make more sense.

In any case,

Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Thanks,
Nícolas

>  
>  	/*
> -	 * Name normalization
> +	 * Name normalization (driver field)
>  	 *
>  	 * The driver name is somewhat special, as it's used as a key for
>  	 * searches in the user-space.
> @@ -1874,6 +1875,14 @@ static void __soc_setup_card_name(char *name, int len,
>  			break;
>  		}
>  	}
> +
> +	/*
> +	 * The driver field should contain a valid string from the user view.
> +	 * The wrapping usually does not work so well here. Set a smaller string
> +	 * in the specific ASoC driver.
> +	 */
> +	if (strlen(src) > len - 1)
> +		dev_err(card->dev, "ASoC: driver name too long '%s' -> '%s'\n", src, name);
>  }
>  
>  static void soc_cleanup_card_resources(struct snd_soc_card *card)
> @@ -2041,12 +2050,12 @@ static int snd_soc_bind_card(struct snd_soc_card *card)
>  	/* try to set some sane longname if DMI is available */
>  	snd_soc_set_dmi_name(card, NULL);
>  
> -	soc_setup_card_name(card->snd_card->shortname,
> -			    card->name, NULL, 0);
> -	soc_setup_card_name(card->snd_card->longname,
> -			    card->long_name, card->name, 0);
> -	soc_setup_card_name(card->snd_card->driver,
> -			    card->driver_name, card->name, 1);
> +	soc_setup_card_name(card, card->snd_card->shortname,
> +			    card->name, NULL);
> +	soc_setup_card_name(card, card->snd_card->longname,
> +			    card->long_name, card->name);
> +	soc_setup_card_name(card, card->snd_card->driver,
> +			    card->driver_name, card->name);
>  
>  	if (card->components) {
>  		/* the current implementation of snd_component_add() accepts */
> -- 
> 2.35.3
> 

Re: [PATCH v2] ASoC: core: clarify the driver name initialization

[Mark Brown]

On Thu, 29 Sep 2022 16:37:54 +0200, Jaroslav Kysela wrote:
> The driver field in the struct snd_ctl_card_info is a valid
> user space identifier. Actually, many ASoC drivers do not care
> and let to initialize this field using a standard wrapping method.
> Unfortunately, in this way, this field becomes unusable and
> unreadable for the drivers with longer card names. Also,
> there is a possibility to have clashes (driver field has
> only limit of 15 characters).
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: core: clarify the driver name initialization
      commit: c8d18e44022518ab026338ae86bf14cdf2e71887

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v2] ASoC: core: clarify the driver name initialization

[Pierre-Louis Bossart]

Hi Jaroslav,

On 9/29/22 09:37, Jaroslav Kysela wrote:
> The driver field in the struct snd_ctl_card_info is a valid
> user space identifier. Actually, many ASoC drivers do not care
> and let to initialize this field using a standard wrapping method.
> Unfortunately, in this way, this field becomes unusable and
> unreadable for the drivers with longer card names. Also,
> there is a possibility to have clashes (driver field has
> only limit of 15 characters).
> 
> This change will print an error when the wrapping is used.
> The developers of the affected drivers should fix the problem.

How should we fix this problem?

I see all kinds of errors thrown in our first CI results based on 6.1-rc1:

[   12.684893] kernel: cml_rt1011_rt5682 cml_rt1011_rt5682: ASoC: driver
name too long 'sof-cml_rt1011_rt5682' -> 'sof-cml_rt1011_'

[   12.219660] kernel: bxt_da7219_max98357a glk_da7219_mx98357a: ASoC:
driver name too long 'sof-glkda7219max' -> 'sof-glkda7219ma'

I have no idea what is expected here in terms of naming. It's far from
obvious to respect the 15-character limit AND have something
readable/sensible given the proliferation of hardware skews.

Any suggestions?

Thanks,
-Pierre

Re: [PATCH v2] ASoC: core: clarify the driver name initialization

[Jaroslav Kysela]

On 19. 10. 22 22:06, Pierre-Louis Bossart wrote:
> Hi Jaroslav,
> 
> On 9/29/22 09:37, Jaroslav Kysela wrote:
>> The driver field in the struct snd_ctl_card_info is a valid
>> user space identifier. Actually, many ASoC drivers do not care
>> and let to initialize this field using a standard wrapping method.
>> Unfortunately, in this way, this field becomes unusable and
>> unreadable for the drivers with longer card names. Also,
>> there is a possibility to have clashes (driver field has
>> only limit of 15 characters).
>>
>> This change will print an error when the wrapping is used.
>> The developers of the affected drivers should fix the problem.
> 
> How should we fix this problem?
> 
> I see all kinds of errors thrown in our first CI results based on 6.1-rc1:
> 
> [   12.684893] kernel: cml_rt1011_rt5682 cml_rt1011_rt5682: ASoC: driver
> name too long 'sof-cml_rt1011_rt5682' -> 'sof-cml_rt1011_'
> 
> [   12.219660] kernel: bxt_da7219_max98357a glk_da7219_mx98357a: ASoC:
> driver name too long 'sof-glkda7219max' -> 'sof-glkda7219ma'
> 
> I have no idea what is expected here in terms of naming. It's far from
> obvious to respect the 15-character limit AND have something
> readable/sensible given the proliferation of hardware skews.
> 
> Any suggestions?

The question is, how deep the driver name should describe the hardware 
details. The two drivers covering the majority hardware use "HDA-Intel" and 
"USB-Audio" strings here and there are a lot of variants of the codec / user 
space devices / mixer controls. The codec chain and eventually the audio 
bridge can be described in other identification strings like card components 
or the card name (note that most end users are not able to identify the 
references to hardware - it's a GUI string).

I would use "SOF-Intel-CML" and "SOF-Intel-GLK" strings or just "SOF-Intel" or 
any other variant as you like (lower case etc.). My opinion is that it's not 
necessary to have an unique string per driver here (the drivers should have 
just something common like the SOF core code).

						Jaroslav

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

Re: [PATCH v2] ASoC: core: clarify the driver name initialization

[Pierre-Louis Bossart]

Hi Jaroslav,

>>> The driver field in the struct snd_ctl_card_info is a valid
>>> user space identifier. Actually, many ASoC drivers do not care
>>> and let to initialize this field using a standard wrapping method.
>>> Unfortunately, in this way, this field becomes unusable and
>>> unreadable for the drivers with longer card names. Also,
>>> there is a possibility to have clashes (driver field has
>>> only limit of 15 characters).
>>>
>>> This change will print an error when the wrapping is used.
>>> The developers of the affected drivers should fix the problem.
>>
>> How should we fix this problem?
>>
>> I see all kinds of errors thrown in our first CI results based on
>> 6.1-rc1:
>>
>> [   12.684893] kernel: cml_rt1011_rt5682 cml_rt1011_rt5682: ASoC: driver
>> name too long 'sof-cml_rt1011_rt5682' -> 'sof-cml_rt1011_'
>>
>> [   12.219660] kernel: bxt_da7219_max98357a glk_da7219_mx98357a: ASoC:
>> driver name too long 'sof-glkda7219max' -> 'sof-glkda7219ma'
>>
>> I have no idea what is expected here in terms of naming. It's far from
>> obvious to respect the 15-character limit AND have something
>> readable/sensible given the proliferation of hardware skews.
>>
>> Any suggestions?
> 
> The question is, how deep the driver name should describe the hardware
> details. The two drivers covering the majority hardware use "HDA-Intel"
> and "USB-Audio" strings here and there are a lot of variants of the
> codec / user space devices / mixer controls. The codec chain and
> eventually the audio bridge can be described in other identification
> strings like card components or the card name (note that most end users
> are not able to identify the references to hardware - it's a GUI string).
> 
> I would use "SOF-Intel-CML" and "SOF-Intel-GLK" strings or just
> "SOF-Intel" or any other variant as you like (lower case etc.). My
> opinion is that it's not necessary to have an unique string per driver
> here (the drivers should have just something common like the SOF core
> code).

ok, adding 'SOF-Intel' would be fine, but remind me again how to set
the driver name?

We already set the card name to e.g.
	broxton_audio_card.name = "glkda7219max";
then the sof-prefix gets added, and that's what we use for UCM [1]

how would I modify the driver name?

If I just go ahead and set .driver_name = "SOF-Intel" in the card
declaration, isn't this going to impact the calls to

	soc_setup_card_name(card, card->snd_card->shortname,
			    card->name, NULL);
	soc_setup_card_name(card, card->snd_card->longname,
			    card->long_name, card->name);
	soc_setup_card_name(card, card->snd_card->driver,
			    card->driver_name, card->name);


#define soc_setup_card_name(card, name, name1, name2) \
	__soc_setup_card_name(card, name, sizeof(name), name1, name2)
static void __soc_setup_card_name(struct snd_soc_card *card,
				  char *name, int len,
				  const char *name1, const char *name2)
{
	const char *src = name1 ? name1 : name2;
	int i;

	snprintf(name, len, "%s", src);

	if (name != card->snd_card->driver)
		return;

so the shortname and longname would never be used?

I am beyond confused on all this :-)


[1]
https://github.com/alsa-project/alsa-ucm-conf/tree/master/ucm2/Intel/sof-glkda7219max

Re: [PATCH v2] ASoC: core: clarify the driver name initialization

[Jaroslav Kysela]

On 20. 10. 22 18:27, Pierre-Louis Bossart wrote:
> Hi Jaroslav,
> 
>>>> The driver field in the struct snd_ctl_card_info is a valid
>>>> user space identifier. Actually, many ASoC drivers do not care
>>>> and let to initialize this field using a standard wrapping method.
>>>> Unfortunately, in this way, this field becomes unusable and
>>>> unreadable for the drivers with longer card names. Also,
>>>> there is a possibility to have clashes (driver field has
>>>> only limit of 15 characters).
>>>>
>>>> This change will print an error when the wrapping is used.
>>>> The developers of the affected drivers should fix the problem.
>>>
>>> How should we fix this problem?
>>>
>>> I see all kinds of errors thrown in our first CI results based on
>>> 6.1-rc1:
>>>
>>> [   12.684893] kernel: cml_rt1011_rt5682 cml_rt1011_rt5682: ASoC: driver
>>> name too long 'sof-cml_rt1011_rt5682' -> 'sof-cml_rt1011_'
>>>
>>> [   12.219660] kernel: bxt_da7219_max98357a glk_da7219_mx98357a: ASoC:
>>> driver name too long 'sof-glkda7219max' -> 'sof-glkda7219ma'
>>>
>>> I have no idea what is expected here in terms of naming. It's far from
>>> obvious to respect the 15-character limit AND have something
>>> readable/sensible given the proliferation of hardware skews.
>>>
>>> Any suggestions?
>>
>> The question is, how deep the driver name should describe the hardware
>> details. The two drivers covering the majority hardware use "HDA-Intel"
>> and "USB-Audio" strings here and there are a lot of variants of the
>> codec / user space devices / mixer controls. The codec chain and
>> eventually the audio bridge can be described in other identification
>> strings like card components or the card name (note that most end users
>> are not able to identify the references to hardware - it's a GUI string).
>>
>> I would use "SOF-Intel-CML" and "SOF-Intel-GLK" strings or just
>> "SOF-Intel" or any other variant as you like (lower case etc.). My
>> opinion is that it's not necessary to have an unique string per driver
>> here (the drivers should have just something common like the SOF core
>> code).
> 
> ok, adding 'SOF-Intel' would be fine, but remind me again how to set
> the driver name?
> 
> We already set the card name to e.g.
> 	broxton_audio_card.name = "glkda7219max";
> then the sof-prefix gets added, and that's what we use for UCM [1]
> 
> how would I modify the driver name?
> 
> If I just go ahead and set .driver_name = "SOF-Intel" in the card
> declaration, isn't this going to impact the calls to
> 
> 	soc_setup_card_name(card, card->snd_card->shortname,
> 			    card->name, NULL);
> 	soc_setup_card_name(card, card->snd_card->longname,
> 			    card->long_name, card->name);
> 	soc_setup_card_name(card, card->snd_card->driver,
> 			    card->driver_name, card->name);
> 
> 
> #define soc_setup_card_name(card, name, name1, name2) \
> 	__soc_setup_card_name(card, name, sizeof(name), name1, name2)
> static void __soc_setup_card_name(struct snd_soc_card *card,
> 				  char *name, int len,
> 				  const char *name1, const char *name2)
> {
> 	const char *src = name1 ? name1 : name2;
> 	int i;
> 
> 	snprintf(name, len, "%s", src);
> 
> 	if (name != card->snd_card->driver)
> 		return;
> 
> so the shortname and longname would never be used?

Nope. It's just a short path for the non-driver field to not further process 
the destination string (the name argument). The snprintf() call sets all field 
types (it's before the condition). Just set the driver_name field in the soc 
card structure and you will be fine.

The UCM config must be updated to handle the new driver name. The fine 
selection key should probably use the card name, because long name is set from 
DMI:

old:

1 [sofglkda7219max]: sof-glkda7219ma - sof-glkda7219max
                      Google-Phaser360-rev4

new:

1 [sofglkda7219max]: SOF-Intel - sof-glkda7219max
                      Google-Phaser360-rev4

UCM substitutions:

1 [${CardId}      ]: ${CardDriver} - ${CardName}
                      ${CardLongName}

UCM conf:

mkdir -p ucm2/conf.d/SOF-Intel
cat > ucm2/conf.d/SOF-Intel/SOF-Intel.conf <<EOF
Syntax 6
Include.0.File "/Intel/\${CardName}/\${CardName}.conf"
EOF

						Jaroslav

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

Re: [PATCH v2] ASoC: core: clarify the driver name initialization

[Pierre-Louis Bossart]

Hi Jaroslav,

> Nope. It's just a short path for the non-driver field to not further
> process the destination string (the name argument). The snprintf() call
> sets all field types (it's before the condition). Just set the
> driver_name field in the soc card structure and you will be fine.
> 
> The UCM config must be updated to handle the new driver name. The fine
> selection key should probably use the card name, because long name is
> set from DMI:
> 
> old:
> 
> 1 [sofglkda7219max]: sof-glkda7219ma - sof-glkda7219max
>                      Google-Phaser360-rev4
> 
> new:
> 
> 1 [sofglkda7219max]: SOF-Intel - sof-glkda7219max
>                      Google-Phaser360-rev4
> 
> UCM substitutions:
> 
> 1 [${CardId}      ]: ${CardDriver} - ${CardName}
>                      ${CardLongName}
> 
> UCM conf:
> 
> mkdir -p ucm2/conf.d/SOF-Intel
> cat > ucm2/conf.d/SOF-Intel/SOF-Intel.conf <<EOF
> Syntax 6
> Include.0.File "/Intel/\${CardName}/\${CardName}.conf"
> EOF

I am not following any of this, sorry.

The existing UCM configuration uses the card name, e.g.
sof-glkda7219max. That works and needs zero extra work.

If all the cards registered in sound/soc/intel/boards use the same
"SOF-Intel" driver name, then the driver name cannot be used for any UCM
selection.

What is the point of including all the cardName.conf files at a higher
level that brings no obvious value beyond an indirection that we already
have with the path ucm2/Intel ?

What am I missing ??

Re: [PATCH v2] ASoC: core: clarify the driver name initialization

[Jaroslav Kysela]

On 20. 10. 22 20:13, Pierre-Louis Bossart wrote:
> Hi Jaroslav,
> 
>> Nope. It's just a short path for the non-driver field to not further
>> process the destination string (the name argument). The snprintf() call
>> sets all field types (it's before the condition). Just set the
>> driver_name field in the soc card structure and you will be fine.
>>
>> The UCM config must be updated to handle the new driver name. The fine
>> selection key should probably use the card name, because long name is
>> set from DMI:
>>
>> old:
>>
>> 1 [sofglkda7219max]: sof-glkda7219ma - sof-glkda7219max
>>                       Google-Phaser360-rev4
>>
>> new:
>>
>> 1 [sofglkda7219max]: SOF-Intel - sof-glkda7219max
>>                       Google-Phaser360-rev4
>>
>> UCM substitutions:
>>
>> 1 [${CardId}      ]: ${CardDriver} - ${CardName}
>>                       ${CardLongName}
>>
>> UCM conf:
>>
>> mkdir -p ucm2/conf.d/SOF-Intel
>> cat > ucm2/conf.d/SOF-Intel/SOF-Intel.conf <<EOF
>> Syntax 6
>> Include.0.File "/Intel/\${CardName}/\${CardName}.conf"
>> EOF
> 
> I am not following any of this, sorry.
> 
> The existing UCM configuration uses the card name, e.g.
> sof-glkda7219max. That works and needs zero extra work.

Unfortunately, actually the wrapped driver names are used for the primary 
lookups. The card name is not used at all in ucm2/conf.d.

> If all the cards registered in sound/soc/intel/boards use the same
> "SOF-Intel" driver name, then the driver name cannot be used for any UCM
> selection.

It can be used for the first level of the lookup. Eventually, we can add
ucm2/conf.d/${CardDriver}/${CardName}.conf search path to ucm2/ucm.conf for 
the direct lookups to handle this case, but it's just an optimization. I would 
start with the ${CardName} redirection as I suggested. We can decide / make 
the ucm.conf change later.

> What is the point of including all the cardName.conf files at a higher
> level that brings no obvious value beyond an indirection that we already
> have with the path ucm2/Intel ?
> 
> What am I missing ??

The goal is to fix the driver names (e.g. "sof-glkda7219ma", "sof-mt8195_r101" 
etc.). If you like to keep the unique names, it's your decision. I just prefer 
to have a string which is understandable to users. UCM can handle the finer 
selection of the configuration at any level now. Examples: sof-soundwire, 
USB-Audio (ucm2/USB-Audio/USB-Audio.conf), SOF (ucm2/Intel/SOF/HiFi.conf).

						Jaroslav

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

Re: [PATCH v2] ASoC: core: clarify the driver name initialization

[Pierre-Louis Bossart]

On 10/21/22 01:37, Jaroslav Kysela wrote:
> On 20. 10. 22 20:13, Pierre-Louis Bossart wrote:
>> Hi Jaroslav,
>>
>>> Nope. It's just a short path for the non-driver field to not further
>>> process the destination string (the name argument). The snprintf() call
>>> sets all field types (it's before the condition). Just set the
>>> driver_name field in the soc card structure and you will be fine.
>>>
>>> The UCM config must be updated to handle the new driver name. The fine
>>> selection key should probably use the card name, because long name is
>>> set from DMI:
>>>
>>> old:
>>>
>>> 1 [sofglkda7219max]: sof-glkda7219ma - sof-glkda7219max
>>>                       Google-Phaser360-rev4
>>>
>>> new:
>>>
>>> 1 [sofglkda7219max]: SOF-Intel - sof-glkda7219max
>>>                       Google-Phaser360-rev4
>>>
>>> UCM substitutions:
>>>
>>> 1 [${CardId}      ]: ${CardDriver} - ${CardName}
>>>                       ${CardLongName}
>>>
>>> UCM conf:
>>>
>>> mkdir -p ucm2/conf.d/SOF-Intel
>>> cat > ucm2/conf.d/SOF-Intel/SOF-Intel.conf <<EOF
>>> Syntax 6
>>> Include.0.File "/Intel/\${CardName}/\${CardName}.conf"
>>> EOF
>>
>> I am not following any of this, sorry.
>>
>> The existing UCM configuration uses the card name, e.g.
>> sof-glkda7219max. That works and needs zero extra work.
> 
> Unfortunately, actually the wrapped driver names are used for the
> primary lookups. The card name is not used at all in ucm2/conf.d.

ok, that's interesting because I've always used the card name with
alsaucm :-)


Usage: alsaucm <options> [command]

Available options:
  -h,--help                  this help
  -c,--card NAME             open card NAME

alsaucm -c sof-glkda7219max set _verb HiFi set _enadev Headphone

And if we move to use the driver name as the primary lookup then we'd
still need the card name for alsaucm to work, so why use the driver name
as a primary lookup?

If we can report a less confusing driver name to the users, that's fine
with me, but I don't get the idea of using the driver name as the first
criterion to identify a setup, you'll also need the card name so why not
use the card name as primary criterion?

>> If all the cards registered in sound/soc/intel/boards use the same
>> "SOF-Intel" driver name, then the driver name cannot be used for any UCM
>> selection.
> 
> It can be used for the first level of the lookup. Eventually, we can add
> ucm2/conf.d/${CardDriver}/${CardName}.conf search path to ucm2/ucm.conf
> for the direct lookups to handle this case, but it's just an
> optimization. I would start with the ${CardName} redirection as I
> suggested. We can decide / make the ucm.conf change later.
> 
>> What is the point of including all the cardName.conf files at a higher
>> level that brings no obvious value beyond an indirection that we already
>> have with the path ucm2/Intel ?
>>
>> What am I missing ??
> 
> The goal is to fix the driver names (e.g. "sof-glkda7219ma",
> "sof-mt8195_r101" etc.). If you like to keep the unique names, it's your
> decision. I just prefer to have a string which is understandable to
> users. UCM can handle the finer selection of the configuration at any
> level now. Examples: sof-soundwire, USB-Audio
> (ucm2/USB-Audio/USB-Audio.conf), SOF (ucm2/Intel/SOF/HiFi.conf).

I don't mind setting a common string, it's not going to be possible to
capture all hardware variations in 15 characters.

What worries me is backwards compatibility with existing user setups. If
someone updates their kernel and the change in driver name ends-up
breaking audio that's a big no-no for me. That's a real issue for us in
terms of support because we typically ask people reporting
SOF/SoundWire/HDA/mic issues if they can installing with our development
kernel.

Re: [PATCH v2] ASoC: core: clarify the driver name initialization

[Jaroslav Kysela]

On 21. 10. 22 16:36, Pierre-Louis Bossart wrote:
> 
> 
> On 10/21/22 01:37, Jaroslav Kysela wrote:
>> On 20. 10. 22 20:13, Pierre-Louis Bossart wrote:
>>> Hi Jaroslav,
>>>
>>>> Nope. It's just a short path for the non-driver field to not further
>>>> process the destination string (the name argument). The snprintf() call
>>>> sets all field types (it's before the condition). Just set the
>>>> driver_name field in the soc card structure and you will be fine.
>>>>
>>>> The UCM config must be updated to handle the new driver name. The fine
>>>> selection key should probably use the card name, because long name is
>>>> set from DMI:
>>>>
>>>> old:
>>>>
>>>> 1 [sofglkda7219max]: sof-glkda7219ma - sof-glkda7219max
>>>>                        Google-Phaser360-rev4
>>>>
>>>> new:
>>>>
>>>> 1 [sofglkda7219max]: SOF-Intel - sof-glkda7219max
>>>>                        Google-Phaser360-rev4
>>>>
>>>> UCM substitutions:
>>>>
>>>> 1 [${CardId}      ]: ${CardDriver} - ${CardName}
>>>>                        ${CardLongName}
>>>>
>>>> UCM conf:
>>>>
>>>> mkdir -p ucm2/conf.d/SOF-Intel
>>>> cat > ucm2/conf.d/SOF-Intel/SOF-Intel.conf <<EOF
>>>> Syntax 6
>>>> Include.0.File "/Intel/\${CardName}/\${CardName}.conf"
>>>> EOF
>>>
>>> I am not following any of this, sorry.
>>>
>>> The existing UCM configuration uses the card name, e.g.
>>> sof-glkda7219max. That works and needs zero extra work.
>>
>> Unfortunately, actually the wrapped driver names are used for the
>> primary lookups. The card name is not used at all in ucm2/conf.d.
> 
> ok, that's interesting because I've always used the card name with
> alsaucm :-)
> 
> 
> Usage: alsaucm <options> [command]
> 
> Available options:
>    -h,--help                  this help
>    -c,--card NAME             open card NAME
> 
> alsaucm -c sof-glkda7219max set _verb HiFi set _enadev Headphone
> 
> And if we move to use the driver name as the primary lookup then we'd
> still need the card name for alsaucm to work, so why use the driver name
> as a primary lookup?

The driver name is used for the lookups as defined in ucm2/ucm.conf. The 
driver/card name/card long name lookups are handled in the alsa-lib's code 
(converted to "hw:X" UCM card name). If you turn this fallback off (use 
"strict:sof-glkda7219max" UCM card name), things won't work.

> If we can report a less confusing driver name to the users, that's fine
> with me, but I don't get the idea of using the driver name as the first
> criterion to identify a setup, you'll also need the card name so why not
> use the card name as primary criterion?

It is not usable for the USB driver where every model has own name set from 
USB descriptors for example.

>>> If all the cards registered in sound/soc/intel/boards use the same
>>> "SOF-Intel" driver name, then the driver name cannot be used for any UCM
>>> selection.
>>
>> It can be used for the first level of the lookup. Eventually, we can add
>> ucm2/conf.d/${CardDriver}/${CardName}.conf search path to ucm2/ucm.conf
>> for the direct lookups to handle this case, but it's just an
>> optimization. I would start with the ${CardName} redirection as I
>> suggested. We can decide / make the ucm.conf change later.
>>
>>> What is the point of including all the cardName.conf files at a higher
>>> level that brings no obvious value beyond an indirection that we already
>>> have with the path ucm2/Intel ?
>>>
>>> What am I missing ??
>>
>> The goal is to fix the driver names (e.g. "sof-glkda7219ma",
>> "sof-mt8195_r101" etc.). If you like to keep the unique names, it's your
>> decision. I just prefer to have a string which is understandable to
>> users. UCM can handle the finer selection of the configuration at any
>> level now. Examples: sof-soundwire, USB-Audio
>> (ucm2/USB-Audio/USB-Audio.conf), SOF (ucm2/Intel/SOF/HiFi.conf).
> 
> I don't mind setting a common string, it's not going to be possible to
> capture all hardware variations in 15 characters.
> 
> What worries me is backwards compatibility with existing user setups. If
> someone updates their kernel and the change in driver name ends-up
> breaking audio that's a big no-no for me. That's a real issue for us in
> terms of support because we typically ask people reporting
> SOF/SoundWire/HDA/mic issues if they can installing with our development
> kernel.

We can use a similar mechanism as we did with 
CONFIG_SND_SOC_INTEL_USER_FRIENDLY_LONG_NAMES . The distributions can enable 
this when packages when UCM configs are updated. Also, new drivers should use 
new driver name scheme, it's only for the current drivers.

						Jaroslav

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

Re: [PATCH v2] ASoC: core: clarify the driver name initialization

[Pierre-Louis Bossart]

On 10/24/22 01:19, Jaroslav Kysela wrote:
> On 21. 10. 22 16:36, Pierre-Louis Bossart wrote:
>>
>>
>> On 10/21/22 01:37, Jaroslav Kysela wrote:
>>> On 20. 10. 22 20:13, Pierre-Louis Bossart wrote:
>>>> Hi Jaroslav,
>>>>
>>>>> Nope. It's just a short path for the non-driver field to not further
>>>>> process the destination string (the name argument). The snprintf()
>>>>> call
>>>>> sets all field types (it's before the condition). Just set the
>>>>> driver_name field in the soc card structure and you will be fine.
>>>>>
>>>>> The UCM config must be updated to handle the new driver name. The fine
>>>>> selection key should probably use the card name, because long name is
>>>>> set from DMI:
>>>>>
>>>>> old:
>>>>>
>>>>> 1 [sofglkda7219max]: sof-glkda7219ma - sof-glkda7219max
>>>>>                        Google-Phaser360-rev4
>>>>>
>>>>> new:
>>>>>
>>>>> 1 [sofglkda7219max]: SOF-Intel - sof-glkda7219max
>>>>>                        Google-Phaser360-rev4
>>>>>
>>>>> UCM substitutions:
>>>>>
>>>>> 1 [${CardId}      ]: ${CardDriver} - ${CardName}
>>>>>                        ${CardLongName}
>>>>>
>>>>> UCM conf:
>>>>>
>>>>> mkdir -p ucm2/conf.d/SOF-Intel
>>>>> cat > ucm2/conf.d/SOF-Intel/SOF-Intel.conf <<EOF
>>>>> Syntax 6
>>>>> Include.0.File "/Intel/\${CardName}/\${CardName}.conf"
>>>>> EOF
>>>>
>>>> I am not following any of this, sorry.
>>>>
>>>> The existing UCM configuration uses the card name, e.g.
>>>> sof-glkda7219max. That works and needs zero extra work.
>>>
>>> Unfortunately, actually the wrapped driver names are used for the
>>> primary lookups. The card name is not used at all in ucm2/conf.d.
>>
>> ok, that's interesting because I've always used the card name with
>> alsaucm :-)
>>
>>
>> Usage: alsaucm <options> [command]
>>
>> Available options:
>>    -h,--help                  this help
>>    -c,--card NAME             open card NAME
>>
>> alsaucm -c sof-glkda7219max set _verb HiFi set _enadev Headphone
>>
>> And if we move to use the driver name as the primary lookup then we'd
>> still need the card name for alsaucm to work, so why use the driver name
>> as a primary lookup?
> 
> The driver name is used for the lookups as defined in ucm2/ucm.conf. The
> driver/card name/card long name lookups are handled in the alsa-lib's
> code (converted to "hw:X" UCM card name). If you turn this fallback off
> (use "strict:sof-glkda7219max" UCM card name), things won't work.

Fair enough, I wasn't even aware of the existence of this 'strict'
directive.

>> If we can report a less confusing driver name to the users, that's fine
>> with me, but I don't get the idea of using the driver name as the first
>> criterion to identify a setup, you'll also need the card name so why not
>> use the card name as primary criterion?
> 
> It is not usable for the USB driver where every model has own name set
> from USB descriptors for example.

How would you use UCM in that context, the use of a driver name would
lead to a lot of abstraction potentially, isn't there a risk of not
being able to detect specific skews that need variants?

>>>> If all the cards registered in sound/soc/intel/boards use the same
>>>> "SOF-Intel" driver name, then the driver name cannot be used for any
>>>> UCM
>>>> selection.
>>>
>>> It can be used for the first level of the lookup. Eventually, we can add
>>> ucm2/conf.d/${CardDriver}/${CardName}.conf search path to ucm2/ucm.conf
>>> for the direct lookups to handle this case, but it's just an
>>> optimization. I would start with the ${CardName} redirection as I
>>> suggested. We can decide / make the ucm.conf change later.
>>>
>>>> What is the point of including all the cardName.conf files at a higher
>>>> level that brings no obvious value beyond an indirection that we
>>>> already
>>>> have with the path ucm2/Intel ?
>>>>
>>>> What am I missing ??
>>>
>>> The goal is to fix the driver names (e.g. "sof-glkda7219ma",
>>> "sof-mt8195_r101" etc.). If you like to keep the unique names, it's your
>>> decision. I just prefer to have a string which is understandable to
>>> users. UCM can handle the finer selection of the configuration at any
>>> level now. Examples: sof-soundwire, USB-Audio
>>> (ucm2/USB-Audio/USB-Audio.conf), SOF (ucm2/Intel/SOF/HiFi.conf).
>>
>> I don't mind setting a common string, it's not going to be possible to
>> capture all hardware variations in 15 characters.
>>
>> What worries me is backwards compatibility with existing user setups. If
>> someone updates their kernel and the change in driver name ends-up
>> breaking audio that's a big no-no for me. That's a real issue for us in
>> terms of support because we typically ask people reporting
>> SOF/SoundWire/HDA/mic issues if they can installing with our development
>> kernel.
> 
> We can use a similar mechanism as we did with
> CONFIG_SND_SOC_INTEL_USER_FRIENDLY_LONG_NAMES . The distributions can
> enable this when packages when UCM configs are updated. Also, new
> drivers should use new driver name scheme, it's only for the current
> drivers.

That would be good indeed. FWIW, I reverted this patch in our
development tree to remove confusing error messages that make tests fail.

That would not be an Intel only option though, right? There are tons of
other ASoC machine drivers who don't set the driver name at all, so it
could take time to make that transition.

Re: [PATCH v2] ASoC: core: clarify the driver name initialization

[Jaroslav Kysela]

On 24. 10. 22 16:08, Pierre-Louis Bossart wrote:

>>> If we can report a less confusing driver name to the users, that's fine
>>> with me, but I don't get the idea of using the driver name as the first
>>> criterion to identify a setup, you'll also need the card name so why not
>>> use the card name as primary criterion?
>>
>> It is not usable for the USB driver where every model has own name set
>> from USB descriptors for example.
> 
> How would you use UCM in that context, the use of a driver name would
> lead to a lot of abstraction potentially, isn't there a risk of not
> being able to detect specific skews that need variants?

The fine USB device ID matching is used. This USB device ID is in the 
components string. But yes, it's the next level after the basic lookup.

>> We can use a similar mechanism as we did with
>> CONFIG_SND_SOC_INTEL_USER_FRIENDLY_LONG_NAMES . The distributions can
>> enable this when packages when UCM configs are updated. Also, new
>> drivers should use new driver name scheme, it's only for the current
>> drivers.
> 
> That would be good indeed. FWIW, I reverted this patch in our
> development tree to remove confusing error messages that make tests fail.
> 
> That would not be an Intel only option though, right? There are tons of
> other ASoC machine drivers who don't set the driver name at all, so it
> could take time to make that transition.

Yes, but we need to start somewhere. It seems that a most of ASoC drivers do 
not use card names bigger than 15 characters (I noted this recently in UCM).

						Jaroslav

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