Re: [PATCH] USB: usblp: don't call usb_set_interface if there's a single alt
On Sat, 23 Jan 2021 18:21:36 -0600
Jeremy Figgins wrote:
> Signed-off-by: Jeremy Figgins
> +++ b/drivers/usb/class/usblp.c
> + if (usblp->intf->num_altsetting > 1) {
Acked-by: Pete Zaitcev
I am having some misgivings about it, but let's see if it works.
At worst, someone will complain and we'll revert to quirks.
-- Pete
Re: KASAN: use-after-free Read in usblp_bulk_read
On Wed, 06 May 2020 11:14:42 +0200 Oliver Neukum wrote: > Very well. We are not going to find it without exceptional luck. Yet > there may be a real issue, too. We simply do not know. How about the > attached patch? > usblp_unlink_urbs(usblp); > mutex_unlock(>mut); > + usb_poison_anchored_urbs(>urbs); > > if (!usblp->used) > usblp_cleanup(usblp); This can't be right. Our URBs are freed by the callback, and this technique is not compatible with poisoning, at least with how the usb/core.c implements it. The usb_poison_urb() waits for URB to complete, and if the callback frees it, it's a problem. -- Pete
Re: [PATCH 1/1] usblp: do not set TASK_INTERRUPTIBLE before lock
On Mon, 2 Nov 2015 10:27:00 +0100
Jiri Slaby wrote:
> Signed-off-by: Jiri Slaby
> --- a/drivers/usb/class/usblp.c
> +++ b/drivers/usb/class/usblp.c
> @@ -884,11 +884,11 @@ static int usblp_wwait(struct usblp *usblp, int
> nonblock)
>
> add_wait_queue(>wwait, );
> for (;;) {
> - set_current_state(TASK_INTERRUPTIBLE);
> if (mutex_lock_interruptible(>mut)) {
> rc = -EINTR;
> break;
> }
> + set_current_state(TASK_INTERRUPTIBLE);
> rc = usblp_wtest(usblp, nonblock);
> mutex_unlock(>mut);
> if (rc <= 0)
I'm fully onboard with this. In the original "big cleanup" 317c67b8f,
I got this right. But then I either missed that mutex_lock_interruptible()
can set the state, or it didn't do it back then (it was in 2007), and
the 7f477358e introduced the existing code.
Acked-By: Pete Zaitcev
-- Pete
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] usblp: do not set TASK_INTERRUPTIBLE before lock
On Mon, 2 Nov 2015 10:27:00 +0100
Jiri Slaby <jsl...@suse.cz> wrote:
> Signed-off-by: Jiri Slaby <jsl...@suse.cz>
> --- a/drivers/usb/class/usblp.c
> +++ b/drivers/usb/class/usblp.c
> @@ -884,11 +884,11 @@ static int usblp_wwait(struct usblp *usblp, int
> nonblock)
>
> add_wait_queue(>wwait, );
> for (;;) {
> - set_current_state(TASK_INTERRUPTIBLE);
> if (mutex_lock_interruptible(>mut)) {
> rc = -EINTR;
> break;
> }
> + set_current_state(TASK_INTERRUPTIBLE);
> rc = usblp_wtest(usblp, nonblock);
> mutex_unlock(>mut);
> if (rc <= 0)
I'm fully onboard with this. In the original "big cleanup" 317c67b8f,
I got this right. But then I either missed that mutex_lock_interruptible()
can set the state, or it didn't do it back then (it was in 2007), and
the 7f477358e introduced the existing code.
Acked-By: Pete Zaitcev <zait...@yahoo.com>
-- Pete
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] USB: usbmon: Remove timeval usage for timestamp
On Tue, 05 May 2015 11:24:16 +0200
Arnd Bergmann wrote:
> On Tuesday 05 May 2015 11:44:33 Tina Ruchandani wrote:
> > static inline unsigned int mon_get_timestamp(void)
> > {
> > - struct timeval tval;
> > + struct timespec64 now;
> > unsigned int stamp;
> >
> > - do_gettimeofday();
> > - stamp = tval.tv_sec & 0xFFF;/* 2^32 = 4294967296. Limit to 4096s. */
> > - stamp = stamp * 100 + tval.tv_usec;
> > + getnstimeofday64();
> > + stamp = now.tv_sec & 0xFFF; /* now.tv_sec is 64-bit. Limit to 4096s */
> > + stamp = stamp * USEC_PER_SEC + now.tv_nsec / NSEC_PER_USEC;
> > return stamp;
> > }
>
> Your conversion looks entirely correct, but the original code is a bit
> odd here as it does not use the entire range of the 32-bit microsecond
> value, and counts from 0 to 409600us instead of the more intuitive
> 0 to 4294967296 us range before wrapping around.
The intent was to create a rolling timestamp that is not too large.
Remember that the text format is intended to be eyeballed. The wrap point
could be anything. No consideration was given to what's intuitive.
> it might be more obvious what is going on, but it would slightly change
> the output in the debugfs file to use the full range. Do we know what
> behavior is expected by normal user space here?
> [...]
> I also wonder if we should make the output use monotonic time instead
The only guarantee we give is that the time corresponds to microseconds.
This is sometimes necessary to debug things in microntrollers in USB
devices.
A monotonic time is fine.
One thing though, I object to Tina's new comment. It does not matter what
now.tv_sec is. The comment is about the destination, that is the stamp.
So, please don't change it, unless you change the type of the ep->tstamp.
> static inline unsigned int mon_get_timestamp(void)
> {
> return ktime_to_us(ktime_get_real());
> }
>
> it might be more obvious what is going on, but it would slightly change
Now you made the truncation explicit, so if anyhing it's less obvious.
The code is fine, but at least add a comment to that effect, if you don't
want to tack %409600 or %40.
-- Pete
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] USB: usbmon: Remove timeval usage for timestamp
On Tue, 05 May 2015 11:24:16 +0200
Arnd Bergmann a...@arndb.de wrote:
On Tuesday 05 May 2015 11:44:33 Tina Ruchandani wrote:
static inline unsigned int mon_get_timestamp(void)
{
- struct timeval tval;
+ struct timespec64 now;
unsigned int stamp;
- do_gettimeofday(tval);
- stamp = tval.tv_sec 0xFFF;/* 2^32 = 4294967296. Limit to 4096s. */
- stamp = stamp * 100 + tval.tv_usec;
+ getnstimeofday64(now);
+ stamp = now.tv_sec 0xFFF; /* now.tv_sec is 64-bit. Limit to 4096s */
+ stamp = stamp * USEC_PER_SEC + now.tv_nsec / NSEC_PER_USEC;
return stamp;
}
Your conversion looks entirely correct, but the original code is a bit
odd here as it does not use the entire range of the 32-bit microsecond
value, and counts from 0 to 409600us instead of the more intuitive
0 to 4294967296 us range before wrapping around.
The intent was to create a rolling timestamp that is not too large.
Remember that the text format is intended to be eyeballed. The wrap point
could be anything. No consideration was given to what's intuitive.
it might be more obvious what is going on, but it would slightly change
the output in the debugfs file to use the full range. Do we know what
behavior is expected by normal user space here?
[...]
I also wonder if we should make the output use monotonic time instead
The only guarantee we give is that the time corresponds to microseconds.
This is sometimes necessary to debug things in microntrollers in USB
devices.
A monotonic time is fine.
One thing though, I object to Tina's new comment. It does not matter what
now.tv_sec is. The comment is about the destination, that is the stamp.
So, please don't change it, unless you change the type of the ep-tstamp.
static inline unsigned int mon_get_timestamp(void)
{
return ktime_to_us(ktime_get_real());
}
it might be more obvious what is going on, but it would slightly change
Now you made the truncation explicit, so if anyhing it's less obvious.
The code is fine, but at least add a comment to that effect, if you don't
want to tack %409600 or %40.
-- Pete
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: improved unaligned access error fix
On Thu, 14 Nov 2013 12:09:21 -0500 Chris Metcalf wrote: > - __u8 buffer[CN_PROC_MSG_SIZE]; > + __u8 buffer[CN_PROC_MSG_SIZE] __aligned(8); > - msg = (struct cn_msg *)buffer; > + msg = buffer_to_cn_msg(buffer); > ev = (struct proc_event *)msg->data; > memset(>event_data, 0, sizeof(ev->event_data)); Why is memset(buffer,0,CN_PROC_MSG_SIZE) not acceptable? -- Pete -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] connector: improved unaligned access error fix
On Thu, 14 Nov 2013 12:09:21 -0500 Chris Metcalf cmetc...@tilera.com wrote: - __u8 buffer[CN_PROC_MSG_SIZE]; + __u8 buffer[CN_PROC_MSG_SIZE] __aligned(8); - msg = (struct cn_msg *)buffer; + msg = buffer_to_cn_msg(buffer); ev = (struct proc_event *)msg-data; memset(ev-event_data, 0, sizeof(ev-event_data)); Why is memset(buffer,0,CN_PROC_MSG_SIZE) not acceptable? -- Pete -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Huawei E220 and usb storage
On Wed, 20 Feb 2008 07:57:23 +0100, Norbert Preining <[EMAIL PROTECTED]> wrote: > > that you did, after taking care of detection and initialization. > > Look at his dmesg in comment #44 in this: > > Yes, that looks very similar. Someone reported on the bug that a firmware update exists to resolve the issue (version 11.117.07.00.67). Probably they started to comply with the spec and return 12 bytes of sense according to the allocation length in the SCSI command. https://bugzilla.redhat.com/show_bug.cgi?id=253096#c51 -- 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: Huawei E220 and usb storage
On Wed, 20 Feb 2008 07:57:23 +0100, Norbert Preining [EMAIL PROTECTED] wrote: that you did, after taking care of detection and initialization. Look at his dmesg in comment #44 in this: Yes, that looks very similar. Someone reported on the bug that a firmware update exists to resolve the issue (version 11.117.07.00.67). Probably they started to comply with the spec and return 12 bytes of sense according to the allocation length in the SCSI command. https://bugzilla.redhat.com/show_bug.cgi?id=253096#c51 -- 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: Huawei E220 and usb storage
On Fri, 16 Nov 2007 14:22:56 +0100, Norbert Preining <[EMAIL PROTECTED]> wrote: > > > The difference with huaweiAktBbo.c seems that kernel uses a nonzero > > > length. > > > Can you try zero length with the kernel? It's the second argument to the > > > last. > > > > I tried with the git patch plus changing the penultimage argument from > > 0x1 to 0. > > Ok, did new tests with 2.6.24-rc2: > - with plain kernel the usb-storage modules attaches and detaches > permanently a virtual cd drive, I stopped after 30+ iterations. It looks like between Dave Russel and I, we hit the same problem that you did, after taking care of detection and initialization. Look at his dmesg in comment #44 in this: https://bugzilla.redhat.com/show_bug.cgi?id=253096#c44 > - changing the penultimage argument in the usb_stor_huawei_e220_init > function from 0x1 to 0 stopped this misbehaviour, but > > - with the change from 0x1 to 0 the initialization works automatically. I may be able to test this. As you recall, Huawei people themselves suggested nonzero length, this is why we didn't want to change it. But perhaps they are mistaken about the operation of their own hardware. Stranger things happened... -- 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: Huawei E220 and usb storage
On Fri, 16 Nov 2007 14:22:56 +0100, Norbert Preining [EMAIL PROTECTED] wrote: The difference with huaweiAktBbo.c seems that kernel uses a nonzero length. Can you try zero length with the kernel? It's the second argument to the last. I tried with the git patch plus changing the penultimage argument from 0x1 to 0. Ok, did new tests with 2.6.24-rc2: - with plain kernel the usb-storage modules attaches and detaches permanently a virtual cd drive, I stopped after 30+ iterations. It looks like between Dave Russel and I, we hit the same problem that you did, after taking care of detection and initialization. Look at his dmesg in comment #44 in this: https://bugzilla.redhat.com/show_bug.cgi?id=253096#c44 - changing the penultimage argument in the usb_stor_huawei_e220_init function from 0x1 to 0 stopped this misbehaviour, but - with the change from 0x1 to 0 the initialization works automatically. I may be able to test this. As you recall, Huawei people themselves suggested nonzero length, this is why we didn't want to change it. But perhaps they are mistaken about the operation of their own hardware. Stranger things happened... -- 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: [2.6.24 REGRESSION] BUG: Soft lockup - with VFS
On Tue, 12 Feb 2008 10:46:12 +0900, FUJITA Tomonori <[EMAIL PROTECTED]> wrote: > On a serious note, it seems that two scatter lists per request leaded > to this bug. Can the scatter list in struct ub_request be removed? Good question. It's an eyesore to be sure. The duplication exists for the sake of retries combined with the separation of requests from commands. Please bear with me, if you're curious: commands can be launched without requests (at probe time, for instance, or when sense is requested). So, they need an s/g table. But then, the lifetime of a request is greater than than of a command, in case of a retry especially. Therefore a request needs the s/g table too. So, one way to kill this duplication is to mandate that a request existed for every command. It seemed like way more code than just one memcpy() when I wrote it. Another way would be to make commands flexible, e.g. sometimes with just a virtual address and size, sometimes with an s/g table. If you guys make struct scatterlist illegal to copy with memcpy one day, this is probably what I'll do. -- 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: [2.6.24 REGRESSION] BUG: Soft lockup - with VFS
On Tue, 12 Feb 2008 10:46:12 +0900, FUJITA Tomonori [EMAIL PROTECTED] wrote: On a serious note, it seems that two scatter lists per request leaded to this bug. Can the scatter list in struct ub_request be removed? Good question. It's an eyesore to be sure. The duplication exists for the sake of retries combined with the separation of requests from commands. Please bear with me, if you're curious: commands can be launched without requests (at probe time, for instance, or when sense is requested). So, they need an s/g table. But then, the lifetime of a request is greater than than of a command, in case of a retry especially. Therefore a request needs the s/g table too. So, one way to kill this duplication is to mandate that a request existed for every command. It seemed like way more code than just one memcpy() when I wrote it. Another way would be to make commands flexible, e.g. sometimes with just a virtual address and size, sometimes with an s/g table. If you guys make struct scatterlist illegal to copy with memcpy one day, this is probably what I'll do. -- 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: [2.6.24 REGRESSION] BUG: Soft lockup - with VFS
On Tue, 5 Feb 2008 14:05:06 -0800, Andrew Morton <[EMAIL PROTECTED]> wrote:
> > > http://students.zipernowsky.hu/~oliverp/kernel/regression_2624/
> I think ub.c is basically abandoned in favour of usb-storage.
> If so, perhaps we should remove or disble ub.c?
Looks like it's just Tomo or Jens made a mistake when converting to
the new s/g API. Nothing to be too concerned about. I know I should've
reviewed their patch closer, but it seemed too simple...
-- Pete
Fix up the conversion to sg_init_table().
Signed-off-by: Pete Zaitcev <[EMAIL PROTECTED]>
--- a/drivers/block/ub.c
+++ b/drivers/block/ub.c
@@ -657,7 +657,6 @@ static int ub_request_fn_1(struct ub_lun *lun, struct
request *rq)
if ((cmd = ub_get_cmd(lun)) == NULL)
return -1;
memset(cmd, 0, sizeof(struct ub_scsi_cmd));
- sg_init_table(cmd->sgv, UB_MAX_REQ_SG);
blkdev_dequeue_request(rq);
@@ -668,6 +667,7 @@ static int ub_request_fn_1(struct ub_lun *lun, struct
request *rq)
/*
* get scatterlist from block layer
*/
+ sg_init_table(>sgv[0], UB_MAX_REQ_SG);
n_elem = blk_rq_map_sg(lun->disk->queue, rq, >sgv[0]);
if (n_elem < 0) {
/* Impossible, because blk_rq_map_sg should not hit ENOMEM. */
--
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: [2.6.24 REGRESSION] BUG: Soft lockup - with VFS
On Tue, 5 Feb 2008 14:05:06 -0800, Andrew Morton [EMAIL PROTECTED] wrote:
http://students.zipernowsky.hu/~oliverp/kernel/regression_2624/
I think ub.c is basically abandoned in favour of usb-storage.
If so, perhaps we should remove or disble ub.c?
Looks like it's just Tomo or Jens made a mistake when converting to
the new s/g API. Nothing to be too concerned about. I know I should've
reviewed their patch closer, but it seemed too simple...
-- Pete
Fix up the conversion to sg_init_table().
Signed-off-by: Pete Zaitcev [EMAIL PROTECTED]
--- a/drivers/block/ub.c
+++ b/drivers/block/ub.c
@@ -657,7 +657,6 @@ static int ub_request_fn_1(struct ub_lun *lun, struct
request *rq)
if ((cmd = ub_get_cmd(lun)) == NULL)
return -1;
memset(cmd, 0, sizeof(struct ub_scsi_cmd));
- sg_init_table(cmd-sgv, UB_MAX_REQ_SG);
blkdev_dequeue_request(rq);
@@ -668,6 +667,7 @@ static int ub_request_fn_1(struct ub_lun *lun, struct
request *rq)
/*
* get scatterlist from block layer
*/
+ sg_init_table(urq-sgv[0], UB_MAX_REQ_SG);
n_elem = blk_rq_map_sg(lun-disk-queue, rq, urq-sgv[0]);
if (n_elem 0) {
/* Impossible, because blk_rq_map_sg should not hit ENOMEM. */
--
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: [2.6.24 REGRESSION] BUG: Soft lockup - with VFS
On Tue, 5 Feb 2008 14:05:06 -0800, Andrew Morton <[EMAIL PROTECTED]> wrote: > Looks like you deadlocked in ub_request_fn(). I assume that you were using > ub.c in 2.6.23 and that it worked OK? If so, we broke it, possibly via > changes to the core block layer. > > I think ub.c is basically abandoned in favour of usb-storage. If so, > perhaps we should remove or disble ub.c? Actually I think it may be an argument for keeping ub, if ub exposes a bug in the __blk_end_request. I'll look at the head of the thread and see if Mr. Pinter has hit anything related to Mr. Ueda's work. -- 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: [2.6.24 REGRESSION] BUG: Soft lockup - with VFS
On Tue, 5 Feb 2008 14:05:06 -0800, Andrew Morton [EMAIL PROTECTED] wrote: Looks like you deadlocked in ub_request_fn(). I assume that you were using ub.c in 2.6.23 and that it worked OK? If so, we broke it, possibly via changes to the core block layer. I think ub.c is basically abandoned in favour of usb-storage. If so, perhaps we should remove or disble ub.c? Actually I think it may be an argument for keeping ub, if ub exposes a bug in the __blk_end_request. I'll look at the head of the thread and see if Mr. Pinter has hit anything related to Mr. Ueda's work. -- 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: [PATCH 4/4] usb: libusual: locking cleanup
On Sun, 23 Dec 2007 08:46:37 -0800, Daniel Walker <[EMAIL PROTECTED]> wrote: > I noticed you also have a spinlock held in usu_probe_thread(), the > usu_lock.. That spinlock would preclude anything inside request_module() > from sleeping.. The usu_lock is not held across request_module. In fact, it can be easily taken from inside request_module, when it invokes modprobe. Stop scaring me :-) > One thing that has bothered me is that I don't see a reason why this > couldn't become a complete, yet you have a comment which says that it > can't be a complete.. I honestly didn't understand the comment.. I would > imagine that you tried a complete , and it didn't work? Yes, it was a completition initially. But suppose you have two storage devices, plugged in across a reboot. Two threads are created and have to wait until the libusual's init function ends. Since we post one completion, only one thread continues. -- 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: [PATCH 4/4] usb: libusual: locking cleanup
On Sun, 23 Dec 2007 08:46:37 -0800, Daniel Walker [EMAIL PROTECTED] wrote: I noticed you also have a spinlock held in usu_probe_thread(), the usu_lock.. That spinlock would preclude anything inside request_module() from sleeping.. The usu_lock is not held across request_module. In fact, it can be easily taken from inside request_module, when it invokes modprobe. Stop scaring me :-) One thing that has bothered me is that I don't see a reason why this couldn't become a complete, yet you have a comment which says that it can't be a complete.. I honestly didn't understand the comment.. I would imagine that you tried a complete , and it didn't work? Yes, it was a completition initially. But suppose you have two storage devices, plugged in across a reboot. Two threads are created and have to wait until the libusual's init function ends. Since we post one completion, only one thread continues. -- 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: [PATCH 4/4] usb: libusual: locking cleanup
On Sat, 22 Dec 2007 09:01:50 -0800, Daniel Walker <[EMAIL PROTECTED]> wrote: > Then in usu_probe_thread() your basically stopping it at the start of > the function with a down(), and the up() is just ancillary .. So you > could easily move the up() further down in the function and still have > the same level of exclusion.. The unfortunate complication here is request_module. I didn't want to keep a semaphore locked across it, in case child waits for something. I wonder if there may be some deadlock that we cannot foresee. But I guess it won't hurt to try. I tested the patch and it seems to work ok. -- 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: [PATCH 4/4] usb: libusual: locking cleanup
On Sat, 22 Dec 2007 09:01:50 -0800, Daniel Walker [EMAIL PROTECTED] wrote: Then in usu_probe_thread() your basically stopping it at the start of the function with a down(), and the up() is just ancillary .. So you could easily move the up() further down in the function and still have the same level of exclusion.. The unfortunate complication here is request_module. I didn't want to keep a semaphore locked across it, in case child waits for something. I wonder if there may be some deadlock that we cannot foresee. But I guess it won't hurt to try. I tested the patch and it seems to work ok. -- 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: [PATCH 4/4] usb: libusual: locking cleanup
On Fri, 21 Dec 2007 00:00:04 -0800, Daniel Walker <[EMAIL PROTECTED]> wrote: > I converted the usu_init_notify semaphore to normal mutex usage, and it > should still prevent the request_module before the init routine is > complete. Before it acted more like a complete, now the mutex protects > two distinct section from running at the same time.. Let's see. > @@ -178,10 +179,7 @@ static int usu_probe_thread(void *arg) > int rc; > unsigned long flags; > > - /* A completion does not work here because it's counted. */ > - down(_init_notify); > - up(_init_notify); > - > + mutex_lock(_probe_mutex); > rc = request_module(bias_names[type]); When I tried it, usb-storage would not load with unresolved symbols. It happens if child (usu_probe_thread) runs ahead of its parent (usb_usual_init -> usb_register -> usu_probe). It's entirely possible, depending on your scheduler. I hate this down-up trick too, so if you have a better idea, I'm all ears. -- 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: [PATCH 4/4] usb: libusual: locking cleanup
On Fri, 21 Dec 2007 00:00:04 -0800, Daniel Walker [EMAIL PROTECTED] wrote: I converted the usu_init_notify semaphore to normal mutex usage, and it should still prevent the request_module before the init routine is complete. Before it acted more like a complete, now the mutex protects two distinct section from running at the same time.. Let's see. @@ -178,10 +179,7 @@ static int usu_probe_thread(void *arg) int rc; unsigned long flags; - /* A completion does not work here because it's counted. */ - down(usu_init_notify); - up(usu_init_notify); - + mutex_lock(usu_probe_mutex); rc = request_module(bias_names[type]); When I tried it, usb-storage would not load with unresolved symbols. It happens if child (usu_probe_thread) runs ahead of its parent (usb_usual_init - usb_register - usu_probe). It's entirely possible, depending on your scheduler. I hate this down-up trick too, so if you have a better idea, I'm all ears. -- 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: [PATCH 12/30] blk_end_request: changing ub (take 4)
On Fri, 14 Dec 2007 12:04:54 -0500 (EST), Kiyoshi Ueda <[EMAIL PROTECTED]> wrote: > I have investigated all code paths which call ub_end_rq() in ub.c, > and confirmed that ub_end_rq() is always called with the queue lock > held. (sc->lock is registered as a queue lock.) Thanks for reminding me about blk_init_queue, I forgot. Sorry for the confusion. Greetings, -- 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: [PATCH 12/30] blk_end_request: changing ub (take 4)
On Fri, 14 Dec 2007 12:04:54 -0500 (EST), Kiyoshi Ueda [EMAIL PROTECTED] wrote: I have investigated all code paths which call ub_end_rq() in ub.c, and confirmed that ub_end_rq() is always called with the queue lock held. (sc-lock is registered as a queue lock.) Thanks for reminding me about blk_init_queue, I forgot. Sorry for the confusion. Greetings, -- 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: [PATCH 12/30] blk_end_request: changing ub (take 4)
On Wed, 12 Dec 2007 15:38:15 -0500 (EST), Kiyoshi Ueda <[EMAIL PROTECTED]> wrote: > On Tue, 11 Dec 2007 15:48:03 -0800, Pete Zaitcev <[EMAIL PROTECTED]> wrote: > > > - end_that_request_first(rq, uptodate, rq->hard_nr_sectors); > > > - end_that_request_last(rq, uptodate); > > > + if (__blk_end_request(rq, error, blk_rq_bytes(rq))) > > > + BUG(); > > My understanding was, blk_end_request() is the same thing, only > > takes the queue lock. But then, should I refactor ub so that it > > calls __blk_end_request if request function ends with an error > > and blk_end_request if the end-of-IO even is processed? > I'm using __blk_end_request() here and I think it's sufficient, because: > o end_that_request_last() must be called with the queue lock held > o ub_end_rq() calls end_that_request_last() without taking > the queue lock in itself. > So the queue lock must have been taken outside ub_end_rq(). > But, if ub is calling end_that_request_last() without the queue lock, > it is a bug in the original code and we should use blk_end_request() > to fix that. So, I have to rewrite ub to split the paths after all, right? Let's do this then: I'll wait until your patch gets to Linus and then update it with the split. The reason is, I need the whole enchilada applied and I don't want to bother tracking iterations and all the little segments (of which you already have 30). Until then, ub will have a race by using your original small patch. Best wishes, -- 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: [PATCH 12/30] blk_end_request: changing ub (take 4)
On Wed, 12 Dec 2007 15:38:15 -0500 (EST), Kiyoshi Ueda [EMAIL PROTECTED] wrote: On Tue, 11 Dec 2007 15:48:03 -0800, Pete Zaitcev [EMAIL PROTECTED] wrote: - end_that_request_first(rq, uptodate, rq-hard_nr_sectors); - end_that_request_last(rq, uptodate); + if (__blk_end_request(rq, error, blk_rq_bytes(rq))) + BUG(); My understanding was, blk_end_request() is the same thing, only takes the queue lock. But then, should I refactor ub so that it calls __blk_end_request if request function ends with an error and blk_end_request if the end-of-IO even is processed? I'm using __blk_end_request() here and I think it's sufficient, because: o end_that_request_last() must be called with the queue lock held o ub_end_rq() calls end_that_request_last() without taking the queue lock in itself. So the queue lock must have been taken outside ub_end_rq(). But, if ub is calling end_that_request_last() without the queue lock, it is a bug in the original code and we should use blk_end_request() to fix that. So, I have to rewrite ub to split the paths after all, right? Let's do this then: I'll wait until your patch gets to Linus and then update it with the split. The reason is, I need the whole enchilada applied and I don't want to bother tracking iterations and all the little segments (of which you already have 30). Until then, ub will have a race by using your original small patch. Best wishes, -- 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: [PATCH 12/30] blk_end_request: changing ub (take 4)
On Tue, 11 Dec 2007 17:46:47 -0500 (EST), Kiyoshi Ueda <[EMAIL PROTECTED]>
wrote:
> if (scsi_status == 0) {
> - uptodate = 1;
> + error = 0;
> } else {
> - uptodate = 0;
> + error = -EIO;
> rq->errors = scsi_status;
> }
> - end_that_request_first(rq, uptodate, rq->hard_nr_sectors);
> - end_that_request_last(rq, uptodate);
> + if (__blk_end_request(rq, error, blk_rq_bytes(rq)))
> + BUG();
Acked-by: Pete Zaitcev <[EMAIL PROTECTED]>
I follow the discussion, actually, and wanted to ask someone to look
closer if it's appropriate to use __blk_end_request() here.
My understanding was, blk_end_request() is the same thing, only
takes the queue lock. But then, should I refactor ub so that it
calls __blk_end_request if request function ends with an error
and blk_end_request if the end-of-IO even is processed? If not,
and the above is sufficient, why have blk_end_request at all?
-- 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: [PATCH 12/30] blk_end_request: changing ub (take 4)
On Tue, 11 Dec 2007 17:46:47 -0500 (EST), Kiyoshi Ueda [EMAIL PROTECTED]
wrote:
if (scsi_status == 0) {
- uptodate = 1;
+ error = 0;
} else {
- uptodate = 0;
+ error = -EIO;
rq-errors = scsi_status;
}
- end_that_request_first(rq, uptodate, rq-hard_nr_sectors);
- end_that_request_last(rq, uptodate);
+ if (__blk_end_request(rq, error, blk_rq_bytes(rq)))
+ BUG();
Acked-by: Pete Zaitcev [EMAIL PROTECTED]
I follow the discussion, actually, and wanted to ask someone to look
closer if it's appropriate to use __blk_end_request() here.
My understanding was, blk_end_request() is the same thing, only
takes the queue lock. But then, should I refactor ub so that it
calls __blk_end_request if request function ends with an error
and blk_end_request if the end-of-IO even is processed? If not,
and the above is sufficient, why have blk_end_request at all?
-- 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: [PATCH] USB: option: Bind to the correct interface of the Huawei E220
On Wed, 5 Dec 2007 19:23:14 +0100, Oliver Neukum <[EMAIL PROTECTED]> wrote: > Am Mittwoch, 5. Dezember 2007 17:34:23 schrieb Pete Zaitcev: > > Looks good to me, shorter than my patch, has no duplication, allows to > > use the storage, looks like a winner. Unfortunately, it leaves ub for dead, > > Is that new? How could ub do this up to now? No, it's not a regression. A user-mode initializer was always required to use E220 with ub. So, the patch is good. -- 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: [patch 12/18] usb: mon nopage
On Wed, 05 Dec 2007 18:15:59 +1100, [EMAIL PROTECTED] wrote: > Convert USB mon driver from nopage to fault. > if (offset >= rp->b_size) > - return NOPAGE_SIGBUS; > + return VM_FAULT_SIGBUS; > chunk_idx = offset / CHUNK_SIZE; > pageptr = rp->b_vec[chunk_idx].pg; > get_page(pageptr); > - if (type) > - *type = VM_FAULT_MINOR; > - return pageptr; > + vmf->page = pageptr; > + return 0; Looks like a trivial change, I ack this. It's a rarely used API, I have to run tests to see how it works. I'll collect any failing pieces later if any. Signed-off-by: Pete Zaitcev <[EMAIL PROTECTED]> -- 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: [PATCH] USB: option: Bind to the correct interface of the Huawei E220
On Fri, 30 Nov 2007 16:30:11 +, Jaime Velasco Juan <[EMAIL PROTECTED]>
wrote:
> --- a/drivers/usb/serial/option.c
> +++ b/drivers/usb/serial/option.c
> - { USB_DEVICE(HUAWEI_VENDOR_ID, HUAWEI_PRODUCT_E220) },
> - { USB_DEVICE(HUAWEI_VENDOR_ID, HUAWEI_PRODUCT_E220BIS) },
> + { USB_DEVICE_AND_INTERFACE_INFO(HUAWEI_VENDOR_ID, HUAWEI_PRODUCT_E220,
> 0xff, 0xff, 0xff) },
> + { USB_DEVICE_AND_INTERFACE_INFO(HUAWEI_VENDOR_ID,
> HUAWEI_PRODUCT_E220BIS, 0xff, 0xff, 0xff) },
Looks good to me, shorter than my patch, has no duplication, allows to
use the storage, looks like a winner. Unfortunately, it leaves ub for dead,
because ub cannot invoke the necessary initializer unless we move it to
libusual. But oh well, I'll think about it.
Took me this long to test because I had to ask kind people in England
to replug the modem.
Signed-off-by: Pete Zaitcev <[EMAIL PROTECTED]>
-- 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: [patch 12/18] usb: mon nopage
On Wed, 05 Dec 2007 18:15:59 +1100, [EMAIL PROTECTED] wrote: Convert USB mon driver from nopage to fault. if (offset = rp-b_size) - return NOPAGE_SIGBUS; + return VM_FAULT_SIGBUS; chunk_idx = offset / CHUNK_SIZE; pageptr = rp-b_vec[chunk_idx].pg; get_page(pageptr); - if (type) - *type = VM_FAULT_MINOR; - return pageptr; + vmf-page = pageptr; + return 0; Looks like a trivial change, I ack this. It's a rarely used API, I have to run tests to see how it works. I'll collect any failing pieces later if any. Signed-off-by: Pete Zaitcev [EMAIL PROTECTED] -- 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: [PATCH] USB: option: Bind to the correct interface of the Huawei E220
On Wed, 5 Dec 2007 19:23:14 +0100, Oliver Neukum [EMAIL PROTECTED] wrote: Am Mittwoch, 5. Dezember 2007 17:34:23 schrieb Pete Zaitcev: Looks good to me, shorter than my patch, has no duplication, allows to use the storage, looks like a winner. Unfortunately, it leaves ub for dead, Is that new? How could ub do this up to now? No, it's not a regression. A user-mode initializer was always required to use E220 with ub. So, the patch is good. -- 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: [PATCH] USB: option: Bind to the correct interface of the Huawei E220
On Fri, 30 Nov 2007 16:30:11 +, Jaime Velasco Juan [EMAIL PROTECTED]
wrote:
--- a/drivers/usb/serial/option.c
+++ b/drivers/usb/serial/option.c
- { USB_DEVICE(HUAWEI_VENDOR_ID, HUAWEI_PRODUCT_E220) },
- { USB_DEVICE(HUAWEI_VENDOR_ID, HUAWEI_PRODUCT_E220BIS) },
+ { USB_DEVICE_AND_INTERFACE_INFO(HUAWEI_VENDOR_ID, HUAWEI_PRODUCT_E220,
0xff, 0xff, 0xff) },
+ { USB_DEVICE_AND_INTERFACE_INFO(HUAWEI_VENDOR_ID,
HUAWEI_PRODUCT_E220BIS, 0xff, 0xff, 0xff) },
Looks good to me, shorter than my patch, has no duplication, allows to
use the storage, looks like a winner. Unfortunately, it leaves ub for dead,
because ub cannot invoke the necessary initializer unless we move it to
libusual. But oh well, I'll think about it.
Took me this long to test because I had to ask kind people in England
to replug the modem.
Signed-off-by: Pete Zaitcev [EMAIL PROTECTED]
-- 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: [PATCH] USB: option: Bind to the correct interface of the Huawei E220
On Sat, 1 Dec 2007 09:07:38 +0100, Norbert Preining <[EMAIL PROTECTED]> wrote: > is this the only addition that should be needed, ortogether with the > changes in option to call the huawei init function? The only one. > I tried 2.6.24-rc3 with this patch only and it I again got the infinite > loop of connect/disconnect events instantiating cdroms. Your problem is something else. Neither my patch nor Jaime's patch address it. Honestly, I'm not even sure how to tackle it. I seem to recall that I had a usbmon trace from you but I'm unable to find it now. Gettin it (again?) probably would be a good place to restart that investigation. -- 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: [PATCH] USB: option: Bind to the correct interface of the Huawei E220
On Sat, 1 Dec 2007 09:07:38 +0100, Norbert Preining [EMAIL PROTECTED] wrote: is this the only addition that should be needed, ortogether with the changes in option to call the huawei init function? The only one. I tried 2.6.24-rc3 with this patch only and it I again got the infinite loop of connect/disconnect events instantiating cdroms. Your problem is something else. Neither my patch nor Jaime's patch address it. Honestly, I'm not even sure how to tackle it. I seem to recall that I had a usbmon trace from you but I'm unable to find it now. Gettin it (again?) probably would be a good place to restart that investigation. -- 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: Add the infamous Huawei E220 to option.c
On Thu, 29 Nov 2007 09:04:28 +0100, Oliver Neukum <[EMAIL PROTECTED]> wrote: > > > Isn't it possible to fix this in option's module table? > > > > At first thought it'll need adding a field to struct usb_serial to save > > the driver_info from the ID table in usb_serial_probe. It's something I'd > > Why? It is passed to the subdrivers in their probe(). > Maybe it should simply be passed in attach(), too? My bad, you're right. I'm just mentally dead now. -- 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: Add the infamous Huawei E220 to option.c
On Thu, 29 Nov 2007 08:44:38 +0100, Oliver Neukum <[EMAIL PROTECTED]> wrote: > 3. Make sure usbcore doesn't probe the devices in the wrong mode with the > option driver This fixes duplication. And to take it further, why don't we turn this idea around and let usb-storage fail to attach with some "ignore" quirk? I suspect that someone put the initializer into usb-storage in order to let the generic usb serial to work, but this simply was a bad idea. -- 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: Add the infamous Huawei E220 to option.c
On Thu, 29 Nov 2007 09:04:28 +0100, Oliver Neukum [EMAIL PROTECTED] wrote: Isn't it possible to fix this in option's module table? At first thought it'll need adding a field to struct usb_serial to save the driver_info from the ID table in usb_serial_probe. It's something I'd Why? It is passed to the subdrivers in their probe(). Maybe it should simply be passed in attach(), too? My bad, you're right. I'm just mentally dead now. -- 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: Add the infamous Huawei E220 to option.c
On Thu, 29 Nov 2007 08:44:38 +0100, Oliver Neukum [EMAIL PROTECTED] wrote: 3. Make sure usbcore doesn't probe the devices in the wrong mode with the option driver This fixes duplication. And to take it further, why don't we turn this idea around and let usb-storage fail to attach with some ignore quirk? I suspect that someone put the initializer into usb-storage in order to let the generic usb serial to work, but this simply was a bad idea. -- 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: Add the infamous Huawei E220 to option.c
On Thu, 29 Nov 2007 08:38:59 +0100, Oliver Neukum <[EMAIL PROTECTED]> wrote: > Am Donnerstag, 29. November 2007 01:13:05 schrieb Pete Zaitcev: > > The problem stems from the fact that both option and usb-storage can bind > > to the modem when in storage mode: the former binds because of the storage > > class, the latter binds because of VID/PID match. The modprobe loads both, > > Isn't it possible to fix this in option's module table? At first thought it'll need adding a field to struct usb_serial to save the driver_info from the ID table in usb_serial_probe. It's something I'd like to discuss actually. I hate fields which store information this way: filled in one place, used in another place... From the perspective of code prettiness I would rather add another method for usb_serial_probe to call. But I'm not sure really. -- 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/
Add the infamous Huawei E220 to option.c
Hi, All:
It looks like the Huawei E220 saga is not over yet. A collegue of mine,
David Russll, reported that the modem does not work reliably on Fedora 8,
which does have the initializer in usb-storage.
The problem stems from the fact that both option and usb-storage can bind
to the modem when in storage mode: the former binds because of the storage
class, the latter binds because of VID/PID match. The modprobe loads both,
it's random which wins. If usb-storage wins, everything is fine. If option
wins, it binds to modem still in storage mode and does not work.
I propose we add the same initializer that usb-storage has to the option.
This way no matter which driver wins the modem gets initialized. The
patch is tested on David's modem, but I would like someone give it more
testing.
I dunno, do we want some kind of code sharing between storage and option?
They both could use the normal usb_control_msg, I think.
Also, from archives it looks like Johann may need PID 0x1004 added.
Since we're on topic, David's modem has exactly same IDs as Norbert's,
but works fine with the length of 1. Although it's possible that the
firmware is different without different firmware reported in USB desc-
riptors. Does anyone know a magic AT command? ATI or something?
Norbert, please try my patch, maybe it'll work this time.
And finally, pleas stop using that script from the polish website and
above all quit using the generic serial subdriver. The option must
work now with the patch. Please let me know if it fails.
Thanks in advance,
-- Pete
diff -urp -X dontdiff linux-2.6.23.1-42.fc8/drivers/usb/serial/option.c
linux-2.6.23.1-42.fc8.e220.1/drivers/usb/serial/option.c
--- linux-2.6.23.1-42.fc8/drivers/usb/serial/option.c 2007-10-09
13:31:38.0 -0700
+++ linux-2.6.23.1-42.fc8.e220.1/drivers/usb/serial/option.c2007-11-27
21:36:11.0 -0800
@@ -448,7 +448,7 @@ static void option_indat_callback(struct
err = usb_submit_urb(urb, GFP_ATOMIC);
if (err)
printk(KERN_ERR "%s: resubmit read urb failed. "
- "(%d)", __FUNCTION__, err);
+ "(%d)\n", __FUNCTION__, err);
}
}
return;
@@ -728,6 +728,35 @@ static int option_send_setup(struct usb_
return 0;
}
+static void option_start_huawei(struct usb_serial *serial)
+{
+ struct usb_device *dev = serial->dev;
+ char *buf;
+ int rc;
+
+ if (!(le16_to_cpu(dev->descriptor.idVendor) == HUAWEI_VENDOR_ID &&
+ le16_to_cpu(dev->descriptor.idProduct) == HUAWEI_PRODUCT_E220))
+ return;
+
+ if ((buf = kmalloc(1, GFP_KERNEL)) == 0)
+ goto err_buf;
+
+ buf[0] = 0x1;
+ rc = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
+ USB_REQ_SET_FEATURE, USB_TYPE_STANDARD | USB_RECIP_DEVICE,
+ 0x01, 0x0, buf, 1, 1000);
+ if (rc) {
+ printk(KERN_ERR "%s: HUAWEI E220 setup failed (%d)\n",
+ __FUNCTION__, rc);
+ }
+
+ kfree(buf);
+ return;
+
+err_buf:
+ ;
+}
+
static int option_startup(struct usb_serial *serial)
{
int i, err;
@@ -736,6 +765,8 @@ static int option_startup(struct usb_ser
dbg("%s", __FUNCTION__);
+ option_start_huawei(serial);
+
/* Now setup per port private data */
for (i = 0; i < serial->num_ports; i++) {
port = serial->port[i];
-
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/
Add the infamous Huawei E220 to option.c
Hi, All:
It looks like the Huawei E220 saga is not over yet. A collegue of mine,
David Russll, reported that the modem does not work reliably on Fedora 8,
which does have the initializer in usb-storage.
The problem stems from the fact that both option and usb-storage can bind
to the modem when in storage mode: the former binds because of the storage
class, the latter binds because of VID/PID match. The modprobe loads both,
it's random which wins. If usb-storage wins, everything is fine. If option
wins, it binds to modem still in storage mode and does not work.
I propose we add the same initializer that usb-storage has to the option.
This way no matter which driver wins the modem gets initialized. The
patch is tested on David's modem, but I would like someone give it more
testing.
I dunno, do we want some kind of code sharing between storage and option?
They both could use the normal usb_control_msg, I think.
Also, from archives it looks like Johann may need PID 0x1004 added.
Since we're on topic, David's modem has exactly same IDs as Norbert's,
but works fine with the length of 1. Although it's possible that the
firmware is different without different firmware reported in USB desc-
riptors. Does anyone know a magic AT command? ATI or something?
Norbert, please try my patch, maybe it'll work this time.
And finally, pleas stop using that script from the polish website and
above all quit using the generic serial subdriver. The option must
work now with the patch. Please let me know if it fails.
Thanks in advance,
-- Pete
diff -urp -X dontdiff linux-2.6.23.1-42.fc8/drivers/usb/serial/option.c
linux-2.6.23.1-42.fc8.e220.1/drivers/usb/serial/option.c
--- linux-2.6.23.1-42.fc8/drivers/usb/serial/option.c 2007-10-09
13:31:38.0 -0700
+++ linux-2.6.23.1-42.fc8.e220.1/drivers/usb/serial/option.c2007-11-27
21:36:11.0 -0800
@@ -448,7 +448,7 @@ static void option_indat_callback(struct
err = usb_submit_urb(urb, GFP_ATOMIC);
if (err)
printk(KERN_ERR %s: resubmit read urb failed.
- (%d), __FUNCTION__, err);
+ (%d)\n, __FUNCTION__, err);
}
}
return;
@@ -728,6 +728,35 @@ static int option_send_setup(struct usb_
return 0;
}
+static void option_start_huawei(struct usb_serial *serial)
+{
+ struct usb_device *dev = serial-dev;
+ char *buf;
+ int rc;
+
+ if (!(le16_to_cpu(dev-descriptor.idVendor) == HUAWEI_VENDOR_ID
+ le16_to_cpu(dev-descriptor.idProduct) == HUAWEI_PRODUCT_E220))
+ return;
+
+ if ((buf = kmalloc(1, GFP_KERNEL)) == 0)
+ goto err_buf;
+
+ buf[0] = 0x1;
+ rc = usb_control_msg(dev, usb_sndctrlpipe(dev, 0),
+ USB_REQ_SET_FEATURE, USB_TYPE_STANDARD | USB_RECIP_DEVICE,
+ 0x01, 0x0, buf, 1, 1000);
+ if (rc) {
+ printk(KERN_ERR %s: HUAWEI E220 setup failed (%d)\n,
+ __FUNCTION__, rc);
+ }
+
+ kfree(buf);
+ return;
+
+err_buf:
+ ;
+}
+
static int option_startup(struct usb_serial *serial)
{
int i, err;
@@ -736,6 +765,8 @@ static int option_startup(struct usb_ser
dbg(%s, __FUNCTION__);
+ option_start_huawei(serial);
+
/* Now setup per port private data */
for (i = 0; i serial-num_ports; i++) {
port = serial-port[i];
-
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: Add the infamous Huawei E220 to option.c
On Thu, 29 Nov 2007 08:38:59 +0100, Oliver Neukum [EMAIL PROTECTED] wrote: Am Donnerstag, 29. November 2007 01:13:05 schrieb Pete Zaitcev: The problem stems from the fact that both option and usb-storage can bind to the modem when in storage mode: the former binds because of the storage class, the latter binds because of VID/PID match. The modprobe loads both, Isn't it possible to fix this in option's module table? At first thought it'll need adding a field to struct usb_serial to save the driver_info from the ID table in usb_serial_probe. It's something I'd like to discuss actually. I hate fields which store information this way: filled in one place, used in another place... From the perspective of code prettiness I would rather add another method for usb_serial_probe to call. But I'm not sure really. -- 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: [PATCH] mct232: speed, new termios and compliance cleanups
On Mon, 19 Nov 2007 15:22:11 +, Alan Cox <[EMAIL PROTECTED]> wrote:
> Signed-off-by: Alan Cox <[EMAIL PROTECTED]>
This looks good, but have a couple of comments. Maybe I can fix it up.
> * we do not know how to support. We ignore them for the moment.
> * XXX Rate-limit the error message, it's user triggerable.
This XXX item is fixed by this patch, so should be removed.
> + /* FIXME: Can we use any divider - should we do
> +divider = 115200/value;
> +real baud = 115200/divider */
> switch (value) {
> case 300: break;
I do not understand this FIXME comment. What is wrong with current
code? The result of the function is given to the device's sequencer.
Do you want to get rid of the switch? Please unconfuse me.
-- 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: [PATCH] mct232: speed, new termios and compliance cleanups
On Mon, 19 Nov 2007 15:22:11 +, Alan Cox [EMAIL PROTECTED] wrote:
Signed-off-by: Alan Cox [EMAIL PROTECTED]
This looks good, but have a couple of comments. Maybe I can fix it up.
* we do not know how to support. We ignore them for the moment.
* XXX Rate-limit the error message, it's user triggerable.
This XXX item is fixed by this patch, so should be removed.
+ /* FIXME: Can we use any divider - should we do
+divider = 115200/value;
+real baud = 115200/divider */
switch (value) {
case 300: break;
I do not understand this FIXME comment. What is wrong with current
code? The result of the function is given to the device's sequencer.
Do you want to get rid of the switch? Please unconfuse me.
-- 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, 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, 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;
-
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 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: Huawei E220 and usb storage
On Wed, 31 Oct 2007 07:23:56 +0100, Norbert Preining <[EMAIL PROTECTED]> wrote: > Hmm, in addition I added a > printk(KERN_ERR "ENTERING usb_stor_huawei_e220_init!\n"); > at the beginning of the function but it never showed up in my log files. > So it seems that the UNUSUAL_DEV entry does not match. This doesn't seem to be possible, considering the /proc/bus/usb/devices that you posted. I would rather suspect that you forgot to perform some step in your kernel installation, and end using a stale usb-storage module. -- 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: Huawei E220 and usb storage
On Tue, 30 Oct 2007 22:22:13 +0100, Norbert Preining <[EMAIL PROTECTED]> wrote: > I tried the attached patch which I found on the usb list, but it didn't > work, the cdrom was still always found, and it was > connected/disconnected permanently. The difference with huaweiAktBbo.c seems that kernel uses a nonzero length. Can you try zero length with the kernel? It's the second argument to the last. -- 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: Huawei E220 and usb storage
On Tue, 30 Oct 2007 20:09:45 +0100, Norbert Preining <[EMAIL PROTECTED]> wrote: > Is this a regression from 2.6.20, or is it supposed to work? Please post your /proc/bus/usb/devices. Maybe it just fails to match. -- 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, 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, 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: Huawei E220 and usb storage
On Tue, 30 Oct 2007 20:09:45 +0100, Norbert Preining [EMAIL PROTECTED] wrote: Is this a regression from 2.6.20, or is it supposed to work? Please post your /proc/bus/usb/devices. Maybe it just fails to match. -- 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: Huawei E220 and usb storage
On Tue, 30 Oct 2007 22:22:13 +0100, Norbert Preining [EMAIL PROTECTED] wrote: I tried the attached patch which I found on the usb list, but it didn't work, the cdrom was still always found, and it was connected/disconnected permanently. The difference with huaweiAktBbo.c seems that kernel uses a nonzero length. Can you try zero length with the kernel? It's the second argument to the last. -- 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: Huawei E220 and usb storage
On Wed, 31 Oct 2007 07:23:56 +0100, Norbert Preining [EMAIL PROTECTED] wrote: Hmm, in addition I added a printk(KERN_ERR ENTERING usb_stor_huawei_e220_init!\n); at the beginning of the function but it never showed up in my log files. So it seems that the UNUSUAL_DEV entry does not match. This doesn't seem to be possible, considering the /proc/bus/usb/devices that you posted. I would rather suspect that you forgot to perform some step in your kernel installation, and end using a stale usb-storage module. -- 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 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
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
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, 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
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;
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: 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 = mutex_lock_interruptible
Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.
On Fri, 19 Oct 2007 20:40:35 +0300, Vitaliy Ivanov <[EMAIL PROTECTED]> wrote: Hi, Vitaly, I added you on cc: for the 2.6 cleanup. Please double-check what I'm doing there and use it for your 2.4 version. I hope my intentions get clearer with an example. Now, about the specific question: > Static lock minor_table_mutex is used for minor table structure. > And dev->sem for dev manipulations and that's why for open_count. > If you will simply browse /drivers/usb dir for 2.4 you will see that > such approach is widely used there. > What's not right? The fundamental reason why you cannot lock a free-able structure with an in-structure lock is this. Imagine thread A locks in order to process a disconnect. Thread B wants to open and waits for the lock. Notice that the struct is not open, so thread A frees it. At this point, thread B is using a freed memory. The solution is to lock the instance struct dev with dev->mtx, except for the open count, which is locked by a static lock (I'm ignoring interrupts here, which cannot use mutexes). I'm sorry to say, you're quite right: a number of drivers in 2.4 got it wrong, and some (like adutux) carried it through 2.6.23. -- 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
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
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
Re: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.
On Fri, 19 Oct 2007 20:40:35 +0300, Vitaliy Ivanov [EMAIL PROTECTED] wrote: Hi, Vitaly, I added you on cc: for the 2.6 cleanup. Please double-check what I'm doing there and use it for your 2.4 version. I hope my intentions get clearer with an example. Now, about the specific question: Static lock minor_table_mutex is used for minor table structure. And dev-sem for dev manipulations and that's why for open_count. If you will simply browse /drivers/usb dir for 2.4 you will see that such approach is widely used there. What's not right? The fundamental reason why you cannot lock a free-able structure with an in-structure lock is this. Imagine thread A locks in order to process a disconnect. Thread B wants to open and waits for the lock. Notice that the struct is not open, so thread A frees it. At this point, thread B is using a freed memory. The solution is to lock the instance struct dev with dev-mtx, except for the open count, which is locked by a static lock (I'm ignoring interrupts here, which cannot use mutexes). I'm sorry to say, you're quite right: a number of drivers in 2.4 got it wrong, and some (like adutux) carried it through 2.6.23. -- 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: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.
On Fri, 19 Oct 2007 18:26:35 +0300, "Vitaliy Ivanov" <[EMAIL PROTECTED]> wrote: > Didn't here anything on this? What is our final decision here? It's gotten worse, not better. Apparently, you aren't getting the concept of protecting the open count with a static lock and my explanations are just not vivid enough or something. So I decided to fix it myself. Maybe then the patch in C will explain it better than English. But I didn't have time to do it. Also, there's an outright bug in the latest version. Your purge of the wrong lock was incomplete and so there was an unbalanced up(). But this is moot. So, the version before the latest is borderline acceptable. If Willy wants to take it, it's fine. I'll fix it up later together with 2.6. -- 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: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.
On Fri, 19 Oct 2007 18:26:35 +0300, Vitaliy Ivanov [EMAIL PROTECTED] wrote: Didn't here anything on this? What is our final decision here? It's gotten worse, not better. Apparently, you aren't getting the concept of protecting the open count with a static lock and my explanations are just not vivid enough or something. So I decided to fix it myself. Maybe then the patch in C will explain it better than English. But I didn't have time to do it. Also, there's an outright bug in the latest version. Your purge of the wrong lock was incomplete and so there was an unbalanced up(). But this is moot. So, the version before the latest is borderline acceptable. If Willy wants to take it, it's fine. I'll fix it up later together with 2.6. -- 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: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.
On Tue, 16 Oct 2007 17:41:38 +0200, Willy Tarreau <[EMAIL PROTECTED]> wrote: > OK, I have no objection, but please apply the fixes the le16 problem as > suggested by Pete and Greg first. Also, you will probably receive more > comments, and/or criticisms from further reviews. This is normal and > expected. You just have to fix your code so that it can be merged. I do not object to the driver (with byte order fixed), although it seems written oddly... E.g. lots of trivial comments, the open count locked by wrong lock. But I do not want to hold the 2.4 version hostage to errors of the mainline. We should've caught them when we merged adutux into Greg's tree. I have to admit I'm skipping reviewing a lot of 2.6. -- 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: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.
On Tue, 16 Oct 2007 16:54:49 +0300, Vitaliy Ivanov <[EMAIL PROTECTED]> wrote:
> Again, comments are welcomed.
It looks like you misunderstood why a static lock protects open counts.
This is done so you do not need to worry about in-structure lock which
can be freed together with the structure. Look at this:
> +static int adu_release_internal(struct adu_device *dev)
> +{
> + /* lock this device */
> + down(>sem);
> + /* decrement our usage count for the device */
> + --dev->open_count;
> + if (dev->open_count <= 0) {
> + adu_abort_transfers(dev);
> + dev->open_count = 0;
> + }
> + /* unlock this device */
> + up(>sem);
The dev->sem is entirely unnecessary here. Every time you use
open_count, it's protected by minor_table_mutex. The name is a litte
unfortunate, feel free to rename it.
This is probably a problem in 2.6 as well. I don't know why people keep
writing these things. Someone at Ontrak needs to look into it.
-- 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: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.
On Tue, 16 Oct 2007 16:54:49 +0300, Vitaliy Ivanov [EMAIL PROTECTED] wrote:
Again, comments are welcomed.
It looks like you misunderstood why a static lock protects open counts.
This is done so you do not need to worry about in-structure lock which
can be freed together with the structure. Look at this:
+static int adu_release_internal(struct adu_device *dev)
+{
+ /* lock this device */
+ down(dev-sem);
+ /* decrement our usage count for the device */
+ --dev-open_count;
+ if (dev-open_count = 0) {
+ adu_abort_transfers(dev);
+ dev-open_count = 0;
+ }
+ /* unlock this device */
+ up(dev-sem);
The dev-sem is entirely unnecessary here. Every time you use
open_count, it's protected by minor_table_mutex. The name is a litte
unfortunate, feel free to rename it.
This is probably a problem in 2.6 as well. I don't know why people keep
writing these things. Someone at Ontrak needs to look into it.
-- 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: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.
On Tue, 16 Oct 2007 17:41:38 +0200, Willy Tarreau [EMAIL PROTECTED] wrote: OK, I have no objection, but please apply the fixes the le16 problem as suggested by Pete and Greg first. Also, you will probably receive more comments, and/or criticisms from further reviews. This is normal and expected. You just have to fix your code so that it can be merged. I do not object to the driver (with byte order fixed), although it seems written oddly... E.g. lots of trivial comments, the open count locked by wrong lock. But I do not want to hold the 2.4 version hostage to errors of the mainline. We should've caught them when we merged adutux into Greg's tree. I have to admit I'm skipping reviewing a lot of 2.6. -- 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: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.
On Sun, 14 Oct 2007 23:45:36 +0300, "Vitaliy Ivanov" <[EMAIL PROTECTED]> wrote:
> Also IMHO the more drivers are in the tree the more users will use it.
> Once it will be merged in the mainline then it will be backported to
> enterprise kernels and would gain wide usage.
At least in case of RHEL, such backports never were automatic. In any
case, RHEL 2.1 and 3 do not receive new drivers anymore. We only do
bugfixes if something comes up. Realistically speaking, 2.4 kernels
are just too old for anyone to use. So, I think it would be best for
you to think in terms of Willy's tree only.
> + in_end_size = le16_to_cpu(dev->interrupt_in_endpoint->wMaxPacketSize);
> + out_end_size = le16_to_cpu(dev->interrupt_out_endpoint->wMaxPacketSize);
Did you verify if this works? We use pre-swapped descriptors in 2.4.
I suspect you allocate 256 times more memory than necessary.
> +static void adu_delete(struct adu_device *dev)
> + kfree(dev);
> +static int adu_release_internal(struct adu_device *dev)
> + if (dev->udev == NULL) {
> + adu_delete(dev);
> +static int adu_open(struct inode *inode, struct file *file)
> + retval = adu_release_internal(dev);
> + up(>sem);
The above very clearly is a use-after-free, in case the device was
open across a disconnect. Solution: Use minor_table_mutex to lock
dev->open_count instead of dev->sem. There's no rule that the lock
has to live inside the same structure with members it locks.
-- 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: [2.4 patch] Port of adutux driver from 2.6 kernel to 2.4.
On Sun, 14 Oct 2007 23:45:36 +0300, Vitaliy Ivanov [EMAIL PROTECTED] wrote:
Also IMHO the more drivers are in the tree the more users will use it.
Once it will be merged in the mainline then it will be backported to
enterprise kernels and would gain wide usage.
At least in case of RHEL, such backports never were automatic. In any
case, RHEL 2.1 and 3 do not receive new drivers anymore. We only do
bugfixes if something comes up. Realistically speaking, 2.4 kernels
are just too old for anyone to use. So, I think it would be best for
you to think in terms of Willy's tree only.
+ in_end_size = le16_to_cpu(dev-interrupt_in_endpoint-wMaxPacketSize);
+ out_end_size = le16_to_cpu(dev-interrupt_out_endpoint-wMaxPacketSize);
Did you verify if this works? We use pre-swapped descriptors in 2.4.
I suspect you allocate 256 times more memory than necessary.
+static void adu_delete(struct adu_device *dev)
+ kfree(dev);
+static int adu_release_internal(struct adu_device *dev)
+ if (dev-udev == NULL) {
+ adu_delete(dev);
+static int adu_open(struct inode *inode, struct file *file)
+ retval = adu_release_internal(dev);
+ up(dev-sem);
The above very clearly is a use-after-free, in case the device was
open across a disconnect. Solution: Use minor_table_mutex to lock
dev-open_count instead of dev-sem. There's no rule that the lock
has to live inside the same structure with members it locks.
-- 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 subsystem merge plans
On Fri, 12 Oct 2007 14:57:28 +0200, Oliver Neukum <[EMAIL PROTECTED]> wrote: > > Should I be submitting my driver as a patch to linux.kernel or are Alan > > or Greg going to push their changes? > > Please submit your patches to Greg and the USB development list. > Greg has a directory on kernel.org where the scheduled patches are stored. Specifically, I use this: /pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-03-usb Look at README inside, it has the base kernel version. Also, use series file to apply; don't just use shell globbing, or conflicts will happen. -- 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 subsystem merge plans
On Fri, 12 Oct 2007 14:57:28 +0200, Oliver Neukum [EMAIL PROTECTED] wrote: Should I be submitting my driver as a patch to linux.kernel or are Alan or Greg going to push their changes? Please submit your patches to Greg and the USB development list. Greg has a directory on kernel.org where the scheduled patches are stored. Specifically, I use this: /pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-03-usb Look at README inside, it has the base kernel version. Also, use series file to apply; don't just use shell globbing, or conflicts will happen. -- 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: 2.6.23-rc6: usb 1-1: device not accepting address 2, error -62
On Sat, 15 Sep 2007 03:48:19 -0700, Andrew Morton <[EMAIL PROTECTED]> wrote: > > I have an error message with 2.6.23-rc6. > > This did not happen with 2.6.22. > > Another one for Michal's dirt file. No, I think it's the module ordering again. > > 2.6.23-rc6 boot.msg extract ( hub/usb ) I wish users stopped this filtering, it's a very bad idea. As it is, there's no message showing when ehci_hcd was loaded. We can try piece together the picture: > > <6>usb usb1: configuration #1 chosen from 1 choice > > <6>hub 1-0:1.0: USB hub found > > <6>hub 1-0:1.0: 2 ports detected > > <6>usb usb2: configuration #1 chosen from 1 choice > > <6>hub 2-0:1.0: USB hub found > > <6>hub 2-0:1.0: 2 ports detected > > <6>usb 1-1: new low speed USB device using ohci_hcd and address 2 ok this was ohci_hcd > > <3>usb 1-1: device not accepting address 2, error -62 > > <6>usb 5-2: new full speed USB device using uhci_hcd and address 2 was ehci here? We don't know but I bet it was, because: > > <6>usb 6-2: new high speed USB device using ehci_hcd and address 2 > > 2.6.22 boot.msg extract ( hub/usb ) This one seems far shorter. The EHCI comes online late here as well, but not as late as in the "regression" case. I am wondering if we have some kind of "parallel PCI probing" option in action here. -- 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: 2.6.23-rc6: usb 1-1: device not accepting address 2, error -62
On Sat, 15 Sep 2007 03:48:19 -0700, Andrew Morton [EMAIL PROTECTED] wrote: I have an error message with 2.6.23-rc6. This did not happen with 2.6.22. Another one for Michal's dirt file. No, I think it's the module ordering again. 2.6.23-rc6 boot.msg extract ( hub/usb ) I wish users stopped this filtering, it's a very bad idea. As it is, there's no message showing when ehci_hcd was loaded. We can try piece together the picture: 6usb usb1: configuration #1 chosen from 1 choice 6hub 1-0:1.0: USB hub found 6hub 1-0:1.0: 2 ports detected 6usb usb2: configuration #1 chosen from 1 choice 6hub 2-0:1.0: USB hub found 6hub 2-0:1.0: 2 ports detected 6usb 1-1: new low speed USB device using ohci_hcd and address 2 ok this was ohci_hcd 3usb 1-1: device not accepting address 2, error -62 6usb 5-2: new full speed USB device using uhci_hcd and address 2 was ehci here? We don't know but I bet it was, because: 6usb 6-2: new high speed USB device using ehci_hcd and address 2 2.6.22 boot.msg extract ( hub/usb ) This one seems far shorter. The EHCI comes online late here as well, but not as late as in the regression case. I am wondering if we have some kind of parallel PCI probing option in action here. -- 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: [GIT PATCH] USB autosuspend fixes for 2.6.23-rc6
On Thu, 13 Sep 2007 09:43:13 -0700 (PDT), Linus Torvalds <[EMAIL PROTECTED]>
wrote:
> So why not make the 64 sector limit be the default? Get rid of the quirk:
> we already allow people to override it in /sys if they really want to, but
> realistically, it's probably not going to make any difference what-so-ever
> for *any* normal load. So we seem to have a quirk that really doesn't buy
> us anything but headache.
Well, ub does that today. And there is a measurable performance differential
with usb-storage when driving rotating discs, or so I heard.
> Other quirks worth looking at (but likely unfixable) are:
> - US_FL_IGNORE_RESIDUE:
> Does this really matter? Can we not just always do the
> US_FL_IGNORE_RESIDUE thing? Windows must not be doing what we're
> doing.
I'm afraid this is valuable. However, a number of devices only return
garbage as residue if the transfer length is greater than 32KB. Limiting
that would trim this blacklist, I think. The vast majority of devices
work correctly in this regard, and ub checks the residue without any
blacklist.
> - US_FL_FIX_CAPACITY:
> This is a generic SCSI issue, not a USB one, and maybe there are
> better solutions to it. Are we perhaps doing something wrong? Is
> there some patterns we haven't seen? Why do we need this, when
> presumably Windows does not?
It has something to do with the way our partition detection works. Linux
tends to rely on the reported device size. Windows reads the first block
and then goes further based on its contents. If we exterminate partitioning
code which uses the reported device size for autodetection, then this
problem will fix itself.
> - US_FL_SINGLE_LUN:
> At least a few of these seem to indicate that the real problem
> could be detected dynamically ("device reports Sub=ff") rather
> than with a quirk. Quirks are unmaintainable (and change), but
> noticing when devices return impossible values and going into a
> "safe mode" is just defensive programming.
This is being worked upon. The recent change for floppies eliminated
a big number of those.
-- 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: [GIT PATCH] USB autosuspend fixes for 2.6.23-rc6
On Thu, 13 Sep 2007 09:43:13 -0700 (PDT), Linus Torvalds [EMAIL PROTECTED] wrote: So why not make the 64 sector limit be the default? Get rid of the quirk: we already allow people to override it in /sys if they really want to, but realistically, it's probably not going to make any difference what-so-ever for *any* normal load. So we seem to have a quirk that really doesn't buy us anything but headache. Well, ub does that today. And there is a measurable performance differential with usb-storage when driving rotating discs, or so I heard. Other quirks worth looking at (but likely unfixable) are: - US_FL_IGNORE_RESIDUE: Does this really matter? Can we not just always do the US_FL_IGNORE_RESIDUE thing? Windows must not be doing what we're doing. I'm afraid this is valuable. However, a number of devices only return garbage as residue if the transfer length is greater than 32KB. Limiting that would trim this blacklist, I think. The vast majority of devices work correctly in this regard, and ub checks the residue without any blacklist. - US_FL_FIX_CAPACITY: This is a generic SCSI issue, not a USB one, and maybe there are better solutions to it. Are we perhaps doing something wrong? Is there some patterns we haven't seen? Why do we need this, when presumably Windows does not? It has something to do with the way our partition detection works. Linux tends to rely on the reported device size. Windows reads the first block and then goes further based on its contents. If we exterminate partitioning code which uses the reported device size for autodetection, then this problem will fix itself. - US_FL_SINGLE_LUN: At least a few of these seem to indicate that the real problem could be detected dynamically (device reports Sub=ff) rather than with a quirk. Quirks are unmaintainable (and change), but noticing when devices return impossible values and going into a safe mode is just defensive programming. This is being worked upon. The recent change for floppies eliminated a big number of those. -- 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/
Oops in make_class_name in 2.6.22.1 on Fedora
Hi, Greg (and others): I do not seem to be able to find an answer, sorry. Do you happen to remember if this was fixed after 2.6.22.1: localhost kernel: EIP is at make_class_name+0x27/0x7a localhost kernel: [] class_device_del+0x97/0x119 localhost kernel: [] class_device_unregister+0x8/0x10 localhost kernel: [] __scsi_remove_device+0x1d/0x60 [scsi_mod] localhost kernel: [] scsi_forget_host+0x2d/0x4a [scsi_mod] localhost kernel: [] scsi_remove_host+0x65/0xd5 [scsi_mod] localhost kernel: [] storage_disconnect+0xe/0x16 [usb_storage] localhost kernel: [] usb_unbind_interface+0x44/0x85 localhost kernel: [] __device_release_driver+0x6e/0x8b Obviously a known bug but all I see is users reporting it. In my case it's this: https://bugzilla.redhat.com/show_bug.cgi?id=253424 I saw Alan giving it a try here: http://lkml.org/lkml/2007/7/12/259 Yours, -- 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/
Oops in make_class_name in 2.6.22.1 on Fedora
Hi, Greg (and others): I do not seem to be able to find an answer, sorry. Do you happen to remember if this was fixed after 2.6.22.1: localhost kernel: EIP is at make_class_name+0x27/0x7a localhost kernel: [c05586f1] class_device_del+0x97/0x119 localhost kernel: [c055877b] class_device_unregister+0x8/0x10 localhost kernel: [e08735f4] __scsi_remove_device+0x1d/0x60 [scsi_mod] localhost kernel: [e08711ae] scsi_forget_host+0x2d/0x4a [scsi_mod] localhost kernel: [e086c49c] scsi_remove_host+0x65/0xd5 [scsi_mod] localhost kernel: [e0e76775] storage_disconnect+0xe/0x16 [usb_storage] localhost kernel: [c056f8b8] usb_unbind_interface+0x44/0x85 localhost kernel: [c0557df7] __device_release_driver+0x6e/0x8b Obviously a known bug but all I see is users reporting it. In my case it's this: https://bugzilla.redhat.com/show_bug.cgi?id=253424 I saw Alan giving it a try here: http://lkml.org/lkml/2007/7/12/259 Yours, -- 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: [PATCH] [490/2many] MAINTAINERS - USB BLOCK DRIVER (UB ub)
I received two updates, and something jumped out: On Sun, 12 Aug 2007 23:38:00 -0700, [EMAIL PROTECTED] wrote: > +F: drivers/block/ub.c On Sun, 12 Aug 2007 23:38:30 -0700, [EMAIL PROTECTED] wrote: > +F: /drivers/usb/class/usblp.c Why do some patterns start with a leading slash and others do not? -- 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: [PATCH] [490/2many] MAINTAINERS - USB BLOCK DRIVER (UB ub)
I received two updates, and something jumped out: On Sun, 12 Aug 2007 23:38:00 -0700, [EMAIL PROTECTED] wrote: +F: drivers/block/ub.c On Sun, 12 Aug 2007 23:38:30 -0700, [EMAIL PROTECTED] wrote: +F: /drivers/usb/class/usblp.c Why do some patterns start with a leading slash and others do not? -- 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: [PATCH] USB: Only enable autosuspend by default on certain device classes
On Fri, 3 Aug 2007 15:45:32 -0400, Dave Jones <[EMAIL PROTECTED]> wrote: > > > My experience suggests the opposite. Of the several I've tried so far, > > > none have worked with usb suspend. > > > > All of mine work. I'm wondering if this has something to do with > > a hub or motherboard... How should we test it? Tried plugging elsewhere? > > no hubs involved, directly plugged them into the mainboard. > Fairly recent Intel chipsets on all the systems I tested. Sounds bad. BTW, when I took over from Vojtech, I looked at the suspend methods for the printer and it seemed possible that they can only work if the device is not open when suspend occurs. I was going to look at that at the first opportunity. So, I'd rather keep at least some kind of capability to suspend printers, even if disabled by default. I thought Matthew's patch was too harsh. -- 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: [PATCH] USB: Only enable autosuspend by default on certain device classes
On Fri, 03 Aug 2007 15:29:21 -0400, Chuck Ebbert <[EMAIL PROTECTED]> wrote: > > Well, we did - with hindsight it may not have been such a great plan :) > > I believe that Fedora did as well, but have disabled it in an update > > kernel. > > Yeah, autosuspend broke too many devices. Way too many. Glad you chimed in, Chuck. I was wondering about it... I saw something like 3 reports about broken printers, two of them for the same printer, Samsung ML-2010. It's on the installed base of 300,000 to 500,000. People simply do not report squat. Today DaveJ said that none of his printers work, which sounds bad, and was news to me. But the point is, we are trying to extrapolate from too few cases. I am wondering if Ubuntu has better user reporting, so if Matthew complains, it really means something. -- 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: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes
On Fri, 3 Aug 2007 10:24:16 -0400, Dave Jones <[EMAIL PROTECTED]> wrote: > On Fri, Aug 03, 2007 at 09:57:45AM +0200, Oliver Neukum wrote: > > > Kernel developers are a diverser lot than you think ;-) > > We don't enable autosuspend in drivers we can't test, except where > > the lack of a kernel driver forces us to use a broad swipe. Printers > > were tested, too, and most printers seem to work. > > My experience suggests the opposite. Of the several I've tried so far, > none have worked with usb suspend. All of mine work. I'm wondering if this has something to do with a hub or motherboard... How should we test it? Tried plugging elsewhere? -- 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: [linux-usb-devel] [PATCH] USB: Only enable autosuspend by default on certain device classes
On Fri, 3 Aug 2007 10:24:16 -0400, Dave Jones [EMAIL PROTECTED] wrote: On Fri, Aug 03, 2007 at 09:57:45AM +0200, Oliver Neukum wrote: Kernel developers are a diverser lot than you think ;-) We don't enable autosuspend in drivers we can't test, except where the lack of a kernel driver forces us to use a broad swipe. Printers were tested, too, and most printers seem to work. My experience suggests the opposite. Of the several I've tried so far, none have worked with usb suspend. All of mine work. I'm wondering if this has something to do with a hub or motherboard... How should we test it? Tried plugging elsewhere? -- 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/
