Regression: ftdi_sio is slow (since Wed Oct 10 15:05:06 2012)
Subsystems (3)
Hi.
We have a regression because of this patch:
http://lkml.indiana.edu/hypermail/linux/kernel/1210.1/01456.html
While it is arguably reasonable to have this for tcdrain or close, it also slows down poll/select a lot because n_tty_poll() does this:
tty_chars_in_buffer(tty) < WAKEUP_CHARS
And it also slows down TIOCOUTQ ioctl I think (not measured). The slowdown of select() is big, the customer reports the inability to work that way.
Is this patch really needed? I mean, if the time to check TEMT is longer than to xmit that char, then what's the use?
Or, if it is really a big deal, I guess it would be necessary to add a separate, .chars_in_buffer_fast method.
Thoughts?
Greg KH
maintainerQuoted 8 lines · Stas Sergeev
Hi.
We have a regression because of this patch:
http://lkml.indiana.edu/hypermail/linux/kernel/1210.1/01456.html
While it is arguably reasonable to have this for tcdrain or close, it also slows down poll/select a lot becausen_tty_poll()does this:
tty_chars_in_buffer(tty)<WAKEUP_CHARS
What do you mean "slows down"?
Quoted 3 lines
And it also slows down TIOCOUTQ ioctl I think (not measured). The slowdown of
select()is big, the customer reports the inability to work that way.
Inability to work what way?
Quoted 4 lines
Is this patch really needed? I mean, if the time to check TEMT is longer than to xmit that char, then what's the use?
Or, if it is really a big deal, I guess it would be necessary to add a separate, .chars_in_buffer_fast method.
We need some way to check the chars in the buffer, is the device you are using just very slow to respond to this request? How slow? Do you have a test case that we can see how it is affected?
thanks,
greg k-h
03.05.2013 20:30, Greg KH пишет:
Quoted 3 lines
We need some way to check the chars in the buffer, is the device you are using just very slow to respond to this request? How slow? Do you have a test case that we can see how it is affected?
Greg, unfortunately, I do have nothing.
The customer is in CC list, so maybe he will
provide the test-case, but I doubt.
Please, what are your concerns here?
The patch in question does this:
---
+ ret = usb_control_msg(port->serial->dev,
+ usb_rcvctrlpipe(port->serial->dev, 0),
+ FTDI_SIO_GET_MODEM_STATUS_REQUEST,
+ FTDI_SIO_GET_MODEM_STATUS_REQUEST_TYPE,
+ 0, priv->interface,
+ buf, 2, WDR_TIMEOUT);
---
Obviously, this is too expensive to call too frequently, or am I missing something?
I asked the customer to comment out tty_chars_in_buffer(tty) < WAKEUP_CHARS
line in n_tty.c, and he said that cured his problems, so I think my guess was right.
The patch claims it only affects tcdrain() and close(). Its trivial to see it also affects poll(), select() and TIOCOUTQ ioctl, so even from that it is already broken.
Why do you need a test-case for this?
Greg KH
maintainerQuoted 8 lines · Stas Sergeev
03.05.2013 20:30, Greg KH пишет:
Quoted 3 lines
We need some way to check the chars in the buffer, is the device you are using just very slow to respond to this request? How slow? Do you have a test case that we can see how it is affected?
Greg, unfortunately, I do have nothing.
The customer is in CC list, so maybe he will
provide the test-case, but I doubt.
Quoted 12 lines
Please, what are your concerns here?
The patch in question does this:
---
+ ret = usb_control_msg(port->serial->dev,
+usb_rcvctrlpipe(port->serial->dev, 0),
+FTDI_SIO_GET_MODEM_STATUS_REQUEST,
+FTDI_SIO_GET_MODEM_STATUS_REQUEST_TYPE,
+ 0,priv->interface,
+ buf, 2,WDR_TIMEOUT);
---
Obviously, this is too expensive to call too frequently, or am I missing something?
Why do you think that is too expensive to call? Does it somehow stop the data being sent to the device through the serial endpoints? Is userspace calling this function too much slowing something else down?
Quoted 4 lines
I asked the customer to comment out
tty_chars_in_buffer(tty)<WAKEUP_CHARS
line inn_tty.c, and he said that cured his problems, so I think my guess was right.
What exactly is the "problem" being seen?
Quoted 4 lines
The patch claims it only affects
tcdrain()andclose(). Its trivial to see it also affectspoll(),select()and TIOCOUTQ ioctl, so even from that it is already broken.
Why do you need a test-case for this?
Because I don't know what the problem really is :)
thanks,
greg k-h
03.05.2013 20:52, Greg KH пишет:
Quoted 21 lines · Stas Sergeev
Quoted 8 lines
03.05.2013 20:30, Greg KH пишет:
Quoted 3 lines
We need some way to check the chars in the buffer, is the device you are using just very slow to respond to this request? How slow? Do you have a test case that we can see how it is affected?
Greg, unfortunately, I do have nothing.
The customer is in CC list, so maybe he will
provide the test-case, but I doubt.Quoted 12 lines
Please, what are your concerns here?
The patch in question does this:
---
+ ret = usb_control_msg(port->serial->dev,
+usb_rcvctrlpipe(port->serial->dev, 0),
+FTDI_SIO_GET_MODEM_STATUS_REQUEST,
+FTDI_SIO_GET_MODEM_STATUS_REQUEST_TYPE,
+ 0,priv->interface,
+ buf, 2,WDR_TIMEOUT);
---
Obviously, this is too expensive to call too frequently, or am I missing something?
Quoted 3 lines
Why do you think that is too expensive to call? Does it somehow stop the data being sent to the device through the serial endpoints? Is userspace calling this function too much slowing something else down?
No, it doesn't slow down the data transfer.
But it makes a select() call to take much longer to complete, and the same goes to TIOCOUTQ ioctl. Yes, the app calls select() quite too much, and it is single-threaded, too. :)
Quoted 5 lines
Quoted 4 lines
I asked the customer to comment out
tty_chars_in_buffer(tty)<WAKEUP_CHARS
line inn_tty.c, and he said that cured his problems, so I think my guess was right.What exactly is the "problem" being seen?
No idea.
Well, I can make a test-case that does 1000000 select() calls in a loop and time it. This is probably the best I can do.
Quoted 5 lines
Quoted 4 lines
The patch claims it only affects
tcdrain()andclose(). Its trivial to see it also affectspoll(),select()and TIOCOUTQ ioctl, so even from that it is already broken.
Why do you need a test-case for this?Because I don't know what the problem really is :)
Slow select() call (and other calls).
Can we just use usb_serial_generic_chars_in_buffer() in ftdi_sio? What was the reason behind the patch at all, why it is so importand to query TEMT? I can write some test-case, but it would be better if I at least understand why the patch was needed at all. I don't understand why quering TEMT is that important.
I can code up the patch that will just stop quering TEMT, test it with the customer and send it to you (basically, a revert of the patch in question).
Greg KH
maintainerQuoted 22 lines · Stas Sergeev
03.05.2013 20:52, Greg KH пишет:
Quoted 21 lines · Stas Sergeev
Quoted 20 lines
03.05.2013 20:30, Greg KH пишет:
Quoted 3 lines
We need some way to check the chars in the buffer, is the device you are using just very slow to respond to this request? How slow? Do you have a test case that we can see how it is affected?
Greg, unfortunately, I do have nothing.
The customer is in CC list, so maybe he will
provide the test-case, but I doubt.Please, what are your concerns here?
The patch in question does this:
---
+ ret = usb_control_msg(port->serial->dev,
+usb_rcvctrlpipe(port->serial->dev, 0),
+FTDI_SIO_GET_MODEM_STATUS_REQUEST,
+FTDI_SIO_GET_MODEM_STATUS_REQUEST_TYPE,
+ 0,priv->interface,
+ buf, 2,WDR_TIMEOUT);
---
Obviously, this is too expensive to call too frequently, or am I missing something?
Quoted 5 lines
Quoted 3 lines
Why do you think that is too expensive to call? Does it somehow stop the data being sent to the device through the serial endpoints? Is userspace calling this function too much slowing something else down?
No, it doesn't slow down the data transfer.
But it makes aselect()call to take much longer to complete,
Please define "much longer".
Quoted 2 lines
and the same goes to TIOCOUTQ ioctl. Yes, the app calls
select()quite too much, and it is single-threaded, too. :)
Sounds like an application is doing a foolish thing and should stop it. There's no guarantee as to how long select or an ioctl will take, and now that we have fixed another bug, this device is slower.
If you change hardware types to use a different usb to serial chip, that select call might take 4 times as long. Are we somehow supposed to change the kernel to "fix" that?
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
I asked the customer to comment out
tty_chars_in_buffer(tty)<WAKEUP_CHARS
line inn_tty.c, and he said that cured his problems, so I think my guess was right.What exactly is the "problem" being seen?
No idea.
Well, I can make a test-case that does 1000000select()calls in a loop and time it. This is probably the best I can do.
That's really not a valid test case, as it's nothing that we ever optimize a serial driver for. Throughput is the proper thing to care about, right?
And if select takes longer, nothing really "slows down" as now you get data more often between select calls, right?
Quoted 9 lines
Quoted 5 lines
Quoted 4 lines
The patch claims it only affects
tcdrain()andclose(). Its trivial to see it also affectspoll(),select()and TIOCOUTQ ioctl, so even from that it is already broken.
Why do you need a test-case for this?Because I don't know what the problem really is :)
Slow
select()call (and other calls).
Can we just useusb_serial_generic_chars_in_buffer()inftdi_sio? What was the reason behind the patch at all, why it is so importand to query TEMT?
To actually determine how many characters the device has in its buffer.
thanks,
greg k-h
03.05.2013 21:16, Greg KH пишет:
Quoted 1 line
Sounds like an application is doing a foolish thing and should stop it.
Its not.
The app is quering only for _input_ (specifying only read fds to select). But the select() in linux is implemented the way that even when it polls for input, it will still call tty_chars_in_buffer()...
Quoted 6 lines
There's no guarantee as to how long select or an ioctl will take, and now that we have fixed another bug, this device is slower.
If you change hardware types to use a different usb to serial chip, that select call might take 4 times as long. Are we somehow supposed to change the kernel to "fix" that?
Previously, the kernel was not calling to a device at all, so select() was independent of the chip, and it was fast. I was not aware you changed that willingly.
Quoted 11 lines
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
I asked the customer to comment out
tty_chars_in_buffer(tty)<WAKEUP_CHARS
line inn_tty.c, and he said that cured his problems, so I think my guess was right.What exactly is the "problem" being seen?
No idea.
Well, I can make a test-case that does 1000000select()calls in a loop and time it. This is probably the best I can do.That's really not a valid test case, as it's nothing that we ever optimize a serial driver for. Throughput is the proper thing to care about, right?
Sure, but the throughput was not improved by the aforementioned patch, so what was the upside of it?
Quoted 1 line
To actually determine how many characters the device has in its buffer.
You are adding only 1 char, but the time to query TEMT is probably longer than to xmit 1 char. So how could it help in some real scenario? When you done quering TEMT, the char is actually already sent, so the effect is quite the reverse.
My scenario is:
the app calls select() before xmitting every char. It seems
it can never fill up the output buffer now, so the throughput have suffered.
What would you suggest to improve it?
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Greg KH
maintainerQuoted 7 lines · Stas Sergeev
03.05.2013 21:16, Greg KH пишет:
Quoted 1 line
Sounds like an application is doing a foolish thing and should stop it.
Its not.
The app is quering only for _input_ (specifying only read fds to select). But theselect()in linux is implemented the way that even when it polls for input, it will still calltty_chars_in_buffer()...
I think that's the line dicipline doing this, not select itself.
Quoted 9 lines
Quoted 6 lines
There's no guarantee as to how long select or an ioctl will take, and now that we have fixed another bug, this device is slower.
If you change hardware types to use a different usb to serial chip, that select call might take 4 times as long. Are we somehow supposed to change the kernel to "fix" that?
Previously, the kernel was not calling to a device at all, so
select()was independent of the chip, and it was fast. I was not aware you changed that willingly.
I don't understand, what do you mean by this? Some drivers just return the value of an internally held number, and don't query the device.
The only way the FTDI driver can determine if the hardware buffer on the chip way out on the end of the USB cable is empty or not, is to query it. So the driver now does so.
Quoted 14 lines
Quoted 11 lines
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
I asked the customer to comment out
tty_chars_in_buffer(tty)<WAKEUP_CHARS
line inn_tty.c, and he said that cured his problems, so I think my guess was right.What exactly is the "problem" being seen?
No idea.
Well, I can make a test-case that does 1000000select()calls in a loop and time it. This is probably the best I can do.That's really not a valid test case, as it's nothing that we ever optimize a serial driver for. Throughput is the proper thing to care about, right?
Sure, but the throughput was not improved by the aforementioned patch, so what was the upside of it?
Quoted 8 lines
Quoted 1 line
To actually determine how many characters the device has in its buffer.
You are adding only 1 char, but the time to query TEMT is probably longer than to xmit 1 char. So how could it help in some real scenario? When you done quering TEMT, the char is actually already sent, so the effect is quite the reverse.
My scenario is:
the app callsselect()before xmitting every char.
Every character? Why? That defeats all of the buffering that the kernel, and the hardware, provide for you. No wonder the program is so slow, that's just crazy.
Quoted 3 lines
It seems it can never fill up the output buffer now, so the throughput have suffered.
What would you suggest to improve it?
Don't call select for every character :)
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
04.05.2013 00:34, Greg KH пишет:
Quoted 9 lines · Stas Sergeev
Quoted 7 lines
03.05.2013 21:16, Greg KH пишет:
Quoted 1 line
Sounds like an application is doing a foolish thing and should stop it.
Its not.
The app is quering only for _input_ (specifying only read fds to select). But theselect()in linux is implemented the way that even when it polls for input, it will still calltty_chars_in_buffer()...I think that's the line dicipline doing this, not select itself.
Line discipline only provides the .poll method, like this:
---
static unsigned int n_tty_poll(struct tty_struct *tty, struct file *file, poll_table *wait)
---
It doesn't look like the poll callback can find out whether it is polling for input or for output. So it just returns all the flags it can.
Any suggestions?
Quoted 9 lines
Quoted 9 lines
Quoted 6 lines
There's no guarantee as to how long select or an ioctl will take, and now that we have fixed another bug, this device is slower.
If you change hardware types to use a different usb to serial chip, that select call might take 4 times as long. Are we somehow supposed to change the kernel to "fix" that?
Previously, the kernel was not calling to a device at all, so
select()was independent of the chip, and it was fast. I was not aware you changed that willingly.
Quoted 6 lines
I don't understand, what do you mean by this? Some drivers just return the value of an internally held number, and don't query the device.
The only way the FTDI driver can determine if the hardware buffer on the chip way out on the end of the USB cable is empty or not, is to query it. So the driver now does so.
It does so only for one char. And the query takes longer than to just xmit that char. So why do you think this even works as expected?
Quoted 15 lines
Quoted 15 lines
Quoted 11 lines
Quoted 8 lines
Quoted 5 lines
Quoted 4 lines
I asked the customer to comment out
tty_chars_in_buffer(tty)<WAKEUP_CHARS
line inn_tty.c, and he said that cured his problems, so I think my guess was right.What exactly is the "problem" being seen?
No idea.
Well, I can make a test-case that does 1000000select()calls in a loop and time it. This is probably the best I can do.That's really not a valid test case, as it's nothing that we ever optimize a serial driver for. Throughput is the proper thing to care about, right?
Sure, but the throughput was not improved by the aforementioned patch, so what was the upside of it?
Quoted 1 line
To actually determine how many characters the device has in its buffer.
Quoted 8 lines
Quoted 7 lines
You are adding only 1 char, but the time to query TEMT is probably longer than to xmit 1 char. So how could it help in some real scenario? When you done quering TEMT, the char is actually already sent, so the effect is quite the reverse.
My scenario is:
the app callsselect()before xmitting every char.Every character? Why?
Because, as I said, this polls for an input, not output... Ah, wait, it also does TIOCOUTQ ioctl to find out just how much is buffered. Are you suggesting to stop using TIOCOUTQ too? Just because it stopped to work fast enough to be of any use at all?
Quoted 1 line
Don't call select for every character :)
How about the "we do not break userspace" rule?
And oh yes, it polls just for an input...
Quoted 3 lines · Stas Sergeev
04.05.2013 00:34, Greg KH пишет:
Quoted 2 lines · Stas Sergeev
Quoted 1 line
03.05.2013 21:16, Greg KH пишет:
[...]
Quoted 12 lines
Quoted 12 lines
Quoted 9 lines
Quoted 6 lines
There's no guarantee as to how long select or an ioctl will take, and now that we have fixed another bug, this device is slower.
If you change hardware types to use a different usb to serial chip, that select call might take 4 times as long. Are we somehow supposed to change the kernel to "fix" that?
Previously, the kernel was not calling to a device at all, so
select()was independent of the chip, and it was fast. I was not aware you changed that willingly.I don't understand, what do you mean by this? Some drivers just return the value of an internally held number, and don't query the device.
Quoted 6 lines
Quoted 3 lines
The only way the FTDI driver can determine if the hardware buffer on the chip way out on the end of the USB cable is empty or not, is to query it. So the driver now does so.
It does so only for one char. And the query takes longer than to just xmit that char. So why do you think this even works as expected?
The query takes longer than the transmit at decent baudrates (>=38k) and under the assumption that flow control isn't causing any delays.
But you do have a point, and I have been meaning to look into whether the added overhead of checking the hardware buffers could be mitigated by adding wait_until_sent support to usb-serial. This way the we would only query the hardware buffers on tty_wait_until_sent (e.g. at close) and select and TIOCMOUTQ would not suffer. This is also the way things are handled in serial_core.
I'll prepare a series which adds wait_until_sent to usb-serial, but I doubt it would be stable material (even if it could get into 3.10).
What do you think Greg, is this overhead to chars_in_buffer reason enough to disable it in the stable trees or should we simply fix it in 3.11 (or 3.10)? (The overhead is about 3-400 us per call when the port fifo is empty, which makes chars_in_buffer about 100 times slower on my test system.)
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 29 lines · Johan Hovold
Quoted 3 lines · Stas Sergeev
04.05.2013 00:34, Greg KH пишет:
Quoted 2 lines · Stas Sergeev
Quoted 1 line
03.05.2013 21:16, Greg KH пишет:
[...]
Quoted 12 lines
Quoted 12 lines
Quoted 9 lines
Quoted 6 lines
There's no guarantee as to how long select or an ioctl will take, and now that we have fixed another bug, this device is slower.
If you change hardware types to use a different usb to serial chip, that select call might take 4 times as long. Are we somehow supposed to change the kernel to "fix" that?
Previously, the kernel was not calling to a device at all, so
select()was independent of the chip, and it was fast. I was not aware you changed that willingly.I don't understand, what do you mean by this? Some drivers just return the value of an internally held number, and don't query the device.
Quoted 6 lines
Quoted 3 lines
The only way the FTDI driver can determine if the hardware buffer on the chip way out on the end of the USB cable is empty or not, is to query it. So the driver now does so.
It does so only for one char. And the query takes longer than to just xmit that char. So why do you think this even works as expected?
The query takes longer than the transmit at decent baudrates (>=38k) and under the assumption that flow control isn't causing any delays.
Quoted 6 lines
But you do have a point, and I have been meaning to look into whether the added overhead of checking the hardware buffers could be mitigated by adding
wait_until_sentsupport to usb-serial. This way the we would only query the hardware buffers ontty_wait_until_sent(e.g. at close) and select and TIOCMOUTQ would not suffer. This is also the way things are handled inserial_core.
Agreed. This is the correct solution.
Quoted 8 lines
I'll prepare a series which adds
wait_until_sentto usb-serial, but I doubt it would be stable material (even if it could get into 3.10).What do you think Greg, is this overhead to
chars_in_bufferreason enough to disable it in the stable trees or should we simply fix it in 3.11 (or 3.10)? (The overhead is about 3-400 us per call when the port fifo is empty, which makeschars_in_bufferabout 100 times slower on my test system.)
A better solution for stable would be to set port->drain_delay. It won't help tcdrain() but at least the port won't shutdown on live outbound data.
Regards,
Peter Hurley
Quoted 5 lines · Peter Hurley
Quoted 4 lines · Johan Hovold
Quoted 3 lines · Stas Sergeev
04.05.2013 00:34, Greg KH пишет:
Quoted 2 lines · Stas Sergeev
Quoted 1 line
03.05.2013 21:16, Greg KH пишет:
[...]
Quoted 18 lines
Quoted 6 lines
But you do have a point, and I have been meaning to look into whether the added overhead of checking the hardware buffers could be mitigated by adding
wait_until_sentsupport to usb-serial. This way the we would only query the hardware buffers ontty_wait_until_sent(e.g. at close) and select and TIOCMOUTQ would not suffer. This is also the way things are handled inserial_core.Agreed. This is the correct solution.
Quoted 8 lines
I'll prepare a series which adds
wait_until_sentto usb-serial, but I doubt it would be stable material (even if it could get into 3.10).What do you think Greg, is this overhead to
chars_in_bufferreason enough to disable it in the stable trees or should we simply fix it in 3.11 (or 3.10)? (The overhead is about 3-400 us per call when the port fifo is empty, which makeschars_in_bufferabout 100 times slower on my test system.)
Quoted 3 lines
A better solution for stable would be to set
port->drain_delay. It won't helptcdrain()but at least the port won't shutdown on live outbound data.
Removing the hardware-buffer checks from chars_in_buffer would mean breaking tty_wait_until_sent and thus also, for example, tcdrain, tcsendbreak, and close. Using drain_delay would partially work-around the problem with close, but at the cost of adding delays for all users while still not being able to guarantee that the buffers have been drained. Either way, this seems to amount to a regression.
On the other hand, the added overhead to chars_in_buffer seems to break at least one application as well.
I've implemented wait_until_sent-support for usb-serial, which fixes the problem without any such trade-offs and which I believe needs to go in to v3.10.
For the stable trees I think keeping the current, less efficient (but more complete) chars_in_buffer implementations is preferred over introducing further regressions. Back-porting wait_until_sent-support could perhaps also be considered.
I'm responding to this mail with a wait_until_sent-support series.
Thanks,
Johan
cover USB: serial: add wait_until_sent-support
These patches add wait_until_sent-support to usb-serial, which removes the need to check hardware buffers in chars_in_buffer.
This fixes a problem in ftdi_sio (since 3.7) where select or TIOCMOUTQ would take much longer than before due the hardware buffers being queried.
Hardware buffers are also currently checked in chars_in_buffer in io_ti (since 3.8) and ti_usb_3410_5052 (in 3.10).
Note that simply removing the hardware-buffer checks (e.g. for the stable trees) would break tty_wait_until_sent, which is used, for instance, by tcdrain, tcsendbreak, and close.
Johan
Johan Hovold (7):
USB: serial: add wait_until_sent operation
USB: serial: add generic wait_until_sent implementation
USB: ftdi_sio: clean up get_modem_status
USB: ftdi_sio: fix chars_in_buffer overhead
USB: io_ti: fix chars_in_buffer overhead
USB: ti_usb_3410_5052: fix chars_in_buffer overhead
USB: serial: clean up chars_in_buffer
drivers/usb/serial/ftdi_sio.c | 28 +++++++++------------------- drivers/usb/serial/generic.c | 29 +++++++++++++++++++++++++++++ drivers/usb/serial/io_ti.c | 22 ++++++++++++++-------- drivers/usb/serial/ti_usb_3410_5052.c | 23 +++++++++++++++-------- drivers/usb/serial/usb-serial.c | 30 +++++++++++++++++++++--------- include/linux/usb/serial.h | 4 ++++ 6 files changed, 92 insertions(+), 44 deletions(-)
Signature · 1 line
1.8.2.1
PATCH 1/7 USB: serial: add wait_until_sent operation
Add wait_until_sent operation which can be used to wait for hardware buffers to drain.
--- drivers/usb/serial/usb-serial.c | 17 +++++++++++++++++ include/linux/usb/serial.h | 1 + 2 files changed, 18 insertions(+)
drivers/usb/serial/usb-serial.c +17 -0
@@ -375,6 +375,22 @@ static int serial_chars_in_buffer(struct tty_struct *tty)return count; } +static void serial_wait_until_sent (struct tty_struct *tty,int timeout) +{ +struct usb_serial_port *port = tty->driver_data; +struct usb_serial *serial = port->serial; + +dev_dbg (tty->dev,"%s\n" , __func__); + +if (!port->serial->type->wait_until_sent) +return ; + +mutex_lock (&serial->disc_mutex); +if (!serial->disconnected) + port->serial->type->wait_until_sent (tty, timeout); +mutex_unlock (&serial->disc_mutex); +} +static void serial_throttle (struct tty_struct *tty) {struct usb_serial_port *port = tty->driver_data;
@@ -1191,6 +1207,7 @@ static const struct tty_operations serial_ops = { .unthrottle = serial_unthrottle, .break_ctl = serial_break, .chars_in_buffer = serial_chars_in_buffer, + .wait_until_sent = serial_wait_until_sent, .tiocmget = serial_tiocmget, .tiocmset = serial_tiocmset, .get_icount = serial_get_icount,
include/linux/usb/serial.h +1 -0
@@ -268,6 +268,7 @@ struct usb_serial_driver { struct usb_serial_port *port, struct ktermios *old);void (*break_ctl)(struct tty_struct *tty,int break_state);int (*chars_in_buffer)(struct tty_struct *tty); +void (*wait_until_sent)(struct tty_struct *tty,long timeout);void (*throttle)(struct tty_struct *tty);void (*unthrottle)(struct tty_struct *tty);int (*tiocmget)(struct tty_struct *tty);
Signature · 1 line
1.8.2.1
PATCH 2/7 USB: serial: add generic wait_until_sent implementation
In reply to Johan HovoldAdd generic wait_until_sent implementation which polls for empty hardware buffers using the new port-operation tx_empty.
The generic implementation will be used for all sub-drivers that implement tx_empty but does not define wait_until_sent.
--- drivers/usb/serial/generic.c | 29 +++++++++++++++++++++++++++++ drivers/usb/serial/usb-serial.c | 2 ++ include/linux/usb/serial.h | 3 +++ 3 files changed, 34 insertions(+)
drivers/usb/serial/generic.c +29 -0
@@ -253,6 +253,35 @@ int usb_serial_generic_chars_in_buffer(struct tty_struct *tty) }EXPORT_SYMBOL_GPL (usb_serial_generic_chars_in_buffer); +void usb_serial_generic_wait_until_sent (struct tty_struct *tty,long timeout) +{ +struct usb_serial_port *port = tty->driver_data; +unsigned int bps; +unsigned long period; +unsigned long expire; + +/* +* Use a poll-period of roughly the time it takes to send one +* character or at least one jiffy. +*/ + bps =tty_get_baud_rate (tty); + period = max_t(unsigned long, (10 * HZ / bps), 1); + period = min_t(unsigned long, period, timeout); + +dev_dbg (&port->dev,"%s - timeout = %u ms, period = %u ms\n" , + __func__,jiffies_to_msecs (timeout), +jiffies_to_msecs (period)); + expire = jiffies + timeout; +while (!port->serial->type->tx_empty (port)) { +schedule_timeout_interruptible (period); +if (signal_pending (current)) +break ; +if (time_after (jiffies, expire)) +break ; + } +} +EXPORT_SYMBOL_GPL (usb_serial_generic_wait_until_sent); +static int usb_serial_generic_submit_read_urb (struct usb_serial_port *port,int index,gfp_t mem_flags) {
drivers/usb/serial/usb-serial.c +2 -0
@@ -1333,6 +1333,8 @@ static void usb_serial_operations_init(struct usb_serial_driver *device)set_to_generic_if_null (device, close);set_to_generic_if_null (device, write_room);set_to_generic_if_null (device, chars_in_buffer); +if (device->tx_empty) +set_to_generic_if_null (device, wait_until_sent);set_to_generic_if_null (device, read_bulk_callback);set_to_generic_if_null (device, write_bulk_callback);set_to_generic_if_null (device, process_read_urb);
include/linux/usb/serial.h +3 -0
@@ -269,6 +269,7 @@ struct usb_serial_driver {void (*break_ctl)(struct tty_struct *tty,int break_state);int (*chars_in_buffer)(struct tty_struct *tty);void (*wait_until_sent)(struct tty_struct *tty,long timeout); +bool (*tx_empty)(struct usb_serial_port *port);void (*throttle)(struct tty_struct *tty);void (*unthrottle)(struct tty_struct *tty);int (*tiocmget)(struct tty_struct *tty);
@@ -328,6 +329,8 @@ extern void usb_serial_generic_close(struct usb_serial_port *port);extern int usb_serial_generic_resume (struct usb_serial *serial);extern int usb_serial_generic_write_room (struct tty_struct *tty);extern int usb_serial_generic_chars_in_buffer (struct tty_struct *tty); +extern void usb_serial_generic_wait_until_sent (struct tty_struct *tty, +long timeout);extern void usb_serial_generic_read_bulk_callback (struct urb *urb);extern void usb_serial_generic_write_bulk_callback (struct urb *urb);extern void usb_serial_generic_throttle (struct tty_struct *tty);
Signature · 6 lines
1.8.2.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] USB: serial: add generic wait_until_sent implementation
05.05.2013 22:32, Johan Hovold пишет:
Quoted 5 lines
Add generic
wait_until_sentimplementation which polls for empty hardware buffers using the new port-operationtx_empty.The generic implementation will be used for all sub-drivers that implement
tx_emptybut does not definewait_until_sent.
Hi Johan.
The customer reports an Oops below.
Does that ring any bells?
Well, there is only one division in usb_serial_generic_wait_until_sent(), anyway. :)
---
May 8 08:23:06 localhost kernel: [ 19.086679] divide error: 0000 [#1] SMP May 8 08:23:06 localhost kernel: [ 19.086801] Modules linked in: ip6t_REJECT nf_conntrack_ipv6 nf_conntrack_ipv4 nf_defrag_
ipv6 nf_defrag_ipv4 xt_state nf_conntrack ip6table_filter ip6_tables coretemp e1000e kvm snd_hda_codec_hdmi snd_hda_codec_real tek snd_hda_intel snd_hda_codec snd_hwdep snd_pcm crc32c_intel snd_page_alloc snd_timer iTCO_wdt pcspkr iTCO_vendor_support sn d i2c_i801 microcode ptp of_i2c usblp soundcore lpc_ich pps_core ftdi_sio raid1 i915 video i2c_algo_bit drm_kms_helper drm i2c _core
May 8 08:23:06 localhost kernel: [ 19.088296] CPU: 0 PID: 2554 Comm: dosemu.bin Not tainted 3.9.0+ #1 May 8 08:23:06 localhost kernel: [ 19.088344] Hardware name: MSI MS-9899/MS-9899, BIOS V1.0b13 11/30/2012 May 8 08:23:06 localhost kernel: [ 19.088393] task: f6b572c0 ti: f6a2c000 task.ti: f6a2c000 May 8 08:23:06 localhost kernel: [ 19.088440] EIP: 0060:[<c07f38df>] EFLAGS: 00010246 CPU: 0 May 8 08:23:06 localhost kernel: [ 19.088489] EIP is at usb_serial_generic_wait_until_sent+0x2f/0xe0 May 8 08:23:06 localhost kernel: [ 19.088537] EAX: 00002710 EBX: f6a40400 ECX: 00000000 EDX: 00000000 May 8 08:23:06 localhost kernel: [ 19.088585] ESI: 00000001 EDI: 7fffffff EBP: f6a2dd60 ESP: f6a2dd30 May 8 08:23:06 localhost kernel: [ 19.088634] DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068 May 8 08:23:06 localhost kernel: [ 19.088679] CR0: 80050033 CR2: 0818cf69 CR3: 3139b000 CR4: 000407d0 May 8 08:23:06 localhost kernel: [ 19.088725] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000 May 8 08:23:06 localhost kernel: [ 19.088774] DR6: ffff0ff0 DR7: 00000400 May 8 08:23:06 localhost kernel: [ 19.088843] Stack: May 8 08:23:06 localhost kernel: [ 19.088910] 00000015 f5fe2620 00000000 00000206 00000206 f6a2dd70 c07f3d88 f1a030c0 May 8 08:23:06 localhost kernel: [ 19.089305] f6a2dd60 f39bc180 f39bc1b8 f1b5e000 f6a2dd90 c07f11ed f39bc180 7fffffff May 8 08:23:06 localhost kernel: [ 19.089703] f6a2dd90 c07f147d 7fffffff f6a40400 00000015 7fffffff f1b5e000 7fffffff May 8 08:23:06 localhost kernel: [ 19.090096] Call Trace: May 8 08:23:06 localhost kernel: [ 19.090167] [<c07f3d88>] ? usb_serial_generic_chars_in_buffer+0x68/0xa0 May 8 08:23:06 localhost kernel: [ 19.090243] [<c07f11ed>] serial_wait_until_sent+0x7d/0xc0 May 8 08:23:06 localhost kernel: [ 19.090315] [<c07f147d>] ? serial_chars_in_buffer+0x3d/0x70 May 8 08:23:06 localhost kernel: [ 19.090389] [<c0715710>] tty_wait_until_sent+0xc0/0xe0 May 8 08:23:06 localhost kernel: [ 19.090464] [<c0716586>] ? tty_ldisc_deref+0x36/0x90 May 8 08:23:06 localhost kernel: [ 19.090537] [<c0715a5f>] set_termios+0x13f/0x260 May 8 08:23:06 localhost kernel: [ 19.090608] [<c0715f83>] tty_mode_ioctl+0x403/0x510 May 8 08:23:06 localhost kernel: [ 19.090681] [<c04fa4d9>] ? generic_file_aio_write+0xa9/0xd0 May 8 08:23:06 localhost kernel: [ 19.090754] [<c07160c0>] n_tty_ioctl_helper+0x30/0x100 May 8 08:23:06 localhost kernel: [ 19.090826] [<c0712af2>] n_tty_ioctl+0x32/0xe0 May 8 08:23:06 localhost kernel: [ 19.090898] [<c0712ac0>] ? n_tty_set_room+0x130/0x130 May 8 08:23:06 localhost kernel: [ 19.090971] [<c071113d>] tty_ioctl+0x27d/0xa40 May 8 08:23:06 localhost kernel: [ 19.091043] [<c0712ac0>] ? n_tty_set_room+0x130/0x130 May 8 08:23:06 localhost kernel: [ 19.091116] [<c051b360>] ? handle_pte_fault+0x80/0x7e0 May 8 08:23:06 localhost kernel: [ 19.091191] [<c0548217>] ? do_sync_write+0x97/0xd0 May 8 08:23:06 localhost kernel: [ 19.091263] [<c0710ec0>] ? no_tty+0x30/0x30 May 8 08:23:06 localhost kernel: [ 19.091335] [<c0557f47>] do_vfs_ioctl+0x77/0x590 May 8 08:23:06 localhost kernel: [ 19.091407] [<c054a5d1>] ? __sb_end_write+0x31/0x70 May 8 08:23:06 localhost kernel: [ 19.091479] [<c0548dd5>] ? vfs_write+0x165/0x1c0 May 8 08:23:06 localhost kernel: [ 19.091552] [<c05584d8>] SyS_ioctl+0x78/0x90 May 8 08:23:06 localhost kernel: [ 19.091623] [<c0985141>] sysenter_do_call+0x12/0x22 May 8 08:23:06 localhost kernel: [ 19.091694] Code: 56 53 83 e4 f8 83 ec 20 66 66 66 66 90 be 01 00 00 00 8b 98 78 01 00 00 83 e8 80 89 d7 e8 ba 13 f2 ff 31 d2 89 c1 b8 10 27 00 00 <f7> f1 85 c0 0f 45 f0 39 fe 0f 47 f7 f6 05 9a 00 c4 c0 04 75 49 May 8 08:23:06 localhost kernel: [ 19.094241] EIP: [<c07f38df>] usb_serial_generic_wait_until_sent+0x2f/0xe0 SS:ESP 0068:f6 a2dd30 May 8 08:23:06 localhost kernel: [ 19.094443] ---[ end trace 571b299bbe1655fd ]--- May 8 08:23:09 localhost lcdcontroller.sh[598]: Goodbye
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/7] USB: serial: add generic wait_until_sent implementation
Quoted 4 lines · Stas Sergeev
05.05.2013 22:32, Johan Hovold пишет:
Quoted 3 lines
Add generic
wait_until_sentimplementation which polls for empty hardware buffers using the new port-operationtx_empty.
Quoted 8 lines
Quoted 2 lines
The generic implementation will be used for all sub-drivers that implement
tx_emptybut does not definewait_until_sent.Hi Johan.
The customer reports an Oops below.
Does that ring any bells?
Well, there is only one division inusb_serial_generic_wait_until_sent(), anyway. :)
Yes, you're right I forgot about B0. I'll send a v2.
Thanks for catching this,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 2/8 USB: serial: add generic wait_until_sent implementation
In reply to Johan HovoldAdd generic wait_until_sent implementation which polls for empty hardware buffers using the new port-operation tx_empty.
The generic implementation will be used for all sub-drivers that implement tx_empty but does not define wait_until_sent.
---
v2: make sure to handle B0
drivers/usb/serial/generic.c | 31 +++++++++++++++++++++++++++++++ drivers/usb/serial/usb-serial.c | 2 ++ include/linux/usb/serial.h | 3 +++ 3 files changed, 36 insertions(+)
drivers/usb/serial/generic.c +31 -0
@@ -253,6 +253,37 @@ int usb_serial_generic_chars_in_buffer(struct tty_struct *tty) }EXPORT_SYMBOL_GPL (usb_serial_generic_chars_in_buffer); +void usb_serial_generic_wait_until_sent (struct tty_struct *tty,long timeout) +{ +struct usb_serial_port *port = tty->driver_data; +unsigned int bps; +unsigned long period; +unsigned long expire; + + bps =tty_get_baud_rate (tty); +if (!bps) + bps =9600 ;/* B0 */ +/* +* Use a poll-period of roughly the time it takes to send one +* character or at least one jiffy. +*/ + period = max_t(unsigned long, (10 * HZ / bps), 1); + period = min_t(unsigned long, period, timeout); + +dev_dbg (&port->dev,"%s - timeout = %u ms, period = %u ms\n" , + __func__,jiffies_to_msecs (timeout), +jiffies_to_msecs (period)); + expire = jiffies + timeout; +while (!port->serial->type->tx_empty (port)) { +schedule_timeout_interruptible (period); +if (signal_pending (current)) +break ; +if (time_after (jiffies, expire)) +break ; + } +} +EXPORT_SYMBOL_GPL (usb_serial_generic_wait_until_sent); +static int usb_serial_generic_submit_read_urb (struct usb_serial_port *port,int index,gfp_t mem_flags) {
drivers/usb/serial/usb-serial.c +2 -0
@@ -1333,6 +1333,8 @@ static void usb_serial_operations_init(struct usb_serial_driver *device)set_to_generic_if_null (device, close);set_to_generic_if_null (device, write_room);set_to_generic_if_null (device, chars_in_buffer); +if (device->tx_empty) +set_to_generic_if_null (device, wait_until_sent);set_to_generic_if_null (device, read_bulk_callback);set_to_generic_if_null (device, write_bulk_callback);set_to_generic_if_null (device, process_read_urb);
include/linux/usb/serial.h +3 -0
@@ -269,6 +269,7 @@ struct usb_serial_driver {void (*break_ctl)(struct tty_struct *tty,int break_state);int (*chars_in_buffer)(struct tty_struct *tty);void (*wait_until_sent)(struct tty_struct *tty,long timeout); +bool (*tx_empty)(struct usb_serial_port *port);void (*throttle)(struct tty_struct *tty);void (*unthrottle)(struct tty_struct *tty);int (*tiocmget)(struct tty_struct *tty);
@@ -328,6 +329,8 @@ extern void usb_serial_generic_close(struct usb_serial_port *port);extern int usb_serial_generic_resume (struct usb_serial *serial);extern int usb_serial_generic_write_room (struct tty_struct *tty);extern int usb_serial_generic_chars_in_buffer (struct tty_struct *tty); +extern void usb_serial_generic_wait_until_sent (struct tty_struct *tty, +long timeout);extern void usb_serial_generic_read_bulk_callback (struct urb *urb);extern void usb_serial_generic_write_bulk_callback (struct urb *urb);extern void usb_serial_generic_throttle (struct tty_struct *tty);
Signature · 1 line
1.8.2.1
PATCH 3/7 USB: ftdi_sio: clean up get_modem_status
In reply to Johan HovoldUse usb-serial port rather than tty as argument to get_modem_status.
--- drivers/usb/serial/ftdi_sio.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/usb/serial/ftdi_sio.c +4 -5
@@ -925,7 +925,7 @@ static int ftdi_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg);static void ftdi_break_ctl (struct tty_struct *tty,int break_state);static int ftdi_chars_in_buffer (struct tty_struct *tty); -static int ftdi_get_modem_status (struct tty_struct *tty, +static int ftdi_get_modem_status (struct usb_serial_port *port,unsigned char status[2 ]);static unsigned short int ftdi_232am_baud_base_to_divisor (int baud,int base);
@@ -2068,7 +2068,7 @@ static int ftdi_chars_in_buffer(struct tty_struct *tty) goto out;/* Check if hardware buffer is empty. */ - ret =ftdi_get_modem_status (tty, buf); + ret =ftdi_get_modem_status (port, buf);if (ret ==2 ) {if (!(buf[1 ] &FTDI_RS_TEMT )) chars =1 ;
@@ -2268,10 +2268,9 @@ no_c_cflag_changes:* Returns the number of status bytes retrieved (device dependant), or * negative error code. */ -static int ftdi_get_modem_status (struct tty_struct *tty, +static int ftdi_get_modem_status (struct usb_serial_port *port,unsigned char status[2 ]) { -struct usb_serial_port *port = tty->driver_data;struct ftdi_private *priv =usb_get_serial_port_data (port);unsigned char *buf;int len;
@@ -2336,7 +2335,7 @@ static int ftdi_tiocmget(struct tty_struct *tty)unsigned char buf[2 ];int ret; - ret =ftdi_get_modem_status (tty, buf); + ret =ftdi_get_modem_status (port, buf);if (ret <0 )return ret;
Signature · 1 line
1.8.2.1
PATCH 4/7 USB: ftdi_sio: fix chars_in_buffer overhead
In reply to Johan HovoldUse the new generic usb-serial wait_until_sent implementation to wait for hardware buffers to drain.
This removes the need to check the hardware buffers in chars_in_buffer and thus removes the overhead introduced by commit 6f602912 ("usb:
serial: ftdi_sio: Add missing chars_in_buffer function") without breaking tty_wait_until_sent (used by, for example, tcdrain, tcsendbreak and close).
--- drivers/usb/serial/ftdi_sio.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-)
drivers/usb/serial/ftdi_sio.c +5 -14
@@ -924,7 +924,7 @@ static int ftdi_tiocmset(struct tty_struct *tty,static int ftdi_ioctl (struct tty_struct *tty,unsigned int cmd,unsigned long arg);static void ftdi_break_ctl (struct tty_struct *tty,int break_state); -static int ftdi_chars_in_buffer (struct tty_struct *tty); +static bool ftdi_tx_empty (struct usb_serial_port *port);static int ftdi_get_modem_status (struct usb_serial_port *port,unsigned char status[2 ]);
@@ -961,7 +961,7 @@ static struct usb_serial_driver ftdi_sio_device = { .ioctl = ftdi_ioctl, .set_termios = ftdi_set_termios, .break_ctl = ftdi_break_ctl, - .chars_in_buffer = ftdi_chars_in_buffer, + .tx_empty = ftdi_tx_empty, };static struct usb_serial_driver *const serial_drivers[] = {
@@ -2056,27 +2056,18 @@ static void ftdi_break_ctl(struct tty_struct *tty, int break_state) } -static int ftdi_chars_in_buffer (struct tty_struct *tty) +static bool ftdi_tx_empty (struct usb_serial_port *port) { -struct usb_serial_port *port = tty->driver_data; -int chars;unsigned char buf[2 ];int ret; - chars =usb_serial_generic_chars_in_buffer (tty); -if (chars) - goto out; - -/* Check if hardware buffer is empty. */ ret =ftdi_get_modem_status (port, buf);if (ret ==2 ) {if (!(buf[1 ] &FTDI_RS_TEMT )) - chars =1 ; +return false; } -out: -dev_dbg (&port->dev,"%s - %d\n" , __func__, chars); -return chars; +return true; }/* old_termios contains the original termios settings and tty->termios contains
Signature · 1 line
1.8.2.1
PATCH 5/7 USB: io_ti: fix chars_in_buffer overhead
In reply to Johan HovoldUse the new generic usb-serial wait_until_sent implementation to wait for hardware buffers to drain.
This removes the need to check the hardware buffers in chars_in_buffer and thus removes the overhead introduced by commit 263e1f9f ("USB:io_ti: query hardware-buffer status in chars_in_buffer") without breaking tty_wait_until_sent (used by, for example, tcdrain, tcsendbreak and close).
--- drivers/usb/serial/io_ti.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
drivers/usb/serial/io_ti.c +14 -8
@@ -2019,8 +2019,6 @@ static int edge_chars_in_buffer(struct tty_struct *tty)struct edgeport_port *edge_port =usb_get_serial_port_data (port);int chars =0 ;unsigned long flags; -int ret; -if (edge_port ==NULL )return 0 ;
@@ -2028,16 +2026,22 @@ static int edge_chars_in_buffer(struct tty_struct *tty) chars =kfifo_len (&edge_port->write_fifo);spin_unlock_irqrestore (&edge_port->ep_lock, flags); -if (!chars) { - ret =tx_active (edge_port); -if (ret >0 ) - chars = ret; - } -dev_dbg (&port->dev,"%s - returns %d\n" , __func__, chars);return chars; } +static bool edge_tx_empty (struct usb_serial_port *port) +{ +struct edgeport_port *edge_port =usb_get_serial_port_data (port); +int ret; + + ret =tx_active (edge_port); +if (ret >0 ) +return false; + +return true; +} +static void edge_throttle (struct tty_struct *tty) {struct usb_serial_port *port = tty->driver_data;
@@ -2557,6 +2561,7 @@ static struct usb_serial_driver edgeport_1port_device = { .write = edge_write, .write_room = edge_write_room, .chars_in_buffer = edge_chars_in_buffer, + .tx_empty = edge_tx_empty, .break_ctl = edge_break, .read_int_callback = edge_interrupt_callback, .read_bulk_callback = edge_bulk_in_callback,
@@ -2589,6 +2594,7 @@ static struct usb_serial_driver edgeport_2port_device = { .write = edge_write, .write_room = edge_write_room, .chars_in_buffer = edge_chars_in_buffer, + .tx_empty = edge_tx_empty, .break_ctl = edge_break, .read_int_callback = edge_interrupt_callback, .read_bulk_callback = edge_bulk_in_callback,
Signature · 1 line
1.8.2.1
PATCH 6/7 USB: ti_usb_3410_5052: fix chars_in_buffer overhead
In reply to Johan HovoldUse the new generic usb-serial wait_until_sent implementation to wait for hardware buffers to drain.
This removes the need to check the hardware buffers in chars_in_buffer and thus removes the overhead introduced by commit 2c992cd73 ("USB:ti_usb_3410_5052: query hardware-buffer status in chars_in_buffer") without breaking tty_wait_until_sent (used by, for example, tcdrain, tcsendbreak and close).
--- drivers/usb/serial/ti_usb_3410_5052.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)
drivers/usb/serial/ti_usb_3410_5052.c +15 -8
@@ -101,6 +101,7 @@ static int ti_write(struct tty_struct *tty, struct usb_serial_port *port, const unsigned char *data, int count);static int ti_write_room (struct tty_struct *tty);static int ti_chars_in_buffer (struct tty_struct *tty); +static bool ti_tx_empty (struct usb_serial_port *port);static void ti_throttle (struct tty_struct *tty);static void ti_unthrottle (struct tty_struct *tty); static int ti_ioctl(struct tty_struct *tty,
@@ -222,6 +223,7 @@ static struct usb_serial_driver ti_1port_device = { .write = ti_write, .write_room = ti_write_room, .chars_in_buffer = ti_chars_in_buffer, + .tx_empty = ti_tx_empty, .throttle = ti_throttle, .unthrottle = ti_unthrottle, .ioctl = ti_ioctl,
@@ -253,6 +255,7 @@ static struct usb_serial_driver ti_2port_device = { .write = ti_write, .write_room = ti_write_room, .chars_in_buffer = ti_chars_in_buffer, + .tx_empty = ti_tx_empty, .throttle = ti_throttle, .unthrottle = ti_unthrottle, .ioctl = ti_ioctl,
@@ -684,8 +687,6 @@ static int ti_chars_in_buffer(struct tty_struct *tty)struct ti_port *tport =usb_get_serial_port_data (port);int chars =0 ;unsigned long flags; -int ret; -u8 lsr;if (tport ==NULL )return 0 ;
@@ -694,16 +695,22 @@ static int ti_chars_in_buffer(struct tty_struct *tty) chars =kfifo_len (&tport->write_fifo);spin_unlock_irqrestore (&tport->tp_lock, flags); -if (!chars) { - ret =ti_get_lsr (tport, &lsr); -if (!ret && !(lsr &TI_LSR_TX_EMPTY )) - chars =1 ; - } -dev_dbg (&port->dev,"%s - returns %d\n" , __func__, chars);return chars; } +static bool ti_tx_empty (struct usb_serial_port *port) +{ +struct ti_port *tport =usb_get_serial_port_data (port); +int ret; +u8 lsr; + + ret =ti_get_lsr (tport, &lsr); +if (!ret && !(lsr &TI_LSR_TX_EMPTY )) +return false; + +return true; +}static void ti_throttle (struct tty_struct *tty) {
Signature · 1 line
1.8.2.1
PATCH 7/7 USB: serial: clean up chars_in_buffer
In reply to Johan HovoldNo need to grab disconnect mutex in chars_in_buffer now that no sub-driver is or should be querying hardware buffers anymore. (They should use wait_until_sent.)
--- drivers/usb/serial/usb-serial.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)
drivers/usb/serial/usb-serial.c +2 -9
@@ -359,20 +359,13 @@ static int serial_chars_in_buffer(struct tty_struct *tty) {struct usb_serial_port *port = tty->driver_data;struct usb_serial *serial = port->serial; -int count =0 ;dev_dbg (tty->dev,"%s\n" , __func__); -mutex_lock (&serial->disc_mutex); -/* if the device was unplugged then any remaining characters -fell out of the connector ;) */ if (serial->disconnected) - count =0 ; -else - count = serial->type->chars_in_buffer (tty); -mutex_unlock (&serial->disc_mutex); +return 0 ; -return count; +return serial->type->chars_in_buffer (tty); }static void serial_wait_until_sent (struct tty_struct *tty,int timeout)
Signature · 1 line
1.8.2.1
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 0/7] USB: serial: add wait_until_sent-support
Quoted 3 lines · Caylan Larson
Johan,
I have tested these patches and the performance is much better. Thank you.
Thanks for testing. The patches are already in the usb tree (usb-linus branch) and should show up in v3.10-rc soon.
Johan
Quoted 20 lines
Tested-by Caylan Larson <[email protected]>Caylan
Quoted 13 lines · Johan Hovold
These patches add wait_until_sent-support to usb-serial, which removes the need to check hardware buffers in
chars_in_buffer.This fixes a problem in
ftdi_sio(since 3.7) where select or TIOCMOUTQ would take much longer than before due the hardware buffers being queried.Hardware buffers are also currently checked in
chars_in_bufferinio_ti(since 3.8) andti_usb_3410_5052(in 3.10).Note that simply removing the hardware-buffer checks (e.g. for the stable trees) would break
tty_wait_until_sent, which is used, for instance, by tcdrain, tcsendbreak, and close.
04.05.2013 15:15, Johan Hovold пишет:
Quoted 9 lines
The query takes longer than the transmit at decent baudrates (>=38k) and under the assumption that flow control isn't causing any delays.
But you do have a point, and I have been meaning to look into whether the added overhead of checking the hardware buffers could be mitigated by adding
wait_until_sentsupport to usb-serial. This way the we would only query the hardware buffers ontty_wait_until_sent(e.g. at close) and select and TIOCMOUTQ would not suffer. This is also the way things are handled inserial_core.
Thanks for taking a look.
Indeed, it seems .wait_until_sent is the best candidate for that kind of things, and the patch in question would even match its description be it using .wait_until_sent and not .chars_in_buffer. Please count on testing your patches here when they are ready. --
To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
04.05.2013 00:34, Greg KH пишет:
Quoted 1 line
Don't call select for every character :)
OK, let me draw an approximate workflow of the setup in question.
Lets say we have 3 tty devices, A, B and C.
A and B are blazingly fast, while C is a casual usb-serial chip.
The app must establish a bidirectional relay between A and C, making sure that the output queue on C never exceeds some margin M. B is used for acknowledges: the next char from A will arrive only after an acknowledge is sent via B.
Here's what the app approximately does:
1. select() on A and C for _input only_.
2. relay the char
3. if the char was from A, send ack to B and increment an ack counter (call that counter Q).
4. If Q>N (N is a threshold value that should be below M, currently 14), do TIOCOUTQ ioctl to make sure C is not overflowing, set Q to the value returned by TIOCOUTQ. If Q still above N, stop sending acks to B and do TIOCOUTQ periodically, until Q is below N, then resume the normal operations.
5. goto 1
So that's the workflow, and it used to work perfectly in the past. Now even on step 1, when select returns, there is already a big delay. Not to speak about TIOCOUTQ, a very funny way to query the buffer: the buffer is now entirely flushed while we query it.
What exactly is so horrible here that it was deserved to break? How to improve the algo? And no, we can't improve the protocol. For instance, we can't send multiple acks and hope that multiple chars will be received from A - the protocol cannot be changed. Any suggestions?
03.05.2013 20:52, Greg KH пишет:
Quoted 1 line
What exactly is the "problem" being seen?
OK, the problem is that while select() "stalls", the entire output queue is transmitted, and when the app writes more, there is already a big pause.
In fact, the app calls select() before writing every char, so then the data transfer is very slow.