Hello Takashi
Thanks for your comments, Please find my answers below
Regards Arnaud
On 03/23/2017 08:02 PM, Takashi Iwai wrote:
On Thu, 23 Mar 2017 19:39:55 +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 calls snd_pcm_stream_lock, player->substream is already null.
Signed-off-by: Arnaud Pouliquen arnaud.pouliquen@st.com
V2: Add spinlock to protect player/reader->substream
sound/soc/sti/uniperif.h | 1 + sound/soc/sti/uniperif_player.c | 34 +++++++++++++++++++++++----------- sound/soc/sti/uniperif_reader.c | 24 ++++++++++++++++++++---- 3 files changed, 44 insertions(+), 15 deletions(-)
diff --git a/sound/soc/sti/uniperif.h b/sound/soc/sti/uniperif.h index d487dd2..cfcb0ea 100644 --- a/sound/soc/sti/uniperif.h +++ b/sound/soc/sti/uniperif.h @@ -1299,6 +1299,7 @@ struct uniperif { int ver; /* IP version, used by register access macros */ struct regmap_field *clk_sel; struct regmap_field *valid_sel;
spinlock_t irq_lock; /* used to prevent race condition with IRQ */
/* capabilities */ const struct snd_pcm_hardware *hw;
diff --git a/sound/soc/sti/uniperif_player.c b/sound/soc/sti/uniperif_player.c index 60ae31a..8f3a582 100644 --- a/sound/soc/sti/uniperif_player.c +++ b/sound/soc/sti/uniperif_player.c @@ -65,10 +65,13 @@ static irqreturn_t uni_player_irq_handler(int irq, void *dev_id) unsigned int status; unsigned int tmp;
- if (player->state == UNIPERIF_STATE_STOPPED) {
/* Unexpected IRQ: do nothing */return IRQ_NONE;- }
- spin_lock(&player->irq_lock);
- if (!player->substream)
goto IRQ_END;This will be NULL-dereference, as it calls snd_pcm_stream_unlock() there. Also, use lower letters for labels.
Beginner's fault...
- snd_pcm_stream_lock(player->substream);
Actually we don't need to take this lock here any longer since we have irq_lock. Better to keep the stream lock only around the stop.
Needed to protect "player->state". The irq_lock does not ensure that application calls a PCM stop in parallel on another CPU. That means that i need also to protect uni_player_stop and uni_player_start to protect layer->state. But in this case i will have a double lock when call snd_pcm_stop in IRQ handler.
if (player->state == UNIPERIF_STATE_STOPPED)
goto IRQ_END;/* Get interrupt status & clear them immediately */ status = GET_UNIPERIF_ITS(player);
@@ -88,9 +91,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);BTW, these three calls can be simplified with snd_pcm_stop_xrun().
Takashi