Need help with patch for #2724 (usbaudio, core)

Need help with patch for #2724 (usbaudio, core)

[Gregor Jasny]

[-- Attachment #1: Type: text/plain, Size: 1082 bytes --]

Hi,

I've tried to write a patch for bug #2724. The problem is that some
Logitech webcams have interface alternatives with a rate of 0:

  Interface 3
    Altset 1
    Format: 0x2
    Channels: 1
    Endpoint: 6 IN (ASYNC)
    Rates: 0
  Interface 3
    Altset 2
    Format: 0x2
    Channels: 1
    Endpoint: 6 IN (ASYNC)
    Rates: 0
  Interface 3
    Altset 3
    Format: 0x2
    Channels: 1
    Endpoint: 6 IN (ASYNC)
    Rates: 16000

The ALSA Kernel driver seems to be somewhat unhappy about this and
oopses in snd_interval_list which gets called with:

i: dc4f0928 count: 3635438588 list: ffffffff mask 4294967295

I've tried to track down the origin of -1 in constraint_list->list but got
lost in the depths of the hw param stuff.

The attached patch skips all alternatives with no valid rates. With this
patch my Quickcam 5000 works like a charm. But I'm unsure if this is the
right approach to fix this bug.

I think the right way to go is to fix also the cause of the oops, so
that the ALSA core doesn't oops when no valid rate is given.

Thanks for your comments,
Gregor

[-- Attachment #2: logitech.diff --]
[-- Type: text/plain, Size: 1255 bytes --]

diff --git a/sound/usb/usbaudio.c b/sound/usb/usbaudio.c
index 19bdcc7..661e557 100644
--- a/sound/usb/usbaudio.c
+++ b/sound/usb/usbaudio.c
@@ -2455,7 +2455,7 @@ static int parse_audio_format_rates(struct snd_usb_audio *chip, struct audioform
 		/*
 		 * build the rate table and bitmap flags
 		 */
-		int r, idx, c;
+		int r, idx, c, nonzero_rates = 0;
 		/* this table corresponds to the SNDRV_PCM_RATE_XXX bit */
 		static unsigned int conv_rates[] = {
 			5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000,
@@ -2478,6 +2478,7 @@ static int parse_audio_format_rates(struct snd_usb_audio *chip, struct audioform
 			    fp->altsetting == 5 && fp->maxpacksize == 392)
 				rate = 96000;
 			fp->rate_table[r] = rate;
+                        nonzero_rates |= rate;
 			if (rate < fp->rate_min)
 				fp->rate_min = rate;
 			else if (rate > fp->rate_max)
@@ -2493,6 +2494,10 @@ static int parse_audio_format_rates(struct snd_usb_audio *chip, struct audioform
 			if (!found)
 				fp->needs_knot = 1;
 		}
+                if (!nonzero_rates) {
+                        hwc_debug("No rate was nonzero. Skipping format!\n");
+                        return -1;
+                }
 		if (fp->needs_knot)
 			fp->rates |= SNDRV_PCM_RATE_KNOT;
 	} else {

[-- Attachment #3: Type: text/plain, Size: 347 bytes --]

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[-- Attachment #4: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/alsa-devel

Re: Need help with patch for #2724 (usbaudio, core)

[Takashi Iwai]

At Sat, 27 Jan 2007 14:24:45 +0100,
Gregor Jasny wrote:
> 
> Hi,
> 
> I've tried to write a patch for bug #2724. The problem is that some
> Logitech webcams have interface alternatives with a rate of 0:
(snip)
> I've tried to track down the origin of -1 in constraint_list->list but got
> lost in the depths of the hw param stuff.

Good catch.  How about the patch below?  Can it at least avoid Oops
with rates == 0 (even without your patch)?

> The attached patch skips all alternatives with no valid rates. With this
> patch my Quickcam 5000 works like a charm. But I'm unsure if this is the
> right approach to fix this bug.

This is the right fix, too.  Of course, pcm-core should be more
robust, but the cause of the bug is the bogus entry.

The patch looks fine to me.  Please provide a proper changelog and a
sign-off for merging.


Thanks,

Takashi


diff -r 3be2f03501ef core/pcm_native.c
--- a/core/pcm_native.c	Tue Jan 30 17:30:55 2007 +0100
+++ b/core/pcm_native.c	Tue Jan 30 17:47:49 2007 +0100
@@ -1798,6 +1798,8 @@ static int snd_pcm_hw_rule_rate(struct s
 				struct snd_pcm_hw_rule *rule)
 {
 	struct snd_pcm_hardware *hw = rule->private;
+	if (!hw->rates)
+		return -EINVAL;
 	return snd_interval_list(hw_param_interval(params, rule->var),
 				 ARRAY_SIZE(rates), rates, hw->rates);
 }		
@@ -1959,52 +1961,83 @@ int snd_pcm_hw_constraints_complete(stru
 			mask |= 1 << SNDRV_PCM_ACCESS_MMAP_COMPLEX;
 	}
 	err = snd_pcm_hw_constraint_mask(runtime, SNDRV_PCM_HW_PARAM_ACCESS, mask);
-	snd_assert(err >= 0, return -EINVAL);
+	if (err < 0) {
+		snd_printd("cannot set access mask\n");
+		return err;
+	}
 
 	err = snd_pcm_hw_constraint_mask64(runtime, SNDRV_PCM_HW_PARAM_FORMAT, hw->formats);
-	snd_assert(err >= 0, return -EINVAL);
+	if (err < 0) {
+		snd_printd("cannot set format mask\n");
+		return err;
+	}
 
 	err = snd_pcm_hw_constraint_mask(runtime, SNDRV_PCM_HW_PARAM_SUBFORMAT, 1 << SNDRV_PCM_SUBFORMAT_STD);
-	snd_assert(err >= 0, return -EINVAL);
+	if (err < 0) {
+		snd_printd("cannot set subformat mask\n");
+		return err;
+	}
 
 	err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_CHANNELS,
 					   hw->channels_min, hw->channels_max);
-	snd_assert(err >= 0, return -EINVAL);
+	if (err < 0) {
+		snd_printd("cannot set channels minmax\n");
+		return err;
+	}
 
 	err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_RATE,
 					   hw->rate_min, hw->rate_max);
-	snd_assert(err >= 0, return -EINVAL);
+	if (err < 0) {
+		snd_printd("cannot set rate minmax\n");
+		return err;
+	}
 
 	err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
 					   hw->period_bytes_min, hw->period_bytes_max);
-	snd_assert(err >= 0, return -EINVAL);
+	if (err < 0) {
+		snd_printd("cannot set periods_bytes minmax\n");
+		return err;
+	}
 
 	err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_PERIODS,
 					   hw->periods_min, hw->periods_max);
-	snd_assert(err >= 0, return -EINVAL);
+	if (err < 0) {
+		snd_printd("cannot set periods minmax\n");
+		return err;
+	}
 
 	err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_BUFFER_BYTES,
 					   hw->period_bytes_min, hw->buffer_bytes_max);
-	snd_assert(err >= 0, return -EINVAL);
+	if (err < 0) {
+		snd_printd("cannot set buffer_bytes minmax\n");
+		return err;
+	}
 
 	err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 
 				  snd_pcm_hw_rule_buffer_bytes_max, substream,
 				  SNDRV_PCM_HW_PARAM_BUFFER_BYTES, -1);
-	if (err < 0)
+	if (err < 0) {
+		snd_printd("cannot set buffer_bytes limit\n");
 		return err;
+	}
 
 	/* FIXME: remove */
 	if (runtime->dma_bytes) {
 		err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 0, runtime->dma_bytes);
-		snd_assert(err >= 0, return -EINVAL);
+		if (err < 0) {
+			snd_printd("cannot set dma_bytes limit\n");
+			return err;
+		}
 	}
 
 	if (!(hw->rates & (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS))) {
 		err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE, 
 					  snd_pcm_hw_rule_rate, hw,
 					  SNDRV_PCM_HW_PARAM_RATE, -1);
-		if (err < 0)
+		if (err < 0) {
+			snd_printd("cannot set rate mask\n");
 			return err;
+		}
 	}
 
 	/* FIXME: this belong to lowlevel */

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: Need help with patch for #2724 (usbaudio, core)

[Gregor Jasny]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

Takashi Iwai wrote:
> This is the right fix, too.  Of course, pcm-core should be more
> robust, but the cause of the bug is the bogus entry.

Your patch does not change anything. I've enabled 
CONFIG_SND_VERBOSE_PRINTK, CONFIG_SND_DEBUG and CONFIG_SND_DEBUG_DETECT 
in the kernel config.

None of your snd_printd output gets triggered. I've placed a test output 
in pcm-native, so debugging works.

Note that I've applied your patch to 2.6.19.2.

> The patch looks fine to me.  Please provide a proper changelog and a
> sign-off for merging.

Done. The patch is against the current hg snapshot of alsa-kernel.

Gregor

[-- Attachment #2: prevent-zero-rates.diff --]
[-- Type: text/plain, Size: 1239 bytes --]

From: Gregor Jasny <gjasny@web.de>

This is a patch for ALSA Bug #2724. Some webcams provide bogus
settings with no valid rates. With this patch those are skipped.

Signed-off-by: Gregor Jasny <gjasny@web.de>
diff -r b4265ee02e26 usb/usbaudio.c
--- a/usb/usbaudio.c	Wed Jan 31 10:35:19 2007 +0100
+++ b/usb/usbaudio.c	Wed Jan 31 12:20:29 2007 +0100
@@ -2463,6 +2463,7 @@ static int parse_audio_format_rates(stru
 		 * build the rate table and bitmap flags
 		 */
 		int r, idx, c;
+		unsigned int nonzero_rates = 0;
 		/* this table corresponds to the SNDRV_PCM_RATE_XXX bit */
 		static unsigned int conv_rates[] = {
 			5512, 8000, 11025, 16000, 22050, 32000, 44100, 48000,
@@ -2485,6 +2486,7 @@ static int parse_audio_format_rates(stru
 			    fp->altsetting == 5 && fp->maxpacksize == 392)
 				rate = 96000;
 			fp->rate_table[r] = rate;
+			nonzero_rates |= rate;
 			if (rate < fp->rate_min)
 				fp->rate_min = rate;
 			else if (rate > fp->rate_max)
@@ -2499,6 +2501,10 @@ static int parse_audio_format_rates(stru
 			}
 			if (!found)
 				fp->needs_knot = 1;
+		}
+		if (!nonzero_rates) {
+			hwc_debug("All rates were zero. Skipping format!\n");
+			return -1;
 		}
 		if (fp->needs_knot)
 			fp->rates |= SNDRV_PCM_RATE_KNOT;

[-- Attachment #3: Type: text/plain, Size: 347 bytes --]

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

[-- Attachment #4: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/alsa-devel

Re: Need help with patch for #2724 (usbaudio, core)

[Takashi Iwai]

At Wed, 31 Jan 2007 12:22:19 +0100,
Gregor Jasny wrote:
> 
> Takashi Iwai wrote:
> > This is the right fix, too.  Of course, pcm-core should be more
> > robust, but the cause of the bug is the bogus entry.
> 
> Your patch does not change anything. I've enabled 
> CONFIG_SND_VERBOSE_PRINTK, CONFIG_SND_DEBUG and CONFIG_SND_DEBUG_DETECT 
> in the kernel config.

Interesting.  So, you still got an Oops, right?

> None of your snd_printd output gets triggered. I've placed a test output 
> in pcm-native, so debugging works.

OK, then how about the one below?

> > The patch looks fine to me.  Please provide a proper changelog and a
> > sign-off for merging.
> 
> Done. The patch is against the current hg snapshot of alsa-kernel.

Thanks.  I applied it to HG tree now.


Takashi

diff -r 776cac567090 core/pcm_native.c
--- a/core/pcm_native.c	Wed Jan 31 12:27:39 2007 +0100
+++ b/core/pcm_native.c	Wed Jan 31 12:31:37 2007 +0100
@@ -1959,52 +1959,87 @@ int snd_pcm_hw_constraints_complete(stru
 			mask |= 1 << SNDRV_PCM_ACCESS_MMAP_COMPLEX;
 	}
 	err = snd_pcm_hw_constraint_mask(runtime, SNDRV_PCM_HW_PARAM_ACCESS, mask);
-	snd_assert(err >= 0, return -EINVAL);
+	if (err < 0) {
+		snd_printd("cannot set access mask\n");
+		return err;
+	}
 
 	err = snd_pcm_hw_constraint_mask64(runtime, SNDRV_PCM_HW_PARAM_FORMAT, hw->formats);
-	snd_assert(err >= 0, return -EINVAL);
+	if (err < 0) {
+		snd_printd("cannot set format mask\n");
+		return err;
+	}
 
 	err = snd_pcm_hw_constraint_mask(runtime, SNDRV_PCM_HW_PARAM_SUBFORMAT, 1 << SNDRV_PCM_SUBFORMAT_STD);
-	snd_assert(err >= 0, return -EINVAL);
+	if (err < 0) {
+		snd_printd("cannot set subformat mask\n");
+		return err;
+	}
 
 	err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_CHANNELS,
 					   hw->channels_min, hw->channels_max);
-	snd_assert(err >= 0, return -EINVAL);
+	if (err < 0) {
+		snd_printd("cannot set channels minmax\n");
+		return err;
+	}
 
 	err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_RATE,
 					   hw->rate_min, hw->rate_max);
-	snd_assert(err >= 0, return -EINVAL);
+	if (err < 0) {
+		snd_printd("cannot set rate minmax\n");
+		return err;
+	}
+	if (!hw->rates) {
+		snd_printd("invalid hw->rates\n");
+		return -EINVAL;
+	}
 
 	err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_PERIOD_BYTES,
 					   hw->period_bytes_min, hw->period_bytes_max);
-	snd_assert(err >= 0, return -EINVAL);
+	if (err < 0) {
+		snd_printd("cannot set periods_bytes minmax\n");
+		return err;
+	}
 
 	err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_PERIODS,
 					   hw->periods_min, hw->periods_max);
-	snd_assert(err >= 0, return -EINVAL);
+	if (err < 0) {
+		snd_printd("cannot set periods minmax\n");
+		return err;
+	}
 
 	err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_BUFFER_BYTES,
 					   hw->period_bytes_min, hw->buffer_bytes_max);
-	snd_assert(err >= 0, return -EINVAL);
+	if (err < 0) {
+		snd_printd("cannot set buffer_bytes minmax\n");
+		return err;
+	}
 
 	err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 
 				  snd_pcm_hw_rule_buffer_bytes_max, substream,
 				  SNDRV_PCM_HW_PARAM_BUFFER_BYTES, -1);
-	if (err < 0)
+	if (err < 0) {
+		snd_printd("cannot set buffer_bytes limit\n");
 		return err;
+	}
 
 	/* FIXME: remove */
 	if (runtime->dma_bytes) {
 		err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 0, runtime->dma_bytes);
-		snd_assert(err >= 0, return -EINVAL);
+		if (err < 0) {
+			snd_printd("cannot set dma_bytes limit\n");
+			return err;
+		}
 	}
 
 	if (!(hw->rates & (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS))) {
 		err = snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE, 
 					  snd_pcm_hw_rule_rate, hw,
 					  SNDRV_PCM_HW_PARAM_RATE, -1);
-		if (err < 0)
+		if (err < 0) {
+			snd_printd("cannot set rate mask\n");
 			return err;
+		}
 	}
 
 	/* FIXME: this belong to lowlevel */

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: Need help with patch for #2724 (usbaudio, core)

[Gregor Jasny]

Takashi Iwai wrote:
> OK, then how about the one below?

Still oopsing.

ALSA sound/core/pcm_native.c:2067: SUCCESS -> Leaves 
snd_pcm_hw_constraints_complete with no error.

After that I get the following debug output:

hw_rule_rate: (0,16000)
   --> (0, 16000) (changed = 0)
hw_rule_channels: (1,1)
   --> (1, 1) (changed = 0)
hw_rule_format: 4:0
   --> 4:0 (changed = 0)

general protection fault: 0000 [#8]
[OOPS]

Please attach patches for me as seperate file. Need to hand-apply it 
otherwise.

Thanks,
Gregor

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

Re: Need help with patch for #2724 (usbaudio, core)

[Takashi Iwai]

At Wed, 31 Jan 2007 14:14:35 +0100,
Gregor Jasny wrote:
> 
> Takashi Iwai wrote:
> > OK, then how about the one below?
> 
> Still oopsing.
> 
> ALSA sound/core/pcm_native.c:2067: SUCCESS -> Leaves 
> snd_pcm_hw_constraints_complete with no error.
> 
> After that I get the following debug output:
> 
> hw_rule_rate: (0,16000)
>    --> (0, 16000) (changed = 0)
> hw_rule_channels: (1,1)
>    --> (1, 1) (changed = 0)
> hw_rule_format: 4:0
>    --> 4:0 (changed = 0)
> 
> general protection fault: 0000 [#8]
> [OOPS]

Which position does it happen exactly?
Could you show more detail of the Oops entry?


> Please attach patches for me as seperate file. Need to hand-apply it 
> otherwise.

OK for me, but why?  Can't you get a text from your mail folder?
Just from curiosity...


Takashi

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV