[alsa-devel] [PATCH 3/3] ASoC: max98090: fix possible race conditions

[Tzung-Bi Shih]

On Thu, Nov 21, 2019 at 12:10 AM Pierre-Louis Bossart
<pierre-louis.bossart at linux.intel.com> wrote:
> On 11/20/19 12:02 AM, Tzung-Bi Shih wrote:
> > 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.
>
> if you re-test that the PLL is already running, then you can exit the
> work function immediately without redoing the sequence?

ACK.

> maybe also play with the masks so that the PLL unlock is masked in the
> interrupt and unmasked after the PLL locks?

ACK, this is our option A mentioned below.

> > 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 0x1
> >                          assert ULK == 0 (2).
>
> what are those 0x01 and 0x1? is the second a typo possibly?

ACK, a typo.

> > 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.
> >
> > There are 2 possible 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.
> >
> > Adopts option B which is more straightforward.
>
> but has the side effect of possibly adding a 10ms delay in the interrupt
> thread?

(forgot to mention) Option A cannot fix the case (2) race condition:
there would be 2 threads read the same clear-on-read register.  In
theory, the hardware should faster than CPUs' accesses via I2C.
max98090 should returns ULK=1 any time if PLL is unlocked.  Shall we
ignore the case (2) and adopt option A?