On Thu, Nov 21, 2019 at 11:20 AM Pierre-Louis Bossart pierre-louis.bossart@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.h...
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); } }
I guess we have 2 choices: i. stick to the original plan: option B, remove the second thread of execution. In the case, we would be punished by at most 10~12ms delay in the interrupt handler thread. I browsed the max98090_interrupt() and tend to think it should be fine if we delay jack detection for extra 10ms.
ii. adopt option A, play with the masks; and drop the previous patch to avoid multiple threads access 0x01 We would be forced to wait >10ms before re-enabling ULK interrupt. Notably, Documentation/timers/timers-howto.txt states "msleep(1~20) may not do what the caller intends, and will often sleep longer".Documentation/timers/timers-howto.txt".
Either way should be fine. Which one would you suggest?