Date:	Thu, 26 Jun 2008 12:31:32 +0200
From:	Oliver Neukum <oliver@...kum.org>
To:	"Alfred E. Heggestad" <aeh@...org>
Cc:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] input: driver for USB VoIP phones with CM109 chipset

Am Mittwoch 25 Juni 2008 22:07:34 schrieb Alfred E. Heggestad:

Very well

- urb->status is about to go away and replaced by a parameter
- reliably kill two URBs submitting each other cannot be done with
  usb_kill_urb() alone
- you close the device but leave the buzzer on
- input_ev() races with disconnect
- no support for suspend/resume
- no support for pre/post_reset

Could you test this additional patch?

	Regards
		Oliver

--- linux-2.6.26-sierra/drivers/input/misc/cm109.alt.c	2008-06-26 08:15:16.000000000 +0200
+++ linux-2.6.26-sierra/drivers/input/misc/cm109.c	2008-06-26 12:02:54.000000000 +0200
@@ -93,6 +93,7 @@ enum {USB_PKT_LEN = sizeof(struct cm109_
 struct cm109_dev {
 	struct input_dev *idev;	 /* input device */
 	struct usb_device *udev; /* usb device */
+	struct usb_interface *intf;
 
 	/* irq input channel */
 	struct cm109_ctl_packet *irq_data;
@@ -107,7 +108,12 @@ struct cm109_dev {
 	struct urb *urb_ctl;
 
 	spinlock_t submit_lock;
-	int disconnecting;
+	char disconnecting:1;
+	char shutting_down:1;
+	char buzz_state:1;
+	char open:1;
+	char resetting:1;
+	wait_queue_head_t wait;
 
 	char phys[64];		/* physical device path */
 	int key_code;		/* last reported key */
@@ -115,6 +121,9 @@ struct cm109_dev {
 	u8 gpi;			/* Cached value of GPI (high nibble) */
 };
 
+static DEFINE_MUTEX(close_disc_mutex);
+static int buzz(struct cm109_dev *dev, int on);
+
 /******************************************************************************
  * CM109 key interface
  *****************************************************************************/
@@ -279,7 +288,7 @@ static void report_key(struct cm109_dev
 static void urb_irq_callback(struct urb *urb)
 {
 	struct cm109_dev *dev = urb->context;
-	int ret;
+	int ret, status = urb->status;
 
 #if CM109_DEBUG
 	info("### URB IRQ: [0x%02x 0x%02x 0x%02x 0x%02x] keybit=0x%02x",
@@ -290,10 +299,10 @@ static void urb_irq_callback(struct urb
 	     dev->keybit);
 #endif
 
-	if (urb->status) {
-		if (-ESHUTDOWN == urb->status)
+	if (status) {
+		if (-ESHUTDOWN == status)
 			return;
-		err("%s - urb status %d", __FUNCTION__, urb->status);
+		err("%s - urb status %d", __func__, status);
 	}
 
 	/* Scan key column */
@@ -319,15 +328,19 @@ static void urb_irq_callback(struct urb
 	dev->ctl_data->byte[HID_OR2] = dev->keybit;
 
       out:
-	ret = usb_submit_urb(dev->urb_ctl, GFP_ATOMIC);
-	if (ret)
-		err("%s - usb_submit_urb failed %d", __FUNCTION__, ret);
+	spin_lock(&dev->submit_lock);
+	if (!dev->shutting_down) {
+		ret = usb_submit_urb(dev->urb_ctl, GFP_ATOMIC);
+		if (ret)
+			err("%s - usb_submit_urb failed %d", __func__, ret);
+	}
+	spin_unlock(&dev->submit_lock);
 }
 
 static void urb_ctl_callback(struct urb *urb)
 {
 	struct cm109_dev *dev = urb->context;
-	int ret = 0;
+	int ret = 0, status = urb->status;
 
 #if CM109_DEBUG
 	info("### URB CTL: [0x%02x 0x%02x 0x%02x 0x%02x]",
@@ -337,24 +350,37 @@ static void urb_ctl_callback(struct urb
 	     dev->ctl_data->byte[3]);
 #endif
 
-	if (urb->status)
-		err("%s - urb status %d", __FUNCTION__, urb->status);
+	if (status)
+		err("%s - urb status %d", __func__, status);
 
 	spin_lock(&dev->submit_lock);
 	/* ask for a response */
-	if (!dev->disconnecting)
+	if (!dev->shutting_down)
 		ret = usb_submit_urb(dev->urb_irq, GFP_ATOMIC);
 	spin_unlock(&dev->submit_lock);
 
 	if (ret)
 		err("%s - usb_submit_urb failed %d", __FUNCTION__, ret);
+	wake_up(&dev->wait);
 }
 
 /******************************************************************************
  * input event interface
  *****************************************************************************/
 
-static DEFINE_SPINLOCK(cm109_buzz_lock);
+static void stop_traffic(struct cm109_dev *dev)
+{
+	spin_lock_irq(&dev->submit_lock);
+	dev->shutting_down = 1;
+	spin_unlock_irq(&dev->submit_lock);
+
+	usb_kill_urb(dev->urb_ctl);
+	usb_kill_urb(dev->urb_irq);
+
+	spin_lock_irq(&dev->submit_lock);
+	dev->shutting_down = 0;
+	spin_unlock_irq(&dev->submit_lock);
+}
 
 static int input_open(struct input_dev *idev)
 {
@@ -370,41 +396,81 @@ static int input_open(struct input_dev *
 	dev->ctl_data->byte[HID_OR2] = dev->keybit;
 	dev->ctl_data->byte[HID_OR3] = 0x00;
 
+	ret = usb_autopm_get_interface(dev->intf);
+	if (ret < 0) {
+		err("%s - cannot autoresume, result %d",
+		    __func__, ret);
+		return ret;
+	}
+
+	mutex_lock(&close_disc_mutex);
 	if ((ret = usb_submit_urb(dev->urb_ctl, GFP_KERNEL)) != 0) {
 		err("%s - usb_submit_urb failed with result %d",
-		    __FUNCTION__, ret);
+		    __func__, ret);
+		usb_autopm_put_interface(dev->intf);
+		mutex_unlock(&close_disc_mutex);
 		return ret;
 	}
 
+	dev->open = 1;
+	mutex_unlock(&close_disc_mutex);
+
 	return 0;
 }
 
 static void input_close(struct input_dev *idev)
 {
 	struct cm109_dev *dev = input_get_drvdata(idev);
+	int traffic = 0;
+	int r;
 
-	usb_kill_urb(dev->urb_ctl);
-	usb_kill_urb(dev->urb_irq);
+	stop_traffic(dev);
+	dev->open = 0;
+	spin_lock_irq(&dev->submit_lock);
+	if (!dev->disconnecting && dev->buzz_state) {
+		r = buzz(dev, 0);
+		spin_unlock_irq(&dev->submit_lock);
+		if (!r) {
+			wait_event(dev->wait, !dev->buzz_state);
+			traffic = 1;
+		}
+	} else {
+		spin_unlock_irq(&dev->submit_lock);
+	}
+	if (traffic)
+		stop_traffic(dev);
+
+	mutex_lock(&close_disc_mutex);
+	if (!dev->disconnecting)
+		usb_autopm_put_interface(dev->intf);
+	mutex_unlock(&close_disc_mutex);
 }
 
-static void buzz(struct cm109_dev *dev, int on)
+static int buzz(struct cm109_dev *dev, int on)
 {
-	int ret;
+	int ret = 0;
 
 	if (dev == NULL) {
 		err("buzz: dev is NULL");
-		return;
+		return -EINVAL;
 	}
 
 	dbg("Buzzer %s", on ? "on" : "off");
+	if (dev->resetting)
+		goto skip_io;
 	if (on)
 		dev->ctl_data->byte[HID_OR0] |= BUZZER_ON;
 	else
 		dev->ctl_data->byte[HID_OR0] &= ~BUZZER_ON;
 
 	ret = usb_submit_urb(dev->urb_ctl, GFP_ATOMIC);
-	if (ret)
-		err("%s - usb_submit_urb failed %d", __FUNCTION__, ret);
+	if (ret) {
+		err("%s - usb_submit_urb failed %d", __func__, ret);
+	} else {
+skip_io:
+		dev->buzz_state = on ? 1 : 0;
+	}
+	return ret;
 }
 
 static int input_ev(struct input_dev *idev, unsigned int type,
@@ -412,6 +478,7 @@ static int input_ev(struct input_dev *id
 {
 	struct cm109_dev *dev = input_get_drvdata(idev);
 	unsigned long flags;
+	int rv = 0;
 
 #if CM109_DEBUG
 	info("input_ev: type=%u code=%u value=%d", type, code, value);
@@ -428,11 +495,12 @@ static int input_ev(struct input_dev *id
 		return -EINVAL;
 	}
 
-	spin_lock_irqsave(&cm109_buzz_lock, flags);
-	buzz(dev, value);
-	spin_unlock_irqrestore(&cm109_buzz_lock, flags);
+	spin_lock_irqsave(&dev->submit_lock, flags);
+	if (!dev->disconnecting)
+		rv = buzz(dev, value);
+	spin_unlock_irqrestore(&dev->submit_lock, flags);
 
-	return 0;
+	return rv;
 }
 
 
@@ -468,13 +536,12 @@ static const struct usb_device_id usb_ta
 	{}
 };
 
-static int usb_cleanup(struct cm109_dev *dev, int err)
+static void usb_cleanup(struct cm109_dev *dev, int err)
 {
 	if (dev == NULL)
-		return err;
+		return;
 
-	usb_kill_urb(dev->urb_irq);	/* parameter validation in core/urb */
-	usb_kill_urb(dev->urb_ctl);	/* parameter validation in core/urb */
+	stop_traffic(dev);
 
 	if (dev->idev) {
 		if (err)
@@ -495,8 +562,6 @@ static int usb_cleanup(struct cm109_dev
 	usb_free_urb(dev->urb_irq);	/* parameter validation in core/urb */
 	usb_free_urb(dev->urb_ctl);	/* parameter validation in core/urb */
 	kfree(dev);
-
-	return err;
 }
 
 static void usb_disconnect(struct usb_interface *interface)
@@ -506,9 +571,12 @@ static void usb_disconnect(struct usb_in
 	dev = usb_get_intfdata(interface);
 
 	/* Wait for URB idle */
+	
+	mutex_lock(&close_disc_mutex);
 	spin_lock_irq(&dev->submit_lock);
 	dev->disconnecting = 1;
 	spin_unlock_irq(&dev->submit_lock);
+	mutex_unlock(&close_disc_mutex);
 
 	usb_set_intfdata(interface, NULL);
 
@@ -536,9 +604,9 @@ static int usb_probe(struct usb_interfac
 		return -ENOMEM;
 
 	spin_lock_init(&dev->submit_lock);
-	dev->disconnecting = 0;
 
 	dev->udev = udev;
+	dev->intf = intf;
 
 	dev->idev = input_dev = input_allocate_device();
 	if (!input_dev)
@@ -638,14 +706,81 @@ static int usb_probe(struct usb_interfac
 	return 0;
 
       err:
-	return usb_cleanup(dev, -ENOMEM);
+	usb_cleanup(dev, 1);
+	return -ENOMEM;
+}
+
+static int restore_state(struct cm109_dev *dev)
+{
+	int rv;
+
+	spin_lock_irq(&dev->submit_lock);
+	/* if not open, just restore buzz, else submit urb */
+	dev->shutting_down = dev->open;
+	spin_unlock_irq(&dev->submit_lock);
+	rv = buzz(dev, dev->buzz_state);
+	spin_lock_irq(&dev->submit_lock);
+	dev->shutting_down = 0;
+	spin_unlock_irq(&dev->submit_lock);
+
+	return rv;
+}
+
+static int usb_resume(struct usb_interface *intf)
+{
+	struct cm109_dev *dev = usb_get_intfdata(intf);
+	int rv;
+
+	rv = restore_state(dev);
+
+	return rv;
+}
+
+static int usb_suspend(struct usb_interface *intf, pm_message_t message)
+{
+	struct cm109_dev *dev = usb_get_intfdata(intf);
+
+	stop_traffic(dev);
+	return 0;
+}
+
+static int usb_pre_reset(struct usb_interface *intf)
+{
+	struct cm109_dev *dev = usb_get_intfdata(intf);
+
+	mutex_lock(&close_disc_mutex);
+	spin_lock_irq(&dev->submit_lock);
+	dev->resetting = 1;
+	spin_unlock_irq(&dev->submit_lock);
+	stop_traffic(dev);
+
+	return 0;
+}
+
+static int usb_post_reset(struct usb_interface *intf)
+{
+	struct cm109_dev *dev = usb_get_intfdata(intf);
+	int rv;
+
+	spin_lock_irq(&dev->submit_lock);
+	dev->resetting = 0;
+	spin_unlock_irq(&dev->submit_lock);
+	rv = restore_state(dev);
+	mutex_unlock(&close_disc_mutex);
+	return rv;
 }
 
 static struct usb_driver cm109_driver = {
-	.name       = "cm109",
-	.probe      = usb_probe,
-	.disconnect = usb_disconnect,
-	.id_table   = usb_table,
+	.name		= "cm109",
+	.probe		= usb_probe,
+	.disconnect	= usb_disconnect,
+	.resume		= usb_resume,
+	.reset_resume	= usb_resume,
+	.suspend	= usb_suspend,
+	.pre_reset	= usb_pre_reset,
+	.post_reset	= usb_post_reset,
+	.id_table	= usb_table,
+	.supports_autosuspend = 1,
 };
 
 static int __init select_keymap(void)
@@ -685,7 +820,7 @@ static int __init cm109_dev_init(void)
 
 	info(DRIVER_DESC ": " DRIVER_VERSION " (C) " DRIVER_AUTHOR);
 
-	return err;
+	return 0;
 }
 
 static void __exit cm109_dev_exit(void)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists