ASoC: max98090: fix possible race conditions

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>

diff --git a/sound/soc/codecs/max98090.c b/sound/soc/codecs/max98090.c
index f531e5a..e46b6ad 100644 (file)
--- 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;
        unsigned int pll;
        int i;
@@ -2275,7 +2273,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) {
@@ -2438,7 +2436,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,
@@ -2491,7 +2488,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 57965cd..a197114 100644 (file)
--- 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;