cvs commit: src/sys/dev/kbdmux kbdmux.c

Maksim Yevmenkin maksim.yevmenkin at gmail.com
Tue Sep 19 16:18:15 PDT 2006


On 9/19/06, Ruslan Ermilov <ru at freebsd.org> wrote:
> On Tue, Sep 19, 2006 at 01:00:55PM -0700, Maksim Yevmenkin wrote:
> > On 9/19/06, Ruslan Ermilov <ru at freebsd.org> wrote:
> > >On Tue, Sep 19, 2006 at 12:36:38PM -0700, Maksim Yevmenkin wrote:
> > >> nope, same crash. the only thing that seems to help is to reverting
> > >> back to (int *) cast just like other keyboard drivers do. then it
> > >> works.
> > >>
> > >> i'm in the process of getting amd64 snapshot iso to try it on a couple
> > >> of boxes. if it will work then i'm going to back (int *) -> (intptr_t
> > >> *) changes introduced in rev 1.8.
> > >>
> > >Why?  amd64 and i386 are unaffected (both of my i386 and amd64 work
> > >with the committed revision), and on sparc64 we need to find a proper
> > >fix.  It cannot work properly with (intptr_t *) removed as demonstrated
> > >by the test program I sent to you.  Here it's again for reference:
> >
> > ok, i'm officially confused now. from what i understood,
> >
> > i386 is not affected (intptr_t *) casing, right?
> >
> Yes, because intptr_t is "int" on i386.
>
> > (intptr_t *) casing causes problem with amd64, right?
> >
> Caused, because you applied *(intptr_t *) to things that didn't
> require it, which are now reverted back to *(int *).

ok

> > sparc64 crashes with (intptr_t *) casting (tested locally)
> >
> We need to find *where*, and find a proper fix.

that will take some time, as ultra5 is very slow at compiling kernel :)

> > now,
> >
> > what i'm suggesting is to replace (intptr_t *) casting with (int *) as
> > it was before rev 1.8. i claim that after this change
> >
> > i386 will not affected.
> >
> Yes.
>
> > sparc64 will work and will not crash (tested locally)
> >
> not crash != work properly.  Can you print the value of argument
> to KDSETLED and verify it's correct?  Print it both as
> (int)*(intptr_t *)arg and *(int *)arg, and compare.

ok, it works from user's point of view, i.e. i press caps, num lock
etc. and the light comes on/off on keyboard(s).

> > amd64 should work (i think), but i'm in the process of getting amd64
> > iso to build a test box.
> >
> Yes, it should.
>
> > i tested (int *) casting on sparc64 with SETLED, SETREPEAT and it
> > works fine. i made sure that parameters passed via ioctl() are all
> > correct. i will do the same test on amd64 before committing.
> >
> KDSETREPEAT _does_ use *(int *) in the committed code, as it's
> a normal "_IOW('K', 102, keyboard_repeat_t)" which takes a pointer
> as an argument.

yes, so?

> KDSETLED isn't used outside the kernel, so I assume you tested it
> only when it's called from within the kernel?  If so, try passing
> it from useland to see the endianness problem; I'm pretty sure it
> will fire.  If it does, there are two options:

ok, i see what you are saying now. i will try to use KDSETLED from userspace.

> - Fix in-kernel callers of KDSETLED to behave like userland.
> - Fix the definition of KDSETLED to be:
>
> #define KDSETLED       _IOW('K', 66, int)
>
> and then fix kbdmux.c to use *(int *) when it accesses KDSETLED's
> argument.

ok

> Similarly for KDSKBSTATE.  KDSETRAD should still be accessed
> through (int)*(intptr_t *)arg to stay binary compatible.

or perhaps as keyboard_repeat_t

> > also, like i said before, all other keyboard drivers, including
> > sunkbd(4) use (int *) cast and NOT (intptr_t *), and it seems to work
> > just fine.
> >
> I think this problem is unnoticeable because you're talking about
> the KDSETLED here, and it's only used inside the kernel, and
> inside the kernel it's passed as a "pointer to int", while in
> case of userland it would be passed as a 64-bit value on the
> stack (on sparc64), and causing an endianness problem when
> accessed through *(int *)arg.

from quick search, i can see that SETLED, KBSKBMODE and KDBSKBSTATE
are called from kernel with (caddr_t) &foo, i.e. pointer to int value.

from another quick search in /usr/src/{usr.}{s}bin/ i do not see any
userspace code that uses these ioctl() directly (and, yes, it does not
mean that it does not exists).

so, i propose

1) we change kbd/syscons/etc kernel code back to its default
assumption that KBxxx ioctls defined as _IO(foo, bar) accept 3rd
parameter as a pointer to int.

2) we change

KDSETLED  _IO('K', 66, /* int */)

to

KDSETLED  _IO('K', 66, /* (int *) */)

so its "documented".

3) take an "ostrich approach" (put our heads in the sand) to any 3rd
party application that might use SETLED, KBSKBMODE etc. ioctls
directly.

the above requires minimal code change, which is, imo, is a good thing.

> > while i'm interested what is going on here, i'd rather commit
> > something that is tested and known to work then wait. it will buy some
> > time to do the proper analysis and fix.
> >
> kbdmux didn't work on sparc64 anyway to the best of my knowledge,
> or am I mistaken?

after recent sunkbd(4) fix it is possible to use kbdmux(4) on sparc64.
i have a ulta5 behind me that runs 6-stable with kbdmux(4) sunkbd(4)
and ukbd(4) connected.

sunkbd(4) needs to be fixed (read implement read_char() and
check_char()) but its not kbdmux(4) problem.

thanks,
max

>
>
> Cheers,
> --
> Ruslan Ermilov
> ru at FreeBSD.org
> FreeBSD committer
>
>
>


More information about the cvs-src mailing list