| Message ID | 20231206151046.25773-1-ivan.orlov0322@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ALSA: pcmtest: stop timer in the hw_free callback | expand |
On Wed, 06 Dec 2023 16:10:46 +0100, Ivan Orlov wrote: > > Stop timer in the 'hw_free' callback instead of the 'close' callback > since we want the timer to be stopped before the DMA buffer is released. > Otherwise, it could trigger a kernel panic in some circumstances, for > instance when the DMA buffer is already released but the timer callback > is still running. You can't call timer_shutdown_sync() at hw_free. The PCM stream is still there and you can re-setup via hw_params without closing. But, after timer_shutdown_sync(), the timer instance must not be used any longer. A more proper way would be to call timer_delete() (no sync) at trigger STOP, then call timer_delete_sync() at sync_stop op in addition. This assures the immediate stop and the sync before changing to another PCM state. timer_shutdown_sync() can be kept at the close op to be sure. thanks, Takashi > > Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com> > --- > V1 -> V2: > - Remove useless NULLing of v_iter->substream. It will be released in > 'close' callback. > > sound/drivers/pcmtest.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/sound/drivers/pcmtest.c b/sound/drivers/pcmtest.c > index b59b78a09224..8a78fa56106f 100644 > --- a/sound/drivers/pcmtest.c > +++ b/sound/drivers/pcmtest.c > @@ -396,8 +396,6 @@ static int snd_pcmtst_pcm_close(struct snd_pcm_substream *substream) > { > struct pcmtst_buf_iter *v_iter = substream->runtime->private_data; > > - timer_shutdown_sync(&v_iter->timer_instance); > - v_iter->substream = NULL; > playback_capture_test = !v_iter->is_buf_corrupted; > kfree(v_iter); > return 0; > @@ -499,6 +497,10 @@ static int snd_pcmtst_pcm_hw_params(struct snd_pcm_substream *substream, > > static int snd_pcmtst_pcm_hw_free(struct snd_pcm_substream *substream) > { > + struct pcmtst_buf_iter *v_iter = substream->runtime->private_data; > + > + timer_shutdown_sync(&v_iter->timer_instance); > + > return 0; > } > > -- > 2.34.1 >
On 12/6/23 16:59, Takashi Iwai wrote: > On Wed, 06 Dec 2023 16:10:46 +0100, > Ivan Orlov wrote: >> >> Stop timer in the 'hw_free' callback instead of the 'close' callback >> since we want the timer to be stopped before the DMA buffer is released. >> Otherwise, it could trigger a kernel panic in some circumstances, for >> instance when the DMA buffer is already released but the timer callback >> is still running. > > You can't call timer_shutdown_sync() at hw_free. The PCM stream is > still there and you can re-setup via hw_params without closing. But, > after timer_shutdown_sync(), the timer instance must not be used any > longer. > > A more proper way would be to call timer_delete() (no sync) at trigger > STOP, then call timer_delete_sync() at sync_stop op in addition. This > assures the immediate stop and the sync before changing to another PCM > state. > > timer_shutdown_sync() can be kept at the close op to be sure. > > > thanks, > > Takashi > Hi Takashi, Thanks a lot for the review, sounds reasonable, I'll make suggested changes and send the version 3 :) -- Kind regards, Ivan Orlov
diff --git a/sound/drivers/pcmtest.c b/sound/drivers/pcmtest.c index b59b78a09224..8a78fa56106f 100644 --- a/sound/drivers/pcmtest.c +++ b/sound/drivers/pcmtest.c @@ -396,8 +396,6 @@ static int snd_pcmtst_pcm_close(struct snd_pcm_substream *substream) { struct pcmtst_buf_iter *v_iter = substream->runtime->private_data; - timer_shutdown_sync(&v_iter->timer_instance); - v_iter->substream = NULL; playback_capture_test = !v_iter->is_buf_corrupted; kfree(v_iter); return 0; @@ -499,6 +497,10 @@ static int snd_pcmtst_pcm_hw_params(struct snd_pcm_substream *substream, static int snd_pcmtst_pcm_hw_free(struct snd_pcm_substream *substream) { + struct pcmtst_buf_iter *v_iter = substream->runtime->private_data; + + timer_shutdown_sync(&v_iter->timer_instance); + return 0; }
Stop timer in the 'hw_free' callback instead of the 'close' callback since we want the timer to be stopped before the DMA buffer is released. Otherwise, it could trigger a kernel panic in some circumstances, for instance when the DMA buffer is already released but the timer callback is still running. Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com> --- V1 -> V2: - Remove useless NULLing of v_iter->substream. It will be released in 'close' callback. sound/drivers/pcmtest.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)