linux-kernel - Re: PATCH [1/3] drivers/input/xpad.c: Improve Xbox 360 wireless support and add sysfs interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 2 Mar 2009 19:12:30 -0800 (PST)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Mike Murphy <mamurph@...clemson.edu>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
	linux-usb@...r.kernel.org, greg@...ah.com, oliver@...kum.org,
	fweisbec@...il.com
Subject: Re: PATCH [1/3] drivers/input/xpad.c: Improve Xbox 360 wireless 
 support and add sysfs interface

On Mon, 2 Mar 2009, Mike Murphy wrote:
> 
> 3. The outer cast (__s16) converts the unsigned values to signed
> values, while the "~" inverts the y axes to make them function like a
> joystick instead of a flight simulator control.

You should do the ~ before the cast, or use - if you just want to reverse 
things. It probably doesn't much matter (the difference between ~ and - i 
just one), but still..

Also, quite frankly, it looks like your 'coords[]' array should just be of 
type 's16' (rather than 'int') to begin with. You seem to really never use 
it as an int anyway. That would get rid of the cast.

> Is there a cleaner way to accomplish the transition from 16-bit
> unsigned little endian to 16-bit signed host endian?

I think the code is fine, but I think you'd be better off if the "data" 
pointer was perhaps of type "le16 *" to begin with.

That obviously means that your "offset" addition should now be in 16-bit 
words rather than in bytes, so you'd need to divide the offsets by two to 
do that, but those are just numbers anyway. And quite frankly, it looks 
like the actual data is just offset differently - but with the same fixed 
offset between values - for the two cases, so you could just have _one_ 
offset (and even just add that into the 'data' pointer).

That would get rid of the second cast. You'd end up with just

	s16 coords[4];

	/* In words - so this is 12 vs 6 bytes into the data */
	data += (xpad->xtype == XTYPE_XBOX) ? 6 : 3;

	coords[0] = le16_to_cpup(data);
	coords[1] = ~le16_to_cpup(data + 1);
	coords[2] = le16_to_cpup(data + 2);
	coords[3] = ~le16_to_cpup(data + 3);
	..

which looks a bit shorter and avoids those casts. I dunno.

		Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/