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

[Tzung-Bi Shih]

On Thu, Nov 21, 2019 at 11:41 PM Pierre-Louis Bossart
<pierre-louis.bossart at linux.intel.com> wrote:
> On 11/21/19 12:17 AM, Tzung-Bi Shih wrote:
> > On Thu, Nov 21, 2019 at 11:20 AM Pierre-Louis Bossart
> > <pierre-louis.bossart at linux.intel.com> wrote:
> >> Still you may want to clarify your second point and the commit message.
> >> The first race condition was clear, the second one not so much.
> >>
> >> what did you mean with 'assert ULK == 0' and what does this map to in
> >> pll_work
> >
> > The case (2) race condition is based on the previous patch:
> > https://mailman.alsa-project.org/pipermail/alsa-devel/2019-November/158852.html
>
> ah, ok, so that's a race you introduced :-)
>
> >
> > After applying the patch, the code would be something like: (in the
> > case, two threads read 0x01.)
> > static void max98090_pll_work(struct work_struct *work)
> > {
> > [snip]
> >          snd_soc_component_update_bits(component, M98090_REG_DEVICE_SHUTDOWN,
> >                              M98090_SHDNN_MASK, 0);
> >          snd_soc_component_update_bits(component, M98090_REG_DEVICE_SHUTDOWN,
> >                              M98090_SHDNN_MASK, M98090_SHDNN_MASK);
> >
> >          for (i = 0; i < 10; ++i) {
> >                  /* Check lock status */
> >                  pll = snd_soc_component_read32(
> >                                  component, M98090_REG_DEVICE_STATUS);
> >                  if (!(pll & M98090_ULK_MASK))
> >                          break;
> >
> >                  /* Give PLL time to lock */
> >                  usleep_range(1000, 1200);
> >          }
> > }
>
> Sorry, I don't see at what point the race happens.
>
> if your interrupt sees an ULK status, it will schedule the pll_work.
> You only test the status again *after* switching the device on/off, so
> what is the side effect?

Both interrupt thread and pll_work thread access the status register.
The side effect is: interrupt thread could read the status register
right before pll_work thread's read.

> I am also wondering if you shouldn't sleep first in the loop, then check
> the status?

ACK, "sleep first and check the status" makes more sense.

> And in case you do have a side effect due to the clear on read, maybe
> you could save the status in a context and retest in the workqueue, that
> way you'd have a trace of the ULK event even if the register was cleared.

I think cache the status couldn't be a good idea.  We couldn't make
sure the freshness of the cache.


Can we suppose CPUs always get ULK=1 if PLL is unlocked so that we can
simply ignore the case (2)?  Since we know the register is volatile
and read the register via I2C should be much slower than hardware
raise the bit.