Re: mbg(4): tsleep(9) -> tsleep_nsec(9)
On Fri, Dec 04, 2020 at 12:08:39PM -0600, Scott Cheloha wrote:
> On Fri, Dec 04, 2020 at 10:07:07AM +0100, Claudio Jeker wrote:
> > On Thu, Dec 03, 2020 at 10:42:50PM -0600, Scott Cheloha wrote:
> > > Hi,
> > >
> > > mbg(4) is among the few remaining drivers using tsleep(9).
> > >
> > > In a few spots, when the kernel is not cold, the driver will spin for
> > > up to 1/10 seconds waiting for the MBG_BUSY flag to go low.
> > >
> > > We can approximate this behavior by spinning 10 times and sleeping 10
> > > milliseconds each iteration. 10 x 10ms = 100ms = 1/10 seconds.
> > >
> > > I can't test this but I was able to compile it on amd64. It isn't
> > > normally built for amd64, though. Just i386.
> > >
> > > I have my doubts that anyone has this card and is able to actually
> > > test this diff.
> > >
> > > Anybody ok?
> >
> > This code needs to wait for around 70us for the card to process the
> > command (according to the comment). The cold code sleeps a max of
> > 50 * 20us (1ms). I don't see why the regular code should sleep so much
> > longer. I would suggest to use a 1ms timeout and loop 10 times. This is a
> > magnitude more than enough and most probably only one cycle will be
> > needed.
> >
> > IIRC someone got a mbg(4) device some time ago apart from mbalmer@
>
> Makes sense to me. Updated diff attached.
OK claudio@
> How are we going to find this person?
Best way is to commit the diff and wait :)
> Index: mbg.c
> ===
> RCS file: /cvs/src/sys/dev/pci/mbg.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 mbg.c
> --- mbg.c 29 Nov 2020 03:17:27 - 1.31
> +++ mbg.c 4 Dec 2020 18:07:43 -
> @@ -417,12 +417,12 @@ mbg_read_amcc_s5920(struct mbg_softc *sc
>
> /* wait for the BUSY flag to go low (approx 70 us on i386) */
> timer = 0;
> - tmax = cold ? 50 : hz / 10;
> + tmax = cold ? 50 : 10;
> do {
> if (cold)
> delay(20);
> else
> - tsleep(tstamp, 0, "mbg", 1);
> + tsleep_nsec(tstamp, 0, "mbg", MSEC_TO_NSEC(1));
> status = bus_space_read_1(sc->sc_iot, sc->sc_ioh,
> AMCC_IMB4 + 3);
> } while ((status & MBG_BUSY) && timer++ < tmax);
> @@ -473,12 +473,12 @@ mbg_read_amcc_s5933(struct mbg_softc *sc
>
> /* wait for the BUSY flag to go low (approx 70 us on i386) */
> timer = 0;
> - tmax = cold ? 50 : hz / 10;
> + tmax = cold ? 50 : 10;
> do {
> if (cold)
> delay(20);
> else
> - tsleep(tstamp, 0, "mbg", 1);
> + tsleep_nsec(tstamp, 0, "mbg", MSEC_TO_NSEC(1));
> status = bus_space_read_1(sc->sc_iot, sc->sc_ioh,
> AMCC_IMB4 + 3);
> } while ((status & MBG_BUSY) && timer++ < tmax);
> @@ -525,12 +525,12 @@ mbg_read_asic(struct mbg_softc *sc, int
>
> /* wait for the BUSY flag to go low */
> timer = 0;
> - tmax = cold ? 50 : hz / 10;
> + tmax = cold ? 50 : 10;
> do {
> if (cold)
> delay(20);
> else
> - tsleep(tstamp, 0, "mbg", 1);
> + tsleep_nsec(tstamp, 0, "mbg", MSEC_TO_NSEC(1));
> status = bus_space_read_1(sc->sc_iot, sc->sc_ioh, ASIC_STATUS);
> } while ((status & MBG_BUSY) && timer++ < tmax);
>
--
:wq Claudio
Re: mbg(4): tsleep(9) -> tsleep_nsec(9)
On Fri, Dec 04, 2020 at 10:07:07AM +0100, Claudio Jeker wrote:
> On Thu, Dec 03, 2020 at 10:42:50PM -0600, Scott Cheloha wrote:
> > Hi,
> >
> > mbg(4) is among the few remaining drivers using tsleep(9).
> >
> > In a few spots, when the kernel is not cold, the driver will spin for
> > up to 1/10 seconds waiting for the MBG_BUSY flag to go low.
> >
> > We can approximate this behavior by spinning 10 times and sleeping 10
> > milliseconds each iteration. 10 x 10ms = 100ms = 1/10 seconds.
> >
> > I can't test this but I was able to compile it on amd64. It isn't
> > normally built for amd64, though. Just i386.
> >
> > I have my doubts that anyone has this card and is able to actually
> > test this diff.
> >
> > Anybody ok?
>
> This code needs to wait for around 70us for the card to process the
> command (according to the comment). The cold code sleeps a max of
> 50 * 20us (1ms). I don't see why the regular code should sleep so much
> longer. I would suggest to use a 1ms timeout and loop 10 times. This is a
> magnitude more than enough and most probably only one cycle will be
> needed.
>
> IIRC someone got a mbg(4) device some time ago apart from mbalmer@
Makes sense to me. Updated diff attached.
How are we going to find this person?
Index: mbg.c
===
RCS file: /cvs/src/sys/dev/pci/mbg.c,v
retrieving revision 1.31
diff -u -p -r1.31 mbg.c
--- mbg.c 29 Nov 2020 03:17:27 - 1.31
+++ mbg.c 4 Dec 2020 18:07:43 -
@@ -417,12 +417,12 @@ mbg_read_amcc_s5920(struct mbg_softc *sc
/* wait for the BUSY flag to go low (approx 70 us on i386) */
timer = 0;
- tmax = cold ? 50 : hz / 10;
+ tmax = cold ? 50 : 10;
do {
if (cold)
delay(20);
else
- tsleep(tstamp, 0, "mbg", 1);
+ tsleep_nsec(tstamp, 0, "mbg", MSEC_TO_NSEC(1));
status = bus_space_read_1(sc->sc_iot, sc->sc_ioh,
AMCC_IMB4 + 3);
} while ((status & MBG_BUSY) && timer++ < tmax);
@@ -473,12 +473,12 @@ mbg_read_amcc_s5933(struct mbg_softc *sc
/* wait for the BUSY flag to go low (approx 70 us on i386) */
timer = 0;
- tmax = cold ? 50 : hz / 10;
+ tmax = cold ? 50 : 10;
do {
if (cold)
delay(20);
else
- tsleep(tstamp, 0, "mbg", 1);
+ tsleep_nsec(tstamp, 0, "mbg", MSEC_TO_NSEC(1));
status = bus_space_read_1(sc->sc_iot, sc->sc_ioh,
AMCC_IMB4 + 3);
} while ((status & MBG_BUSY) && timer++ < tmax);
@@ -525,12 +525,12 @@ mbg_read_asic(struct mbg_softc *sc, int
/* wait for the BUSY flag to go low */
timer = 0;
- tmax = cold ? 50 : hz / 10;
+ tmax = cold ? 50 : 10;
do {
if (cold)
delay(20);
else
- tsleep(tstamp, 0, "mbg", 1);
+ tsleep_nsec(tstamp, 0, "mbg", MSEC_TO_NSEC(1));
status = bus_space_read_1(sc->sc_iot, sc->sc_ioh, ASIC_STATUS);
} while ((status & MBG_BUSY) && timer++ < tmax);
Re: mbg(4): tsleep(9) -> tsleep_nsec(9)
On Thu, Dec 03, 2020 at 10:42:50PM -0600, Scott Cheloha wrote:
> Hi,
>
> mbg(4) is among the few remaining drivers using tsleep(9).
>
> In a few spots, when the kernel is not cold, the driver will spin for
> up to 1/10 seconds waiting for the MBG_BUSY flag to go low.
>
> We can approximate this behavior by spinning 10 times and sleeping 10
> milliseconds each iteration. 10 x 10ms = 100ms = 1/10 seconds.
>
> I can't test this but I was able to compile it on amd64. It isn't
> normally built for amd64, though. Just i386.
>
> I have my doubts that anyone has this card and is able to actually
> test this diff.
>
> Anybody ok?
This code needs to wait for around 70us for the card to process the
command (according to the comment). The cold code sleeps a max of
50 * 20us (1ms). I don't see why the regular code should sleep so much
longer. I would suggest to use a 1ms timeout and loop 10 times. This is a
magnitude more than enough and most probably only one cycle will be
needed.
IIRC someone got a mbg(4) device some time ago apart from mbalmer@
> Index: mbg.c
> ===
> RCS file: /cvs/src/sys/dev/pci/mbg.c,v
> retrieving revision 1.31
> diff -u -p -r1.31 mbg.c
> --- mbg.c 29 Nov 2020 03:17:27 - 1.31
> +++ mbg.c 4 Dec 2020 04:39:59 -
> @@ -417,12 +417,12 @@ mbg_read_amcc_s5920(struct mbg_softc *sc
>
> /* wait for the BUSY flag to go low (approx 70 us on i386) */
> timer = 0;
> - tmax = cold ? 50 : hz / 10;
> + tmax = cold ? 50 : 10;
> do {
> if (cold)
> delay(20);
> else
> - tsleep(tstamp, 0, "mbg", 1);
> + tsleep_nsec(tstamp, 0, "mbg", MSEC_TO_NSEC(10));
> status = bus_space_read_1(sc->sc_iot, sc->sc_ioh,
> AMCC_IMB4 + 3);
> } while ((status & MBG_BUSY) && timer++ < tmax);
> @@ -473,12 +473,12 @@ mbg_read_amcc_s5933(struct mbg_softc *sc
>
> /* wait for the BUSY flag to go low (approx 70 us on i386) */
> timer = 0;
> - tmax = cold ? 50 : hz / 10;
> + tmax = cold ? 50 : 10;
> do {
> if (cold)
> delay(20);
> else
> - tsleep(tstamp, 0, "mbg", 1);
> + tsleep_nsec(tstamp, 0, "mbg", MSEC_TO_NSEC(10));
> status = bus_space_read_1(sc->sc_iot, sc->sc_ioh,
> AMCC_IMB4 + 3);
> } while ((status & MBG_BUSY) && timer++ < tmax);
> @@ -525,12 +525,12 @@ mbg_read_asic(struct mbg_softc *sc, int
>
> /* wait for the BUSY flag to go low */
> timer = 0;
> - tmax = cold ? 50 : hz / 10;
> + tmax = cold ? 50 : 10;
> do {
> if (cold)
> delay(20);
> else
> - tsleep(tstamp, 0, "mbg", 1);
> + tsleep_nsec(tstamp, 0, "mbg", MSEC_TO_NSEC(10));
> status = bus_space_read_1(sc->sc_iot, sc->sc_ioh, ASIC_STATUS);
> } while ((status & MBG_BUSY) && timer++ < tmax);
>
>
--
:wq Claudio
Re: an(4): tsleep(9) -> tsleep_nsec(9)
On Thu, Nov 26, 2020 at 08:25:48PM +1100, Jonathan Gray wrote:
> On Tue, Nov 24, 2020 at 07:20:46PM -0600, Scott Cheloha wrote:
> > Hi,
> >
> > Both kettenis@ and mpi@ have mentioned in private that my proposed
> > changes to tsleep_nsec(9) etc. would be nicer if we could just get rid
> > of tsleep(9) etc. entirely.
> >
> > This is difficult, but I'll try.
> >
> > Worst case, we thin out the remaining callers. There are not many
> > left.
> >
> > --
> >
> > So, an(4) is one such caller.
> >
> > In an_wait() we spin for (3 * hz) ticks waiting for CSR_WRITE_2 to
> > return the AN_EV_CMD flag. There is no code handling a case where
> > this fails to happen.
> >
> > What we do in practice is very nearly equivalent to spinning for 3
> > seconds waiting for CSR_WRITE_2 to return the AN_EV_CMD flag, so I
> > have converted it to use tsleep_nsec(9).
> >
> > This compiles on amd64 but I can't test it.
> >
> > Thoughts? ok?
>
> I don't see why the upper bound would have to be so precise.
>
> Why not just
>
> for (i = 0; i < 3000; i += 100) {
> if (CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD)
> break;
> tsleep_nsec(sc, PWAIT, "anatch", MSEC_TO_NSEC(100));
> }
I was just trying to imitate the current behavior as closely as
possible.
If you're fine fudging it like that then I'm fine with it. Just
beware that that tsleep_nsec(9) can and will oversleep by up to 1/hz
seconds.
Did you intend to increase the poll interval from 10ms to 100ms or is
that a typo?
Re: an(4): tsleep(9) -> tsleep_nsec(9)
On Thu, Nov 26, 2020 at 08:25:48PM +1100, Jonathan Gray wrote:
> On Tue, Nov 24, 2020 at 07:20:46PM -0600, Scott Cheloha wrote:
> > Hi,
> >
> > Both kettenis@ and mpi@ have mentioned in private that my proposed
> > changes to tsleep_nsec(9) etc. would be nicer if we could just get rid
> > of tsleep(9) etc. entirely.
> >
> > This is difficult, but I'll try.
> >
> > Worst case, we thin out the remaining callers. There are not many
> > left.
> >
> > --
> >
> > So, an(4) is one such caller.
> >
> > In an_wait() we spin for (3 * hz) ticks waiting for CSR_WRITE_2 to
> > return the AN_EV_CMD flag. There is no code handling a case where
> > this fails to happen.
> >
> > What we do in practice is very nearly equivalent to spinning for 3
> > seconds waiting for CSR_WRITE_2 to return the AN_EV_CMD flag, so I
> > have converted it to use tsleep_nsec(9).
> >
> > This compiles on amd64 but I can't test it.
> >
> > Thoughts? ok?
>
> I don't see why the upper bound would have to be so precise.
>
> Why not just
>
> for (i = 0; i < 3000; i += 100) {
> if (CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD)
> break;
> tsleep_nsec(sc, PWAIT, "anatch", MSEC_TO_NSEC(100));
> }
>
Agreed, that seems more in the spirit of such wait loops.
> >
> > Index: an.c
> > ===
> > RCS file: /cvs/src/sys/dev/ic/an.c,v
> > retrieving revision 1.76
> > diff -u -p -r1.76 an.c
> > --- an.c10 Jul 2020 13:26:37 - 1.76
> > +++ an.c25 Nov 2020 01:19:16 -
> > @@ -678,13 +678,18 @@ an_linkstat_intr(struct an_softc *sc)
> > void
> > an_wait(struct an_softc *sc)
> > {
> > - int i;
> > + struct timespec now, end;
> > +
> > + nanouptime();
> > + end = now;
> > + end.tv_sec += 3;/* spin for at most three seconds */
> >
> > CSR_WRITE_2(sc, AN_COMMAND, AN_CMD_NOOP2);
> > - for (i = 0; i < 3*hz; i++) {
> > - if (CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD)
> > + while ((CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD) == 0) {
> > + nanouptime();
> > + if (timespeccmp(, , <=))
> > break;
> > - (void)tsleep(sc, PWAIT, "anatch", 1);
> > + tsleep_nsec(sc, PWAIT, "anatch", MSEC_TO_NSEC(10));
> > }
> > CSR_WRITE_2(sc, AN_EVENT_ACK, AN_EV_CMD);
> > }
> >
> >
>
--
:wq Claudio
Re: an(4): tsleep(9) -> tsleep_nsec(9)
On Tue, Nov 24, 2020 at 07:20:46PM -0600, Scott Cheloha wrote:
> Hi,
>
> Both kettenis@ and mpi@ have mentioned in private that my proposed
> changes to tsleep_nsec(9) etc. would be nicer if we could just get rid
> of tsleep(9) etc. entirely.
>
> This is difficult, but I'll try.
>
> Worst case, we thin out the remaining callers. There are not many
> left.
>
> --
>
> So, an(4) is one such caller.
>
> In an_wait() we spin for (3 * hz) ticks waiting for CSR_WRITE_2 to
> return the AN_EV_CMD flag. There is no code handling a case where
> this fails to happen.
>
> What we do in practice is very nearly equivalent to spinning for 3
> seconds waiting for CSR_WRITE_2 to return the AN_EV_CMD flag, so I
> have converted it to use tsleep_nsec(9).
>
> This compiles on amd64 but I can't test it.
>
> Thoughts? ok?
I don't see why the upper bound would have to be so precise.
Why not just
for (i = 0; i < 3000; i += 100) {
if (CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD)
break;
tsleep_nsec(sc, PWAIT, "anatch", MSEC_TO_NSEC(100));
}
>
> Index: an.c
> ===
> RCS file: /cvs/src/sys/dev/ic/an.c,v
> retrieving revision 1.76
> diff -u -p -r1.76 an.c
> --- an.c 10 Jul 2020 13:26:37 - 1.76
> +++ an.c 25 Nov 2020 01:19:16 -
> @@ -678,13 +678,18 @@ an_linkstat_intr(struct an_softc *sc)
> void
> an_wait(struct an_softc *sc)
> {
> - int i;
> + struct timespec now, end;
> +
> + nanouptime();
> + end = now;
> + end.tv_sec += 3;/* spin for at most three seconds */
>
> CSR_WRITE_2(sc, AN_COMMAND, AN_CMD_NOOP2);
> - for (i = 0; i < 3*hz; i++) {
> - if (CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD)
> + while ((CSR_READ_2(sc, AN_EVENT_STAT) & AN_EV_CMD) == 0) {
> + nanouptime();
> + if (timespeccmp(, , <=))
> break;
> - (void)tsleep(sc, PWAIT, "anatch", 1);
> + tsleep_nsec(sc, PWAIT, "anatch", MSEC_TO_NSEC(10));
> }
> CSR_WRITE_2(sc, AN_EVENT_ACK, AN_EV_CMD);
> }
>
>
Re: wi(4): tsleep(9) -> tsleep_nsec(9)
On Mon, Apr 06, 2020 at 02:03:36PM -0500, Scott Cheloha wrote:
> Ticks to seconds. Trivial.
>
> ok?
ok stsp@
> Index: if_wi.c
> ===
> RCS file: /cvs/src/sys/dev/ic/if_wi.c,v
> retrieving revision 1.171
> diff -u -p -r1.171 if_wi.c
> --- if_wi.c 31 Dec 2019 10:05:32 - 1.171
> +++ if_wi.c 6 Apr 2020 18:29:33 -
> @@ -1865,8 +1865,8 @@ wi_ioctl(struct ifnet *ifp, u_long comma
> timeout_add(>wi_scan_timeout, len);
>
> /* Let the userspace process wait for completion */
> - error = tsleep(>wi_scan_lock, PCATCH, "wiscan",
> - hz * IEEE80211_SCAN_TIMEOUT);
> + error = tsleep_nsec(>wi_scan_lock, PCATCH, "wiscan",
> + SEC_TO_NSEC(IEEE80211_SCAN_TIMEOUT));
> break;
> case SIOCG80211ALLNODES:
> {
>
Re: dwiic(4): tsleep(9) -> tsleep_nsec(9)
On Fri, Jan 10, 2020 at 06:54:26PM -0600, Scott Cheloha wrote:
> Ticks to milliseconds.
>
> ok?
Bump and rebase.
1/2 seconds, so, 500 milliseconds.
ok?
Index: dwiic.c
===
RCS file: /cvs/src/sys/dev/ic/dwiic.c,v
retrieving revision 1.10
diff -u -p -r1.10 dwiic.c
--- dwiic.c 18 Feb 2020 12:13:39 - 1.10
+++ dwiic.c 20 Feb 2020 02:09:46 -
@@ -271,7 +271,8 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
dwiic_read(sc, DW_IC_CLR_INTR);
dwiic_write(sc, DW_IC_INTR_MASK, DW_IC_INTR_TX_EMPTY);
- if (tsleep(>sc_writewait, PRIBIO, "dwiic", hz / 2) != 0)
+ if (tsleep_nsec(>sc_writewait, PRIBIO, "dwiic",
+ MSEC_TO_NSEC(500)) != 0)
printf("%s: timed out waiting for tx_empty intr\n",
sc->sc_dev.dv_xname);
splx(s);
@@ -368,8 +369,8 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
dwiic_write(sc, DW_IC_INTR_MASK,
DW_IC_INTR_RX_FULL);
- if (tsleep(>sc_readwait, PRIBIO, "dwiic",
- hz / 2) != 0)
+ if (tsleep_nsec(>sc_readwait, PRIBIO,
"dwiic",
+ MSEC_TO_NSEC(500)) != 0)
printf("%s: timed out waiting for "
"rx_full intr\n",
sc->sc_dev.dv_xname);
@@ -433,8 +434,8 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
while (sc->sc_busy) {
dwiic_write(sc, DW_IC_INTR_MASK,
DW_IC_INTR_STOP_DET);
- if (tsleep(>sc_busy, PRIBIO, "dwiic",
- hz / 2) != 0)
+ if (tsleep_nsec(>sc_busy, PRIBIO, "dwiic",
+ MSEC_TO_NSEC(500)) != 0)
printf("%s: timed out waiting for "
"stop intr\n",
sc->sc_dev.dv_xname);
Re: ips(4): tsleep(9) -> tsleep_nsec(9)
On Mon, Feb 17, 2020 at 07:40:40PM -0500, Kenneth R Westerback wrote:
> On Mon, Feb 17, 2020 at 06:23:01PM -0600, Scott Cheloha wrote:
> > On Sat, Jan 11, 2020 at 02:57:14AM -0600, Scott Cheloha wrote:
> > > The sleep duration is in milliseconds. We can rip out the timeval and
> > > tvtohz(9) and use MSEC_TO_NSEC() directly instead. I've added a local
> > > "msecs" variable because "timo" (a) feels a bit ambiguous and (b) is
> > > used with different units earlier in the same function.
> > >
> > > [...]
> >
> > Bump and rebase.
> >
> > This is a straightforward ticks-to-milliseconds change. We also kill
> > off one of the remaining tvtohz(9) calls.
> >
> > ok?
>
> Looks good to me. But can't timo be deleted by using the new msec in
> the for loop above? ok krw@ either way.
timo is used in the SCSI_NOSLEEP path.
... while we're here we can pick a better variable name,
though.
Does this look alright to you? Instead of ten thousand
100-microsecond chunks we count out each of the million
microseconds in the for-loop.
Index: ips.c
===
RCS file: /cvs/src/sys/dev/pci/ips.c,v
retrieving revision 1.117
diff -u -p -r1.117 ips.c
--- ips.c 14 Feb 2020 18:37:03 - 1.117
+++ ips.c 18 Feb 2020 15:10:01 -
@@ -1409,8 +1409,7 @@ ips_cmd(struct ips_softc *sc, struct ips
int
ips_poll(struct ips_softc *sc, struct ips_ccb *ccb)
{
- struct timeval tv;
- int error, timo;
+ int error, msecs, usecs;
splassert(IPL_BIO);
@@ -1419,7 +1418,7 @@ ips_poll(struct ips_softc *sc, struct ip
DPRINTF(IPS_D_XFER, ("%s: ips_poll: busy-wait\n",
sc->sc_dev.dv_xname));
- for (timo = 1; timo > 0; timo--) {
+ for (usecs = 100; usecs > 0; usecs -= 100) {
delay(100);
ips_intr(sc);
if (ccb->c_state == IPS_CCB_DONE)
@@ -1427,14 +1426,11 @@ ips_poll(struct ips_softc *sc, struct ip
}
} else {
/* sleep */
- timo = ccb->c_xfer ? ccb->c_xfer->timeout : IPS_TIMEOUT;
- tv.tv_sec = timo / 1000;
- tv.tv_usec = (timo % 1000) * 1000;
- timo = tvtohz();
+ msecs = ccb->c_xfer ? ccb->c_xfer->timeout : IPS_TIMEOUT;
- DPRINTF(IPS_D_XFER, ("%s: ips_poll: sleep %d hz\n",
- sc->sc_dev.dv_xname, timo));
- tsleep(ccb, PRIBIO + 1, "ipscmd", timo);
+ DPRINTF(IPS_D_XFER, ("%s: ips_poll: sleep %d ms\n",
+ sc->sc_dev.dv_xname, msecs));
+ tsleep_nsec(ccb, PRIBIO + 1, "ipscmd", MSEC_TO_NSEC(msecs));
}
DPRINTF(IPS_D_XFER, ("%s: ips_poll: state %d\n", sc->sc_dev.dv_xname,
ccb->c_state));
Re: ips(4): tsleep(9) -> tsleep_nsec(9)
On Sat, Jan 11, 2020 at 02:57:14AM -0600, Scott Cheloha wrote:
> The sleep duration is in milliseconds. We can rip out the timeval and
> tvtohz(9) and use MSEC_TO_NSEC() directly instead. I've added a local
> "msecs" variable because "timo" (a) feels a bit ambiguous and (b) is
> used with different units earlier in the same function.
>
> [...]
Bump and rebase.
This is a straightforward ticks-to-milliseconds change. We also kill
off one of the remaining tvtohz(9) calls.
ok?
Index: ips.c
===
RCS file: /cvs/src/sys/dev/pci/ips.c,v
retrieving revision 1.117
diff -u -p -r1.117 ips.c
--- ips.c 14 Feb 2020 18:37:03 - 1.117
+++ ips.c 18 Feb 2020 00:21:48 -
@@ -1409,8 +1409,7 @@ ips_cmd(struct ips_softc *sc, struct ips
int
ips_poll(struct ips_softc *sc, struct ips_ccb *ccb)
{
- struct timeval tv;
- int error, timo;
+ int error, msecs, timo;
splassert(IPL_BIO);
@@ -1427,14 +1426,11 @@ ips_poll(struct ips_softc *sc, struct ip
}
} else {
/* sleep */
- timo = ccb->c_xfer ? ccb->c_xfer->timeout : IPS_TIMEOUT;
- tv.tv_sec = timo / 1000;
- tv.tv_usec = (timo % 1000) * 1000;
- timo = tvtohz();
+ msecs = ccb->c_xfer ? ccb->c_xfer->timeout : IPS_TIMEOUT;
- DPRINTF(IPS_D_XFER, ("%s: ips_poll: sleep %d hz\n",
- sc->sc_dev.dv_xname, timo));
- tsleep(ccb, PRIBIO + 1, "ipscmd", timo);
+ DPRINTF(IPS_D_XFER, ("%s: ips_poll: sleep %d ms\n",
+ sc->sc_dev.dv_xname, msecs));
+ tsleep_nsec(ccb, PRIBIO + 1, "ipscmd", MSEC_TO_NSEC(msecs));
}
DPRINTF(IPS_D_XFER, ("%s: ips_poll: state %d\n", sc->sc_dev.dv_xname,
ccb->c_state));
Re: sdhc(4): tsleep(9) -> tsleep_nsec(9)
On Fri, Jan 10, 2020 at 05:55:14PM -0600, Scott Cheloha wrote:
> Here the timeout constants are all in terms of seconds. We can change
> the input unit of the wait functions from hz to seconds and then
> convert as needed.
>
> sdhc_wait_intr_cold() uses delay(9), so convert to microseconds.
>
> sdhc_wait_intr() (now) uses tsleep_nsec(9), so convert to nanoseconds.
>
> I've sprinkled in some name changes and intermediate variables to make
> the units in use more obvious.
>
> ok?
Bump.
Index: sdmmc/sdhc.c
===
RCS file: /cvs/src/sys/dev/sdmmc/sdhc.c,v
retrieving revision 1.63
diff -u -p -r1.63 sdhc.c
--- sdmmc/sdhc.c22 Jan 2020 07:52:37 - 1.63
+++ sdmmc/sdhc.c7 Feb 2020 14:00:23 -
@@ -35,10 +35,11 @@
#include
#include
-#define SDHC_COMMAND_TIMEOUT hz
-#define SDHC_BUFFER_TIMEOUThz
-#define SDHC_TRANSFER_TIMEOUT hz
-#define SDHC_DMA_TIMEOUT (hz*3)
+/* Timeouts in seconds */
+#define SDHC_COMMAND_TIMEOUT 1
+#define SDHC_BUFFER_TIMEOUT1
+#define SDHC_TRANSFER_TIMEOUT 1
+#define SDHC_DMA_TIMEOUT 3
struct sdhc_host {
struct sdhc_softc *sc; /* host controller device */
@@ -1103,12 +1104,12 @@ sdhc_soft_reset(struct sdhc_host *hp, in
}
int
-sdhc_wait_intr_cold(struct sdhc_host *hp, int mask, int timo)
+sdhc_wait_intr_cold(struct sdhc_host *hp, int mask, int secs)
{
- int status;
+ int status, usecs;
mask |= SDHC_ERROR_INTERRUPT;
- timo = timo * tick;
+ usecs = secs * 100;
status = hp->intr_status;
while ((status & mask) == 0) {
@@ -1142,7 +1143,7 @@ sdhc_wait_intr_cold(struct sdhc_host *hp
}
delay(1);
- if (timo-- == 0) {
+ if (usecs-- == 0) {
status |= SDHC_ERROR_INTERRUPT;
break;
}
@@ -1153,20 +1154,22 @@ sdhc_wait_intr_cold(struct sdhc_host *hp
}
int
-sdhc_wait_intr(struct sdhc_host *hp, int mask, int timo)
+sdhc_wait_intr(struct sdhc_host *hp, int mask, int secs)
{
+ uint64_t nsecs;
int status;
int s;
if (cold)
- return (sdhc_wait_intr_cold(hp, mask, timo));
+ return (sdhc_wait_intr_cold(hp, mask, secs));
mask |= SDHC_ERROR_INTERRUPT;
+ nsecs = SEC_TO_NSEC(secs);
s = splsdmmc();
status = hp->intr_status & mask;
while (status == 0) {
- if (tsleep(>intr_status, PWAIT, "hcintr", timo)
+ if (tsleep_nsec(>intr_status, PWAIT, "hcintr", nsecs)
== EWOULDBLOCK) {
status |= SDHC_ERROR_INTERRUPT;
break;
Re: xbf(4): tsleep(9) -> tsleep_nsec(9)
Scott Cheloha writes: > Given the SCSI_NOSLEEP split here I think the simplest thing we can do > is ask to sleep as much as we delay(9). > > The question is: if you *could* poll in 10us intervals here with > tsleep_nsec(9), would you want to? If so, then this works. If > not, what is a more appropriate interval? > Hi, I believe it would be fine to use the same value as in the delay, "1" was just the smallest available for the tsleep. OK mikeb for the change. Cheers, Mike > Index: pv/xbf.c > === > RCS file: /cvs/src/sys/dev/pv/xbf.c,v > retrieving revision 1.32 > diff -u -p -r1.32 xbf.c > --- pv/xbf.c 17 Jul 2017 10:30:03 - 1.32 > +++ pv/xbf.c 15 Jan 2020 06:20:25 - > @@ -738,7 +738,7 @@ xbf_poll_cmd(struct scsi_xfer *xs) > if (ISSET(xs->flags, SCSI_NOSLEEP)) > delay(10); > else > - tsleep(xs, PRIBIO, "xbfpoll", 1); > + tsleep_nsec(xs, PRIBIO, "xbfpoll", USEC_TO_NSEC(10)); > xbf_intr(xs->sc_link->adapter_softc); > } while(--timo > 0); >
Re: qle(4): tsleep(9) -> tsleep_nsec(9)
On Mon, Jan 13, 2020 at 06:32:00PM -0600, Scott Cheloha wrote:
> These sleeps don't have units, though in practice they are 1 second.
> Just prior in the code I see a delay(9) of 100 microseconds. Is the
> 100 ticks here a typo? What is a reasonable sleep duration for this
> hardware?
Both of these are inside loops that terminate when the hardware (actually the
fibrechannel switch the hardware is connected to, in this case) responsds.
The tsleep length is arbitrary, so one second is as good as anything else.
ok jmatthew@
>
> Index: pci/qle.c
> ===
> RCS file: /cvs/src/sys/dev/pci/qle.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 qle.c
> --- pci/qle.c 31 Dec 2019 22:57:07 - 1.48
> +++ pci/qle.c 14 Jan 2020 00:29:34 -
> @@ -1886,7 +1886,8 @@ qle_ct_pass_through(struct qle_softc *sc
> if (qle_read_isr(sc, , ) != 0)
> qle_handle_intr(sc, isr, info);
> } else {
> - tsleep(sc->sc_scratch, PRIBIO, "qle_fabric", 100);
> + tsleep_nsec(sc->sc_scratch, PRIBIO, "qle_fabric",
> + SEC_TO_NSEC(1));
> }
> }
> if (rv == 0)
> @@ -2014,7 +2015,8 @@ qle_fabric_plogx(struct qle_softc *sc, s
> if (qle_read_isr(sc, , ) != 0)
> qle_handle_intr(sc, isr, info);
> } else {
> - tsleep(sc->sc_scratch, PRIBIO, "qle_fabric", 100);
> + tsleep_nsec(sc->sc_scratch, PRIBIO, "qle_fabric",
> + SEC_TO_NSEC(1));
> }
> }
> sc->sc_fabric_pending = 0;
Re: fd(4): tsleep(9) -> tsleep_nsec(9), timeout_add(9) -> timeout_add_msec(9)
On Tue, Jan 07, 2020 at 08:03:27AM -, Miod Vallat wrote: > > > Convert ticks to milliseconds. > > > > The 33 milliseconds in the timeout_add_msec(9) call will be truncated > > to 3 ticks, but that was already the case with the current (hz / 30) > > code so this is no worse. > > [...] > > > /* allow 1/30 second for heads to settle */ > > - timeout_add(>fdcpseudointr_to, hz / 30); > > + timeout_add_msec(>fdcpseudointr_to, 33); > > Why not simply write `1000 / 30' rather than 33, to match the comment? Eh, sometimes when I see that stuff I wish I didn't have to do the integer division in my head. Not that big of a deal though. Sure. Index: dev/isa/fd.c === RCS file: /cvs/src/sys/dev/isa/fd.c,v retrieving revision 1.106 diff -u -p -r1.106 fd.c --- dev/isa/fd.c30 Dec 2017 23:08:29 - 1.106 +++ dev/isa/fd.c10 Jan 2020 03:21:35 - @@ -210,11 +210,11 @@ fdprobe(struct device *parent, void *mat /* select drive and turn on motor */ bus_space_write_1(iot, ioh, fdout, drive | FDO_FRST | FDO_MOEN(drive)); /* wait for motor to spin up */ - tsleep(fdc, 0, "fdprobe", 250 * hz / 1000); + tsleep_nsec(fdc, 0, "fdprobe", MSEC_TO_NSEC(250)); out_fdc(iot, ioh, NE7CMD_RECAL); out_fdc(iot, ioh, drive); /* wait for recalibrate */ - tsleep(fdc, 0, "fdprobe", 2000 * hz / 1000); + tsleep_nsec(fdc, 0, "fdprobe", MSEC_TO_NSEC(2000)); out_fdc(iot, ioh, NE7CMD_SENSEI); n = fdcresult(fdc); #ifdef FD_DEBUG @@ -228,7 +228,7 @@ fdprobe(struct device *parent, void *mat #endif /* turn off motor */ - tsleep(fdc, 0, "fdprobe", 250 * hz / 1000); + tsleep_nsec(fdc, 0, "fdprobe", MSEC_TO_NSEC(250)); bus_space_write_1(iot, ioh, fdout, FDO_FRST); /* flags & 0x20 forces the drive to be found even if it won't probe */ @@ -900,7 +900,7 @@ loop: timeout_del(>fdtimeout_to); fdc->sc_state = RECALCOMPLETE; /* allow 1/30 second for heads to settle */ - timeout_add(>fdcpseudointr_to, hz / 30); + timeout_add_msec(>fdcpseudointr_to, 1000 / 30); return 1; /* will return later */ case RECALCOMPLETE:
Re: fd(4): tsleep(9) -> tsleep_nsec(9), timeout_add(9) -> timeout_add_msec(9)
> Convert ticks to milliseconds. > > The 33 milliseconds in the timeout_add_msec(9) call will be truncated > to 3 ticks, but that was already the case with the current (hz / 30) > code so this is no worse. [...] > /* allow 1/30 second for heads to settle */ > - timeout_add(>fdcpseudointr_to, hz / 30); > + timeout_add_msec(>fdcpseudointr_to, 33); Why not simply write `1000 / 30' rather than 33, to match the comment?
Re: bktr(4): tsleep(9) -> tsleep_nsec(9)
On Sun, Dec 15, 2019 at 10:28:32PM -0600, Scott Cheloha wrote: > I don't have any of these cards, but these are straightforward conversions. > > ok? > ok
