You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/11/19 12:04:42 UTC
[GitHub] [incubator-nuttx] tidklaas opened a new pull request #2343: serial: Prevent RX stall
tidklaas opened a new pull request #2343:
URL: https://github.com/apache/incubator-nuttx/pull/2343
## Summary
Re-check RX queue status after uart_enablerxint() and before blocking
the reading task on the receive semaphore. cdcacm (and maybe other UART
drivers) can push buffered data into the receive queue during
uart_enablerxint(), leading to a blocked task while data is already
available.
## Impact
Prevents problems with tasks waiting for input that has already been received and queued.
## Testing
Tested with program reading short commands via ttyACM, which reliably hangs without this patch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a change in pull request #2343: serial: Prevent RX stall
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #2343:
URL: https://github.com/apache/incubator-nuttx/pull/2343#discussion_r527651572
##########
File path: drivers/serial/serial.c
##########
@@ -924,6 +924,20 @@ static ssize_t uart_read(FAR struct file *filep, FAR char *buffer, size_t buflen
/* Re-enable UART Rx interrupts */
uart_enablerxint(dev);
+
+ /* Check again if the RX buffer is empty. The UART driver
Review comment:
@xiaoxiang781216 but if it is not true at least for CDCACM then I think it is reasonable to include this fix, also it is just a double check, it will not bring side effects to existing drivers.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2343: serial: Prevent RX stall
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2343:
URL: https://github.com/apache/incubator-nuttx/pull/2343#discussion_r526927310
##########
File path: drivers/serial/serial.c
##########
@@ -924,6 +924,20 @@ static ssize_t uart_read(FAR struct file *filep, FAR char *buffer, size_t buflen
/* Re-enable UART Rx interrupts */
uart_enablerxint(dev);
+
+ /* Check again if the RX buffer is empty. The UART driver
Review comment:
so it's specific to cdcacm driver. I am fine with the change but it seem to redundant in most case, so it's better to add some comment to specify this particular case.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on a change in pull request #2343: serial: Prevent RX stall
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #2343:
URL: https://github.com/apache/incubator-nuttx/pull/2343#discussion_r527175361
##########
File path: drivers/serial/serial.c
##########
@@ -924,6 +924,20 @@ static ssize_t uart_read(FAR struct file *filep, FAR char *buffer, size_t buflen
/* Re-enable UART Rx interrupts */
uart_enablerxint(dev);
+
+ /* Check again if the RX buffer is empty. The UART driver
Review comment:
Hi @xiaoxiang781216, maybe if @tidklaas modify the comment to include that explanation will be enough!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2343: serial: Prevent RX stall
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2343:
URL: https://github.com/apache/incubator-nuttx/pull/2343#discussion_r527553464
##########
File path: drivers/serial/serial.c
##########
@@ -924,6 +924,20 @@ static ssize_t uart_read(FAR struct file *filep, FAR char *buffer, size_t buflen
/* Re-enable UART Rx interrupts */
uart_enablerxint(dev);
+
+ /* Check again if the RX buffer is empty. The UART driver
Review comment:
But, it's reasonable assumption that no more data receive after serial framework call rxint(false).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] tidklaas commented on a change in pull request #2343: serial: Prevent RX stall
Posted by GitBox <gi...@apache.org>.
tidklaas commented on a change in pull request #2343:
URL: https://github.com/apache/incubator-nuttx/pull/2343#discussion_r527535710
##########
File path: drivers/serial/serial.c
##########
@@ -924,6 +924,20 @@ static ssize_t uart_read(FAR struct file *filep, FAR char *buffer, size_t buflen
/* Re-enable UART Rx interrupts */
uart_enablerxint(dev);
+
+ /* Check again if the RX buffer is empty. The UART driver
Review comment:
CDCACM is just the one that tripped me up, I do not know if this issue lurks in any of the other UART drivers. I can certainly see a well-meaning developer go "Oh, let's save us a costly context switch by draining the RX FIFO before enabling the interrupt."
I guess the problem is that the semantics of the `uart_ops_s.rxint` function is not well defined. The include file just says _"Call to enable or disable RX interrupts"_ , while the expectation here seems to be _"Enable the interrupt, but do not queue any data until global interrupts are re-enabled"_.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2343: serial: Prevent RX stall
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2343:
URL: https://github.com/apache/incubator-nuttx/pull/2343#discussion_r527650718
##########
File path: drivers/serial/serial.c
##########
@@ -924,6 +924,20 @@ static ssize_t uart_read(FAR struct file *filep, FAR char *buffer, size_t buflen
/* Re-enable UART Rx interrupts */
uart_enablerxint(dev);
+
+ /* Check again if the RX buffer is empty. The UART driver
Review comment:
> Hi @xiaoxiang781216, maybe if @tidklaas modify the comment to include that explanation will be enough!
Yes, some explanation will help people understand the real sequence.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] tidklaas commented on a change in pull request #2343: serial: Prevent RX stall
Posted by GitBox <gi...@apache.org>.
tidklaas commented on a change in pull request #2343:
URL: https://github.com/apache/incubator-nuttx/pull/2343#discussion_r526879103
##########
File path: drivers/serial/serial.c
##########
@@ -924,6 +924,20 @@ static ssize_t uart_read(FAR struct file *filep, FAR char *buffer, size_t buflen
/* Re-enable UART Rx interrupts */
uart_enablerxint(dev);
+
+ /* Check again if the RX buffer is empty. The UART driver
Review comment:
`uart_disablerxint()` is called in line 898. For cdcacm this just sets a flag that will prevent it from copying data into the RX buffer, but it will still accept new data from the USB layer and put it into its completion queue. So if a USB packet arrives between `uart_disablerxint()` at line 898 and `enter_critical_section()` at line 922, there will be data waiting in cdcacm's queue. When `uart_enablerxint()` is called at line 926, [cdcuart_rxint](https://github.com/apache/incubator-nuttx/blob/6ee7fdf874d802f7853db1f7a382b6e4bce708fb/drivers/usbdev/cdcacm.c#L2582) will call [cdcacm_release_rxpending](https://github.com/apache/incubator-nuttx/blob/6ee7fdf874d802f7853db1f7a382b6e4bce708fb/drivers/usbdev/cdcacm.c#L667), which will copy the queued data into the RX buffer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2343: serial: Prevent RX stall
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #2343:
URL: https://github.com/apache/incubator-nuttx/pull/2343#discussion_r526851860
##########
File path: drivers/serial/serial.c
##########
@@ -924,6 +924,20 @@ static ssize_t uart_read(FAR struct file *filep, FAR char *buffer, size_t buflen
/* Re-enable UART Rx interrupts */
uart_enablerxint(dev);
+
+ /* Check again if the RX buffer is empty. The UART driver
Review comment:
but line 922 already disable CPU receiving any interrupt before enable rx irq. I am curious why the irq can still dispatch to your interrupt handler.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] tidklaas commented on a change in pull request #2343: serial: Prevent RX stall
Posted by GitBox <gi...@apache.org>.
tidklaas commented on a change in pull request #2343:
URL: https://github.com/apache/incubator-nuttx/pull/2343#discussion_r526879103
##########
File path: drivers/serial/serial.c
##########
@@ -924,6 +924,20 @@ static ssize_t uart_read(FAR struct file *filep, FAR char *buffer, size_t buflen
/* Re-enable UART Rx interrupts */
uart_enablerxint(dev);
+
+ /* Check again if the RX buffer is empty. The UART driver
Review comment:
`uart_disablerxint()` is called in line 898. For cdcacm this just sets a flag that will prevent it from copying data into the RX buffer, but it will still accept new data from the USB layer and put it into its completion queue. So if a USB packet arrives between `uart_disablerxint()` at line 898 and `enter_critical_section()` at line 922, there will be data waiting in cdcacm's queue. When `uart_enablerxint()` is called at line 926, [cdcuart_rxint](https://github.com/apache/incubator-nuttx/blob/6ee7fdf874d802f7853db1f7a382b6e4bce708fb/drivers/usbdev/cdcacm.c#L2582) will call [cdcacm_release_rxpending](https://github.com/apache/incubator-nuttx/blob/6ee7fdf874d802f7853db1f7a382b6e4bce708fb/drivers/usbdev/cdcacm.c#L667), which will copy the queued data unto the RX buffer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #2343: serial: Prevent RX stall
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #2343:
URL: https://github.com/apache/incubator-nuttx/pull/2343
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org