lkml.org 
[lkml]   [2009]   [Feb]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] input: xpad.c - Xbox 360 wireless and sysfs support
    Date
    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



    \
     
     \ /
      Last update: 2009-02-18 12:23    [W:2.265 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site