Page MenuHomeFreeBSD

OSS: allow unplug soundcars without apps close devices
Needs ReviewPublic

Authored by rozhuk.im-gmail.com on Jun 10 2017, 8:50 PM.

Details

Reviewers
hselasky
Summary

This patch fix use case: user unplug USB sound card before all apps close /dev/mixer and /dev/dsp and then USB driver hang/wait until all apps closed, no old devices can be removed, no new devices can be added.

dmesg show for mixer:
pcm4: unregister: mixer busy
pcm4: Waiting for sound application to exit!
And almost same for dsp, but also prints app pid.

Test Plan
  • apply patch
  • rebuild kernel
  • install kernel and reboot
  • start playing sound/music then unplug USB sound card
  • connect sound card and restart player

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Hi,

Did you check that no memory is used after free?

Did you build a kernel with WITNESS and memory debugging turned on?

--HPS

No.
How I can check mem use after free and how turn on mem debug?

Hi,

I'm afraid that pending read/write/ioctls might use freed structures. I'll try to analyze your patch carefully next week. Currently at bsdcan2017 .

--HPS

Hi,

Make a test app like this:

fd = open("/dev/dsp" ...);
while (1) {
if (ioctl(fd, SND_XXXX, XXX) != 0)
break;
}
close(fd);

And run it while you yank the USB audio device. Also try reading(), writing() and mmap() in a loop when detaching.

--HPS

Thank you.

Im not sure about race condition in case: app read/write/ioctl/close and in same time unplug code started in another thread.
May be destroy_dev() handle this.
If yes then I'm sure that mixer OK, but dsp code match more complicated, so I hope that some one with expert knowledge in OSS and dev subsystem will rewiew/help.

destroy_dev() will make sure all system calls have returned. In your patch are you destroying the device first?

--HPS

I think you approach may work for the mixer case, assuming no IOCTLs are sleeping. Please make sure that destroy_dev() will flush all pending system calls.

sys/dev/sound/usb/uaudio.c
1239

Is this change needed. I think mixer_uninit() will now always succeed?