diff mbox series

Add duplex sound support for USB devices using implicit feedback

Message ID 2410739.SCZni40SNb@alpha-wolf (mailing list archive)
State New, archived
Headers show
Series Add duplex sound support for USB devices using implicit feedback | expand

Commit Message

Erwin Burema May 10, 2020, 6:29 p.m. UTC
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@gmail.com>
---
 sound/usb/card.h     |   1 +
 sound/usb/endpoint.c | 195 ++++++++++++++++++++++++++++++++++++++++++-
 sound/usb/pcm.c      |   5 ++
 3 files changed, 197 insertions(+), 4 deletions(-)

Comments

Takashi Iwai May 15, 2020, 5:13 p.m. UTC | #1
On Sun, 10 May 2020 20:29:11 +0200,
Erwin Burema wrote:
> 
> 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@gmail.com>

Applied now to for-next branch.  Thanks!


Takashi
Alexander Tsoy May 23, 2020, 5:53 p.m. UTC | #2
В Вс, 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@gmail.com>
> ---

This patch seems to cause kernel panic on my system. This happens
during boot when gdm (with pulseaudio) is starting up.

$ grep CONFIG_IRQ_FORCED_THREADING /boot/config-5.4.42-gentoo 
CONFIG_IRQ_FORCED_THREADING=y

$ grep threadirqs /proc/cmdline 
BOOT_IMAGE=/vmlinuz-5.4.42-gentoo root=/dev/mapper/vg_system-root ro rd.md.uuid=cdf11511:cf0ca8c5:cc165dc3:3d3d248f rd.luks.uuid=a5a6e532-af4e-49b2-8178-95e54c293799 rd.lvm.lv=vg_system/root rd.lvm.lv=vg_system/swap rd.lvm.lv=vg_system/usr resume=/dev/mapper/vg_system-swap rootflags=relatime,logbufs=8,logbsize=256k rootfstype=xfs init=/lib/systemd/systemd noautogroup loglevel=5 console=ttyS1,115200 console=tty0 threadirqs mgag200.modeset=1

[   60.563598] BUG: unable to handle page fault for address: ffffb80602983ff0
[   60.570478] #PF: supervisor write access in kernel mode
[   60.575701] #PF: error_code(0x0002) - not-present page
[   60.580833] PGD 813498067 P4D 813498067 PUD 813499067 PMD 80e0e0067 PTE 0
[   60.587619] Oops: 0002 [#1] PREEMPT SMP NOPTI
[   60.591979] CPU: 4 PID: 242 Comm: irq/34-xhci_hcd Tainted: G           O    T 5.4.42-gentoo #1
[   60.600585] Hardware name: Supermicro H8SCM/H8SCM, BIOS 3.5b       03/18/2016
[   60.607723] RIP: 0010:__memcpy+0x12/0x20
[   60.611646] Code: c1 e2 20 48 09 c2 48 31 d3 e9 71 ff ff ff 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 <f3> 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 f3 a4
[   60.630401] RSP: 0018:ffffb806004a3c80 EFLAGS: 00010046
[   60.635628] RAX: ffffb80602983ff0 RBX: 0000000000000010 RCX: 000000000000000a
[   60.642759] RDX: 0000000000000000 RSI: ffff9d5f6659b000 RDI: ffffb80602983ff0
[   60.649883] RBP: ffff9d5f62c38c00 R08: 0000000000000002 R09: 0000000000000000
[   60.657017] R10: 0000000000000002 R11: 0000000000000001 R12: 0000000000000050
[   60.664160] R13: 0000000000000000 R14: 0000000000079ff0 R15: ffff9d608c1d4908
[   60.671293] FS:  0000000000000000(0000) GS:ffff9d6096b00000(0000) knlGS:0000000000000000
[   60.679380] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   60.685125] CR2: ffffb80602983ff0 CR3: 000000080f55c000 CR4: 00000000000406e0
[   60.692258] Call Trace:
[   60.694719]  retire_capture_urb+0x201/0x270 [snd_usb_audio]
[   60.700288]  snd_complete_urb+0x1b7/0x270 [snd_usb_audio]
[   60.705686]  __usb_hcd_giveback_urb+0x77/0xe0
[   60.710054]  xhci_giveback_urb_in_irq.isra.0+0x6d/0x100
[   60.715278]  xhci_td_cleanup+0xc4/0xe0
[   60.719032]  xhci_irq+0x825/0x1780
[   60.722438]  ? __schedule+0x2c3/0x650
[   60.726105]  ? irq_finalize_oneshot.part.0+0xd0/0xd0
[   60.731071]  irq_forced_thread_fn+0x25/0x70
[   60.735257]  irq_thread+0xea/0x170
[   60.738672]  ? wake_threads_waitq+0x30/0x30
[   60.742859]  kthread+0xf8/0x130
[   60.746005]  ? irq_thread_check_affinity+0x90/0x90
[   60.750805]  ? kthread_park+0x90/0x90
[   60.754473]  ret_from_fork+0x22/0x40
[   60.758053] Modules linked in: ebtable_filter ebtables bridge stp llc netconsole wireguard ip6_udp_tunnel udp_tunnel xt_limit xt_comment xt_recent xt_multiport xt_MASQUERADE iptable_nat nf_nat nf_conntrack_netbios_ns nf_conntrack_broadcast xt_CT ip6table_raw iptable_raw snd_seq_midi snd_seq_midi_event amd64_edac_mod kvm_amd kvm pcspkr irqbypass uvcvideo videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 videodev snd_hda_codec_hdmi videobuf2_common snd_hda_intel joydev snd_intel_nhlt snd_hda_codec snd_usb_audio sp5100_tco snd_usbmidi_lib snd_rawmidi snd_hwdep e1000e snd_hda_core snd_pcm nfsd tcp_yeah tcp_vegas sch_fq_codel uas amdgpu crc32c_intel mgag200 drm_vram_helper gpu_sched ttm vhost_scsi target_core_mod configfs vhost_net tun tap vhost vhba(O) jc42 ipmi_si ipmi_devintf ipmi_msghandler fuse eeprom
[   60.828924] CR2: ffffb80602983ff0
[   60.832245] ---[ end trace 1945ea8b50798a03 ]---
[   60.836862] RIP: 0010:__memcpy+0x12/0x20
[   60.840797] Code: c1 e2 20 48 09 c2 48 31 d3 e9 71 ff ff ff 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 <f3> 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 f3 a4
[   60.859553] RSP: 0018:ffffb806004a3c80 EFLAGS: 00010046
[   60.864778] RAX: ffffb80602983ff0 RBX: 0000000000000010 RCX: 000000000000000a
[   60.871911] RDX: 0000000000000000 RSI: ffff9d5f6659b000 RDI: ffffb80602983ff0
[   60.879036] RBP: ffff9d5f62c38c00 R08: 0000000000000002 R09: 0000000000000000
[   60.886168] R10: 0000000000000002 R11: 0000000000000001 R12: 0000000000000050
[   60.893301] R13: 0000000000000000 R14: 0000000000079ff0 R15: ffff9d608c1d4908
[   60.900453] FS:  0000000000000000(0000) GS:ffff9d6096b00000(0000) knlGS:0000000000000000
[   60.908538] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   60.914276] CR2: ffffb80602983ff0 CR3: 000000080f55c000 CR4: 00000000000406e0
[   60.921411] Kernel panic - not syncing: Fatal exception in interrupt
[   60.927783] Kernel Offset: 0x21000000 from 0xffffffff81000000 (relocation range: 0xffffffff80000000-0xffffffffbfffffff)
[   60.938554] ---[ end Kernel panic - not syncing: Fatal exception in interrupt ]---
Erwin Burema May 23, 2020, 6:09 p.m. UTC | #3
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@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?

> $ grep CONFIG_IRQ_FORCED_THREADING /boot/config-5.4.42-gentoo
> CONFIG_IRQ_FORCED_THREADING=y
> 
> $ grep threadirqs /proc/cmdline
> BOOT_IMAGE=/vmlinuz-5.4.42-gentoo root=/dev/mapper/vg_system-root ro
> rd.md.uuid=cdf11511:cf0ca8c5:cc165dc3:3d3d248f
> rd.luks.uuid=a5a6e532-af4e-49b2-8178-95e54c293799 rd.lvm.lv=vg_system/root
> rd.lvm.lv=vg_system/swap rd.lvm.lv=vg_system/usr
> resume=/dev/mapper/vg_system-swap
> rootflags=relatime,logbufs=8,logbsize=256k rootfstype=xfs
> init=/lib/systemd/systemd noautogroup loglevel=5 console=ttyS1,115200
> console=tty0 threadirqs mgag200.modeset=1
> 
> [   60.563598] BUG: unable to handle page fault for address:
> ffffb80602983ff0 [   60.570478] #PF: supervisor write access in kernel mode
> [   60.575701] #PF: error_code(0x0002) - not-present page
> [   60.580833] PGD 813498067 P4D 813498067 PUD 813499067 PMD 80e0e0067 PTE 0
> [   60.587619] Oops: 0002 [#1] PREEMPT SMP NOPTI
> [   60.591979] CPU: 4 PID: 242 Comm: irq/34-xhci_hcd Tainted: G           O 
>   T 5.4.42-gentoo #1 [   60.600585] Hardware name: Supermicro H8SCM/H8SCM,
> BIOS 3.5b       03/18/2016 [   60.607723] RIP: 0010:__memcpy+0x12/0x20
> [   60.611646] Code: c1 e2 20 48 09 c2 48 31 d3 e9 71 ff ff ff 90 90 90 90
> 90 90 90 90 90 90 0f 1f 44 00 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07
> <f3> 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 f3 a4 [  
> 60.630401] RSP: 0018:ffffb806004a3c80 EFLAGS: 00010046
> [   60.635628] RAX: ffffb80602983ff0 RBX: 0000000000000010 RCX:
> 000000000000000a [   60.642759] RDX: 0000000000000000 RSI: ffff9d5f6659b000
> RDI: ffffb80602983ff0 [   60.649883] RBP: ffff9d5f62c38c00 R08:
> 0000000000000002 R09: 0000000000000000 [   60.657017] R10: 0000000000000002
> R11: 0000000000000001 R12: 0000000000000050 [   60.664160] R13:
> 0000000000000000 R14: 0000000000079ff0 R15: ffff9d608c1d4908 [   60.671293]
> FS:  0000000000000000(0000) GS:ffff9d6096b00000(0000)
> knlGS:0000000000000000 [   60.679380] CS:  0010 DS: 0000 ES: 0000 CR0:
> 0000000080050033
> [   60.685125] CR2: ffffb80602983ff0 CR3: 000000080f55c000 CR4:
> 00000000000406e0 [   60.692258] Call Trace:
> [   60.694719]  retire_capture_urb+0x201/0x270 [snd_usb_audio]
> [   60.700288]  snd_complete_urb+0x1b7/0x270 [snd_usb_audio]
> [   60.705686]  __usb_hcd_giveback_urb+0x77/0xe0
> [   60.710054]  xhci_giveback_urb_in_irq.isra.0+0x6d/0x100
> [   60.715278]  xhci_td_cleanup+0xc4/0xe0
> [   60.719032]  xhci_irq+0x825/0x1780
> [   60.722438]  ? __schedule+0x2c3/0x650
> [   60.726105]  ? irq_finalize_oneshot.part.0+0xd0/0xd0
> [   60.731071]  irq_forced_thread_fn+0x25/0x70
> [   60.735257]  irq_thread+0xea/0x170
> [   60.738672]  ? wake_threads_waitq+0x30/0x30
> [   60.742859]  kthread+0xf8/0x130
> [   60.746005]  ? irq_thread_check_affinity+0x90/0x90
> [   60.750805]  ? kthread_park+0x90/0x90
> [   60.754473]  ret_from_fork+0x22/0x40
> [   60.758053] Modules linked in: ebtable_filter ebtables bridge stp llc
> netconsole wireguard ip6_udp_tunnel udp_tunnel xt_limit xt_comment
> xt_recent xt_multiport xt_MASQUERADE iptable_nat nf_nat
> nf_conntrack_netbios_ns nf_conntrack_broadcast xt_CT ip6table_raw
> iptable_raw snd_seq_midi snd_seq_midi_event amd64_edac_mod kvm_amd kvm
> pcspkr irqbypass uvcvideo videobuf2_vmalloc videobuf2_memops videobuf2_v4l2
> videodev snd_hda_codec_hdmi videobuf2_common snd_hda_intel joydev
> snd_intel_nhlt snd_hda_codec snd_usb_audio sp5100_tco snd_usbmidi_lib
> snd_rawmidi snd_hwdep e1000e snd_hda_core snd_pcm nfsd tcp_yeah tcp_vegas
> sch_fq_codel uas amdgpu crc32c_intel mgag200 drm_vram_helper gpu_sched ttm
> vhost_scsi target_core_mod configfs vhost_net tun tap vhost vhba(O) jc42
> ipmi_si ipmi_devintf ipmi_msghandler fuse eeprom [   60.828924] CR2:
> ffffb80602983ff0
> [   60.832245] ---[ end trace 1945ea8b50798a03 ]---
> [   60.836862] RIP: 0010:__memcpy+0x12/0x20
> [   60.840797] Code: c1 e2 20 48 09 c2 48 31 d3 e9 71 ff ff ff 90 90 90 90
> 90 90 90 90 90 90 0f 1f 44 00 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07
> <f3> 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 f3 a4 [  
> 60.859553] RSP: 0018:ffffb806004a3c80 EFLAGS: 00010046
> [   60.864778] RAX: ffffb80602983ff0 RBX: 0000000000000010 RCX:
> 000000000000000a [   60.871911] RDX: 0000000000000000 RSI: ffff9d5f6659b000
> RDI: ffffb80602983ff0 [   60.879036] RBP: ffff9d5f62c38c00 R08:
> 0000000000000002 R09: 0000000000000000 [   60.886168] R10: 0000000000000002
> R11: 0000000000000001 R12: 0000000000000050 [   60.893301] R13:
> 0000000000000000 R14: 0000000000079ff0 R15: ffff9d608c1d4908 [   60.900453]
> FS:  0000000000000000(0000) GS:ffff9d6096b00000(0000)
> knlGS:0000000000000000 [   60.908538] CS:  0010 DS: 0000 ES: 0000 CR0:
> 0000000080050033
> [   60.914276] CR2: ffffb80602983ff0 CR3: 000000080f55c000 CR4:
> 00000000000406e0 [   60.921411] Kernel panic - not syncing: Fatal exception
> in interrupt [   60.927783] Kernel Offset: 0x21000000 from
> 0xffffffff81000000 (relocation range:
> 0xffffffff80000000-0xffffffffbfffffff) [   60.938554] ---[ end Kernel panic
> - not syncing: Fatal exception in interrupt ]---
Takashi Iwai May 24, 2020, 8:37 a.m. UTC | #4
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@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?


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:
Erwin Burema May 26, 2020, 9:04 p.m. UTC | #5
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@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.

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:
Takashi Iwai May 27, 2020, 7:06 a.m. UTC | #6
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@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:
> 
> 
> 
>
Alexander Tsoy May 28, 2020, 1:11 p.m. UTC | #7
В Ср, 27/05/2020 в 09:06 +0200, Takashi Iwai пишет:
> ....
> 
> Let's see whether Alexander can test the patch.

Sorry for the late reply. I will try your patch this weekend.
Alexander Tsoy June 1, 2020, 4:37 p.m. UTC | #8
В Вс, 24/05/2020 в 10:37 +0200, Takashi Iwai пишет:
> 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@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?

Unfortunately, I can no longer reproduce kernel panic, so can't really
test this patch. That's interesting, because it was 100% reproducible
on my hw a week ago.
Takashi Iwai June 2, 2020, 3:15 p.m. UTC | #9
On Mon, 01 Jun 2020 18:37:38 +0200,
Alexander Tsoy wrote:
> 
> В Вс, 24/05/2020 в 10:37 +0200, Takashi Iwai пишет:
> > 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@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?
> 
> Unfortunately, I can no longer reproduce kernel panic, so can't really
> test this patch. That's interesting, because it was 100% reproducible
> on my hw a week ago.

It's a pity, but interesting.  If so, it might be different from what
I've though in the above, and we might need a different fix...

Let me know if you can trigger the bug more or less reliably.


thanks,

Takashi
diff mbox series

Patch

diff --git a/sound/usb/card.h b/sound/usb/card.h
index 820e564656ed..d6219fba9699 100644
--- a/sound/usb/card.h
+++ b/sound/usb/card.h
@@ -108,6 +108,7 @@  struct snd_usb_endpoint {
 	int iface, altsetting;
 	int skip_packets;		/* quirks for devices to ignore the first n packets
 					   in a stream */
+	bool is_implicit_feedback;      /* This endpoint is used as implicit feedback */
 
 	spinlock_t lock;
 	struct list_head list;
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 50104f658ed4..9bea7d3f99f8 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -522,6 +522,8 @@  struct snd_usb_endpoint *snd_usb_add_endpoint(struct snd_usb_audio *chip,
 
 	list_add_tail(&ep->list, &chip->ep_list);
 
+	ep->is_implicit_feedback = 0;
+
 __exit_unlock:
 	mutex_unlock(&chip->mutex);
 
@@ -621,6 +623,178 @@  static void release_urbs(struct snd_usb_endpoint *ep, int force)
 	ep->nurbs = 0;
 }
 
+/*
+ * Check data endpoint for format differences
+ */
+static bool check_ep_params(struct snd_usb_endpoint *ep,
+			      snd_pcm_format_t pcm_format,
+			      unsigned int channels,
+			      unsigned int period_bytes,
+			      unsigned int frames_per_period,
+			      unsigned int periods_per_buffer,
+			      struct audioformat *fmt,
+			      struct snd_usb_endpoint *sync_ep)
+{
+	unsigned int maxsize, minsize, packs_per_ms, max_packs_per_urb;
+	unsigned int max_packs_per_period, urbs_per_period, urb_packs;
+	unsigned int max_urbs;
+	int frame_bits = snd_pcm_format_physical_width(pcm_format) * channels;
+	int tx_length_quirk = (ep->chip->tx_length_quirk &&
+			       usb_pipeout(ep->pipe));
+	bool ret = 1;
+
+	if (pcm_format == SNDRV_PCM_FORMAT_DSD_U16_LE && fmt->dsd_dop) {
+		/*
+		 * When operating in DSD DOP mode, the size of a sample frame
+		 * in hardware differs from the actual physical format width
+		 * because we need to make room for the DOP markers.
+		 */
+		frame_bits += channels << 3;
+	}
+
+	ret = ret && (ep->datainterval == fmt->datainterval);
+	ret = ret && (ep->stride == frame_bits >> 3);
+
+	switch (pcm_format) {
+	case SNDRV_PCM_FORMAT_U8:
+		ret = ret && (ep->silence_value == 0x80);
+		break;
+	case SNDRV_PCM_FORMAT_DSD_U8:
+	case SNDRV_PCM_FORMAT_DSD_U16_LE:
+	case SNDRV_PCM_FORMAT_DSD_U32_LE:
+	case SNDRV_PCM_FORMAT_DSD_U16_BE:
+	case SNDRV_PCM_FORMAT_DSD_U32_BE:
+		ret = ret && (ep->silence_value == 0x69);
+		break;
+	default:
+		ret = ret && (ep->silence_value == 0);
+	}
+
+	/* assume max. frequency is 50% higher than nominal */
+	ret = ret && (ep->freqmax == ep->freqn + (ep->freqn >> 1));
+	/* Round up freqmax to nearest integer in order to calculate maximum
+	 * packet size, which must represent a whole number of frames.
+	 * This is accomplished by adding 0x0.ffff before converting the
+	 * Q16.16 format into integer.
+	 * In order to accurately calculate the maximum packet size when
+	 * the data interval is more than 1 (i.e. ep->datainterval > 0),
+	 * multiply by the data interval prior to rounding. For instance,
+	 * a freqmax of 41 kHz will result in a max packet size of 6 (5.125)
+	 * frames with a data interval of 1, but 11 (10.25) frames with a
+	 * data interval of 2.
+	 * (ep->freqmax << ep->datainterval overflows at 8.192 MHz for the
+	 * maximum datainterval value of 3, at USB full speed, higher for
+	 * USB high speed, noting that ep->freqmax is in units of
+	 * frames per packet in Q16.16 format.)
+	 */
+	maxsize = (((ep->freqmax << ep->datainterval) + 0xffff) >> 16) *
+			 (frame_bits >> 3);
+	if (tx_length_quirk)
+		maxsize += sizeof(__le32); /* Space for length descriptor */
+	/* but wMaxPacketSize might reduce this */
+	if (ep->maxpacksize && ep->maxpacksize < maxsize) {
+		/* whatever fits into a max. size packet */
+		unsigned int data_maxsize = maxsize = ep->maxpacksize;
+
+		if (tx_length_quirk)
+			/* Need to remove the length descriptor to calc freq */
+			data_maxsize -= sizeof(__le32);
+		ret = ret && (ep->freqmax == (data_maxsize / (frame_bits >> 3))
+				<< (16 - ep->datainterval));
+	}
+
+	if (ep->fill_max)
+		ret = ret && (ep->curpacksize == ep->maxpacksize);
+	else
+		ret = ret && (ep->curpacksize == maxsize);
+
+	if (snd_usb_get_speed(ep->chip->dev) != USB_SPEED_FULL) {
+		packs_per_ms = 8 >> ep->datainterval;
+		max_packs_per_urb = MAX_PACKS_HS;
+	} else {
+		packs_per_ms = 1;
+		max_packs_per_urb = MAX_PACKS;
+	}
+	if (sync_ep && !snd_usb_endpoint_implicit_feedback_sink(ep))
+		max_packs_per_urb = min(max_packs_per_urb,
+					1U << sync_ep->syncinterval);
+	max_packs_per_urb = max(1u, max_packs_per_urb >> ep->datainterval);
+
+	/*
+	 * Capture endpoints need to use small URBs because there's no way
+	 * to tell in advance where the next period will end, and we don't
+	 * want the next URB to complete much after the period ends.
+	 *
+	 * Playback endpoints with implicit sync much use the same parameters
+	 * as their corresponding capture endpoint.
+	 */
+	if (usb_pipein(ep->pipe) ||
+			snd_usb_endpoint_implicit_feedback_sink(ep)) {
+
+		urb_packs = packs_per_ms;
+		/*
+		 * Wireless devices can poll at a max rate of once per 4ms.
+		 * For dataintervals less than 5, increase the packet count to
+		 * allow the host controller to use bursting to fill in the
+		 * gaps.
+		 */
+		if (snd_usb_get_speed(ep->chip->dev) == USB_SPEED_WIRELESS) {
+			int interval = ep->datainterval;
+
+			while (interval < 5) {
+				urb_packs <<= 1;
+				++interval;
+			}
+		}
+		/* make capture URBs <= 1 ms and smaller than a period */
+		urb_packs = min(max_packs_per_urb, urb_packs);
+		while (urb_packs > 1 && urb_packs * maxsize >= period_bytes)
+			urb_packs >>= 1;
+		ret = ret && (ep->nurbs == MAX_URBS);
+
+	/*
+	 * Playback endpoints without implicit sync are adjusted so that
+	 * a period fits as evenly as possible in the smallest number of
+	 * URBs.  The total number of URBs is adjusted to the size of the
+	 * ALSA buffer, subject to the MAX_URBS and MAX_QUEUE limits.
+	 */
+	} else {
+		/* determine how small a packet can be */
+		minsize = (ep->freqn >> (16 - ep->datainterval)) *
+				(frame_bits >> 3);
+		/* with sync from device, assume it can be 12% lower */
+		if (sync_ep)
+			minsize -= minsize >> 3;
+		minsize = max(minsize, 1u);
+
+		/* how many packets will contain an entire ALSA period? */
+		max_packs_per_period = DIV_ROUND_UP(period_bytes, minsize);
+
+		/* how many URBs will contain a period? */
+		urbs_per_period = DIV_ROUND_UP(max_packs_per_period,
+				max_packs_per_urb);
+		/* how many packets are needed in each URB? */
+		urb_packs = DIV_ROUND_UP(max_packs_per_period, urbs_per_period);
+
+		/* limit the number of frames in a single URB */
+		ret = ret && (ep->max_urb_frames ==
+			DIV_ROUND_UP(frames_per_period, urbs_per_period));
+
+		/* try to use enough URBs to contain an entire ALSA buffer */
+		max_urbs = min((unsigned) MAX_URBS,
+				MAX_QUEUE * packs_per_ms / urb_packs);
+		ret = ret && (ep->nurbs == min(max_urbs,
+				urbs_per_period * periods_per_buffer));
+	}
+
+	ret = ret && (ep->datainterval == fmt->datainterval);
+	ret = ret && (ep->maxpacksize == fmt->maxpacksize);
+	ret = ret &&
+		(ep->fill_max == !!(fmt->attributes & UAC_EP_CS_ATTR_FILL_MAX));
+
+	return ret;
+}
+
 /*
  * configure a data endpoint
  */
@@ -886,10 +1060,23 @@  int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep,
 	int err;
 
 	if (ep->use_count != 0) {
-		usb_audio_warn(ep->chip,
-			 "Unable to change format on ep #%x: already in use\n",
-			 ep->ep_num);
-		return -EBUSY;
+		bool check = ep->is_implicit_feedback &&
+			check_ep_params(ep, pcm_format,
+					     channels, period_bytes,
+					     period_frames, buffer_periods,
+					     fmt, sync_ep);
+
+		if (!check) {
+			usb_audio_warn(ep->chip,
+				"Unable to change format on ep #%x: already in use\n",
+				ep->ep_num);
+			return -EBUSY;
+		}
+
+		usb_audio_dbg(ep->chip,
+			      "Ep #%x already in use as implicit feedback but format not changed\n",
+			      ep->ep_num);
+		return 0;
 	}
 
 	/* release old buffers, if any */
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index b50965ab3b3a..d61c2f1095b5 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -404,6 +404,8 @@  static int set_sync_ep_implicit_fb_quirk(struct snd_usb_substream *subs,
 	if (!subs->sync_endpoint)
 		return -EINVAL;
 
+	subs->sync_endpoint->is_implicit_feedback = 1;
+
 	subs->data_endpoint->sync_master = subs->sync_endpoint;
 
 	return 1;
@@ -502,12 +504,15 @@  static int set_sync_endpoint(struct snd_usb_substream *subs,
 						   implicit_fb ?
 							SND_USB_ENDPOINT_TYPE_DATA :
 							SND_USB_ENDPOINT_TYPE_SYNC);
+
 	if (!subs->sync_endpoint) {
 		if (is_playback && attr == USB_ENDPOINT_SYNC_NONE)
 			return 0;
 		return -EINVAL;
 	}
 
+	subs->sync_endpoint->is_implicit_feedback = implicit_fb;
+
 	subs->data_endpoint->sync_master = subs->sync_endpoint;
 
 	return 0;