[patch]reliably killing urbs in yealink driver

[patch]reliably killing urbs in yealink driver

[Oliver Neukum]

Hi,

yealink uses two URBs that submit each other. This arrangement cannot
be reliably killed with usb_kill_urb() alone, as there's a window during which
the wrong URB may be killed. The fix is to introduce a flag.

	Regards
		Oliver

Signed-off-by: Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org>

---

--- linux-2.6.26-sierra/drivers/input/misc/yealink.alt.c	2008-06-24 10:17:14.000000000 +0200
+++ linux-2.6.26-sierra/drivers/input/misc/yealink.c	2008-06-26 13:34:35.000000000 +0200
@@ -424,10 +424,10 @@ send_update:
 static void urb_irq_callback(struct urb *urb)
 {
 	struct yealink_dev *yld = urb->context;
-	int ret;
+	int ret, status = urb->status;
 
-	if (urb->status)
-		err("%s - urb status %d", __FUNCTION__, urb->status);
+	if (status)
+		err("%s - urb status %d", __func__, status);
 
 	switch (yld->irq_data->cmd) {
 	case CMD_KEYPRESS:
@@ -446,34 +446,43 @@ static void urb_irq_callback(struct urb
 	}
 
 	yealink_do_idle_tasks(yld);
-
-	ret = usb_submit_urb(yld->urb_ctl, GFP_ATOMIC);
-	if (ret)
-		err("%s - usb_submit_urb failed %d", __FUNCTION__, ret);
+	spin_lock(&yld->spin);
+	if (!yld->shutdown) {
+		ret = usb_submit_urb(yld->urb_ctl, GFP_ATOMIC);
+		if (ret && ret != -EPERM)
+			err("%s - usb_submit_urb failed %d", __func__, ret);
+	}
+	spin_unlock(&yld->spin);
 }
 
 static void urb_ctl_callback(struct urb *urb)
 {
 	struct yealink_dev *yld = urb->context;
-	int ret;
+	int ret = 0, status = urb->status;
 
-	if (urb->status)
-		err("%s - urb status %d", __FUNCTION__, urb->status);
+	if (status)
+		err("%s - urb status %d", __func__, status);
 
 	switch (yld->ctl_data->cmd) {
 	case CMD_KEYPRESS:
 	case CMD_SCANCODE:
 		/* ask for a response */
-		ret = usb_submit_urb(yld->urb_irq, GFP_ATOMIC);
+		spin_lock(&yld->spin);
+		if (!yld->shutdown)
+			ret = usb_submit_urb(yld->urb_irq, GFP_ATOMIC);
+		spin_unlock(&yld->spin);
 		break;
 	default:
 		/* send new command */
 		yealink_do_idle_tasks(yld);
-		ret = usb_submit_urb(yld->urb_ctl, GFP_ATOMIC);
+		spin_lock(&yld->spin);
+		if (!yld->shutdown)
+			ret = usb_submit_urb(yld->urb_ctl, GFP_ATOMIC);
+		spin_unlock(&yld->spin);
 	}
 
-	if (ret)
-		err("%s - usb_submit_urb failed %d", __FUNCTION__, ret);
+	if (ret && ret != -EPERM)
+		err("%s - usb_submit_urb failed %d", __func__, ret);
 }
 
 /*******************************************************************************
@@ -527,12 +536,25 @@ static int input_open(struct input_dev *
 	return 0;
 }
 
-static void input_close(struct input_dev *dev)
+static void stop_traffic(struct yealink_dev *yld)
 {
-	struct yealink_dev *yld = input_get_drvdata(dev);
+	spin_lock_irq(&yld->spin);
+	yld->shutdown = 1;
+	spin_unlock_irq(&yld->spin);
 
 	usb_kill_urb(yld->urb_ctl);
 	usb_kill_urb(yld->urb_irq);
+
+	spin_lock_irq(&yld->spin);
+	yld->shutdown = 0;
+	spin_unlock_irq(&yld->spin);
+}
+
+static void input_close(struct input_dev *dev)
+{
+	struct yealink_dev *yld = input_get_drvdata(dev);
+
+	stop_traffic(yld);
 }
 
 /*******************************************************************************
@@ -809,8 +831,7 @@ static int usb_cleanup(struct yealink_de
 	if (yld == NULL)
 		return err;
 
-	usb_kill_urb(yld->urb_irq);	/* parameter validation in core/urb */
-	usb_kill_urb(yld->urb_ctl);	/* parameter validation in core/urb */
+	stop_traffic(yld);
 
         if (yld->idev) {
 		if (err)
@@ -866,6 +887,7 @@ static int usb_probe(struct usb_interfac
 		return -ENOMEM;
 
 	yld->udev = udev;
+	spin_lock_init(&yld->spin);
 
 	yld->idev = input_dev = input_allocate_device();
 	if (!input_dev)

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch]reliably killing urbs in yealink driver

[Dmitry Torokhov]

Hi Oliver,

On Thu, Jun 26, 2008 at 01:44:30PM +0200, Oliver Neukum wrote:
> @@ -809,8 +831,7 @@ static int usb_cleanup(struct yealink_de
>  	if (yld == NULL)
>  		return err;
>  
> -	usb_kill_urb(yld->urb_irq);	/* parameter validation in core/urb */
> -	usb_kill_urb(yld->urb_ctl);	/* parameter validation in core/urb */
> +	stop_traffic(yld);

stop_traffic is unneeded here since input core guarantees that close is
called when you unregister and opened device. Do you have the hardware
to test? If not I will fold stop_traffic back into yealink_close
locally.

-- 
Dmitry

Re: [patch]reliably killing urbs in yealink driver

[Oliver Neukum]

Am Donnerstag 26 Juni 2008 16:26:34 schrieb Dmitry Torokhov:
> Hi Oliver,
> 
> On Thu, Jun 26, 2008 at 01:44:30PM +0200, Oliver Neukum wrote:
> > @@ -809,8 +831,7 @@ static int usb_cleanup(struct yealink_de
> >  	if (yld == NULL)
> >  		return err;
> >  
> > -	usb_kill_urb(yld->urb_irq);	/* parameter validation in core/urb */
> > -	usb_kill_urb(yld->urb_ctl);	/* parameter validation in core/urb */
> > +	stop_traffic(yld);
> 
> stop_traffic is unneeded here since input core guarantees that close is
> called when you unregister and opened device. Do you have the hardware
> to test? If not I will fold stop_traffic back into yealink_close
> locally.
> 

No, I don't have hardware to test.

	Regards
		Oliver

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch]reliably killing urbs in yealink driver

[Dmitry Torokhov]

On Thu, Jun 26, 2008 at 04:32:16PM +0200, Oliver Neukum wrote:
> Am Donnerstag 26 Juni 2008 16:26:34 schrieb Dmitry Torokhov:
> > Hi Oliver,
> > 
> > On Thu, Jun 26, 2008 at 01:44:30PM +0200, Oliver Neukum wrote:
> > > @@ -809,8 +831,7 @@ static int usb_cleanup(struct yealink_de
> > >  	if (yld == NULL)
> > >  		return err;
> > >  
> > > -	usb_kill_urb(yld->urb_irq);	/* parameter validation in core/urb */
> > > -	usb_kill_urb(yld->urb_ctl);	/* parameter validation in core/urb */
> > > +	stop_traffic(yld);
> > 
> > stop_traffic is unneeded here since input core guarantees that close is
> > called when you unregister and opened device. Do you have the hardware
> > to test? If not I will fold stop_traffic back into yealink_close
> > locally.
> > 
> 
> No, I don't have hardware to test.
> 

OK, then I'll change it. I wonder whether we really need spinlock
though or a simple wmb() is enough to make sure other CPUs notice
the flag being set before we get to executing usb_kill_urb()...

-- 
Dmitry

Re: [patch]reliably killing urbs in yealink driver

[Oliver Neukum]

Am Donnerstag 26 Juni 2008 19:43:15 schrieb Dmitry Torokhov:
> On Thu, Jun 26, 2008 at 04:32:16PM +0200, Oliver Neukum wrote:
> > Am Donnerstag 26 Juni 2008 16:26:34 schrieb Dmitry Torokhov:
> > > Hi Oliver,
> > > 
> > > On Thu, Jun 26, 2008 at 01:44:30PM +0200, Oliver Neukum wrote:
> > > > @@ -809,8 +831,7 @@ static int usb_cleanup(struct yealink_de
> > > >  	if (yld == NULL)
> > > >  		return err;
> > > >  
> > > > -	usb_kill_urb(yld->urb_irq);	/* parameter validation in core/urb */
> > > > -	usb_kill_urb(yld->urb_ctl);	/* parameter validation in core/urb */
> > > > +	stop_traffic(yld);
> > > 
> > > stop_traffic is unneeded here since input core guarantees that close is
> > > called when you unregister and opened device. Do you have the hardware
> > > to test? If not I will fold stop_traffic back into yealink_close
> > > locally.
> > > 
> > 
> > No, I don't have hardware to test.
> > 
> 
> OK, then I'll change it. I wonder whether we really need spinlock
> though or a simple wmb() is enough to make sure other CPUs notice
> the flag being set before we get to executing usb_kill_urb()...

I am unsure and wouldn't try anything so subtle without a clear need.

	Regards
		Oliver

Re: [patch]reliably killing urbs in yealink driver

[Alfred E. Heggestad]

Oliver Neukum wrote:
> Hi,
> 
> yealink uses two URBs that submit each other. This arrangement cannot
> be reliably killed with usb_kill_urb() alone, as there's a window during which
> the wrong URB may be killed. The fix is to introduce a flag.
> 

Hi Oliver,

just a small comments about this patch;

- the declaration of 'spin' and 'shutdown' is missing,
   and hence the module does not build.

something like this should be added to struct yealink_dev:

         spinlock_t spin;
         char shutdown:1;


/alfred

> 	Regards
> 		Oliver
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.de>
> 
> ---
> 
> --- linux-2.6.26-sierra/drivers/input/misc/yealink.alt.c	2008-06-24 10:17:14.000000000 +0200
> +++ linux-2.6.26-sierra/drivers/input/misc/yealink.c	2008-06-26 13:34:35.000000000 +0200
> @@ -424,10 +424,10 @@ send_update:
>  static void urb_irq_callback(struct urb *urb)
>  {
>  	struct yealink_dev *yld = urb->context;
> -	int ret;
> +	int ret, status = urb->status;
>  
> -	if (urb->status)
> -		err("%s - urb status %d", __FUNCTION__, urb->status);
> +	if (status)
> +		err("%s - urb status %d", __func__, status);
>  
>  	switch (yld->irq_data->cmd) {
>  	case CMD_KEYPRESS:
> @@ -446,34 +446,43 @@ static void urb_irq_callback(struct urb
>  	}
>  
>  	yealink_do_idle_tasks(yld);
> -
> -	ret = usb_submit_urb(yld->urb_ctl, GFP_ATOMIC);
> -	if (ret)
> -		err("%s - usb_submit_urb failed %d", __FUNCTION__, ret);
> +	spin_lock(&yld->spin);
> +	if (!yld->shutdown) {
> +		ret = usb_submit_urb(yld->urb_ctl, GFP_ATOMIC);
> +		if (ret && ret != -EPERM)
> +			err("%s - usb_submit_urb failed %d", __func__, ret);
> +	}
> +	spin_unlock(&yld->spin);
>  }
>  
>  static void urb_ctl_callback(struct urb *urb)
>  {
>  	struct yealink_dev *yld = urb->context;
> -	int ret;
> +	int ret = 0, status = urb->status;
>  
> -	if (urb->status)
> -		err("%s - urb status %d", __FUNCTION__, urb->status);
> +	if (status)
> +		err("%s - urb status %d", __func__, status);
>  
>  	switch (yld->ctl_data->cmd) {
>  	case CMD_KEYPRESS:
>  	case CMD_SCANCODE:
>  		/* ask for a response */
> -		ret = usb_submit_urb(yld->urb_irq, GFP_ATOMIC);
> +		spin_lock(&yld->spin);
> +		if (!yld->shutdown)
> +			ret = usb_submit_urb(yld->urb_irq, GFP_ATOMIC);
> +		spin_unlock(&yld->spin);
>  		break;
>  	default:
>  		/* send new command */
>  		yealink_do_idle_tasks(yld);
> -		ret = usb_submit_urb(yld->urb_ctl, GFP_ATOMIC);
> +		spin_lock(&yld->spin);
> +		if (!yld->shutdown)
> +			ret = usb_submit_urb(yld->urb_ctl, GFP_ATOMIC);
> +		spin_unlock(&yld->spin);
>  	}
>  
> -	if (ret)
> -		err("%s - usb_submit_urb failed %d", __FUNCTION__, ret);
> +	if (ret && ret != -EPERM)
> +		err("%s - usb_submit_urb failed %d", __func__, ret);
>  }
>  
>  /*******************************************************************************
> @@ -527,12 +536,25 @@ static int input_open(struct input_dev *
>  	return 0;
>  }
>  
> -static void input_close(struct input_dev *dev)
> +static void stop_traffic(struct yealink_dev *yld)
>  {
> -	struct yealink_dev *yld = input_get_drvdata(dev);
> +	spin_lock_irq(&yld->spin);
> +	yld->shutdown = 1;
> +	spin_unlock_irq(&yld->spin);
>  
>  	usb_kill_urb(yld->urb_ctl);
>  	usb_kill_urb(yld->urb_irq);
> +
> +	spin_lock_irq(&yld->spin);
> +	yld->shutdown = 0;
> +	spin_unlock_irq(&yld->spin);
> +}
> +
> +static void input_close(struct input_dev *dev)
> +{
> +	struct yealink_dev *yld = input_get_drvdata(dev);
> +
> +	stop_traffic(yld);
>  }
>  
>  /*******************************************************************************
> @@ -809,8 +831,7 @@ static int usb_cleanup(struct yealink_de
>  	if (yld == NULL)
>  		return err;
>  
> -	usb_kill_urb(yld->urb_irq);	/* parameter validation in core/urb */
> -	usb_kill_urb(yld->urb_ctl);	/* parameter validation in core/urb */
> +	stop_traffic(yld);
>  
>          if (yld->idev) {
>  		if (err)
> @@ -866,6 +887,7 @@ static int usb_probe(struct usb_interfac
>  		return -ENOMEM;
>  
>  	yld->udev = udev;
> +	spin_lock_init(&yld->spin);
>  
>  	yld->idev = input_dev = input_allocate_device();
>  	if (!input_dev)
> 

Re: [patch]reliably killing urbs in yealink driver

[Oliver Neukum]

Am Samstag 28 Juni 2008 17:11:17 schrieb Alfred E. Heggestad:
> Oliver Neukum wrote:
> > Hi,
> > 
> > yealink uses two URBs that submit each other. This arrangement cannot
> > be reliably killed with usb_kill_urb() alone, as there's a window during which
> > the wrong URB may be killed. The fix is to introduce a flag.
> > 
> 
> Hi Oliver,
> 
> just a small comments about this patch;
> 
> - the declaration of 'spin' and 'shutdown' is missing,
>    and hence the module does not build.
> 
> something like this should be added to struct yealink_dev:
> 
>          spinlock_t spin;
>          char shutdown:1;
> 

Very true. My testing environment was bad. Dmitry, please ignore
the original patch, here's a working patch.

	Regards
		Oliver

Signed-off-by: Oliver Neukum <oneukum@suse.de>

---

--- linux-2.6.26-sierra/drivers/input/misc/yealink.alt.c	2008-06-24 10:17:14.000000000 +0200
+++ linux-2.6.26-sierra/drivers/input/misc/yealink.c	2008-06-30 14:02:17.000000000 +0200
@@ -101,6 +101,7 @@ static const struct lcd_segment_map {
 struct yealink_dev {
 	struct input_dev *idev;		/* input device */
 	struct usb_device *udev;	/* usb device */
+	spinlock_t spin;
 
 	/* irq input channel */
 	struct yld_ctl_packet	*irq_data;
@@ -118,6 +119,7 @@ struct yealink_dev {
 
 	u8 lcdMap[ARRAY_SIZE(lcdMap)];	/* state of LCD, LED ... */
 	int key_code;			/* last reported key	 */
+	int shutdown:1;
 
 	int	stat_ix;
 	union {
@@ -424,10 +426,10 @@ send_update:
 static void urb_irq_callback(struct urb *urb)
 {
 	struct yealink_dev *yld = urb->context;
-	int ret;
+	int ret, status = urb->status;
 
-	if (urb->status)
-		err("%s - urb status %d", __FUNCTION__, urb->status);
+	if (status)
+		err("%s - urb status %d", __func__, status);
 
 	switch (yld->irq_data->cmd) {
 	case CMD_KEYPRESS:
@@ -446,34 +448,43 @@ static void urb_irq_callback(struct urb
 	}
 
 	yealink_do_idle_tasks(yld);
-
-	ret = usb_submit_urb(yld->urb_ctl, GFP_ATOMIC);
-	if (ret)
-		err("%s - usb_submit_urb failed %d", __FUNCTION__, ret);
+	spin_lock(&yld->spin);
+	if (!yld->shutdown) {
+		ret = usb_submit_urb(yld->urb_ctl, GFP_ATOMIC);
+		if (ret && ret != -EPERM)
+			err("%s - usb_submit_urb failed %d", __func__, ret);
+	}
+	spin_unlock(&yld->spin);
 }
 
 static void urb_ctl_callback(struct urb *urb)
 {
 	struct yealink_dev *yld = urb->context;
-	int ret;
+	int ret = 0, status = urb->status;
 
-	if (urb->status)
-		err("%s - urb status %d", __FUNCTION__, urb->status);
+	if (status)
+		err("%s - urb status %d", __func__, status);
 
 	switch (yld->ctl_data->cmd) {
 	case CMD_KEYPRESS:
 	case CMD_SCANCODE:
 		/* ask for a response */
-		ret = usb_submit_urb(yld->urb_irq, GFP_ATOMIC);
+		spin_lock(&yld->spin);
+		if (!yld->shutdown)
+			ret = usb_submit_urb(yld->urb_irq, GFP_ATOMIC);
+		spin_unlock(&yld->spin);
 		break;
 	default:
 		/* send new command */
 		yealink_do_idle_tasks(yld);
-		ret = usb_submit_urb(yld->urb_ctl, GFP_ATOMIC);
+		spin_lock(&yld->spin);
+		if (!yld->shutdown)
+			ret = usb_submit_urb(yld->urb_ctl, GFP_ATOMIC);
+		spin_unlock(&yld->spin);
 	}
 
-	if (ret)
-		err("%s - usb_submit_urb failed %d", __FUNCTION__, ret);
+	if (ret && ret != -EPERM)
+		err("%s - usb_submit_urb failed %d", __func__, ret);
 }
 
 /*******************************************************************************
@@ -527,12 +538,25 @@ static int input_open(struct input_dev *
 	return 0;
 }
 
-static void input_close(struct input_dev *dev)
+static void stop_traffic(struct yealink_dev *yld)
 {
-	struct yealink_dev *yld = input_get_drvdata(dev);
+	spin_lock_irq(&yld->spin);
+	yld->shutdown = 1;
+	spin_unlock_irq(&yld->spin);
 
 	usb_kill_urb(yld->urb_ctl);
 	usb_kill_urb(yld->urb_irq);
+
+	spin_lock_irq(&yld->spin);
+	yld->shutdown = 0;
+	spin_unlock_irq(&yld->spin);
+}
+
+static void input_close(struct input_dev *dev)
+{
+	struct yealink_dev *yld = input_get_drvdata(dev);
+
+	stop_traffic(yld);
 }
 
 /*******************************************************************************
@@ -809,8 +833,7 @@ static int usb_cleanup(struct yealink_de
 	if (yld == NULL)
 		return err;
 
-	usb_kill_urb(yld->urb_irq);	/* parameter validation in core/urb */
-	usb_kill_urb(yld->urb_ctl);	/* parameter validation in core/urb */
+	stop_traffic(yld);
 
         if (yld->idev) {
 		if (err)
@@ -866,6 +889,7 @@ static int usb_probe(struct usb_interfac
 		return -ENOMEM;
 
 	yld->udev = udev;
+	spin_lock_init(&yld->spin);
 
 	yld->idev = input_dev = input_allocate_device();
 	if (!input_dev)