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 2021/03/07 04:13:42 UTC
[GitHub] [incubator-nuttx] antmerlino opened a new pull request #2989: Fixes race condition in event wait logic of SDMMC driver.
antmerlino opened a new pull request #2989:
URL: https://github.com/apache/incubator-nuttx/pull/2989
## Background
After some configuration changes, namely switching to tickless mode, I started to hit the DEBUGASSERTION here:
https://github.com/apache/incubator-nuttx/blob/master/arch/arm/src/stm32f7/stm32_sdmmc.c#L1460
Basically, the SDMMC driver timed out, but didn't expect to since wkupevents and waitevents are zero. Further debugging shows the event did finish and the timeout should not have occurred.
After @davids5 and I debugged further, we found 2 issues:
1. The first is addressed here. There is a race condition where your transfer can finish before you start your watchdog resulting in the watchdog not being cancelled and you timing out later unexpectedly.
2. The second is an issue @davids5 says he has some recollection of @xiaoxiang781216 identifying awhile back. Basically it's that enter_critical_section's are not being respected when calling into the watchdog in tickless mode. A task switch causes interrupts to be re-enabled and thus breaks the critical section. This sched switch and re-enabling of interrupts is actually the reason the watchdog wasn't getting started until after the the watchdog was started. Wrapping the wd_start in sched_lock() as a test fixed the issue. @xiaoxiang781216 Do you recall this? Is there an issue somewhere we could discuss solutions?
## Summary
This change fixes a race condition that was noted in a comment in the code. The fundamental issue was that the watchdog was being started after the transfer is started. Instead of passing in a timeout and starting the watchdog inside of SDIO_EVENTWAIT, we can simply do that in SDIO_WAITENABLE.
## Impact
- All arch with SDMMC support
- mmcsd_sdio driver
- include/nuttx/sdio.h
The SDMMC driver interface has changed slightly. Moving the timeout parameter from the SDIO_EVENTWAIT call to the SDIO_WAITENABLE call.
Because the SDIO_WAITENABLE call starts the watchdog, the caller must ensure SDIO_CANCEL is called if the caller is not going to continue to calling SDIO_EVENTWAIT.
## Testing
I have tested this on an STM32F765VI based platform and it addresses the race condition.
Kudos to @davids5 for helping debug this one!
----------------------------------------------------------------
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] davids5 commented on pull request #2989: Fixes race condition in event wait logic of SDMMC driver.
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2989:
URL: https://github.com/apache/incubator-nuttx/pull/2989#issuecomment-805796927
@acassis - after the rebase this is not ready to be merged, as we still need to resolve the issue, as reported above. I plan to retest this week.
--
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] davids5 commented on pull request #2989: Fixes race condition in event wait logic of SDMMC driver.
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2989:
URL: https://github.com/apache/incubator-nuttx/pull/2989#issuecomment-813653648
H7 - Now working
F7 - Still working
F4 - Still working
--
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] davids5 commented on pull request #2989: Fixes race condition in event wait logic of SDMMC driver.
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2989:
URL: https://github.com/apache/incubator-nuttx/pull/2989#issuecomment-792725869
@antmerlino MCU: STM32F76xxx, rev. Z - passes.
----------------------------------------------------------------
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] davids5 commented on pull request #2989: Fixes race condition in event wait logic of SDMMC driver.
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2989:
URL: https://github.com/apache/incubator-nuttx/pull/2989#issuecomment-792750741
@antmerlino MCU: STM32F42x, rev. 3 passes.
----------------------------------------------------------------
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 #2989: Fixes race condition in event wait logic of SDMMC driver.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #2989:
URL: https://github.com/apache/incubator-nuttx/pull/2989
--
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] davids5 commented on pull request #2989: Fixes race condition in event wait logic of SDMMC driver.
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2989:
URL: https://github.com/apache/incubator-nuttx/pull/2989#issuecomment-813652953
@antmerlino - Changes added - thank you for your guidance! Rebased on master, squashed and force pushed.
--
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] davids5 commented on pull request #2989: Fixes race condition in event wait logic of SDMMC driver.
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2989:
URL: https://github.com/apache/incubator-nuttx/pull/2989#issuecomment-792705271
I will test this today.
----------------------------------------------------------------
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 pull request #2989: Fixes race condition in event wait logic of SDMMC driver.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #2989:
URL: https://github.com/apache/incubator-nuttx/pull/2989#issuecomment-792203932
> 2. The second is an issue @davids5 says he has some recollection of @xiaoxiang781216 identifying awhile back. Basically it's that enter_critical_section's are not being respected when calling into the watchdog in tickless mode. A task switch causes interrupts to be re-enabled and thus breaks the critical section. This sched switch and re-enabling of interrupts is actually the reason the watchdog wasn't getting started until after the the watchdog was started. Wrapping the wd_start in sched_lock() as a test fixed the issue. @xiaoxiang781216 Do you recall this? Is there an issue somewhere we could discuss solutions?
>
Yes, here is the issue: https://github.com/apache/incubator-nuttx/issues/1138
Basically, enter_critical_section should stop any schedule until the thread initiatively give up the time slice by wait/sleep and should postone the schedule of the high priority thread waked up by the current thread.
----------------------------------------------------------------
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 pull request #2989: Fixes race condition in event wait logic of SDMMC driver.
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #2989:
URL: https://github.com/apache/incubator-nuttx/pull/2989#issuecomment-805794632
@antmerlino please fix the Conflicting files.
--
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] davids5 commented on pull request #2989: Fixes race condition in event wait logic of SDMMC driver.
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2989:
URL: https://github.com/apache/incubator-nuttx/pull/2989#issuecomment-792714728
@antmerlino - there are also some CI failures that need looking at.
----------------------------------------------------------------
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] antmerlino commented on pull request #2989: Fixes race condition in event wait logic of SDMMC driver.
Posted by GitBox <gi...@apache.org>.
antmerlino commented on pull request #2989:
URL: https://github.com/apache/incubator-nuttx/pull/2989#issuecomment-792889175
>
>
> @antmerlino - there are also some CI failures that need looking at.
Yeah, still trying to get them fixed.
----------------------------------------------------------------
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] davids5 commented on pull request #2989: Fixes race condition in event wait logic of SDMMC driver.
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #2989:
URL: https://github.com/apache/incubator-nuttx/pull/2989#issuecomment-813509161
@antmerlino
I am seeing corruption after a write on the H7/
```
nsh> ls /fs/microsd/
/fs/microsd:
.Trashes
etc/
.metadata_never_index
.fseventsd/
.Trash-1000
dataman
ufw/
uavcan.db/
log/
```
```
nsh> echo > /fs/microsd/etc/foo.txt
```
```
nsh> ls /fs/microsd/
/fs/microsd:
RRaA
```
Debugging it now....
--
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