sd: modify sdmmc_wait_ready to always decrement timeout #66243
Conversation
subsys/sd/sd_ops.c
Outdated
| } while (timeout > 0); | ||
| return 1; |
There was a problem hiding this comment.
Usual style
} while (timeout > 0);
return 1;Would it make sense to return ret and not just 1? Otherwise it is a bit confusing that is_ready returns 1 when device is not ready.
There was a problem hiding this comment.
What about returning -EBUSY? The comment for sdmmc_wait_ready indicates it should return 0 if the card is ready (to indicate the wait succeeded).
There was a problem hiding this comment.
Change applied. I've also updated the timeout logic, so that the timeout duration is a bit closer to CONFIG_SD_DATA_TIMEOUT in the case where the card is detached.
| } while (busy && (timeout > 0)); | ||
| return busy; | ||
| /* Delay 125us before polling again */ | ||
| k_busy_wait(125); |
There was a problem hiding this comment.
It could be but I don't think it's worth the extra Kconfig. Really this function is unlikely to wait exactly CONFIG_SD_DATA_TIMEOUT ms, because the actual delay will come down to how the SD host controller driver handles a slow card. The main goal here is simply that an SD card that is unresponsive (or dead) will timeout here, but a working but slow card will function
| /* Delay 125us before polling again */ | ||
| k_busy_wait(125); | ||
| timeout -= 125; | ||
| } while (timeout > 0); |
There was a problem hiding this comment.
CONFIG_SD_CMD_TIMEOUT being greater than ~22 seconds with CONFIG_SD_RETRY_COUNT something like 100 could result in integer overflow in this code, I'm not sure if this is a reasonably expected scenario though, it seems a little ridiculous, but I'm just pointing this out.
There was a problem hiding this comment.
Just in case if you think it's an issue you could use u32_add_overflow from math_extras.h
There was a problem hiding this comment.
$ west build -p always -b lpcxpresso55s69_cpu0 tests/drivers/disk/disk_access/ -DCONFIG_SD_CMD_TIMEOUT=22000 -DCONFIG_SD_RETRY_COUNT=10
[99/155] Building C object zephyr/subsys/sd/CMakeFiles/subsys__sd.dir/sd_ops.c.obj
/home/danieldegrasse/zephyr/zephyrproject/zephyr/subsys/sd/sd_ops.c: In function 'sdmmc_wait_ready':
/home/danieldegrasse/zephyr/zephyrproject/zephyr/subsys/sd/sd_ops.c:80:68: warning: integer overflow in expression of type 'int' results in '-2094967296' [-Woverflow]
80 | CONFIG_SD_RETRY_COUNT) * 1000;
Given that the compiler warns about it, I think we are ok to leave this as is.
There was a problem hiding this comment.
okay, makes sense, this compiler option will probably be kept on by default
There was a problem hiding this comment.
"As described in issue #65027, sdmmc_wait_ready can enter an"
Would be nice to have issue URL instead of Github reference.
As described in issue: #65027, sdmmc_wait_ready can enter an infinite loop if the card is removed while waiting for it to report an idle status. Fix this by always decrementing the timeout in sdmmc_wait_ready, regardless of whether the SD card is busy. Fixes #65027 Signed-off-by: Daniel DeGrasse <[email protected]>
The github UI refuses to display it, but I've updated the commit to include the URL. |
As described in issue #65027, sdmmc_wait_ready can enter an infinite loop if the card is removed while waiting for it to report an idle status. Fix this by always decrementing the timeout in sdmmc_wait_ready, regardless of whether the SD card is busy.
Fixes #65027