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@xxxxxxxxxxxxxxx> 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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
