[PATCH 2.4] usb serial write fix

[PATCH 2.4] usb serial write fix

[Paul Fulghum]

Fix usb serial write path in post_helper to check return
code from component driver write routine and
resubmit if necessary. The post helper introduced in
2.4.27-pre6 can lose write data if component device write is busy.

This was previously reported as a problem with
the pl2303 driver running PPP by oleksiy@jabber.ru
Oleksiy has tested the patch with success.

Signed-off-by: Paul Fulghum <paulkf@microgate.com>

-- 
Paul Fulghum
paulkf@microgate.com

--- linux-2.4.28-pre4/drivers/usb/serial/usbserial.c	2004-08-07 18:26:05.000000000 -0500
+++ b/drivers/usb/serial/usbserial.c	2004-11-01 08:29:07.000000000 -0600
@@ -508,8 +508,18 @@ static void post_helper(void *arg)
 		down(&port->sem);
 		dbg("%s - port %d len %d backlog %d", __FUNCTION__,
 		    port->number, job->len, port->write_backlog);
-		if (port->tty != NULL)
-			__serial_write(port, 0, job->buff, job->len);
+		if (port->tty != NULL) {
+			int rc;
+			int sent = 0;
+			while (sent < job->len) {
+				rc = __serial_write(port, 0, job->buff + sent, job->len - sent);
+				if ((rc < 0) || signal_pending(current))
+					break;
+				sent += rc;
+				if ((sent < job->len) && current->need_resched)
+					schedule();
+			}
+		}
 		up(&port->sem);
 
 		spin_lock_irqsave(&post_lock, flags);

Re: [PATCH 2.4] usb serial write fix

[Pete Zaitcev]

On Mon, 01 Nov 2004 08:51:08 -0600, Paul Fulghum <paulkf@microgate.com> wrote:

Not a bad idea. I must admit that I thought about that, but then I had
concerns about monopolizing keventd, decided to think later about it, and
forgot about the issue.

> +++ b/drivers/usb/serial/usbserial.c	2004-11-01 08:29:07.000000000 -0600
> +		if (port->tty != NULL) {
> +			int rc;
> +			int sent = 0;
> +			while (sent < job->len) {
> +				rc = __serial_write(port, 0, job->buff + sent, job->len - sent);
> +				if ((rc < 0) || signal_pending(current))
> +					break;

Why testing for signals? Do you expect any?

> +				sent += rc;
> +				if ((sent < job->len) && current->need_resched)
> +					schedule();

That's the main problem here, isn't it. Serial communications are slow.
Tying up a shared thread just because of this just does not look right.
And in such CPU intensive way, too.

Looking at pl2303 in 2.4, I do not see any difference between its ->write
method and generic_write which would be specific to pl2303. The key
difference is that generic_write participates in the protocol governed by
port->write_busy. So why don't you simply drop pl2303_write?

-- Pete

Re: [PATCH 2.4] usb serial write fix

[Paul Fulghum]

On Mon, 2004-11-01 at 21:36, Pete Zaitcev wrote:

> Why testing for signals? Do you expect any?

post_helper can run in a user process as well
as keventd. The user process can get a signal
like HUP to pppd.

> Tying up a shared thread just because of this just does not look right.

OK. 

post_helper could hold the job and reschedule the work routine,
so it does not block other work routines.
Throwing the job away is not workable.

> Looking at pl2303 in 2.4, I do not see any difference between its ->write
> method and generic_write which would be specific to pl2303. The key
> difference is that generic_write participates in the protocol governed by
> port->write_busy. So why don't you simply drop pl2303_write?

That might fix the problem for pl2303, but if other component drivers
have driver specific write routines that do not implement this protocol,
they will have the problem also.

It seemed a better to fix this in one location instead
of auditing all component drivers and replicating a
fix in multiple places. Maybe no other component drivers
implement a driver specific write routine, I have not checked.

If pl2303_write has no necessary difference from generic_write
then pl2303_write should certainly be dropped.

-- 
Paul Fulghum
paulkf@microgate.com

Re: [PATCH 2.4] usb serial write fix

[Paul Fulghum]

On Tue, 2004-11-02 at 08:03, Paul Fulghum wrote:
> On Mon, 2004-11-01 at 21:36, Pete Zaitcev wrote:
> > Why testing for signals? Do you expect any?
> 
> post_helper can run in a user process as well
> as keventd. The user process can get a signal
> like HUP to pppd.

This brings up a question.

post_helper is using a single list to queue
write requests from all user processes.

A write request on the list can be processed
in a user process different than the submitting process.

Signals sent to one user process can interfere with
the processing of write requests from a different process.

Is this not a security problem?

-- 
Paul Fulghum
paulkf@microgate.com

Re: [PATCH 2.4] usb serial write fix

[Pete Zaitcev]

On Tue, 02 Nov 2004 09:25:08 -0600, Paul Fulghum <paulkf@microgate.com> wrote:

> On Tue, 2004-11-02 at 08:03, Paul Fulghum wrote:
> > On Mon, 2004-11-01 at 21:36, Pete Zaitcev wrote:
> > > Why testing for signals? Do you expect any?
> > 
> > post_helper can run in a user process as well
> > as keventd. The user process can get a signal
> > like HUP to pppd.

> Signals sent to one user process can interfere with
> the processing of write requests from a different process.

This is a problem only _if_ we apply your patch (which checks for
signals), this is why I asked about it in the first place.

-- Pete