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

[Pierre-Louis Bossart]

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?

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

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.