Re: [PATCH] input: Synaptics USB device driver
flat view
From: Oliver Neukum <hidden>
Date: 2012-01-04 08:24:16
Also in:
lkml
Am Dienstag, 3. Januar 2012, 19:40:33 schrieb Jan Steinhoff:
From: Jan Steinhoff <redacted> This patch adds a driver for Synaptics USB touchpad or pointing stick devices. These USB devices emulate an USB mouse by default, so one can also use the usbhid driver. However, in combination with special user space drivers this kernel driver allows one to customize the behaviour of the device.
Hi, thank you for this driver. There are a few issues which I have commented on in the text. Regards Oliver [..]
quoted hunk ↗ jump to hunk
+ input_report_abs(idev, ABS_PRESSURE, pressure); + + input_report_key(idev, BTN_LEFT, data[1] & 0x04); + input_report_key(idev, BTN_RIGHT, data[1] & 0x01); + input_report_key(idev, BTN_MIDDLE, data[1] & 0x02); + if (synusb->has_display) + input_report_key(idev, btn_middle ? BTN_MIDDLE : BTN_MISC, + data[1] & 0x08); + + input_sync(idev); +resubmit: + res = usb_submit_urb(urb, GFP_ATOMIC); + if (res)
You are racing with disconnect, suspend and pre_reset. This may lead to a spurious error message. The correct check is (res && res != -EPERM) [..]
quoted hunk ↗ jump to hunk
+ +/* + * initialization of usb data structures + */ + +static int synusb_setup_iurb(struct synusb *synusb, + struct usb_endpoint_descriptor *endpoint) +{ + char *buf; + + if (endpoint->wMaxPacketSize < 8) + return 0;
How could this happen?
quoted hunk ↗ jump to hunk
+ if (synusb->iurb) { + synusb_warn(synusb, "Found more than one possible " + "int in endpoint"); + return 0; + } + synusb->iurb = usb_alloc_urb(0, GFP_KERNEL); + if (synusb->iurb == NULL) + return -ENOMEM; + buf = usb_alloc_coherent(synusb->udev, 8, GFP_ATOMIC,
No need for an atomic allocation here.
quoted hunk ↗ jump to hunk
+ &synusb->iurb->transfer_dma); + if (buf == NULL) + return -ENOMEM; + usb_fill_int_urb(synusb->iurb, synusb->udev, + usb_rcvintpipe(synusb->udev, + endpoint->bEndpointAddress), + buf, 8, synusb_input_callback, + synusb, endpoint->bInterval); + synusb->iurb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; + return 0; +} + +static inline int synusb_match_endpoint(struct usb_endpoint_descriptor *ep) +{ + if (((ep->bEndpointAddress & USB_DIR_IN) == USB_DIR_IN) && + ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) + == USB_ENDPOINT_XFER_INT))
We have macros for such checks. Please use them.
quoted hunk ↗ jump to hunk
+ return 0; + + return -ENODEV; +} +
[..]
quoted hunk ↗ jump to hunk
+static void synusb_disconnect(struct usb_interface *interface) +{ + struct synusb *synusb; + + synusb = usb_get_intfdata(interface); + usb_set_intfdata(interface, NULL); + + cancel_delayed_work_sync(&synusb->isubmit); + + usb_kill_urb(synusb->iurb);
Code duplication. If you define draw_down(), use it.
quoted hunk ↗ jump to hunk
+ input_unregister_device(synusb->idev); + synusb->idev = NULL; + + kref_put(&synusb->kref, synusb_delete); + + dev_info(&interface->dev, "Synaptics device disconnected\n"); +} + + +/* + * suspend code + */ + +static void synusb_draw_down(struct synusb *synusb) +{ + cancel_delayed_work_sync(&synusb->isubmit); + usb_kill_urb(synusb->iurb); +} + +static int synusb_suspend(struct usb_interface *intf, pm_message_t message) +{ + struct synusb *synusb = usb_get_intfdata(intf); + + if (synusb == NULL) + return 0;
How can this happen?
quoted hunk ↗ jump to hunk
+ + synusb_draw_down(synusb); + + return 0; +} + +static int synusb_resume(struct usb_interface *intf) +{ + struct synusb *synusb = usb_get_intfdata(intf); + int res; + + res = usb_submit_urb(synusb->iurb, GFP_ATOMIC);
GFP_NOIO
quoted hunk ↗ jump to hunk
+ if (res) + synusb_err(synusb, "submit int in urb failed during resume " + "with result %d", res); + + return res; +} + +static int synusb_pre_reset(struct usb_interface *intf) +{ + struct synusb *synusb = usb_get_intfdata(intf); + + synusb_draw_down(synusb); + + return 0; +} + +static int synusb_post_reset(struct usb_interface *intf) +{ + struct synusb *synusb = usb_get_intfdata(intf); + int res; + + res = usb_submit_urb(synusb->iurb, GFP_ATOMIC);
GFP_NOIO
quoted hunk ↗ jump to hunk
+ if (res) + synusb_err(synusb, "submit int in urb failed in during " + "post_reset with result %d", res); + + return res; +} + +static int synusb_reset_resume(struct usb_interface *intf) +{ + struct synusb *synusb = usb_get_intfdata(intf); + int res; + + res = usb_submit_urb(synusb->iurb, GFP_ATOMIC);
GFP_NOIO
quoted hunk ↗ jump to hunk
+ if (res) + synusb_err(synusb, "submit int in urb failed during " + "reset-resume with result %d", res); + + return res; +} + +/* the id table is filled via sysfs, so usbhid is always the default driver */ +static struct usb_device_id synusb_idtable[] = { { } }; +MODULE_DEVICE_TABLE(usb, synusb_idtable); + +static struct usb_driver synusb_driver = { + .name = "synaptics_usb", + .probe = synusb_probe, + .disconnect = synusb_disconnect, + .id_table = synusb_idtable, + .suspend = synusb_suspend, + .resume = synusb_resume, + .pre_reset = synusb_pre_reset, + .post_reset = synusb_post_reset, + .reset_resume = synusb_reset_resume, + .supports_autosuspend = 1,
Yet you do not manage the busy state. Do you really want to play ping-pong with the power state?