usb-audio: some UAC2 fixes

usb-audio: some UAC2 fixes

[Daniel Mack]

Hi,

here are four small patches that fix some minor bugs in the UAC2
implementation. Thanks to Alex Lee for reporting them.

Don't know if it's possible to get them into 2.6.35 as they're no
regressions in respect to 2.6.34 (I followed the recent discussions on
LKML). However, they fix real bugs and I would also label them
stable@kernel.org if 2.6.35 would have already been released.
So .. I don't know :)

I have a number of more patches that do cleanups and implement more
functions, but they can certainly go into 2.6.36 and they're not
finished yet, so I'm cooking them for some more time.

Thanks,
Daniel

[PATCH 1/4] ALSA: usb-audio: add check for faulty clock in parse_audio_format_rates_v2()
[PATCH 2/4] ALSA: usb-audio: fix control messages for USB_RECIP_INTERFACE
[PATCH 3/4] ALSA: usb-audio: parse UAC2 sample rate ranges correctly
[PATCH 4/4] ALSA: usb-audio: fix UAC2 control value queries

[PATCH 1/4] ALSA: usb-audio: add check for faulty clock in parse_audio_format_rates_v2()

[Daniel Mack]

Signed-off-by: Daniel Mack <daniel@caiaq.de>
---
 sound/usb/format.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/sound/usb/format.c b/sound/usb/format.c
index 5367cd1..df5b29f 100644
--- a/sound/usb/format.c
+++ b/sound/usb/format.c
@@ -218,6 +218,12 @@ static int parse_audio_format_rates_v2(struct snd_usb_audio *chip,
 	int i, nr_rates, data_size, ret = 0;
 	int clock = snd_usb_clock_find_source(chip, chip->ctrl_intf, fp->clock);
 
+	if (clock < 0) {
+		snd_printk(KERN_ERR "%s(): unable to find clock source (clock %d)\n",
+				__func__, clock);
+		goto err;
+	}
+
 	/* get the number of sample rates first by only fetching 2 bytes */
 	ret = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_RANGE,
 			      USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,
-- 
1.7.1

[PATCH 2/4] ALSA: usb-audio: fix control messages for USB_RECIP_INTERFACE

[Daniel Mack]

Control messages directed to an interface must have the interface number
set in the lower 8 bits of wIndex. This wasn't done correctly for some
clock and mixer messages.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Reported-by: Alex Lee <alexlee188@gmail.com>
---
 sound/usb/clock.c  |   12 ++++++++----
 sound/usb/format.c |    6 ++++--
 sound/usb/helper.h |    4 ++++
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/sound/usb/clock.c b/sound/usb/clock.c
index b7aadd6..b585511 100644
--- a/sound/usb/clock.c
+++ b/sound/usb/clock.c
@@ -103,7 +103,8 @@ static int uac_clock_selector_get_val(struct snd_usb_audio *chip, int selector_i
 	ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0),
 			      UAC2_CS_CUR,
 			      USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
-			      UAC2_CX_CLOCK_SELECTOR << 8, selector_id << 8,
+			      UAC2_CX_CLOCK_SELECTOR << 8,
+			      snd_usb_ctrl_intf(chip) | (selector_id << 8),
 			      &buf, sizeof(buf), 1000);
 
 	if (ret < 0)
@@ -120,7 +121,8 @@ static bool uac_clock_source_is_valid(struct snd_usb_audio *chip, int source_id)
 
 	err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_CUR,
 			      USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,
-			      UAC2_CS_CONTROL_CLOCK_VALID << 8, source_id << 8,
+			      UAC2_CS_CONTROL_CLOCK_VALID << 8,
+			      snd_usb_ctrl_intf(chip) | (source_id << 8),
 			      &data, sizeof(data), 1000);
 
 	if (err < 0) {
@@ -269,7 +271,8 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface,
 	data[3] = rate >> 24;
 	if ((err = snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0), UAC2_CS_CUR,
 				   USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_OUT,
-				   UAC2_CS_CONTROL_SAM_FREQ << 8, clock << 8,
+				   UAC2_CS_CONTROL_SAM_FREQ << 8,
+				   snd_usb_ctrl_intf(chip) | (clock << 8),
 				   data, sizeof(data), 1000)) < 0) {
 		snd_printk(KERN_ERR "%d:%d:%d: cannot set freq %d (v2)\n",
 			   dev->devnum, iface, fmt->altsetting, rate);
@@ -278,7 +281,8 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface,
 
 	if ((err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_CUR,
 				   USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,
-				   UAC2_CS_CONTROL_SAM_FREQ << 8, clock << 8,
+				   UAC2_CS_CONTROL_SAM_FREQ << 8,
+				   snd_usb_ctrl_intf(chip) | (clock << 8),
 				   data, sizeof(data), 1000)) < 0) {
 		snd_printk(KERN_WARNING "%d:%d:%d: cannot get freq (v2)\n",
 			   dev->devnum, iface, fmt->altsetting);
diff --git a/sound/usb/format.c b/sound/usb/format.c
index df5b29f..8eccf17 100644
--- a/sound/usb/format.c
+++ b/sound/usb/format.c
@@ -227,7 +227,8 @@ static int parse_audio_format_rates_v2(struct snd_usb_audio *chip,
 	/* get the number of sample rates first by only fetching 2 bytes */
 	ret = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_RANGE,
 			      USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,
-			      UAC2_CS_CONTROL_SAM_FREQ << 8, clock << 8,
+			      UAC2_CS_CONTROL_SAM_FREQ << 8,
+			      snd_usb_ctrl_intf(chip) | (clock << 8),
 			      tmp, sizeof(tmp), 1000);
 
 	if (ret < 0) {
@@ -247,7 +248,8 @@ static int parse_audio_format_rates_v2(struct snd_usb_audio *chip,
 	/* now get the full information */
 	ret = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_RANGE,
 			      USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,
-			      UAC2_CS_CONTROL_SAM_FREQ << 8, clock << 8,
+			      UAC2_CS_CONTROL_SAM_FREQ << 8,
+			      snd_usb_ctrl_intf(chip) | (clock << 8),
 			      data, data_size, 1000);
 
 	if (ret < 0) {
diff --git a/sound/usb/helper.h b/sound/usb/helper.h
index a6b0e51..09bd943 100644
--- a/sound/usb/helper.h
+++ b/sound/usb/helper.h
@@ -28,5 +28,9 @@ unsigned char snd_usb_parse_datainterval(struct snd_usb_audio *chip,
 #define snd_usb_get_speed(dev) ((dev)->speed)
 #endif
 
+static inline int snd_usb_ctrl_intf(struct snd_usb_audio *chip)
+{
+	return get_iface_desc(chip->ctrl_intf)->bInterfaceNumber;
+}
 
 #endif /* __USBAUDIO_HELPER_H */
-- 
1.7.1

[PATCH 3/4] ALSA: usb-audio: parse UAC2 sample rate ranges correctly

[Daniel Mack]

A device may report its supported sample rates in ranges rather than in
discrete triplets. The code used to only parse the MIN field instead of
properly paying attention to the MAX and RES values.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Reported-by: Alex Lee <alexlee188@gmail.com>
---
 sound/usb/format.c |   80 ++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 62 insertions(+), 18 deletions(-)

diff --git a/sound/usb/format.c b/sound/usb/format.c
index 8eccf17..e946db7 100644
--- a/sound/usb/format.c
+++ b/sound/usb/format.c
@@ -206,6 +206,47 @@ static int parse_audio_format_rates_v1(struct snd_usb_audio *chip, struct audiof
 }
 
 /*
+ * Helper function to walk the array of sample rate triplets reported by
+ * the device. The problem is that we need to parse whole array first to
+ * get to know how many sample rates we have to expect.
+ * Then fp->rate_table can be allocated and filled.
+ */
+static int parse_uac2_sample_rate_range(struct audioformat *fp, int nr_triplets,
+					const unsigned char *data)
+{
+	int i, nr_rates = 0;
+
+	fp->rates = fp->rate_min = fp->rate_max = 0;
+
+	for (i = 0; i < nr_triplets; i++) {
+		int min = combine_quad(&data[2 + 12 * i]);
+		int max = combine_quad(&data[6 + 12 * i]);
+		int res = combine_quad(&data[10 + 12 * i]);
+		int rate;
+
+		if ((max < 0) || (min < 0) || (res < 0) || (max < min))
+			continue;
+
+		for (rate = min; rate <= max; rate += res) {
+			if (fp->rate_table)
+				fp->rate_table[nr_rates] = rate;
+			if (!fp->rate_min || rate < fp->rate_min)
+				fp->rate_min = rate;
+			if (!fp->rate_max || rate > fp->rate_max)
+				fp->rate_max = rate;
+			fp->rates |= snd_pcm_rate_to_rate_bit(rate);
+			nr_rates++;
+
+			/* avoid endless loop */
+			if (res == 0)
+				break;
+		}
+	}
+
+	return nr_rates;
+}
+
+/*
  * parse the format descriptor and stores the possible sample rates
  * on the audioformat table (audio class v2).
  */
@@ -215,7 +256,7 @@ static int parse_audio_format_rates_v2(struct snd_usb_audio *chip,
 {
 	struct usb_device *dev = chip->dev;
 	unsigned char tmp[2], *data;
-	int i, nr_rates, data_size, ret = 0;
+	int nr_triplets, data_size, ret = 0;
 	int clock = snd_usb_clock_find_source(chip, chip->ctrl_intf, fp->clock);
 
 	if (clock < 0) {
@@ -237,8 +278,8 @@ static int parse_audio_format_rates_v2(struct snd_usb_audio *chip,
 		goto err;
 	}
 
-	nr_rates = (tmp[1] << 8) | tmp[0];
-	data_size = 2 + 12 * nr_rates;
+	nr_triplets = (tmp[1] << 8) | tmp[0];
+	data_size = 2 + 12 * nr_triplets;
 	data = kzalloc(data_size, GFP_KERNEL);
 	if (!data) {
 		ret = -ENOMEM;
@@ -259,26 +300,29 @@ static int parse_audio_format_rates_v2(struct snd_usb_audio *chip,
 		goto err_free;
 	}
 
-	fp->rate_table = kmalloc(sizeof(int) * nr_rates, GFP_KERNEL);
+	/* Call the triplet parser, and make sure fp->rate_table is NULL.
+	 * We just use the return value to know how many sample rates we
+	 * will have to deal with. */
+	kfree(fp->rate_table);
+	fp->rate_table = NULL;
+	fp->nr_rates = parse_uac2_sample_rate_range(fp, nr_triplets, data);
+
+	if (fp->nr_rates == 0) {
+		snd_printk(KERN_ERR "%s(): unable to parse sample rate triplets\n",
+				__func__);
+		ret = -EINVAL;
+		goto err_free;
+	}
+
+	fp->rate_table = kmalloc(sizeof(int) * fp->nr_rates, GFP_KERNEL);
 	if (!fp->rate_table) {
 		ret = -ENOMEM;
 		goto err_free;
 	}
 
-	fp->nr_rates = 0;
-	fp->rate_min = fp->rate_max = 0;
-
-	for (i = 0; i < nr_rates; i++) {
-		int rate = combine_quad(&data[2 + 12 * i]);
-
-		fp->rate_table[fp->nr_rates] = rate;
-		if (!fp->rate_min || rate < fp->rate_min)
-			fp->rate_min = rate;
-		if (!fp->rate_max || rate > fp->rate_max)
-			fp->rate_max = rate;
-		fp->rates |= snd_pcm_rate_to_rate_bit(rate);
-		fp->nr_rates++;
-	}
+	/* Call the triplet parser again, but this time, fp->rate_table is
+	 * allocated, so the rates will be stored */
+	parse_uac2_sample_rate_range(fp, nr_triplets, data);
 
 err_free:
 	kfree(data);
-- 
1.7.1

[PATCH 4/4] ALSA: usb-audio: fix UAC2 control value queries

[Daniel Mack]

UAC2 controls can be queried in 1-byte, 2-byte or 4-byte request blocks.
The size is actually determined by the number of bytes requested, and as
everything is little-endian, we can always go for the 4-byte variant.

Also, the MIN, MAX and RES value computations were using a wrong offset.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Reported-by: Alex Lee <alexlee188@gmail.com>
---
 sound/usb/mixer.c |   19 ++++++++++++++-----
 1 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index a060d00..6939d0f 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -297,20 +297,27 @@ static int get_ctl_value_v1(struct usb_mixer_elem_info *cval, int request, int v
 
 static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, int validx, int *value_ret)
 {
-	unsigned char buf[14]; /* enough space for one range of 4 bytes */
+	unsigned char buf[2 + 3*sizeof(__u16)]; /* enough space for one range */
 	unsigned char *val;
-	int ret;
+	int ret, size;
 	__u8 bRequest;
 
-	bRequest = (request == UAC_GET_CUR) ?
-		UAC2_CS_CUR : UAC2_CS_RANGE;
+	if (request == UAC_GET_CUR) {
+		bRequest = UAC2_CS_CUR;
+		size = sizeof(__u16);
+	} else {
+		bRequest = UAC2_CS_RANGE;
+		size = sizeof(buf);
+	}
+
+	memset(buf, 0, sizeof(buf));
 
 	ret = snd_usb_ctl_msg(cval->mixer->chip->dev,
 			      usb_rcvctrlpipe(cval->mixer->chip->dev, 0),
 			      bRequest,
 			      USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
 			      validx, cval->mixer->ctrlif | (cval->id << 8),
-			      buf, sizeof(buf), 1000);
+			      buf, size, 1000);
 
 	if (ret < 0) {
 		snd_printk(KERN_ERR "cannot get ctl value: req = %#x, wValue = %#x, wIndex = %#x, type = %d\n",
@@ -318,6 +325,8 @@ static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, int v
 		return ret;
 	}
 
+	/* FIXME: how should we handle multiple triplets here? */
+
 	switch (request) {
 	case UAC_GET_CUR:
 		val = buf;
-- 
1.7.1

Re: [PATCH 4/4] ALSA: usb-audio: fix UAC2 control value queries

[Daniel Mack]

On Fri, Jun 11, 2010 at 03:33:11PM +0200, Daniel Mack wrote:
> UAC2 controls can be queried in 1-byte, 2-byte or 4-byte request blocks.
> The size is actually determined by the number of bytes requested, and as
> everything is little-endian, we can always go for the 4-byte variant.
> 
> Also, the MIN, MAX and RES value computations were using a wrong offset.
> 
> Signed-off-by: Daniel Mack <daniel@caiaq.de>
> Reported-by: Alex Lee <alexlee188@gmail.com>

Damn, I forgot to pick the right commit message for this patch. Below is
a new version. Sorry.

Daniel

>From 9a8be646c65df2960bda3588cbd2cea4834c7750 Mon Sep 17 00:00:00 2001
From: Daniel Mack <daniel@caiaq.de>
Date: Fri, 11 Jun 2010 13:49:56 +0200
Subject: [PATCH] ALSA: usb-audio: fix UAC2 control value queries

For RANGE requests, we should only query as much bytes as we're in face
interested in.

For CUR requests, we shouldn't confuse the firmware with an overlong
request but just ask for 2 bytes.

This might need fixing in the future as it's not entirely clear when to
dispatch 1-byte, 2-byte and 4-byte request blocks. For now, we assume
everything is coded in 16bit - this works for all firmware
implementations I've seen.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Reported-by: Alex Lee <alexlee188@gmail.com>
---
 sound/usb/mixer.c |   19 ++++++++++++++-----
 1 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index a060d00..6939d0f 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -297,20 +297,27 @@ static int get_ctl_value_v1(struct usb_mixer_elem_info *cval, int request, int v
 
 static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, int validx, int *value_ret)
 {
-	unsigned char buf[14]; /* enough space for one range of 4 bytes */
+	unsigned char buf[2 + 3*sizeof(__u16)]; /* enough space for one range */
 	unsigned char *val;
-	int ret;
+	int ret, size;
 	__u8 bRequest;
 
-	bRequest = (request == UAC_GET_CUR) ?
-		UAC2_CS_CUR : UAC2_CS_RANGE;
+	if (request == UAC_GET_CUR) {
+		bRequest = UAC2_CS_CUR;
+		size = sizeof(__u16);
+	} else {
+		bRequest = UAC2_CS_RANGE;
+		size = sizeof(buf);
+	}
+
+	memset(buf, 0, sizeof(buf));
 
 	ret = snd_usb_ctl_msg(cval->mixer->chip->dev,
 			      usb_rcvctrlpipe(cval->mixer->chip->dev, 0),
 			      bRequest,
 			      USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
 			      validx, cval->mixer->ctrlif | (cval->id << 8),
-			      buf, sizeof(buf), 1000);
+			      buf, size, 1000);
 
 	if (ret < 0) {
 		snd_printk(KERN_ERR "cannot get ctl value: req = %#x, wValue = %#x, wIndex = %#x, type = %d\n",
@@ -318,6 +325,8 @@ static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, int v
 		return ret;
 	}
 
+	/* FIXME: how should we handle multiple triplets here? */
+
 	switch (request) {
 	case UAC_GET_CUR:
 		val = buf;
-- 
1.7.1

Re: [PATCH 3/4] ALSA: usb-audio: parse UAC2 sample rate ranges correctly

[Daniel Mack]

On Fri, Jun 11, 2010 at 10:59:44PM +0800, Alex Lee wrote:
> If the device has a variable clock, it may report a RES of only 1.  So you
> may end up with thousands (and even millions) of sample rates.  It might be
> better to have separate code to cater to discrete clocks vs variable clocks.

AFAIK, ALSA cannot cope with that yet. Am I right?

Daniel



> On Fri, Jun 11, 2010 at 9:33 PM, Daniel Mack <daniel@caiaq.de> wrote:
> 
> > A device may report its supported sample rates in ranges rather than in
> > discrete triplets. The code used to only parse the MIN field instead of
> > properly paying attention to the MAX and RES values.
> >
> > Signed-off-by: Daniel Mack <daniel@caiaq.de>
> > Reported-by: Alex Lee <alexlee188@gmail.com>
> > ---
> >  sound/usb/format.c |   80
> > ++++++++++++++++++++++++++++++++++++++++-----------
> >  1 files changed, 62 insertions(+), 18 deletions(-)
> >
> > diff --git a/sound/usb/format.c b/sound/usb/format.c
> > index 8eccf17..e946db7 100644
> > --- a/sound/usb/format.c
> > +++ b/sound/usb/format.c
> > @@ -206,6 +206,47 @@ static int parse_audio_format_rates_v1(struct
> > snd_usb_audio *chip, struct audiof
> >  }
> >
> >  /*
> > + * Helper function to walk the array of sample rate triplets reported by
> > + * the device. The problem is that we need to parse whole array first to
> > + * get to know how many sample rates we have to expect.
> > + * Then fp->rate_table can be allocated and filled.
> > + */
> > +static int parse_uac2_sample_rate_range(struct audioformat *fp, int
> > nr_triplets,
> > +                                       const unsigned char *data)
> > +{
> > +       int i, nr_rates = 0;
> > +
> > +       fp->rates = fp->rate_min = fp->rate_max = 0;
> > +
> > +       for (i = 0; i < nr_triplets; i++) {
> > +               int min = combine_quad(&data[2 + 12 * i]);
> > +               int max = combine_quad(&data[6 + 12 * i]);
> > +               int res = combine_quad(&data[10 + 12 * i]);
> > +               int rate;
> > +
> > +               if ((max < 0) || (min < 0) || (res < 0) || (max < min))
> > +                       continue;
> > +
> > +               for (rate = min; rate <= max; rate += res) {
> > +                       if (fp->rate_table)
> > +                               fp->rate_table[nr_rates] = rate;
> > +                       if (!fp->rate_min || rate < fp->rate_min)
> > +                               fp->rate_min = rate;
> > +                       if (!fp->rate_max || rate > fp->rate_max)
> > +                               fp->rate_max = rate;
> > +                       fp->rates |= snd_pcm_rate_to_rate_bit(rate);
> > +                       nr_rates++;
> > +
> > +                       /* avoid endless loop */
> > +                       if (res == 0)
> > +                               break;
> > +               }
> > +       }
> > +
> > +       return nr_rates;
> > +}
> > +
> > +/*
> >  * parse the format descriptor and stores the possible sample rates
> >  * on the audioformat table (audio class v2).
> >  */
> > @@ -215,7 +256,7 @@ static int parse_audio_format_rates_v2(struct
> > snd_usb_audio *chip,
> >  {
> >        struct usb_device *dev = chip->dev;
> >        unsigned char tmp[2], *data;
> > -       int i, nr_rates, data_size, ret = 0;
> > +       int nr_triplets, data_size, ret = 0;
> >        int clock = snd_usb_clock_find_source(chip, chip->ctrl_intf,
> > fp->clock);
> >
> >        if (clock < 0) {
> > @@ -237,8 +278,8 @@ static int parse_audio_format_rates_v2(struct
> > snd_usb_audio *chip,
> >                goto err;
> >        }
> >
> > -       nr_rates = (tmp[1] << 8) | tmp[0];
> > -       data_size = 2 + 12 * nr_rates;
> > +       nr_triplets = (tmp[1] << 8) | tmp[0];
> > +       data_size = 2 + 12 * nr_triplets;
> >        data = kzalloc(data_size, GFP_KERNEL);
> >        if (!data) {
> >                ret = -ENOMEM;
> > @@ -259,26 +300,29 @@ static int parse_audio_format_rates_v2(struct
> > snd_usb_audio *chip,
> >                goto err_free;
> >        }
> >
> > -       fp->rate_table = kmalloc(sizeof(int) * nr_rates, GFP_KERNEL);
> > +       /* Call the triplet parser, and make sure fp->rate_table is NULL.
> > +        * We just use the return value to know how many sample rates we
> > +        * will have to deal with. */
> > +       kfree(fp->rate_table);
> > +       fp->rate_table = NULL;
> > +       fp->nr_rates = parse_uac2_sample_rate_range(fp, nr_triplets, data);
> > +
> > +       if (fp->nr_rates == 0) {
> > +               snd_printk(KERN_ERR "%s(): unable to parse sample rate
> > triplets\n",
> > +                               __func__);
> > +               ret = -EINVAL;
> > +               goto err_free;
> > +       }
> > +
> > +       fp->rate_table = kmalloc(sizeof(int) * fp->nr_rates, GFP_KERNEL);
> >        if (!fp->rate_table) {
> >                ret = -ENOMEM;
> >                goto err_free;
> >        }
> >
> > -       fp->nr_rates = 0;
> > -       fp->rate_min = fp->rate_max = 0;
> > -
> > -       for (i = 0; i < nr_rates; i++) {
> > -               int rate = combine_quad(&data[2 + 12 * i]);
> > -
> > -               fp->rate_table[fp->nr_rates] = rate;
> > -               if (!fp->rate_min || rate < fp->rate_min)
> > -                       fp->rate_min = rate;
> > -               if (!fp->rate_max || rate > fp->rate_max)
> > -                       fp->rate_max = rate;
> > -               fp->rates |= snd_pcm_rate_to_rate_bit(rate);
> > -               fp->nr_rates++;
> > -       }
> > +       /* Call the triplet parser again, but this time, fp->rate_table is
> > +        * allocated, so the rates will be stored */
> > +       parse_uac2_sample_rate_range(fp, nr_triplets, data);
> >
> >  err_free:
> >        kfree(data);
> > --
> > 1.7.1
> >
> >

Re: [PATCH 3/4] ALSA: usb-audio: parse UAC2 sample rate ranges correctly

[Clemens Ladisch]

Daniel Mack wrote:
> On Fri, Jun 11, 2010 at 10:59:44PM +0800, Alex Lee wrote:
> > If the device has a variable clock, it may report a RES of only 1.  So you
> > may end up with thousands (and even millions) of sample rates.  It might be
> > better to have separate code to cater to discrete clocks vs variable clocks.
> 
> AFAIK, ALSA can[...] cope with that [...]. Am I right?

Yes; set fp->rates to SNDRV_PCM_RATE_CONTINUOUS to allow all rates between
fp->rate_min and fp->rate_max.  See, for example, parse_audio_format_rates_v1.


Regards,
Clemens

Re: [PATCH 3/4] ALSA: usb-audio: parse UAC2 sample rate ranges correctly

[Mark Brown]

On Fri, Jun 11, 2010 at 05:04:35PM +0200, Daniel Mack wrote:
> On Fri, Jun 11, 2010 at 10:59:44PM +0800, Alex Lee wrote:
> > If the device has a variable clock, it may report a RES of only 1.  So you
> > may end up with thousands (and even millions) of sample rates.  It might be
> > better to have separate code to cater to discrete clocks vs variable clocks.

> AFAIK, ALSA cannot cope with that yet. Am I right?

There's _CONTINUOUS and _KNOT.

Re: [PATCH 3/4] ALSA: usb-audio: parse UAC2 sample rate ranges correctly

[Daniel Mack]

On Fri, Jun 11, 2010 at 05:14:57PM +0200, Clemens Ladisch wrote:
> Daniel Mack wrote:
> > On Fri, Jun 11, 2010 at 10:59:44PM +0800, Alex Lee wrote:
> > > If the device has a variable clock, it may report a RES of only 1.  So you
> > > may end up with thousands (and even millions) of sample rates.  It might be
> > > better to have separate code to cater to discrete clocks vs variable clocks.
> > 
> > AFAIK, ALSA can[...] cope with that [...]. Am I right?
> 
> Yes; set fp->rates to SNDRV_PCM_RATE_CONTINUOUS to allow all rates between
> fp->rate_min and fp->rate_max.  See, for example, parse_audio_format_rates_v1.

Ah, ok. So we should we do this for all USB devices that report a RES
value of 1. I'll respin this series then. Thanks!

Daniel

[PATCH 1/4] ALSA: usb-audio: add check for faulty clock in parse_audio_format_rates_v2()

[Daniel Mack]

Signed-off-by: Daniel Mack <daniel@caiaq.de>
---
 sound/usb/format.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/sound/usb/format.c b/sound/usb/format.c
index 5367cd1..df5b29f 100644
--- a/sound/usb/format.c
+++ b/sound/usb/format.c
@@ -218,6 +218,12 @@ static int parse_audio_format_rates_v2(struct snd_usb_audio *chip,
 	int i, nr_rates, data_size, ret = 0;
 	int clock = snd_usb_clock_find_source(chip, chip->ctrl_intf, fp->clock);
 
+	if (clock < 0) {
+		snd_printk(KERN_ERR "%s(): unable to find clock source (clock %d)\n",
+				__func__, clock);
+		goto err;
+	}
+
 	/* get the number of sample rates first by only fetching 2 bytes */
 	ret = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_RANGE,
 			      USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,
-- 
1.7.1

[PATCH 2/4] ALSA: usb-audio: fix control messages for USB_RECIP_INTERFACE

[Daniel Mack]

Control messages directed to an interface must have the interface number
set in the lower 8 bits of wIndex. This wasn't done correctly for some
clock and mixer messages.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Reported-by: Alex Lee <alexlee188@gmail.com>
---
 sound/usb/clock.c  |   12 ++++++++----
 sound/usb/format.c |    6 ++++--
 sound/usb/helper.h |    4 ++++
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/sound/usb/clock.c b/sound/usb/clock.c
index b7aadd6..b585511 100644
--- a/sound/usb/clock.c
+++ b/sound/usb/clock.c
@@ -103,7 +103,8 @@ static int uac_clock_selector_get_val(struct snd_usb_audio *chip, int selector_i
 	ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0),
 			      UAC2_CS_CUR,
 			      USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
-			      UAC2_CX_CLOCK_SELECTOR << 8, selector_id << 8,
+			      UAC2_CX_CLOCK_SELECTOR << 8,
+			      snd_usb_ctrl_intf(chip) | (selector_id << 8),
 			      &buf, sizeof(buf), 1000);
 
 	if (ret < 0)
@@ -120,7 +121,8 @@ static bool uac_clock_source_is_valid(struct snd_usb_audio *chip, int source_id)
 
 	err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_CUR,
 			      USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,
-			      UAC2_CS_CONTROL_CLOCK_VALID << 8, source_id << 8,
+			      UAC2_CS_CONTROL_CLOCK_VALID << 8,
+			      snd_usb_ctrl_intf(chip) | (source_id << 8),
 			      &data, sizeof(data), 1000);
 
 	if (err < 0) {
@@ -269,7 +271,8 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface,
 	data[3] = rate >> 24;
 	if ((err = snd_usb_ctl_msg(dev, usb_sndctrlpipe(dev, 0), UAC2_CS_CUR,
 				   USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_OUT,
-				   UAC2_CS_CONTROL_SAM_FREQ << 8, clock << 8,
+				   UAC2_CS_CONTROL_SAM_FREQ << 8,
+				   snd_usb_ctrl_intf(chip) | (clock << 8),
 				   data, sizeof(data), 1000)) < 0) {
 		snd_printk(KERN_ERR "%d:%d:%d: cannot set freq %d (v2)\n",
 			   dev->devnum, iface, fmt->altsetting, rate);
@@ -278,7 +281,8 @@ static int set_sample_rate_v2(struct snd_usb_audio *chip, int iface,
 
 	if ((err = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_CUR,
 				   USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,
-				   UAC2_CS_CONTROL_SAM_FREQ << 8, clock << 8,
+				   UAC2_CS_CONTROL_SAM_FREQ << 8,
+				   snd_usb_ctrl_intf(chip) | (clock << 8),
 				   data, sizeof(data), 1000)) < 0) {
 		snd_printk(KERN_WARNING "%d:%d:%d: cannot get freq (v2)\n",
 			   dev->devnum, iface, fmt->altsetting);
diff --git a/sound/usb/format.c b/sound/usb/format.c
index df5b29f..8eccf17 100644
--- a/sound/usb/format.c
+++ b/sound/usb/format.c
@@ -227,7 +227,8 @@ static int parse_audio_format_rates_v2(struct snd_usb_audio *chip,
 	/* get the number of sample rates first by only fetching 2 bytes */
 	ret = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_RANGE,
 			      USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,
-			      UAC2_CS_CONTROL_SAM_FREQ << 8, clock << 8,
+			      UAC2_CS_CONTROL_SAM_FREQ << 8,
+			      snd_usb_ctrl_intf(chip) | (clock << 8),
 			      tmp, sizeof(tmp), 1000);
 
 	if (ret < 0) {
@@ -247,7 +248,8 @@ static int parse_audio_format_rates_v2(struct snd_usb_audio *chip,
 	/* now get the full information */
 	ret = snd_usb_ctl_msg(dev, usb_rcvctrlpipe(dev, 0), UAC2_CS_RANGE,
 			      USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,
-			      UAC2_CS_CONTROL_SAM_FREQ << 8, clock << 8,
+			      UAC2_CS_CONTROL_SAM_FREQ << 8,
+			      snd_usb_ctrl_intf(chip) | (clock << 8),
 			      data, data_size, 1000);
 
 	if (ret < 0) {
diff --git a/sound/usb/helper.h b/sound/usb/helper.h
index a6b0e51..09bd943 100644
--- a/sound/usb/helper.h
+++ b/sound/usb/helper.h
@@ -28,5 +28,9 @@ unsigned char snd_usb_parse_datainterval(struct snd_usb_audio *chip,
 #define snd_usb_get_speed(dev) ((dev)->speed)
 #endif
 
+static inline int snd_usb_ctrl_intf(struct snd_usb_audio *chip)
+{
+	return get_iface_desc(chip->ctrl_intf)->bInterfaceNumber;
+}
 
 #endif /* __USBAUDIO_HELPER_H */
-- 
1.7.1

[PATCH 3/4] ALSA: usb-audio: parse UAC2 sample rate ranges correctly

[Daniel Mack]

A device may report its supported sample rates in ranges rather than in
discrete triplets. The code used to only parse the MIN field instead of
properly paying attention to the MAX and RES values.

Also, handle RES values of 1 corrently and announce a continous sample
rate range in this case.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Reported-by: Alex Lee <alexlee188@gmail.com>
---
 sound/usb/format.c |   89 +++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 71 insertions(+), 18 deletions(-)

diff --git a/sound/usb/format.c b/sound/usb/format.c
index 8eccf17..7a54a0a 100644
--- a/sound/usb/format.c
+++ b/sound/usb/format.c
@@ -206,6 +206,57 @@ static int parse_audio_format_rates_v1(struct snd_usb_audio *chip, struct audiof
 }
 
 /*
+ * Helper function to walk the array of sample rate triplets reported by
+ * the device. The problem is that we need to parse whole array first to
+ * get to know how many sample rates we have to expect.
+ * Then fp->rate_table can be allocated and filled.
+ */
+static int parse_uac2_sample_rate_range(struct audioformat *fp, int nr_triplets,
+					const unsigned char *data)
+{
+	int i, nr_rates = 0;
+
+	fp->rates = fp->rate_min = fp->rate_max = 0;
+
+	for (i = 0; i < nr_triplets; i++) {
+		int min = combine_quad(&data[2 + 12 * i]);
+		int max = combine_quad(&data[6 + 12 * i]);
+		int res = combine_quad(&data[10 + 12 * i]);
+		int rate;
+
+		if ((max < 0) || (min < 0) || (res < 0) || (max < min))
+			continue;
+
+		/*
+		 * for ranges with res == 1, we want a continuous sample
+		 * rate range, so this function should return 0.
+		 */
+		if (res == 1) {
+			fp->rates = SNDRV_PCM_RATE_CONTINUOUS;
+			return 0;
+		}
+
+		for (rate = min; rate <= max; rate += res) {
+			if (fp->rate_table)
+				fp->rate_table[nr_rates] = rate;
+			if (!fp->rate_min || rate < fp->rate_min)
+				fp->rate_min = rate;
+			if (!fp->rate_max || rate > fp->rate_max)
+				fp->rate_max = rate;
+			fp->rates |= snd_pcm_rate_to_rate_bit(rate);
+
+			nr_rates++;
+
+			/* avoid endless loop */
+			if (res == 0)
+				break;
+		}
+	}
+
+	return nr_rates;
+}
+
+/*
  * parse the format descriptor and stores the possible sample rates
  * on the audioformat table (audio class v2).
  */
@@ -215,7 +266,7 @@ static int parse_audio_format_rates_v2(struct snd_usb_audio *chip,
 {
 	struct usb_device *dev = chip->dev;
 	unsigned char tmp[2], *data;
-	int i, nr_rates, data_size, ret = 0;
+	int nr_triplets, data_size, ret = 0;
 	int clock = snd_usb_clock_find_source(chip, chip->ctrl_intf, fp->clock);
 
 	if (clock < 0) {
@@ -237,8 +288,8 @@ static int parse_audio_format_rates_v2(struct snd_usb_audio *chip,
 		goto err;
 	}
 
-	nr_rates = (tmp[1] << 8) | tmp[0];
-	data_size = 2 + 12 * nr_rates;
+	nr_triplets = (tmp[1] << 8) | tmp[0];
+	data_size = 2 + 12 * nr_triplets;
 	data = kzalloc(data_size, GFP_KERNEL);
 	if (!data) {
 		ret = -ENOMEM;
@@ -259,26 +310,28 @@ static int parse_audio_format_rates_v2(struct snd_usb_audio *chip,
 		goto err_free;
 	}
 
-	fp->rate_table = kmalloc(sizeof(int) * nr_rates, GFP_KERNEL);
+	/* Call the triplet parser, and make sure fp->rate_table is NULL.
+	 * We just use the return value to know how many sample rates we
+	 * will have to deal with. */
+	kfree(fp->rate_table);
+	fp->rate_table = NULL;
+	fp->nr_rates = parse_uac2_sample_rate_range(fp, nr_triplets, data);
+
+	if (fp->nr_rates == 0) {
+		/* SNDRV_PCM_RATE_CONTINUOUS */
+		err = 0;
+		goto err_free;
+	}
+
+	fp->rate_table = kmalloc(sizeof(int) * fp->nr_rates, GFP_KERNEL);
 	if (!fp->rate_table) {
 		ret = -ENOMEM;
 		goto err_free;
 	}
 
-	fp->nr_rates = 0;
-	fp->rate_min = fp->rate_max = 0;
-
-	for (i = 0; i < nr_rates; i++) {
-		int rate = combine_quad(&data[2 + 12 * i]);
-
-		fp->rate_table[fp->nr_rates] = rate;
-		if (!fp->rate_min || rate < fp->rate_min)
-			fp->rate_min = rate;
-		if (!fp->rate_max || rate > fp->rate_max)
-			fp->rate_max = rate;
-		fp->rates |= snd_pcm_rate_to_rate_bit(rate);
-		fp->nr_rates++;
-	}
+	/* Call the triplet parser again, but this time, fp->rate_table is
+	 * allocated, so the rates will be stored */
+	parse_uac2_sample_rate_range(fp, nr_triplets, data);
 
 err_free:
 	kfree(data);
-- 
1.7.1

[PATCH 4/4] ALSA: usb-audio: fix UAC2 control value queries

[Daniel Mack]

For RANGE requests, we should only query as much bytes as we're in fact
interested in.

For CUR requests, we shouldn't confuse the firmware with an overlong
request but just ask for 2 bytes.

This might need fixing in the future as it's not entirely clear when to
dispatch 1-byte, 2-byte and 4-byte request blocks. For now, we assume
everything is coded in 16bit - this works for all firmware
implementations I've seen.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Reported-by: Alex Lee <alexlee188@gmail.com>
---
 sound/usb/mixer.c |   19 ++++++++++++++-----
 1 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
index a060d00..6939d0f 100644
--- a/sound/usb/mixer.c
+++ b/sound/usb/mixer.c
@@ -297,20 +297,27 @@ static int get_ctl_value_v1(struct usb_mixer_elem_info *cval, int request, int v
 
 static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, int validx, int *value_ret)
 {
-	unsigned char buf[14]; /* enough space for one range of 4 bytes */
+	unsigned char buf[2 + 3*sizeof(__u16)]; /* enough space for one range */
 	unsigned char *val;
-	int ret;
+	int ret, size;
 	__u8 bRequest;
 
-	bRequest = (request == UAC_GET_CUR) ?
-		UAC2_CS_CUR : UAC2_CS_RANGE;
+	if (request == UAC_GET_CUR) {
+		bRequest = UAC2_CS_CUR;
+		size = sizeof(__u16);
+	} else {
+		bRequest = UAC2_CS_RANGE;
+		size = sizeof(buf);
+	}
+
+	memset(buf, 0, sizeof(buf));
 
 	ret = snd_usb_ctl_msg(cval->mixer->chip->dev,
 			      usb_rcvctrlpipe(cval->mixer->chip->dev, 0),
 			      bRequest,
 			      USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
 			      validx, cval->mixer->ctrlif | (cval->id << 8),
-			      buf, sizeof(buf), 1000);
+			      buf, size, 1000);
 
 	if (ret < 0) {
 		snd_printk(KERN_ERR "cannot get ctl value: req = %#x, wValue = %#x, wIndex = %#x, type = %d\n",
@@ -318,6 +325,8 @@ static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, int v
 		return ret;
 	}
 
+	/* FIXME: how should we handle multiple triplets here? */
+
 	switch (request) {
 	case UAC_GET_CUR:
 		val = buf;
-- 
1.7.1

Re: [PATCH 3/4] ALSA: usb-audio: parse UAC2 sample rate ranges correctly

[Takashi Iwai]

At Fri, 11 Jun 2010 17:34:21 +0200,
Daniel Mack wrote:
> 
> +		/*
> +		 * for ranges with res == 1, we want a continuous sample
> +		 * rate range, so this function should return 0.
> +		 */
> +		if (res == 1) {
> +			fp->rates = SNDRV_PCM_RATE_CONTINUOUS;
> +			return 0;

Missing setup of fp->rate_min and fp->rate_max here?


Takashi

Re: [PATCH 3/4] ALSA: usb-audio: parse UAC2 sample rate ranges correctly

[Daniel Mack]

On Fri, Jun 11, 2010 at 05:43:04PM +0200, Takashi Iwai wrote:
> At Fri, 11 Jun 2010 17:34:21 +0200,
> Daniel Mack wrote:
> > 
> > +		/*
> > +		 * for ranges with res == 1, we want a continuous sample
> > +		 * rate range, so this function should return 0.
> > +		 */
> > +		if (res == 1) {
> > +			fp->rates = SNDRV_PCM_RATE_CONTINUOUS;
> > +			return 0;
> 
> Missing setup of fp->rate_min and fp->rate_max here?

Gna, not my day. I did that, and of course tested it with a modified
firmware and it worked well.

But appearantly I exported the wrong patch. Sorry.


Daniel


>From 6cefee7ab5168b0875af0aaaa78a0e7b6cce1890 Mon Sep 17 00:00:00 2001
From: Daniel Mack <daniel@caiaq.de>
Date: Fri, 11 Jun 2010 11:32:29 +0200
Subject: [PATCH] ALSA: usb-audio: parse UAC2 sample rate ranges correctly

A device may report its supported sample rates in ranges rather than in
discrete triplets. The code used to only parse the MIN field instead of
properly paying attention to the MAX and RES values.

Also, handle RES values of 1 correctly and announce a continous sample
rate range in this case.

Signed-off-by: Daniel Mack <daniel@caiaq.de>
Reported-by: Alex Lee <alexlee188@gmail.com>
---
 sound/usb/format.c |   92 +++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 74 insertions(+), 18 deletions(-)

diff --git a/sound/usb/format.c b/sound/usb/format.c
index 8eccf17..30364ab 100644
--- a/sound/usb/format.c
+++ b/sound/usb/format.c
@@ -206,6 +206,60 @@ static int parse_audio_format_rates_v1(struct snd_usb_audio *chip, struct audiof
 }
 
 /*
+ * Helper function to walk the array of sample rate triplets reported by
+ * the device. The problem is that we need to parse whole array first to
+ * get to know how many sample rates we have to expect.
+ * Then fp->rate_table can be allocated and filled.
+ */
+static int parse_uac2_sample_rate_range(struct audioformat *fp, int nr_triplets,
+					const unsigned char *data)
+{
+	int i, nr_rates = 0;
+
+	fp->rates = fp->rate_min = fp->rate_max = 0;
+
+	for (i = 0; i < nr_triplets; i++) {
+		int min = combine_quad(&data[2 + 12 * i]);
+		int max = combine_quad(&data[6 + 12 * i]);
+		int res = combine_quad(&data[10 + 12 * i]);
+		int rate;
+
+		if ((max < 0) || (min < 0) || (res < 0) || (max < min))
+			continue;
+
+		/*
+		 * for ranges with res == 1, we announce a continuous sample
+		 * rate range, and this function should return 0 for no further
+		 * parsing.
+		 */
+		if (res == 1) {
+			fp->rate_min = min;
+			fp->rate_max = max;
+			fp->rates = SNDRV_PCM_RATE_CONTINUOUS;
+			return 0;
+		}
+
+		for (rate = min; rate <= max; rate += res) {
+			if (fp->rate_table)
+				fp->rate_table[nr_rates] = rate;
+			if (!fp->rate_min || rate < fp->rate_min)
+				fp->rate_min = rate;
+			if (!fp->rate_max || rate > fp->rate_max)
+				fp->rate_max = rate;
+			fp->rates |= snd_pcm_rate_to_rate_bit(rate);
+
+			nr_rates++;
+
+			/* avoid endless loop */
+			if (res == 0)
+				break;
+		}
+	}
+
+	return nr_rates;
+}
+
+/*
  * parse the format descriptor and stores the possible sample rates
  * on the audioformat table (audio class v2).
  */
@@ -215,7 +269,7 @@ static int parse_audio_format_rates_v2(struct snd_usb_audio *chip,
 {
 	struct usb_device *dev = chip->dev;
 	unsigned char tmp[2], *data;
-	int i, nr_rates, data_size, ret = 0;
+	int nr_triplets, data_size, ret = 0;
 	int clock = snd_usb_clock_find_source(chip, chip->ctrl_intf, fp->clock);
 
 	if (clock < 0) {
@@ -237,8 +291,8 @@ static int parse_audio_format_rates_v2(struct snd_usb_audio *chip,
 		goto err;
 	}
 
-	nr_rates = (tmp[1] << 8) | tmp[0];
-	data_size = 2 + 12 * nr_rates;
+	nr_triplets = (tmp[1] << 8) | tmp[0];
+	data_size = 2 + 12 * nr_triplets;
 	data = kzalloc(data_size, GFP_KERNEL);
 	if (!data) {
 		ret = -ENOMEM;
@@ -259,26 +313,28 @@ static int parse_audio_format_rates_v2(struct snd_usb_audio *chip,
 		goto err_free;
 	}
 
-	fp->rate_table = kmalloc(sizeof(int) * nr_rates, GFP_KERNEL);
+	/* Call the triplet parser, and make sure fp->rate_table is NULL.
+	 * We just use the return value to know how many sample rates we
+	 * will have to deal with. */
+	kfree(fp->rate_table);
+	fp->rate_table = NULL;
+	fp->nr_rates = parse_uac2_sample_rate_range(fp, nr_triplets, data);
+
+	if (fp->nr_rates == 0) {
+		/* SNDRV_PCM_RATE_CONTINUOUS */
+		ret = 0;
+		goto err_free;
+	}
+
+	fp->rate_table = kmalloc(sizeof(int) * fp->nr_rates, GFP_KERNEL);
 	if (!fp->rate_table) {
 		ret = -ENOMEM;
 		goto err_free;
 	}
 
-	fp->nr_rates = 0;
-	fp->rate_min = fp->rate_max = 0;
-
-	for (i = 0; i < nr_rates; i++) {
-		int rate = combine_quad(&data[2 + 12 * i]);
-
-		fp->rate_table[fp->nr_rates] = rate;
-		if (!fp->rate_min || rate < fp->rate_min)
-			fp->rate_min = rate;
-		if (!fp->rate_max || rate > fp->rate_max)
-			fp->rate_max = rate;
-		fp->rates |= snd_pcm_rate_to_rate_bit(rate);
-		fp->nr_rates++;
-	}
+	/* Call the triplet parser again, but this time, fp->rate_table is
+	 * allocated, so the rates will be stored */
+	parse_uac2_sample_rate_range(fp, nr_triplets, data);
 
 err_free:
 	kfree(data);
-- 
1.7.1

Re: [PATCH 3/4] ALSA: usb-audio: parse UAC2 sample rate ranges correctly

[Takashi Iwai]

At Fri, 11 Jun 2010 17:46:33 +0200,
Daniel Mack wrote:
> 
> On Fri, Jun 11, 2010 at 05:43:04PM +0200, Takashi Iwai wrote:
> > At Fri, 11 Jun 2010 17:34:21 +0200,
> > Daniel Mack wrote:
> > > 
> > > +		/*
> > > +		 * for ranges with res == 1, we want a continuous sample
> > > +		 * rate range, so this function should return 0.
> > > +		 */
> > > +		if (res == 1) {
> > > +			fp->rates = SNDRV_PCM_RATE_CONTINUOUS;
> > > +			return 0;
> > 
> > Missing setup of fp->rate_min and fp->rate_max here?
> 
> Gna, not my day. I did that, and of course tested it with a modified
> firmware and it worked well.
> 
> But appearantly I exported the wrong patch. Sorry.

Well, people can understand it if they see the current score of the
opening football game ;)


Takashi

Re: [PATCH 1/4] ALSA: usb-audio: add check for faulty clock in parse_audio_format_rates_v2()

[Takashi Iwai]

At Fri, 11 Jun 2010 17:34:19 +0200,
Daniel Mack wrote:
> 
> Signed-off-by: Daniel Mack <daniel@caiaq.de>

Applied all (fixed) patches now.  Thanks!


Takashi

Re: [PATCH 3/4] ALSA: usb-audio: parse UAC2 sample rate ranges correctly

[Daniel Mack]

On Fri, Jun 11, 2010 at 10:59:44PM +0800, Alex Lee wrote:
> If the device has a variable clock, it may report a RES of only 1.  So you
> may end up with thousands (and even millions) of sample rates.  It might be
> better to have separate code to cater to discrete clocks vs variable clocks.

Just for the records: this is exactly what Mac OS X does, and it will
comsume 100% CPU when a device with such descriptors is plugged in. So
even though this is now sanely handled by ALSA, it really shouldn't be
done that way by implementors.

Daniel