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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 18 Dec 2009 16:18:33 +0100 (CET)
From:	Jiri Kosina <jkosina@...e.cz>
To:	"Rick L. Vinyard Jr." <rvinyard@...nmsu.edu>
Cc:	linux-kernel@...r.kernel.org, felipe.balbi@...ia.com,
	krzysztof.h1@...pl, Andrew Morton <akpm@...ux-foundation.org>,
	linux-usb@...r.kernel.org, Oliver Neukum <oliver@...kum.org>,
	linux-input@...r.kernel.org
Subject: Re: [PATCH] Logitech G13 driver 0.0.2

On Wed, 16 Dec 2009, akpm@...ux-foundation.org wrote:

> +static unsigned char g13_lcd_bits[] = {
> +   0x00, 0x00, 0x07, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +   0x00, 0x00, 0x00, 0x28, 0x03, 0x00, 0x40, 0x01, 0x00, 0xc0, 0x3f, 0x00,
> +   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xa1,
> +   0x08, 0x00, 0x08, 0x00, 0x00, 0xe0, 0x7f, 0x00, 0x00, 0x00, 0x00, 0x00,
> +   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x3c, 0x25, 0x00, 0xf3, 0x03,
> +   0x00, 0xe0, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +   0x00, 0x00, 0x00, 0x0e, 0x05, 0x00, 0x20, 0x16, 0x00, 0xf0, 0xff, 0x00,
> +   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x20, 0x09,
> +   0x00, 0x00, 0x00, 0x42, 0x00, 0xf0, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00,
> +   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0x05, 0x60, 0x80, 0x00, 0x14,
> +   0x00, 0x30, 0xe7, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +   0x00, 0x00, 0x90, 0x00, 0x00, 0x20, 0x00, 0x10, 0x00, 0x10, 0xe3, 0x01,
> +   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x90, 0x02,
> +   0xe0, 0xdd, 0x03, 0x90, 0x00, 0x50, 0xcb, 0x01, 0x00, 0xfe, 0xff, 0x7f,
> +   0xf0, 0x3f, 0x00, 0xff, 0xff, 0x7f, 0x80, 0x00, 0xfa, 0xe3, 0x07, 0x38,
> +   0x00, 0x10, 0xc1, 0x01, 0x00, 0xff, 0xff, 0x7f, 0xf0, 0x3f, 0x00, 0xff,
> +   0xff, 0xff, 0xd0, 0x00, 0xfc, 0x87, 0x0f, 0x90, 0x00, 0x30, 0xe0, 0x01,
> +   0x80, 0xff, 0xff, 0x7f, 0xf0, 0x3f, 0x00, 0xff, 0xff, 0xff, 0x81, 0x80,
> +   0xfe, 0xa7, 0x3f, 0x30, 0x00, 0x30, 0xc0, 0x01, 0xc0, 0xff, 0xff, 0x7f,
> +   0xf0, 0x3f, 0x00, 0xff, 0xff, 0xff, 0x93, 0x42, 0x0f, 0x08, 0x3a, 0x30,
> +   0x00, 0x10, 0xe8, 0x01, 0xc0, 0xff, 0xff, 0x7f, 0xf0, 0x3f, 0x00, 0xff,
> +   0xff, 0xff, 0x93, 0xa4, 0x41, 0x20, 0x20, 0x94, 0x00, 0x30, 0xf4, 0x03,
> +   0xc0, 0x07, 0x00, 0x00, 0x00, 0x3e, 0x00, 0x00, 0x00, 0xe0, 0x93, 0x41,
> +   0x60, 0x48, 0xe1, 0x98, 0x00, 0x70, 0xda, 0x07, 0xc0, 0x07, 0x00, 0x00,
> +   0x00, 0x3e, 0x00, 0x00, 0x00, 0xe0, 0x83, 0x77, 0x10, 0x82, 0xc5, 0x1f,
> +   0x00, 0xd0, 0xcc, 0x07, 0xc0, 0x07, 0x00, 0x00, 0x00, 0x3e, 0x00, 0x00,
> +   0x00, 0xe0, 0x23, 0x3f, 0x00, 0x90, 0xc0, 0x4f, 0x00, 0x98, 0x83, 0x0f,
> +   0xc0, 0x07, 0x00, 0x00, 0x00, 0x3e, 0x00, 0x00, 0x00, 0xe0, 0x03, 0x3f,
> +   0x00, 0x85, 0x86, 0x27, 0x00, 0x0c, 0x80, 0x0f, 0xc0, 0x07, 0x00, 0x00,
> +   0x00, 0x3e, 0x00, 0x00, 0x00, 0xe0, 0x03, 0x3e, 0xc0, 0x83, 0x86, 0x0b,
> +   0x00, 0x0c, 0x80, 0x1f, 0xc0, 0x07, 0x00, 0x00, 0x00, 0x3e, 0x00, 0x00,
> +   0x00, 0xe0, 0x03, 0x11, 0x00, 0x00, 0x04, 0x0d, 0x00, 0x0e, 0x00, 0x3f,
> +   0xc0, 0x07, 0xff, 0xff, 0x00, 0x3e, 0x00, 0xff, 0xff, 0xff, 0x03, 0x0c,
> +   0x10, 0x00, 0x02, 0x02, 0x00, 0x0f, 0x00, 0x3f, 0xc0, 0x07, 0xff, 0xff,
> +   0x00, 0x3e, 0x00, 0xff, 0xff, 0xff, 0x01, 0x00, 0x08, 0x00, 0x1c, 0x02,
> +   0x00, 0x07, 0x00, 0x7e, 0xc0, 0x07, 0xff, 0xff, 0x00, 0x3e, 0x00, 0xff,
> +   0xff, 0xff, 0x00, 0x00, 0x04, 0x00, 0x28, 0x04, 0x80, 0x03, 0x00, 0x7e,
> +   0xc0, 0x07, 0xff, 0xff, 0x00, 0x3e, 0x00, 0xff, 0xff, 0xff, 0x01, 0x08,
> +   0x02, 0x58, 0x08, 0x00, 0x80, 0x03, 0x00, 0xfc, 0xc0, 0x07, 0xff, 0xff,
> +   0x00, 0x3e, 0x00, 0xff, 0xff, 0xff, 0x01, 0x00, 0x01, 0x08, 0x21, 0x00,
> +   0x80, 0x03, 0x00, 0xfc, 0xc0, 0x07, 0x00, 0xf8, 0x00, 0x3e, 0x00, 0x00,
> +   0x00, 0xe0, 0x03, 0xa4, 0x01, 0x28, 0x00, 0x00, 0xc0, 0x01, 0x00, 0xfc,
> +   0xc0, 0x07, 0x00, 0xf8, 0x00, 0x3e, 0x00, 0x00, 0x00, 0xe0, 0x03, 0x2e,
> +   0x02, 0x00, 0x00, 0x00, 0xc0, 0x01, 0x00, 0xfc, 0xc1, 0x07, 0x00, 0xf8,
> +   0x00, 0x3e, 0x00, 0x00, 0x00, 0xe0, 0x03, 0x20, 0x00, 0x02, 0x00, 0x00,
> +   0xe0, 0x01, 0x00, 0xfc, 0xc1, 0x07, 0x00, 0xf8, 0x00, 0x3e, 0x00, 0x00,
> +   0x00, 0xe0, 0x03, 0x20, 0x02, 0x00, 0x10, 0x00, 0xe0, 0x01, 0x00, 0xe8,
> +   0xc1, 0x07, 0x00, 0xf8, 0x00, 0x3e, 0x00, 0x00, 0x00, 0xe0, 0x03, 0x20,
> +   0x02, 0x04, 0x00, 0x00, 0xe0, 0x00, 0x00, 0xfc, 0xc1, 0x07, 0x00, 0xf8,
> +   0x00, 0x3e, 0x00, 0x00, 0x00, 0xe0, 0x03, 0x00, 0x0c, 0x00, 0x00, 0x00,
> +   0xf0, 0x01, 0x00, 0xfe, 0xc0, 0xff, 0xff, 0xff, 0xf0, 0xff, 0x0f, 0xff,
> +   0xff, 0xff, 0x03, 0x40, 0x08, 0x08, 0x0d, 0x00, 0x58, 0x03, 0x00, 0x7c,
> +   0xc0, 0xff, 0xff, 0xff, 0xf0, 0xff, 0x0f, 0xff, 0xff, 0xff, 0x03, 0x40,
> +   0x10, 0xa0, 0x18, 0x00, 0xa8, 0x06, 0x00, 0xb8, 0x81, 0xff, 0xff, 0xff,
> +   0xf0, 0xff, 0x0f, 0xff, 0xff, 0xff, 0x01, 0x00, 0x10, 0x00, 0x00, 0x00,
> +   0x5e, 0x1d, 0x00, 0xe8, 0x82, 0xff, 0xff, 0xff, 0xf0, 0xff, 0x0f, 0xff,
> +   0xff, 0xff, 0x01, 0x00, 0x20, 0xc0, 0x07, 0x00, 0xab, 0x3a, 0x00, 0x54,
> +   0x03, 0xfe, 0xff, 0xff, 0xf0, 0xff, 0x0f, 0xff, 0xff, 0x7f, 0x00, 0x80,
> +   0x40, 0x01, 0x01, 0x00, 0x55, 0x3d, 0x00, 0xaa, 0x06, 0x00, 0x00, 0x00,
> +   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x81, 0x01, 0x01, 0x00,
> +   0xab, 0x1a, 0xc0, 0x55, 0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +   0x00, 0x00, 0x00, 0x00, 0x80, 0x03, 0x00, 0x00, 0x55, 0x35, 0xe0, 0xab,
> +   0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +   0xc0, 0x43, 0x01, 0x00, 0xab, 0xaa, 0xff, 0x55, 0x03, 0x00, 0x00, 0x00,
> +   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xc7, 0x00, 0x00,
> +   0x57, 0xf5, 0xff, 0xaa, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +   0x00, 0x00, 0x00, 0x00, 0x00, 0xfc, 0x00, 0x00, 0xaa, 0xea, 0xff, 0xd5,
> +   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +   0x00, 0xfe, 0x03, 0x00, 0x7c, 0x75, 0x80, 0x2b, 0x00, 0x00, 0x00, 0x00,
> +   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xf8, 0x03, 0x00,
> +   0x80, 0x1f, 0x00, 0x1f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +   0x00, 0x00, 0x00, 0x00, 0x00, 0xf0, 0x01, 0x00 };

Probably somme comment explaining what this actually is would be nice 
here.

[ .. snip .. ]

I can't really comment too much on the framebuffer part, so if you could 
get some Ack for this part from someone skilled in writing framebuffer 
drivers, I'd appreciate it.

> +static ssize_t g13_keymap_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t count)
> +{
> +	struct hid_device *hdev;
> +	int scanned;
> +	int consumed;
> +	int scancd;
> +	int keycd;
> +	int set_result;
> +	int set = 0;
> +	int gkey;
> +	int index;
> +	int good;
> +	struct g13_data *data;
> +
> +	/* Get the hid associated with the device */
> +	hdev = container_of(dev, struct hid_device, dev);
> +
> +	/* If we have an invalid pointer we'll return ENODATA */
> +	if (hdev == NULL || &(hdev->dev) != dev)
> +		return -ENODATA;
> +
> +	/* Now, let's get the data structure */
> +	data = hid_get_g13data(hdev);
> +	if (data == NULL)
> +		return -ENODATA;
> +
> +	do {
> +		good = 0;
> +
> +		/* Look for scancode keycode pair in hex */
> +		scanned = sscanf(buf,
> +				 "%x %x%n",
> +				 &scancd, &keycd, &consumed);
> +		if (scanned == 2) {
> +			buf += consumed;
> +			set_result = g13_input_setkeycode(data->input_dev,
> +							  scancd,
> +							  keycd);
> +			if (set_result < 0)
> +				return set_result;
> +			set++;
> +			good = 1;
> +		} else {
> +			/*
> +			 * Look for Gkey keycode pair and assign to current
> +			 * keymap
> +			 */
> +			scanned = sscanf(buf,
> +					 "G%d %x%n",
> +					 &gkey, &keycd, &consumed);
> +			if (scanned == 2 && gkey > 0 && gkey <= G13_KEYS) {
> +				buf += consumed;
> +				scancd = data->curkeymap * G13_KEYS + gkey - 1;
> +				set_result =
> +					g13_input_setkeycode(data->input_dev,
> +							     scancd, keycd);
> +				if (set_result < 0)
> +					return set_result;
> +				set++;
> +				good = 1;
> +			} else {
> +				/*
> +				 * Look for Gkey-index keycode pair and assign
> +				 * to indexed keymap
> +				 */
> +				scanned = sscanf(buf,
> +						 "G%d-%d %x%n",
> +						 &gkey,
> +						 &index,
> +						 &keycd,
> +						 &consumed);

These constructions are ugly (funnily enough, this is currently being 
discussed on lkml -- see http://lkml.org/lkml/2009/12/15/490

Could you perharps split this into some sub-functions, to make it look 
nicer?

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.
--
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/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ