snd-core: enlarge snd_card.components for up to 4 codecs

PATCH In review Wu Fengguang 3 participants 8 messages Oct 7, 2008 Sound

Share & tools

Wu Fengguang

author

2008-10-07 06:56 UTC #0 lore raw

PATCH 1/1 snd-core: enlarge snd_card.components for up to 4 codecs

Enlarge snd_card.components[80] to 128 bytes, with space for 4 codecs. The previous size 80 cannot support HP 2230s which has 3 codecs.

Signed-off-by Wu Fengguang <[email protected]>

---

include/sound/asound.h +1 -1

index ca2f358..b3801b0 100644
@@ -708,7 +708,7 @@ struct snd_ctl_card_info {
 	unsigned char longname[80];	/* name + info text about soundcard */
 	unsigned char reserved_[16];	/* reserved for future (was ID of mixer) */
 	unsigned char mixername[80];	/* visual mixer identification */
-	unsigned char components[80];	/* card components / fine identification, delimited with one space (AC97 etc..) */
+	unsigned char components[128];	/* card components / fine identification, delimited with one space (AC97 etc..) */
 	unsigned char reserved[48];	/* reserved for future */
 };

include/sound/core.h +1 -1

index f52ab6f..dcbc424 100644
@@ -116,7 +116,7 @@ struct snd_card {
 	char shortname[32];		/* short name of this soundcard */
 	char longname[80];		/* name of this soundcard */
 	char mixername[80];		/* mixer name */
-	char components[80];		/* card components delimited with
+	char components[128];		/* card components delimited with
 								space */
 	struct module *module;		/* top-level module */

Takashi Iwai

2008-10-07 08:10 UTC #1 lore raw

At Tue, 7 Oct 2008 14:56:06 +0800,

Quoted 5 lines · Wu Fengguang

Enlarge snd_card.components[80] to 128 bytes, with space for 4 codecs. The previous size 80 cannot support HP 2230s which has 3 codecs.
Signed-off-by Wu Fengguang <[email protected]>

Thanks for the patch. Unfortunately, this can't be applied as is. struct snd_ctl_card_info is exported and used as the ioctl parameter, thus you cannot change the size.

One possible fix is to merge reserved[48] into components[80] so that the struct size is kept as is.
But still we have to be very careful to such a change, and need to investigate all possible places to refer it.

Takashi

Quoted 29 lines

---

include/sound/asound.h +1 -1

index ca2f358..b3801b0 100644
@@ -708,7 +708,7 @@ struct snd_ctl_card_info {
 	unsigned char longname[80];	/* name + info text about soundcard */
 	unsigned char reserved_[16];	/* reserved for future (was ID of mixer) */
 	unsigned char mixername[80];	/* visual mixer identification */
-	unsigned char components[80];	/* card components / fine identification, delimited with one space (AC97 etc..) */
+	unsigned char components[128];	/* card components / fine identification, delimited with one space (AC97 etc..) */
 	unsigned char reserved[48];	/* reserved for future */
 };

include/sound/core.h +1 -1

index f52ab6f..dcbc424 100644
@@ -116,7 +116,7 @@ struct snd_card {
 	char shortname[32];		/* short name of this soundcard */
 	char longname[80];		/* name of this soundcard */
 	char mixername[80];		/* mixer name */
-	char components[80];		/* card components delimited with
+	char components[128];		/* card components delimited with
 								space */
 	struct module *module;		/* top-level module */

Wu Fengguang

author

2008-10-07 08:34 UTC #2 lore raw

Quoted 8 lines · Takashi Iwai

At Tue, 7 Oct 2008 14:56:06 +0800,
Quoted 5 lines · Wu Fengguang
Enlarge snd_card.components[80] to 128 bytes, with space for 4 codecs. The previous size 80 cannot support HP 2230s which has 3 codecs.
Signed-off-by Wu Fengguang <[email protected]>

Quoted 8 lines

Thanks for the patch. Unfortunately, this can't be applied as is. struct snd_ctl_card_info is exported and used as the ioctl parameter, thus you cannot change the size.
One possible fix is to merge reserved[48] into components[80] so that the struct size is kept as is.
But still we have to be very careful to such a change, and need to investigate all possible places to refer it.

OK.

FYI, this is the debug patch and kernel message(lookout the first line) on HP 2230s:

sound/core/init.c +2 -0

index 8af467d..704d7e8 100644
@@ -706,6 +706,8 @@ int snd_component_add(struct snd_card *card, const char *component)
 			return 1;
 	}
 	if (strlen(card->components) + 1 + len + 1 > sizeof(card->components)) {
+		snd_printk(KERN_ERR "components = %s + %s\n",
+				card->components, component);
 		snd_BUG();
 		return -ENOMEM;
 	}

[   12.590150] ALSA sound/core/init.c:710: components = HDA:11d4194a,103c3037,00100400 HDA:11c11040,103c1378,00100200 + HDA:80862802,80860101,00100000
[   12.590163] ------------[ cut here ]------------
[   12.590166] WARNING: at sound/core/init.c:711 snd_component_add+0xfa/0x110 [snd]()
[   12.590168] BUG?
[   12.590170] Modules linked in: snd_hda_intel(+) snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_pcm_oss snd_mixer_oss iwlagn snd_pcm snd_timer iwlcore snd_page_alloc snd_hwdep rfkill snd pcspkr led_class sky2 wmi soundcore uhci_hcd ohci_hcd ehci_hcd
[   12.590197] Pid: 2410, comm: modprobe Not tainted 2.6.27-rc7 #4
[   12.590200] 
[   12.590200] Call Trace:
[   12.590209]  [<ffffffff81045d47>] warn_slowpath+0xb7/0xf0
[   12.590213]  [<ffffffff8106c000>] ? static_obj+0x80/0x90
[   12.590217]  [<ffffffff8106e29d>] ? trace_hardirqs_on+0xd/0x10
[   12.590220]  [<ffffffff8106c065>] ? lockdep_init_map+0x55/0x5c0
[   12.590225]  [<ffffffff811e37a9>] ? __next_cpu+0x19/0x30
[   12.590234]  [<ffffffffa003cbb5>] ? snd_verbose_printk+0x85/0xe0 [snd]
[   12.590238]  [<ffffffff811eb4d8>] ? sprintf+0x68/0x70
[   12.590242]  [<ffffffff810607c4>] ? __mutex_init+0x54/0x70
[   12.590247]  [<ffffffffa004ede8>] ? snd_hwdep_new+0xc8/0x150 [snd_hwdep]
[   12.590255]  [<ffffffffa0036eea>] snd_component_add+0xfa/0x110 [snd]
[   12.590273]  [<ffffffffa01056ce>] snd_hda_codec_new+0x45d/0x68e [snd_hda_intel]
[   12.590290]  [<ffffffffa01046bc>] azx_probe+0x4dc/0xdb0 [snd_hda_intel]
[   12.590306]  [<ffffffffa00e1490>] ? azx_send_cmd+0x0/0x170 [snd_hda_intel]
[   12.590321]  [<ffffffffa00e1600>] ? azx_get_response+0x0/0x2a0 [snd_hda_intel]
[   12.590336]  [<ffffffffa00e1080>] ? azx_power_notify+0x0/0x80 [snd_hda_intel]
[   12.590341]  [<ffffffff81201048>] pci_device_probe+0xd8/0x130
[   12.590346]  [<ffffffff8112e75e>] ? sysfs_create_link+0xe/0x10
[   12.590351]  [<ffffffff812c1fe2>] driver_probe_device+0xa2/0x1e0
[   12.590355]  [<ffffffff812c21ab>] __driver_attach+0x8b/0x90
[   12.590358]  [<ffffffff812c2120>] ? __driver_attach+0x0/0x90
[   12.590361]  [<ffffffff812c177b>] bus_for_each_dev+0x6b/0xa0
[   12.590365]  [<ffffffff812c1e2c>] driver_attach+0x1c/0x20
[   12.590368]  [<ffffffff812c0f98>] bus_add_driver+0x208/0x280
[   12.590372]  [<ffffffff812c2393>] driver_register+0x73/0x170
[   12.590376]  [<ffffffff8120131d>] __pci_register_driver+0x7d/0xd0
[   12.590388]  [<ffffffffa016d000>] ? alsa_card_azx_init+0x0/0x20 [snd_hda_intel]
[   12.590400]  [<ffffffffa016d01e>] alsa_card_azx_init+0x1e/0x20 [snd_hda_intel]
[   12.590404]  [<ffffffff81009040>] _stext+0x40/0x1a0
[   12.590407]  [<ffffffff8106e239>] ? trace_hardirqs_on_caller+0x129/0x180
[   12.590413]  [<ffffffff8107bf1f>] sys_init_module+0xaf/0x1e0
[   12.590416]  [<ffffffff8100c60a>] system_call_fastpath+0x16/0x1b
[   12.590418] 
[   12.590421] ---[ end trace 5ac18ecbb660a8b0 ]---

Cheers,
Fengguang

Takashi Iwai

2008-10-07 09:00 UTC #3 lore raw

At Tue, 7 Oct 2008 16:34:29 +0800,

Quoted 38 lines · Wu Fengguang

Quoted 8 lines · Takashi Iwai
At Tue, 7 Oct 2008 14:56:06 +0800,
Quoted 5 lines · Wu Fengguang
Enlarge snd_card.components[80] to 128 bytes, with space for 4 codecs. The previous size 80 cannot support HP 2230s which has 3 codecs.
Signed-off-by Wu Fengguang <[email protected]>
Quoted 8 lines
Thanks for the patch. Unfortunately, this can't be applied as is. struct snd_ctl_card_info is exported and used as the ioctl parameter, thus you cannot change the size.
One possible fix is to merge reserved[48] into components[80] so that the struct size is kept as is.
But still we have to be very careful to such a change, and need to investigate all possible places to refer it.
OK.
FYI, this is the debug patch and kernel message(lookout the first line) on HP 2230s:
sound/core/init.c +2 -0
index 8af467d..704d7e8 100644
@@ -706,6 +706,8 @@ int snd_component_add(struct snd_card *card, const char *component)
 			return 1;
 	}
 	if (strlen(card->components) + 1 + len + 1 > sizeof(card->components)) {
+		snd_printk(KERN_ERR "components = %s + %s\n",
+				card->components, component);
 		snd_BUG();
 		return -ENOMEM;
 	}

Quoted 6 lines

[   12.590150] ALSA sound/core/init.c:710: components = HDA:11d4194a,103c3037,00100400 HDA:11c11040,103c1378,00100200 + HDA:80862802,80860101,00100000
[   12.590163] ------------[ cut here ]------------
[   12.590166] WARNING: at sound/core/init.c:711 snd_component_add+0xfa/0x110 [snd]()
[   12.590168] BUG?
[   12.590170] Modules linked in: snd_hda_intel(+) snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_pcm_oss snd_mixer_oss iwlagn snd_pcm snd_timer iwlcore snd_page_alloc snd_hwdep rfkill snd pcspkr led_class sky2 wmi soundcore uhci_hcd ohci_hcd ehci_hcd
[   12.590197] Pid: 2410, comm: modprobe Not tainted 2.6.27-rc7 #4

(snip)

Argh, that's bad.
Then I agree with increasing the components as a simple workaround.

How about the patch below? The control protocol number is increased for this change as well.

thanks,

Takashi

include/sound/asound.h +2 -3

index 3eaf155..fbfe992 100644
@@ -696,7 +696,7 @@ struct snd_timer_tread {
  *                                                                          *
  ****************************************************************************/
 
-#define SNDRV_CTL_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 5)
+#define SNDRV_CTL_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 6)
 
 struct snd_ctl_card_info {
 	int card;			/* card number */

@@ -707,8 +707,7 @@ struct snd_ctl_card_info {
 	unsigned char longname[80];	/* name + info text about soundcard */
 	unsigned char reserved_[16];	/* reserved for future (was ID of mixer) */
 	unsigned char mixername[80];	/* visual mixer identification */
-	unsigned char components[80];	/* card components / fine identification, delimited with one space (AC97 etc..) */
-	unsigned char reserved[48];	/* reserved for future */
+	unsigned char components[128];	/* card components / fine identification, delimited with one space (AC97 etc..) */
 };
 
 typedef int __bitwise snd_ctl_elem_type_t;

include/sound/core.h +1 -1

index 558b962..221554e 100644
@@ -114,7 +114,7 @@ struct snd_card {
 	char shortname[32];		/* short name of this soundcard */
 	char longname[80];		/* name of this soundcard */
 	char mixername[80];		/* mixer name */
-	char components[80];		/* card components delimited with
+	char components[128];		/* card components delimited with
 								space */
 	struct module *module;		/* top-level module */

Jaroslav Kysela

maintainer Acked-by

2008-10-07 09:06 UTC #4 lore raw

Quoted 38 lines · Takashi Iwai

Then I agree with increasing the components as a simple workaround.

How about the patch below? The control protocol number is increased for this change as well.

thanks,

Takashi

include/sound/asound.h +2 -3

index 3eaf155..fbfe992 100644
@@ -696,7 +696,7 @@ struct snd_timer_tread {
  *                                                                          *
  ****************************************************************************/
 
-#define SNDRV_CTL_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 5)
+#define SNDRV_CTL_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 6)
 
 struct snd_ctl_card_info {
 	int card;			/* card number */

@@ -707,8 +707,7 @@ struct snd_ctl_card_info {
 	unsigned char longname[80];	/* name + info text about soundcard */
 	unsigned char reserved_[16];	/* reserved for future (was ID of mixer) */
 	unsigned char mixername[80];	/* visual mixer identification */
-	unsigned char components[80];	/* card components / fine identification, delimited with one space (AC97 etc..) */
-	unsigned char reserved[48];	/* reserved for future */
+	unsigned char components[128];	/* card components / fine identification, delimited with one space (AC97 etc..) */
 };
 
 typedef int __bitwise snd_ctl_elem_type_t;

include/sound/core.h +0 -0

index 558b962..221554e 100644

include/sound/core.h struct snd_card { · line 119

Quoted 7 lines

include/sound/core.h +1 -1

@@ -114,7 +114,7 @@ struct snd_card {
 	char shortname[32];		/* short name of this soundcard */
 	char longname[80];		/* name of this soundcard */
 	char mixername[80];		/* mixer name */
-	char components[80];		/* card components delimited with
+	char components[128];		/* card components delimited with
 								space */
 	struct module *module;		/* top-level module */

Acked-by Jaroslav Kysela <[email protected]>

-----
Jaroslav Kysela <[email protected]>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Takashi Iwai

2008-10-07 09:42 UTC #5 lore raw

At Tue, 7 Oct 2008 11:06:28 +0200 (CEST),

Quoted 35 lines · Jaroslav Kysela

Quoted 32 lines · Takashi Iwai

Then I agree with increasing the components as a simple workaround.

How about the patch below? The control protocol number is increased for this change as well.

thanks,

Takashi

include/sound/asound.h +2 -3

index 3eaf155..fbfe992 100644
@@ -696,7 +696,7 @@ struct snd_timer_tread {
  *                                                                          *
  ****************************************************************************/
 
-#define SNDRV_CTL_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 5)
+#define SNDRV_CTL_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 6)
 
 struct snd_ctl_card_info {
 	int card;			/* card number */

@@ -707,8 +707,7 @@ struct snd_ctl_card_info {
 	unsigned char longname[80];	/* name + info text about soundcard */
 	unsigned char reserved_[16];	/* reserved for future (was ID of mixer) */
 	unsigned char mixername[80];	/* visual mixer identification */
-	unsigned char components[80];	/* card components / fine identification, delimited with one space (AC97 etc..) */
-	unsigned char reserved[48];	/* reserved for future */
+	unsigned char components[128];	/* card components / fine identification, delimited with one space (AC97 etc..) */
 };

Quoted 15 lines

include/sound/core.h struct snd_card { · line 119

Quoted 13 lines

include/sound/asound.h +0 -0

 typedef int __bitwise snd_ctl_elem_type_t;

include/sound/core.h +1 -1

index 558b962..221554e 100644
@@ -114,7 +114,7 @@ struct snd_card {
 	char shortname[32];		/* short name of this soundcard */
 	char longname[80];		/* name of this soundcard */
 	char mixername[80];		/* mixer name */
-	char components[80];		/* card components delimited with
+	char components[128];		/* card components delimited with
 								space */
 	struct module *module;		/* top-level module */

Acked-by Jaroslav Kysela <[email protected]>

OK, applied now...
Will change alsa-lib, too.

thanks,

Takashi

Wu Fengguang

author

2008-10-07 09:14 UTC #6 lore raw

In reply to Takashi Iwai

Quoted 24 lines · Takashi Iwai

At Tue, 7 Oct 2008 16:34:29 +0800,

Quoted 21 lines

FYI, this is the debug patch and kernel message(lookout the first line) on HP 2230s:

sound/core/init.c +2 -0

index 8af467d..704d7e8 100644
@@ -706,6 +706,8 @@ int snd_component_add(struct snd_card *card, const char *component)
 			return 1;
 	}
 	if (strlen(card->components) + 1 + len + 1 > sizeof(card->components)) {
+		snd_printk(KERN_ERR "components = %s + %s\n",
+				card->components, component);
 		snd_BUG();
 		return -ENOMEM;
 	}


[   12.590150] ALSA sound/core/init.c:710: components = HDA:11d4194a,103c3037,00100400 HDA:11c11040,103c1378,00100200 + HDA:80862802,80860101,00100000
[   12.590163] ------------[ cut here ]------------
[   12.590166] WARNING: at sound/core/init.c:711 snd_component_add+0xfa/0x110 [snd]()
[   12.590168] BUG?

(snip)

Quoted 5 lines

Argh, that's bad.
Then I agree with increasing the components as a simple workaround.
How about the patch below? The control protocol number is increased for this change as well.

The modified patch looks fine, I'll test it out in the following days.

So far grep gives the following referees:

./alsa-driver/acore/ioctl32/ioctl32_new.c:      MAP_COMPAT(SNDRV_CTL_IOCTL_CARD_INFO),
./alsa-driver/acore/ioctl32/ioctl32_old.c:      MAP_COMPAT(SNDRV_CTL_IOCTL_CARD_INFO),
./alsa-kernel/sound/core/control_compat.c:      case SNDRV_CTL_IOCTL_CARD_INFO:
./alsa-kernel/sound/core/control.c:     case SNDRV_CTL_IOCTL_CARD_INFO:
./alsa-kernel/include/sound/asound.h:   SNDRV_CTL_IOCTL_CARD_INFO = _IOR('U', 0x01, struct snd_ctl_card_info),
./alsa-lib/src/control/control_shm.c:   ctrl->cmd = SNDRV_CTL_IOCTL_CARD_INFO;
./alsa-lib/src/control/control_hw.c:    if (ioctl(hw->fd, SNDRV_CTL_IOCTL_CARD_INFO, info) < 0) {

./alsa-lib/src/control/control_hw.c: SYSERR("SNDRV_CTL_IOCTL_CARD_INFO failed");
./alsa-lib/aserver/aserver.c: case SNDRV_CTL_IOCTL_CARD_INFO:
./alsa-lib/include/sound/asound.h: SNDRV_CTL_IOCTL_CARD_INFO = _IOR('U', 0x01, struct sndrv_ctl_card_info),

Thanks,
Fengguang
---

Quoted 41 lines

thanks,

Takashi

include/sound/asound.h +2 -3

index 3eaf155..fbfe992 100644
@@ -696,7 +696,7 @@ struct snd_timer_tread {
  *                                                                          *
  ****************************************************************************/
 
-#define SNDRV_CTL_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 5)
+#define SNDRV_CTL_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 6)
 
 struct snd_ctl_card_info {
 	int card;			/* card number */

@@ -707,8 +707,7 @@ struct snd_ctl_card_info {
 	unsigned char longname[80];	/* name + info text about soundcard */
 	unsigned char reserved_[16];	/* reserved for future (was ID of mixer) */
 	unsigned char mixername[80];	/* visual mixer identification */
-	unsigned char components[80];	/* card components / fine identification, delimited with one space (AC97 etc..) */
-	unsigned char reserved[48];	/* reserved for future */
+	unsigned char components[128];	/* card components / fine identification, delimited with one space (AC97 etc..) */
 };
 
 typedef int __bitwise snd_ctl_elem_type_t;

include/sound/core.h +1 -1

index 558b962..221554e 100644
@@ -114,7 +114,7 @@ struct snd_card {
 	char shortname[32];		/* short name of this soundcard */
 	char longname[80];		/* name of this soundcard */
 	char mixername[80];		/* mixer name */
-	char components[80];		/* card components delimited with
+	char components[128];		/* card components delimited with
 								space */
 	struct module *module;		/* top-level module */

Takashi Iwai

2008-10-07 09:48 UTC #7 lore raw

At Tue, 7 Oct 2008 17:11:18 +0800,

Quoted 43 lines · Wu Fengguang

Quoted 24 lines · Takashi Iwai

At Tue, 7 Oct 2008 16:34:29 +0800,

Quoted 21 lines

FYI, this is the debug patch and kernel message(lookout the first line) on HP 2230s:

sound/core/init.c +2 -0

index 8af467d..704d7e8 100644
@@ -706,6 +706,8 @@ int snd_component_add(struct snd_card *card, const char *component)
 			return 1;
 	}
 	if (strlen(card->components) + 1 + len + 1 > sizeof(card->components)) {
+		snd_printk(KERN_ERR "components = %s + %s\n",
+				card->components, component);
 		snd_BUG();
 		return -ENOMEM;
 	}


[   12.590150] ALSA sound/core/init.c:710: components = HDA:11d4194a,103c3037,00100400 HDA:11c11040,103c1378,00100200 + HDA:80862802,80860101,00100000
[   12.590163] ------------[ cut here ]------------
[   12.590166] WARNING: at sound/core/init.c:711 snd_component_add+0xfa/0x110 [snd]()
[   12.590168] BUG?

(snip)

Quoted 5 lines

Argh, that's bad.
Then I agree with increasing the components as a simple workaround.
How about the patch below? The control protocol number is increased for this change as well.

The modified patch looks fine, I'll test it out in the following days.

So far grep gives the following referees:

./alsa-driver/acore/ioctl32/ioctl32_new.c:      MAP_COMPAT(SNDRV_CTL_IOCTL_CARD_INFO),
./alsa-driver/acore/ioctl32/ioctl32_old.c:      MAP_COMPAT(SNDRV_CTL_IOCTL_CARD_INFO),
./alsa-kernel/sound/core/control_compat.c:      case SNDRV_CTL_IOCTL_CARD_INFO:
./alsa-kernel/sound/core/control.c:     case SNDRV_CTL_IOCTL_CARD_INFO:
./alsa-kernel/include/sound/asound.h:   SNDRV_CTL_IOCTL_CARD_INFO = _IOR('U', 0x01, struct snd_ctl_card_info),
./alsa-lib/src/control/control_shm.c:   ctrl->cmd = SNDRV_CTL_IOCTL_CARD_INFO;
./alsa-lib/src/control/control_hw.c:    if (ioctl(hw->fd, SNDRV_CTL_IOCTL_CARD_INFO, info) < 0) {

Quoted 3 lines

./alsa-lib/src/control/control_hw.c: SYSERR("SNDRV_CTL_IOCTL_CARD_INFO failed");
./alsa-lib/aserver/aserver.c: case SNDRV_CTL_IOCTL_CARD_INFO:
./alsa-lib/include/sound/asound.h: SNDRV_CTL_IOCTL_CARD_INFO = _IOR('U', 0x01, struct sndrv_ctl_card_info),

Thanks. These are all just referring to the struct, so it must be OK. We need a similar fix for alsa-lib. I'll work on it now.

Takashi