On Tue, Sep 19, 2006 at 01:00:55PM -0700, Maksim Yevmenkin wrote: > On 9/19/06, Ruslan Ermilov <[EMAIL PROTECTED]> 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
[EMAIL PROTECTED]
FreeBSD committer
pgpXbz5F4Xyp2.pgp
Description: PGP signature
