snd_hdsp*: Fix M_NOWAIT malloc(9) calls

[christos]

Summary

Sponsored by: The FreeBSD Foundation
MFC after: 1 week

[kib]

https://reviews.freebsd.org/D53896

sys/dev/sound/pci/hdsp-pcm.c

729

It seems that sc->lock mutex is owned there, which means that code cannot sleep. Probably this together with lazyness was the reason why M_NOWAIT was used.

Perhaps you can make the allocations before acquiring the mutex. Or, why this mutex is needed during the initialization at all?

[christos]

sys/dev/sound/pci/hdsp-pcm.c

729

I'm also not sure why the lock is held here. This is a kobj method that gets called by sound(4) through CHANNEL_INIT() in chn_init() (sys/dev/sound/pcm/channel.c). Making the allocations before acquiring the mutex here probably wouldn't work because we acquire it before the ch = &scp->chan[num];.

My first guess is that the lock might be here to make sure that we don't race when accessing scp->chan[num], since num is incremented every time the driver creates a channel. However num is incremented in hdsp_pcm_attach() below in sequence, it does not run in parallel. There are also a few hardware commands that are issued while holding the lock here. I'm pretty sure it's safe to get rid of the lock?

[kib]

sys/dev/sound/pci/hdsp-pcm.c

729

Well, either we can make the allocations before taking the lock because you argue that the lock is not needed at all, or there is something more convoluted.

If we indeed must take a lock to correctly evaluate the number and channel pointer, then there is another pattern that might help:

  mtx_lock();
  for (;;) {
       num = XXX;
       ch = XXX;
       mtx_unlock();
       malloc(M_WAITOK);
       mtx_lock();
       if (recheck that num and ch are still valid) {
            break;
       } else {
           mtx_unlock();
           deallocate();
           mtx_lock();
       }
}