| Message ID | 20220929080654.326311-1-perex@perex.cz (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: core: clarify the driver name initialization | expand |
On Thu, Sep 29, 2022 at 10:06:54AM +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. This breaks at least an arm multi_v7_defconfig build: /build/stage/linux/sound/soc/soc-core.c: In function ‘snd_soc_bind_card’: /build/stage/linux/sound/soc/soc-core.c:2055:36: error: ‘struct snd_soc_card’ ha s no member named ‘dmi_longname’ 2055 | if (card->long_name == card->dmi_longname) | ^~ > Also, it does not make sense to set the driver field to the > card name composed from DMI. This card name is longer in most > (all?) cases. Use a generic "ASoC-DMI" string here. This should be a separate change, and DMI is a term specific to the ACPI/EFI so I don't think we should be using it as a generic here, this seems like a step back. If we want to make a change there I'd expect it to be more picking the actual card driver name.
On 29. 09. 22 15:38, Mark Brown wrote: > On Thu, Sep 29, 2022 at 10:06:54AM +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. > > This breaks at least an arm multi_v7_defconfig build: > > /build/stage/linux/sound/soc/soc-core.c: In function ‘snd_soc_bind_card’: > /build/stage/linux/sound/soc/soc-core.c:2055:36: error: ‘struct snd_soc_card’ ha > s no member named ‘dmi_longname’ > 2055 | if (card->long_name == card->dmi_longname) > | ^~ > > >> Also, it does not make sense to set the driver field to the >> card name composed from DMI. This card name is longer in most >> (all?) cases. Use a generic "ASoC-DMI" string here. > > This should be a separate change, and DMI is a term specific to the > ACPI/EFI so I don't think we should be using it as a generic here, this > seems like a step back. If we want to make a change there I'd expect it > to be more picking the actual card driver name. Thanks for the review. Yes, I made a mistake here. I wrongly mixed name and long name strings in my head. I removed the DMI check and posted v2 of the patch. Jaroslav
On Thu, 29 Sep 2022 10:06: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
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index e824ff1a9fc0..fd037208c222 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) @@ -1928,6 +1937,7 @@ static int snd_soc_bind_card(struct snd_soc_card *card) struct snd_soc_pcm_runtime *rtd; struct snd_soc_component *component; struct snd_soc_dai_link *dai_link; + const char *fallback; int ret, i; mutex_lock(&client_mutex); @@ -2041,12 +2051,15 @@ 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); + fallback = card->name; + soc_setup_card_name(card, card->snd_card->longname, + card->long_name, fallback); + if (card->long_name == card->dmi_longname) + fallback = "ASoC-DMI"; + soc_setup_card_name(card, card->snd_card->driver, + card->driver_name, fallback); 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. Also, it does not make sense to set the driver field to the card name composed from DMI. This card name is longer in most (all?) cases. Use a generic "ASoC-DMI" string here. Signed-off-by: Jaroslav Kysela <perex@perex.cz> --- sound/soc/soc-core.c | 41 +++++++++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 14 deletions(-)