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