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
mbg(4): tsleep(9) -> tsleep_nsec(9)
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?
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);
