Am Monday 16 February 2009 14:22:05 schrieb Mike Murphy:
> >
> > 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
I don't know. I also find that documentation thin.
Please add that the memory is freed elsewhere.
> 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);
> >
> 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).
Also what happens if the work is scheduled and you unplug?
> 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
That is not correct. It is true while holding a spinlock, not a mutex.
> 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.
Please post that BUG.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
