From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To: Tzung-Bi Shih <tzungbi@google.com>
Cc: ALSA development <alsa-devel@alsa-project.org>,
Mark Brown <broonie@kernel.org>, Dylan Reid <dgreid@google.com>,
Jimmy Cheng-Yi Chiang <cychiang@google.com>
Subject: Re: [alsa-devel] [PATCH 3/3] ASoC: max98090: fix possible race conditions
Date: Thu, 21 Nov 2019 09:08:51 -0600 [thread overview]
Message-ID: <d97b9724-27d0-f157-ce5d-ed6c75bafae8@linux.intel.com> (raw)
In-Reply-To: <CA+Px+wX2UtU3As-3Bf4rWn1kzqDMPw60xk508gDDaOPk6Q5dgw@mail.gmail.com>
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@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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
next prev parent reply other threads:[~2019-11-21 15:42 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-20 6:02 [alsa-devel] [PATCH 0/3] ASoC: max98090: fix PLL unlocked workaround-related Tzung-Bi Shih
2019-11-20 6:02 ` [alsa-devel] [PATCH 1/3] ASoC: max98090: remove msleep in PLL unlocked workaround Tzung-Bi Shih
2019-11-20 14:33 ` Pierre-Louis Bossart
2019-11-21 2:37 ` Tzung-Bi Shih
2019-11-20 6:02 ` [alsa-devel] [PATCH 2/3] ASoC: max98090: exit workaround earlier if PLL is locked Tzung-Bi Shih
2019-11-20 6:02 ` [alsa-devel] [PATCH 3/3] ASoC: max98090: fix possible race conditions Tzung-Bi Shih
2019-11-20 14:32 ` Pierre-Louis Bossart
2019-11-21 2:19 ` Tzung-Bi Shih
2019-11-21 3:20 ` Pierre-Louis Bossart
2019-11-21 6:17 ` Tzung-Bi Shih
2019-11-21 15:08 ` Pierre-Louis Bossart [this message]
2019-11-21 16:49 ` Tzung-Bi Shih
2019-11-21 17:20 ` Pierre-Louis Bossart
2019-11-22 7:52 ` Tzung-Bi Shih
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d97b9724-27d0-f157-ce5d-ed6c75bafae8@linux.intel.com \
--to=pierre-louis.bossart@linux.intel.com \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=cychiang@google.com \
--cc=dgreid@google.com \
--cc=tzungbi@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).