Re: [PATCH v4 09/15] ALSA: usb-audio: conditional interface altsetting

[Eldad Zack <eldad@fogrefinery.com>]

On Mon, 7 Oct 2013, Takashi Iwai wrote:

> At Sun,  6 Oct 2013 22:31:14 +0200,
> Eldad Zack wrote:
> > 
> > On some devices, if the endpoint for the other direction is in use,
> > setting one interface will shutdown the other (in use) endpoint.
> > This patch moves all of the alternate setting operations for pcm
> > ops to one function which checks if we can do so.
> > 
> > If current alternate is 0, it is safe to set.
> 
> Is this check needed for all devices?

I'm not sure, I only have this one device to test with.
I asked once on the list if this behavior (when the alts is set to 0,
the endpoint stops if it is running) or does it only happen on implicit
feedback, but got no response.

> The combination of playback and capture streams in snd_usb_substream
> is casual, just depending on the enumeration.  It doesn't always mean
> that these are coupled.  For non-implicit-fb devices, won't this
> result in a false positive?

The worst that can happen is that the altsetting will not go back to 0, 
if the other substream is in use. Is that an issue?

> 
> Also...
> 
> > +static int subs_set_interface(struct snd_usb_substream *subs, int ifnum,
> > +			      int altsetting)
> > +{
> > +	struct snd_usb_substream *other_subs;
> > +	int err;
> > +
> > +	snd_printdd(KERN_DEBUG "%s: Requested set interface %d alts %d\n",
> > +		    __func__, ifnum, altsetting);
> > +
> > +	if (subs == NULL)
> > +		return usb_set_interface(subs->dev, ifnum, altsetting);
> 
> It must lead to Oops.

Yes, that's bad code. :/
Thank you for pointing it out!

Although it won't get triggered, since it won't get any NULL subs from 
any callsite as far as I see. I'll remove this and add WARN_ON(!subs).

Cheers,
Eldad