| From 6af4fd8d354244dcacd4306a5ea5f928f18d39b8 Mon Sep 17 00:00:00 2001 |
| From: Tzung-Bi Shih <tzungbi@google.com> |
| Date: Fri, 22 Nov 2019 15:31:14 +0800 |
| Subject: [PATCH] ASoC: max98090: fix possible race conditions |
| |
| commit 45dfbf56975994822cce00b7475732a49f8aefed upstream. |
| |
| max98090_interrupt() and max98090_pll_work() run in 2 different threads. |
| There are 2 possible races: |
| |
| Note: M98090_REG_DEVICE_STATUS = 0x01. |
| Note: ULK == 0, PLL is locked; ULK == 1, PLL is unlocked. |
| |
| max98090_interrupt max98090_pll_work |
| ---------------------------------------------- |
| schedule max98090_pll_work |
| restart max98090 codec |
| receive ULK INT |
| assert ULK == 0 |
| schedule max98090_pll_work (1). |
| |
| In the case (1), the PLL is locked but max98090_interrupt unnecessarily |
| schedules another max98090_pll_work. |
| |
| max98090_interrupt max98090_pll_work max98090 codec |
| ---------------------------------------------------------------------- |
| ULK = 1 |
| receive ULK INT |
| read 0x01 |
| ULK = 0 (clear on read) |
| schedule max98090_pll_work |
| restart max98090 codec |
| ULK = 1 |
| receive ULK INT |
| read 0x01 |
| ULK = 0 (clear on read) |
| read 0x01 |
| assert ULK == 0 (2). |
| |
| In the case (2), both max98090_interrupt and max98090_pll_work read |
| the same clear-on-read register. max98090_pll_work would falsely |
| thought PLL is locked. |
| Note: the case (2) race is introduced by the previous commit ("ASoC: |
| max98090: exit workaround earlier if PLL is locked") to check the status |
| and exit the loop earlier in max98090_pll_work. |
| |
| There are 2 possible solution options: |
| A. turn off ULK interrupt before scheduling max98090_pll_work; and turn |
| on again before exiting max98090_pll_work. |
| B. remove the second thread of execution. |
| |
| Option A cannot fix the case (2) race because it still has 2 threads |
| access the same clear-on-read register simultaneously. Although we |
| could suppose the register is volatile and read the status via I2C could |
| be much slower than the hardware raises the bits. |
| |
| Option B introduces a maximum 10~12 msec penalty delay in the interrupt |
| handler. However, it could only punish the jack detection by extra |
| 10~12 msec. |
| |
| Adopts option B which is the better solution overall. |
| |
| Signed-off-by: Tzung-Bi Shih <tzungbi@google.com> |
| Link: https://lore.kernel.org/r/20191122073114.219945-4-tzungbi@google.com |
| Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> |
| Signed-off-by: Mark Brown <broonie@kernel.org> |
| Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> |
| |
| diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c |
| index f6bf4cfbea23..45da2b51543e 100644 |
| --- a/sound/soc/codecs/max98090.c |
| +++ b/sound/soc/codecs/max98090.c |
| @@ -2103,10 +2103,8 @@ static void max98090_pll_det_disable_work(struct work_struct *work) |
| M98090_IULK_MASK, 0); |
| } |
| |
| -static void max98090_pll_work(struct work_struct *work) |
| +static void max98090_pll_work(struct max98090_priv *max98090) |
| { |
| - struct max98090_priv *max98090 = |
| - container_of(work, struct max98090_priv, pll_work); |
| struct snd_soc_component *component = max98090->component; |
| |
| if (!snd_soc_component_is_active(component)) |
| @@ -2259,7 +2257,7 @@ static irqreturn_t max98090_interrupt(int irq, void *data) |
| |
| if (active & M98090_ULK_MASK) { |
| dev_dbg(component->dev, "M98090_ULK_MASK\n"); |
| - schedule_work(&max98090->pll_work); |
| + max98090_pll_work(max98090); |
| } |
| |
| if (active & M98090_JDET_MASK) { |
| @@ -2422,7 +2420,6 @@ static int max98090_probe(struct snd_soc_component *component) |
| max98090_pll_det_enable_work); |
| INIT_WORK(&max98090->pll_det_disable_work, |
| max98090_pll_det_disable_work); |
| - INIT_WORK(&max98090->pll_work, max98090_pll_work); |
| |
| /* Enable jack detection */ |
| snd_soc_component_write(component, M98090_REG_JACK_DETECT, |
| @@ -2475,7 +2472,6 @@ static void max98090_remove(struct snd_soc_component *component) |
| cancel_delayed_work_sync(&max98090->jack_work); |
| cancel_delayed_work_sync(&max98090->pll_det_enable_work); |
| cancel_work_sync(&max98090->pll_det_disable_work); |
| - cancel_work_sync(&max98090->pll_work); |
| max98090->component = NULL; |
| } |
| |
| diff --git a/sound/soc/codecs/max98090.h b/sound/soc/codecs/max98090.h |
| index 57965cd678b4..a197114b0dad 100644 |
| --- a/sound/soc/codecs/max98090.h |
| +++ b/sound/soc/codecs/max98090.h |
| @@ -1530,7 +1530,6 @@ struct max98090_priv { |
| struct delayed_work jack_work; |
| struct delayed_work pll_det_enable_work; |
| struct work_struct pll_det_disable_work; |
| - struct work_struct pll_work; |
| struct snd_soc_jack *jack; |
| unsigned int dai_fmt; |
| int tdm_slots; |
| -- |
| 2.7.4 |
| |