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-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
