[PATCH] USB: usblp.c (Was: usblp_write spins forever after an error)

Paulo Marques wrote:

> David Woodhouse wrote:
>
>> On Thu, 2004-03-04 at 12:33 +0000, Paulo Marques wrote:
>>
>>> Yes, unfortunately it did went into 2.6.4-rc1. However it is already
>>> corrected in 2.6.4-rc2. Luckily it didn't went into any "non-rc"
>>> official release.

> >>> Please try 2.6.4-rc2, and check to see if the bug went away...

>>
>> Seems to work; thanks. Does this need backporting to 2.4 too?
>>
>
>
> Unfortunately this isn't over yet.
>
> I got suspicious about this bug fix, because I *did* test my patch
> before submitting it and the kernel that didn't work before, worked fine
> with my patch.
>
> But now it seems that it is the other way around. After a few digging I
> found out the problem:
>
> The application that I was testing with uses the usblp handle with
> non-blocking I/O .
>
> So my patch does work for non-blocking I/O uses of the port, but wrecks
> the normal blocking mode.
>
> I've already produced a version that works for both cases. I'll just
> clean it up a bit and submit it to 2.4 and 2.6 kernels.


Here it is.

The patch is only one line for 2.6.4-rc2. (I also did a little formatting
adjustment to better comply with CodingStyle)

For the 2.4.26-pre1 kernel, I also backported the return codes correction patch
from Oliver Neukum.


The problem with the write function was that, in non-blocking mode, after
submitting the first urb, the function would return with -EAGAIN, not reporting
to the application that in fact it had already sent "transfer_length" bytes.
This way the application would have to send the data *again* causing lots of
errors.

It did return the correct amount with my first patch, because the writecount was
being updated on the end of the loop. However this was wrong for blocking I/O.

The "transfer_length" local variable is still needed because if we used the
transfer_buffer_length field from the urb, then on a second call to write, if
the urb was still pending (in non-blocking mode), the write would return an
incorrect amount of data written.

Anyway, this time I tested it using blocking and non-blocking I/O and it works
for both cases. Even better, this patch only changes the behaviour for
non-blocking I/O, and keeps the same behaviour for the more usual blocking I/O
(at least on kernel 2.6).

drivers/usb/class/usblp.c

@@ -610,8 +610,10 @@ static ssize_t usblp_write(struct file *file, const char __user *buffer, size_t
 	while (writecount < count) {
 		if (!usblp->wcomplete) {
 			barrier();
-			if (file->f_flags & O_NONBLOCK)
+			if (file->f_flags & O_NONBLOCK) {
+				writecount += transfer_length;
 				return writecount ? writecount : -EAGAIN;
+			}
 
 			timeout = USBLP_WRITE_TIMEOUT;
 			add_wait_queue(&usblp->wait, &wait);
@@ -671,7 +673,8 @@ static ssize_t usblp_write(struct file *file, const char __user *buffer, size_t
 
 		usblp->writeurb->transfer_buffer_length = transfer_length;
 
-		if (copy_from_user(usblp->writeurb->transfer_buffer, buffer + writecount, transfer_length)) {
+		if (copy_from_user(usblp->writeurb->transfer_buffer, 
+				   buffer + writecount, transfer_length)) {
 			up(&usblp->sem);
 			return writecount ? writecount : -EFAULT;
 		}