Re: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support

[Oliver Neukum <oliver@neukum.org>]

Am Sunday 15 February 2009 05:08:46 schrieb Mike Murphy:
> Greetings,
> 
> The attached patch is a result of a few days of hacking to get better
> Linux support for Xbox 360 wireless controllers. Major functional
> improvements include support for the LEDs on the wireless controllers
> (which are auto-set to the controller number as they are on the
> official console), added support for rumble effects on wireless
> controllers, added support for dead zones on all supported
> controllers, and added support for a square axis mapping for the left
> stick on all supported controllers. A large amount of the bulk in this
> patch is from the addition of a sysfs interface to retrieve
> information and adjust the dead zone/square axis settings.
[..]
> Patch generated against 2.6.28.2 (no changes in relevant in-kernel
> driver from 2.6.28.2 to 2.6.28.5, so should apply against latest
> stable).
> 
> Comments are appreciated. I am _NOT_ subscribed to the LKML, so please
> CC me directly.

1. You need to check the returns of sscanf
2. This is very ugly:

+/* read-only attrs */
+static ssize_t xpad_show_int(struct xpad_data *xd, struct xpad_attribute *attr,
+	char *buf)
+{
+	int value;
+	if (!strcmp(attr->attr.name, "controller_number"))
+		value = xd->controller_number;
+	else if (!strcmp(attr->attr.name, "pad_present"))
+		value = xd->pad_present;
+	else if (!strcmp(attr->attr.name, "controller_type"))
+		value = xd->controller_type;
+	else
+		value = 0;
+	return sprintf(buf, "%d\n", value);
+}

3. Possible memory leak in error case:

+static struct xpad_data *xpad_create_data(const char *name, struct kobject *parent) {
+	struct xpad_data *data = NULL;
+	int check;
+	
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return NULL;
+	
+	check = kobject_init_and_add(&data->kobj, &xpad_ktype, parent, "%s", name);
+	if (check) {
+		kobject_put(&data->kobj);
+		return NULL;
+	}

4. Why the cpup variety?

+	coords[0] = (__s16) le16_to_cpup((__le16 *)(data + x_offset));

5. What happens if this work is already scheduled?

 	if (data[0] & 0x08) {
+		padnum = xpad->controller_data->controller_number;
 		if (data[1] & 0x80) {
-			xpad->pad_present = 1;
-			usb_submit_urb(xpad->bulk_out, GFP_ATOMIC);
-		} else
-			xpad->pad_present = 0;
+			printk(KERN_INFO "Wireless Xbox 360 pad #%d present\n", padnum);
+			xpad->controller_data->pad_present = 1;
+			
+			INIT_WORK(&xpad->work, &xpad_work_controller);
+			schedule_work(&xpad->work);

6. No GFP_ATOMIC. If you can take a mutex you can sleep.
+		usb_submit_urb(xpad->irq_out, GFP_ATOMIC);

	Regards
		Oliver