| Message ID | 20200307185741.32578-2-crwulff@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | snd-usb-audio: Fix Kingston HyperX Amp (0951:16d8). | expand |
On Sat, 07 Mar 2020 19:57:41 +0100, crwulff@gmail.com wrote: > > From: Chris Wulff <crwulff@gmail.com> > > The newer version of the HyperX Amp has controls on two > separate USB interfaces (0 and 2.) > > This patch fixes the use of two separate usb interfaces > for controls and audio by using the controls mixer interface > instead of the card interface for each control and by not > merging new streams with a pcm device that has already > been registered. > > Signed-off-by: Chris Wulff <crwulff@gmail.com> First off, thanks for the patch and the detailed analysis. You had a nice description in the cover letter (patch 0), and it could be added here as well. Also, the patch can be split to two: one for changing the mixer interface reference and another for avoiding the addition of the substream to the already registered device. Now about the code changes: > diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c > index 45eee5cc312e..2498107ca89f 100644 > --- a/sound/usb/mixer.c > +++ b/sound/usb/mixer.c > @@ -306,7 +306,7 @@ static int get_ctl_value_v1(struct usb_mixer_elem_info *cval, int request, > return -EIO; > > while (timeout-- > 0) { > - idx = snd_usb_ctrl_intf(chip) | (cval->head.id << 8); > + idx = cval->head.mixer->hostif->desc.bInterfaceNumber | (cval->head.id << 8); This and the whole other conversion could be a bit nicer if we introduce a helper macro / inline function instead of the open coding, e.g. idx = mixer_host_intf(cval->head.mixer) | (cval->head.id << 8); ... and static inline int mixer_host_intf(struct usb_mixer_interface *mixer) { return get_iface_desc(mixer->hostif)->bInterfaceNumber; } Those changes are drastic, hence the fix will be likely queued at first as post 5.6 change, i.e. it'll be merged to 5.7 kernel. > diff --git a/sound/usb/stream.c b/sound/usb/stream.c > index 11785f9652ad..d286c18f8d43 100644 > --- a/sound/usb/stream.c > +++ b/sound/usb/stream.c > @@ -482,6 +482,7 @@ static int __snd_usb_add_audio_stream(struct snd_usb_audio *chip, > struct snd_usb_stream *as; > struct snd_usb_substream *subs; > struct snd_pcm *pcm; > + struct snd_device *dev; > int err; > > list_for_each_entry(as, &chip->pcm_list, list) { > @@ -502,6 +503,13 @@ static int __snd_usb_add_audio_stream(struct snd_usb_audio *chip, > subs = &as->substream[stream]; > if (subs->ep_num) > continue; > + > + list_for_each_entry(dev, &as->pcm->card->devices, list) > + if (dev->device_data == as->pcm) > + break; > + if (dev && (dev->state == SNDRV_DEV_REGISTERED)) > + continue; > + It's a bit too hackish, and it's unfortunate that we have no way to identify whether the device got already registered or not in the current code. And, as you stated in the cover letter, this would lead some confusion to PulseAudio. I guess we can still merge the substreams into the single device if the card registration is delayed. That is, introduce a check before snd_card_register() call in usb_audio_probe(); if the probed interface matches with a blacklist (e.g. the combo of USB-device matching and the interface number matching), it skips the snd_card_register() call and lets the next interface doing it. In addition to that, we can put the workaround for the avoidance of the already registered device as you did in the above but in a slightly different form, too. For example, we can introduce at first snd_pcm.registered flag that is set in PCM dev_register ops callback. Or we put a struct snd_device reverse mapping there so that you can check the device state, too. When we add the workaround, it should also print some info or error message, too. Basically it's a racy action and it can be handled better with the delayed snd_card_register(), and the message indicates the needed change. thanks, Takashi
diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c index 45eee5cc312e..2498107ca89f 100644 --- a/sound/usb/mixer.c +++ b/sound/usb/mixer.c @@ -306,7 +306,7 @@ static int get_ctl_value_v1(struct usb_mixer_elem_info *cval, int request, return -EIO; while (timeout-- > 0) { - idx = snd_usb_ctrl_intf(chip) | (cval->head.id << 8); + idx = cval->head.mixer->hostif->desc.bInterfaceNumber | (cval->head.id << 8); err = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), request, USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN, validx, idx, buf, val_len); @@ -354,7 +354,7 @@ static int get_ctl_value_v2(struct usb_mixer_elem_info *cval, int request, if (ret) goto error; - idx = snd_usb_ctrl_intf(chip) | (cval->head.id << 8); + idx = cval->head.mixer->hostif->desc.bInterfaceNumber | (cval->head.id << 8); ret = snd_usb_ctl_msg(chip->dev, usb_rcvctrlpipe(chip->dev, 0), bRequest, USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN, validx, idx, buf, size); @@ -479,7 +479,7 @@ int snd_usb_mixer_set_ctl_value(struct usb_mixer_elem_info *cval, return -EIO; while (timeout-- > 0) { - idx = snd_usb_ctrl_intf(chip) | (cval->head.id << 8); + idx = cval->head.mixer->hostif->desc.bInterfaceNumber | (cval->head.id << 8); err = snd_usb_ctl_msg(chip->dev, usb_sndctrlpipe(chip->dev, 0), request, USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_OUT, @@ -1195,7 +1195,7 @@ static int get_min_max_with_quirks(struct usb_mixer_elem_info *cval, get_ctl_value(cval, UAC_GET_MIN, (cval->control << 8) | minchn, &cval->min) < 0) { usb_audio_err(cval->head.mixer->chip, "%d:%d: cannot get min/max values for control %d (id %d)\n", - cval->head.id, snd_usb_ctrl_intf(cval->head.mixer->chip), + cval->head.id, cval->head.mixer->hostif->desc.bInterfaceNumber, cval->control, cval->head.id); return -EINVAL; } @@ -1414,7 +1414,7 @@ static int mixer_ctl_connector_get(struct snd_kcontrol *kcontrol, if (ret) goto error; - idx = snd_usb_ctrl_intf(chip) | (cval->head.id << 8); + idx = cval->head.mixer->hostif->desc.bInterfaceNumber | (cval->head.id << 8); if (cval->head.mixer->protocol == UAC_VERSION_2) { struct uac2_connectors_ctl_blk uac2_conn; @@ -3192,7 +3192,7 @@ static void snd_usb_mixer_proc_read(struct snd_info_entry *entry, list_for_each_entry(mixer, &chip->mixer_list, list) { snd_iprintf(buffer, "USB Mixer: usb_id=0x%08x, ctrlif=%i, ctlerr=%i\n", - chip->usb_id, snd_usb_ctrl_intf(chip), + chip->usb_id, mixer->hostif->desc.bInterfaceNumber, mixer->ignore_ctl_error); snd_iprintf(buffer, "Card: %s\n", chip->card->longname); for (unitid = 0; unitid < MAX_ID_ELEMS; unitid++) { diff --git a/sound/usb/stream.c b/sound/usb/stream.c index 11785f9652ad..d286c18f8d43 100644 --- a/sound/usb/stream.c +++ b/sound/usb/stream.c @@ -482,6 +482,7 @@ static int __snd_usb_add_audio_stream(struct snd_usb_audio *chip, struct snd_usb_stream *as; struct snd_usb_substream *subs; struct snd_pcm *pcm; + struct snd_device *dev; int err; list_for_each_entry(as, &chip->pcm_list, list) { @@ -502,6 +503,13 @@ static int __snd_usb_add_audio_stream(struct snd_usb_audio *chip, subs = &as->substream[stream]; if (subs->ep_num) continue; + + list_for_each_entry(dev, &as->pcm->card->devices, list) + if (dev->device_data == as->pcm) + break; + if (dev && (dev->state == SNDRV_DEV_REGISTERED)) + continue; + err = snd_pcm_new_stream(as->pcm, stream, 1); if (err < 0) return err;