Date: Wed, 5 Jan 2022 10:26:13 -0700 From: Warner Losh <imp@bsdimp.com> To: Alexander Motin <mav@freebsd.org> Cc: src-committers <src-committers@freebsd.org>, "<dev-commits-src-all@freebsd.org>" <dev-commits-src-all@freebsd.org>, dev-commits-src-main@freebsd.org Subject: Re: git: 9e007a88d65b - main - atkbd: Reduce polling rate from 10Hz to ~1Hz. Message-ID: <CANCZdfrHtui-zGkhq-QEm0ANQD28n=39V3gy_3SDKcKCnZkXgg@mail.gmail.com> In-Reply-To: <202201051641.205GfWZV041757@gitrepo.freebsd.org> References: <202201051641.205GfWZV041757@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] Maybe we default hw.atkbd.hz to 0. That will give us more info if this is even a thing still. And it would give users hit by this a no recompile fix and give us feedback as to how often this happens. We used to miss ISA interrupts in the early SMPNG days, and that's the time this change was introduced. The PIC does a good job of latching the state and we have no other drivers that have this workaround absent issues with the device itself. Warner On Wed, Jan 5, 2022, 9:41 AM Alexander Motin <mav@freebsd.org> wrote: > The branch main has been updated by mav: > > URL: > https://cgit.FreeBSD.org/src/commit/?id=9e007a88d65ba0d23e73c3c052d474a78260d503 > > commit 9e007a88d65ba0d23e73c3c052d474a78260d503 > Author: Alexander Motin <mav@FreeBSD.org> > AuthorDate: 2022-01-05 16:32:44 +0000 > Commit: Alexander Motin <mav@FreeBSD.org> > CommitDate: 2022-01-05 16:41:26 +0000 > > atkbd: Reduce polling rate from 10Hz to ~1Hz. > > In my understanding this is only needed to workaround lost interrupts. > I was thinking to remove it completely, but the comment about edge- > triggered interrupt may be true and needs deeper investigation. ~1Hz > should be often enough to handle the supposedly rare loss cases, but > rare enough to not appear in top. Add sysctl hw.atkbd.hz to tune it. > > MFC after: 1 month > --- > sys/dev/atkbdc/atkbd.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/sys/dev/atkbdc/atkbd.c b/sys/dev/atkbdc/atkbd.c > index 40dd698984e3..cee1207df973 100644 > --- a/sys/dev/atkbdc/atkbd.c > +++ b/sys/dev/atkbdc/atkbd.c > @@ -42,6 +42,7 @@ __FBSDID("$FreeBSD$"); > #include <sys/proc.h> > #include <sys/limits.h> > #include <sys/malloc.h> > +#include <sys/sysctl.h> > > #include <machine/bus.h> > #include <machine/resource.h> > @@ -73,6 +74,13 @@ typedef struct atkbd_state { > #endif > } atkbd_state_t; > > +static SYSCTL_NODE(_hw, OID_AUTO, atkbd, CTLFLAG_RD | CTLFLAG_MPSAFE, 0, > + "AT keyboard"); > + > +static int atkbdhz = 1; > +SYSCTL_INT(_hw_atkbd, OID_AUTO, hz, CTLFLAG_RWTUN, &atkbdhz, 0, > + "Polling frequency (in hz)"); > + > static void atkbd_timeout(void *arg); > static int atkbd_reset(KBDC kbdc, int flags, int c); > > @@ -198,8 +206,11 @@ atkbd_timeout(void *arg) > kbdd_intr(kbd, NULL); > } > splx(s); > - state = (atkbd_state_t *)kbd->kb_data; > - callout_reset(&state->ks_timer, hz / 10, atkbd_timeout, arg); > + if (atkbdhz > 0) { > + state = (atkbd_state_t *)kbd->kb_data; > + callout_reset_sbt(&state->ks_timer, SBT_1S / atkbdhz, 0, > + atkbd_timeout, arg, C_PREL(1)); > + } > } > > /* LOW-LEVEL */ > [-- Attachment #2 --] <div dir="auto"><div>Maybe we default hw.atkbd.hz to 0. That will give us more info if this is even a thing still. And it would give users hit by this a no recompile fix and give us feedback as to how often this happens.</div><div dir="auto"><br></div><div dir="auto">We used to miss ISA interrupts in the early SMPNG days, and that's the time this change was introduced. The PIC does a good job of latching the state and we have no other drivers that have this workaround absent issues with the device itself.</div><div dir="auto"><br></div><div dir="auto">Warner<br><br><div class="gmail_quote" dir="auto"><div dir="ltr" class="gmail_attr">On Wed, Jan 5, 2022, 9:41 AM Alexander Motin <<a href="mailto:mav@freebsd.org">mav@freebsd.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The branch main has been updated by mav:<br> <br> URL: <a href="https://cgit.FreeBSD.org/src/commit/?id=9e007a88d65ba0d23e73c3c052d474a78260d503" rel="noreferrer noreferrer" target="_blank">https://cgit.FreeBSD.org/src/commit/?id=9e007a88d65ba0d23e73c3c052d474a78260d503</a><br>; <br> commit 9e007a88d65ba0d23e73c3c052d474a78260d503<br> Author: Alexander Motin <mav@FreeBSD.org><br> AuthorDate: 2022-01-05 16:32:44 +0000<br> Commit: Alexander Motin <mav@FreeBSD.org><br> CommitDate: 2022-01-05 16:41:26 +0000<br> <br> atkbd: Reduce polling rate from 10Hz to ~1Hz.<br> <br> In my understanding this is only needed to workaround lost interrupts.<br> I was thinking to remove it completely, but the comment about edge-<br> triggered interrupt may be true and needs deeper investigation. ~1Hz<br> should be often enough to handle the supposedly rare loss cases, but<br> rare enough to not appear in top. Add sysctl hw.atkbd.hz to tune it.<br> <br> MFC after: 1 month<br> ---<br> sys/dev/atkbdc/atkbd.c | 15 +++++++++++++--<br> 1 file changed, 13 insertions(+), 2 deletions(-)<br> <br> diff --git a/sys/dev/atkbdc/atkbd.c b/sys/dev/atkbdc/atkbd.c<br> index 40dd698984e3..cee1207df973 100644<br> --- a/sys/dev/atkbdc/atkbd.c<br> +++ b/sys/dev/atkbdc/atkbd.c<br> @@ -42,6 +42,7 @@ __FBSDID("$FreeBSD$");<br> #include <sys/proc.h><br> #include <sys/limits.h><br> #include <sys/malloc.h><br> +#include <sys/sysctl.h><br> <br> #include <machine/bus.h><br> #include <machine/resource.h><br> @@ -73,6 +74,13 @@ typedef struct atkbd_state {<br> #endif<br> } atkbd_state_t;<br> <br> +static SYSCTL_NODE(_hw, OID_AUTO, atkbd, CTLFLAG_RD | CTLFLAG_MPSAFE, 0,<br> + "AT keyboard");<br> +<br> +static int atkbdhz = 1;<br> +SYSCTL_INT(_hw_atkbd, OID_AUTO, hz, CTLFLAG_RWTUN, &atkbdhz, 0,<br> + "Polling frequency (in hz)");<br> +<br> static void atkbd_timeout(void *arg);<br> static int atkbd_reset(KBDC kbdc, int flags, int c);<br> <br> @@ -198,8 +206,11 @@ atkbd_timeout(void *arg)<br> kbdd_intr(kbd, NULL);<br> }<br> splx(s);<br> - state = (atkbd_state_t *)kbd->kb_data;<br> - callout_reset(&state->ks_timer, hz / 10, atkbd_timeout, arg);<br> + if (atkbdhz > 0) {<br> + state = (atkbd_state_t *)kbd->kb_data;<br> + callout_reset_sbt(&state->ks_timer, SBT_1S / atkbdhz, 0,<br> + atkbd_timeout, arg, C_PREL(1));<br> + }<br> }<br> <br> /* LOW-LEVEL */<br> </blockquote></div></div></div>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfrHtui-zGkhq-QEm0ANQD28n=39V3gy_3SDKcKCnZkXgg>