| Message ID | 20220929143754.345144-1-perex@perex.cz |
|---|---|
| State | Accepted |
| Commit | c8d18e44022518ab026338ae86bf14cdf2e71887 |
| Headers | show |
| Series | [v2] ASoC: core: clarify the driver name initialization | expand |
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
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
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
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
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
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
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.
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 */
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(-)