| From e83774645a1679a986c14bea32208b12d6e4efa2 Mon Sep 17 00:00:00 2001 |
| From: Olivier Moysan <olivier.moysan@st.com> |
| Date: Thu, 9 Jan 2020 09:32:54 +0100 |
| Subject: [PATCH] ASoC: stm32: sai: fix possible circular locking |
| |
| commit a14bf98c045bf119b7e779f186528e38c6428830 upstream. |
| |
| In current driver, locks can be taken as follows: |
| - Register access: take a lock on regmap config and then on clock. |
| - Master clock provider: take a lock on clock and then on regmap config. |
| This can lead to the circular locking summarized below. |
| |
| Remove peripheral clock management through regmap framework, and manage |
| peripheral clock in driver instead. On register access, lock on clock |
| is taken first, which allows to avoid possible locking issue. |
| |
| [ 6696.561513] ====================================================== |
| [ 6696.567670] WARNING: possible circular locking dependency detected |
| [ 6696.573842] 4.19.49 #866 Not tainted |
| [ 6696.577397] ------------------------------------------------------ |
| [ 6696.583566] pulseaudio/6439 is trying to acquire lock: |
| [ 6696.588697] 87b0a25b (enable_lock){..-.}, at: clk_enable_lock+0x64/0x128 |
| [ 6696.595377] |
| [ 6696.595377] but task is already holding lock: |
| [ 6696.601197] d858f825 (stm32_sai_sub:1342:(sai->regmap_config)->lock){....} |
| ... |
| [ 6696.812513] Possible unsafe locking scenario: |
| [ 6696.812513] |
| [ 6696.818418] CPU0 CPU1 |
| [ 6696.822935] ---- ---- |
| [ 6696.827451] lock(stm32_sai_sub:1342:(sai->regmap_config)->lock); |
| [ 6696.833618] lock(enable_lock); |
| [ 6696.839350] lock(stm32_sai_sub:1342: |
| (sai->regmap_config)->lock); |
| [ 6696.848035] lock(enable_lock); |
| |
| Fixes: 03e78a242a15 ("ASoC: stm32: sai: add h7 support") |
| |
| Signed-off-by: Olivier Moysan <olivier.moysan@st.com> |
| Link: https://lore.kernel.org/r/20200109083254.478-1-olivier.moysan@st.com |
| Signed-off-by: Mark Brown <broonie@kernel.org> |
| Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> |
| |
| diff --git a/sound/soc/stm/stm32_sai_sub.c b/sound/soc/stm/stm32_sai_sub.c |
| index 49ee86d3dbec..0174692b288e 100644 |
| --- a/sound/soc/stm/stm32_sai_sub.c |
| +++ b/sound/soc/stm/stm32_sai_sub.c |
| @@ -184,6 +184,56 @@ static bool stm32_sai_sub_writeable_reg(struct device *dev, unsigned int reg) |
| } |
| } |
| |
| +static int stm32_sai_sub_reg_up(struct stm32_sai_sub_data *sai, |
| + unsigned int reg, unsigned int mask, |
| + unsigned int val) |
| +{ |
| + int ret; |
| + |
| + ret = clk_enable(sai->pdata->pclk); |
| + if (ret < 0) |
| + return ret; |
| + |
| + ret = regmap_update_bits(sai->regmap, reg, mask, val); |
| + |
| + clk_disable(sai->pdata->pclk); |
| + |
| + return ret; |
| +} |
| + |
| +static int stm32_sai_sub_reg_wr(struct stm32_sai_sub_data *sai, |
| + unsigned int reg, unsigned int mask, |
| + unsigned int val) |
| +{ |
| + int ret; |
| + |
| + ret = clk_enable(sai->pdata->pclk); |
| + if (ret < 0) |
| + return ret; |
| + |
| + ret = regmap_write_bits(sai->regmap, reg, mask, val); |
| + |
| + clk_disable(sai->pdata->pclk); |
| + |
| + return ret; |
| +} |
| + |
| +static int stm32_sai_sub_reg_rd(struct stm32_sai_sub_data *sai, |
| + unsigned int reg, unsigned int *val) |
| +{ |
| + int ret; |
| + |
| + ret = clk_enable(sai->pdata->pclk); |
| + if (ret < 0) |
| + return ret; |
| + |
| + ret = regmap_read(sai->regmap, reg, val); |
| + |
| + clk_disable(sai->pdata->pclk); |
| + |
| + return ret; |
| +} |
| + |
| static const struct regmap_config stm32_sai_sub_regmap_config_f4 = { |
| .reg_bits = 32, |
| .reg_stride = 4, |
| @@ -295,7 +345,7 @@ static int stm32_sai_set_clk_div(struct stm32_sai_sub_data *sai, |
| |
| mask = SAI_XCR1_MCKDIV_MASK(SAI_XCR1_MCKDIV_WIDTH(version)); |
| cr1 = SAI_XCR1_MCKDIV_SET(div); |
| - ret = regmap_update_bits(sai->regmap, STM_SAI_CR1_REGX, mask, cr1); |
| + ret = stm32_sai_sub_reg_up(sai, STM_SAI_CR1_REGX, mask, cr1); |
| if (ret < 0) |
| dev_err(&sai->pdev->dev, "Failed to update CR1 register\n"); |
| |
| @@ -372,8 +422,8 @@ static int stm32_sai_mclk_enable(struct clk_hw *hw) |
| |
| dev_dbg(&sai->pdev->dev, "Enable master clock\n"); |
| |
| - return regmap_update_bits(sai->regmap, STM_SAI_CR1_REGX, |
| - SAI_XCR1_MCKEN, SAI_XCR1_MCKEN); |
| + return stm32_sai_sub_reg_up(sai, STM_SAI_CR1_REGX, |
| + SAI_XCR1_MCKEN, SAI_XCR1_MCKEN); |
| } |
| |
| static void stm32_sai_mclk_disable(struct clk_hw *hw) |
| @@ -383,7 +433,7 @@ static void stm32_sai_mclk_disable(struct clk_hw *hw) |
| |
| dev_dbg(&sai->pdev->dev, "Disable master clock\n"); |
| |
| - regmap_update_bits(sai->regmap, STM_SAI_CR1_REGX, SAI_XCR1_MCKEN, 0); |
| + stm32_sai_sub_reg_up(sai, STM_SAI_CR1_REGX, SAI_XCR1_MCKEN, 0); |
| } |
| |
| static const struct clk_ops mclk_ops = { |
| @@ -446,15 +496,15 @@ static irqreturn_t stm32_sai_isr(int irq, void *devid) |
| unsigned int sr, imr, flags; |
| snd_pcm_state_t status = SNDRV_PCM_STATE_RUNNING; |
| |
| - regmap_read(sai->regmap, STM_SAI_IMR_REGX, &imr); |
| - regmap_read(sai->regmap, STM_SAI_SR_REGX, &sr); |
| + stm32_sai_sub_reg_rd(sai, STM_SAI_IMR_REGX, &imr); |
| + stm32_sai_sub_reg_rd(sai, STM_SAI_SR_REGX, &sr); |
| |
| flags = sr & imr; |
| if (!flags) |
| return IRQ_NONE; |
| |
| - regmap_write_bits(sai->regmap, STM_SAI_CLRFR_REGX, SAI_XCLRFR_MASK, |
| - SAI_XCLRFR_MASK); |
| + stm32_sai_sub_reg_wr(sai, STM_SAI_CLRFR_REGX, SAI_XCLRFR_MASK, |
| + SAI_XCLRFR_MASK); |
| |
| if (!sai->substream) { |
| dev_err(&pdev->dev, "Device stopped. Spurious IRQ 0x%x\n", sr); |
| @@ -503,8 +553,8 @@ static int stm32_sai_set_sysclk(struct snd_soc_dai *cpu_dai, |
| int ret; |
| |
| if (dir == SND_SOC_CLOCK_OUT && sai->sai_mclk) { |
| - ret = regmap_update_bits(sai->regmap, STM_SAI_CR1_REGX, |
| - SAI_XCR1_NODIV, |
| + ret = stm32_sai_sub_reg_up(sai, STM_SAI_CR1_REGX, |
| + SAI_XCR1_NODIV, |
| (unsigned int)~SAI_XCR1_NODIV); |
| if (ret < 0) |
| return ret; |
| @@ -573,7 +623,7 @@ static int stm32_sai_set_dai_tdm_slot(struct snd_soc_dai *cpu_dai, u32 tx_mask, |
| |
| slotr_mask |= SAI_XSLOTR_SLOTEN_MASK; |
| |
| - regmap_update_bits(sai->regmap, STM_SAI_SLOTR_REGX, slotr_mask, slotr); |
| + stm32_sai_sub_reg_up(sai, STM_SAI_SLOTR_REGX, slotr_mask, slotr); |
| |
| sai->slot_width = slot_width; |
| sai->slots = slots; |
| @@ -655,7 +705,7 @@ static int stm32_sai_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt) |
| cr1_mask |= SAI_XCR1_CKSTR; |
| frcr_mask |= SAI_XFRCR_FSPOL; |
| |
| - regmap_update_bits(sai->regmap, STM_SAI_FRCR_REGX, frcr_mask, frcr); |
| + stm32_sai_sub_reg_up(sai, STM_SAI_FRCR_REGX, frcr_mask, frcr); |
| |
| /* DAI clock master masks */ |
| switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { |
| @@ -683,7 +733,7 @@ static int stm32_sai_set_dai_fmt(struct snd_soc_dai *cpu_dai, unsigned int fmt) |
| cr1_mask |= SAI_XCR1_SLAVE; |
| |
| conf_update: |
| - ret = regmap_update_bits(sai->regmap, STM_SAI_CR1_REGX, cr1_mask, cr1); |
| + ret = stm32_sai_sub_reg_up(sai, STM_SAI_CR1_REGX, cr1_mask, cr1); |
| if (ret < 0) { |
| dev_err(cpu_dai->dev, "Failed to update CR1 register\n"); |
| return ret; |
| @@ -720,12 +770,12 @@ static int stm32_sai_startup(struct snd_pcm_substream *substream, |
| } |
| |
| /* Enable ITs */ |
| - regmap_write_bits(sai->regmap, STM_SAI_CLRFR_REGX, |
| - SAI_XCLRFR_MASK, SAI_XCLRFR_MASK); |
| + stm32_sai_sub_reg_wr(sai, STM_SAI_CLRFR_REGX, |
| + SAI_XCLRFR_MASK, SAI_XCLRFR_MASK); |
| |
| imr = SAI_XIMR_OVRUDRIE; |
| if (STM_SAI_IS_CAPTURE(sai)) { |
| - regmap_read(sai->regmap, STM_SAI_CR2_REGX, &cr2); |
| + stm32_sai_sub_reg_rd(sai, STM_SAI_CR2_REGX, &cr2); |
| if (cr2 & SAI_XCR2_MUTECNT_MASK) |
| imr |= SAI_XIMR_MUTEDETIE; |
| } |
| @@ -735,8 +785,8 @@ static int stm32_sai_startup(struct snd_pcm_substream *substream, |
| else |
| imr |= SAI_XIMR_AFSDETIE | SAI_XIMR_LFSDETIE; |
| |
| - regmap_update_bits(sai->regmap, STM_SAI_IMR_REGX, |
| - SAI_XIMR_MASK, imr); |
| + stm32_sai_sub_reg_up(sai, STM_SAI_IMR_REGX, |
| + SAI_XIMR_MASK, imr); |
| |
| return 0; |
| } |
| @@ -753,10 +803,10 @@ static int stm32_sai_set_config(struct snd_soc_dai *cpu_dai, |
| * SAI fifo threshold is set to half fifo, to keep enough space |
| * for DMA incoming bursts. |
| */ |
| - regmap_write_bits(sai->regmap, STM_SAI_CR2_REGX, |
| - SAI_XCR2_FFLUSH | SAI_XCR2_FTH_MASK, |
| - SAI_XCR2_FFLUSH | |
| - SAI_XCR2_FTH_SET(STM_SAI_FIFO_TH_HALF)); |
| + stm32_sai_sub_reg_wr(sai, STM_SAI_CR2_REGX, |
| + SAI_XCR2_FFLUSH | SAI_XCR2_FTH_MASK, |
| + SAI_XCR2_FFLUSH | |
| + SAI_XCR2_FTH_SET(STM_SAI_FIFO_TH_HALF)); |
| |
| /* DS bits in CR1 not set for SPDIF (size forced to 24 bits).*/ |
| if (STM_SAI_PROTOCOL_IS_SPDIF(sai)) { |
| @@ -785,7 +835,7 @@ static int stm32_sai_set_config(struct snd_soc_dai *cpu_dai, |
| if ((sai->slots == 2) && (params_channels(params) == 1)) |
| cr1 |= SAI_XCR1_MONO; |
| |
| - ret = regmap_update_bits(sai->regmap, STM_SAI_CR1_REGX, cr1_mask, cr1); |
| + ret = stm32_sai_sub_reg_up(sai, STM_SAI_CR1_REGX, cr1_mask, cr1); |
| if (ret < 0) { |
| dev_err(cpu_dai->dev, "Failed to update CR1 register\n"); |
| return ret; |
| @@ -799,7 +849,7 @@ static int stm32_sai_set_slots(struct snd_soc_dai *cpu_dai) |
| struct stm32_sai_sub_data *sai = snd_soc_dai_get_drvdata(cpu_dai); |
| int slotr, slot_sz; |
| |
| - regmap_read(sai->regmap, STM_SAI_SLOTR_REGX, &slotr); |
| + stm32_sai_sub_reg_rd(sai, STM_SAI_SLOTR_REGX, &slotr); |
| |
| /* |
| * If SLOTSZ is set to auto in SLOTR, align slot width on data size |
| @@ -821,16 +871,16 @@ static int stm32_sai_set_slots(struct snd_soc_dai *cpu_dai) |
| sai->slots = 2; |
| |
| /* The number of slots in the audio frame is equal to NBSLOT[3:0] + 1*/ |
| - regmap_update_bits(sai->regmap, STM_SAI_SLOTR_REGX, |
| - SAI_XSLOTR_NBSLOT_MASK, |
| - SAI_XSLOTR_NBSLOT_SET((sai->slots - 1))); |
| + stm32_sai_sub_reg_up(sai, STM_SAI_SLOTR_REGX, |
| + SAI_XSLOTR_NBSLOT_MASK, |
| + SAI_XSLOTR_NBSLOT_SET((sai->slots - 1))); |
| |
| /* Set default slots mask if not already set from DT */ |
| if (!(slotr & SAI_XSLOTR_SLOTEN_MASK)) { |
| sai->slot_mask = (1 << sai->slots) - 1; |
| - regmap_update_bits(sai->regmap, |
| - STM_SAI_SLOTR_REGX, SAI_XSLOTR_SLOTEN_MASK, |
| - SAI_XSLOTR_SLOTEN_SET(sai->slot_mask)); |
| + stm32_sai_sub_reg_up(sai, |
| + STM_SAI_SLOTR_REGX, SAI_XSLOTR_SLOTEN_MASK, |
| + SAI_XSLOTR_SLOTEN_SET(sai->slot_mask)); |
| } |
| |
| dev_dbg(cpu_dai->dev, "Slots %d, slot width %d\n", |
| @@ -860,14 +910,14 @@ static void stm32_sai_set_frame(struct snd_soc_dai *cpu_dai) |
| dev_dbg(cpu_dai->dev, "Frame length %d, frame active %d\n", |
| sai->fs_length, fs_active); |
| |
| - regmap_update_bits(sai->regmap, STM_SAI_FRCR_REGX, frcr_mask, frcr); |
| + stm32_sai_sub_reg_up(sai, STM_SAI_FRCR_REGX, frcr_mask, frcr); |
| |
| if ((sai->fmt & SND_SOC_DAIFMT_FORMAT_MASK) == SND_SOC_DAIFMT_LSB) { |
| offset = sai->slot_width - sai->data_size; |
| |
| - regmap_update_bits(sai->regmap, STM_SAI_SLOTR_REGX, |
| - SAI_XSLOTR_FBOFF_MASK, |
| - SAI_XSLOTR_FBOFF_SET(offset)); |
| + stm32_sai_sub_reg_up(sai, STM_SAI_SLOTR_REGX, |
| + SAI_XSLOTR_FBOFF_MASK, |
| + SAI_XSLOTR_FBOFF_SET(offset)); |
| } |
| } |
| |
| @@ -984,9 +1034,9 @@ static int stm32_sai_configure_clock(struct snd_soc_dai *cpu_dai, |
| return -EINVAL; |
| } |
| |
| - regmap_update_bits(sai->regmap, |
| - STM_SAI_CR1_REGX, |
| - SAI_XCR1_OSR, cr1); |
| + stm32_sai_sub_reg_up(sai, |
| + STM_SAI_CR1_REGX, |
| + SAI_XCR1_OSR, cr1); |
| |
| div = stm32_sai_get_clk_div(sai, sai_clk_rate, |
| sai->mclk_rate); |
| @@ -1048,12 +1098,12 @@ static int stm32_sai_trigger(struct snd_pcm_substream *substream, int cmd, |
| case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: |
| dev_dbg(cpu_dai->dev, "Enable DMA and SAI\n"); |
| |
| - regmap_update_bits(sai->regmap, STM_SAI_CR1_REGX, |
| - SAI_XCR1_DMAEN, SAI_XCR1_DMAEN); |
| + stm32_sai_sub_reg_up(sai, STM_SAI_CR1_REGX, |
| + SAI_XCR1_DMAEN, SAI_XCR1_DMAEN); |
| |
| /* Enable SAI */ |
| - ret = regmap_update_bits(sai->regmap, STM_SAI_CR1_REGX, |
| - SAI_XCR1_SAIEN, SAI_XCR1_SAIEN); |
| + ret = stm32_sai_sub_reg_up(sai, STM_SAI_CR1_REGX, |
| + SAI_XCR1_SAIEN, SAI_XCR1_SAIEN); |
| if (ret < 0) |
| dev_err(cpu_dai->dev, "Failed to update CR1 register\n"); |
| break; |
| @@ -1062,16 +1112,16 @@ static int stm32_sai_trigger(struct snd_pcm_substream *substream, int cmd, |
| case SNDRV_PCM_TRIGGER_STOP: |
| dev_dbg(cpu_dai->dev, "Disable DMA and SAI\n"); |
| |
| - regmap_update_bits(sai->regmap, STM_SAI_IMR_REGX, |
| - SAI_XIMR_MASK, 0); |
| + stm32_sai_sub_reg_up(sai, STM_SAI_IMR_REGX, |
| + SAI_XIMR_MASK, 0); |
| |
| - regmap_update_bits(sai->regmap, STM_SAI_CR1_REGX, |
| - SAI_XCR1_SAIEN, |
| - (unsigned int)~SAI_XCR1_SAIEN); |
| + stm32_sai_sub_reg_up(sai, STM_SAI_CR1_REGX, |
| + SAI_XCR1_SAIEN, |
| + (unsigned int)~SAI_XCR1_SAIEN); |
| |
| - ret = regmap_update_bits(sai->regmap, STM_SAI_CR1_REGX, |
| - SAI_XCR1_DMAEN, |
| - (unsigned int)~SAI_XCR1_DMAEN); |
| + ret = stm32_sai_sub_reg_up(sai, STM_SAI_CR1_REGX, |
| + SAI_XCR1_DMAEN, |
| + (unsigned int)~SAI_XCR1_DMAEN); |
| if (ret < 0) |
| dev_err(cpu_dai->dev, "Failed to update CR1 register\n"); |
| |
| @@ -1091,7 +1141,7 @@ static void stm32_sai_shutdown(struct snd_pcm_substream *substream, |
| struct stm32_sai_sub_data *sai = snd_soc_dai_get_drvdata(cpu_dai); |
| unsigned long flags; |
| |
| - regmap_update_bits(sai->regmap, STM_SAI_IMR_REGX, SAI_XIMR_MASK, 0); |
| + stm32_sai_sub_reg_up(sai, STM_SAI_IMR_REGX, SAI_XIMR_MASK, 0); |
| |
| regmap_update_bits(sai->regmap, STM_SAI_CR1_REGX, SAI_XCR1_NODIV, |
| SAI_XCR1_NODIV); |
| @@ -1166,7 +1216,7 @@ static int stm32_sai_dai_probe(struct snd_soc_dai *cpu_dai) |
| cr1_mask |= SAI_XCR1_SYNCEN_MASK; |
| cr1 |= SAI_XCR1_SYNCEN_SET(sai->sync); |
| |
| - return regmap_update_bits(sai->regmap, STM_SAI_CR1_REGX, cr1_mask, cr1); |
| + return stm32_sai_sub_reg_up(sai, STM_SAI_CR1_REGX, cr1_mask, cr1); |
| } |
| |
| static const struct snd_soc_dai_ops stm32_sai_pcm_dai_ops = { |
| @@ -1319,8 +1369,13 @@ static int stm32_sai_sub_parse_of(struct platform_device *pdev, |
| if (STM_SAI_IS_H7(sai->pdata) && STM_SAI_IS_SUB_A(sai)) |
| sai->regmap_config = &stm32_sai_sub_regmap_config_h7; |
| |
| - sai->regmap = devm_regmap_init_mmio_clk(&pdev->dev, "sai_ck", |
| - base, sai->regmap_config); |
| + /* |
| + * Do not manage peripheral clock through regmap framework as this |
| + * can lead to circular locking issue with sai master clock provider. |
| + * Manage peripheral clock directly in driver instead. |
| + */ |
| + sai->regmap = devm_regmap_init_mmio(&pdev->dev, base, |
| + sai->regmap_config); |
| if (IS_ERR(sai->regmap)) { |
| dev_err(&pdev->dev, "Failed to initialize MMIO\n"); |
| return PTR_ERR(sai->regmap); |
| @@ -1417,6 +1472,10 @@ static int stm32_sai_sub_parse_of(struct platform_device *pdev, |
| return PTR_ERR(sai->sai_ck); |
| } |
| |
| + ret = clk_prepare(sai->pdata->pclk); |
| + if (ret < 0) |
| + return ret; |
| + |
| if (STM_SAI_IS_F4(sai->pdata)) |
| return 0; |
| |
| @@ -1498,22 +1557,48 @@ static int stm32_sai_sub_probe(struct platform_device *pdev) |
| return 0; |
| } |
| |
| +static int stm32_sai_sub_remove(struct platform_device *pdev) |
| +{ |
| + struct stm32_sai_sub_data *sai = dev_get_drvdata(&pdev->dev); |
| + |
| + clk_unprepare(sai->pdata->pclk); |
| + |
| + return 0; |
| +} |
| + |
| #ifdef CONFIG_PM_SLEEP |
| static int stm32_sai_sub_suspend(struct device *dev) |
| { |
| struct stm32_sai_sub_data *sai = dev_get_drvdata(dev); |
| + int ret; |
| + |
| + ret = clk_enable(sai->pdata->pclk); |
| + if (ret < 0) |
| + return ret; |
| |
| regcache_cache_only(sai->regmap, true); |
| regcache_mark_dirty(sai->regmap); |
| + |
| + clk_disable(sai->pdata->pclk); |
| + |
| return 0; |
| } |
| |
| static int stm32_sai_sub_resume(struct device *dev) |
| { |
| struct stm32_sai_sub_data *sai = dev_get_drvdata(dev); |
| + int ret; |
| + |
| + ret = clk_enable(sai->pdata->pclk); |
| + if (ret < 0) |
| + return ret; |
| |
| regcache_cache_only(sai->regmap, false); |
| - return regcache_sync(sai->regmap); |
| + ret = regcache_sync(sai->regmap); |
| + |
| + clk_disable(sai->pdata->pclk); |
| + |
| + return ret; |
| } |
| #endif /* CONFIG_PM_SLEEP */ |
| |
| @@ -1528,6 +1613,7 @@ static struct platform_driver stm32_sai_sub_driver = { |
| .pm = &stm32_sai_sub_pm_ops, |
| }, |
| .probe = stm32_sai_sub_probe, |
| + .remove = stm32_sai_sub_remove, |
| }; |
| |
| module_platform_driver(stm32_sai_sub_driver); |
| -- |
| 2.7.4 |
| |