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

On Mon, Feb 16, 2009 at 3:31 AM, Oliver Neukum <oliver@xxxxxxxxxx> wrote:
...

1. You need to check the returns of sscanf

Will add... this is currently preliminary and not very well tested.

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);
+}

The above code is basically following the example in
samples/kobject/kset-example.c. I broke the rest of the sysfs stuff
out such that it uses separate functions for show/store, which
definitely looks cleaner. However, given the large amount of code that
results, I'm starting to think that re-factoring and pulling the sysfs
code out to a separate file might be useful.


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;
+ }


My understanding from Documentation/kobject.txt is that the
kobject_put in the 2nd error check will set the kobj's reference
counter to zero, eventually causing the kobject core to call my
cleanup function for the ktype (xpad_release) and free the memory. Is
this not correct? I find the sysfs docs to be fairly thin... and sysfs
seems to be substantially more complex than procfs or ioctls would be
for the same purpose. However, everything I read suggested that sysfs
is the "best" way to go in a modern kernel.

4. Why the cpup variety?

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


The cpup cast is in the original stable driver
(drivers/input/joystick/xpad.c), and I didn't question it.

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);


I'm still a little fuzzy on this... in theory, I could see that
INIT_WORK would clobber the existing work structures while they wait
in the queue (thought about changing to PREPARE_WORK).

However, in practice, this work queue trick is only used when a
wireless 360 controller connects to the receiver. There is 1 instance
of struct usb_xpad per wireless controller (4 total, since the
receiver exposes 4 controller slots), and each instance has a separate
struct work_struct. So two things have to happen to reschedule the
work before it completes:

1. The user has to remove the battery pack from the controller,
reinstall the battery pack, and re-activate the controller by pushing
and holding the center button for at least 1 second.

2. The kernel has to be busy enough not to have completed the work in
the ~2 seconds a human could have done (1).

I need a bit of guidance from someone who has a better understanding
of the work queues to have a good solution to this one. Is switching
to PREPARE_WORK sufficient (with an INIT_WORK somewhere in
xpad_probe)? Or is a more involved solution needed?

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


Per the "Linux Device Drivers" book (O'Reilly, 3rd ed), the claim is
made that submissions while holding a mutex should be GFP_ATOMIC. My
tests seemed to verify this claim... as sending LED commands
GFP_KERNEL while holding the mutex resulted in BUGs (scheduling while
atomic) in dmesg. Switching those GFP_KERNELs to GFP_ATOMICs
eliminated that particular BUG.

Regards
Oliver

Thanks for your reply... I will keep working on the driver as time
allows. This is really the first driver on which I've done any
substantial hacking, and my formal kernel-level programming training
was on an older version of the FreeBSD kernel, so I'm having to learn
things as I go. I'm trying to develop based off the latest stable
sources, so the outdated nature of most of the reference material I
have is not helping matters.

Thanks,
Mike
--
Mike Murphy
Ph.D. Candidate and NSF Graduate Research Fellow
Clemson University School of Computing
120 McAdams Hall
Clemson, SC 29634-0974 USA
Tel: +1 864.656.2838 Fax: +1 864.656.0145
http://cirg.cs.clemson.edu/~mamurph
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/