Re: git: c51978f4b2f0 - main - kbd: add KBD_DELAY1 and KBD_DELAY2

On Sun, Feb 26, 2023 at 9:12 PM Mark Millard <marklmi@yahoo.com> wrote:

> On Feb 26, 2023, at 19:33, Warner Losh <imp@bsdimp.com> wrote:
>
> > On Sat, Feb 25, 2023 at 10:03 AM Mark Millard <marklmi@yahoo.com> wrote:
> > Warner Losh <imp_at_FreeBSD.org> wrote on
> > Date: Sat, 25 Feb 2023 06:26:00 UTC :
> >
> > > The branch main has been updated by imp:
> > >
> > > URL:
> https://cgit.FreeBSD.org/src/commit/?id=c51978f4b2f080c80ddc891c24b151d35acb8ba4
> > >
> > > commit c51978f4b2f080c80ddc891c24b151d35acb8ba4
> > > Author:     Michael Paepcke <git@paepcke.de>
> > > AuthorDate: 2023-02-18 09:11:37 +0000
> > > Commit:     Warner Losh <imp@FreeBSD.org>
> > > CommitDate: 2023-02-25 06:19:05 +0000
> > >
> > >     kbd: add KBD_DELAY1 and KBD_DELAY2
> > >
> > >     Allow to configure KBD_DELAY* via KERNCONF for user-land less
> embedded
> > >     and security appliances
> > >
> > >     Reviewed by: imp (folded)
> > >     Pull Request: https://github.com/freebsd/freebsd-src/pull/649
> > > ---
> > >  sys/conf/options     | 3 ++-
> > >  sys/dev/kbd/kbd.c    | 4 ++--
> > >  sys/dev/kbd/kbdreg.h | 8 ++++++--
> > >  3 files changed, 10 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/sys/conf/options b/sys/conf/options
> > > index 7855a2f3f20e..42529a90a54e 100644
> > > --- a/sys/conf/options
> > > +++ b/sys/conf/options
> > > @@ -803,8 +803,9 @@ KBD_INSTALL_CDEV  opt_kbd.h
> > >  KBD_MAXRETRY         opt_kbd.h
> > >  KBD_MAXWAIT          opt_kbd.h
> > >  KBD_RESETDELAY               opt_kbd.h
> > > +KBD_DELAY1           opt_kbd.h
> > > +KBD_DELAY2           opt_kbd.h
> > >  KBDIO_DEBUG          opt_kbd.h
> > > -
> > >  KBDMUX_DFLT_KEYMAP   opt_kbdmux.h
> > >
> > >  # options for the Atheros driver
> > > diff --git a/sys/dev/kbd/kbd.c b/sys/dev/kbd/kbd.c
> > > index 205d76639e0f..ebc779de4073 100644
> > > --- a/sys/dev/kbd/kbd.c
> > > +++ b/sys/dev/kbd/kbd.c
> > > @@ -143,8 +143,8 @@ kbd_init_struct(keyboard_t *kbd, char *name, int
> type, int unit, int config,
> > >       kbd->kb_accentmap = NULL;
> > >       kbd->kb_fkeytab = NULL;
> > >       kbd->kb_fkeytab_size = 0;
> > > -     kbd->kb_delay1 = KB_DELAY1;     /* these values are advisory
> only */
> > > -     kbd->kb_delay2 = KB_DELAY2;
> > > +     kbd->kb_delay1 = KBD_DELAY1;    /* these values are advisory
> only */
> > > +     kbd->kb_delay2 = KBD_DELAY2;
> > >       kbd->kb_count = 0L;
> > >       bzero(kbd->kb_lastact, sizeof(kbd->kb_lastact));
> > >  }
> > > diff --git a/sys/dev/kbd/kbdreg.h b/sys/dev/kbd/kbdreg.h
> > > index 15b7e5183f35..2839e259420d 100644
> > > --- a/sys/dev/kbd/kbdreg.h
> > > +++ b/sys/dev/kbd/kbdreg.h
> > > @@ -151,8 +151,12 @@ struct keyboard {
> > >       void            *kb_data;       /* the driver's private data */
> > >       int             kb_delay1;
> > >       int             kb_delay2;
> > > -#define KB_DELAY1    500
> > > -#define KB_DELAY2    100
> > > +#ifndef KBD_DELAY1
> > > +#define KBD_DELAY1   500
> > > +#endif
> > > +#ifndef KBD_DELAY2
> > > +#define KBD_DELAY2   100
> > > +#endif
> >
> > [Just reporting Ximalas's Discord note.]
> >
> > So opt_kbd.h must be included before kbdreg.h in
> > order to avoid: macro redefined in opt_kbd.h ?
> >
> > Should something force the right order?
> >
> > If we include them in the wrong order, then the compiler will complain,
> > and that's likely sufficient. I'll double check NOTES to make sure that
> the
> > values there are different than the defaults.
>
> The report on Discord (Kernel) was that the attempted use
> resulted in:
>
> --- psm.o ---
> In file included from /usr/src/sys/dev/atkbdc/psm.c:100:
> In file included from /usr/src/sys/dev/atkbdc/atkbdcreg.h:39:
> ./opt_kbd.h:1:9: error: 'KBD_DELAY1' macro redefined
> [-Werror,-Wmacro-redefined]
> #define KBD_DELAY1 200
>         ^
> /usr/src/sys/dev/kbd/kbdreg.h:155:9: note: previous definition is here
> #define KBD_DELAY1      500
>         ^
> In file included from /usr/src/sys/dev/atkbdc/psm.c:100:
> In file included from /usr/src/sys/dev/atkbdc/atkbdcreg.h:39:
> ./opt_kbd.h:3:9: error: 'KBD_DELAY2' macro redefined
> [-Werror,-Wmacro-redefined]
> #define KBD_DELAY2 15
>         ^
> /usr/src/sys/dev/kbd/kbdreg.h:158:9: note: previous definition is here
> #define KBD_DELAY2      100
>         ^
> 2 errors generated.
> *** [psm.o] Error code 1
>
>
> The reported workaround was:
>
> diff --git a/sys/dev/atkbdc/psm.c b/sys/dev/atkbdc/psm.c
> index a308cc81cd3a..86560f7bc2dd 100644
> --- a/sys/dev/atkbdc/psm.c
> +++ b/sys/dev/atkbdc/psm.c
> @@ -62,6 +62,7 @@
> __FBSDID("$FreeBSD$");
>
> #include "opt_isa.h"
> +#include "opt_kbd.h"
> #include "opt_psm.h"
> #include "opt_evdev.h"
>

That works around it. We include opt_kbd.h from atkbdcreg.h, which is a bit
odd (but it's a kernel-only,
so I moved it). And kbdreg.h is also kernel only, so I added it there
instead. That's likely safer and
I should likely have listened a bit better when you asked about it
before...  And not committed to LINT
from machine A when I test compiled it on machine B by mistake <doh>.

Warner


> > Warner
> >  >       unsigned long   kb_count;       /* # of processed key strokes */
> > >       u_char          kb_lastact[NUM_KEYS/2];
> > >       struct cdev *kb_dev;
>
>
>
> ===
> Mark Millard
> marklmi at yahoo.com
>
>