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.
> 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.
> 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
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?
Received on Tue Sep 19 2006 - 20:36:10 UTC
- application/pgp-signature attachment: stored