On Tue, Mar 22, 2005 at 10:37:29PM -0500, Nick Sillik wrote:
> diff -urN -X dontdiff linux-2.6.12-rc1-mm1/drivers/usb/storage/Kconfig
> linux-2.6.12-rc1-mm1-onetouch/drivers/usb/storage/Kconfig
> --- linux-2.6.12-rc1-mm1/drivers/usb/storage/Kconfig 2005-03-22
> 18:27:15.000000000 -0500
> +++ linux-2.6.12-rc1-mm1-onetouch/drivers/usb/storage/Kconfig 2005-03-22
> 19:59:13.000000000 -0500
> @@ -133,3 +133,16 @@
> Say Y here to include additional code to support the Lexar Jumpshot
> USB CompactFlash reader.
>
> +config USB_STORAGE_ONETOUCH
> + bool "Support OneTouch Button on Maxtor Hard Drives (EXPERIMENTAL)"
> + depends on USB_STORAGE && INPUT_EVDEV && EXPERIMENTAL
> + help
> + Say Y here to include additional code to support the Maxtor
> OneTouch
> + USB hard drive's onetouch button.
> +
> + This code registers the button on the front of Maxtor OneTouch USB
> + hard drive's as an input device. An action can be associated with
> + this input in any keybinding software. (e.g. gnome's keyboard short-
> + cuts)
> +
> +
> diff -urN -X dontdiff linux-2.6.12-rc1-mm1/drivers/usb/storage/Makefile
> linux-2.6.12-rc1-mm1-onetouch/drivers/usb/storage/Makefile
> --- linux-2.6.12-rc1-mm1/drivers/usb/storage/Makefile 2005-03-22
> 18:27:15.000000000 -0500
> +++ linux-2.6.12-rc1-mm1-onetouch/drivers/usb/storage/Makefile
> 2005-03-22 19:59:13.000000000 -0500
> @@ -18,6 +18,7 @@
> usb-storage-obj-$(CONFIG_USB_STORAGE_ISD200) += isd200.o
> usb-storage-obj-$(CONFIG_USB_STORAGE_DATAFAB) += datafab.o
> usb-storage-obj-$(CONFIG_USB_STORAGE_JUMPSHOT) += jumpshot.o
> +usb-storage-obj-$(CONFIG_USB_STORAGE_ONETOUCH) += onetouch.o
>
> usb-storage-objs := scsiglue.o protocol.o transport.o usb.o \
> initializers.o $(usb-storage-obj-y)
> diff -urN -X dontdiff linux-2.6.12-rc1-mm1/drivers/usb/storage/onetouch.c
> linux-2.6.12-rc1-mm1-onetouch/drivers/usb/storage/onetouch.c
> --- linux-2.6.12-rc1-mm1/drivers/usb/storage/onetouch.c 1969-12-31
> 19:00:00.000000000 -0500
> +++ linux-2.6.12-rc1-mm1-onetouch/drivers/usb/storage/onetouch.c
> 2005-03-22 19:59:57.000000000 -0500
> @@ -0,0 +1,210 @@
> +/*
> + * Support for the Maxtor OneTouch USB hard drive's button
> + *
> + * Current development and maintenance by:
> + * Copyright (c) 2005 Nick Sillik <[EMAIL PROTECTED]>
> + *
> + * Initial work by:
> + * Copyright (c) 2003 Erik Thyr?n <[EMAIL PROTECTED]>
> + *
> + * Based on usbmouse.c (Vojtech Pavlik) and xpad.c (Marko Friedemann)
> + *
> + */
> +
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#include <linux/config.h>
> +#include <linux/kernel.h>
> +#include <linux/input.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/usb.h>
> +#include "usb.h"
> +#include "onetouch.h"
> +#include "debug.h"
> +
> +
> +struct usb_onetouch {
> + char name[128];
> + char phys[64];
> + struct input_dev dev; /* input device interface */
> + struct usb_device *udev; /* usb device */
> +
> + struct urb *irq; /* urb for interrupt in report */
> + unsigned char *data; /* input data */
> + dma_addr_t data_dma;
> +
> + int open_count; /* reference count */
> +};
> +
> +static void onetouch_irq(struct urb *urb, struct pt_regs *regs)
> +{
> + struct usb_onetouch *onetouch = urb->context;
> + int retval;
> +
> + switch (urb->status) {
> + case 0:
> + /* success */
> + break;
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + /* this urb is terminated, clean up */
> + dbg("%s - urb shutting down with status: %d", __FUNCTION__,
> + urb->status);
> + return;
> + default:
> + dbg("%s - nonzero urb status received: %d", __FUNCTION__,
> + urb->status);
> + goto resubmit;
> + }
> +
> + input_regs(&onetouch->dev, regs);
> + /*printk(KERN_INFO "input: %02x %02x\n", onetouch->data[0],
> onetouch->data[1]); */
> + input_report_key(&onetouch->dev, ONETOUCH_BUTTON,
> + onetouch->data[0] & 0x02);
> +
> + input_sync(&onetouch->dev);
> +
> + resubmit:
> + retval = usb_submit_urb(urb, GFP_ATOMIC);
> + if (retval)
> + err("%s - usb_submit_urb failed with result %d",
> + __FUNCTION__, retval);
> +}
> +
> +static int onetouch_open(struct input_dev *dev)
> +{
> + struct usb_onetouch *onetouch = dev->private;
> +
> + if (onetouch->open_count++)
> + return 0;
Looks like fops->open for character devices is called under lock_kernel(),
so maybe this is safe for now...
> +
> + onetouch->irq->dev = onetouch->udev;
> + if (usb_submit_urb(onetouch->irq, GFP_KERNEL)) {
> + onetouch->open_count--;
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static void onetouch_close(struct input_dev *dev)
> +{
> + struct usb_onetouch *onetouch = dev->private;
> +
> + if (!--onetouch->open_count)
> + usb_kill_urb(onetouch->irq);
However, I don't see lock_kernel() (or any other locking) in the close
path. E.g., when closing a /dev/input/event*:
sys_close()
filp_close()
fput()
__fput()
file->f_op->release == evdev_release
input_close_device()
handle->dev->close == onetouch_close
Therefore it seems that on SMP different CPUs can simultaneously enter
onetouch_close() and at least corrupt onetouch->open_count. Hmm, the
input subsystem itself seems unsafe.
> +}
> +
> +int onetouch_connect_input(struct us_data *ss)
> +{
> + struct usb_device *udev = ss->pusb_dev;
> + struct usb_onetouch *onetouch;
> + char path[64];
> +
> + if (udev->descriptor.idVendor != VENDOR_MAXTOR
> + || udev->descriptor.idProduct != PRODUCT_ONETOUCH) {
> + /* Not a onetouch device, nothing to see here */
> + return 1;
> + }
> +
> +
> +
> + US_DEBUGP("Connecting OneTouch device\n");
> +
> + onetouch = kmalloc(sizeof(struct usb_onetouch), GFP_KERNEL);
> +
> + if (onetouch == NULL) {
> + err("cannot allocate memory for new onetouch");
> + return 1;
> + }
> + memset(onetouch, 0, sizeof(struct usb_onetouch));
> +
> + onetouch->data = usb_buffer_alloc(udev, ONETOUCH_PKT_LEN,
> + SLAB_ATOMIC,
> + &onetouch->data_dma);
> + if (!onetouch->data) {
> + kfree(onetouch);
> + return 1;
Using goto seems more appropriate here:
if (onetouch == NULL)
goto fail;
...
if (!onetouch->data)
goto fail_free_dev;
...
if (!onetouch->irq)
goto fail_free_buffer;
...
fail_free_buffer:
usb_buffer_free(...);
fail_free_dev:
kfree(onetouch);
fail:
return 1;
Also, the common convention is to return a negative error code (-ENOMEM
for an allocation failure). However, currently usb-storage ignores the
return value of the init function (which seems to be a bug by itself).
> + }
> +
> + onetouch->irq = usb_alloc_urb(0, GFP_KERNEL);
> + if (!onetouch->irq) {
> + err("cannot allocate memory for new onetouch interrupt urb");
> + usb_buffer_free(udev, ONETOUCH_PKT_LEN, onetouch->data,
> + onetouch->data_dma);
> + kfree(onetouch);
> + return 1;
> + }
> +
> + ss->extra_destructor = onetouch_release_input;
> + ss->extra = onetouch;
> +
> + usb_fill_int_urb(onetouch->irq, udev,
> + ss->recv_intr_pipe,
> + onetouch->data, ONETOUCH_PKT_LEN, onetouch_irq,
> + onetouch, ss->ep_bInterval);
> + onetouch->irq->transfer_dma = onetouch->data_dma;
> + onetouch->irq->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> +
> + onetouch->udev = udev;
> +
> + onetouch->dev.id.bustype = BUS_USB;
> + onetouch->dev.id.vendor = udev->descriptor.idVendor;
> + onetouch->dev.id.product = udev->descriptor.idProduct;
> + onetouch->dev.id.version = udev->descriptor.bcdDevice;
> + onetouch->dev.private = onetouch;
> + onetouch->dev.name = onetouch->name;
> + onetouch->dev.phys = onetouch->phys;
> + onetouch->dev.open = onetouch_open;
> + onetouch->dev.close = onetouch_close;
> +
> + usb_make_path(udev, path, 64);
> + snprintf(onetouch->phys, 64, "%s/input0", path);
> + snprintf(onetouch->name, 128,
> + "%s %s",
> + udev->manufacturer,
> + udev->product);
> + if (!strlen(onetouch->name))
This cannot happen - even if udev->manufacturer and udev->product are both
empty, the snprintf() call above will put at least one space into the
string.
> + snprintf(onetouch->name, 128, "Maxtor OneTouch");
> +
> + set_bit(EV_KEY, onetouch->dev.evbit);
> + set_bit(ONETOUCH_BUTTON, onetouch->dev.keybit);
> + clear_bit(0, onetouch->dev.keybit);
> +
> + input_register_device(&onetouch->dev);
> +
> + printk(KERN_INFO "input: %s on %s\n", onetouch->dev.name, path);
> +
> + return 0;
> +}
> +
> +void onetouch_release_input(void *onetouch_)
> +{
> + struct usb_onetouch *onetouch = (struct usb_onetouch *) onetouch_;
> +
> + US_DEBUGP("device found: %s. Releasing\n", onetouch->phys);
> + usb_unlink_urb(onetouch->irq);
> + input_unregister_device(&onetouch->dev);
> + usb_free_urb(onetouch->irq);
> + usb_buffer_free(onetouch->udev, ONETOUCH_PKT_LEN,
> + onetouch->data, onetouch->data_dma);
> + kfree(onetouch);
> +}
> diff -urN -X dontdiff linux-2.6.12-rc1-mm1/drivers/usb/storage/onetouch.h
> linux-2.6.12-rc1-mm1-onetouch/drivers/usb/storage/onetouch.h
> --- linux-2.6.12-rc1-mm1/drivers/usb/storage/onetouch.h 1969-12-31
> 19:00:00.000000000 -0500
> +++ linux-2.6.12-rc1-mm1-onetouch/drivers/usb/storage/onetouch.h
> 2005-03-22 20:00:03.000000000 -0500
> @@ -0,0 +1,12 @@
> +#ifndef _ONETOUCH_H_
> +#define _ONETOUCH_H_
> +
> +#define ONETOUCH_PKT_LEN 0x02
> +#define ONETOUCH_BUTTON KEY_PROG1
> +#define VENDOR_MAXTOR 0x0d49
> +#define PRODUCT_ONETOUCH 0x7010
> +
> +int onetouch_connect_input(struct us_data *ss);
> +void onetouch_release_input(void *onetouch_);
> +
> +#endif
> diff -urN -X dontdiff linux-2.6.12-rc1-mm1/drivers/usb/storage/unusual_devs.h
> linux-2.6.12-rc1-mm1-onetouch/drivers/usb/storage/unusual_devs.h
> --- linux-2.6.12-rc1-mm1/drivers/usb/storage/unusual_devs.h 2005-03-22
> 18:27:55.000000000 -0500
> +++ linux-2.6.12-rc1-mm1-onetouch/drivers/usb/storage/unusual_devs.h
> 2005-03-22 20:09:12.000000000 -0500
> @@ -161,7 +161,7 @@
> "FinePix 1400Zoom",
> US_SC_UFI, US_PR_DEVICE, NULL, US_FL_FIX_INQUIRY |
> US_FL_SINGLE_LUN),
>
> -/* Reported by Peter W?chtler <[EMAIL PROTECTED]>
> +/* Reported by Peter W?chtler <[EMAIL PROTECTED]>
This part must not be in the patch (if you cannot fix the problem with
character encoding, at least you can remove this hunk from the generated
patch manually).
> * The device needs the flags only.
> */
> UNUSUAL_DEV( 0x04ce, 0x0002, 0x0074, 0x0074,
> @@ -991,3 +991,16 @@
> US_SC_SCSI, US_PR_SDDR55, NULL,
> US_FL_SINGLE_LUN),
> #endif
> +
> +/* Submitted by: Nick Sillik <[EMAIL PROTECTED]>
> + * Needed for OneTouch extension to usb-storage
> + *
> + */
> +#ifdef CONFIG_USB_STORAGE_ONETOUCH
> + UNUSUAL_DEV( 0x0d49, 0x7010, 0x0000, 0x9999,
> + "Maxtor",
> + "OneTouch External Harddrive",
> + US_SC_DEVICE, US_PR_DEVICE, onetouch_connect_input,
> + 0),
> +#endif
> +
> diff -urN -X dontdiff linux-2.6.12-rc1-mm1/drivers/usb/storage/usb.c
> linux-2.6.12-rc1-mm1-onetouch/drivers/usb/storage/usb.c
> --- linux-2.6.12-rc1-mm1/drivers/usb/storage/usb.c 2005-03-22
> 18:27:55.000000000 -0500
> +++ linux-2.6.12-rc1-mm1-onetouch/drivers/usb/storage/usb.c 2005-03-22
> 19:59:13.000000000 -0500
> @@ -90,7 +90,9 @@
> #ifdef CONFIG_USB_STORAGE_JUMPSHOT
> #include "jumpshot.h"
> #endif
> -
> +#ifdef CONFIG_USB_STORAGE_ONETOUCH
> +#include "onetouch.h"
> +#endif
>
> /* Some informational data */
> MODULE_AUTHOR("Matthew Dharm <[EMAIL PROTECTED]>");
pgprWzFnIp2LW.pgp
Description: PGP signature
