On Mon, 18 Nov 2019 17:33:51 +0100, Pierre-Louis Bossart wrote:
On 11/17/19 2:53 AM, Takashi Iwai wrote:
The standard programming model of a PCM sound driver is to process snd_pcm_period_elapsed() from an interrupt handler. When a running stream is stopped, PCM core calls the trigger-STOP PCM ops, sets the stream state to SETUP, and moves on to the next step. This is performed in an atomic manner -- this could be called from the interrupt context, after all.
The problem is that, if the stream goes further and reaches to the CLOSE state immediately, the stream might be still being processed in snd_pcm_period_elapsed() in the interrupt context, and hits a NULL dereference. Such a crash happens because of the atomic operation, and we can't wait until the stream-stop finishes.
For addressing such a problem, this commit adds a new PCM ops, sync_stop. This gets called at the appropriate places that need a sync with the stream-stop, i.e. at hw_params, prepare and hw_free.
Some drivers already have a similar mechanism implemented locally, and we'll refactor the code later.
This rings a bell, for the SOF driver Keyon added a workqueue to avoid doing the call to snd_pcm_period_elapsed() while handling IPC interrupts. I believe the reason what that the IPC needs to be serialized, and the call to snd_pcm_period_elapsed could initiate a trigger stop IPC while we were dealing with an IPC, which broke the notion of serialization.
See "ASoC: SOF: PCM: add period_elapsed work to fix race condition in interrupt context" and "ASoC: SOF: ipc: use snd_sof_pcm_period_elapsed"
The new PCM ops is exactly for resolving such a thing. Basically snd_pcm_period_elapsed() is handled asynchronously, so we need the synchronization. For a simpler driver, it's merely synchronize_irq() call (no matter whether it's a hard-irq or a threaded-irq), but for drivers that deal with another async mechanism (e.g. USB-audio), another sync procedure is required.
It's possible that the workaround you've done in SOF can be simplified by this mechanism, but I didn't take a deeper look at it yet.
thanks,
Takashi