lkml.org 
[lkml]   [2020]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.4 006/165] ASoC: max98090: fix possible race conditions
    Date
    From: Tzung-Bi Shih <tzungbi@google.com>

    [ Upstream commit 45dfbf56975994822cce00b7475732a49f8aefed ]

    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: Sasha Levin <sashal@kernel.org>
    ---
    sound/soc/codecs/max98090.c | 8 ++------
    sound/soc/codecs/max98090.h | 1 -
    2 files changed, 2 insertions(+), 7 deletions(-)

    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.20.1


    \
     
     \ /
      Last update: 2020-01-11 11:21    [W:4.379 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site