On Thu, 2009-08-13 at 16:54 +0100, Alan Cox wrote:
> > + qt2_box_flush(serial, port->number, QT2_FLUSH_TX); /* flush tx buffer */
> > + qt2_box_flush(serial, port->number, QT2_FLUSH_RX); /* flush rx buffer */
> > + /* now wait for the flags to magically go back to being true */
> > + jift = jiffies + (10 * HZ);
> > + do {
> > + if ((port_extra->rcv_flush == true) &&
>
> Horkkkk...
>
>
> Use wait_event and wakeup not busy loops.
>
> Also btw if you implement the chars_in_buffer() method the kernel will
> wait for that to report 0 before closing. That is the correct way to
> handle it as the functionality is also needed for things like break
> handling.
Again, vendor driver does it both ways. It's certainly not nice - it
takes 10 seconds to close a device with is driver...
This patch fixes chars_in_buffer() (it takes talent to write a
get-method that needs 4 statements), and thus removes the first stall
loop waiting for buffers to be flushed (I wonder if the windows API
lacks this feature, and so there has to be hardware support to do this
sort of thing? Can't think why else hardware flush support would be
added to the device).
> > + jift = jiffies + (10 * HZ); /* 10 sec timeout */
> > + do {
> > + status = qt2_box_get_register(serial, port->number,
> > + QT2_LINE_STATUS_REGISTER, &lsr_value);
> > + if (status < 0) {
> > + dbg("%s(): qt2_box_get_register failed", __func__);
> > + break;
> > + }
> > + if ((lsr_value & QT2_LSR_TEMT)) {
> > + dbg("UART done sending");
> > + break;
> > + }
> > + schedule();
>
> If you have to poll then it might be a good idea to sleep between polls
> so you don't jam up the USB bus.
Agreed, but I'm not so sure how to write this one. Do I need to wrap the
call to qt2_box_get_register() and the test of
(lsr_value & QT2_LSR_TEMT) up in a function
bool qt2_check_uart_has_done()
and pass that as the second argument of wait_event()? What happens to
time-outs in that case (do it inside qt2_check_uart_has_done()?)?
> > +/* called to deliver writes from the next layer up to the device */
> > +static int qt2_write(struct tty_struct *tty, struct usb_serial_port *port,
> > + const unsigned char *buf, int count)
> > +{
> > + struct usb_serial *serial; /* parent device struct */
> > + __u8 header_array[5]; /* header used to direct writes to the correct
>
> You will save yourself an enormous amount of pain if you ignore the
> existing USB drivers on this one (as they mostly suck rocks) and
> implement a proper 4K or so FIFO. That cleans up so much its unbelievable
> (especially as we have the kfifo API ...). At that point you get proper
> buffering and you just empty the fifo into an URB every time you get a
> completion for the last one.
That sounds like a very good idea. There is already supposed to be a
single URB on the incoming direction which is sent off again every time
we get a completion to handle reading from the device. Apart from the
fact that we never seem to get the completions, this would be a nice way
to implement writes. I might see if someone else writes a nice example
implementation in response to the thread "Combine two one-character
CR-LF writes into one two-character write for O_ONLCR" before I try - it
will probably save a lot of everyone's time.
This patch (number 3 of today's set) is really only the fix to
chars_in_buffer() plus a fixme comment, and hasn't been tested yet.
Richard
Signed-off-by: Richard Ash <richard@xxxxxxxxxxxxxxxx>
---
Index: linux/drivers/staging/quatech_usb2/quatech_usb2.c
===================================================================
--- linux.orig/drivers/staging/quatech_usb2/quatech_usb2.c
+++ linux/drivers/staging/quatech_usb2/quatech_usb2.c
@@ -619,7 +619,10 @@ int qt2_open(struct tty_struct *tty, str
return 0;
}
-/* called when a port is closed by userspace */
+/* called when a port is closed by userspace. It won't be called, however,
+ * until calls to chars_in_buffer() reveal that the port has completed
+ * sending buffered data, and there is nothing else to do. Thus we don't have
+ * to rely on forcing data through in this function. */
/* Setting close_pending should keep new data from being written out,
* once all the data in the enpoint buffers is moved out we won't get
* any more. */
@@ -644,26 +647,8 @@ static void qt2_close(struct usb_serial_
/* get the device private data */
port_extra = qt2_get_port_private(port); /* port private data */
- /* to check we have successfully flushed the buffers on the hardware,
- * we set the flags indicating flushes have occured to false, then ask
- * for flushes to occur, then sit in a timed loop until either we
- * get notified back that the flushes have happened (good) or we get
- * tired of waiting for the flush to happen and give up (bad).
- */
- port_extra->rcv_flush = false;
- port_extra->xmit_flush = false;
- qt2_box_flush(serial, port->number, QT2_FLUSH_TX); /* flush tx buffer */
- qt2_box_flush(serial, port->number, QT2_FLUSH_RX); /* flush rx buffer */
- /* now wait for the flags to magically go back to being true */
- jift = jiffies + (10 * HZ);
- do {
- if ((port_extra->rcv_flush == true) &&
- (port_extra->xmit_flush == true)) {
- dbg("Flush completed");
- break;
- }
- schedule();
- } while (jiffies <= jift);
+ /* we don't need to force flush though the hardware, so we skip using
+ * qt2_box_flush() here */
/* we can now (and only now) stop reading data */
port_extra->close_pending = true;
@@ -672,6 +657,9 @@ static void qt2_close(struct usb_serial_
* still be pushing characters out over the line, so we have to
* wait testing the actual line status until the lines change
* indicating that the data is done transfering. */
+ /* FIXME: slow this polling down so it doesn't run the USB bus flat out
+ * if it actually has to spend any time in this loop (which it normally
+ * doesn't because the buffer is nearly empty) */
jift = jiffies + (10 * HZ); /* 10 sec timeout */
do {
status = qt2_box_get_register(serial, port->number,
@@ -848,16 +836,12 @@ static int qt2_chars_in_buffer(struct tt
{
struct usb_serial_port *port = tty->driver_data;
/* parent usb_serial_port pointer */
- int chars = 0;
struct quatech2_port *port_extra; /* extra data for this port */
port_extra = qt2_get_port_private(port);
- dbg("%s(): port %d", __func__, port->number);
- if ((port->write_urb->status == -EINPROGRESS) &&
- (port_extra->tx_pending_bytes != 0))
- chars = port->write_urb->transfer_buffer_length;
- dbg("%s(): returns %d", __func__, chars);
- return chars;
+ dbg("%s(): port %d: chars_in_buffer = %d", __func__,
+ port->number, port_extra->tx_pending_bytes);
+ return port_extra->tx_pending_bytes;
}
/* called when userspace does an ioctl() on the device. Note that
@@ -879,7 +863,6 @@ static int qt2_ioctl(struct tty_struct *
/* Declare a wait queue named "wait" */
unsigned int value;
- int status;
unsigned int UartNumber;
if (serial == NULL)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
