* [patch]reliably killing urbs in yealink driver
@ 2008-06-26 11:44 Oliver Neukum
2008-06-26 14:26 ` Dmitry Torokhov
2008-06-28 15:11 ` Alfred E. Heggestad
0 siblings, 2 replies; 7+ messages in thread
From: Oliver Neukum @ 2008-06-26 11:44 UTC (permalink / raw)
To: Dmitry Torokhov, Alfred E. Heggestad, Jiri Kosina,
linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
Henk.Vergonet-Re5JQEeQqe8AvxtiuMwx3w
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch]reliably killing urbs in yealink driver
2008-06-26 11:44 [patch]reliably killing urbs in yealink driver Oliver Neukum
@ 2008-06-26 14:26 ` Dmitry Torokhov
[not found] ` <20080626142633.GA16831-692KRdr1A+M6QviIW9vzWfIbXMQ5te18@public.gmane.org>
2008-06-28 15:11 ` Alfred E. Heggestad
1 sibling, 1 reply; 7+ messages in thread
From: Dmitry Torokhov @ 2008-06-26 14:26 UTC (permalink / raw)
To: Oliver Neukum
Cc: Alfred E. Heggestad, Jiri Kosina, linux-usb, linux-input,
Henk.Vergonet
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch]reliably killing urbs in yealink driver
2008-06-26 11:44 [patch]reliably killing urbs in yealink driver Oliver Neukum
2008-06-26 14:26 ` Dmitry Torokhov
@ 2008-06-28 15:11 ` Alfred E. Heggestad
2008-06-30 12:10 ` Oliver Neukum
1 sibling, 1 reply; 7+ messages in thread
From: Alfred E. Heggestad @ 2008-06-28 15:11 UTC (permalink / raw)
To: Oliver Neukum
Cc: Dmitry Torokhov, Jiri Kosina, linux-usb, linux-input,
Henk.Vergonet
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)
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [patch]reliably killing urbs in yealink driver
2008-06-28 15:11 ` Alfred E. Heggestad
@ 2008-06-30 12:10 ` Oliver Neukum
0 siblings, 0 replies; 7+ messages in thread
From: Oliver Neukum @ 2008-06-30 12:10 UTC (permalink / raw)
To: Alfred E. Heggestad
Cc: Dmitry Torokhov, Jiri Kosina, linux-usb, linux-input,
Henk.Vergonet
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)
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2008-06-30 12:10 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-06-26 11:44 [patch]reliably killing urbs in yealink driver Oliver Neukum
2008-06-26 14:26 ` Dmitry Torokhov
[not found] ` <20080626142633.GA16831-692KRdr1A+M6QviIW9vzWfIbXMQ5te18@public.gmane.org>
2008-06-26 14:32 ` Oliver Neukum
2008-06-26 17:43 ` Dmitry Torokhov
2008-06-27 12:00 ` Oliver Neukum
2008-06-28 15:11 ` Alfred E. Heggestad
2008-06-30 12:10 ` Oliver Neukum
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).