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

[Ruslan Ermilov]

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 *).

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

> 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.

> 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.

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:

- 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.

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

> 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.

> 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?


Cheers,
-- 
Ruslan Ermilov
ru at FreeBSD.org
FreeBSD committer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/cvs-all/attachments/20060919/91fb0d40/attachment.pgp