[PATCH] Add duplex sound support for USB devices using implicit feedback

[Takashi Iwai]

On Tue, 26 May 2020 23:04:44 +0200,
Erwin Burema wrote:
> 
> On zondag 24 mei 2020 10:37:34 CEST Takashi Iwai wrote:
> > On Sat, 23 May 2020 20:09:31 +0200,
> > 
> > Erwin Burema wrote:
> > > On zaterdag 23 mei 2020 19:53:49 CEST Alexander Tsoy wrote:
> > > > В Вс, 10/05/2020 в 20:29 +0200, Erwin Burema пишет:
> > > > > For USB sound devices using implicit feedback the endpoint used for
> > > > > this feedback should be able to be opened twice, once for required
> > > > > feedback and second time for audio data. This way these devices can
> > > > > be put in duplex audio mode. Since this only works if the settings of
> > > > > the endpoint don't change a check is included for this.
> > > > > 
> > > > > This fixes bug 207023 ("MOTU M2 regression on duplex audio") and
> > > > > should also fix bug 103751 ("M-Audio Fast Track Ultra usb audio
> > > > > device will not operate full-duplex")
> > > > > 
> > > > > Signed-off-by: Erwin Burema <e.burema at gmail.com>
> > > > > ---
> > > > 
> > > > This patch seems to cause kernel panic on my system. This happens
> > > > during boot when gdm (with pulseaudio) is starting up.
> > > 
> > > That's interesting, not running gnome (and thus also not running gdm,
> > > currently KDE with SDDM) here so would need to take some time
> > > troubleshooting. Suspect I missed something in the check if both input
> > > and output are similarly configured.
> > > 
> > > Will see if I can reproduce it and where exactly it goes wrong, in the
> > > meantime would it be possible to test if 5.6 or later show the same issue
> > > since I intially developed the patch against that release?
> > 
> > Judging from the point triggering the bug (memset()), this can be a
> > leftover URB handling that is performed even after the capture stream
> > is closed.  If so, the procedure would be:
> >  - start capture
> >  - start playback
> >  - stop capture while keeping playback running
> > 
> > If my guess is correct, can the patch below work around the issue?
> > 
> 
> Have spend a large part of my free time trying to replicate it without any 
> luck, might be due to tryin it in a VM with USB passtrough (but wanted to be 
> able to quickly itterate and easier to get debug info), so haven't been able 
> to try out the patch.
> 
> Next step is to see if I can replicate it on my HW, if that doesn't work it 
> might be time to rethink this whole initial patch and might need to do 
> something at substream level instead of endpoint level.

I believe the check for EP is right.  The original code has already
the EP refcount in EP management, so such a linked use case should
have been considered.

Let's see whether Alexander can test the patch.


thanks,

Takashi

> 
> Regards,
> 
> Erwin Burema
> 
> > 
> > thanks,
> > 
> > Takashi
> > 
> > ---
> > --- a/sound/usb/pcm.c
> > +++ b/sound/usb/pcm.c
> > @@ -1782,6 +1782,7 @@ static int snd_usb_substream_capture_trigger(struct
> > snd_pcm_substream *substream return 0;
> >  	case SNDRV_PCM_TRIGGER_STOP:
> >  		stop_endpoints(subs);
> > +		subs->data_endpoint->retire_data_urb = NULL;
> >  		subs->running = 0;
> >  		return 0;
> >  	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> 
> 
> 
>