[PATCH Fix for Kingston HyperX Amp (0951:16d8) 1/1] snd-usb-audio: Fix Kingston HyperX Amp (0951:16d8).

[Takashi Iwai]

On Sat, 07 Mar 2020 19:57:41 +0100,
crwulff at gmail.com wrote:
> 
> From: Chris Wulff <crwulff at 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 at 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