Re: [PATCH 001/001] linux-input: Support for BCM5974 multitouch trackpad

Am Freitag 27 Juni 2008 01:07:19 schrieb Henrik Rydberg:
From: Henrik Rydberg <rydberg@xxxxxxxxxxx>

BCM5974: This driver adds support for the multitouch trackpad on the new
Apple Macbook Air and Macbook Pro Penryn laptops. It replaces the
appletouch driver on those computers, and integrates well with the
synaptics driver of the Xorg system.

Some comments on the driver.

Regards
Oliver

+
+static int atp_wellspring_init(struct usb_device *udev)
+{
+ const struct atp_config_t *cfg = atp_product_config(udev);
+ char data[8];

DMA on the stack. You must kmalloc that buffer.
+ int size;
+
+ /* reset button endpoint */
+ if (usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
+ USB_REQ_CLEAR_FEATURE, USB_RECIP_ENDPOINT,
+ 0, cfg->bt_ep, NULL, 0, 5000)) {
+ err("Could not reset button endpoint");
+ return -EIO;
+ }
+
+ /* reset trackpad endpoint */
+ if (usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
+ USB_REQ_CLEAR_FEATURE, USB_RECIP_ENDPOINT,
+ 0, cfg->tp_ep, NULL, 0, 5000)) {
+ err("Could not reset trackpad endpoint");
+ return -EIO;
+ }
+
+ /* read configuration */
+ size = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
+ ATP_WELLSPRING_MODE_READ_REQUEST_ID,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ ATP_WELLSPRING_MODE_REQUEST_VALUE,
+ ATP_WELLSPRING_MODE_REQUEST_INDEX, data, 8, 5000);
+
+ if (size != 8) {
+ err("Could not do mode read request from device (Wellspring Raw mode)");
+ return -EIO;
+ }
+
+ /* apply the mode switch */
+ data[0] = ATP_WELLSPRING_MODE_VENDOR_VALUE_1;
+ data[1] = ATP_WELLSPRING_MODE_VENDOR_VALUE_2;
+
+ /* write configuration */
+ size = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
+ ATP_WELLSPRING_MODE_WRITE_REQUEST_ID,
+ USB_DIR_OUT | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ ATP_WELLSPRING_MODE_REQUEST_VALUE,
+ ATP_WELLSPRING_MODE_REQUEST_INDEX, data, 8, 5000);
+
+ if (size != 8) {
+ err("Could not do mode write request to device (Wellspring Raw mode)");
+ return -EIO;
+ }
+
+ return 0;
+}
[..]
+static inline int raw2int(unsigned char hi, unsigned char lo)
+{
+ return (short)(hi<<8)+lo;
+}

Use the standard function.

[..]
+static int atp_open(struct input_dev *input)
+{
+ struct atp *dev = input_get_drvdata(input);
+
+ if (usb_submit_urb(dev->bt_urb, GFP_ATOMIC))
+ return -EIO;
+
+ if (usb_submit_urb(dev->tp_urb, GFP_ATOMIC))
+ return -EIO;

Resource leak. You must kill the first urb if you bail out here.
+
+ dev->open = 1;
+ return 0;
+}
+
+static void atp_close(struct input_dev *input)
+{
+ struct atp *dev = input_get_drvdata(input);
+
+ usb_kill_urb(dev->tp_urb);
+ usb_kill_urb(dev->bt_urb);
+ cancel_work_sync(&dev->work);

I can't see where you use that work struct. Anyway, this is a race.
As the work handler can submit urbs, you must first cancel the work,
then kill the urbs.

+ dev->open = 0;
+}
+
[..]
+static void atp_disconnect(struct usb_interface *iface)
+{
+ struct atp *dev = usb_get_intfdata(iface);
+
+ usb_set_intfdata(iface, NULL);
+ if (dev) {
+ usb_kill_urb(dev->tp_urb);
+ usb_kill_urb(dev->bt_urb);

close will do that. no need to kill here.
+ input_unregister_device(dev->input);
+ usb_buffer_free(dev->udev, dev->cfg.tp_datalen,
+ dev->tp_data, dev->tp_urb->transfer_dma);
+ usb_buffer_free(dev->udev, dev->cfg.bt_datalen,
+ dev->bt_data, dev->bt_urb->transfer_dma);
+ usb_free_urb(dev->tp_urb);
+ usb_free_urb(dev->bt_urb);
+ kfree(dev);
+ }
+ printk(KERN_INFO "input: bcm5974 disconnected\n");
+}
+
+static int atp_suspend(struct usb_interface *iface, pm_message_t message)
+{
+ struct atp *dev = usb_get_intfdata(iface);
+
+ usb_kill_urb(dev->tp_urb);
+ dev->tp_valid = 0;
+
+ usb_kill_urb(dev->bt_urb);
+ dev->bt_valid = 0;

Why don't you cancel the work here?
--
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/