November 2019 - Alsa-devel - mailman.alsa-project.org

[alsa-devel] [PATCH 0/2] ASoC: max98090: make sure SHDN is 0 when changing sensitive registers
by Tzung-Bi Shih 21 Nov '19

21 Nov '19

This series is a follow up fix for the question: https://mailman.alsa-project.org/pipermail/alsa-devel/2019-October/157364.h… This series should apply after the series: https://mailman.alsa-project.org/pipermail/alsa-devel/2019-November/158850.… (in terms of line numbers). Tzung-Bi Shih (2): ASoC: core: add SND_SOC_BYTES_E ASoC: max98090: save and restore SHDN when changing sensitive registers include/sound/soc.h | 6 + sound/soc/codecs/max98090.c | 433 ++++++++++++++++++++++++++---------- sound/soc/codecs/max98090.h | 3 +- 3 files changed, 318 insertions(+), 124 deletions(-) -- 2.24.0.432.g9d3f5f5b63-goog

2 5

[alsa-devel] simple-card with platform component
by Daniel Baluta 21 Nov '19

21 Nov '19

Hello, How should simple-card work when we define a DT node like this: sof-audio-cs42888 { compatible = "simple-scu-audio-card"; simple-audio-card,dai-link { format = "i2s"; cpu { sound-dai = <&dummy>; }; codec { sound-dai = <&cs42888>; }; plat { sound-dai = <&dsp>; }; }; Is there any example with platform?

2 1

[alsa-devel] [PATCHv2] aplay: Limit VUMeter progress bar to 100 for negative as well
by Rosen Penev 20 Nov '19

20 Nov '19

While the progress bar cannot be negative, GCC concludes that it can be and assumes tmp can be written past the end. Fixes this GCC warning: aplay.c:1747:18: warning: '%02d' directive writing between 2 and 11 bytes into a region of size 4 [-Wformat-overflow=] 1747 | sprintf(tmp, "%02d%%", maxperc[c]); Signed-off-by: Rosen Penev <rosenp(a)gmail.com> --- v2: Reworded aplay/aplay.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/aplay/aplay.c b/aplay/aplay.c index 78adee2..1a887e4 100644 --- a/aplay/aplay.c +++ b/aplay/aplay.c @@ -54,6 +54,8 @@ #include "formats.h" #include "version.h" +#define ABS(a) (a) < 0 ? -(a) : (a) + #ifdef SND_CHMAP_API_VERSION #define CONFIG_SUPPORT_CHMAP 1 #endif @@ -1741,7 +1743,7 @@ static void print_vu_meter_stereo(int *perc, int *maxperc) line[bar_length + 6 + 1 + p] = '+'; else line[bar_length - p - 1] = '+'; - if (maxperc[c] > 99) + if (ABS(maxperc[c]) > 99) sprintf(tmp, "MAX"); else sprintf(tmp, "%02d%%", maxperc[c]); -- 2.23.0

2 1

[alsa-devel] ASoC, DMI and UCM
by Jaroslav Kysela 20 Nov '19

20 Nov '19

Hi all, I would like to discuss the way we use DMI information for the board identification in ASoC for the user space (long card name). It's a bit redundant information, because DMI is already exposed through /sys/class/dmi/id/ to the user space nowadays. My idea is to add 'DMI:sysfs' ctl info component string for the appropriate ASoC driver to detect the existence of this dmi interface. Then I can add the sysfs support to the ucm2 conditions. But it's really a quick idea. I'd like to have comments. Thank you, Jaroslav -- Jaroslav Kysela <perex(a)perex.cz> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

2 3

Re: [alsa-devel] [PATCH 4/4] aplay: Limit VUMeter progress bar to 100 for negative as well
by Rosen Penev 20 Nov '19

20 Nov '19

On Wed, Nov 20, 2019 at 10:34 AM Takashi Iwai <tiwai(a)suse.de> wrote: > > On Wed, 20 Nov 2019 18:55:43 +0100, > Rosen Penev wrote: > > > > On Tue, Nov 19, 2019 at 10:48 PM Takashi Iwai <tiwai(a)suse.de> wrote: > > > > > > On Wed, 20 Nov 2019 07:36:19 +0100, > > > Rosen Penev wrote: > > > > > > > > On Tue, Nov 19, 2019 at 10:26 PM Takashi Iwai <tiwai(a)suse.de> wrote: > > > > > > > > > > On Wed, 20 Nov 2019 05:28:56 +0100, > > > > > Rosen Penev wrote: > > > > > > > > > > > > If the progress bar somehow becomes negative, it ends up overwritting > > > > > > tmp. Fixes this GCC warning: > > > > > > > > > > > > aplay.c:1747:18: warning: '%02d' directive writing between 2 and 11 bytes > > > > > > into a region of size 4 [-Wformat-overflow=] > > > > > > 1747 | sprintf(tmp, "%02d%%", maxperc[c]); > > > > > > > > > > This is false-positive. The value passed there is guaranteed to be a > > > > > positive integer at the calculation time. > > > > Sure. But best to silence GCC. It probably optimizes better this way. > > > > > > I guess this adds more code in binary. Comparing with 99U would work? > > I tried that. Here are some results: > > > > 134348 for this patch > > 134832 if changed to U. Also tried casting lhs to unnsigned int, same size. > > 135125 originally > > > > I understand this is an exercise in micro-optimization, but still. > > > > Sizes are in bytes. It's the size of a compressed OpenWrt archive. > > Thanks for the analysis. It's surprising, though, the original code > became bigger. I've learned not to question the compiler. If it complains, it means it generates suboptimal code. > > The cast of lhs is basically superfluous since C performs the cast > implicitly at comparison, BTW. > > In anyway, the number tells. Could you respin this patch? I can resend. Not sure what you really want. > Meanwhile I'll apply the rest patches. > > > thanks, > > Takashi

1 0

[alsa-devel] [PATCHv2] aplay: Limit VUMeter progress bar to 100 for negative as well
by Rosen Penev 20 Nov '19

20 Nov '19

While the progress bar cannot be negative, GCC concludes that it can be and assumes tmp can be written past the end. Fixes this GCC warning: aplay.c:1747:18: warning: '%02d' directive writing between 2 and 11 bytes into a region of size 4 [-Wformat-overflow=] 1747 | sprintf(tmp, "%02d%%", maxperc[c]); Signed-off-by: Rosen Penev <rosenp(a)gmail.com> --- v2: Reworded aplay/aplay.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/aplay/aplay.c b/aplay/aplay.c index 78adee2..1a887e4 100644 --- a/aplay/aplay.c +++ b/aplay/aplay.c @@ -54,6 +54,8 @@ #include "formats.h" #include "version.h" +#define ABS(a) (a) < 0 ? -(a) : (a) + #ifdef SND_CHMAP_API_VERSION #define CONFIG_SUPPORT_CHMAP 1 #endif @@ -1741,7 +1743,7 @@ static void print_vu_meter_stereo(int *perc, int *maxperc) line[bar_length + 6 + 1 + p] = '+'; else line[bar_length - p - 1] = '+'; - if (maxperc[c] > 99) + if (ABS(maxperc[c]) > 99) sprintf(tmp, "MAX"); else sprintf(tmp, "%02d%%", maxperc[c]); -- 2.23.0

1 0

[alsa-devel] [PATCH] aplay: Adjust buffer sizes to fix snprintf warnings
by Rosen Penev 20 Nov '19

20 Nov '19

It also reduces compiled size slightly. aplay.c: In function 'capture': aplay.c:3055:34: error: '-01' directive output may be truncated writing 3 bytes into a region of size between 1 and 4097 [-Werror=format-truncation= ] 3055 | snprintf(namebuf, namelen, "%s-01", buf); | ^~~ aplay.c:3055:4: note: 'snprintf' output between 4 and 4100 bytes into a destination of size 4097 3055 | snprintf(namebuf, namelen, "%s-01", buf); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ aplay.c:3053:34: error: '-01.' directive output may be truncated writing 4 bytes into a region of size between 1 and 4097 [-Werror=format-truncation= ] 3053 | snprintf(namebuf, namelen, "%s-01.%s", buf, s); | ^~~~ aplay.c:3053:4: note: 'snprintf' output 5 or more bytes (assuming 4101) into a destination of size 4097 3053 | snprintf(namebuf, namelen, "%s-01.%s", buf, s); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ aplay.c:3065:34: error: '%02i' directive output may be truncated writing between 2 and 10 bytes into a region of size between 0 and 4096 [-Werror= format-truncation=] 3065 | snprintf(namebuf, namelen, "%s-%02i", buf, filecount); | ^~~~ aplay.c:3065:30: note: directive argument in the range [1, 2147483647] 3065 | snprintf(namebuf, namelen, "%s-%02i", buf, filecount); | ^~~~~~~~~ aplay.c:3065:3: note: 'snprintf' output between 4 and 4108 bytes into a destination of size 4097 3065 | snprintf(namebuf, namelen, "%s-%02i", buf, filecount); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ aplay.c:3063:34: error: '%02i' directive output may be truncated writing between 2 and 10 bytes into a region of size between 0 and 4096 [-Werror= format-truncation=] 3063 | snprintf(namebuf, namelen, "%s-%02i.%s", buf, filecount, s); | ^~~~ aplay.c:3063:30: note: directive argument in the range [1, 2147483647] 3063 | snprintf(namebuf, namelen, "%s-%02i.%s", buf, filecount, s); | ^~~~~~~~~~~~ aplay.c:3063:3: note: 'snprintf' output 5 or more bytes (assuming 4109) into a destination of size 4097 3063 | snprintf(namebuf, namelen, "%s-%02i.%s", buf, filecount, s); Signed-off-by: Rosen Penev <rosenp(a)gmail.com> --- aplay/aplay.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/aplay/aplay.c b/aplay/aplay.c index 9c5a11b..1a887e4 100644 --- a/aplay/aplay.c +++ b/aplay/aplay.c @@ -3017,7 +3017,7 @@ static int new_capture_file(char *name, char *namebuf, size_t namelen, int filecount) { char *s; - char buf[PATH_MAX+1]; + char buf[PATH_MAX-10]; time_t t; struct tm *tmp; @@ -3124,7 +3124,7 @@ static void capture(char *orig_name) int tostdout=0; /* boolean which describes output stream */ int filecount=0; /* number of files written */ char *name = orig_name; /* current filename */ - char namebuf[PATH_MAX+1]; + char namebuf[PATH_MAX+2]; off64_t count, rest; /* number of bytes to capture */ struct stat statbuf; -- 2.23.0

1 0

[alsa-devel] [PATCH v3 1/2] ASoC: Intel - use control components to describe card config
by Jaroslav Kysela 20 Nov '19

20 Nov '19

Use the control interface (field 'components' in the info structure) to pass the I/O configuration details. The goal is to replace the card long name with this. Signed-off-by: Jaroslav Kysela <perex(a)perex.cz> Cc: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> Cc: Mark Brown <broonie(a)kernel.org> --- sound/soc/intel/boards/bytcht_es8316.c | 10 +++++++++- sound/soc/intel/boards/bytcr_rt5640.c | 6 ++++++ sound/soc/intel/boards/bytcr_rt5651.c | 18 +++++++++++------- 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/sound/soc/intel/boards/bytcht_es8316.c b/sound/soc/intel/boards/bytcht_es8316.c index 46612331f5ea..4090550f0fe1 100644 --- a/sound/soc/intel/boards/bytcht_es8316.c +++ b/sound/soc/intel/boards/bytcht_es8316.c @@ -361,6 +361,7 @@ static struct snd_soc_dai_link byt_cht_es8316_dais[] = { /* SoC card */ static char codec_name[SND_ACPI_I2C_ID_LEN]; static char long_name[50]; /* = "bytcht-es8316-*-spk-*-mic" */ +static char components_string[32]; /* = "cfg-spk:* cfg-mic:* */ static int byt_cht_es8316_suspend(struct snd_soc_card *card) { @@ -572,17 +573,24 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) } } - /* register the soc card */ + snprintf(components_string, sizeof(components_string), + "cfg-spk:%s cfg-mic:%s", + (quirk & BYT_CHT_ES8316_MONO_SPEAKER) ? "1" : "2", + mic_name[BYT_CHT_ES8316_MAP(quirk)]); + byt_cht_es8316_card.components = components_string; snprintf(long_name, sizeof(long_name), "bytcht-es8316-%s-spk-%s-mic", (quirk & BYT_CHT_ES8316_MONO_SPEAKER) ? "mono" : "stereo", mic_name[BYT_CHT_ES8316_MAP(quirk)]); byt_cht_es8316_card.long_name = long_name; + + /* register the soc card */ snd_soc_card_set_drvdata(&byt_cht_es8316_card, priv); ret = devm_snd_soc_register_card(dev, &byt_cht_es8316_card); if (ret) { gpiod_put(priv->speaker_en_gpio); dev_err(dev, "snd_soc_register_card failed: %d\n", ret); + return ret; } platform_set_drvdata(pdev, &byt_cht_es8316_card); diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c index 9c1aa4ec9cba..0170d31e691a 100644 --- a/sound/soc/intel/boards/bytcr_rt5640.c +++ b/sound/soc/intel/boards/bytcr_rt5640.c @@ -1081,6 +1081,7 @@ static char byt_rt5640_codec_name[SND_ACPI_I2C_ID_LEN]; static char byt_rt5640_codec_aif_name[12]; /* = "rt5640-aif[1|2]" */ static char byt_rt5640_cpu_dai_name[10]; /* = "ssp[0|2]-port" */ static char byt_rt5640_long_name[40]; /* = "bytcr-rt5640-*-spk-*-mic" */ +static char byt_rt5640_components[32]; /* = "cfg-spk:* cfg-mic:*" */ static int byt_rt5640_suspend(struct snd_soc_card *card) { @@ -1303,6 +1304,11 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) } } + snprintf(byt_rt5640_components, sizeof(byt_rt5640_components), + "cfg-spk:%s cfg-mic:%s", + (byt_rt5640_quirk & BYT_RT5640_MONO_SPEAKER) ? "1" : "2", + map_name[BYT_RT5640_MAP(byt_rt5640_quirk)]); + byt_rt5640_card.components = byt_rt5640_components; snprintf(byt_rt5640_long_name, sizeof(byt_rt5640_long_name), "bytcr-rt5640-%s-spk-%s-mic", (byt_rt5640_quirk & BYT_RT5640_MONO_SPEAKER) ? diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c index 4606f6f582d6..80a5674ddb1b 100644 --- a/sound/soc/intel/boards/bytcr_rt5651.c +++ b/sound/soc/intel/boards/bytcr_rt5651.c @@ -798,6 +798,7 @@ static char byt_rt5651_codec_name[SND_ACPI_I2C_ID_LEN]; static char byt_rt5651_codec_aif_name[12]; /* = "rt5651-aif[1|2]" */ static char byt_rt5651_cpu_dai_name[10]; /* = "ssp[0|2]-port" */ static char byt_rt5651_long_name[50]; /* = "bytcr-rt5651-*-spk-*-mic[-swapped-hp]" */ +static char byt_rt5651_components[50]; /* = "cfg-spk:* cfg-mic:*" */ static int byt_rt5651_suspend(struct snd_soc_card *card) { @@ -876,7 +877,6 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) const char *platform_name; struct acpi_device *adev; struct device *codec_dev; - const char *hp_swapped; bool is_bytcr = false; int ret_val = 0; int dai_index = 0; @@ -1080,16 +1080,20 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) } } - if (byt_rt5651_quirk & BYT_RT5651_HP_LR_SWAPPED) - hp_swapped = "-hp-swapped"; - else - hp_swapped = ""; - + snprintf(byt_rt5651_components, sizeof(byt_rt5651_components), + "cfg-spk:%s cfg-mic:%s%s", + (byt_rt5651_quirk & BYT_RT5651_MONO_SPEAKER) ? "1" : "2", + mic_name[BYT_RT5651_MAP(byt_rt5651_quirk)], + (byt_rt5651_quirk & BYT_RT5651_HP_LR_SWAPPED) ? + " cfg-hp:lrswap" : ""); + byt_rt5651_card.components = byt_rt5651_components; snprintf(byt_rt5651_long_name, sizeof(byt_rt5651_long_name), "bytcr-rt5651-%s-spk-%s-mic%s", (byt_rt5651_quirk & BYT_RT5651_MONO_SPEAKER) ? "mono" : "stereo", - mic_name[BYT_RT5651_MAP(byt_rt5651_quirk)], hp_swapped); + mic_name[BYT_RT5651_MAP(byt_rt5651_quirk)], + (byt_rt5651_quirk & BYT_RT5651_HP_LR_SWAPPED) ? + "-hp-swapped" : ""); byt_rt5651_card.long_name = byt_rt5651_long_name; /* override plaform name, if required */ -- 2.20.1

1 2

[alsa-devel] [PATCH v4 1/2] ASoC: Intel - use control components to describe card config
by Jaroslav Kysela 20 Nov '19

20 Nov '19

Use the control interface (field 'components' in the info structure) to pass the I/O configuration details. The goal is to replace the card long name with this. Signed-off-by: Jaroslav Kysela <perex(a)perex.cz> Cc: Pierre-Louis Bossart <pierre-louis.bossart(a)linux.intel.com> Cc: Mark Brown <broonie(a)kernel.org> --- sound/soc/intel/boards/bytcht_es8316.c | 10 +++++++++- sound/soc/intel/boards/bytcr_rt5640.c | 6 ++++++ sound/soc/intel/boards/bytcr_rt5651.c | 18 +++++++++++------- 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/sound/soc/intel/boards/bytcht_es8316.c b/sound/soc/intel/boards/bytcht_es8316.c index 46612331f5ea..4090550f0fe1 100644 --- a/sound/soc/intel/boards/bytcht_es8316.c +++ b/sound/soc/intel/boards/bytcht_es8316.c @@ -361,6 +361,7 @@ static struct snd_soc_dai_link byt_cht_es8316_dais[] = { /* SoC card */ static char codec_name[SND_ACPI_I2C_ID_LEN]; static char long_name[50]; /* = "bytcht-es8316-*-spk-*-mic" */ +static char components_string[32]; /* = "cfg-spk:* cfg-mic:* */ static int byt_cht_es8316_suspend(struct snd_soc_card *card) { @@ -572,17 +573,24 @@ static int snd_byt_cht_es8316_mc_probe(struct platform_device *pdev) } } - /* register the soc card */ + snprintf(components_string, sizeof(components_string), + "cfg-spk:%s cfg-mic:%s", + (quirk & BYT_CHT_ES8316_MONO_SPEAKER) ? "1" : "2", + mic_name[BYT_CHT_ES8316_MAP(quirk)]); + byt_cht_es8316_card.components = components_string; snprintf(long_name, sizeof(long_name), "bytcht-es8316-%s-spk-%s-mic", (quirk & BYT_CHT_ES8316_MONO_SPEAKER) ? "mono" : "stereo", mic_name[BYT_CHT_ES8316_MAP(quirk)]); byt_cht_es8316_card.long_name = long_name; + + /* register the soc card */ snd_soc_card_set_drvdata(&byt_cht_es8316_card, priv); ret = devm_snd_soc_register_card(dev, &byt_cht_es8316_card); if (ret) { gpiod_put(priv->speaker_en_gpio); dev_err(dev, "snd_soc_register_card failed: %d\n", ret); + return ret; } platform_set_drvdata(pdev, &byt_cht_es8316_card); diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c index 9c1aa4ec9cba..0170d31e691a 100644 --- a/sound/soc/intel/boards/bytcr_rt5640.c +++ b/sound/soc/intel/boards/bytcr_rt5640.c @@ -1081,6 +1081,7 @@ static char byt_rt5640_codec_name[SND_ACPI_I2C_ID_LEN]; static char byt_rt5640_codec_aif_name[12]; /* = "rt5640-aif[1|2]" */ static char byt_rt5640_cpu_dai_name[10]; /* = "ssp[0|2]-port" */ static char byt_rt5640_long_name[40]; /* = "bytcr-rt5640-*-spk-*-mic" */ +static char byt_rt5640_components[32]; /* = "cfg-spk:* cfg-mic:*" */ static int byt_rt5640_suspend(struct snd_soc_card *card) { @@ -1303,6 +1304,11 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev) } } + snprintf(byt_rt5640_components, sizeof(byt_rt5640_components), + "cfg-spk:%s cfg-mic:%s", + (byt_rt5640_quirk & BYT_RT5640_MONO_SPEAKER) ? "1" : "2", + map_name[BYT_RT5640_MAP(byt_rt5640_quirk)]); + byt_rt5640_card.components = byt_rt5640_components; snprintf(byt_rt5640_long_name, sizeof(byt_rt5640_long_name), "bytcr-rt5640-%s-spk-%s-mic", (byt_rt5640_quirk & BYT_RT5640_MONO_SPEAKER) ? diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c index 4606f6f582d6..80a5674ddb1b 100644 --- a/sound/soc/intel/boards/bytcr_rt5651.c +++ b/sound/soc/intel/boards/bytcr_rt5651.c @@ -798,6 +798,7 @@ static char byt_rt5651_codec_name[SND_ACPI_I2C_ID_LEN]; static char byt_rt5651_codec_aif_name[12]; /* = "rt5651-aif[1|2]" */ static char byt_rt5651_cpu_dai_name[10]; /* = "ssp[0|2]-port" */ static char byt_rt5651_long_name[50]; /* = "bytcr-rt5651-*-spk-*-mic[-swapped-hp]" */ +static char byt_rt5651_components[50]; /* = "cfg-spk:* cfg-mic:*" */ static int byt_rt5651_suspend(struct snd_soc_card *card) { @@ -876,7 +877,6 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) const char *platform_name; struct acpi_device *adev; struct device *codec_dev; - const char *hp_swapped; bool is_bytcr = false; int ret_val = 0; int dai_index = 0; @@ -1080,16 +1080,20 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev) } } - if (byt_rt5651_quirk & BYT_RT5651_HP_LR_SWAPPED) - hp_swapped = "-hp-swapped"; - else - hp_swapped = ""; - + snprintf(byt_rt5651_components, sizeof(byt_rt5651_components), + "cfg-spk:%s cfg-mic:%s%s", + (byt_rt5651_quirk & BYT_RT5651_MONO_SPEAKER) ? "1" : "2", + mic_name[BYT_RT5651_MAP(byt_rt5651_quirk)], + (byt_rt5651_quirk & BYT_RT5651_HP_LR_SWAPPED) ? + " cfg-hp:lrswap" : ""); + byt_rt5651_card.components = byt_rt5651_components; snprintf(byt_rt5651_long_name, sizeof(byt_rt5651_long_name), "bytcr-rt5651-%s-spk-%s-mic%s", (byt_rt5651_quirk & BYT_RT5651_MONO_SPEAKER) ? "mono" : "stereo", - mic_name[BYT_RT5651_MAP(byt_rt5651_quirk)], hp_swapped); + mic_name[BYT_RT5651_MAP(byt_rt5651_quirk)], + (byt_rt5651_quirk & BYT_RT5651_HP_LR_SWAPPED) ? + "-hp-swapped" : ""); byt_rt5651_card.long_name = byt_rt5651_long_name; /* override plaform name, if required */ -- 2.20.1

1 1

Re: [alsa-devel] [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer instead of jiffies
by Andrew Gabbasov 20 Nov '19

20 Nov '19

Hello Takashi, > -----Original Message----- > From: Takashi Iwai [mailto:tiwai@suse.de] > Sent: Wednesday, November 20, 2019 6:59 PM > To: Gabbasov, Andrew > Cc: alsa-devel(a)alsa-project.org; linux-kernel(a)vger.kernel.org; Jaroslav > Kysela; Takashi Iwai; Timo Wischer > Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer > instead of jiffies > > On Wed, 20 Nov 2019 16:39:00 +0100, > Andrew Gabbasov wrote: > > > > Hello Takashi, > > > > > -----Original Message----- > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > Sent: Wednesday, November 20, 2019 6:32 PM > > > To: Gabbasov, Andrew > > > Cc: alsa-devel(a)alsa-project.org; linux-kernel(a)vger.kernel.org; Jaroslav > > > Kysela; Takashi Iwai; Timo Wischer > > > Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of snd_timer > > > instead of jiffies > > > > > > On Wed, 20 Nov 2019 16:21:36 +0100, > > > Andrew Gabbasov wrote: > > > > > > > > Hello Takashi, > > > > > > > > > -----Original Message----- > > > > > From: Takashi Iwai [mailto:tiwai@suse.de] > > > > > Sent: Wednesday, November 20, 2019 5:34 PM > > > > > To: Gabbasov, Andrew > > > > > Cc: alsa-devel(a)alsa-project.org; linux-kernel(a)vger.kernel.org; > > Jaroslav > > > > > Kysela; Takashi Iwai; Timo Wischer > > > > > Subject: Re: [PATCH v4 6/7] ALSA: aloop: Support selection of > > snd_timer > > > > > instead of jiffies > > > > > > > > > > On Wed, 20 Nov 2019 12:58:55 +0100, > > > > > Andrew Gabbasov wrote: > > > > > > +/* call in loopback->cable_lock */ > > > > > > +static int loopback_snd_timer_open(struct loopback_pcm *dpcm) > > > > > > +{ > > > > > > + int err = 0; > > > > > > + struct snd_timer_id tid = { > > > > > > + .dev_class = SNDRV_TIMER_CLASS_PCM, > > > > > > + .dev_sclass = SNDRV_TIMER_SCLASS_APPLICATION, > > > > > > + }; > > > > > > + struct snd_timer_instance *timeri; > > > > > > + struct loopback_cable *cable = dpcm->cable; > > > > > > + > > > > > > + spin_lock_irq(&cable->lock); > > > > > > + > > > > > > + /* check if timer was already opened. It is only opened once > > > > > > + * per playback and capture subdevice (aka cable). > > > > > > + */ > > > > > > + if (cable->snd_timer.instance) > > > > > > + goto unlock; > > > > > > + > > > > > > + err = loopback_parse_timer_id(dpcm->loopback->timer_source, > > > &tid); > > > > > > + if (err < 0) { > > > > > > + pcm_err(dpcm->substream->pcm, > > > > > > + "Parsing timer source \'%s\' failed with > > %d", > > > > > > + dpcm->loopback->timer_source, err); > > > > > > + goto unlock; > > > > > > + } > > > > > > + > > > > > > + cable->snd_timer.stream = dpcm->substream->stream; > > > > > > + cable->snd_timer.id = tid; > > > > > > + > > > > > > + timeri = snd_timer_instance_new(dpcm->loopback->card->id); > > > > > > + if (!timeri) { > > > > > > + err = -ENOMEM; > > > > > > + goto unlock; > > > > > > + } > > > > > > + /* The callback has to be called from another tasklet. If > > > > > > + * SNDRV_TIMER_IFLG_FAST is specified it will be called from > > > the > > > > > > + * snd_pcm_period_elapsed() call of the selected sound card. > > > > > > + * snd_pcm_period_elapsed() helds > > > snd_pcm_stream_lock_irqsave(). > > > > > > + * Due to our callback loopback_snd_timer_function() also > > > calls > > > > > > + * snd_pcm_period_elapsed() which calls > > > > > snd_pcm_stream_lock_irqsave(). > > > > > > + * This would end up in a dead lock. > > > > > > + */ > > > > > > + timeri->flags |= SNDRV_TIMER_IFLG_AUTO; > > > > > > + timeri->callback = loopback_snd_timer_function; > > > > > > + timeri->callback_data = (void *)cable; > > > > > > + timeri->ccallback = loopback_snd_timer_event; > > > > > > + > > > > > > + /* snd_timer_close() and snd_timer_open() should not be > > called > > > with > > > > > > + * locked spinlock because both functions can block on a > > > mutex. The > > > > > > + * mutex loopback->cable_lock is kept locked. Therefore > > > > > snd_timer_open() > > > > > > + * cannot be called a second time by the other device of the > > > same > > > > > cable. > > > > > > + * Therefore the following issue cannot happen: > > > > > > + * [proc1] Call loopback_timer_open() -> > > > > > > + * Unlock cable->lock for snd_timer_close/open() > > call > > > > > > + * [proc2] Call loopback_timer_open() -> snd_timer_open(), > > > > > > + * snd_timer_start() > > > > > > + * [proc1] Call snd_timer_open() and overwrite running timer > > > > > > + * instance > > > > > > + */ > > > > > > + spin_unlock_irq(&cable->lock); > > > > > > + err = snd_timer_open(timeri, &cable->snd_timer.id, current- > > > >pid); > > > > > > + if (err < 0) { > > > > > > + pcm_err(dpcm->substream->pcm, > > > > > > + "snd_timer_open (%d,%d,%d) failed with %d", > > > > > > + cable->snd_timer.id.card, > > > > > > + cable->snd_timer.id.device, > > > > > > + cable->snd_timer.id.subdevice, > > > > > > + err); > > > > > > + snd_timer_instance_free(timeri); > > > > > > + return err; > > > > > > + } > > > > > > + spin_lock_irq(&cable->lock); > > > > > > + > > > > > > + cable->snd_timer.instance = timeri; > > > > > > + > > > > > > + /* initialise a tasklet used for draining */ > > > > > > + tasklet_init(&cable->snd_timer.event_tasklet, > > > > > > + loopback_snd_timer_tasklet, (unsigned > > > long)timeri); > > > > > > > > > > This has to be set before snd_timer_open(). The callback might be > > > > > called immediately after snd_timer_open(). > > > > > > > > This tasklet is used/scheduled only in ccallback (not regular tick > > > > callback), > > > > and only for SNDRV_TIMER_EVENT_MSTOP event. Can this event really > happen > > > > immediately after snd_timer_open()? > > > > > > Why not? The master timer can be stopped at any time, even between > > > these two lines. > > > > > > Beware that there are fuzzer programs that can trigger such racy > > > things, and you're adding the code to the target that is actively > > > slapped by them :) > > > > OK, got it. > > I'll move this initialization to before snd_timer_open() in the next > > update together with the fixes for the other issues you will find > > in this version. > > I have no other issues, so you can just resubmit only that patch, > too. I'm not sure how to correctly format resubmitting of a single patch from a patch set, so I'm submitting the next version v5 of the whole patch set: https://mailman.alsa-project.org/pipermail/alsa-devel/2019-November/158939.h tml Thanks! Best regards, Andrew

1 0