On Tue, Dec 15, 2020 at 01:47:24PM +0100, Mark Kettenis wrote:
> > Date: Tue, 15 Dec 2020 13:32:22 +0100
> > From: Claudio Jeker <cje...@diehard.n-r-g.com>
> >
> > On Fri, Dec 11, 2020 at 07:07:56PM -0600, Scott Cheloha wrote:
> > > Hi,
> > >
> > > I'd like to remove lbolt from the kernel. I think having it in the
> > > kernel complicates otherwise simple code.
> > >
> > > We can start with sdmmc(4).
> > >
> > > The goal in sdmmc_io_function_enable() is calling
> > > sdmmc_io_function_ready()
> > > up to six times and sleep 1 second between each attempt. Here's rewritten
> > > code that does with without lbolt.
> > >
> > > ok?
> > >
> > > Index: sdmmc_io.c
> > > ===================================================================
> > > RCS file: /cvs/src/sys/dev/sdmmc/sdmmc_io.c,v
> > > retrieving revision 1.41
> > > diff -u -p -r1.41 sdmmc_io.c
> > > --- sdmmc_io.c 31 Dec 2019 10:05:33 -0000 1.41
> > > +++ sdmmc_io.c 12 Dec 2020 01:04:59 -0000
> > > @@ -231,8 +231,8 @@ sdmmc_io_function_enable(struct sdmmc_fu
> > > {
> > > struct sdmmc_softc *sc = sf->sc;
> > > struct sdmmc_function *sf0 = sc->sc_fn0;
> > > + int chan, retry = 5;
> > > u_int8_t rv;
> > > - int retry = 5;
> > >
> > > rw_assert_wrlock(&sc->sc_lock);
> > >
> > > @@ -244,7 +244,7 @@ sdmmc_io_function_enable(struct sdmmc_fu
> > > sdmmc_io_write_1(sf0, SD_IO_CCCR_FN_ENABLE, rv);
> > >
> > > while (!sdmmc_io_function_ready(sf) && retry-- > 0)
> > > - tsleep_nsec(&lbolt, PPAUSE, "pause", INFSLP);
> > > + tsleep_nsec(&chan, PPAUSE, "pause", SEC_TO_NSEC(1));
> > > return (retry >= 0) ? 0 : ETIMEDOUT;
> > > }
> > >
> >
> > Why not use &retry as wait channel instead of adding a new variable
> > chan? Result is the same. Would it make sense to allow NULL as wait
> > channel to make the tsleep not wakeable. At least that could be used in a
> > few places where timeouts are implemented with tsleep and would make the
> > intent more obvious.
>
> Or have an appropriately named global variable? Something like "int nowake"?
Something like the attached patch?
I think the idea of a "dead channel" communicates the intent. Nobody
broadcasts wakeups on the dead channel. If you don't want to receive
wakeup broadcasts you sleep on the dead channel. Hence, "deadchan".
Index: kern/kern_synch.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_synch.c,v
retrieving revision 1.172
diff -u -p -r1.172 kern_synch.c
--- kern/kern_synch.c 7 Dec 2020 16:55:29 -0000 1.172
+++ kern/kern_synch.c 16 Dec 2020 18:50:12 -0000
@@ -87,6 +87,12 @@ sleep_queue_init(void)
TAILQ_INIT(&slpque[i]);
}
+/*
+ * Threads that do not want to receive wakeup(9) broadcasts should
+ * sleep on deadchan.
+ */
+static int __deadchan;
+int *deadchan = &__deadchan;
/*
* During autoconfiguration or after a panic, a sleep will simply
Index: sys/systm.h
===================================================================
RCS file: /cvs/src/sys/sys/systm.h,v
retrieving revision 1.148
diff -u -p -r1.148 systm.h
--- sys/systm.h 26 Aug 2020 03:29:07 -0000 1.148
+++ sys/systm.h 16 Dec 2020 18:50:12 -0000
@@ -107,6 +107,8 @@ extern struct vnode *rootvp; /* vnode eq
extern dev_t swapdev; /* swapping device */
extern struct vnode *swapdev_vp;/* vnode equivalent to above */
+extern int *deadchan; /* dead wakeup(9) channel */
+
struct proc;
struct process;
#define curproc curcpu()->ci_curproc
Index: dev/sdmmc/sdmmc_io.c
===================================================================
RCS file: /cvs/src/sys/dev/sdmmc/sdmmc_io.c,v
retrieving revision 1.41
diff -u -p -r1.41 sdmmc_io.c
--- dev/sdmmc/sdmmc_io.c 31 Dec 2019 10:05:33 -0000 1.41
+++ dev/sdmmc/sdmmc_io.c 16 Dec 2020 18:50:12 -0000
@@ -244,7 +244,7 @@ sdmmc_io_function_enable(struct sdmmc_fu
sdmmc_io_write_1(sf0, SD_IO_CCCR_FN_ENABLE, rv);
while (!sdmmc_io_function_ready(sf) && retry-- > 0)
- tsleep_nsec(&lbolt, PPAUSE, "pause", INFSLP);
+ tsleep_nsec(deadchan, PPAUSE, "pause", SEC_TO_NSEC(1));
return (retry >= 0) ? 0 : ETIMEDOUT;
}
