Re: USB: FIx locks and urb->status in adutux
On Thu, 01 Nov 2007 11:06:24 +0200, Vitaliy Ivanov <[EMAIL PROTECTED]> wrote: > > Do you still want to go ahead with a 2.4 backport? > > Yep. Will do it asap. In latest 2.4 patch we had just a locks issue. > So do you think I should modify 2.4 patch with all modifications we done in > 2.6 version included? Something like that, yes. -- Pete - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb->status in adutux
On Thu, 2007-11-01 at 00:01, Pete Zaitcev wrote: > Sorry about that. I'll try to be more explicit in the future. OK, got it. Thanks. > > I just tried the latest patch and all seems to be good. > > BTW, slab corruption issue that I saw on the original driver we started > > fixing on is not an issue any more. > > Very well, I'll post this for Greg anew today. Great, tnx. > Do you still want to go ahead with a 2.4 backport? Yep. Will do it asap. In latest 2.4 patch we had just a locks issue. So do you think I should modify 2.4 patch with all modifications we done in 2.6 version included? Vitaliy - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb-status in adutux
On Thu, 2007-11-01 at 00:01, Pete Zaitcev wrote: Sorry about that. I'll try to be more explicit in the future. OK, got it. Thanks. I just tried the latest patch and all seems to be good. BTW, slab corruption issue that I saw on the original driver we started fixing on is not an issue any more. Very well, I'll post this for Greg anew today. Great, tnx. Do you still want to go ahead with a 2.4 backport? Yep. Will do it asap. In latest 2.4 patch we had just a locks issue. So do you think I should modify 2.4 patch with all modifications we done in 2.6 version included? Vitaliy - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb-status in adutux
On Thu, 01 Nov 2007 11:06:24 +0200, Vitaliy Ivanov [EMAIL PROTECTED] wrote: Do you still want to go ahead with a 2.4 backport? Yep. Will do it asap. In latest 2.4 patch we had just a locks issue. So do you think I should modify 2.4 patch with all modifications we done in 2.6 version included? Something like that, yes. -- Pete - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
USB: FIx locks and urb->status in adutux (updated)
Two main issues fixed here are:
- An improper use of in-struct lock to protect an open count
- Use of urb status for -EINPROGRESS
Also, along the way:
- Change usb_unlink_urb to usb_kill_urb. Apparently there's no need
to use usb_unlink_urb whatsoever in this driver, and the old use of
usb_kill_urb was outright racy (it unlinked and immediately freed).
- Fix indentation in adu_write. Looks like it was damaged by a script.
- Vitaly wants -EBUSY on multiply opens.
- bInterval was taken from a wrong endpoint.
Signed-off-by: Pete Zaitcev <[EMAIL PROTECTED]>
Signed-off-by: Vitaliy Ivanov <[EMAIL PROTECTED]>
Tested-by: Vitaliy Ivanov <[EMAIL PROTECTED]>
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..5a2c44e 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -79,12 +79,22 @@ MODULE_DEVICE_TABLE(usb, device_table);
#define COMMAND_TIMEOUT(2*HZ) /* 60 second timeout for a command */
+/*
+ * The locking scheme is a vanilla 3-lock:
+ * adu_device.buflock: A spinlock, covers what IRQs touch.
+ * adutux_mutex: A Static lock to cover open_count. It would also cover
+ * any globals, but we don't have them in 2.6.
+ * adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
+ * It covers all of adu_device, except the open_count
+ * and what .buflock covers.
+ */
+
/* Structure to hold all of our device specific stuff */
struct adu_device {
- struct mutexmtx; /* locks this structure */
+ struct mutexmtx;
struct usb_device* udev; /* save off the usb device pointer */
struct usb_interface* interface;
- unsigned char minor; /* the starting minor number for this
device */
+ unsigned intminor; /* the starting minor number for this
device */
charserial_number[8];
int open_count; /* number of times this port has
been opened */
@@ -107,8 +117,11 @@ struct adu_device {
char* interrupt_out_buffer;
struct usb_endpoint_descriptor* interrupt_out_endpoint;
struct urb* interrupt_out_urb;
+ int out_urb_finished;
};
+static DEFINE_MUTEX(adutux_mutex);
+
static struct usb_driver adu_driver;
static void adu_debug_data(int level, const char *function, int size,
@@ -132,27 +145,31 @@ static void adu_debug_data(int level, const char
*function, int size,
*/
static void adu_abort_transfers(struct adu_device *dev)
{
- dbg(2," %s : enter", __FUNCTION__);
+ unsigned long flags;
- if (dev == NULL) {
- dbg(1," %s : dev is null", __FUNCTION__);
- goto exit;
- }
+ dbg(2," %s : enter", __FUNCTION__);
if (dev->udev == NULL) {
dbg(1," %s : udev is null", __FUNCTION__);
goto exit;
}
- dbg(2," %s : udev state %d", __FUNCTION__, dev->udev->state);
- if (dev->udev->state == USB_STATE_NOTATTACHED) {
- dbg(1," %s : udev is not attached", __FUNCTION__);
- goto exit;
- }
-
/* shutdown transfer */
- usb_unlink_urb(dev->interrupt_in_urb);
- usb_unlink_urb(dev->interrupt_out_urb);
+
+ /* XXX Anchor these instead */
+ spin_lock_irqsave(>buflock, flags);
+ if (!dev->read_urb_finished) {
+ spin_unlock_irqrestore(>buflock, flags);
+ usb_kill_urb(dev->interrupt_in_urb);
+ } else
+ spin_unlock_irqrestore(>buflock, flags);
+
+ spin_lock_irqsave(>buflock, flags);
+ if (!dev->out_urb_finished) {
+ spin_unlock_irqrestore(>buflock, flags);
+ usb_kill_urb(dev->interrupt_out_urb);
+ } else
+ spin_unlock_irqrestore(>buflock, flags);
exit:
dbg(2," %s : leave", __FUNCTION__);
@@ -162,8 +179,6 @@ static void adu_delete(struct adu_device *dev)
{
dbg(2, "%s enter", __FUNCTION__);
- adu_abort_transfers(dev);
-
/* free data structures */
usb_free_urb(dev->interrupt_in_urb);
usb_free_urb(dev->interrupt_out_urb);
@@ -239,7 +254,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
goto exit;
}
- wake_up_interruptible(>write_wait);
+ spin_lock(>buflock);
+ dev->out_urb_finished = 1;
+ wake_up(>write_wait);
+ spin_unlock(>buflock);
exit:
adu_debug_data(5, __FUNCTION__, urb->actual_length,
@@ -252,12 +270,17 @@ static int adu_open(struct inode *inode, struct file
*file)
struct adu_device *dev = NULL;
struct usb_interface *interface;
int subminor;
- int retval = 0;
+ int retval;
dbg(2,"%s : enter", __FUNCTION__);
subminor = iminor(inode);
+ if ((retval =
Re: USB: FIx locks and urb->status in adutux
On Wed, 31 Oct 2007 13:54:54 +0200, Vitaliy Ivanov <[EMAIL PROTECTED]> wrote:
> On Tue, 2007-10-30 at 23:54, Pete Zaitcev wrote:
>
> > One other small thing. I see you dropped the dev->mtx bracket that
> > I added around the initialization and submission. Notice that the
> > dev->udev is zeroed under dev->mtx, not the static lock. This is because
> > it has to be seen in read and write paths. If you don't like taking
> > across the submission, how about testing for it ahead of time?
>
> I thought it can be managed under static lock.
The paragraph you quoted above explains why dev->udev cannot be managed
under the static lock: because dev->udev is accessed by read/write methods,
which do not take the static lock.
> I'm not sure what kind of testing do you mean by "ahead of time".
No, I meant testing before the rest of the ->open method is executed,
sorry. This part is "ahead of" the rest:
@@ -267,54 +290,54 @@ static int adu_open(struct inode *inode, struct file
*file)
}
dev = usb_get_intfdata(interface);
- if (!dev) {
+ if (!dev || !dev->udev) {
retval = -ENODEV;
goto exit_no_device;
}
Sorry about that. I'll try to be more explicit in the future.
> I just tried the latest patch and all seems to be good.
> BTW, slab corruption issue that I saw on the original driver we started
> fixing on is not an issue any more.
Very well, I'll post this for Greg anew today.
Do you still want to go ahead with a 2.4 backport?
-- Pete
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb->status in adutux
On Tue, 2007-10-30 at 23:54, Pete Zaitcev wrote: > One other small thing. I see you dropped the dev->mtx bracket that > I added around the initialization and submission. Notice that the > dev->udev is zeroed under dev->mtx, not the static lock. This is because > it has to be seen in read and write paths. If you don't like taking > across the submission, how about testing for it ahead of time? I thought it can be managed under static lock. But if you sure we can leave device lock too, I'm not familiar with all this on such deep level... at least for now;). I'm not sure what kind of testing do you mean by "ahead of time". I just tried the latest patch and all seems to be good. BTW, slab corruption issue that I saw on the original driver we started fixing on is not an issue any more. Vitaliy - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb-status in adutux
On Tue, 2007-10-30 at 23:54, Pete Zaitcev wrote: One other small thing. I see you dropped the dev-mtx bracket that I added around the initialization and submission. Notice that the dev-udev is zeroed under dev-mtx, not the static lock. This is because it has to be seen in read and write paths. If you don't like taking across the submission, how about testing for it ahead of time? I thought it can be managed under static lock. But if you sure we can leave device lock too, I'm not familiar with all this on such deep level... at least for now;). I'm not sure what kind of testing do you mean by ahead of time. I just tried the latest patch and all seems to be good. BTW, slab corruption issue that I saw on the original driver we started fixing on is not an issue any more. Vitaliy - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb-status in adutux
On Wed, 31 Oct 2007 13:54:54 +0200, Vitaliy Ivanov [EMAIL PROTECTED] wrote:
On Tue, 2007-10-30 at 23:54, Pete Zaitcev wrote:
One other small thing. I see you dropped the dev-mtx bracket that
I added around the initialization and submission. Notice that the
dev-udev is zeroed under dev-mtx, not the static lock. This is because
it has to be seen in read and write paths. If you don't like taking
across the submission, how about testing for it ahead of time?
I thought it can be managed under static lock.
The paragraph you quoted above explains why dev-udev cannot be managed
under the static lock: because dev-udev is accessed by read/write methods,
which do not take the static lock.
I'm not sure what kind of testing do you mean by ahead of time.
No, I meant testing before the rest of the -open method is executed,
sorry. This part is ahead of the rest:
@@ -267,54 +290,54 @@ static int adu_open(struct inode *inode, struct file
*file)
}
dev = usb_get_intfdata(interface);
- if (!dev) {
+ if (!dev || !dev-udev) {
retval = -ENODEV;
goto exit_no_device;
}
Sorry about that. I'll try to be more explicit in the future.
I just tried the latest patch and all seems to be good.
BTW, slab corruption issue that I saw on the original driver we started
fixing on is not an issue any more.
Very well, I'll post this for Greg anew today.
Do you still want to go ahead with a 2.4 backport?
-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
USB: FIx locks and urb-status in adutux (updated)
Two main issues fixed here are:
- An improper use of in-struct lock to protect an open count
- Use of urb status for -EINPROGRESS
Also, along the way:
- Change usb_unlink_urb to usb_kill_urb. Apparently there's no need
to use usb_unlink_urb whatsoever in this driver, and the old use of
usb_kill_urb was outright racy (it unlinked and immediately freed).
- Fix indentation in adu_write. Looks like it was damaged by a script.
- Vitaly wants -EBUSY on multiply opens.
- bInterval was taken from a wrong endpoint.
Signed-off-by: Pete Zaitcev [EMAIL PROTECTED]
Signed-off-by: Vitaliy Ivanov [EMAIL PROTECTED]
Tested-by: Vitaliy Ivanov [EMAIL PROTECTED]
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..5a2c44e 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -79,12 +79,22 @@ MODULE_DEVICE_TABLE(usb, device_table);
#define COMMAND_TIMEOUT(2*HZ) /* 60 second timeout for a command */
+/*
+ * The locking scheme is a vanilla 3-lock:
+ * adu_device.buflock: A spinlock, covers what IRQs touch.
+ * adutux_mutex: A Static lock to cover open_count. It would also cover
+ * any globals, but we don't have them in 2.6.
+ * adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
+ * It covers all of adu_device, except the open_count
+ * and what .buflock covers.
+ */
+
/* Structure to hold all of our device specific stuff */
struct adu_device {
- struct mutexmtx; /* locks this structure */
+ struct mutexmtx;
struct usb_device* udev; /* save off the usb device pointer */
struct usb_interface* interface;
- unsigned char minor; /* the starting minor number for this
device */
+ unsigned intminor; /* the starting minor number for this
device */
charserial_number[8];
int open_count; /* number of times this port has
been opened */
@@ -107,8 +117,11 @@ struct adu_device {
char* interrupt_out_buffer;
struct usb_endpoint_descriptor* interrupt_out_endpoint;
struct urb* interrupt_out_urb;
+ int out_urb_finished;
};
+static DEFINE_MUTEX(adutux_mutex);
+
static struct usb_driver adu_driver;
static void adu_debug_data(int level, const char *function, int size,
@@ -132,27 +145,31 @@ static void adu_debug_data(int level, const char
*function, int size,
*/
static void adu_abort_transfers(struct adu_device *dev)
{
- dbg(2, %s : enter, __FUNCTION__);
+ unsigned long flags;
- if (dev == NULL) {
- dbg(1, %s : dev is null, __FUNCTION__);
- goto exit;
- }
+ dbg(2, %s : enter, __FUNCTION__);
if (dev-udev == NULL) {
dbg(1, %s : udev is null, __FUNCTION__);
goto exit;
}
- dbg(2, %s : udev state %d, __FUNCTION__, dev-udev-state);
- if (dev-udev-state == USB_STATE_NOTATTACHED) {
- dbg(1, %s : udev is not attached, __FUNCTION__);
- goto exit;
- }
-
/* shutdown transfer */
- usb_unlink_urb(dev-interrupt_in_urb);
- usb_unlink_urb(dev-interrupt_out_urb);
+
+ /* XXX Anchor these instead */
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-read_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_in_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
+
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-out_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_out_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
exit:
dbg(2, %s : leave, __FUNCTION__);
@@ -162,8 +179,6 @@ static void adu_delete(struct adu_device *dev)
{
dbg(2, %s enter, __FUNCTION__);
- adu_abort_transfers(dev);
-
/* free data structures */
usb_free_urb(dev-interrupt_in_urb);
usb_free_urb(dev-interrupt_out_urb);
@@ -239,7 +254,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
goto exit;
}
- wake_up_interruptible(dev-write_wait);
+ spin_lock(dev-buflock);
+ dev-out_urb_finished = 1;
+ wake_up(dev-write_wait);
+ spin_unlock(dev-buflock);
exit:
adu_debug_data(5, __FUNCTION__, urb-actual_length,
@@ -252,12 +270,17 @@ static int adu_open(struct inode *inode, struct file
*file)
struct adu_device *dev = NULL;
struct usb_interface *interface;
int subminor;
- int retval = 0;
+ int retval;
dbg(2,%s : enter, __FUNCTION__);
subminor = iminor(inode);
+ if ((retval = mutex_lock_interruptible(adutux_mutex)))
Re: USB: FIx locks and urb->status in adutux
On Tue, 30 Oct 2007 15:09:54 +0200, Vitaliy Ivanov <[EMAIL PROTECTED]> wrote:
> As about read_urb_finished probably it's OK. But we shouldn't decrease
> open_count in the case of error as then we return normal exit value.
Oh, thanks. I fixed that.
One other small thing. I see you dropped the dev->mtx bracket that
I added around the initialization and submission. Notice that the
dev->udev is zeroed under dev->mtx, not the static lock. This is because
it has to be seen in read and write paths. If you don't like taking
across the submission, how about testing for it ahead of time?
Here's what I've got now:
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..5a2c44e 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -79,12 +79,22 @@ MODULE_DEVICE_TABLE(usb, device_table);
#define COMMAND_TIMEOUT(2*HZ) /* 60 second timeout for a command */
+/*
+ * The locking scheme is a vanilla 3-lock:
+ * adu_device.buflock: A spinlock, covers what IRQs touch.
+ * adutux_mutex: A Static lock to cover open_count. It would also cover
+ * any globals, but we don't have them in 2.6.
+ * adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
+ * It covers all of adu_device, except the open_count
+ * and what .buflock covers.
+ */
+
/* Structure to hold all of our device specific stuff */
struct adu_device {
- struct mutexmtx; /* locks this structure */
+ struct mutexmtx;
struct usb_device* udev; /* save off the usb device pointer */
struct usb_interface* interface;
- unsigned char minor; /* the starting minor number for this
device */
+ unsigned intminor; /* the starting minor number for this
device */
charserial_number[8];
int open_count; /* number of times this port has
been opened */
@@ -107,8 +117,11 @@ struct adu_device {
char* interrupt_out_buffer;
struct usb_endpoint_descriptor* interrupt_out_endpoint;
struct urb* interrupt_out_urb;
+ int out_urb_finished;
};
+static DEFINE_MUTEX(adutux_mutex);
+
static struct usb_driver adu_driver;
static void adu_debug_data(int level, const char *function, int size,
@@ -132,27 +145,31 @@ static void adu_debug_data(int level, const char
*function, int size,
*/
static void adu_abort_transfers(struct adu_device *dev)
{
- dbg(2," %s : enter", __FUNCTION__);
+ unsigned long flags;
- if (dev == NULL) {
- dbg(1," %s : dev is null", __FUNCTION__);
- goto exit;
- }
+ dbg(2," %s : enter", __FUNCTION__);
if (dev->udev == NULL) {
dbg(1," %s : udev is null", __FUNCTION__);
goto exit;
}
- dbg(2," %s : udev state %d", __FUNCTION__, dev->udev->state);
- if (dev->udev->state == USB_STATE_NOTATTACHED) {
- dbg(1," %s : udev is not attached", __FUNCTION__);
- goto exit;
- }
-
/* shutdown transfer */
- usb_unlink_urb(dev->interrupt_in_urb);
- usb_unlink_urb(dev->interrupt_out_urb);
+
+ /* XXX Anchor these instead */
+ spin_lock_irqsave(>buflock, flags);
+ if (!dev->read_urb_finished) {
+ spin_unlock_irqrestore(>buflock, flags);
+ usb_kill_urb(dev->interrupt_in_urb);
+ } else
+ spin_unlock_irqrestore(>buflock, flags);
+
+ spin_lock_irqsave(>buflock, flags);
+ if (!dev->out_urb_finished) {
+ spin_unlock_irqrestore(>buflock, flags);
+ usb_kill_urb(dev->interrupt_out_urb);
+ } else
+ spin_unlock_irqrestore(>buflock, flags);
exit:
dbg(2," %s : leave", __FUNCTION__);
@@ -162,8 +179,6 @@ static void adu_delete(struct adu_device *dev)
{
dbg(2, "%s enter", __FUNCTION__);
- adu_abort_transfers(dev);
-
/* free data structures */
usb_free_urb(dev->interrupt_in_urb);
usb_free_urb(dev->interrupt_out_urb);
@@ -239,7 +254,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
goto exit;
}
- wake_up_interruptible(>write_wait);
+ spin_lock(>buflock);
+ dev->out_urb_finished = 1;
+ wake_up(>write_wait);
+ spin_unlock(>buflock);
exit:
adu_debug_data(5, __FUNCTION__, urb->actual_length,
@@ -252,12 +270,17 @@ static int adu_open(struct inode *inode, struct file
*file)
struct adu_device *dev = NULL;
struct usb_interface *interface;
int subminor;
- int retval = 0;
+ int retval;
dbg(2,"%s : enter", __FUNCTION__);
subminor = iminor(inode);
+ if ((retval = mutex_lock_interruptible(_mutex))) {
+ dbg(2, "%s : mutex lock failed",
Re: USB: FIx locks and urb->status in adutux
On Tue, 2007-10-30 at 06:24, Pete Zaitcev wrote:
> However, this looks wrong:
>
> > +dev->interrupt_in_endpoint->bInterval);
> > + dev->read_urb_finished = 0;
> > + retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
> > + /* we ignore failure */
> > + /* end of fixup for first read */
> > +
> > + /* initialize out direction */
> > + dev->out_urb_finished = 1;
>
> The finished flag is only set when URB is not in use anymore. Did you
> observe an anomaly with my code? Any hangs? If so, I assure you this
> is not the fix. As it's written, even if we ignore the failure (e.g.
> do not pass it to userland), we sill have to maintain the correct
> flag state.
As about read_urb_finished probably it's OK. But we shouldn't decrease
open_count in the case of error as then we return normal exit value.
Here is what we had before:
dev->interrupt_in_endpoint->bInterval);
dev->read_urb_finished = 0;
retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
if (retval) {
dev->read_urb_finished = 1;
--dev->open_count;
}
So I can left it but w/o this line:
--dev->open_count;
What is more critical is that I added:
/* initialize out direction */
dev->out_urb_finished = 1;
Without this we'll always have write timeouts.
Vitaliy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb-status in adutux
On Tue, 2007-10-30 at 06:24, Pete Zaitcev wrote:
However, this looks wrong:
+dev-interrupt_in_endpoint-bInterval);
+ dev-read_urb_finished = 0;
+ retval = usb_submit_urb(dev-interrupt_in_urb, GFP_KERNEL);
+ /* we ignore failure */
+ /* end of fixup for first read */
+
+ /* initialize out direction */
+ dev-out_urb_finished = 1;
The finished flag is only set when URB is not in use anymore. Did you
observe an anomaly with my code? Any hangs? If so, I assure you this
is not the fix. As it's written, even if we ignore the failure (e.g.
do not pass it to userland), we sill have to maintain the correct
flag state.
As about read_urb_finished probably it's OK. But we shouldn't decrease
open_count in the case of error as then we return normal exit value.
Here is what we had before:
dev-interrupt_in_endpoint-bInterval);
dev-read_urb_finished = 0;
retval = usb_submit_urb(dev-interrupt_in_urb, GFP_KERNEL);
if (retval) {
dev-read_urb_finished = 1;
--dev-open_count;
}
So I can left it but w/o this line:
--dev-open_count;
What is more critical is that I added:
/* initialize out direction */
dev-out_urb_finished = 1;
Without this we'll always have write timeouts.
Vitaliy
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb-status in adutux
On Tue, 30 Oct 2007 15:09:54 +0200, Vitaliy Ivanov [EMAIL PROTECTED] wrote:
As about read_urb_finished probably it's OK. But we shouldn't decrease
open_count in the case of error as then we return normal exit value.
Oh, thanks. I fixed that.
One other small thing. I see you dropped the dev-mtx bracket that
I added around the initialization and submission. Notice that the
dev-udev is zeroed under dev-mtx, not the static lock. This is because
it has to be seen in read and write paths. If you don't like taking
across the submission, how about testing for it ahead of time?
Here's what I've got now:
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..5a2c44e 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -79,12 +79,22 @@ MODULE_DEVICE_TABLE(usb, device_table);
#define COMMAND_TIMEOUT(2*HZ) /* 60 second timeout for a command */
+/*
+ * The locking scheme is a vanilla 3-lock:
+ * adu_device.buflock: A spinlock, covers what IRQs touch.
+ * adutux_mutex: A Static lock to cover open_count. It would also cover
+ * any globals, but we don't have them in 2.6.
+ * adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
+ * It covers all of adu_device, except the open_count
+ * and what .buflock covers.
+ */
+
/* Structure to hold all of our device specific stuff */
struct adu_device {
- struct mutexmtx; /* locks this structure */
+ struct mutexmtx;
struct usb_device* udev; /* save off the usb device pointer */
struct usb_interface* interface;
- unsigned char minor; /* the starting minor number for this
device */
+ unsigned intminor; /* the starting minor number for this
device */
charserial_number[8];
int open_count; /* number of times this port has
been opened */
@@ -107,8 +117,11 @@ struct adu_device {
char* interrupt_out_buffer;
struct usb_endpoint_descriptor* interrupt_out_endpoint;
struct urb* interrupt_out_urb;
+ int out_urb_finished;
};
+static DEFINE_MUTEX(adutux_mutex);
+
static struct usb_driver adu_driver;
static void adu_debug_data(int level, const char *function, int size,
@@ -132,27 +145,31 @@ static void adu_debug_data(int level, const char
*function, int size,
*/
static void adu_abort_transfers(struct adu_device *dev)
{
- dbg(2, %s : enter, __FUNCTION__);
+ unsigned long flags;
- if (dev == NULL) {
- dbg(1, %s : dev is null, __FUNCTION__);
- goto exit;
- }
+ dbg(2, %s : enter, __FUNCTION__);
if (dev-udev == NULL) {
dbg(1, %s : udev is null, __FUNCTION__);
goto exit;
}
- dbg(2, %s : udev state %d, __FUNCTION__, dev-udev-state);
- if (dev-udev-state == USB_STATE_NOTATTACHED) {
- dbg(1, %s : udev is not attached, __FUNCTION__);
- goto exit;
- }
-
/* shutdown transfer */
- usb_unlink_urb(dev-interrupt_in_urb);
- usb_unlink_urb(dev-interrupt_out_urb);
+
+ /* XXX Anchor these instead */
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-read_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_in_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
+
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-out_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_out_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
exit:
dbg(2, %s : leave, __FUNCTION__);
@@ -162,8 +179,6 @@ static void adu_delete(struct adu_device *dev)
{
dbg(2, %s enter, __FUNCTION__);
- adu_abort_transfers(dev);
-
/* free data structures */
usb_free_urb(dev-interrupt_in_urb);
usb_free_urb(dev-interrupt_out_urb);
@@ -239,7 +254,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
goto exit;
}
- wake_up_interruptible(dev-write_wait);
+ spin_lock(dev-buflock);
+ dev-out_urb_finished = 1;
+ wake_up(dev-write_wait);
+ spin_unlock(dev-buflock);
exit:
adu_debug_data(5, __FUNCTION__, urb-actual_length,
@@ -252,12 +270,17 @@ static int adu_open(struct inode *inode, struct file
*file)
struct adu_device *dev = NULL;
struct usb_interface *interface;
int subminor;
- int retval = 0;
+ int retval;
dbg(2,%s : enter, __FUNCTION__);
subminor = iminor(inode);
+ if ((retval = mutex_lock_interruptible(adutux_mutex))) {
+ dbg(2, %s : mutex lock failed,
Re: USB: FIx locks and urb->status in adutux
On Mon, 29 Oct 2007 20:04:57 +0200, Vitaliy Ivanov <[EMAIL PROTECTED]> wrote: > Finally had a time on my weekend to perform tests and fix Pete's patch a > little. Now it seems to be correct. Great! > 1. One device per user space application. Old driver allowed many users > application to work with same device which can lead to IO mess. OK. This trick was popular in UNIX. Personally I think it's in a bad taste, because good applications still need to verify if only one instance is running, and threfore can use application level locking. But if you are gunning for the maintenership I'm not going to argue your style. The busy lock-out certainly works better than "/dev/cua" :-) However, this looks wrong: > + dev->interrupt_in_endpoint->bInterval); > + dev->read_urb_finished = 0; > + retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL); > + /* we ignore failure */ > + /* end of fixup for first read */ > + > + /* initialize out direction */ > + dev->out_urb_finished = 1; The finished flag is only set when URB is not in use anymore. Did you observe an anomaly with my code? Any hangs? If so, I assure you this is not the fix. As it's written, even if we ignore the failure (e.g. do not pass it to userland), we sill have to maintain the correct flag state. -- Pete - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb->status in adutux
Finally had a time on my weekend to perform tests and fix Pete's patch a
little. Now it seems to be correct.
Added a few changes:
1. One device per user space application. Old driver allowed many users
application to work with same device which can lead to IO mess.
2. GFP_KERNEL is used instead of GFP_ATOMIC as we are now out of spin locks.
3. Added myself to maintainers(already discussed this in 2.4). Does anyone mind?
(Diff between this latest patch and the one from Pete is in attach).
Also part of Pete's notes:
Two main issues fixed here are:
- An improper use of in-struct lock to protect an open count
- Use of urb status for -EINPROGRESS
Also, along the way:
- Change usb_unlink_urb to usb_kill_urb. Apparently there's no need
to use usb_unlink_urb whatsoever in this driver, and the old use of
usb_kill_urb was outright racy (it unlinked and immediately freed).
- Fix indentation in adu_write. Looks like it was damaged by a script.
- Remove sleep_on.
Here is the final patch. Comments are welcomed.
--
Signed-off-by: Pete Zaitcev <[EMAIL PROTECTED]>
Signed-off-by: Vitaliy Ivanov <[EMAIL PROTECTED]>
Tested-by: Vitaliy Ivanov <[EMAIL PROTECTED]>
--
diff -uprN -X linux-2.6.24-rc1.org/Documentation/dontdiff
linux-2.6.24-rc1.org/drivers/usb/misc/adutux.c
linux-2.6.24-rc1.my/drivers/usb/misc/adutux.c
--- linux-2.6.24-rc1.org/drivers/usb/misc/adutux.c 2007-10-29
19:25:00.0 +0200
+++ linux-2.6.24-rc1.my/drivers/usb/misc/adutux.c 2007-10-29
19:01:10.0 +0200
@@ -79,12 +79,22 @@ MODULE_DEVICE_TABLE(usb, device_table);
#define COMMAND_TIMEOUT(2*HZ) /* 60 second timeout for a command */
+/*
+ * The locking scheme is a vanilla 3-lock:
+ * adu_device.buflock: A spinlock, covers what IRQs touch.
+ * adutux_mutex: A Static lock to cover open_count. It would also cover
+ * any globals, but we don't have them in 2.6.
+ * adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
+ * It covers all of adu_device, except the open_count
+ * and what .buflock covers.
+ */
+
/* Structure to hold all of our device specific stuff */
struct adu_device {
- struct mutexmtx; /* locks this structure */
+ struct mutexmtx;
struct usb_device* udev; /* save off the usb device pointer */
struct usb_interface* interface;
- unsigned char minor; /* the starting minor number for this
device */
+ unsigned intminor; /* the starting minor number for this
device */
charserial_number[8];
int open_count; /* number of times this port has
been opened */
@@ -107,8 +117,11 @@ struct adu_device {
char* interrupt_out_buffer;
struct usb_endpoint_descriptor* interrupt_out_endpoint;
struct urb* interrupt_out_urb;
+ int out_urb_finished;
};
+static DEFINE_MUTEX(adutux_mutex);
+
static struct usb_driver adu_driver;
static void adu_debug_data(int level, const char *function, int size,
@@ -132,27 +145,31 @@ static void adu_debug_data(int level, co
*/
static void adu_abort_transfers(struct adu_device *dev)
{
- dbg(2," %s : enter", __FUNCTION__);
+ unsigned long flags;
- if (dev == NULL) {
- dbg(1," %s : dev is null", __FUNCTION__);
- goto exit;
- }
+ dbg(2," %s : enter", __FUNCTION__);
if (dev->udev == NULL) {
dbg(1," %s : udev is null", __FUNCTION__);
goto exit;
}
- dbg(2," %s : udev state %d", __FUNCTION__, dev->udev->state);
- if (dev->udev->state == USB_STATE_NOTATTACHED) {
- dbg(1," %s : udev is not attached", __FUNCTION__);
- goto exit;
- }
-
/* shutdown transfer */
- usb_unlink_urb(dev->interrupt_in_urb);
- usb_unlink_urb(dev->interrupt_out_urb);
+
+ /* XXX Anchor these instead */
+ spin_lock_irqsave(>buflock, flags);
+ if (!dev->read_urb_finished) {
+ spin_unlock_irqrestore(>buflock, flags);
+ usb_kill_urb(dev->interrupt_in_urb);
+ } else
+ spin_unlock_irqrestore(>buflock, flags);
+
+ spin_lock_irqsave(>buflock, flags);
+ if (!dev->out_urb_finished) {
+ spin_unlock_irqrestore(>buflock, flags);
+ usb_kill_urb(dev->interrupt_out_urb);
+ } else
+ spin_unlock_irqrestore(>buflock, flags);
exit:
dbg(2," %s : leave", __FUNCTION__);
@@ -162,8 +179,6 @@ static void adu_delete(struct adu_device
{
dbg(2, "%s enter", __FUNCTION__);
- adu_abort_transfers(dev);
-
/* free data structures */
usb_free_urb(dev->interrupt_in_urb);
usb_free_urb(dev->interrupt_out_urb);
@@ -239,7 +254,10 @@ static void
Re: USB: FIx locks and urb-status in adutux
Finally had a time on my weekend to perform tests and fix Pete's patch a
little. Now it seems to be correct.
Added a few changes:
1. One device per user space application. Old driver allowed many users
application to work with same device which can lead to IO mess.
2. GFP_KERNEL is used instead of GFP_ATOMIC as we are now out of spin locks.
3. Added myself to maintainers(already discussed this in 2.4). Does anyone mind?
(Diff between this latest patch and the one from Pete is in attach).
Also part of Pete's notes:
Two main issues fixed here are:
- An improper use of in-struct lock to protect an open count
- Use of urb status for -EINPROGRESS
Also, along the way:
- Change usb_unlink_urb to usb_kill_urb. Apparently there's no need
to use usb_unlink_urb whatsoever in this driver, and the old use of
usb_kill_urb was outright racy (it unlinked and immediately freed).
- Fix indentation in adu_write. Looks like it was damaged by a script.
- Remove sleep_on.
Here is the final patch. Comments are welcomed.
--
Signed-off-by: Pete Zaitcev [EMAIL PROTECTED]
Signed-off-by: Vitaliy Ivanov [EMAIL PROTECTED]
Tested-by: Vitaliy Ivanov [EMAIL PROTECTED]
--
diff -uprN -X linux-2.6.24-rc1.org/Documentation/dontdiff
linux-2.6.24-rc1.org/drivers/usb/misc/adutux.c
linux-2.6.24-rc1.my/drivers/usb/misc/adutux.c
--- linux-2.6.24-rc1.org/drivers/usb/misc/adutux.c 2007-10-29
19:25:00.0 +0200
+++ linux-2.6.24-rc1.my/drivers/usb/misc/adutux.c 2007-10-29
19:01:10.0 +0200
@@ -79,12 +79,22 @@ MODULE_DEVICE_TABLE(usb, device_table);
#define COMMAND_TIMEOUT(2*HZ) /* 60 second timeout for a command */
+/*
+ * The locking scheme is a vanilla 3-lock:
+ * adu_device.buflock: A spinlock, covers what IRQs touch.
+ * adutux_mutex: A Static lock to cover open_count. It would also cover
+ * any globals, but we don't have them in 2.6.
+ * adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
+ * It covers all of adu_device, except the open_count
+ * and what .buflock covers.
+ */
+
/* Structure to hold all of our device specific stuff */
struct adu_device {
- struct mutexmtx; /* locks this structure */
+ struct mutexmtx;
struct usb_device* udev; /* save off the usb device pointer */
struct usb_interface* interface;
- unsigned char minor; /* the starting minor number for this
device */
+ unsigned intminor; /* the starting minor number for this
device */
charserial_number[8];
int open_count; /* number of times this port has
been opened */
@@ -107,8 +117,11 @@ struct adu_device {
char* interrupt_out_buffer;
struct usb_endpoint_descriptor* interrupt_out_endpoint;
struct urb* interrupt_out_urb;
+ int out_urb_finished;
};
+static DEFINE_MUTEX(adutux_mutex);
+
static struct usb_driver adu_driver;
static void adu_debug_data(int level, const char *function, int size,
@@ -132,27 +145,31 @@ static void adu_debug_data(int level, co
*/
static void adu_abort_transfers(struct adu_device *dev)
{
- dbg(2, %s : enter, __FUNCTION__);
+ unsigned long flags;
- if (dev == NULL) {
- dbg(1, %s : dev is null, __FUNCTION__);
- goto exit;
- }
+ dbg(2, %s : enter, __FUNCTION__);
if (dev-udev == NULL) {
dbg(1, %s : udev is null, __FUNCTION__);
goto exit;
}
- dbg(2, %s : udev state %d, __FUNCTION__, dev-udev-state);
- if (dev-udev-state == USB_STATE_NOTATTACHED) {
- dbg(1, %s : udev is not attached, __FUNCTION__);
- goto exit;
- }
-
/* shutdown transfer */
- usb_unlink_urb(dev-interrupt_in_urb);
- usb_unlink_urb(dev-interrupt_out_urb);
+
+ /* XXX Anchor these instead */
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-read_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_in_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
+
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-out_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_out_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
exit:
dbg(2, %s : leave, __FUNCTION__);
@@ -162,8 +179,6 @@ static void adu_delete(struct adu_device
{
dbg(2, %s enter, __FUNCTION__);
- adu_abort_transfers(dev);
-
/* free data structures */
usb_free_urb(dev-interrupt_in_urb);
usb_free_urb(dev-interrupt_out_urb);
@@ -239,7 +254,10 @@ static void adu_interrupt_out_callback(s
Re: USB: FIx locks and urb-status in adutux
On Mon, 29 Oct 2007 20:04:57 +0200, Vitaliy Ivanov [EMAIL PROTECTED] wrote: Finally had a time on my weekend to perform tests and fix Pete's patch a little. Now it seems to be correct. Great! 1. One device per user space application. Old driver allowed many users application to work with same device which can lead to IO mess. OK. This trick was popular in UNIX. Personally I think it's in a bad taste, because good applications still need to verify if only one instance is running, and threfore can use application level locking. But if you are gunning for the maintenership I'm not going to argue your style. The busy lock-out certainly works better than /dev/cua :-) However, this looks wrong: + dev-interrupt_in_endpoint-bInterval); + dev-read_urb_finished = 0; + retval = usb_submit_urb(dev-interrupt_in_urb, GFP_KERNEL); + /* we ignore failure */ + /* end of fixup for first read */ + + /* initialize out direction */ + dev-out_urb_finished = 1; The finished flag is only set when URB is not in use anymore. Did you observe an anomaly with my code? Any hangs? If so, I assure you this is not the fix. As it's written, even if we ignore the failure (e.g. do not pass it to userland), we sill have to maintain the correct flag state. -- Pete - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb->status in adutux
Greg, On 10/25/07, Greg KH <[EMAIL PROTECTED]> wrote: > Hm, I think I'll wait for Pete and you to agree and send me a final > version before applying anything here :) I'm testing the final patch Pete created with all the notes. Will let you know if there are issues with it. If not it can be applied to your tree. Thanks, Vitaliy - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb-status in adutux
Greg, On 10/25/07, Greg KH [EMAIL PROTECTED] wrote: Hm, I think I'll wait for Pete and you to agree and send me a final version before applying anything here :) I'm testing the final patch Pete created with all the notes. Will let you know if there are issues with it. If not it can be applied to your tree. Thanks, Vitaliy - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb->status in adutux
On Thu, 25 Oct 2007 14:03:48 +0200, Oliver Neukum <[EMAIL PROTECTED]> wrote:
> Am Donnerstag 25 Oktober 2007 schrieb Pete Zaitcev:
> > + if (signal_pending(current)) {
> > dbg(1," %s : interrupted", __FUNCTION__);
> > + set_current_state(TASK_RUNNING);
> > retval = -EINTR;
> > - goto exit;
> > + goto exit_onqueue;
>
> Are you sure you cannot have written any data here?
I noticed that myself, but this is what the old driver did.
-- Pete
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb-status in adutux
On Thu, 25 Oct 2007 14:03:48 +0200, Oliver Neukum [EMAIL PROTECTED] wrote:
Am Donnerstag 25 Oktober 2007 schrieb Pete Zaitcev:
+ if (signal_pending(current)) {
dbg(1, %s : interrupted, __FUNCTION__);
+ set_current_state(TASK_RUNNING);
retval = -EINTR;
- goto exit;
+ goto exit_onqueue;
Are you sure you cannot have written any data here?
I noticed that myself, but this is what the old driver did.
-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb->status in adutux
BTW, the patch in my previous message had one buglet - an extra
remove_wait_queue. It was in an error case though, so it should be
ok to test. But just in case, here's a fixed one.
-- Pete
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..6914c0b 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -79,12 +79,22 @@ MODULE_DEVICE_TABLE(usb, device_table);
#define COMMAND_TIMEOUT(2*HZ) /* 60 second timeout for a command */
+/*
+ * The locking scheme is a vanilla 3-lock:
+ * adu_device.buflock: A spinlock, covers what IRQs touch.
+ * adutux_mutex: A Static lock to cover open_count. It would also cover
+ * any globals, but we don't have them in 2.6.
+ * adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
+ * It covers all of adu_device, except the open_count
+ * and what .buflock covers.
+ */
+
/* Structure to hold all of our device specific stuff */
struct adu_device {
- struct mutexmtx; /* locks this structure */
+ struct mutexmtx;
struct usb_device* udev; /* save off the usb device pointer */
struct usb_interface* interface;
- unsigned char minor; /* the starting minor number for this
device */
+ unsigned intminor; /* the starting minor number for this
device */
charserial_number[8];
int open_count; /* number of times this port has
been opened */
@@ -107,8 +117,11 @@ struct adu_device {
char* interrupt_out_buffer;
struct usb_endpoint_descriptor* interrupt_out_endpoint;
struct urb* interrupt_out_urb;
+ int out_urb_finished;
};
+static DEFINE_MUTEX(adutux_mutex);
+
static struct usb_driver adu_driver;
static void adu_debug_data(int level, const char *function, int size,
@@ -132,27 +145,31 @@ static void adu_debug_data(int level, const char
*function, int size,
*/
static void adu_abort_transfers(struct adu_device *dev)
{
- dbg(2," %s : enter", __FUNCTION__);
+ unsigned long flags;
- if (dev == NULL) {
- dbg(1," %s : dev is null", __FUNCTION__);
- goto exit;
- }
+ dbg(2," %s : enter", __FUNCTION__);
if (dev->udev == NULL) {
dbg(1," %s : udev is null", __FUNCTION__);
goto exit;
}
- dbg(2," %s : udev state %d", __FUNCTION__, dev->udev->state);
- if (dev->udev->state == USB_STATE_NOTATTACHED) {
- dbg(1," %s : udev is not attached", __FUNCTION__);
- goto exit;
- }
-
/* shutdown transfer */
- usb_unlink_urb(dev->interrupt_in_urb);
- usb_unlink_urb(dev->interrupt_out_urb);
+
+ /* XXX Anchor these instead */
+ spin_lock_irqsave(>buflock, flags);
+ if (!dev->read_urb_finished) {
+ spin_unlock_irqrestore(>buflock, flags);
+ usb_kill_urb(dev->interrupt_in_urb);
+ } else
+ spin_unlock_irqrestore(>buflock, flags);
+
+ spin_lock_irqsave(>buflock, flags);
+ if (!dev->out_urb_finished) {
+ spin_unlock_irqrestore(>buflock, flags);
+ usb_kill_urb(dev->interrupt_out_urb);
+ } else
+ spin_unlock_irqrestore(>buflock, flags);
exit:
dbg(2," %s : leave", __FUNCTION__);
@@ -162,8 +179,6 @@ static void adu_delete(struct adu_device *dev)
{
dbg(2, "%s enter", __FUNCTION__);
- adu_abort_transfers(dev);
-
/* free data structures */
usb_free_urb(dev->interrupt_in_urb);
usb_free_urb(dev->interrupt_out_urb);
@@ -239,7 +254,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
goto exit;
}
- wake_up_interruptible(>write_wait);
+ spin_lock(>buflock);
+ dev->out_urb_finished = 1;
+ wake_up(>write_wait);
+ spin_unlock(>buflock);
exit:
adu_debug_data(5, __FUNCTION__, urb->actual_length,
@@ -252,12 +270,17 @@ static int adu_open(struct inode *inode, struct file
*file)
struct adu_device *dev = NULL;
struct usb_interface *interface;
int subminor;
- int retval = 0;
+ int retval;
dbg(2,"%s : enter", __FUNCTION__);
subminor = iminor(inode);
+ if ((retval = mutex_lock_interruptible(_mutex))) {
+ dbg(2, "%s : mutex lock failed", __FUNCTION__);
+ goto exit_no_lock;
+ }
+
interface = usb_find_interface(_driver, subminor);
if (!interface) {
err("%s - error, can't find device for minor %d",
@@ -272,13 +295,6 @@ static int adu_open(struct inode *inode, struct file *file)
goto exit_no_device;
}
- /* lock this device */
- if ((retval =
Re: USB: FIx locks and urb->status in adutux
On Wed, 24 Oct 2007 17:09:47 +0300, Vitaliy Ivanov <[EMAIL PROTECTED]> wrote:
> > static void adu_abort_transfers(struct adu_device *dev)
> > {
> ...
> > + mutex_lock(>mtx);
> ...
> > + mutex_unlock(>mtx);
>
> > }
>
> Don't you think it's needed? You call adu_abort_transfers from adu_release
> only where it's locked by adutux_mutex.
Yes, it's not needed anymore. It's a carry-over from the time
when the old code aborted ransfers from the outside of adutux_mutex.
> /* send off the urb */
> usb_fill_int_urb(
> dev->interrupt_out_urb,
> dev->interrupt_in_endpoint->bInterval);
> Typo? Seems like that.
Looks like one. It actually is a risky thing to fix... What if the device
as broken descriptors and the driver worked only thanks to the bug above?
But let's fix it and see.
> I just tried your patch on the adutux from 2.6.24-rc1 but on my 2.6.20.7. It
> failed with the following message:
> [176200.425776] list_add corruption. next->prev should be prev (c15e6c30),
> but was . (next=c58bdf5c).
> [176200.426536] kernel BUG at lib/list_debug.c:28!
> [176200.427843] [] list_add+0xa/0xf
> [176200.427848] [] add_wait_queue+0x1f/0x2d
This is probably what Oliver caught, I did not have all removals
from wait queue where necessary.
> [176200.428593] <3>BUG: sleeping function called from invalid context at
> kernel/rwsem.c:20
This looks like a debugging-caused oops. It should go away by itself
once the above is fixed.
> I'm not sure whether it's because I'm trying it on 2.6.20.7 and not on
> 2.6.24-rc1.
> Will check it on 2.6.24-rc1 asap and let you know.
Please try attached on any of those.
Thank you,
-- Pete
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..bea6262 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -79,12 +79,22 @@ MODULE_DEVICE_TABLE(usb, device_table);
#define COMMAND_TIMEOUT(2*HZ) /* 60 second timeout for a command */
+/*
+ * The locking scheme is a vanilla 3-lock:
+ * adu_device.buflock: A spinlock, covers what IRQs touch.
+ * adutux_mutex: A Static lock to cover open_count. It would also cover
+ * any globals, but we don't have them in 2.6.
+ * adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
+ * It covers all of adu_device, except the open_count
+ * and what .buflock covers.
+ */
+
/* Structure to hold all of our device specific stuff */
struct adu_device {
- struct mutexmtx; /* locks this structure */
+ struct mutexmtx;
struct usb_device* udev; /* save off the usb device pointer */
struct usb_interface* interface;
- unsigned char minor; /* the starting minor number for this
device */
+ unsigned intminor; /* the starting minor number for this
device */
charserial_number[8];
int open_count; /* number of times this port has
been opened */
@@ -107,8 +117,11 @@ struct adu_device {
char* interrupt_out_buffer;
struct usb_endpoint_descriptor* interrupt_out_endpoint;
struct urb* interrupt_out_urb;
+ int out_urb_finished;
};
+static DEFINE_MUTEX(adutux_mutex);
+
static struct usb_driver adu_driver;
static void adu_debug_data(int level, const char *function, int size,
@@ -132,27 +145,31 @@ static void adu_debug_data(int level, const char
*function, int size,
*/
static void adu_abort_transfers(struct adu_device *dev)
{
- dbg(2," %s : enter", __FUNCTION__);
+ unsigned long flags;
- if (dev == NULL) {
- dbg(1," %s : dev is null", __FUNCTION__);
- goto exit;
- }
+ dbg(2," %s : enter", __FUNCTION__);
if (dev->udev == NULL) {
dbg(1," %s : udev is null", __FUNCTION__);
goto exit;
}
- dbg(2," %s : udev state %d", __FUNCTION__, dev->udev->state);
- if (dev->udev->state == USB_STATE_NOTATTACHED) {
- dbg(1," %s : udev is not attached", __FUNCTION__);
- goto exit;
- }
-
/* shutdown transfer */
- usb_unlink_urb(dev->interrupt_in_urb);
- usb_unlink_urb(dev->interrupt_out_urb);
+
+ /* XXX Anchor these instead */
+ spin_lock_irqsave(>buflock, flags);
+ if (!dev->read_urb_finished) {
+ spin_unlock_irqrestore(>buflock, flags);
+ usb_kill_urb(dev->interrupt_in_urb);
+ } else
+ spin_unlock_irqrestore(>buflock, flags);
+
+ spin_lock_irqsave(>buflock, flags);
+ if (!dev->out_urb_finished) {
+ spin_unlock_irqrestore(>buflock, flags);
+ usb_kill_urb(dev->interrupt_out_urb);
+ } else
+ spin_unlock_irqrestore(>buflock, flags);
exit:
Re: USB: FIx locks and urb->status in adutux
On Wed, Oct 24, 2007 at 04:49:12PM +0200, Oliver Neukum wrote: > Am Mittwoch 24 Oktober 2007 schrieb Pete Zaitcev: > > Oliver, thanks for the inftdata catch. I also fixed the sleep_on. > > But you are leaving a function while still on a waitqueue left on the stack. > Here's a patch on top of yours. Hm, I think I'll wait for Pete and you to agree and send me a final version before applying anything here :) thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb->status in adutux
Am Mittwoch 24 Oktober 2007 schrieb Pete Zaitcev:
> Oliver, thanks for the inftdata catch. I also fixed the sleep_on.
But you are leaving a function while still on a waitqueue left on the stack.
Here's a patch on top of yours.
Regards
Oliver
--- work/drivers/usb/misc/adutux.c.alt 2007-10-24 16:36:02.0 +0200
+++ work/drivers/usb/misc/adutux.c 2007-10-24 16:36:06.0 +0200
@@ -567,19 +567,21 @@ static ssize_t adu_write(struct file *fi
retval = mutex_lock_interruptible(>mtx);
if (retval)
- goto exit_nolock;
+ goto exit_nolock_intr;
/* verify that the device wasn't unplugged */
if (dev->udev == NULL) {
+ mutex_unlock(>mtx);
retval = -ENODEV;
err("No device or device unplugged %d", retval);
- goto exit;
+ goto exit_nolock_intr;
}
/* verify that we actually have some data to write */
if (count == 0) {
+ mutex_unlock(>mtx);
dbg(1," %s : write request of 0 bytes", __FUNCTION__);
- goto exit;
+ goto exit_nolock_intr;
}
add_wait_queue(>write_wait, );
@@ -649,13 +651,14 @@ static ssize_t adu_write(struct file *fi
bytes_written += bytes_to_write;
}
}
- remove_wait_queue(>write_wait, );
retval = bytes_written;
exit:
mutex_unlock(>mtx);
exit_nolock:
+ remove_wait_queue(>write_wait, );
+exit_nolock_intr:
dbg(2," %s : leave, return value %d", __FUNCTION__, retval);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb->status in adutux
Pete,
On Wed, 2007-10-24 at 04:53, Pete Zaitcev wrote:
1)
> +/*
> + * The locking scheme is a vanilla 3-lock:
> + * adu_device.buflock: A spinlock, covers what IRQs touch.
> + * adutux_mutex: A Static lock to cover open_count. It would also
> cover
> + * any globals, but we don't have them in 2.6.
> + * adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
> + * It covers all of adu_device, except the open_count
> + * and what .buflock covers.
> + */
> static void adu_abort_transfers(struct adu_device *dev)
> {
...
> + mutex_lock(>mtx);
...
> + mutex_unlock(>mtx);
> }
Don't you think it's needed? You call adu_abort_transfers from adu_release only
where it's locked by adutux_mutex.
2)
Also one issue. adu_write has:
/* send off the urb */
usb_fill_int_urb(
dev->interrupt_out_urb,
dev->udev,
usb_sndintpipe(dev->udev,
dev->interrupt_out_endpoint->bEndpointAddress),
dev->interrupt_out_buffer,
bytes_to_write,
adu_interrupt_out_callback,
dev,
dev->interrupt_in_endpoint->bInterval);
Seems like there should be not:
dev->interrupt_in_endpoint->bInterval
but:
dev->interrupt_out_endpoint->bInterval
Typo? Seems like that.
I just tried your patch on the adutux from 2.6.24-rc1 but on my 2.6.20.7. It
failed with the following message:
-
[176188.466898] drivers/usb/misc/adutux.c : adu_open : enter
[176188.468000] drivers/usb/misc/adutux.c : adu_open : open count 1
[176188.468601] drivers/usb/misc/adutux.c : adu_open : leave, return value 0
[176188.486218] drivers/usb/misc/adutux.c : adu_interrupt_in_callback : enter,
status 0
[176188.486269] drivers/usb/misc/adutux.c: adu_interrupt_in_callback - length =
8, data = 00 00 00 00 00 00 00 00
[176188.486430] drivers/usb/misc/adutux.c: adu_interrupt_in_callback - length =
8, data = 00 00 00 00 00 00 00 00
[176188.486482] drivers/usb/misc/adutux.c : adu_interrupt_in_callback : leave,
status 0
[176191.303853] drivers/usb/misc/adutux.c : adu_write : enter, count = 8
[176193.301872] drivers/usb/misc/adutux.c : adu_write - command timed out.
[176193.301912] drivers/usb/misc/adutux.c : adu_write : leave, return value
-110
[176200.425157] drivers/usb/misc/adutux.c : adu_write : enter, count = 8
[176200.425776] list_add corruption. next->prev should be prev (c15e6c30), but
was . (next=c58bdf5c).
[176200.425776] list_add corruption. next->prev should be prev (c15e6c30), but
was . (next=c58bdf5c).
[176200.426475] [ cut here ]
[176200.426475] [ cut here ]
[176200.426536] kernel BUG at lib/list_debug.c:27!
[176200.426536] kernel BUG at lib/list_debug.c:28!
[176200.426647] invalid opcode: [#1]
[176200.426647] invalid opcode: [#1]
[176200.426736] Modules linked in: adutux(F) ppdev autofs4 hidp rfcomm l2cap
bluetooth sunrpc xt_tcpudp iptable_filter ip_tables x_tables dm_multipath video
button battery asus_acpi backlight ac ipv6 lp parport_pc parport floppy nvram
uhci_hcd sg snd_ens1371 gameport snd_rawmidi snd_ac97_codec ac97_bus
snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_pcm_oss
snd_mixer_oss snd_pcm pcnet32 snd_timer snd pcspkr mii soundcore i2c_piix4
i2c_core snd_page_alloc dm_snapshot dm_zero dm_mirror dm_mod ext3 jbd mptspi
scsi_transport_spi mptscsih sd_mod scsi_mod mptbase
[176200.426736] Modules linked in: adutux(F) ppdev autofs4 hidp rfcomm l2cap
bluetooth sunrpc xt_tcpudp iptable_filter ip_tables x_tables dm_multipath video
button battery asus_acpi backlight ac ipv6 lp parport_pc parport floppy nvram
uhci_hcd sg snd_ens1371 gameport snd_rawmidi snd_ac97_codec ac97_bus
snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_pcm_oss
snd_mixer_oss snd_pcm pcnet32 snd_timer snd pcspkr mii soundcore i2c_piix4
i2c_core snd_page_alloc dm_snapshot dm_zero dm_mirror dm_mod ext3 jbd mptspi
scsi_transport_spi mptscsih sd_mod scsi_mod mptbase
[176200.427025] CPU:0
[176200.427025] CPU:0
[176200.427027] EIP:0060:[]Tainted: GF VLI
[176200.427027] EIP:0060:[]Tainted: GF VLI
[176200.427029] EFLAGS: 00010082 (2.6.20.7 #1)
[176200.427029] EFLAGS: 00010082 (2.6.20.7 #1)
[176200.427405] EIP is at __list_add+0x29/0x60
[176200.427405] EIP is at __list_add+0x29/0x60
[176200.427448] eax: 0071 ebx: c58bdf5c ecx: c0117787 edx: d10e0ab0
[176200.427448] eax: 0071 ebx: c58bdf5c ecx: c0117787 edx: d10e0ab0
[176200.427481] esi: c15e6c08 edi: 0296 ebp: c58bded8 esp: c58bdec0
[176200.427481] esi: c15e6c08 edi: 0296 ebp: c58bded8 esp: c58bdec0
[176200.427503] ds: 007b es: 007b ss: 0068
[176200.427503] ds: 007b es: 007b ss: 0068
[176200.427532] Process aducmd (pid: 14413, ti=c58bd000 task=d10e0ab0
Re: USB: FIx locks and urb->status in adutux
Am Dienstag 23 Oktober 2007 schrieb Pete Zaitcev:
> On Tue, 23 Oct 2007 11:38:37 +0200, Oliver Neukum <[EMAIL PROTECTED]> wrote:
>
> > > + /* XXX Anchor these instead */
> > > + spin_lock_irqsave(>buflock, flags);
> > > + if (!dev->read_urb_finished) {
> > > + spin_unlock_irqrestore(>buflock, flags);
> > > + usb_kill_urb(dev->interrupt_in_urb);
> > > + } else
> > > + spin_unlock_irqrestore(>buflock, flags);
>
> > Why bother? Simply call usb_kill_urb() unconditionally.
>
> Is it always safe to kill unfilled URBs? The filled but unsubmitted ones
> are ok, but in this case it's possible that we only allocated something
> but never submitted. Our current implementation happens to be safe by
> virtue of ->dev being NULL in such case. I do not remember if we always
> guaranteed that and since Vitaly is going to take this code for a
> backport, I decided to play it safe.
I am not sure as far as 2.4 is concerned. In fact I am not sure 2.4 has
usb_kill_urb() at all.
Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb-status in adutux
On Wed, 24 Oct 2007 17:09:47 +0300, Vitaliy Ivanov [EMAIL PROTECTED] wrote:
static void adu_abort_transfers(struct adu_device *dev)
{
...
+ mutex_lock(dev-mtx);
...
+ mutex_unlock(dev-mtx);
}
Don't you think it's needed? You call adu_abort_transfers from adu_release
only where it's locked by adutux_mutex.
Yes, it's not needed anymore. It's a carry-over from the time
when the old code aborted ransfers from the outside of adutux_mutex.
/* send off the urb */
usb_fill_int_urb(
dev-interrupt_out_urb,
dev-interrupt_in_endpoint-bInterval);
Typo? Seems like that.
Looks like one. It actually is a risky thing to fix... What if the device
as broken descriptors and the driver worked only thanks to the bug above?
But let's fix it and see.
I just tried your patch on the adutux from 2.6.24-rc1 but on my 2.6.20.7. It
failed with the following message:
[176200.425776] list_add corruption. next-prev should be prev (c15e6c30),
but was . (next=c58bdf5c).
[176200.426536] kernel BUG at lib/list_debug.c:28!
[176200.427843] [c01e31e3] list_add+0xa/0xf
[176200.427848] [c012bc4b] add_wait_queue+0x1f/0x2d
This is probably what Oliver caught, I did not have all removals
from wait queue where necessary.
[176200.428593] 3BUG: sleeping function called from invalid context at
kernel/rwsem.c:20
This looks like a debugging-caused oops. It should go away by itself
once the above is fixed.
I'm not sure whether it's because I'm trying it on 2.6.20.7 and not on
2.6.24-rc1.
Will check it on 2.6.24-rc1 asap and let you know.
Please try attached on any of those.
Thank you,
-- Pete
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..bea6262 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -79,12 +79,22 @@ MODULE_DEVICE_TABLE(usb, device_table);
#define COMMAND_TIMEOUT(2*HZ) /* 60 second timeout for a command */
+/*
+ * The locking scheme is a vanilla 3-lock:
+ * adu_device.buflock: A spinlock, covers what IRQs touch.
+ * adutux_mutex: A Static lock to cover open_count. It would also cover
+ * any globals, but we don't have them in 2.6.
+ * adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
+ * It covers all of adu_device, except the open_count
+ * and what .buflock covers.
+ */
+
/* Structure to hold all of our device specific stuff */
struct adu_device {
- struct mutexmtx; /* locks this structure */
+ struct mutexmtx;
struct usb_device* udev; /* save off the usb device pointer */
struct usb_interface* interface;
- unsigned char minor; /* the starting minor number for this
device */
+ unsigned intminor; /* the starting minor number for this
device */
charserial_number[8];
int open_count; /* number of times this port has
been opened */
@@ -107,8 +117,11 @@ struct adu_device {
char* interrupt_out_buffer;
struct usb_endpoint_descriptor* interrupt_out_endpoint;
struct urb* interrupt_out_urb;
+ int out_urb_finished;
};
+static DEFINE_MUTEX(adutux_mutex);
+
static struct usb_driver adu_driver;
static void adu_debug_data(int level, const char *function, int size,
@@ -132,27 +145,31 @@ static void adu_debug_data(int level, const char
*function, int size,
*/
static void adu_abort_transfers(struct adu_device *dev)
{
- dbg(2, %s : enter, __FUNCTION__);
+ unsigned long flags;
- if (dev == NULL) {
- dbg(1, %s : dev is null, __FUNCTION__);
- goto exit;
- }
+ dbg(2, %s : enter, __FUNCTION__);
if (dev-udev == NULL) {
dbg(1, %s : udev is null, __FUNCTION__);
goto exit;
}
- dbg(2, %s : udev state %d, __FUNCTION__, dev-udev-state);
- if (dev-udev-state == USB_STATE_NOTATTACHED) {
- dbg(1, %s : udev is not attached, __FUNCTION__);
- goto exit;
- }
-
/* shutdown transfer */
- usb_unlink_urb(dev-interrupt_in_urb);
- usb_unlink_urb(dev-interrupt_out_urb);
+
+ /* XXX Anchor these instead */
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-read_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_in_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
+
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-out_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_out_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
exit:
dbg(2, %s : leave,
Re: USB: FIx locks and urb-status in adutux
BTW, the patch in my previous message had one buglet - an extra
remove_wait_queue. It was in an error case though, so it should be
ok to test. But just in case, here's a fixed one.
-- Pete
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..6914c0b 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -79,12 +79,22 @@ MODULE_DEVICE_TABLE(usb, device_table);
#define COMMAND_TIMEOUT(2*HZ) /* 60 second timeout for a command */
+/*
+ * The locking scheme is a vanilla 3-lock:
+ * adu_device.buflock: A spinlock, covers what IRQs touch.
+ * adutux_mutex: A Static lock to cover open_count. It would also cover
+ * any globals, but we don't have them in 2.6.
+ * adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
+ * It covers all of adu_device, except the open_count
+ * and what .buflock covers.
+ */
+
/* Structure to hold all of our device specific stuff */
struct adu_device {
- struct mutexmtx; /* locks this structure */
+ struct mutexmtx;
struct usb_device* udev; /* save off the usb device pointer */
struct usb_interface* interface;
- unsigned char minor; /* the starting minor number for this
device */
+ unsigned intminor; /* the starting minor number for this
device */
charserial_number[8];
int open_count; /* number of times this port has
been opened */
@@ -107,8 +117,11 @@ struct adu_device {
char* interrupt_out_buffer;
struct usb_endpoint_descriptor* interrupt_out_endpoint;
struct urb* interrupt_out_urb;
+ int out_urb_finished;
};
+static DEFINE_MUTEX(adutux_mutex);
+
static struct usb_driver adu_driver;
static void adu_debug_data(int level, const char *function, int size,
@@ -132,27 +145,31 @@ static void adu_debug_data(int level, const char
*function, int size,
*/
static void adu_abort_transfers(struct adu_device *dev)
{
- dbg(2, %s : enter, __FUNCTION__);
+ unsigned long flags;
- if (dev == NULL) {
- dbg(1, %s : dev is null, __FUNCTION__);
- goto exit;
- }
+ dbg(2, %s : enter, __FUNCTION__);
if (dev-udev == NULL) {
dbg(1, %s : udev is null, __FUNCTION__);
goto exit;
}
- dbg(2, %s : udev state %d, __FUNCTION__, dev-udev-state);
- if (dev-udev-state == USB_STATE_NOTATTACHED) {
- dbg(1, %s : udev is not attached, __FUNCTION__);
- goto exit;
- }
-
/* shutdown transfer */
- usb_unlink_urb(dev-interrupt_in_urb);
- usb_unlink_urb(dev-interrupt_out_urb);
+
+ /* XXX Anchor these instead */
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-read_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_in_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
+
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-out_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_out_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
exit:
dbg(2, %s : leave, __FUNCTION__);
@@ -162,8 +179,6 @@ static void adu_delete(struct adu_device *dev)
{
dbg(2, %s enter, __FUNCTION__);
- adu_abort_transfers(dev);
-
/* free data structures */
usb_free_urb(dev-interrupt_in_urb);
usb_free_urb(dev-interrupt_out_urb);
@@ -239,7 +254,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
goto exit;
}
- wake_up_interruptible(dev-write_wait);
+ spin_lock(dev-buflock);
+ dev-out_urb_finished = 1;
+ wake_up(dev-write_wait);
+ spin_unlock(dev-buflock);
exit:
adu_debug_data(5, __FUNCTION__, urb-actual_length,
@@ -252,12 +270,17 @@ static int adu_open(struct inode *inode, struct file
*file)
struct adu_device *dev = NULL;
struct usb_interface *interface;
int subminor;
- int retval = 0;
+ int retval;
dbg(2,%s : enter, __FUNCTION__);
subminor = iminor(inode);
+ if ((retval = mutex_lock_interruptible(adutux_mutex))) {
+ dbg(2, %s : mutex lock failed, __FUNCTION__);
+ goto exit_no_lock;
+ }
+
interface = usb_find_interface(adu_driver, subminor);
if (!interface) {
err(%s - error, can't find device for minor %d,
@@ -272,13 +295,6 @@ static int adu_open(struct inode *inode, struct file *file)
goto exit_no_device;
}
- /* lock this device */
- if ((retval =
Re: USB: FIx locks and urb-status in adutux
Am Dienstag 23 Oktober 2007 schrieb Pete Zaitcev:
On Tue, 23 Oct 2007 11:38:37 +0200, Oliver Neukum [EMAIL PROTECTED] wrote:
+ /* XXX Anchor these instead */
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-read_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_in_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
Why bother? Simply call usb_kill_urb() unconditionally.
Is it always safe to kill unfilled URBs? The filled but unsubmitted ones
are ok, but in this case it's possible that we only allocated something
but never submitted. Our current implementation happens to be safe by
virtue of -dev being NULL in such case. I do not remember if we always
guaranteed that and since Vitaly is going to take this code for a
backport, I decided to play it safe.
I am not sure as far as 2.4 is concerned. In fact I am not sure 2.4 has
usb_kill_urb() at all.
Regards
Oliver
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb-status in adutux
Pete,
On Wed, 2007-10-24 at 04:53, Pete Zaitcev wrote:
1)
+/*
+ * The locking scheme is a vanilla 3-lock:
+ * adu_device.buflock: A spinlock, covers what IRQs touch.
+ * adutux_mutex: A Static lock to cover open_count. It would also
cover
+ * any globals, but we don't have them in 2.6.
+ * adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
+ * It covers all of adu_device, except the open_count
+ * and what .buflock covers.
+ */
static void adu_abort_transfers(struct adu_device *dev)
{
...
+ mutex_lock(dev-mtx);
...
+ mutex_unlock(dev-mtx);
}
Don't you think it's needed? You call adu_abort_transfers from adu_release only
where it's locked by adutux_mutex.
2)
Also one issue. adu_write has:
/* send off the urb */
usb_fill_int_urb(
dev-interrupt_out_urb,
dev-udev,
usb_sndintpipe(dev-udev,
dev-interrupt_out_endpoint-bEndpointAddress),
dev-interrupt_out_buffer,
bytes_to_write,
adu_interrupt_out_callback,
dev,
dev-interrupt_in_endpoint-bInterval);
Seems like there should be not:
dev-interrupt_in_endpoint-bInterval
but:
dev-interrupt_out_endpoint-bInterval
Typo? Seems like that.
I just tried your patch on the adutux from 2.6.24-rc1 but on my 2.6.20.7. It
failed with the following message:
-
[176188.466898] drivers/usb/misc/adutux.c : adu_open : enter
[176188.468000] drivers/usb/misc/adutux.c : adu_open : open count 1
[176188.468601] drivers/usb/misc/adutux.c : adu_open : leave, return value 0
[176188.486218] drivers/usb/misc/adutux.c : adu_interrupt_in_callback : enter,
status 0
[176188.486269] drivers/usb/misc/adutux.c: adu_interrupt_in_callback - length =
8, data = 00 00 00 00 00 00 00 00
[176188.486430] drivers/usb/misc/adutux.c: adu_interrupt_in_callback - length =
8, data = 00 00 00 00 00 00 00 00
[176188.486482] drivers/usb/misc/adutux.c : adu_interrupt_in_callback : leave,
status 0
[176191.303853] drivers/usb/misc/adutux.c : adu_write : enter, count = 8
[176193.301872] drivers/usb/misc/adutux.c : adu_write - command timed out.
[176193.301912] drivers/usb/misc/adutux.c : adu_write : leave, return value
-110
[176200.425157] drivers/usb/misc/adutux.c : adu_write : enter, count = 8
[176200.425776] list_add corruption. next-prev should be prev (c15e6c30), but
was . (next=c58bdf5c).
[176200.425776] list_add corruption. next-prev should be prev (c15e6c30), but
was . (next=c58bdf5c).
[176200.426475] [ cut here ]
[176200.426475] [ cut here ]
[176200.426536] kernel BUG at lib/list_debug.c:27!
[176200.426536] kernel BUG at lib/list_debug.c:28!
[176200.426647] invalid opcode: [#1]
[176200.426647] invalid opcode: [#1]
[176200.426736] Modules linked in: adutux(F) ppdev autofs4 hidp rfcomm l2cap
bluetooth sunrpc xt_tcpudp iptable_filter ip_tables x_tables dm_multipath video
button battery asus_acpi backlight ac ipv6 lp parport_pc parport floppy nvram
uhci_hcd sg snd_ens1371 gameport snd_rawmidi snd_ac97_codec ac97_bus
snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_pcm_oss
snd_mixer_oss snd_pcm pcnet32 snd_timer snd pcspkr mii soundcore i2c_piix4
i2c_core snd_page_alloc dm_snapshot dm_zero dm_mirror dm_mod ext3 jbd mptspi
scsi_transport_spi mptscsih sd_mod scsi_mod mptbase
[176200.426736] Modules linked in: adutux(F) ppdev autofs4 hidp rfcomm l2cap
bluetooth sunrpc xt_tcpudp iptable_filter ip_tables x_tables dm_multipath video
button battery asus_acpi backlight ac ipv6 lp parport_pc parport floppy nvram
uhci_hcd sg snd_ens1371 gameport snd_rawmidi snd_ac97_codec ac97_bus
snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device snd_pcm_oss
snd_mixer_oss snd_pcm pcnet32 snd_timer snd pcspkr mii soundcore i2c_piix4
i2c_core snd_page_alloc dm_snapshot dm_zero dm_mirror dm_mod ext3 jbd mptspi
scsi_transport_spi mptscsih sd_mod scsi_mod mptbase
[176200.427025] CPU:0
[176200.427025] CPU:0
[176200.427027] EIP:0060:[c01e31a2]Tainted: GF VLI
[176200.427027] EIP:0060:[c01e31a2]Tainted: GF VLI
[176200.427029] EFLAGS: 00010082 (2.6.20.7 #1)
[176200.427029] EFLAGS: 00010082 (2.6.20.7 #1)
[176200.427405] EIP is at __list_add+0x29/0x60
[176200.427405] EIP is at __list_add+0x29/0x60
[176200.427448] eax: 0071 ebx: c58bdf5c ecx: c0117787 edx: d10e0ab0
[176200.427448] eax: 0071 ebx: c58bdf5c ecx: c0117787 edx: d10e0ab0
[176200.427481] esi: c15e6c08 edi: 0296 ebp: c58bded8 esp: c58bdec0
[176200.427481] esi: c15e6c08 edi: 0296 ebp: c58bded8 esp: c58bdec0
[176200.427503] ds: 007b es: 007b ss: 0068
[176200.427503] ds: 007b es: 007b ss: 0068
[176200.427532] Process aducmd (pid: 14413, ti=c58bd000 task=d10e0ab0
Re: USB: FIx locks and urb-status in adutux
Am Mittwoch 24 Oktober 2007 schrieb Pete Zaitcev:
Oliver, thanks for the inftdata catch. I also fixed the sleep_on.
But you are leaving a function while still on a waitqueue left on the stack.
Here's a patch on top of yours.
Regards
Oliver
--- work/drivers/usb/misc/adutux.c.alt 2007-10-24 16:36:02.0 +0200
+++ work/drivers/usb/misc/adutux.c 2007-10-24 16:36:06.0 +0200
@@ -567,19 +567,21 @@ static ssize_t adu_write(struct file *fi
retval = mutex_lock_interruptible(dev-mtx);
if (retval)
- goto exit_nolock;
+ goto exit_nolock_intr;
/* verify that the device wasn't unplugged */
if (dev-udev == NULL) {
+ mutex_unlock(dev-mtx);
retval = -ENODEV;
err(No device or device unplugged %d, retval);
- goto exit;
+ goto exit_nolock_intr;
}
/* verify that we actually have some data to write */
if (count == 0) {
+ mutex_unlock(dev-mtx);
dbg(1, %s : write request of 0 bytes, __FUNCTION__);
- goto exit;
+ goto exit_nolock_intr;
}
add_wait_queue(dev-write_wait, waita);
@@ -649,13 +651,14 @@ static ssize_t adu_write(struct file *fi
bytes_written += bytes_to_write;
}
}
- remove_wait_queue(dev-write_wait, waita);
retval = bytes_written;
exit:
mutex_unlock(dev-mtx);
exit_nolock:
+ remove_wait_queue(dev-write_wait, waita);
+exit_nolock_intr:
dbg(2, %s : leave, return value %d, __FUNCTION__, retval);
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb-status in adutux
On Wed, Oct 24, 2007 at 04:49:12PM +0200, Oliver Neukum wrote: Am Mittwoch 24 Oktober 2007 schrieb Pete Zaitcev: Oliver, thanks for the inftdata catch. I also fixed the sleep_on. But you are leaving a function while still on a waitqueue left on the stack. Here's a patch on top of yours. Hm, I think I'll wait for Pete and you to agree and send me a final version before applying anything here :) thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: USB: FIx locks and urb->status in adutux
Two main issues fixed here are:
- An improper use of in-struct lock to protect an open count
- Use of urb status for -EINPROGRESS
Also, along the way:
- Change usb_unlink_urb to usb_kill_urb. Apparently there's no need
to use usb_unlink_urb whatsoever in this driver, and the old use of
usb_kill_urb was outright racy (it unlinked and immediately freed).
- Fix indentation in adu_write. Looks like it was damaged by a script.
- Remove sleep_on.
Signed-off-by: Pete Zaitcev <[EMAIL PROTECTED]>
---
Oliver, thanks for the inftdata catch. I also fixed the sleep_on.
But I left the silly dances around usb_kill_urb in, at least for now.
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..d278161 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -79,12 +79,22 @@ MODULE_DEVICE_TABLE(usb, device_table);
#define COMMAND_TIMEOUT(2*HZ) /* 60 second timeout for a command */
+/*
+ * The locking scheme is a vanilla 3-lock:
+ * adu_device.buflock: A spinlock, covers what IRQs touch.
+ * adutux_mutex: A Static lock to cover open_count. It would also cover
+ * any globals, but we don't have them in 2.6.
+ * adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
+ * It covers all of adu_device, except the open_count
+ * and what .buflock covers.
+ */
+
/* Structure to hold all of our device specific stuff */
struct adu_device {
- struct mutexmtx; /* locks this structure */
+ struct mutexmtx;
struct usb_device* udev; /* save off the usb device pointer */
struct usb_interface* interface;
- unsigned char minor; /* the starting minor number for this
device */
+ unsigned intminor; /* the starting minor number for this
device */
charserial_number[8];
int open_count; /* number of times this port has
been opened */
@@ -107,8 +117,11 @@ struct adu_device {
char* interrupt_out_buffer;
struct usb_endpoint_descriptor* interrupt_out_endpoint;
struct urb* interrupt_out_urb;
+ int out_urb_finished;
};
+static DEFINE_MUTEX(adutux_mutex);
+
static struct usb_driver adu_driver;
static void adu_debug_data(int level, const char *function, int size,
@@ -132,29 +145,36 @@ static void adu_debug_data(int level, const char
*function, int size,
*/
static void adu_abort_transfers(struct adu_device *dev)
{
+ unsigned long flags;
+
dbg(2," %s : enter", __FUNCTION__);
- if (dev == NULL) {
- dbg(1," %s : dev is null", __FUNCTION__);
- goto exit;
- }
+ mutex_lock(>mtx);
if (dev->udev == NULL) {
dbg(1," %s : udev is null", __FUNCTION__);
goto exit;
}
- dbg(2," %s : udev state %d", __FUNCTION__, dev->udev->state);
- if (dev->udev->state == USB_STATE_NOTATTACHED) {
- dbg(1," %s : udev is not attached", __FUNCTION__);
- goto exit;
- }
-
/* shutdown transfer */
- usb_unlink_urb(dev->interrupt_in_urb);
- usb_unlink_urb(dev->interrupt_out_urb);
+
+ /* XXX Anchor these instead */
+ spin_lock_irqsave(>buflock, flags);
+ if (!dev->read_urb_finished) {
+ spin_unlock_irqrestore(>buflock, flags);
+ usb_kill_urb(dev->interrupt_in_urb);
+ } else
+ spin_unlock_irqrestore(>buflock, flags);
+
+ spin_lock_irqsave(>buflock, flags);
+ if (!dev->out_urb_finished) {
+ spin_unlock_irqrestore(>buflock, flags);
+ usb_kill_urb(dev->interrupt_out_urb);
+ } else
+ spin_unlock_irqrestore(>buflock, flags);
exit:
+ mutex_unlock(>mtx);
dbg(2," %s : leave", __FUNCTION__);
}
@@ -162,8 +182,6 @@ static void adu_delete(struct adu_device *dev)
{
dbg(2, "%s enter", __FUNCTION__);
- adu_abort_transfers(dev);
-
/* free data structures */
usb_free_urb(dev->interrupt_in_urb);
usb_free_urb(dev->interrupt_out_urb);
@@ -239,7 +257,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
goto exit;
}
- wake_up_interruptible(>write_wait);
+ spin_lock(>buflock);
+ dev->out_urb_finished = 1;
+ wake_up(>write_wait);
+ spin_unlock(>buflock);
exit:
adu_debug_data(5, __FUNCTION__, urb->actual_length,
@@ -252,12 +273,17 @@ static int adu_open(struct inode *inode, struct file
*file)
struct adu_device *dev = NULL;
struct usb_interface *interface;
int subminor;
- int retval = 0;
+ int retval;
dbg(2,"%s : enter", __FUNCTION__);
subminor = iminor(inode);
+ if ((retval = mutex_lock_interruptible(_mutex))) {
Re: USB: FIx locks and urb->status in adutux
On Tue, 23 Oct 2007 11:38:37 +0200, Oliver Neukum <[EMAIL PROTECTED]> wrote:
> > + /* XXX Anchor these instead */
> > + spin_lock_irqsave(>buflock, flags);
> > + if (!dev->read_urb_finished) {
> > + spin_unlock_irqrestore(>buflock, flags);
> > + usb_kill_urb(dev->interrupt_in_urb);
> > + } else
> > + spin_unlock_irqrestore(>buflock, flags);
> Why bother? Simply call usb_kill_urb() unconditionally.
Is it always safe to kill unfilled URBs? The filled but unsubmitted ones
are ok, but in this case it's possible that we only allocated something
but never submitted. Our current implementation happens to be safe by
virtue of ->dev being NULL in such case. I do not remember if we always
guaranteed that and since Vitaly is going to take this code for a
backport, I decided to play it safe.
I would rather leave this.
> > @@ -162,8 +182,6 @@ static void adu_delete(struct adu_device *dev)
> > {
> > dbg(2, "%s enter", __FUNCTION__);
> >
> > - adu_abort_transfers(dev);
> > -
> > /* free data structures */
> > usb_free_urb(dev->interrupt_in_urb);
> > usb_free_urb(dev->interrupt_out_urb);
> > @@ -239,7 +257,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
> > goto exit;
> > }
> >
> > + spin_lock(>buflock);
> > + dev->out_urb_finished = 1;
> > wake_up_interruptible(>write_wait);
> > + spin_unlock(>buflock);
>
> This leaves a race here:
>
> while (count > 0) {
> spin_lock_irqsave(>buflock, flags);
> if (!dev->out_urb_finished) {
> spin_unlock_irqrestore(>buflock, flags);
>
> timeout = COMMAND_TIMEOUT;
> while (timeout > 0) {
> if (signal_pending(current)) {
> dbg(1," %s : interrupted",
> __FUNCTION__);
> retval = -EINTR;
> goto exit;
> }
> mutex_unlock(>mtx);
> timeout =
> interruptible_sleep_on_timeout(>write_wait, timeout);
>
> You can detect that the urb has not finished but the notification happens
> before
> you go to sleep.
That's a common issue with sleep_on and its derivatives really.
I would need to replace it with explicit queue adds to fix.
I suppose I can fix it too.
> > @@ -272,13 +293,11 @@ static int adu_open(struct inode *inode, struct file
> > *file)
> > goto exit_no_device;
> > }
> >
> > - /* lock this device */
> > - if ((retval = mutex_lock_interruptible(>mtx))) {
> > + if ((retval = mutex_lock_interruptible(_mutex))) {
> > dbg(2, "%s : mutex lock failed", __FUNCTION__);
> > goto exit_no_device;
> > }
> This is racy:
> dev = usb_get_intfdata(interface);
> if ((retval = mutex_lock_interruptible(_mutex))) {
> dbg(2, "%s : mutex lock failed", __FUNCTION__);
> goto exit_no_device;
> }
>
> You need to manipulate intfdata under lock, or disconnect will
> happily free the datastructures.
Ah yes. Sorry about that, will fix.
> > @@ -297,13 +316,14 @@ static int adu_open(struct inode *inode, struct file
> > *file)
> > dev->read_urb_finished = 0;
> > retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
> > - if (retval)
> > + if (retval) {
> > + dev->read_urb_finished = 1;
> > --dev->open_count;
> > + }
>
> You should set file->private_data to NULL in the error case.
What for? Nobody ever tests it or dereferences it after ->open returned
an error. I can set 0x to it just as well.
> > @@ -486,10 +495,14 @@ static ssize_t adu_read(struct file *file, __user
> > char *buffer, size_t count,
> > /* we wait for I/O to complete */
> > set_current_state(TASK_INTERRUPTIBLE);
> > add_wait_queue(>read_wait, );
> > - if (!dev->read_urb_finished)
> > + spin_lock_irqsave(>buflock, flags);
> > + if (!dev->read_urb_finished) {
> > + spin_unlock_irqrestore(>buflock,
> > flags);
> > timeout =
> > schedule_timeout(COMMAND_TIMEOUT);
>
> I find it a bit silly to bother with _irqsave if you call schedule_timeout()
> in the
> next line.
True, this is not really necessary... And I set and clear such flags
without locking around them when I'm sure that URB is not in progress.
I just added it in case someone wants to expand this code by testing
two things together or something, because here the callback certainly
can strike at any time. The whole excercise started because Vitaliy
wanted to reuse the code.
-- Pete
-
To unsubscribe from this list: send the
Re: [linux-usb-devel] USB: FIx locks and urb->status in adutux
Am Dienstag 23 Oktober 2007 schrieb Pete Zaitcev:
> Two main issues fixed here are:
> - An improper use of in-struct lock to protect an open count
> - Use of urb status for -EINPROGRESS
> + /* XXX Anchor these instead */
> + spin_lock_irqsave(>buflock, flags);
> + if (!dev->read_urb_finished) {
> + spin_unlock_irqrestore(>buflock, flags);
> + usb_kill_urb(dev->interrupt_in_urb);
> + } else
> + spin_unlock_irqrestore(>buflock, flags);
> +
> + spin_lock_irqsave(>buflock, flags);
> + if (!dev->out_urb_finished) {
> + spin_unlock_irqrestore(>buflock, flags);
> + usb_kill_urb(dev->interrupt_out_urb);
> + } else
> + spin_unlock_irqrestore(>buflock, flags);
Why bother? Simply call usb_kill_urb() unconditionally.
> exit:
> + mutex_unlock(>mtx);
> dbg(2," %s : leave", __FUNCTION__);
> }
>
> @@ -162,8 +182,6 @@ static void adu_delete(struct adu_device *dev)
> {
> dbg(2, "%s enter", __FUNCTION__);
>
> - adu_abort_transfers(dev);
> -
> /* free data structures */
> usb_free_urb(dev->interrupt_in_urb);
> usb_free_urb(dev->interrupt_out_urb);
> @@ -239,7 +257,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
> goto exit;
> }
>
> + spin_lock(>buflock);
> + dev->out_urb_finished = 1;
> wake_up_interruptible(>write_wait);
> + spin_unlock(>buflock);
This leaves a race here:
while (count > 0) {
spin_lock_irqsave(>buflock, flags);
if (!dev->out_urb_finished) {
spin_unlock_irqrestore(>buflock, flags);
timeout = COMMAND_TIMEOUT;
while (timeout > 0) {
if (signal_pending(current)) {
dbg(1," %s : interrupted",
__FUNCTION__);
retval = -EINTR;
goto exit;
}
mutex_unlock(>mtx);
timeout =
interruptible_sleep_on_timeout(>write_wait, timeout);
You can detect that the urb has not finished but the notification happens before
you go to sleep.
> exit:
>
> adu_debug_data(5, __FUNCTION__, urb->actual_length,
> @@ -272,13 +293,11 @@ static int adu_open(struct inode *inode, struct file
> *file)
> goto exit_no_device;
> }
>
> - /* lock this device */
> - if ((retval = mutex_lock_interruptible(>mtx))) {
> + if ((retval = mutex_lock_interruptible(_mutex))) {
> dbg(2, "%s : mutex lock failed", __FUNCTION__);
> goto exit_no_device;
> }
This is racy:
dev = usb_get_intfdata(interface);
if (!dev) {
retval = -ENODEV;
goto exit_no_device;
}
if ((retval = mutex_lock_interruptible(_mutex))) {
dbg(2, "%s : mutex lock failed", __FUNCTION__);
goto exit_no_device;
}
You need to manipulate intfdata under lock, or disconnect will
happily free the datastructures.
>
> - /* increment our usage count for the device */
> ++dev->open_count;
> dbg(2,"%s : open count %d", __FUNCTION__, dev->open_count);
>
> @@ -297,13 +316,14 @@ static int adu_open(struct inode *inode, struct file
> *file)
>
> le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize),
>adu_interrupt_in_callback, dev,
>dev->interrupt_in_endpoint->bInterval);
> - /* dev->interrupt_in_urb->transfer_flags |= URB_ASYNC_UNLINK; */
> dev->read_urb_finished = 0;
> retval = usb_submit_urb(dev->interrupt_in_urb, GFP_KERNEL);
> - if (retval)
> + if (retval) {
> + dev->read_urb_finished = 1;
> --dev->open_count;
> + }
You should set file->private_data to NULL in the error case.
[..]
> @@ -486,10 +495,14 @@ static ssize_t adu_read(struct file *file, __user char
> *buffer, size_t count,
> /* we wait for I/O to complete */
> set_current_state(TASK_INTERRUPTIBLE);
> add_wait_queue(>read_wait, );
> - if (!dev->read_urb_finished)
> + spin_lock_irqsave(>buflock, flags);
> + if (!dev->read_urb_finished) {
> + spin_unlock_irqrestore(>buflock,
> flags);
> timeout =
> schedule_timeout(COMMAND_TIMEOUT);
I find it a bit silly to bother with _irqsave if you call schedule_timeout() in
the
next line.
Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe
Re: [linux-usb-devel] USB: FIx locks and urb-status in adutux
Am Dienstag 23 Oktober 2007 schrieb Pete Zaitcev:
Two main issues fixed here are:
- An improper use of in-struct lock to protect an open count
- Use of urb status for -EINPROGRESS
+ /* XXX Anchor these instead */
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-read_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_in_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
+
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-out_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_out_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
Why bother? Simply call usb_kill_urb() unconditionally.
exit:
+ mutex_unlock(dev-mtx);
dbg(2, %s : leave, __FUNCTION__);
}
@@ -162,8 +182,6 @@ static void adu_delete(struct adu_device *dev)
{
dbg(2, %s enter, __FUNCTION__);
- adu_abort_transfers(dev);
-
/* free data structures */
usb_free_urb(dev-interrupt_in_urb);
usb_free_urb(dev-interrupt_out_urb);
@@ -239,7 +257,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
goto exit;
}
+ spin_lock(dev-buflock);
+ dev-out_urb_finished = 1;
wake_up_interruptible(dev-write_wait);
+ spin_unlock(dev-buflock);
This leaves a race here:
while (count 0) {
spin_lock_irqsave(dev-buflock, flags);
if (!dev-out_urb_finished) {
spin_unlock_irqrestore(dev-buflock, flags);
timeout = COMMAND_TIMEOUT;
while (timeout 0) {
if (signal_pending(current)) {
dbg(1, %s : interrupted,
__FUNCTION__);
retval = -EINTR;
goto exit;
}
mutex_unlock(dev-mtx);
timeout =
interruptible_sleep_on_timeout(dev-write_wait, timeout);
You can detect that the urb has not finished but the notification happens before
you go to sleep.
exit:
adu_debug_data(5, __FUNCTION__, urb-actual_length,
@@ -272,13 +293,11 @@ static int adu_open(struct inode *inode, struct file
*file)
goto exit_no_device;
}
- /* lock this device */
- if ((retval = mutex_lock_interruptible(dev-mtx))) {
+ if ((retval = mutex_lock_interruptible(adutux_mutex))) {
dbg(2, %s : mutex lock failed, __FUNCTION__);
goto exit_no_device;
}
This is racy:
dev = usb_get_intfdata(interface);
if (!dev) {
retval = -ENODEV;
goto exit_no_device;
}
if ((retval = mutex_lock_interruptible(adutux_mutex))) {
dbg(2, %s : mutex lock failed, __FUNCTION__);
goto exit_no_device;
}
You need to manipulate intfdata under lock, or disconnect will
happily free the datastructures.
- /* increment our usage count for the device */
++dev-open_count;
dbg(2,%s : open count %d, __FUNCTION__, dev-open_count);
@@ -297,13 +316,14 @@ static int adu_open(struct inode *inode, struct file
*file)
le16_to_cpu(dev-interrupt_in_endpoint-wMaxPacketSize),
adu_interrupt_in_callback, dev,
dev-interrupt_in_endpoint-bInterval);
- /* dev-interrupt_in_urb-transfer_flags |= URB_ASYNC_UNLINK; */
dev-read_urb_finished = 0;
retval = usb_submit_urb(dev-interrupt_in_urb, GFP_KERNEL);
- if (retval)
+ if (retval) {
+ dev-read_urb_finished = 1;
--dev-open_count;
+ }
You should set file-private_data to NULL in the error case.
[..]
@@ -486,10 +495,14 @@ static ssize_t adu_read(struct file *file, __user char
*buffer, size_t count,
/* we wait for I/O to complete */
set_current_state(TASK_INTERRUPTIBLE);
add_wait_queue(dev-read_wait, wait);
- if (!dev-read_urb_finished)
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-read_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock,
flags);
timeout =
schedule_timeout(COMMAND_TIMEOUT);
I find it a bit silly to bother with _irqsave if you call schedule_timeout() in
the
next line.
Regards
Oliver
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More
Re: USB: FIx locks and urb-status in adutux
On Tue, 23 Oct 2007 11:38:37 +0200, Oliver Neukum [EMAIL PROTECTED] wrote:
+ /* XXX Anchor these instead */
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-read_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_in_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
Why bother? Simply call usb_kill_urb() unconditionally.
Is it always safe to kill unfilled URBs? The filled but unsubmitted ones
are ok, but in this case it's possible that we only allocated something
but never submitted. Our current implementation happens to be safe by
virtue of -dev being NULL in such case. I do not remember if we always
guaranteed that and since Vitaly is going to take this code for a
backport, I decided to play it safe.
I would rather leave this.
@@ -162,8 +182,6 @@ static void adu_delete(struct adu_device *dev)
{
dbg(2, %s enter, __FUNCTION__);
- adu_abort_transfers(dev);
-
/* free data structures */
usb_free_urb(dev-interrupt_in_urb);
usb_free_urb(dev-interrupt_out_urb);
@@ -239,7 +257,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
goto exit;
}
+ spin_lock(dev-buflock);
+ dev-out_urb_finished = 1;
wake_up_interruptible(dev-write_wait);
+ spin_unlock(dev-buflock);
This leaves a race here:
while (count 0) {
spin_lock_irqsave(dev-buflock, flags);
if (!dev-out_urb_finished) {
spin_unlock_irqrestore(dev-buflock, flags);
timeout = COMMAND_TIMEOUT;
while (timeout 0) {
if (signal_pending(current)) {
dbg(1, %s : interrupted,
__FUNCTION__);
retval = -EINTR;
goto exit;
}
mutex_unlock(dev-mtx);
timeout =
interruptible_sleep_on_timeout(dev-write_wait, timeout);
You can detect that the urb has not finished but the notification happens
before
you go to sleep.
That's a common issue with sleep_on and its derivatives really.
I would need to replace it with explicit queue adds to fix.
I suppose I can fix it too.
@@ -272,13 +293,11 @@ static int adu_open(struct inode *inode, struct file
*file)
goto exit_no_device;
}
- /* lock this device */
- if ((retval = mutex_lock_interruptible(dev-mtx))) {
+ if ((retval = mutex_lock_interruptible(adutux_mutex))) {
dbg(2, %s : mutex lock failed, __FUNCTION__);
goto exit_no_device;
}
This is racy:
dev = usb_get_intfdata(interface);
if ((retval = mutex_lock_interruptible(adutux_mutex))) {
dbg(2, %s : mutex lock failed, __FUNCTION__);
goto exit_no_device;
}
You need to manipulate intfdata under lock, or disconnect will
happily free the datastructures.
Ah yes. Sorry about that, will fix.
@@ -297,13 +316,14 @@ static int adu_open(struct inode *inode, struct file
*file)
dev-read_urb_finished = 0;
retval = usb_submit_urb(dev-interrupt_in_urb, GFP_KERNEL);
- if (retval)
+ if (retval) {
+ dev-read_urb_finished = 1;
--dev-open_count;
+ }
You should set file-private_data to NULL in the error case.
What for? Nobody ever tests it or dereferences it after -open returned
an error. I can set 0x to it just as well.
@@ -486,10 +495,14 @@ static ssize_t adu_read(struct file *file, __user
char *buffer, size_t count,
/* we wait for I/O to complete */
set_current_state(TASK_INTERRUPTIBLE);
add_wait_queue(dev-read_wait, wait);
- if (!dev-read_urb_finished)
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-read_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock,
flags);
timeout =
schedule_timeout(COMMAND_TIMEOUT);
I find it a bit silly to bother with _irqsave if you call schedule_timeout()
in the
next line.
True, this is not really necessary... And I set and clear such flags
without locking around them when I'm sure that URB is not in progress.
I just added it in case someone wants to expand this code by testing
two things together or something, because here the callback certainly
can strike at any time. The whole excercise started because Vitaliy
wanted to reuse the code.
-- Pete
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at
Re: USB: FIx locks and urb-status in adutux
Two main issues fixed here are:
- An improper use of in-struct lock to protect an open count
- Use of urb status for -EINPROGRESS
Also, along the way:
- Change usb_unlink_urb to usb_kill_urb. Apparently there's no need
to use usb_unlink_urb whatsoever in this driver, and the old use of
usb_kill_urb was outright racy (it unlinked and immediately freed).
- Fix indentation in adu_write. Looks like it was damaged by a script.
- Remove sleep_on.
Signed-off-by: Pete Zaitcev [EMAIL PROTECTED]
---
Oliver, thanks for the inftdata catch. I also fixed the sleep_on.
But I left the silly dances around usb_kill_urb in, at least for now.
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..d278161 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -79,12 +79,22 @@ MODULE_DEVICE_TABLE(usb, device_table);
#define COMMAND_TIMEOUT(2*HZ) /* 60 second timeout for a command */
+/*
+ * The locking scheme is a vanilla 3-lock:
+ * adu_device.buflock: A spinlock, covers what IRQs touch.
+ * adutux_mutex: A Static lock to cover open_count. It would also cover
+ * any globals, but we don't have them in 2.6.
+ * adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
+ * It covers all of adu_device, except the open_count
+ * and what .buflock covers.
+ */
+
/* Structure to hold all of our device specific stuff */
struct adu_device {
- struct mutexmtx; /* locks this structure */
+ struct mutexmtx;
struct usb_device* udev; /* save off the usb device pointer */
struct usb_interface* interface;
- unsigned char minor; /* the starting minor number for this
device */
+ unsigned intminor; /* the starting minor number for this
device */
charserial_number[8];
int open_count; /* number of times this port has
been opened */
@@ -107,8 +117,11 @@ struct adu_device {
char* interrupt_out_buffer;
struct usb_endpoint_descriptor* interrupt_out_endpoint;
struct urb* interrupt_out_urb;
+ int out_urb_finished;
};
+static DEFINE_MUTEX(adutux_mutex);
+
static struct usb_driver adu_driver;
static void adu_debug_data(int level, const char *function, int size,
@@ -132,29 +145,36 @@ static void adu_debug_data(int level, const char
*function, int size,
*/
static void adu_abort_transfers(struct adu_device *dev)
{
+ unsigned long flags;
+
dbg(2, %s : enter, __FUNCTION__);
- if (dev == NULL) {
- dbg(1, %s : dev is null, __FUNCTION__);
- goto exit;
- }
+ mutex_lock(dev-mtx);
if (dev-udev == NULL) {
dbg(1, %s : udev is null, __FUNCTION__);
goto exit;
}
- dbg(2, %s : udev state %d, __FUNCTION__, dev-udev-state);
- if (dev-udev-state == USB_STATE_NOTATTACHED) {
- dbg(1, %s : udev is not attached, __FUNCTION__);
- goto exit;
- }
-
/* shutdown transfer */
- usb_unlink_urb(dev-interrupt_in_urb);
- usb_unlink_urb(dev-interrupt_out_urb);
+
+ /* XXX Anchor these instead */
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-read_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_in_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
+
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-out_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_out_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
exit:
+ mutex_unlock(dev-mtx);
dbg(2, %s : leave, __FUNCTION__);
}
@@ -162,8 +182,6 @@ static void adu_delete(struct adu_device *dev)
{
dbg(2, %s enter, __FUNCTION__);
- adu_abort_transfers(dev);
-
/* free data structures */
usb_free_urb(dev-interrupt_in_urb);
usb_free_urb(dev-interrupt_out_urb);
@@ -239,7 +257,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
goto exit;
}
- wake_up_interruptible(dev-write_wait);
+ spin_lock(dev-buflock);
+ dev-out_urb_finished = 1;
+ wake_up(dev-write_wait);
+ spin_unlock(dev-buflock);
exit:
adu_debug_data(5, __FUNCTION__, urb-actual_length,
@@ -252,12 +273,17 @@ static int adu_open(struct inode *inode, struct file
*file)
struct adu_device *dev = NULL;
struct usb_interface *interface;
int subminor;
- int retval = 0;
+ int retval;
dbg(2,%s : enter, __FUNCTION__);
subminor = iminor(inode);
+ if ((retval =
USB: FIx locks and urb->status in adutux
Two main issues fixed here are:
- An improper use of in-struct lock to protect an open count
- Use of urb status for -EINPROGRESS
Also, along the way:
- Change usb_unlink_urb to usb_kill_urb. Apparently there's no need
to use usb_unlink_urb whatsoever in this driver, and the old use of
usb_kill_urb was outright racy (it unlinked and immediately freed).
- Fix indentation in adu_write. Looks like it was damaged by a script.
Signed-off-by: Pete Zaitcev <[EMAIL PROTECTED]>
---
If someone with a real device tested this, it would be great. Please
make sure to pull the plug while application is opening/closing.
A critical review is welcome too.
Vitaliy, this is the static lock example, which I promised on Friday.
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..a8afb66 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -79,12 +79,22 @@ MODULE_DEVICE_TABLE(usb, device_table);
#define COMMAND_TIMEOUT(2*HZ) /* 60 second timeout for a command */
+/*
+ * The locking scheme is a vanilla 3-lock:
+ * adu_device.buflock: A spinlock, covers what IRQs touch.
+ * adutux_mutex: A Static lock to cover open_count. It would also cover
+ * any globals, but we don't have them in 2.6.
+ * adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
+ * It covers all of adu_device, except the open_count
+ * and what .buflock covers.
+ */
+
/* Structure to hold all of our device specific stuff */
struct adu_device {
- struct mutexmtx; /* locks this structure */
+ struct mutexmtx;
struct usb_device* udev; /* save off the usb device pointer */
struct usb_interface* interface;
- unsigned char minor; /* the starting minor number for this
device */
+ unsigned intminor; /* the starting minor number for this
device */
charserial_number[8];
int open_count; /* number of times this port has
been opened */
@@ -107,8 +117,11 @@ struct adu_device {
char* interrupt_out_buffer;
struct usb_endpoint_descriptor* interrupt_out_endpoint;
struct urb* interrupt_out_urb;
+ int out_urb_finished;
};
+static DEFINE_MUTEX(adutux_mutex);
+
static struct usb_driver adu_driver;
static void adu_debug_data(int level, const char *function, int size,
@@ -132,29 +145,36 @@ static void adu_debug_data(int level, const char
*function, int size,
*/
static void adu_abort_transfers(struct adu_device *dev)
{
+ unsigned long flags;
+
dbg(2," %s : enter", __FUNCTION__);
- if (dev == NULL) {
- dbg(1," %s : dev is null", __FUNCTION__);
- goto exit;
- }
+ mutex_lock(>mtx);
if (dev->udev == NULL) {
dbg(1," %s : udev is null", __FUNCTION__);
goto exit;
}
- dbg(2," %s : udev state %d", __FUNCTION__, dev->udev->state);
- if (dev->udev->state == USB_STATE_NOTATTACHED) {
- dbg(1," %s : udev is not attached", __FUNCTION__);
- goto exit;
- }
-
/* shutdown transfer */
- usb_unlink_urb(dev->interrupt_in_urb);
- usb_unlink_urb(dev->interrupt_out_urb);
+
+ /* XXX Anchor these instead */
+ spin_lock_irqsave(>buflock, flags);
+ if (!dev->read_urb_finished) {
+ spin_unlock_irqrestore(>buflock, flags);
+ usb_kill_urb(dev->interrupt_in_urb);
+ } else
+ spin_unlock_irqrestore(>buflock, flags);
+
+ spin_lock_irqsave(>buflock, flags);
+ if (!dev->out_urb_finished) {
+ spin_unlock_irqrestore(>buflock, flags);
+ usb_kill_urb(dev->interrupt_out_urb);
+ } else
+ spin_unlock_irqrestore(>buflock, flags);
exit:
+ mutex_unlock(>mtx);
dbg(2," %s : leave", __FUNCTION__);
}
@@ -162,8 +182,6 @@ static void adu_delete(struct adu_device *dev)
{
dbg(2, "%s enter", __FUNCTION__);
- adu_abort_transfers(dev);
-
/* free data structures */
usb_free_urb(dev->interrupt_in_urb);
usb_free_urb(dev->interrupt_out_urb);
@@ -239,7 +257,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
goto exit;
}
+ spin_lock(>buflock);
+ dev->out_urb_finished = 1;
wake_up_interruptible(>write_wait);
+ spin_unlock(>buflock);
exit:
adu_debug_data(5, __FUNCTION__, urb->actual_length,
@@ -272,13 +293,11 @@ static int adu_open(struct inode *inode, struct file
*file)
goto exit_no_device;
}
- /* lock this device */
- if ((retval = mutex_lock_interruptible(>mtx))) {
+ if ((retval = mutex_lock_interruptible(_mutex))) {
dbg(2, "%s : mutex
USB: FIx locks and urb-status in adutux
Two main issues fixed here are:
- An improper use of in-struct lock to protect an open count
- Use of urb status for -EINPROGRESS
Also, along the way:
- Change usb_unlink_urb to usb_kill_urb. Apparently there's no need
to use usb_unlink_urb whatsoever in this driver, and the old use of
usb_kill_urb was outright racy (it unlinked and immediately freed).
- Fix indentation in adu_write. Looks like it was damaged by a script.
Signed-off-by: Pete Zaitcev [EMAIL PROTECTED]
---
If someone with a real device tested this, it would be great. Please
make sure to pull the plug while application is opening/closing.
A critical review is welcome too.
Vitaliy, this is the static lock example, which I promised on Friday.
diff --git a/drivers/usb/misc/adutux.c b/drivers/usb/misc/adutux.c
index c567aa7..a8afb66 100644
--- a/drivers/usb/misc/adutux.c
+++ b/drivers/usb/misc/adutux.c
@@ -79,12 +79,22 @@ MODULE_DEVICE_TABLE(usb, device_table);
#define COMMAND_TIMEOUT(2*HZ) /* 60 second timeout for a command */
+/*
+ * The locking scheme is a vanilla 3-lock:
+ * adu_device.buflock: A spinlock, covers what IRQs touch.
+ * adutux_mutex: A Static lock to cover open_count. It would also cover
+ * any globals, but we don't have them in 2.6.
+ * adu_device.mtx: A mutex to hold across sleepers like copy_from_user.
+ * It covers all of adu_device, except the open_count
+ * and what .buflock covers.
+ */
+
/* Structure to hold all of our device specific stuff */
struct adu_device {
- struct mutexmtx; /* locks this structure */
+ struct mutexmtx;
struct usb_device* udev; /* save off the usb device pointer */
struct usb_interface* interface;
- unsigned char minor; /* the starting minor number for this
device */
+ unsigned intminor; /* the starting minor number for this
device */
charserial_number[8];
int open_count; /* number of times this port has
been opened */
@@ -107,8 +117,11 @@ struct adu_device {
char* interrupt_out_buffer;
struct usb_endpoint_descriptor* interrupt_out_endpoint;
struct urb* interrupt_out_urb;
+ int out_urb_finished;
};
+static DEFINE_MUTEX(adutux_mutex);
+
static struct usb_driver adu_driver;
static void adu_debug_data(int level, const char *function, int size,
@@ -132,29 +145,36 @@ static void adu_debug_data(int level, const char
*function, int size,
*/
static void adu_abort_transfers(struct adu_device *dev)
{
+ unsigned long flags;
+
dbg(2, %s : enter, __FUNCTION__);
- if (dev == NULL) {
- dbg(1, %s : dev is null, __FUNCTION__);
- goto exit;
- }
+ mutex_lock(dev-mtx);
if (dev-udev == NULL) {
dbg(1, %s : udev is null, __FUNCTION__);
goto exit;
}
- dbg(2, %s : udev state %d, __FUNCTION__, dev-udev-state);
- if (dev-udev-state == USB_STATE_NOTATTACHED) {
- dbg(1, %s : udev is not attached, __FUNCTION__);
- goto exit;
- }
-
/* shutdown transfer */
- usb_unlink_urb(dev-interrupt_in_urb);
- usb_unlink_urb(dev-interrupt_out_urb);
+
+ /* XXX Anchor these instead */
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-read_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_in_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
+
+ spin_lock_irqsave(dev-buflock, flags);
+ if (!dev-out_urb_finished) {
+ spin_unlock_irqrestore(dev-buflock, flags);
+ usb_kill_urb(dev-interrupt_out_urb);
+ } else
+ spin_unlock_irqrestore(dev-buflock, flags);
exit:
+ mutex_unlock(dev-mtx);
dbg(2, %s : leave, __FUNCTION__);
}
@@ -162,8 +182,6 @@ static void adu_delete(struct adu_device *dev)
{
dbg(2, %s enter, __FUNCTION__);
- adu_abort_transfers(dev);
-
/* free data structures */
usb_free_urb(dev-interrupt_in_urb);
usb_free_urb(dev-interrupt_out_urb);
@@ -239,7 +257,10 @@ static void adu_interrupt_out_callback(struct urb *urb)
goto exit;
}
+ spin_lock(dev-buflock);
+ dev-out_urb_finished = 1;
wake_up_interruptible(dev-write_wait);
+ spin_unlock(dev-buflock);
exit:
adu_debug_data(5, __FUNCTION__, urb-actual_length,
@@ -272,13 +293,11 @@ static int adu_open(struct inode *inode, struct file
*file)
goto exit_no_device;
}
- /* lock this device */
- if ((retval = mutex_lock_interruptible(dev-mtx))) {
+ if ((retval = mutex_lock_interruptible(adutux_mutex))) {
dbg(2, %s
