lkml.org 
[lkml]   [2008]   [Jul]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 001/002] linux-input: bcm5974-0.31: fixed resource leak, removed work struct, device data struct introduced

    > Subject: [PATCH 001/002] linux-input: bcm5974-0.31: fixed resource leak, removed work struct, device data struct introduced

    hm, I wonder where [002/002] went.

    Please copy the USB list on USB patches.

    On Sat, 28 Jun 2008 14:54:20 +0200
    Henrik Rydberg <rydberg@euromail.se> wrote:

    > 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.

    - erk, the driver uses two-spaces for indenting all over the place.
    Mayeb the email client mangled it in imaginative ways, but it _looks_
    to be intentional.

    If so, please don't do that. Kernel code uses hard tabs at
    8-cols-per-tab for indenting.


    > +config MOUSE_BCM5974
    > + tristate "Apple USB BCM5974 Multitouch trackpad support"
    > + depends on USB_ARCH_HAS_HCD
    > + select USB

    selecting USB here seems like a bad idea, although I note that several
    other drivers had bad ideas. `depends on' would be better, IMO.

    > + help
    > + Say Y here if you have an Apple USB BCM5974 Multitouch
    > + trackpad.
    > +
    > + The BCM5974 is the multitouch trackpad found in the Macbook
    > + Air (JAN2008) and Macbook Pro Penryn (FEB2008) laptops.
    > +
    > + It is also found in the IPhone (2007) and Ipod Touch (2008).
    > +
    > + This driver provides multitouch functionality together with
    > + the synaptics X11 driver.
    > +
    > + The interface is currently identical to the appletouch interface,
    > + for further information, see
    > + <file:Documentation/input/appletouch.txt>.
    > +
    > + To compile this driver as a module, choose M here: the
    > + module will be called bcm5974.
    > +
    > config MOUSE_INPORT
    > tristate "InPort/MS/ATIXL busmouse"
    > depends on ISA
    > diff -puN drivers/input/mouse/Makefile~input-support-for-bcm5974-multitouch-trackpad drivers/input/mouse/Makefile
    > --- a/drivers/input/mouse/Makefile~input-support-for-bcm5974-multitouch-trackpad
    > +++ a/drivers/input/mouse/Makefile
    > @@ -6,6 +6,7 @@
    >
    > obj-$(CONFIG_MOUSE_AMIGA) += amimouse.o
    > obj-$(CONFIG_MOUSE_APPLETOUCH) += appletouch.o
    > +obj-$(CONFIG_MOUSE_BCM5974) += bcm5974.o
    > obj-$(CONFIG_MOUSE_ATARI) += atarimouse.o
    > obj-$(CONFIG_MOUSE_RISCPC) += rpcmouse.o
    > obj-$(CONFIG_MOUSE_INPORT) += inport.o
    >
    > ...
    >
    > +static inline
    > +const struct atp_config_t *atp_product_config(struct usb_device *udev)
    > +{
    > + u16 id = le16_to_cpu(udev->descriptor.idProduct);
    > + const struct atp_config_t *config;
    > + for (config = atp_config_table; config->ansi; ++config)
    > + if (config->ansi == id || config->iso == id || config->jis == id)
    > + return config;
    > + return atp_config_table;
    > +}

    This is waaaaaaay too big for inlining.

    > +/* Wellspring initialization constants */
    > +#define ATP_WELLSPRING_MODE_READ_REQUEST_ID 1
    > +#define ATP_WELLSPRING_MODE_WRITE_REQUEST_ID 9
    > +#define ATP_WELLSPRING_MODE_REQUEST_VALUE 0x300
    > +#define ATP_WELLSPRING_MODE_REQUEST_INDEX 0
    > +#define ATP_WELLSPRING_MODE_VENDOR_VALUE_1 0x01
    > +#define ATP_WELLSPRING_MODE_VENDOR_VALUE_2 0x05
    > +
    > +#define dprintk(format, a...) \
    > + { if (debug) printk(KERN_DEBUG format, ##a); }
    > +
    > +MODULE_AUTHOR("Henrik Rydberg");
    > +MODULE_DESCRIPTION("Apple USB BCM5974 multitouch driver");
    > +MODULE_LICENSE("GPL");
    > +
    > +static int debug = 1;
    > +module_param(debug, int, 0644);
    > +MODULE_PARM_DESC(debug, "Activate debugging output");
    > +
    > +static int atp_wellspring_init(struct usb_device *udev)
    > +{
    > + const struct atp_config_t *cfg = atp_product_config(udev);
    > + char *data = kmalloc(8, GFP_DMA);
    > + 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");
    > + goto error;
    > + }
    > +
    > + /* 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");
    > + goto error;
    > + }
    > +
    > + /* 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)");
    > + goto error;
    > + }
    > +
    > + /* apply the mode switch */
    > + data[0] = ATP_WELLSPRING_MODE_VENDOR_VALUE_1;
    > + data[1] = ATP_WELLSPRING_MODE_VENDOR_VALUE_2;

    This will crash the kernel if the kmalloc() failed. And
    kmalloc(GFP_DMA) can easily fail.

    Do we really need memory from the GFP_DMA region for this driver? It
    would be terribly old-fashioned.

    I dunno what USB driver normally use for their dma memory. Perhaps
    dma_alloc_coherent()?

    > + /* 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)");
    > + goto error;
    > + }
    > +
    > + kfree(data);
    > + return 0;
    > +
    > + error:
    > + kfree(data);
    > + return -EIO;
    > +}
    > +
    > +/* Structure to hold all of our device specific stuff */
    > +struct atp {
    > + char phys[64];
    > + struct usb_device *udev; /* usb device */
    > + struct input_dev *input; /* input dev */
    > + struct atp_config_t cfg; /* device configuration */
    > + unsigned open; /* non-zero if opened */
    > + struct urb *bt_urb; /* button usb request block */
    > + signed char *bt_data; /* button transferred data */
    > + unsigned bt_valid; /* are the button sensors valid ? */
    > + unsigned bt_state; /* current button state */
    > + struct urb *tp_urb; /* trackpad usb request block */
    > + signed char *tp_data; /* trackpad transferred data */
    > + unsigned tp_valid; /* are the trackpad sensors valid ? */
    > +};
    > +
    > +static inline int raw2int(__le16 x)
    > +{
    > + return (short)le16_to_cpu(x);

    Given that this function returns signed 32-bit, I'd worry about what it
    does when it is presented with 0xffff. otoh, perhaps you indeed
    intended to handle negative 16-bit quantities in that fashion, which
    would make sense.

    > +}
    > +
    > +static inline int int2scale(const struct atp_params_t *p, int x)
    > +{
    > + return ((p->max-p->min)*x)/(p->devmax-p->devmin);
    > +}

    Is there any way in which this can get a divide-by-zero?

    > +/**
    > + * all value ranges, both device and logical, are assumed to be [a,b).
    > + */
    > +static inline int int2bound(const struct atp_params_t *p, int x)
    > +{
    > + int s = p->min+int2scale(p, x);

    Please put a space around the operators in expressions like this.

    > + return s < p->min?p->min:s >= p->max?p->max-1:s;

    That's just unreadable. Please simplify it.

    > +}
    > +
    > +/**

    This leadin is exclusively used to identify a kerneldoc-style comment.

    > + * check quality of reading.
    > + * -1: bad data
    > + * 0: ignore this reading
    > + * 1: good reading
    > + */

    But this is not a kerneldoc comment. Please remove all the incorrect
    /** instances.

    >
    > ...
    >
    > +/**
    > + * convert raw data to synaptics sensor output.
    > + * returns the number of fingers on the trackpad,
    > + * or a negative number in vase of bad data.
    > + */
    > +static int compute_movement(int *abs_x, int *abs_y,
    > + int *rel_x, int *rel_y,
    > + int *pressure,
    > + struct atp_config_t *cfg,
    > + const unsigned char *data, int size)
    > +{
    > + const int nfinger = (size-26)/28;
    > + const struct tp_data_t *tp = (struct tp_data_t *) data;
    > +
    > + if (nfinger) {
    > + *abs_x = int2bound(&cfg->x, raw2int(tp->finger->abs_x) - cfg->x.devmin);
    > + *abs_y = int2bound(&cfg->y, cfg->y.devmax - raw2int(tp->finger->abs_y));
    > + *rel_x = int2scale(&cfg->x, raw2int(tp->finger->rel_x));
    > + *rel_y = int2scale(&cfg->y, -raw2int(tp->finger->rel_y));
    > + *pressure = int2bound(&cfg->p, raw2int(tp->finger->force_major));
    > + } else {
    > + *abs_x = 0;
    > + *abs_y = 0;
    > + *rel_x = 0;
    > + *rel_y = 0;
    > + *pressure = 0;
    > + }
    > +
    > + /* report zero fingers for zero pressure */
    > + return *pressure > 0?nfinger:0;

    Normal style is

    return *pressure > 0 ? nfinger : 0;

    (lots of other places)

    > +}
    > +
    > +static void atp_button(struct urb *urb)
    > +{
    > + struct atp *dev = urb->context;
    > + const unsigned char *data = dev->bt_data;
    > + const int size = dev->bt_urb->actual_length;
    > + int button = 0, retval;
    > +
    > + switch (urb->status) {
    > + case 0:
    > + /* success */
    > + break;
    > + case -EOVERFLOW:
    > + case -ECONNRESET:
    > + case -ENOENT:
    > + case -ESHUTDOWN:
    > + /* This urb is terminated, clean up */
    > + dbg("%s - urb shutting down with status: %d",
    > + __func__, urb->status);
    > + return;
    > + default:
    > + dbg("%s - nonzero urb status received: %d",
    > + __func__, urb->status);
    > + goto exit;
    > + }
    > +
    > + /* first sample data ignored */
    > + if (!dev->bt_valid) {
    > + dev->bt_valid = 1;
    > + goto exit;
    > + }
    > +
    > + /* drop incomplete datasets */
    > + if (size != 4) {
    > + dprintk("bcm5974: incomplete button package (first byte: %d, length: %d)\n",
    > + (int)data[0], size);
    > + goto exit;
    > + }
    > +
    > + button = data[1];
    > +
    > + /* only report button state changes */
    > + if (button != dev->bt_state) {
    > + input_report_key(dev->input, BTN_LEFT, button);
    > + input_sync(dev->input);
    > + }
    > +
    > + dev->bt_state = button;
    > +
    > + exit:
    > + retval = usb_submit_urb(dev->bt_urb, GFP_ATOMIC);

    GFP_ATOMIC is a red flag. Is this quite unrelaible allocation mode
    really needed here?

    > + if (retval) {
    > + err("%s - button usb_submit_urb failed with result %d",
    > + __func__, retval);
    > + }
    > +}
    > +
    > +static void atp_trackpad(struct urb *urb)
    > +{
    > + struct atp *dev = urb->context;
    > + int abs_x, abs_y, rel_x, rel_y, pressure;
    > + int quality, nfinger, retval;
    > +
    > + switch (urb->status) {
    > + case 0:
    > + /* success */
    > + break;
    > + case -EOVERFLOW:
    > + case -ECONNRESET:
    > + case -ENOENT:
    > + case -ESHUTDOWN:
    > + /* This urb is terminated, clean up */
    > + dbg("%s - urb shutting down with status: %d",
    > + __func__, urb->status);
    > + return;
    > + default:
    > + dbg("%s - nonzero urb status received: %d",
    > + __func__, urb->status);
    > + goto exit;
    > + }
    > +
    > + /* first sample data ignored */
    > + if (!dev->tp_valid) {
    > + dev->tp_valid = 1;
    > + goto exit;
    > + }
    > +
    > + /* determine quality of reading */
    > + quality = compute_quality(dev->tp_data, dev->tp_urb->actual_length);
    > +
    > + /* drop incomplete datasets */
    > + if (quality < 0) {
    > + dprintk("bcm5974: incomplete trackpad package "
    > + "(first byte: %d, length: %d)\n",
    > + (int)dev->tp_data[0], dev->tp_urb->actual_length);
    > + goto exit;
    > + }
    > +
    > + /* drop poor quality readings */
    > + if (quality == 0)
    > + goto exit;
    > +
    > + nfinger = compute_movement(&abs_x, &abs_y, &rel_x, &rel_y, &pressure,
    > + &dev->cfg,
    > + dev->tp_data, dev->tp_urb->actual_length);
    > +
    > + if (debug > 1) {
    > + printk(KERN_DEBUG "bcm5974: x: %04d y: %04d dx: %3d dy: %3d p: %3d\n",
    > + abs_x, abs_y, rel_x, rel_y, pressure);
    > + }
    > +
    > + /* input_report_key(dev->input,BTN_TOUCH,pressure>dev->cfg.p.fuzz); */
    > + input_report_abs(dev->input, ABS_PRESSURE, pressure);
    > + input_report_abs(dev->input, ABS_X, abs_x);
    > + input_report_abs(dev->input, ABS_Y, abs_y);
    > + /* input_report_rel(dev->input, REL_X, rel_x); */
    > + /* input_report_rel(dev->input, REL_Y, rel_y); */
    > + input_report_key(dev->input, BTN_TOOL_FINGER, nfinger == 1);
    > + input_report_key(dev->input, BTN_TOOL_DOUBLETAP, nfinger == 2);
    > + input_report_key(dev->input, BTN_TOOL_TRIPLETAP, nfinger > 2);
    > + input_sync(dev->input);
    > +
    > + exit:
    > + retval = usb_submit_urb(dev->tp_urb, GFP_ATOMIC);

    ditto.

    > + if (retval) {
    > + err("%s - trackpad usb_submit_urb failed with result %d",
    > + __func__, retval);
    > + }
    > +}
    > +
    > +static int atp_open(struct input_dev *input)
    > +{
    > + struct atp *dev = input_get_drvdata(input);
    > +
    > + if (usb_submit_urb(dev->bt_urb, GFP_ATOMIC))
    > + goto error;
    > +
    > + if (usb_submit_urb(dev->tp_urb, GFP_ATOMIC))
    > + goto err_free_bt_urb;

    etc.

    > + dev->open = 1;
    > + return 0;
    > +
    > + err_free_bt_urb:
    > + usb_free_urb(dev->bt_urb);
    > + error:
    > + return -EIO;
    > +}
    > +
    >
    > ...
    >



    \
     
     \ /
      Last update: 2008-07-02 01:03    [W:0.062 / U:0.700 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site