USB: serial: ftdi_sio: fix extreme low-latency setting

PATCH In review Johan Hovold 3 participants 6 messages Jan 25, 2017

Subsystems (3)

Stable branch USB USB serial

lore

Share & tools

lore

Johan Hovold

author maintainer

2017-01-25 14:35 UTC #0 lore raw

PATCH 1/1 USB: serial: ftdi_sio: fix extreme low-latency setting

Since commit 557aaa7ffab6 ("ft232: support the ASYNC_LOW_LATENCY flag") the FTDI driver has been using a receive latency-timer value of 1 ms instead of the device default of 16 ms.

The latency timer is used to periodically empty a non-full receive buffer, but a status header is always sent when the timer expires including when the buffer is empty. This means that a two-byte bulk message is received every millisecond also for an otherwise idle port as long as it is open.

Let's restore the pre-2009 behaviour which reduces rate of status messages to 1/16th (e.g. interrupt frequency drops from 1 kHz to 62.5 Hz) by not setting ASYNC_LOW_LATENCY by default.

Anyone willing to pay the price for the minimum-latency behaviour should set the flag explicitly instead using the TIOCSSERIAL ioctl or a tool such as setserial (e.g. setserial /dev/ttyUSB0 low_latency).

Note that since commit 0cbd81a9f6ba ("USB: ftdi_sio: remove tty->low_latency") the ASYNC_LOW_LATENCY flag has no other effects but to set a minimal latency timer.

Reported-by Antoine Aubert <[email protected]>

Fixes 557aaa7ffab6 ("ft232: support the ASYNC_LOW_LATENCY flag")

Cc stable <[email protected]> # v2.6.31

Signed-off-by Johan Hovold <[email protected]>

---

Greg,

I've been aware of this overhead for a while, but never realised it was actually a regression introduced in 2009.

Fixing something like this after such a long time obviously means risking a regression for anyone who is now relying on the new default behaviour instead. I still think it's reasonable in this case to restore the earlier behaviour given the penalty everyone else is paying for a minimal-latency behaviour that they likely do not need or want.

Whether this should go to stable is a different question. Perhaps the stable tag is not warranted, and this should just be the default behaviour going forward? What do you think?

Thanks,
Johan

 drivers/usb/serial/ftdi_sio.c | 2 --
 1 file changed, 2 deletions(-)

drivers/usb/serial/ftdi_sio.c +0 -2

index b064fad8e3ee..4bd556d9307d 100644
@@ -1805,8 +1805,6 @@ static int ftdi_sio_port_probe(struct usb_serial_port *port)
 
 	mutex_init(&priv->cfg_lock);
 
-	priv->flags = ASYNC_LOW_LATENCY;
-
 	if (quirk && quirk->port_probe)
 		quirk->port_probe(priv);

Signature · 1 line

2.10.2

Greg Kroah-Hartman

maintainer

2017-01-25 15:06 UTC #1 lore raw

Quoted 39 lines · Johan Hovold

Since commit 557aaa7ffab6 ("ft232: support the ASYNC_LOW_LATENCY flag") the FTDI driver has been using a receive latency-timer value of 1 ms instead of the device default of 16 ms.
The latency timer is used to periodically empty a non-full receive buffer, but a status header is always sent when the timer expires including when the buffer is empty. This means that a two-byte bulk message is received every millisecond also for an otherwise idle port as long as it is open.
Let's restore the pre-2009 behaviour which reduces rate of status messages to 1/16th (e.g. interrupt frequency drops from 1 kHz to 62.5 Hz) by not setting ASYNC_LOW_LATENCY by default.
Anyone willing to pay the price for the minimum-latency behaviour should set the flag explicitly instead using the TIOCSSERIAL ioctl or a tool such as setserial (e.g. setserial /dev/ttyUSB0 low_latency).
Note that since commit 0cbd81a9f6ba ("USB: ftdi_sio: remove tty->low_latency") the ASYNC_LOW_LATENCY flag has no other effects but to set a minimal latency timer.
Reported-by Antoine Aubert <[email protected]>
Fixes 557aaa7ffab6 ("ft232: support the ASYNC_LOW_LATENCY flag")
Cc stable <[email protected]> # v2.6.31
Signed-off-by Johan Hovold <[email protected]>
---
Greg,
I've been aware of this overhead for a while, but never realised it was actually a regression introduced in 2009.
Fixing something like this after such a long time obviously means risking a regression for anyone who is now relying on the new default behaviour instead. I still think it's reasonable in this case to restore the earlier behaviour given the penalty everyone else is paying for a minimal-latency behaviour that they likely do not need or want.

Quoted 3 lines

Whether this should go to stable is a different question. Perhaps the stable tag is not warranted, and this should just be the default behaviour going forward? What do you think?

I think the stable tag is warrented here. Do you want me to take this patch now into my usb-linus tree, or will you include it in a pull request?

thanks,

greg k-h

Johan Hovold

author maintainer

2017-01-25 15:14 UTC #2 lore raw

Quoted 40 lines · Greg Kroah-Hartman

Quoted 39 lines · Johan Hovold
Since commit 557aaa7ffab6 ("ft232: support the ASYNC_LOW_LATENCY flag") the FTDI driver has been using a receive latency-timer value of 1 ms instead of the device default of 16 ms.
The latency timer is used to periodically empty a non-full receive buffer, but a status header is always sent when the timer expires including when the buffer is empty. This means that a two-byte bulk message is received every millisecond also for an otherwise idle port as long as it is open.
Let's restore the pre-2009 behaviour which reduces rate of status messages to 1/16th (e.g. interrupt frequency drops from 1 kHz to 62.5 Hz) by not setting ASYNC_LOW_LATENCY by default.
Anyone willing to pay the price for the minimum-latency behaviour should set the flag explicitly instead using the TIOCSSERIAL ioctl or a tool such as setserial (e.g. setserial /dev/ttyUSB0 low_latency).
Note that since commit 0cbd81a9f6ba ("USB: ftdi_sio: remove tty->low_latency") the ASYNC_LOW_LATENCY flag has no other effects but to set a minimal latency timer.
Reported-by Antoine Aubert <[email protected]>
Fixes 557aaa7ffab6 ("ft232: support the ASYNC_LOW_LATENCY flag")
Cc stable <[email protected]> # v2.6.31
Signed-off-by Johan Hovold <[email protected]>
---
Greg,
I've been aware of this overhead for a while, but never realised it was actually a regression introduced in 2009.
Fixing something like this after such a long time obviously means risking a regression for anyone who is now relying on the new default behaviour instead. I still think it's reasonable in this case to restore the earlier behaviour given the penalty everyone else is paying for a minimal-latency behaviour that they likely do not need or want.

Quoted 7 lines

Quoted 3 lines
Whether this should go to stable is a different question. Perhaps the stable tag is not warranted, and this should just be the default behaviour going forward? What do you think?
I think the stable tag is warrented here. Do you want me to take this patch now into my usb-linus tree, or will you include it in a pull request?

I'll include it in a pull request. I was gonna apply it for -next, but if you prefer I can send it along with some new device ids I have queued up for 4.10-rc?

Thanks,
Johan

Greg Kroah-Hartman

maintainer

2017-01-25 15:41 UTC #3 lore raw

Quoted 45 lines · Johan Hovold

Quoted 44 lines · Greg Kroah-Hartman
Quoted 42 lines · Johan Hovold
Since commit 557aaa7ffab6 ("ft232: support the ASYNC_LOW_LATENCY flag") the FTDI driver has been using a receive latency-timer value of 1 ms instead of the device default of 16 ms.
The latency timer is used to periodically empty a non-full receive buffer, but a status header is always sent when the timer expires including when the buffer is empty. This means that a two-byte bulk message is received every millisecond also for an otherwise idle port as long as it is open.
Let's restore the pre-2009 behaviour which reduces rate of status messages to 1/16th (e.g. interrupt frequency drops from 1 kHz to 62.5 Hz) by not setting ASYNC_LOW_LATENCY by default.
Anyone willing to pay the price for the minimum-latency behaviour should set the flag explicitly instead using the TIOCSSERIAL ioctl or a tool such as setserial (e.g. setserial /dev/ttyUSB0 low_latency).
Note that since commit 0cbd81a9f6ba ("USB: ftdi_sio: remove tty->low_latency") the ASYNC_LOW_LATENCY flag has no other effects but to set a minimal latency timer.
Reported-by Antoine Aubert <[email protected]>
Fixes 557aaa7ffab6 ("ft232: support the ASYNC_LOW_LATENCY flag")
Cc stable <[email protected]> # v2.6.31
Signed-off-by Johan Hovold <[email protected]>
---
Greg,
I've been aware of this overhead for a while, but never realised it was actually a regression introduced in 2009.
Fixing something like this after such a long time obviously means risking a regression for anyone who is now relying on the new default behaviour instead. I still think it's reasonable in this case to restore the earlier behaviour given the penalty everyone else is paying for a minimal-latency behaviour that they likely do not need or want.
Whether this should go to stable is a different question. Perhaps the stable tag is not warranted, and this should just be the default behaviour going forward? What do you think?

Quoted 7 lines

Quoted 3 lines
I think the stable tag is warrented here. Do you want me to take this patch now into my usb-linus tree, or will you include it in a pull request?
I'll include it in a pull request. I was gonna apply it for -next, but if you prefer I can send it along with some new device ids I have queued up for 4.10-rc?

-next is probably good, it's not like there is a rush for it :)

Johan Hovold

author maintainer

2017-01-25 16:02 UTC #4 lore raw

Quoted 50 lines · Greg Kroah-Hartman

Quoted 49 lines · Johan Hovold
Quoted 47 lines · Greg Kroah-Hartman
Quoted 39 lines · Johan Hovold
Since commit 557aaa7ffab6 ("ft232: support the ASYNC_LOW_LATENCY flag") the FTDI driver has been using a receive latency-timer value of 1 ms instead of the device default of 16 ms.
The latency timer is used to periodically empty a non-full receive buffer, but a status header is always sent when the timer expires including when the buffer is empty. This means that a two-byte bulk message is received every millisecond also for an otherwise idle port as long as it is open.
Let's restore the pre-2009 behaviour which reduces rate of status messages to 1/16th (e.g. interrupt frequency drops from 1 kHz to 62.5 Hz) by not setting ASYNC_LOW_LATENCY by default.
Anyone willing to pay the price for the minimum-latency behaviour should set the flag explicitly instead using the TIOCSSERIAL ioctl or a tool such as setserial (e.g. setserial /dev/ttyUSB0 low_latency).
Note that since commit 0cbd81a9f6ba ("USB: ftdi_sio: remove tty->low_latency") the ASYNC_LOW_LATENCY flag has no other effects but to set a minimal latency timer.
Reported-by Antoine Aubert <[email protected]>
Fixes 557aaa7ffab6 ("ft232: support the ASYNC_LOW_LATENCY flag")
Cc stable <[email protected]> # v2.6.31
Signed-off-by Johan Hovold <[email protected]>
---
Greg,
I've been aware of this overhead for a while, but never realised it was actually a regression introduced in 2009.
Fixing something like this after such a long time obviously means risking a regression for anyone who is now relying on the new default behaviour instead. I still think it's reasonable in this case to restore the earlier behaviour given the penalty everyone else is paying for a minimal-latency behaviour that they likely do not need or want.
Quoted 3 lines
Whether this should go to stable is a different question. Perhaps the stable tag is not warranted, and this should just be the default behaviour going forward? What do you think?
I think the stable tag is warrented here. Do you want me to take this patch now into my usb-linus tree, or will you include it in a pull request?

Quoted 5 lines

Quoted 3 lines
I'll include it in a pull request. I was gonna apply it for -next, but if you prefer I can send it along with some new device ids I have queued up for 4.10-rc?
-next is probably good, it's not like there is a rush for it :)

True. Don't really wanna think about how much power has been wasted on these insane interrupt rates considering the number of these devices that are out there though...

Now applied for -next, with a stable dependency on patch already in my tree which did not need to be backported while ASYNC_LOW_LATENCY was set.

Thanks,
Johan

David Laight

2017-01-25 15:42 UTC #5 lore raw

In reply to Johan Hovold

From: Johan Hovold

Quoted 10 lines

Sent: 25 January 2017 14:35
Since commit 557aaa7ffab6 ("ft232: support the ASYNC_LOW_LATENCY flag") the FTDI driver has been using a receive latency-timer value of 1 ms instead of the device default of 16 ms.
The latency timer is used to periodically empty a non-full receive buffer, but a status header is always sent when the timer expires including when the buffer is empty. This means that a two-byte bulk message is received every millisecond also for an otherwise idle port as long as it is open.

...

I'd wondered what all those packets were.
It seems beyond stupid that the hardware is sending them at all.

David