[PATCH v5 0/5] ALSA: usb-audio: Fixes for Pioneer devices

[PATCH v5 0/5] ALSA: usb-audio: Fixes for Pioneer devices

[Takashi Iwai]

Hi,

here is the revised patches for addressing the regressions reported
on the Pioneer USB-audio devices.  Those have the special
configuration where both playback and capture endpoints share the same
interface with the vendor-spec descriptors.  It required the extension
of the endpoint management code and the setup of the implicit feedback
mode.

v4->v5:
  - Fix the implicit feedback detection
  - Correct the maxpacksize for ep_idx=1 case


Takashi

===

Takashi Iwai (5):
  ALSA: usb-audio: Fix the missing endpoints creations for quirks
  ALSA: usb-audio: Choose audioformat of a counter-part substream
  ALSA: usb-audio: Avoid unnecessary interface re-setup
  ALSA: usb-audio: Annotate the endpoint index in audioformat
  ALSA: usb-audio: Fix implicit feedback sync setup for Pioneer devices

 sound/usb/card.c         |  5 +--
 sound/usb/card.h         |  3 ++
 sound/usb/endpoint.c     | 84 ++++++++++++++++++++++++++++++++++------
 sound/usb/endpoint.h     |  2 +-
 sound/usb/implicit.c     | 57 ++++++++++++++++++++-------
 sound/usb/quirks-table.h |  6 +++
 sound/usb/quirks.c       | 58 +++++++++++++++++++++------
 sound/usb/usbaudio.h     |  1 +
 8 files changed, 173 insertions(+), 43 deletions(-)

-- 
2.26.2

[PATCH v5 1/5] ALSA: usb-audio: Fix the missing endpoints creations for quirks

[Takashi Iwai]

The recent change in the endpoint management moved the endpoint object
creation from the stream open time to the parser of the audio
descriptor.  It works fine for the standard audio, but it overlooked
the other places that create audio streams via quirks
(QUIRK_AUDIO_FIXED_ENDPOINT) like the reported a few Pioneer devices;
those call snd_usb_add_audio_stream() manually, hence they miss the
endpoints, eventually resulting in the error at opening streams.
Moreover, now the sync EP setup was moved to the explicit call of
snd_usb_audioformat_set_sync_ep(), and this needs to be added for
those places, too.

This patch addresses those regressions for quirks.  It adds a local
helper function add_audio_stream_from_fixed_fmt(), which does the all
needed tasks, and replaces the calls of snd_usb_add_audio_stream()
with this new function.

Fixes: 54cb31901b83 ("ALSA: usb-audio: Create endpoint objects at parsing phase")
Reported-by: František Kučera <konference@frantovo.cz>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/quirks.c | 54 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 10 deletions(-)

diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index e4a690bb4c99..b70e2ebc3e29 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -120,6 +120,40 @@ static int create_standard_audio_quirk(struct snd_usb_audio *chip,
 	return 0;
 }
 
+/* create the audio stream and the corresponding endpoints from the fixed
+ * audioformat object; this is used for quirks with the fixed EPs
+ */
+static int add_audio_stream_from_fixed_fmt(struct snd_usb_audio *chip,
+					   struct audioformat *fp)
+{
+	int stream, err;
+
+	stream = (fp->endpoint & USB_DIR_IN) ?
+		SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
+
+	snd_usb_audioformat_set_sync_ep(chip, fp);
+
+	err = snd_usb_add_audio_stream(chip, stream, fp);
+	if (err < 0)
+		return err;
+
+	err = snd_usb_add_endpoint(chip, fp->endpoint,
+				   SND_USB_ENDPOINT_TYPE_DATA);
+	if (err < 0)
+		return err;
+
+	if (fp->sync_ep) {
+		err = snd_usb_add_endpoint(chip, fp->sync_ep,
+					   fp->implicit_fb ?
+					   SND_USB_ENDPOINT_TYPE_DATA :
+					   SND_USB_ENDPOINT_TYPE_SYNC);
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
+}
+
 /*
  * create a stream for an endpoint/altsetting without proper descriptors
  */
@@ -131,8 +165,8 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
 	struct audioformat *fp;
 	struct usb_host_interface *alts;
 	struct usb_interface_descriptor *altsd;
-	int stream, err;
 	unsigned *rate_table = NULL;
+	int err;
 
 	fp = kmemdup(quirk->data, sizeof(*fp), GFP_KERNEL);
 	if (!fp)
@@ -153,11 +187,6 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
 		fp->rate_table = rate_table;
 	}
 
-	stream = (fp->endpoint & USB_DIR_IN)
-		? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
-	err = snd_usb_add_audio_stream(chip, stream, fp);
-	if (err < 0)
-		goto error;
 	if (fp->iface != get_iface_desc(&iface->altsetting[0])->bInterfaceNumber ||
 	    fp->altset_idx >= iface->num_altsetting) {
 		err = -EINVAL;
@@ -176,6 +205,13 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
 		fp->datainterval = snd_usb_parse_datainterval(chip, alts);
 	if (fp->maxpacksize == 0)
 		fp->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize);
+	if (!fp->fmt_type)
+		fp->fmt_type = UAC_FORMAT_TYPE_I;
+
+	err = add_audio_stream_from_fixed_fmt(chip, fp);
+	if (err < 0)
+		goto error;
+
 	usb_set_interface(chip->dev, fp->iface, 0);
 	snd_usb_init_pitch(chip, fp);
 	snd_usb_init_sample_rate(chip, fp, fp->rate_max);
@@ -417,7 +453,7 @@ static int create_uaxx_quirk(struct snd_usb_audio *chip,
 	struct usb_host_interface *alts;
 	struct usb_interface_descriptor *altsd;
 	struct audioformat *fp;
-	int stream, err;
+	int err;
 
 	/* both PCM and MIDI interfaces have 2 or more altsettings */
 	if (iface->num_altsetting < 2)
@@ -482,9 +518,7 @@ static int create_uaxx_quirk(struct snd_usb_audio *chip,
 		return -ENXIO;
 	}
 
-	stream = (fp->endpoint & USB_DIR_IN)
-		? SNDRV_PCM_STREAM_CAPTURE : SNDRV_PCM_STREAM_PLAYBACK;
-	err = snd_usb_add_audio_stream(chip, stream, fp);
+	err = add_audio_stream_from_fixed_fmt(chip, fp);
 	if (err < 0) {
 		list_del(&fp->list); /* unlink for avoiding double-free */
 		kfree(fp);
-- 
2.26.2

[PATCH v5 2/5] ALSA: usb-audio: Choose audioformat of a counter-part substream

[Takashi Iwai]

The implicit feedback mode needs to handle two endpoints and the
choice of the audioformat object for the sync EP is important since
this determines the compatibility of the hw_params.  The current code
uses the same audioformat object if both the main EP and the sync EP
point to the same iface/altsetting.  This was done in consideration of
the non-implicit-fb sync EP handling, and it doesn't match well with
the cases where actually to endpoints are defined in the sameiface /
altsetting like a few Pioneer devices.

Modify snd_usb_find_implicit_fb_sync_format() to pick up the
audioformat that is assigned in the counter-part substreams primarily,
so that the actual capture stream can be opened properly.  We keep the
same audioformat object only as a fallback in case nothing found,
though.

Fixes: 9fddc15e8039 ("ALSA: usb-audio: Factor out the implicit feedback quirk code")
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/implicit.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/sound/usb/implicit.c b/sound/usb/implicit.c
index 931042a6a051..9724efe1cdce 100644
--- a/sound/usb/implicit.c
+++ b/sound/usb/implicit.c
@@ -378,20 +378,19 @@ snd_usb_find_implicit_fb_sync_format(struct snd_usb_audio *chip,
 				     int stream)
 {
 	struct snd_usb_substream *subs;
-	const struct audioformat *fp, *sync_fmt;
+	const struct audioformat *fp, *sync_fmt = NULL;
 	int score, high_score;
 
-	/* When sharing the same altset, use the original audioformat */
+	/* Use the original audioformat as fallback for the shared altset */
 	if (target->iface == target->sync_iface &&
 	    target->altsetting == target->sync_altsetting)
-		return target;
+		sync_fmt = target;
 
 	subs = find_matching_substream(chip, stream, target->sync_ep,
 				       target->fmt_type);
 	if (!subs)
-		return NULL;
+		return sync_fmt;
 
-	sync_fmt = NULL;
 	high_score = 0;
 	list_for_each_entry(fp, &subs->fmt_list, list) {
 		score = match_endpoint_audioformats(subs, fp,
-- 
2.26.2

[PATCH v5 3/5] ALSA: usb-audio: Avoid unnecessary interface re-setup

[Takashi Iwai]

The current endpoint handling assumed (more or less) a unique 1:1
relation between the endpoint and the iface/altset.  The exception was
the sync EP without the implicit feedback which has usually the
secondary EP of the same altset.  This works fine for most devices,
but it turned out that some unusual devices like Pinoeer's ones have
both playback and capture endpoints in the same iface/altsetting and
use both for the implicit feedback mode.  For handling such a case, we
need to extend the endpoint management to take the shared interface
into account.

This patch does that: it adds a new object snd_usb_iface_ref for
managing the reference counts of the each USB interface that is used
by each endpoint.  The interface setup is performed only once for the
(sharing) endpoints, and the doubly initialization is avoided.

Along with this, the resource release of endpoints and interface
refcounts are put into a single function, snd_usb_endpoint_free_all()
instead of looping in the caller side.

Fixes: bf6313a0ff76 ("ALSA: usb-audio: Refactor endpoint management")
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/card.c     |  5 ++-
 sound/usb/card.h     |  2 ++
 sound/usb/endpoint.c | 82 ++++++++++++++++++++++++++++++++++++++------
 sound/usb/endpoint.h |  2 +-
 sound/usb/usbaudio.h |  1 +
 5 files changed, 77 insertions(+), 15 deletions(-)

diff --git a/sound/usb/card.c b/sound/usb/card.c
index d731ca62d599..e08fbf8e3ee0 100644
--- a/sound/usb/card.c
+++ b/sound/usb/card.c
@@ -450,10 +450,8 @@ lookup_device_name(u32 id)
 static void snd_usb_audio_free(struct snd_card *card)
 {
 	struct snd_usb_audio *chip = card->private_data;
-	struct snd_usb_endpoint *ep, *n;
 
-	list_for_each_entry_safe(ep, n, &chip->ep_list, list)
-		snd_usb_endpoint_free(ep);
+	snd_usb_endpoint_free_all(chip);
 
 	mutex_destroy(&chip->mutex);
 	if (!atomic_read(&chip->shutdown))
@@ -611,6 +609,7 @@ static int snd_usb_audio_create(struct usb_interface *intf,
 	chip->usb_id = usb_id;
 	INIT_LIST_HEAD(&chip->pcm_list);
 	INIT_LIST_HEAD(&chip->ep_list);
+	INIT_LIST_HEAD(&chip->iface_ref_list);
 	INIT_LIST_HEAD(&chip->midi_list);
 	INIT_LIST_HEAD(&chip->mixer_list);
 
diff --git a/sound/usb/card.h b/sound/usb/card.h
index 6a027c349194..de0d2aa883fa 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -42,6 +42,7 @@ struct audioformat {
 };
 
 struct snd_usb_substream;
+struct snd_usb_iface_ref;
 struct snd_usb_endpoint;
 struct snd_usb_power_domain;
 
@@ -58,6 +59,7 @@ struct snd_urb_ctx {
 
 struct snd_usb_endpoint {
 	struct snd_usb_audio *chip;
+	struct snd_usb_iface_ref *iface_ref;
 
 	int opened;		/* open refcount; protect with chip->mutex */
 	atomic_t running;	/* running status */
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 162da7a50046..ae6276aded91 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -24,6 +24,14 @@
 #define EP_FLAG_RUNNING		1
 #define EP_FLAG_STOPPING	2
 
+/* interface refcounting */
+struct snd_usb_iface_ref {
+	unsigned char iface;
+	bool need_setup;
+	int opened;
+	struct list_head list;
+};
+
 /*
  * snd_usb_endpoint is a model that abstracts everything related to an
  * USB endpoint and its streaming.
@@ -488,6 +496,28 @@ static void snd_complete_urb(struct urb *urb)
 	clear_bit(ctx->index, &ep->active_mask);
 }
 
+/*
+ * Find or create a refcount object for the given interface
+ *
+ * The objects are released altogether in snd_usb_endpoint_free_all()
+ */
+static struct snd_usb_iface_ref *
+iface_ref_find(struct snd_usb_audio *chip, int iface)
+{
+	struct snd_usb_iface_ref *ip;
+
+	list_for_each_entry(ip, &chip->iface_ref_list, list)
+		if (ip->iface == iface)
+			return ip;
+
+	ip = kzalloc(sizeof(*ip), GFP_KERNEL);
+	if (!ip)
+		return NULL;
+	ip->iface = iface;
+	list_add_tail(&ip->list, &chip->iface_ref_list);
+	return ip;
+}
+
 /*
  * Get the existing endpoint object corresponding EP
  * Returns NULL if not present.
@@ -520,8 +550,8 @@ snd_usb_get_endpoint(struct snd_usb_audio *chip, int ep_num)
  *
  * Returns zero on success or a negative error code.
  *
- * New endpoints will be added to chip->ep_list and must be freed by
- * calling snd_usb_endpoint_free().
+ * New endpoints will be added to chip->ep_list and freed by
+ * calling snd_usb_endpoint_free_all().
  *
  * For SND_USB_ENDPOINT_TYPE_SYNC, the caller needs to guarantee that
  * bNumEndpoints > 1 beforehand.
@@ -658,6 +688,12 @@ snd_usb_endpoint_open(struct snd_usb_audio *chip,
 		usb_audio_dbg(chip, "Open EP 0x%x, iface=%d:%d, idx=%d\n",
 			      ep_num, ep->iface, ep->altsetting, ep->ep_idx);
 
+		ep->iface_ref = iface_ref_find(chip, ep->iface);
+		if (!ep->iface_ref) {
+			ep = NULL;
+			goto unlock;
+		}
+
 		ep->cur_audiofmt = fp;
 		ep->cur_channels = fp->channels;
 		ep->cur_rate = params_rate(params);
@@ -681,6 +717,11 @@ snd_usb_endpoint_open(struct snd_usb_audio *chip,
 			      ep->implicit_fb_sync);
 
 	} else {
+		if (WARN_ON(!ep->iface_ref)) {
+			ep = NULL;
+			goto unlock;
+		}
+
 		if (!endpoint_compatible(ep, fp, params)) {
 			usb_audio_err(chip, "Incompatible EP setup for 0x%x\n",
 				      ep_num);
@@ -692,6 +733,9 @@ snd_usb_endpoint_open(struct snd_usb_audio *chip,
 			      ep_num, ep->opened);
 	}
 
+	if (!ep->iface_ref->opened++)
+		ep->iface_ref->need_setup = true;
+
 	ep->opened++;
 
  unlock:
@@ -760,12 +804,16 @@ void snd_usb_endpoint_close(struct snd_usb_audio *chip,
 	mutex_lock(&chip->mutex);
 	usb_audio_dbg(chip, "Closing EP 0x%x (count %d)\n",
 		      ep->ep_num, ep->opened);
-	if (!--ep->opened) {
+
+	if (!--ep->iface_ref->opened)
 		endpoint_set_interface(chip, ep, false);
+
+	if (!--ep->opened) {
 		ep->iface = 0;
 		ep->altsetting = 0;
 		ep->cur_audiofmt = NULL;
 		ep->cur_rate = 0;
+		ep->iface_ref = NULL;
 		usb_audio_dbg(chip, "EP 0x%x closed\n", ep->ep_num);
 	}
 	mutex_unlock(&chip->mutex);
@@ -775,6 +823,8 @@ void snd_usb_endpoint_close(struct snd_usb_audio *chip,
 void snd_usb_endpoint_suspend(struct snd_usb_endpoint *ep)
 {
 	ep->need_setup = true;
+	if (ep->iface_ref)
+		ep->iface_ref->need_setup = true;
 }
 
 /*
@@ -1195,11 +1245,13 @@ int snd_usb_endpoint_configure(struct snd_usb_audio *chip,
 	int err = 0;
 
 	mutex_lock(&chip->mutex);
+	if (WARN_ON(!ep->iface_ref))
+		goto unlock;
 	if (!ep->need_setup)
 		goto unlock;
 
-	/* No need to (re-)configure the sync EP belonging to the same altset */
-	if (ep->ep_idx) {
+	/* If the interface has been already set up, just set EP parameters */
+	if (!ep->iface_ref->need_setup) {
 		err = snd_usb_endpoint_set_params(chip, ep);
 		if (err < 0)
 			goto unlock;
@@ -1242,6 +1294,8 @@ int snd_usb_endpoint_configure(struct snd_usb_audio *chip,
 			goto unlock;
 	}
 
+	ep->iface_ref->need_setup = false;
+
  done:
 	ep->need_setup = false;
 	err = 1;
@@ -1387,15 +1441,21 @@ void snd_usb_endpoint_release(struct snd_usb_endpoint *ep)
 }
 
 /**
- * snd_usb_endpoint_free: Free the resources of an snd_usb_endpoint
+ * snd_usb_endpoint_free_all: Free the resources of an snd_usb_endpoint
+ * @card: The chip
  *
- * @ep: the endpoint to free
- *
- * This free all resources of the given ep.
+ * This free all endpoints and those resources
  */
-void snd_usb_endpoint_free(struct snd_usb_endpoint *ep)
+void snd_usb_endpoint_free_all(struct snd_usb_audio *chip)
 {
-	kfree(ep);
+	struct snd_usb_endpoint *ep, *en;
+	struct snd_usb_iface_ref *ip, *in;
+
+	list_for_each_entry_safe(ep, en, &chip->ep_list, list)
+		kfree(ep);
+
+	list_for_each_entry_safe(ip, in, &chip->iface_ref_list, list)
+		kfree(ip);
 }
 
 /*
diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h
index 11e3bb839fd7..eea4ca49876d 100644
--- a/sound/usb/endpoint.h
+++ b/sound/usb/endpoint.h
@@ -42,7 +42,7 @@ void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep);
 void snd_usb_endpoint_suspend(struct snd_usb_endpoint *ep);
 int  snd_usb_endpoint_activate(struct snd_usb_endpoint *ep);
 void snd_usb_endpoint_release(struct snd_usb_endpoint *ep);
-void snd_usb_endpoint_free(struct snd_usb_endpoint *ep);
+void snd_usb_endpoint_free_all(struct snd_usb_audio *chip);
 
 int snd_usb_endpoint_implicit_feedback_sink(struct snd_usb_endpoint *ep);
 int snd_usb_endpoint_next_packet_size(struct snd_usb_endpoint *ep,
diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h
index 980287aadd36..215c1771dd57 100644
--- a/sound/usb/usbaudio.h
+++ b/sound/usb/usbaudio.h
@@ -44,6 +44,7 @@ struct snd_usb_audio {
 
 	struct list_head pcm_list;	/* list of pcm streams */
 	struct list_head ep_list;	/* list of audio-related endpoints */
+	struct list_head iface_ref_list; /* list of interface refcounts */
 	int pcm_devs;
 
 	struct list_head midi_list;	/* list of midi interfaces */
-- 
2.26.2

Re: [PATCH v5 3/5] ALSA: usb-audio: Avoid unnecessary interface re-setup

[Pierre-Louis Bossart]

>   /**
> - * snd_usb_endpoint_free: Free the resources of an snd_usb_endpoint
> + * snd_usb_endpoint_free_all: Free the resources of an snd_usb_endpoint
> + * @card: The chip
>    *
> - * @ep: the endpoint to free
> - *
> - * This free all resources of the given ep.
> + * This free all endpoints and those resources
>    */
> -void snd_usb_endpoint_free(struct snd_usb_endpoint *ep)
> +void snd_usb_endpoint_free_all(struct snd_usb_audio *chip)

this causes warnings with make sound/ W=1

   CC [M]  sound/usb/endpoint.o
sound/usb/endpoint.c:1459: warning: Function parameter or member 'chip' 
not described in 'snd_usb_endpoint_free_all'
sound/usb/endpoint.c:1459: warning: Excess function parameter 'card' 
description in 'snd_usb_endpoint_free_all'

the following diff removes it, but I don't know if the description is 
good enough...

diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 8e568823c992..4d1c678a0d80 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -1451,7 +1451,7 @@ void snd_usb_endpoint_release(struct 
snd_usb_endpoint *ep)

  /**
   * snd_usb_endpoint_free_all: Free the resources of an snd_usb_endpoint
- * @card: The chip
+ * @chip: The chip
   *
   * This free all endpoints and those resources
   */

Re: [PATCH v5 3/5] ALSA: usb-audio: Avoid unnecessary interface re-setup

[Takashi Iwai]

On Thu, 04 Feb 2021 22:44:00 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> 
> >   /**
> > - * snd_usb_endpoint_free: Free the resources of an snd_usb_endpoint
> > + * snd_usb_endpoint_free_all: Free the resources of an snd_usb_endpoint
> > + * @card: The chip
> >    *
> > - * @ep: the endpoint to free
> > - *
> > - * This free all resources of the given ep.
> > + * This free all endpoints and those resources
> >    */
> > -void snd_usb_endpoint_free(struct snd_usb_endpoint *ep)
> > +void snd_usb_endpoint_free_all(struct snd_usb_audio *chip)
> 
> this causes warnings with make sound/ W=1
> 
>   CC [M]  sound/usb/endpoint.o
> sound/usb/endpoint.c:1459: warning: Function parameter or member
> 'chip' not described in 'snd_usb_endpoint_free_all'
> sound/usb/endpoint.c:1459: warning: Excess function parameter 'card'
> description in 'snd_usb_endpoint_free_all'
> 
> the following diff removes it, but I don't know if the description is
> good enough...
> 
> diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
> index 8e568823c992..4d1c678a0d80 100644
> --- a/sound/usb/endpoint.c
> +++ b/sound/usb/endpoint.c
> @@ -1451,7 +1451,7 @@ void snd_usb_endpoint_release(struct
> snd_usb_endpoint *ep)
> 
>  /**
>   * snd_usb_endpoint_free_all: Free the resources of an snd_usb_endpoint
> - * @card: The chip
> + * @chip: The chip
>   *
>   * This free all endpoints and those resources
>   */

Thanks, I'll queue the fix.


Takashi

[PATCH v5 4/5] ALSA: usb-audio: Annotate the endpoint index in audioformat

[Takashi Iwai]

There are devices that have multiple endpoints sharing the same
iface/altset not only for sync but also for the actual streams, and
the audioformat for such an endpoint needs to be handled with the
proper endpoint index; otherwise it confuses the endpoint management.

This patch extends the audioformat to annotate the endpoint index, and
put the proper ep_idx=1 to Pioneer device quirk entries accordingly.

Fixes: bf6313a0ff76 ("ALSA: usb-audio: Refactor endpoint management")
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/card.h         | 1 +
 sound/usb/endpoint.c     | 2 +-
 sound/usb/quirks-table.h | 6 ++++++
 sound/usb/quirks.c       | 4 ++--
 4 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/sound/usb/card.h b/sound/usb/card.h
index de0d2aa883fa..37091b117614 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -18,6 +18,7 @@ struct audioformat {
 	unsigned int frame_size;	/* samples per frame for non-audio */
 	unsigned char iface;		/* interface number */
 	unsigned char altsetting;	/* corresponding alternate setting */
+	unsigned char ep_idx;		/* endpoint array index */
 	unsigned char altset_idx;	/* array index of altenate setting */
 	unsigned char attributes;	/* corresponding attributes of cs endpoint */
 	unsigned char endpoint;		/* endpoint */
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index ae6276aded91..fe73fe3ff2bc 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -683,7 +683,7 @@ snd_usb_endpoint_open(struct snd_usb_audio *chip,
 		} else {
 			ep->iface = fp->iface;
 			ep->altsetting = fp->altsetting;
-			ep->ep_idx = 0;
+			ep->ep_idx = fp->ep_idx;
 		}
 		usb_audio_dbg(chip, "Open EP 0x%x, iface=%d:%d, idx=%d\n",
 			      ep_num, ep->iface, ep->altsetting, ep->ep_idx);
diff --git a/sound/usb/quirks-table.h b/sound/usb/quirks-table.h
index 0e11cb96fa8c..c8a4bdf18207 100644
--- a/sound/usb/quirks-table.h
+++ b/sound/usb/quirks-table.h
@@ -3362,6 +3362,7 @@ AU0828_DEVICE(0x2040, 0x7270, "Hauppauge", "HVR-950Q"),
 					.altsetting = 1,
 					.altset_idx = 1,
 					.endpoint = 0x86,
+					.ep_idx = 1,
 					.ep_attr = USB_ENDPOINT_XFER_ISOC|
 						 USB_ENDPOINT_SYNC_ASYNC|
 						 USB_ENDPOINT_USAGE_IMPLICIT_FB,
@@ -3450,6 +3451,7 @@ AU0828_DEVICE(0x2040, 0x7270, "Hauppauge", "HVR-950Q"),
 					.altsetting = 1,
 					.altset_idx = 1,
 					.endpoint = 0x82,
+					.ep_idx = 1,
 					.ep_attr = USB_ENDPOINT_XFER_ISOC|
 						USB_ENDPOINT_SYNC_ASYNC|
 						USB_ENDPOINT_USAGE_IMPLICIT_FB,
@@ -3506,6 +3508,7 @@ AU0828_DEVICE(0x2040, 0x7270, "Hauppauge", "HVR-950Q"),
 					.altsetting = 1,
 					.altset_idx = 1,
 					.endpoint = 0x82,
+					.ep_idx = 1,
 					.ep_attr = USB_ENDPOINT_XFER_ISOC|
 						 USB_ENDPOINT_SYNC_ASYNC|
 						 USB_ENDPOINT_USAGE_IMPLICIT_FB,
@@ -3562,6 +3565,7 @@ AU0828_DEVICE(0x2040, 0x7270, "Hauppauge", "HVR-950Q"),
 					.altsetting = 1,
 					.altset_idx = 1,
 					.endpoint = 0x82,
+					.ep_idx = 1,
 					.ep_attr = USB_ENDPOINT_XFER_ISOC|
 						 USB_ENDPOINT_SYNC_ASYNC|
 						 USB_ENDPOINT_USAGE_IMPLICIT_FB,
@@ -3619,6 +3623,7 @@ AU0828_DEVICE(0x2040, 0x7270, "Hauppauge", "HVR-950Q"),
 					.altsetting = 1,
 					.altset_idx = 1,
 					.endpoint = 0x82,
+					.ep_idx = 1,
 					.ep_attr = USB_ENDPOINT_XFER_ISOC|
 						USB_ENDPOINT_SYNC_ASYNC|
 					USB_ENDPOINT_USAGE_IMPLICIT_FB,
@@ -3679,6 +3684,7 @@ AU0828_DEVICE(0x2040, 0x7270, "Hauppauge", "HVR-950Q"),
 					.altsetting = 1,
 					.altset_idx = 1,
 					.endpoint = 0x82,
+					.ep_idx = 1,
 					.ep_attr = USB_ENDPOINT_XFER_ISOC|
 					    USB_ENDPOINT_SYNC_ASYNC|
 					    USB_ENDPOINT_USAGE_IMPLICIT_FB,
diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c
index b70e2ebc3e29..89e172642d98 100644
--- a/sound/usb/quirks.c
+++ b/sound/usb/quirks.c
@@ -194,7 +194,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
 	}
 	alts = &iface->altsetting[fp->altset_idx];
 	altsd = get_iface_desc(alts);
-	if (altsd->bNumEndpoints < 1) {
+	if (altsd->bNumEndpoints <= fp->ep_idx) {
 		err = -EINVAL;
 		goto error;
 	}
@@ -204,7 +204,7 @@ static int create_fixed_stream_quirk(struct snd_usb_audio *chip,
 	if (fp->datainterval == 0)
 		fp->datainterval = snd_usb_parse_datainterval(chip, alts);
 	if (fp->maxpacksize == 0)
-		fp->maxpacksize = le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize);
+		fp->maxpacksize = le16_to_cpu(get_endpoint(alts, fp->ep_idx)->wMaxPacketSize);
 	if (!fp->fmt_type)
 		fp->fmt_type = UAC_FORMAT_TYPE_I;
 
-- 
2.26.2

[PATCH v5 5/5] ALSA: usb-audio: Fix implicit feedback sync setup for Pioneer devices

[Takashi Iwai]

Pioneer devices have both playback and capture streams sharing the
same iface/altsetting, and those need to be paired as implicit
feedback.  Instead of a half-baked (and broken) static quirk entry,
set up more generically for those devices by checking the number of
endpoints and the attribute of the secondary EP.

Fixes: bf6313a0ff76 ("ALSA: usb-audio: Refactor endpoint management")
Reported-by: František Kučera <konference@frantovo.cz>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/implicit.c | 48 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/sound/usb/implicit.c b/sound/usb/implicit.c
index 9724efe1cdce..1ac2cc6c33fb 100644
--- a/sound/usb/implicit.c
+++ b/sound/usb/implicit.c
@@ -58,8 +58,6 @@ static const struct snd_usb_implicit_fb_match playback_implicit_fb_quirks[] = {
 	IMPLICIT_FB_FIXED_DEV(0x0499, 0x172f, 0x81, 2), /* Steinberg UR22C */
 	IMPLICIT_FB_FIXED_DEV(0x0d9a, 0x00df, 0x81, 2), /* RTX6001 */
 	IMPLICIT_FB_FIXED_DEV(0x22f0, 0x0006, 0x81, 3), /* Allen&Heath Qu-16 */
-	IMPLICIT_FB_FIXED_DEV(0x2b73, 0x000a, 0x82, 0), /* Pioneer DJ DJM-900NXS2 */
-	IMPLICIT_FB_FIXED_DEV(0x2b73, 0x0017, 0x82, 0), /* Pioneer DJ DJM-250MK2 */
 	IMPLICIT_FB_FIXED_DEV(0x1686, 0xf029, 0x82, 2), /* Zoom UAC-2 */
 	IMPLICIT_FB_FIXED_DEV(0x2466, 0x8003, 0x86, 2), /* Fractal Audio Axe-Fx II */
 	IMPLICIT_FB_FIXED_DEV(0x0499, 0x172a, 0x86, 2), /* Yamaha MODX */
@@ -100,7 +98,7 @@ static const struct snd_usb_implicit_fb_match capture_implicit_fb_quirks[] = {
 /* set up sync EP information on the audioformat */
 static int add_implicit_fb_sync_ep(struct snd_usb_audio *chip,
 				   struct audioformat *fmt,
-				   int ep, int ifnum,
+				   int ep, int ep_idx, int ifnum,
 				   const struct usb_host_interface *alts)
 {
 	struct usb_interface *iface;
@@ -115,7 +113,7 @@ static int add_implicit_fb_sync_ep(struct snd_usb_audio *chip,
 	fmt->sync_ep = ep;
 	fmt->sync_iface = ifnum;
 	fmt->sync_altsetting = alts->desc.bAlternateSetting;
-	fmt->sync_ep_idx = 0;
+	fmt->sync_ep_idx = ep_idx;
 	fmt->implicit_fb = 1;
 	usb_audio_dbg(chip,
 		      "%d:%d: added %s implicit_fb sync_ep %x, iface %d:%d\n",
@@ -147,7 +145,7 @@ static int add_generic_uac2_implicit_fb(struct snd_usb_audio *chip,
 	    (epd->bmAttributes & USB_ENDPOINT_USAGE_MASK) !=
 					USB_ENDPOINT_USAGE_IMPLICIT_FB)
 		return 0;
-	return add_implicit_fb_sync_ep(chip, fmt, epd->bEndpointAddress,
+	return add_implicit_fb_sync_ep(chip, fmt, epd->bEndpointAddress, 0,
 				       ifnum, alts);
 }
 
@@ -173,10 +171,32 @@ static int add_roland_implicit_fb(struct snd_usb_audio *chip,
 	    (epd->bmAttributes & USB_ENDPOINT_USAGE_MASK) !=
 					USB_ENDPOINT_USAGE_IMPLICIT_FB)
 		return 0;
-	return add_implicit_fb_sync_ep(chip, fmt, epd->bEndpointAddress,
+	return add_implicit_fb_sync_ep(chip, fmt, epd->bEndpointAddress, 0,
 				       ifnum, alts);
 }
 
+/* Pioneer devices: playback and capture streams sharing the same iface/altset
+ */
+static int add_pioneer_implicit_fb(struct snd_usb_audio *chip,
+				   struct audioformat *fmt,
+				   struct usb_host_interface *alts)
+{
+	struct usb_endpoint_descriptor *epd;
+
+	if (alts->desc.bNumEndpoints != 2)
+		return 0;
+
+	epd = get_endpoint(alts, 1);
+	if (!usb_endpoint_is_isoc_in(epd) ||
+	    (epd->bmAttributes & USB_ENDPOINT_SYNCTYPE) != USB_ENDPOINT_SYNC_ASYNC ||
+	    ((epd->bmAttributes & USB_ENDPOINT_USAGE_MASK) !=
+	     USB_ENDPOINT_USAGE_DATA &&
+	     (epd->bmAttributes & USB_ENDPOINT_USAGE_MASK) !=
+	     USB_ENDPOINT_USAGE_IMPLICIT_FB))
+		return 0;
+	return add_implicit_fb_sync_ep(chip, fmt, epd->bEndpointAddress, 1,
+				       alts->desc.bInterfaceNumber, alts);
+}
 
 static int __add_generic_implicit_fb(struct snd_usb_audio *chip,
 				     struct audioformat *fmt,
@@ -197,7 +217,7 @@ static int __add_generic_implicit_fb(struct snd_usb_audio *chip,
 	if (!usb_endpoint_is_isoc_in(epd) ||
 	    (epd->bmAttributes & USB_ENDPOINT_SYNCTYPE) != USB_ENDPOINT_SYNC_ASYNC)
 		return 0;
-	return add_implicit_fb_sync_ep(chip, fmt, epd->bEndpointAddress,
+	return add_implicit_fb_sync_ep(chip, fmt, epd->bEndpointAddress, 0,
 				       iface, alts);
 }
 
@@ -250,7 +270,7 @@ static int audioformat_implicit_fb_quirk(struct snd_usb_audio *chip,
 		case IMPLICIT_FB_NONE:
 			return 0; /* No quirk */
 		case IMPLICIT_FB_FIXED:
-			return add_implicit_fb_sync_ep(chip, fmt, p->ep_num,
+			return add_implicit_fb_sync_ep(chip, fmt, p->ep_num, 0,
 						       p->iface, NULL);
 		}
 	}
@@ -278,6 +298,14 @@ static int audioformat_implicit_fb_quirk(struct snd_usb_audio *chip,
 			return 1;
 	}
 
+	/* Pioneer devices implicit feedback with vendor spec class */
+	if (attr == USB_ENDPOINT_SYNC_ASYNC &&
+	    alts->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC &&
+	    USB_ID_VENDOR(chip->usb_id) == 0x2b73 /* Pioneer */) {
+		if (add_pioneer_implicit_fb(chip, fmt, alts))
+			return 1;
+	}
+
 	/* Try the generic implicit fb if available */
 	if (chip->generic_implicit_fb)
 		return add_generic_implicit_fb(chip, fmt, alts);
@@ -295,8 +323,8 @@ static int audioformat_capture_quirk(struct snd_usb_audio *chip,
 
 	p = find_implicit_fb_entry(chip, capture_implicit_fb_quirks, alts);
 	if (p && p->type == IMPLICIT_FB_FIXED)
-		return add_implicit_fb_sync_ep(chip, fmt, p->ep_num, p->iface,
-					       NULL);
+		return add_implicit_fb_sync_ep(chip, fmt, p->ep_num, 0,
+					       p->iface, NULL);
 	return 0;
 }
 
-- 
2.26.2