[PATCH] ALSA: hda: Fix the control element identification for multiple codecs

[PATCH] ALSA: hda: Fix the control element identification for multiple codecs

[Jaroslav Kysela]

Some motherboards have multiple HDA codecs connected to the serial bus.
The current code may create multiple mixer controls with the almost
identical identification.

The current code use id.device field from the control element structure
to store the codec address to avoid such clashes for multiple codecs.
Unfortunately, the user space do not handle this correctly. For mixer
controls, only name and index are used for the identifiers.

This patch fixes this problem to compose the index using the codec
address as an offset in case, when the control already exists. It is
really unlikely that one codec will create 10 similar controls.

This patch adds new kernel module parameter 'ctl_dev_id' to allow
select the old behaviour, too. The CONFIG_SND_HDA_CTL_DEV_ID Kconfig
option sets the default value.

BugLink: https://github.com/alsa-project/alsa-lib/issues/294
BugLink: https://github.com/alsa-project/alsa-lib/issues/205
Fixes: 54d174031576 ("[ALSA] hda-codec - Fix connection list parsing")
Fixes: 1afe206ab699 ("ALSA: hda - Try to find an empty control index when it's occupied")
Signed-off-by: Jaroslav Kysela <perex@perex.cz>

--

rfc..v1:
 - added CONFIG_SND_HDA_CTL_DEV_ID Kconfig option
---
 include/sound/hda_codec.h      |  1 +
 sound/pci/hda/Kconfig          | 14 ++++++++++++++
 sound/pci/hda/hda_codec.c      | 13 ++++++++++---
 sound/pci/hda/hda_controller.c |  1 +
 sound/pci/hda/hda_controller.h |  1 +
 sound/pci/hda/hda_intel.c      |  6 ++++++
 6 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h
index eba23daf2c29..bbb7805e85d8 100644
--- a/include/sound/hda_codec.h
+++ b/include/sound/hda_codec.h
@@ -259,6 +259,7 @@ struct hda_codec {
 	unsigned int relaxed_resume:1;	/* don't resume forcibly for jack */
 	unsigned int forced_resume:1; /* forced resume for jack */
 	unsigned int no_stream_clean_at_suspend:1; /* do not clean streams at suspend */
+	unsigned int ctl_dev_id:1; /* old control element id build behaviour */
 
 #ifdef CONFIG_PM
 	unsigned long power_on_acct;
diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
index 06d304db4183..886255a03e8b 100644
--- a/sound/pci/hda/Kconfig
+++ b/sound/pci/hda/Kconfig
@@ -302,6 +302,20 @@ config SND_HDA_INTEL_HDMI_SILENT_STREAM
 	  This feature can impact power consumption as resources
 	  are kept reserved both at transmitter and receiver.
 
+config SND_HDA_CTL_DEV_ID
+	bool "Use the device identifier field for controls"
+	depends on SND_HDA_INTEL
+	help
+	  Say Y to use the device identifier field for (mixer)
+	  controls (old behaviour until this option is available).
+
+	  When enabled, the multiple HDA codecs may set the device
+	  field in control (mixer) element identifiers. The use
+	  of this field is not recommended and defined for mixer controls.
+
+	  The old behaviour (Y) is obsolete and will be removed. Consider
+	  to not enable this option.
+
 endif
 
 endmenu
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c
index edd653ece70d..8f20abc036bf 100644
--- a/sound/pci/hda/hda_codec.c
+++ b/sound/pci/hda/hda_codec.c
@@ -3390,7 +3390,12 @@ int snd_hda_add_new_ctls(struct hda_codec *codec,
 			kctl = snd_ctl_new1(knew, codec);
 			if (!kctl)
 				return -ENOMEM;
-			if (addr > 0)
+			/* Do not use the id.device field for MIXER elements.
+			 * This field is for real device numbers (like PCM) but codecs
+			 * are hidden components from the user space view (unrelated
+			 * to the mixer element identification).
+			 */
+			if (addr > 0 && codec->ctl_dev_id)
 				kctl->id.device = addr;
 			if (idx > 0)
 				kctl->id.index = idx;
@@ -3401,9 +3406,11 @@ int snd_hda_add_new_ctls(struct hda_codec *codec,
 			 * the codec addr; if it still fails (or it's the
 			 * primary codec), then try another control index
 			 */
-			if (!addr && codec->core.addr)
+			if (!addr && codec->core.addr) {
 				addr = codec->core.addr;
-			else if (!idx && !knew->index) {
+				if (!codec->ctl_dev_id)
+					idx += 10 * addr;
+			} else if (!idx && !knew->index) {
 				idx = find_empty_mixer_ctl_idx(codec,
 							       knew->name, 0);
 				if (idx <= 0)
diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c
index 0ff286b7b66b..083df287c1a4 100644
--- a/sound/pci/hda/hda_controller.c
+++ b/sound/pci/hda/hda_controller.c
@@ -1231,6 +1231,7 @@ int azx_probe_codecs(struct azx *chip, unsigned int max_slots)
 				continue;
 			codec->jackpoll_interval = chip->jackpoll_interval;
 			codec->beep_mode = chip->beep_mode;
+			codec->ctl_dev_id = chip->ctl_dev_id;
 			codecs++;
 		}
 	}
diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h
index f5bf295eb830..8556031bcd68 100644
--- a/sound/pci/hda/hda_controller.h
+++ b/sound/pci/hda/hda_controller.h
@@ -124,6 +124,7 @@ struct azx {
 	/* HD codec */
 	int  codec_probe_mask; /* copied from probe_mask option */
 	unsigned int beep_mode;
+	bool ctl_dev_id;
 
 #ifdef CONFIG_SND_HDA_PATCH_LOADER
 	const struct firmware *fw;
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 87002670c0c9..048656234280 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -119,6 +119,8 @@ static bool beep_mode[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] =
 					CONFIG_SND_HDA_INPUT_BEEP_MODE};
 #endif
 static bool dmic_detect = 1;
+static bool ctl_dev_id[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] =
+					CONFIG_SND_HDA_CTL_DEV_ID};
 
 module_param_array(index, int, NULL, 0444);
 MODULE_PARM_DESC(index, "Index value for Intel HD audio interface.");
@@ -157,6 +159,8 @@ module_param(dmic_detect, bool, 0444);
 MODULE_PARM_DESC(dmic_detect, "Allow DSP driver selection (bypass this driver) "
 			     "(0=off, 1=on) (default=1); "
 		 "deprecated, use snd-intel-dspcfg.dsp_driver option instead");
+module_param_array(ctl_dev_id, bool, NULL, 0444);
+MODULE_PARM_DESC(ctl_dev_id, "Use control device identifier (based on codec address).");
 
 #ifdef CONFIG_PM
 static int param_set_xint(const char *val, const struct kernel_param *kp);
@@ -2278,6 +2282,8 @@ static int azx_probe_continue(struct azx *chip)
 	chip->beep_mode = beep_mode[dev];
 #endif
 
+	chip->ctl_dev_id = ctl_dev_id[dev];
+
 	/* create codec instances */
 	if (bus->codec_mask) {
 		err = azx_probe_codecs(chip, azx_max_codecs[chip->driver_type]);
-- 
2.39.0

Re: [PATCH] ALSA: hda: Fix the control element identification for multiple codecs

[Takashi Iwai]

On Tue, 31 Jan 2023 10:42:15 +0100,
Jaroslav Kysela wrote:
> 
> Some motherboards have multiple HDA codecs connected to the serial bus.
> The current code may create multiple mixer controls with the almost
> identical identification.
> 
> The current code use id.device field from the control element structure
> to store the codec address to avoid such clashes for multiple codecs.
> Unfortunately, the user space do not handle this correctly. For mixer
> controls, only name and index are used for the identifiers.
> 
> This patch fixes this problem to compose the index using the codec
> address as an offset in case, when the control already exists. It is
> really unlikely that one codec will create 10 similar controls.
> 
> This patch adds new kernel module parameter 'ctl_dev_id' to allow
> select the old behaviour, too. The CONFIG_SND_HDA_CTL_DEV_ID Kconfig
> option sets the default value.
> 
> BugLink: https://github.com/alsa-project/alsa-lib/issues/294
> BugLink: https://github.com/alsa-project/alsa-lib/issues/205
> Fixes: 54d174031576 ("[ALSA] hda-codec - Fix connection list parsing")
> Fixes: 1afe206ab699 ("ALSA: hda - Try to find an empty control index when it's occupied")
> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
> 
> --
> 
> rfc..v1:
>  - added CONFIG_SND_HDA_CTL_DEV_ID Kconfig option

Looks almost fine.  One thing would be to just make it a single option
instead of array.  This behavior should be consistent on the whole
system, not specific to a certain chip, after all, and the array
option is error-prone.


thanks,

Takashi

Re: [PATCH] ALSA: hda: Fix the control element identification for multiple codecs

[kernel test robot]

Hi Jaroslav,

I love your patch! Yet something to improve:

[auto build test ERROR on tiwai-sound/for-next]
[also build test ERROR on tiwai-sound/for-linus linus/master v6.2-rc6 next-20230202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jaroslav-Kysela/ALSA-hda-Fix-the-control-element-identification-for-multiple-codecs/20230131-174413
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
patch link:    https://lore.kernel.org/r/20230131094215.3545993-1-perex%40perex.cz
patch subject: [PATCH] ALSA: hda: Fix the control element identification for multiple codecs
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20230202/202302021656.nqNSqtwW-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/8dfc91ec424bfc92232d31eadddd1901fa5c65f6
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Jaroslav-Kysela/ALSA-hda-Fix-the-control-element-identification-for-multiple-codecs/20230131-174413
        git checkout 8dfc91ec424bfc92232d31eadddd1901fa5c65f6
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 olddefconfig
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> sound/pci/hda/hda_intel.c:123:41: error: 'CONFIG_SND_HDA_CTL_DEV_ID' undeclared here (not in a function); did you mean 'CONFIG_SND_HDA_CORE'?
     123 |                                         CONFIG_SND_HDA_CTL_DEV_ID};
         |                                         ^~~~~~~~~~~~~~~~~~~~~~~~~
         |                                         CONFIG_SND_HDA_CORE


vim +123 sound/pci/hda/hda_intel.c

   101	
   102	
   103	static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;
   104	static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;
   105	static bool enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE_PNP;
   106	static char *model[SNDRV_CARDS];
   107	static int position_fix[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] = -1};
   108	static int bdl_pos_adj[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] = -1};
   109	static int probe_mask[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] = -1};
   110	static int probe_only[SNDRV_CARDS];
   111	static int jackpoll_ms[SNDRV_CARDS];
   112	static int single_cmd = -1;
   113	static int enable_msi = -1;
   114	#ifdef CONFIG_SND_HDA_PATCH_LOADER
   115	static char *patch[SNDRV_CARDS];
   116	#endif
   117	#ifdef CONFIG_SND_HDA_INPUT_BEEP
   118	static bool beep_mode[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] =
   119						CONFIG_SND_HDA_INPUT_BEEP_MODE};
   120	#endif
   121	static bool dmic_detect = 1;
   122	static bool ctl_dev_id[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] =
 > 123						CONFIG_SND_HDA_CTL_DEV_ID};
   124	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests