Re: [PATCH 001/002] linux-input: bcm5974-0.31: fixed resourceleak, removed work struct, device data struct introduced

[Andrew Morton]

> 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@xxxxxxxxxxx> 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;
> +}
> +
>
> ...
>

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