Hi,
this is a patch set as a follow up of the thread about the errors reported by kselftest mixer-test. It changes HD-audio and vmaster control behavior to return -EINVAL for invalid input values.
There is a change in kselftest itself to skip the write tests for volatile controls, too. It's for the channel map controls that can't hold the stable values.
Takashi
===
Takashi Iwai (5): ALSA: vmaster: Return error for invalid input values ALSA: hda: Return -EINVAL for invalid volume/switch inputs ALSA: control: Apply sanity check of input values for user elements kselftest/alsa: mixer-test: Skip write tests for volatile controls ALSA: chmap: Mark Channel Map controls as volatile
sound/core/control.c | 3 ++- sound/core/pcm_lib.c | 1 + sound/core/vmaster.c | 8 ++++++++ sound/pci/hda/hda_codec.c | 23 ++++++++++++++++++----- tools/testing/selftests/alsa/mixer-test.c | 21 +++++++++++++++++++++ 5 files changed, 50 insertions(+), 6 deletions(-)
So far the vmaster code has been tolerant about the input values and accepts any values by correcting internally. But now our own selftest starts complaining about this behavior, so let's be picky and change the behavior to return -EINVAL for invalid input values instead.
Reported-by: Paul Menzel pmenzel@molgen.mpg.de Closes: https://lore.kernel.org/r/1d44be36-9bb9-4d82-8953-5ae2a4f09405@molgen.mpg.de Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/vmaster.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/sound/core/vmaster.c b/sound/core/vmaster.c index 04a57f7be6ea..c657659b236c 100644 --- a/sound/core/vmaster.c +++ b/sound/core/vmaster.c @@ -198,6 +198,12 @@ static int follower_put(struct snd_kcontrol *kcontrol, err = follower_init(follower); if (err < 0) return err; + for (ch = 0; ch < follower->info.count; ch++) { + if (ucontrol->value.integer.value[ch] < follower->info.min_val || + ucontrol->value.integer.value[ch] > follower->info.max_val) + return -EINVAL; + } + for (ch = 0; ch < follower->info.count; ch++) { if (follower->vals[ch] != ucontrol->value.integer.value[ch]) { changed = 1; @@ -365,6 +371,8 @@ static int master_put(struct snd_kcontrol *kcontrol, new_val = ucontrol->value.integer.value[0]; if (new_val == old_val) return 0; + if (new_val < master->info.min_val || new_val > master->info.max_val) + return -EINVAL;
err = sync_followers(master, old_val, new_val); if (err < 0)
So far the HD-audio driver has been tolerant about the input values and accepts any values by correcting the amp volume and switch values internally. But now our own selftest starts complaining about this behavior, so let's be picky and change the behavior to return -EINVAL for invalid input values instead.
Reported-by: Paul Menzel pmenzel@molgen.mpg.de Closes: https://lore.kernel.org/r/1d44be36-9bb9-4d82-8953-5ae2a4f09405@molgen.mpg.de Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/hda_codec.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 325e8f0b99a8..3dd1bda0c5c6 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -1496,7 +1496,7 @@ update_amp_value(struct hda_codec *codec, hda_nid_t nid, /* ofs = 0: raw max value */ maxval = get_amp_max_value(codec, nid, dir, 0); if (val > maxval) - val = maxval; + return -EINVAL; return snd_hda_codec_amp_update(codec, nid, ch, dir, idx, HDA_AMP_VOLMASK, val); } @@ -1547,13 +1547,21 @@ int snd_hda_mixer_amp_volume_put(struct snd_kcontrol *kcontrol, unsigned int ofs = get_amp_offset(kcontrol); long *valp = ucontrol->value.integer.value; int change = 0; + int err;
if (chs & 1) { - change = update_amp_value(codec, nid, 0, dir, idx, ofs, *valp); + err = update_amp_value(codec, nid, 0, dir, idx, ofs, *valp); + if (err < 0) + return err; + change |= err; valp++; } - if (chs & 2) - change |= update_amp_value(codec, nid, 1, dir, idx, ofs, *valp); + if (chs & 2) { + err = update_amp_value(codec, nid, 1, dir, idx, ofs, *valp); + if (err < 0) + return err; + change |= err; + } return change; } EXPORT_SYMBOL_GPL(snd_hda_mixer_amp_volume_put); @@ -2149,15 +2157,20 @@ int snd_hda_mixer_amp_switch_put(struct snd_kcontrol *kcontrol, int change = 0;
if (chs & 1) { + if (*valp < 0 || *valp > 1) + return -EINVAL; change = snd_hda_codec_amp_update(codec, nid, 0, dir, idx, HDA_AMP_MUTE, *valp ? 0 : HDA_AMP_MUTE); valp++; } - if (chs & 2) + if (chs & 2) { + if (*valp < 0 || *valp > 1) + return -EINVAL; change |= snd_hda_codec_amp_update(codec, nid, 1, dir, idx, HDA_AMP_MUTE, *valp ? 0 : HDA_AMP_MUTE); + } hda_call_check_power_status(codec, nid); return change; }
Although we have already a mechanism for sanity checks of input values for control writes, it's not applied unless the kconfig CONFIG_SND_CTL_INPUT_VALIDATION is set due to the performance reason. Nevertheless, it still makes sense to apply the check for user elements despite of its cost, as that's the only way to filter out the invalid values; the user controls are handled solely in ALSA core code, and there is no corresponding driver, after all.
This patch enables the input value validation for user control elements no matter whether CONFIG_SND_CTL_INPUT_VALIDATION is set or not. The kselftest will be happier with this change, as the incorrect values will be bailed out now with errors.
For other normal controls, the check is applied still only when CONFIG_SND_CTL_INPUT_VALIDATION is set.
Reported-by: Paul Menzel pmenzel@molgen.mpg.de Closes: https://lore.kernel.org/r/1d44be36-9bb9-4d82-8953-5ae2a4f09405@molgen.mpg.de Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/control.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/core/control.c b/sound/core/control.c index fb0c60044f7b..50890983d7e2 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -1317,7 +1317,8 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file, snd_ctl_build_ioff(&control->id, kctl, index_offset); result = snd_power_ref_and_wait(card); /* validate input values */ - if (IS_ENABLED(CONFIG_SND_CTL_INPUT_VALIDATION) && !result) { + if ((IS_ENABLED(CONFIG_SND_CTL_INPUT_VALIDATION) || + (vd->access & SNDRV_CTL_ELEM_ACCESS_USER)) && !result) { struct snd_ctl_elem_info info;
memset(&info, 0, sizeof(info));
On 14. 06. 24 14:47, Takashi Iwai wrote:
Although we have already a mechanism for sanity checks of input values for control writes, it's not applied unless the kconfig CONFIG_SND_CTL_INPUT_VALIDATION is set due to the performance reason. Nevertheless, it still makes sense to apply the check for user elements despite of its cost, as that's the only way to filter out the invalid values; the user controls are handled solely in ALSA core code, and there is no corresponding driver, after all.
This patch enables the input value validation for user control elements no matter whether CONFIG_SND_CTL_INPUT_VALIDATION is set or not. The kselftest will be happier with this change, as the incorrect values will be bailed out now with errors.
For other normal controls, the check is applied still only when CONFIG_SND_CTL_INPUT_VALIDATION is set.
Reported-by: Paul Menzel pmenzel@molgen.mpg.de Closes: https://lore.kernel.org/r/1d44be36-9bb9-4d82-8953-5ae2a4f09405@molgen.mpg.de Signed-off-by: Takashi Iwai tiwai@suse.de
sound/core/control.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sound/core/control.c b/sound/core/control.c index fb0c60044f7b..50890983d7e2 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -1317,7 +1317,8 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file, snd_ctl_build_ioff(&control->id, kctl, index_offset); result = snd_power_ref_and_wait(card); /* validate input values */
- if (IS_ENABLED(CONFIG_SND_CTL_INPUT_VALIDATION) && !result) {
- if ((IS_ENABLED(CONFIG_SND_CTL_INPUT_VALIDATION) ||
(vd->access & SNDRV_CTL_ELEM_ACCESS_USER)) && !result) {
The info structure for user elements may be used directly without this intermediate variable for the validation. But it may be a possible future improvement.
Jaroslav
On Fri, Jun 14, 2024 at 02:47:25PM +0200, Takashi Iwai wrote:
Although we have already a mechanism for sanity checks of input values for control writes, it's not applied unless the kconfig CONFIG_SND_CTL_INPUT_VALIDATION is set due to the performance reason. Nevertheless, it still makes sense to apply the check for user elements despite of its cost, as that's the only way to filter out the invalid values; the user controls are handled solely in ALSA core code, and there is no corresponding driver, after all.
This patch enables the input value validation for user control elements no matter whether CONFIG_SND_CTL_INPUT_VALIDATION is set or not. The kselftest will be happier with this change, as the incorrect values will be bailed out now with errors.
Reviewed-by: Mark Brown broonie@kernel.org
The control elements with volatile flag don't guarantee that the written values are actually saved for the next reads, hence they aren't suitable for the standard mixer tests. Skip the write tests for those volatile controls for avoiding confusion.
Reported-by: Paul Menzel pmenzel@molgen.mpg.de Closes: https://lore.kernel.org/r/1d44be36-9bb9-4d82-8953-5ae2a4f09405@molgen.mpg.de Signed-off-by: Takashi Iwai tiwai@suse.de --- tools/testing/selftests/alsa/mixer-test.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)
diff --git a/tools/testing/selftests/alsa/mixer-test.c b/tools/testing/selftests/alsa/mixer-test.c index 1c04e5f638a0..3c9a45fb5a29 100644 --- a/tools/testing/selftests/alsa/mixer-test.c +++ b/tools/testing/selftests/alsa/mixer-test.c @@ -702,6 +702,13 @@ static void test_ctl_write_default(struct ctl_data *ctl) return; }
+ if (snd_ctl_elem_info_is_volatile(ctl->info)) { + ksft_print_msg("%s is volatile\n", ctl->name); + ksft_test_result_skip("write_default.%d.%d\n", + ctl->card->card, ctl->elem); + return; + } + err = write_and_verify(ctl, ctl->def_val, NULL);
ksft_test_result(err >= 0, "write_default.%d.%d\n", @@ -827,6 +834,13 @@ static void test_ctl_write_valid(struct ctl_data *ctl) return; }
+ if (snd_ctl_elem_info_is_volatile(ctl->info)) { + ksft_print_msg("%s is volatile\n", ctl->name); + ksft_test_result_skip("write_valid.%d.%d\n", + ctl->card->card, ctl->elem); + return; + } + switch (snd_ctl_elem_info_get_type(ctl->info)) { case SND_CTL_ELEM_TYPE_BOOLEAN: pass = test_ctl_write_valid_boolean(ctl); @@ -1039,6 +1053,13 @@ static void test_ctl_write_invalid(struct ctl_data *ctl) return; }
+ if (!snd_ctl_elem_info_is_volatile(ctl->info)) { + ksft_print_msg("%s is volatile\n", ctl->name); + ksft_test_result_skip("write_invalid.%d.%d\n", + ctl->card->card, ctl->elem); + return; + } + switch (snd_ctl_elem_info_get_type(ctl->info)) { case SND_CTL_ELEM_TYPE_BOOLEAN: pass = test_ctl_write_invalid_boolean(ctl);
On Fri, Jun 14, 2024 at 02:47:26PM +0200, Takashi Iwai wrote:
The control elements with volatile flag don't guarantee that the written values are actually saved for the next reads, hence they aren't suitable for the standard mixer tests. Skip the write tests for those volatile controls for avoiding confusion.
We should still verify that you can actually write I think...
- if (!snd_ctl_elem_info_is_volatile(ctl->info)) {
ksft_print_msg("%s is volatile\n", ctl->name);ksft_test_result_skip("write_invalid.%d.%d\n",ctl->card->card, ctl->elem);return;- }
...and that you don't read back invalid values after a write like this for example. I think any change for this should be in the validation of the read but we should still try the writes we think we can do.
On Fri, 14 Jun 2024 16:29:03 +0200, Mark Brown wrote:
On Fri, Jun 14, 2024 at 02:47:26PM +0200, Takashi Iwai wrote:
The control elements with volatile flag don't guarantee that the written values are actually saved for the next reads, hence they aren't suitable for the standard mixer tests. Skip the write tests for those volatile controls for avoiding confusion.
We should still verify that you can actually write I think...
- if (!snd_ctl_elem_info_is_volatile(ctl->info)) {
ksft_print_msg("%s is volatile\n", ctl->name);ksft_test_result_skip("write_invalid.%d.%d\n",ctl->card->card, ctl->elem);return;- }
...and that you don't read back invalid values after a write like this for example. I think any change for this should be in the validation of the read but we should still try the writes we think we can do.
OK, makes sense. Will respin this one.
thanks,
Takashi
The values returned from Playback Channel Map and Capture Channel Map controls may vary dynamically depending on the corresponding PCM stream. Mark those as volatile to indicate the values are unstable and not suitable for testing.
Note that we may change the driver to return -EINVAL, but this would bring other side effects, such as "alsactl restore" would start receiving unexpected errors. So we still keep returning 0 for those invalid inputs.
Reported-by: Paul Menzel pmenzel@molgen.mpg.de Closes: https://lore.kernel.org/r/1d44be36-9bb9-4d82-8953-5ae2a4f09405@molgen.mpg.de Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/core/pcm_lib.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 6f73b3c2c205..071c67cbc479 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -2556,6 +2556,7 @@ int snd_pcm_add_chmap_ctls(struct snd_pcm *pcm, int stream, struct snd_kcontrol_new knew = { .iface = SNDRV_CTL_ELEM_IFACE_PCM, .access = SNDRV_CTL_ELEM_ACCESS_READ | + SNDRV_CTL_ELEM_ACCESS_VOLATILE | SNDRV_CTL_ELEM_ACCESS_TLV_READ | SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK, .info = pcm_chmap_ctl_info,
On 14. 06. 24 14:47, Takashi Iwai wrote:
Hi,
this is a patch set as a follow up of the thread about the errors reported by kselftest mixer-test. It changes HD-audio and vmaster control behavior to return -EINVAL for invalid input values.
There is a change in kselftest itself to skip the write tests for volatile controls, too. It's for the channel map controls that can't hold the stable values.
Thanks.
Reviewed-by: Jaroslav Kysela perex@perex.cz
linux-kselftest-mirror@lists.linaro.org
-
Jaroslav Kysela -
Mark Brown -
Takashi Iwai