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 2022/03/16 06:38:59 UTC
[GitHub] [incubator-nuttx] SPRESENSE opened a new pull request #5752: arch: cxd56xx: Fix critical section in serial transmission
SPRESENSE opened a new pull request #5752:
URL: https://github.com/apache/incubator-nuttx/pull/5752
## Summary
Fix an issue that the serial transmission buffers are corrupted because
serial transmission are not protected by critical section in non-smp mode.
## Impact
Only for spresense board.
## Testing
spresense:nsh
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 edited a comment on pull request #5752: arch: cxd56xx: Fix critical section in serial transmission
Posted by GitBox <gi...@apache.org>.
Ouss4 edited a comment on pull request #5752:
URL: https://github.com/apache/incubator-nuttx/pull/5752#issuecomment-1069012395
Yes, I remember the deadlock, this is why we unlock the spinlock before calling `uart_xmitchars`:
https://github.com/apache/incubator-nuttx/blob/bbf12f3b1b2a8ba272a21db20c351ad7e41d3ecd/arch/xtensa/src/esp32/esp32_serial.c#L1801-L1802
`uart_xmitchars` will end up calling `up_txint` recursively, so without that it will deadlock.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull request #5752: arch: cxd56xx: Fix critical section in serial transmission
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #5752:
URL: https://github.com/apache/incubator-nuttx/pull/5752#discussion_r827895676
##########
File path: arch/arm/src/cxd56xx/cxd56_serial.c
##########
@@ -1001,9 +1001,13 @@ static void up_txint(FAR struct uart_dev_s *dev, bool enable)
* interrupts disabled (note this may recurse).
*/
+# ifdef CONFIG_SMP
spin_unlock_irqrestore(&priv->lock, flags);
Review comment:
@pkarashchenko
Yeah, but I've never seen such corruption in SMP.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull request #5752: arch: cxd56xx: Fix critical section in serial transmission
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #5752:
URL: https://github.com/apache/incubator-nuttx/pull/5752#discussion_r827898406
##########
File path: arch/arm/src/cxd56xx/cxd56_serial.c
##########
@@ -1001,9 +1001,13 @@ static void up_txint(FAR struct uart_dev_s *dev, bool enable)
* interrupts disabled (note this may recurse).
*/
+# ifdef CONFIG_SMP
spin_unlock_irqrestore(&priv->lock, flags);
Review comment:
@pkarashchenko
If we find such corruption in SMP mode in the future, I would revert the changes. (i.e. use critical section API instead of spinlock API for cxd56_serial.c)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #5752: arch: cxd56xx: Fix critical section in serial transmission
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5752:
URL: https://github.com/apache/incubator-nuttx/pull/5752#issuecomment-1069019365
>If we find such corruption in SMP mode in the future, I would revert the changes. (i.e. use critical section API instead of spinlock API for cxd56_serial.c)
Let me merge this PR.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #5752: arch: cxd56xx: Fix critical section in serial transmission
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #5752:
URL: https://github.com/apache/incubator-nuttx/pull/5752#issuecomment-1068974936
We discussed this part of code before with @masayuki2009, I think that there is an `#else` missing in the logic. It should be:
```c
#ifndef CONFIG_SUPPRESS_SERIAL_INTS
priv->ier |= UART_INTR_TX;
up_serialout(priv, CXD56_UART_IMSC, priv->ier);
#else
/* Fake a TX interrupt here by just calling uart_xmitchars() with
* interrupts disabled (note this may recurse).
*/
spin_unlock_irqrestore(&priv->lock, flags);
uart_xmitchars(dev);
flags = spin_lock_irqsave(&priv->lock);
#endif
```
We fixed it in some chips (ESP32 and STM32), but for CXD56 I think he got some issues.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 merged pull request #5752: arch: cxd56xx: Fix critical section in serial transmission
Posted by GitBox <gi...@apache.org>.
masayuki2009 merged pull request #5752:
URL: https://github.com/apache/incubator-nuttx/pull/5752
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5752: arch: cxd56xx: Fix critical section in serial transmission
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5752:
URL: https://github.com/apache/incubator-nuttx/pull/5752#discussion_r827731503
##########
File path: arch/arm/src/cxd56xx/cxd56_serial.c
##########
@@ -1001,9 +1001,13 @@ static void up_txint(FAR struct uart_dev_s *dev, bool enable)
* interrupts disabled (note this may recurse).
*/
+# ifdef CONFIG_SMP
spin_unlock_irqrestore(&priv->lock, flags);
Review comment:
what about SMP case? The buffer will not be corrupted?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on pull request #5752: arch: cxd56xx: Fix critical section in serial transmission
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5752:
URL: https://github.com/apache/incubator-nuttx/pull/5752#issuecomment-1069008050
>We discussed this part of code before with @masayuki2009, I think that there is an #else missing in the logic. It should
@Ouss4
I tried it before but a deadlock happened.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5752: arch: cxd56xx: Fix critical section in serial transmission
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5752:
URL: https://github.com/apache/incubator-nuttx/pull/5752#discussion_r827894155
##########
File path: arch/arm/src/cxd56xx/cxd56_serial.c
##########
@@ -1001,9 +1001,13 @@ static void up_txint(FAR struct uart_dev_s *dev, bool enable)
* interrupts disabled (note this may recurse).
*/
+# ifdef CONFIG_SMP
spin_unlock_irqrestore(&priv->lock, flags);
Review comment:
I mean that xmit from a single core in SMP mode should lead to the same situation as in single core isn't it?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #5752: arch: cxd56xx: Fix critical section in serial transmission
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #5752:
URL: https://github.com/apache/incubator-nuttx/pull/5752#issuecomment-1069012395
Yes, I remember the deadlock, this is why we unlock the spinlock before calling `uart_xmitchars`:
https://github.com/apache/incubator-nuttx/blob/bbf12f3b1b2a8ba272a21db20c351ad7e41d3ecd/arch/xtensa/src/esp32/esp32_serial.c#L1801-L1802
`uart_xmitchars` will ended up calling `up_txint` recursively, so without that it will deadlock.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 edited a comment on pull request #5752: arch: cxd56xx: Fix critical section in serial transmission
Posted by GitBox <gi...@apache.org>.
masayuki2009 edited a comment on pull request #5752:
URL: https://github.com/apache/incubator-nuttx/pull/5752#issuecomment-1069008050
>We discussed this part of code before with @masayuki2009, I think that there is an #else missing in the logic. It should
@Ouss4
I tried it before but a deadlock happened.
https://github.com/apache/incubator-nuttx/pull/4403#issuecomment-905071324
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull request #5752: arch: cxd56xx: Fix critical section in serial transmission
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #5752:
URL: https://github.com/apache/incubator-nuttx/pull/5752#discussion_r827890886
##########
File path: arch/arm/src/cxd56xx/cxd56_serial.c
##########
@@ -1001,9 +1001,13 @@ static void up_txint(FAR struct uart_dev_s *dev, bool enable)
* interrupts disabled (note this may recurse).
*/
+# ifdef CONFIG_SMP
spin_unlock_irqrestore(&priv->lock, flags);
Review comment:
@pkarashchenko
I've been testing spresense:wifi_smp and spresense:rndis_smp but I've never seen the corruption.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org