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?
On Fri, May 03, 2013 at 07:02:46PM +0400, Stas Sergeev wrote:
> 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
What do you mean "slows down"?
> 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?
> 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 пишет:
> 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?
On Fri, May 03, 2013 at 09:38:50PM +0400, Stas Sergeev wrote:
> 03.05.2013 20:30, Greg KH пишет:
> >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?
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?
> 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.
What exactly is the "problem" being seen?
> 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?
Because I don't know what the problem really is :)
thanks,
greg k-h
03.05.2013 20:52, Greg KH пишет:
> On Fri, May 03, 2013 at 09:38:50PM +0400, Stas Sergeev wrote:
>> 03.05.2013 20:30, Greg KH пишет:
>>> 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?
> 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. :)
>> 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.
> 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.
>> 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?
> 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).
On Fri, May 03, 2013 at 10:05:55PM +0400, Stas Sergeev wrote:
> 03.05.2013 20:52, Greg KH пишет:
> >On Fri, May 03, 2013 at 09:38:50PM +0400, Stas Sergeev wrote:
> >>03.05.2013 20:30, Greg KH пишет:
> >>>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?
> >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,
Please define "much longer".
> 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?
> >>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.
> >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.
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?
> >>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?
> >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?
To actually determine how many characters the device has in its buffer.
thanks,
greg k-h
03.05.2013 20:52, Greg KH пишет:
> 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.
03.05.2013 21:16, Greg KH пишет:
> 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()...
> 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 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.
>>> 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.
> 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?
> 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?
On Fri, May 03, 2013 at 10:27:18PM +0400, Stas Sergeev wrote:
> 03.05.2013 21:16, Greg KH пишет:
>
> >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()...
I think that's the line dicipline doing this, not select itself.
> >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.
> >>>>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.
> >>>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.
> >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?
>
> >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.
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.
> 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
04.05.2013 00:34, Greg KH пишет:
> On Fri, May 03, 2013 at 10:27:18PM +0400, Stas Sergeev wrote:
>> 03.05.2013 21:16, Greg KH пишет:
>>
>>> 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()...
> 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?
>>> 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.
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?
>>>>>> 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.
>>>>> 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.
>>> 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?
>>
>>> 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.
> 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?
> 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...
04.05.2013 00:34, Greg KH пишет:
> 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?
On Sat, May 04, 2013 at 01:50:42AM +0400, Stas Sergeev wrote:
> 04.05.2013 00:34, Greg KH пишет:
> > On Fri, May 03, 2013 at 10:27:18PM +0400, Stas Sergeev wrote:
> >> 03.05.2013 21:16, Greg KH пишет:
[...]
> >>> 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.
> 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
On 05/04/2013 07:15 AM, Johan Hovold wrote:
> On Sat, May 04, 2013 at 01:50:42AM +0400, Stas Sergeev wrote:
>> 04.05.2013 00:34, Greg KH пишет:
>>> On Fri, May 03, 2013 at 10:27:18PM +0400, Stas Sergeev wrote:
>>>> 03.05.2013 21:16, Greg KH пишет:
>
> [...]
>
>>>>> 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.
>> 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.
Agreed. This is the correct solution.
> 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.)
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
04.05.2013 15:15, Johan Hovold пишет:
> 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.
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.
On Sat, May 04, 2013 at 07:39:57AM -0400, Peter Hurley wrote:
> On 05/04/2013 07:15 AM, Johan Hovold wrote:
> > On Sat, May 04, 2013 at 01:50:42AM +0400, Stas Sergeev wrote:
> >> 04.05.2013 00:34, Greg KH пишет:
> >>> On Fri, May 03, 2013 at 10:27:18PM +0400, Stas Sergeev wrote:
> >>>> 03.05.2013 21:16, Greg KH пишет:
[...]
> > 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.
>
> Agreed. This is the correct solution.
>
> > 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.)
>
> 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.
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
Use usb-serial port rather than tty as argument to get_modem_status.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/usb/serial/ftdi_sio.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 242b577..1159fd4 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -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;
--
1.8.2.1
Use 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).
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/usb/serial/ti_usb_3410_5052.c | 23 +++++++++++++++--------
1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
index cac47ae..c92c5ed 100644
--- a/drivers/usb/serial/ti_usb_3410_5052.c
+++ b/drivers/usb/serial/ti_usb_3410_5052.c
@@ -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)
{
--
1.8.2.1
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(-)
--
1.8.2.1
Add 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.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/usb/serial/generic.c | 29 +++++++++++++++++++++++++++++
drivers/usb/serial/usb-serial.c | 2 ++
include/linux/usb/serial.h | 3 +++
3 files changed, 34 insertions(+)
diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index 297665f..70ae019 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -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)
{
diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 31d2768..60caf9c 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -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);
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index afbb7ee..302ddf5 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -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);
--
1.8.2.1
Add wait_until_sent operation which can be used to wait for hardware
buffers to drain.
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/usb/serial/usb-serial.c | 17 +++++++++++++++++
include/linux/usb/serial.h | 1 +
2 files changed, 18 insertions(+)
diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index cf75beb..31d2768 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -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,
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index b9b0f7b4..afbb7ee 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -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);
--
1.8.2.1
Use 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).
Reported-by: Stas Sergeev <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/usb/serial/ftdi_sio.c | 19 +++++--------------
1 file changed, 5 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 1159fd4..a62a75a 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -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
--
1.8.2.1
No 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.)
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/usb/serial/usb-serial.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 60caf9c..4753c00 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -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)
--
1.8.2.1
Use 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).
Signed-off-by: Johan Hovold <[email protected]>
---
drivers/usb/serial/io_ti.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
index 158bf4b..1be6ba7 100644
--- a/drivers/usb/serial/io_ti.c
+++ b/drivers/usb/serial/io_ti.c
@@ -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,
--
1.8.2.1
05.05.2013 22:32, Johan Hovold пишет:
> Add 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.
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
On Wed, May 08, 2013 at 06:25:13PM +0400, Stas Sergeev wrote:
> 05.05.2013 22:32, Johan Hovold пишет:
> > Add 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.
> 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. :)
Yes, you're right I forgot about B0. I'll send a v2.
Thanks for catching this,
Johan
Add 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.
Signed-off-by: Johan Hovold <[email protected]>
---
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(+)
diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index 297665f..ba45170 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -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)
{
diff --git a/drivers/usb/serial/usb-serial.c b/drivers/usb/serial/usb-serial.c
index 31d2768..60caf9c 100644
--- a/drivers/usb/serial/usb-serial.c
+++ b/drivers/usb/serial/usb-serial.c
@@ -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);
diff --git a/include/linux/usb/serial.h b/include/linux/usb/serial.h
index afbb7ee..302ddf5 100644
--- a/include/linux/usb/serial.h
+++ b/include/linux/usb/serial.h
@@ -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);
--
1.8.2.1
On Fri, May 17, 2013 at 10:46:37AM -0500, Caylan Larson wrote:
> 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
> Tested-by: Caylan Larson <[email protected]>
>
> Caylan
>
>
> On May 5, 2013, at 1:32 PM, Johan Hovold <[email protected]> wrote:
>
> > 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.