Re: [PATCH v2] [RFC] NEXIO (or iNexio) support for usbtouchscreen

From: Oliver Neukum
Date: Wed Nov 18 2009 - 10:25:56 EST

Am Mittwoch, 18. November 2009 13:18:43 schrieb Ondrej Zary:

> > > + /* send init command */
> > > + ret = usb_bulk_msg(dev, usb_sndbulkpipe(dev, NEXIO_OUTPUT_EP), init,
> > > + sizeof(init), &actual_len, NEXIO_TIMEOUT);
> >
> > DMA on the kernel stack
>
> I don't know any details about this - what's the correct way to do this?

You must kmalloc the buffer.

> I tried moving the init[] array outside the function but that didn't work
> at all - compiled fine but device was not reporting the name firmware
> version. I ended up with copying it to that kmalloced buf.

That's correct.

>
> I'm worried about nexio_ack - that shouldn't work too then. Maybe it really
> does not and the device does not care.

It works on some architectures. But it is still buggy.

> I also found another problem: when I disconnect the USB cable, kernel
> oopses: BUG: unable to handle kernel NULL pointer dereference
> IP: [<f7c794fa>] start_unlink_async+0x63/0xfc
>
> Call Trace:
> ehci_urb_dequeue+0x7c/0x11a [ehci_hcd]
> unlink1+0xaa/0xc7 [usbcore]
> usb_hcd_unlink_urb+0x57/0x84 [usbcore]
> usb_kill_urb+0x40/0xbe [usbcore]
> default_wake_function+0x0/0x2b
> usb_start_wait_urb+0x6e/0xb0 [usbcore]
> usb_control_msg+0x10a/0x136 [usbcore]
> hub_port_status+0x77/0xf7 [usbcore]
> hub_thread+0x56d/0xe14 [usbcore]
> autoremove_wake_function+0x0/0x4f
> hub_thread+0x0/0xe14 [usbcore]
> kthread+0x7a/0x7f
> kthread+0x0/0x7f
>
> Are there any other problems with my code that can cause this oops?

Odd.

> + /* read firmware version */
> + ret = usb_bulk_msg(dev, usb_rcvbulkpipe(dev, NEXIO_INPUT_EP), buf,
> + NEXIO_BUFSIZE, &actual_len, NEXIO_TIMEOUT);
> + if (ret < 0)
> + goto err_out;
> + buf[buf[1]] = 0; /* second byte is data(string) length */

You'd better check buf[1] for sanity.

> + firmware_ver = kstrdup(&buf[2], GFP_KERNEL);

Are you sure this is safe?

> + /* read device name */
> + ret = usb_bulk_msg(dev, usb_rcvbulkpipe(dev, NEXIO_INPUT_EP), buf,
> + NEXIO_BUFSIZE, &actual_len, NEXIO_TIMEOUT);
> + if (ret < 0) {
> + kfree(firmware_ver);
> + goto err_out;
> + }
> + buf[buf[1]] = 0;
> + printk(KERN_INFO "Nexio device: %s, firmware version %s\n",
> + &buf[2], firmware_ver);
> + kfree(firmware_ver);
> +
> + /* prepare ACK URB */
> + usbtouch->ack = usb_alloc_urb(0, GFP_KERNEL);
> + if (!usbtouch->ack) {
> + dbg("%s - usb_alloc_urb failed: ack_urb", __func__);
> + return 0;

Are you sure this shouldn't error out?

> + }
> + usb_fill_bulk_urb(usbtouch->ack, usbtouch->udev,
> + usb_sndbulkpipe(usbtouch->udev, NEXIO_OUTPUT_EP),
> + nexio_ack_pkt, sizeof(nexio_ack_pkt), NULL,

DMA on the heap.

> + usbtouch);
> + /* submit first IRQ URB */
> + ret = usb_submit_urb(usbtouch->irq, GFP_KERNEL);
> +err_out:
> + kfree(buf);
> +err_nobuf:
> + return ret;
> +}
> +
> +static int nexio_read_data(struct usbtouch_usb *usbtouch, unsigned char
> *pkt) +{
> + int x, y, begin_x, begin_y, end_x, end_y, w, h, ret;
> + struct nexio_touch_packet *packet = (void *) pkt;
> +
> + /* got touch data? */
> + if ((pkt[0] & 0xe0) != 0xe0)
> + return 0;
> +
> + /* send ACK */
> + ret = usb_submit_urb(usbtouch->ack, GFP_KERNEL);

In interrupt context. You must use GFP_ATOMIC.

> +
> + if (!usbtouch->type->max_xc) {
> + usbtouch->type->max_xc = 2 * be16_to_cpu(packet->x_len);
> + input_set_abs_params(usbtouch->input, ABS_X, 0,
> + 2 * be16_to_cpu(packet->x_len), 0, 0);
> + usbtouch->type->max_yc = 2 * be16_to_cpu(packet->y_len);
> + input_set_abs_params(usbtouch->input, ABS_Y, 0,
> + 2 * be16_to_cpu(packet->y_len), 0, 0);
> + }
> + /* The device reports state of IR sensors on X and Y axes.
> + * Each byte represents "darkness" percentage (0-100) of one element.
> + * 17" touchscreen reports only 64 x 52 bytes so the resolution is low.
> + * This also means that there's a limited multi-touch capability but
> + * it's disabled (and untested) here as there's no X driver for that.
> + */
> + begin_x = end_x = begin_y = end_y = -1;
> + for (x = 0; x < be16_to_cpu(packet->x_len); x++) {
> + if (begin_x == -1 && packet->data[x] > NEXIO_THRESHOLD) {
> + begin_x = x;
> + continue;
> + }
> + if (end_x == -1 && begin_x != -1 && packet->data[x] < NEXIO_THRESHOLD) {
> + end_x = x - 1;
> + for (y = be16_to_cpu(packet->x_len);
> + y < be16_to_cpu(packet->data_len); y++) {
> + if (begin_y == -1 && packet->data[y] > NEXIO_THRESHOLD) {
> + begin_y = y - be16_to_cpu(packet->x_len);
> + continue;
> + }
> + if (end_y == -1 &&
> + begin_y != -1 && packet->data[y] < NEXIO_THRESHOLD) {
> + end_y = y - 1 - be16_to_cpu(packet->x_len);
> + w = end_x - begin_x;
> + h = end_y - begin_y;
> + /* multi-touch */
> +/* input_report_abs(usbtouch->input,
> + ABS_MT_TOUCH_MAJOR, max(w,h));
> + input_report_abs(usbtouch->input,
> + ABS_MT_TOUCH_MINOR, min(x,h));
> + input_report_abs(usbtouch->input,
> + ABS_MT_POSITION_X, 2*begin_x+w);
> + input_report_abs(usbtouch->input,
> + ABS_MT_POSITION_Y, 2*begin_y+h);
> + input_report_abs(usbtouch->input,
> + ABS_MT_ORIENTATION, w > h);
> + input_mt_sync(usbtouch->input);*/
> + /* single touch */
> + usbtouch->x = 2 * begin_x + w;
> + usbtouch->y = 2 * begin_y + h;
> + usbtouch->touch = packet->flags & 0x01;
> + begin_y = end_y = -1;
> + return 1;
> + }
> + }
> + begin_x = end_x = -1;
> + }
> +
> + }
> + return 0;
> +}
> +#endif
> +

> @@ -1007,8 +1198,10 @@ static void usbtouch_disconnect(struct u
> dbg("%s - usbtouch is initialized, cleaning up", __func__);
> usb_set_intfdata(intf, NULL);
> usb_kill_urb(usbtouch->irq);
> + usb_kill_urb(usbtouch->ack);

Have you checked the order is correct?

> input_unregister_device(usbtouch->input);
> usb_free_urb(usbtouch->irq);
> + usb_free_urb(usbtouch->ack);
> usbtouch_free_buffers(interface_to_usbdev(intf), usbtouch);
> kfree(usbtouch);
> }
>

Regards
Oliver

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