On 03/20/2017 06:42 PM, Takashi Iwai wrote: > On Mon, 20 Mar 2017 18:09:15 +0100, > Arnaud Pouliquen wrote: >> >> With RTlinux a race condition has been found that leads to NULL ptr crash: >> - On CPU 0: uni_player_irq_handler is called to treat XRUN >> "(player->state == UNIPERIF_STATE_STOPPED)" is FALSE so status is checked, >> dev_err(player->dev, "FIFO underflow error detected") is printed >> and then snd_pcm_stream_lock should be called to lock stream for stopping. >> - On CPU 1: application stop and close the stream. >> Issue is that the stop and shutdown functions are executed while >> "FIFO underflow error detected" is printed. >> So when CPU 0 call snd_pcm_stream_lock is player->substream is already null. > > Hrm, how the first call of snd_pcm_stream_lock() with > player->substream to be guaranteed to be non-NULL? Not sure that it is possible to be 100% save, but player->substream should not be null... Use case at risk: Stop request occurs just before IRQ. 1) IRQ handler locked on snd_pcm_stream_lock(player->substream) 2) uni_player_stop clear IRQ flags and set player->state == UNIPERIF_STATE_STOPPED. So no more IRQs after UNIPERIF_STATE_STOPPED state. 3) As stream is cleared after close, i guess that between stop and close following IRQ handler code should be executed if (player->state == UNIPERIF_STATE_STOPPED) { /* Unexpected IRQ: do nothing */ snd_pcm_stream_unlock(player->substream); return IRQ_NONE; } Anyway adding a check of player->substream in IRQ handler before locking seems more save. I will add it in a V2 Thanks Arnaud > > > Takashi > > >> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxx> >> --- >> sound/soc/sti/uniperif_player.c | 9 +++------ >> sound/soc/sti/uniperif_reader.c | 8 +++++--- >> 2 files changed, 8 insertions(+), 9 deletions(-) >> >> diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c >> index 60ae31a..6fa9eee 100644 >> --- a/sound/soc/sti/uniperif_player.c >> +++ b/sound/soc/sti/uniperif_player.c >> @@ -65,8 +65,10 @@ static irqreturn_t uni_player_irq_handler(int irq, void *dev_id) >> unsigned int status; >> unsigned int tmp; >> >> + snd_pcm_stream_lock(player->substream); >> if (player->state == UNIPERIF_STATE_STOPPED) { >> /* Unexpected IRQ: do nothing */ >> + snd_pcm_stream_unlock(player->substream); >> return IRQ_NONE; >> } >> >> @@ -88,9 +90,7 @@ static irqreturn_t uni_player_irq_handler(int irq, void *dev_id) >> SET_UNIPERIF_ITM_BCLR_FIFO_ERROR(player); >> >> /* Stop the player */ >> - snd_pcm_stream_lock(player->substream); >> snd_pcm_stop(player->substream, SNDRV_PCM_STATE_XRUN); >> - snd_pcm_stream_unlock(player->substream); >> } >> >> ret = IRQ_HANDLED; >> @@ -104,9 +104,7 @@ static irqreturn_t uni_player_irq_handler(int irq, void *dev_id) >> SET_UNIPERIF_ITM_BCLR_DMA_ERROR(player); >> >> /* Stop the player */ >> - snd_pcm_stream_lock(player->substream); >> snd_pcm_stop(player->substream, SNDRV_PCM_STATE_XRUN); >> - snd_pcm_stream_unlock(player->substream); >> >> ret = IRQ_HANDLED; >> } >> @@ -138,13 +136,12 @@ static irqreturn_t uni_player_irq_handler(int irq, void *dev_id) >> dev_err(player->dev, "Underflow recovery failed\n"); >> >> /* Stop the player */ >> - snd_pcm_stream_lock(player->substream); >> snd_pcm_stop(player->substream, SNDRV_PCM_STATE_XRUN); >> - snd_pcm_stream_unlock(player->substream); >> >> ret = IRQ_HANDLED; >> } >> >> + snd_pcm_stream_unlock(player->substream); >> return ret; >> } >> >> diff --git a/sound/soc/sti/uniperif_reader.c b/sound/soc/sti/uniperif_reader.c >> index 5992c6a..9273f59 100644 >> --- a/sound/soc/sti/uniperif_reader.c >> +++ b/sound/soc/sti/uniperif_reader.c >> @@ -46,9 +46,11 @@ static irqreturn_t uni_reader_irq_handler(int irq, void *dev_id) >> struct uniperif *reader = dev_id; >> unsigned int status; >> >> + snd_pcm_stream_lock(reader->substream); >> if (reader->state == UNIPERIF_STATE_STOPPED) { >> /* Unexpected IRQ: do nothing */ >> dev_warn(reader->dev, "unexpected IRQ\n"); >> + snd_pcm_stream_unlock(reader->substream); >> return IRQ_HANDLED; >> } >> >> @@ -60,13 +62,13 @@ static irqreturn_t uni_reader_irq_handler(int irq, void *dev_id) >> if (unlikely(status & UNIPERIF_ITS_FIFO_ERROR_MASK(reader))) { >> dev_err(reader->dev, "FIFO error detected\n"); >> >> - snd_pcm_stream_lock(reader->substream); >> snd_pcm_stop(reader->substream, SNDRV_PCM_STATE_XRUN); >> - snd_pcm_stream_unlock(reader->substream); >> >> - return IRQ_HANDLED; >> + ret = IRQ_HANDLED; >> } >> >> + snd_pcm_stream_unlock(reader->substream); >> + >> return ret; >> } >> >> -- >> 1.9.1 >> _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel