git: 35547df5c786 - main - Call wakeup() with the lock held to avoid missed wakeup races.

[Luiz Otavio O Souza]

On Wed, Aug 11, 2021 at 3:49 PM John Baldwin <jhb at freebsd.org> wrote:
>
> On 8/10/21 3:41 PM, Scott Long wrote:
> > The branch main has been updated by scottl:
> >
> > URL: https://cgit.FreeBSD.org/src/commit/?id=35547df5c78653b2da030f920323c0357056099f
> >
> > commit 35547df5c78653b2da030f920323c0357056099f
> > Author:     Scott Long <scottl at FreeBSD.org>
> > AuthorDate: 2021-08-10 22:36:38 +0000
> > Commit:     Scott Long <scottl at FreeBSD.org>
> > CommitDate: 2021-08-10 22:36:38 +0000
> >
> >      Call wakeup() with the lock held to avoid missed wakeup races.
> >
> >      Submitted by: luiz
> >      Sponsored by: Rubicon Communications, LLC ("Netgate")
> > ---
> >   sys/dev/sdhci/sdhci.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/sys/dev/sdhci/sdhci.c b/sys/dev/sdhci/sdhci.c
> > index d075c2e05000..573e6949b57e 100644
> > --- a/sys/dev/sdhci/sdhci.c
> > +++ b/sys/dev/sdhci/sdhci.c
> > @@ -2078,8 +2078,8 @@ sdhci_generic_release_host(device_t brdev __unused, device_t reqdev)
> >       /* Deactivate led. */
> >       WR1(slot, SDHCI_HOST_CONTROL, slot->hostctrl &= ~SDHCI_CTRL_LED);
> >       slot->bus_busy--;
> > -     SDHCI_UNLOCK(slot);
> >       wakeup(slot);
> > +     SDHCI_UNLOCK(slot);
> >       return (0);
> >   }
>
> Hmm, how does this avoid a race?  The sleep is checking bus_busy under
> the lock and should never see a stale value and go back to sleep after
> the wakeup has occurred:
>
>         SDHCI_LOCK(slot);
>         while (slot->bus_busy)
>                 msleep(slot, &slot->mtx, 0, "sdhciah", 0);
>         slot->bus_busy++;
>         /* Activate led. */
>         WR1(slot, SDHCI_HOST_CONTROL, slot->hostctrl |= SDHCI_CTRL_LED);
>         SDHCI_UNLOCK(slot);
>
> Dropping the lock before wakeup() is a tiny optimization that avoids
> having the second thread wakeup and immediately block on the lock before
> it has been released by the first thread.
>

'race' is probably wrong here.  this change will prevent a second
thread from taking the bus before you call wakeup() - poking all other
threads unnecessarily.

luiz