Skip to content

drivers/sdmmc_sdhc: add timeout to wait for ISR mutex unlock #21432

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jun 3, 2025
Merged

drivers/sdmmc_sdhc: add timeout to wait for ISR mutex unlock #21432

merged 2 commits into from
Jun 3, 2025

Conversation

fabian18
Copy link
Contributor

@fabian18 fabian18 commented Apr 24, 2025 •
edited
Loading

Contribution description

I witnessed situations where the sdmmc driver stalls while waiting for an IRQ.
It might be hardware though ...
However even if it is hardware, it keeps going with this changes in this PR.

Testing procedure

Internal app where data is written to an eMMC and I keep pressing ENTER to continuously execute vfs ls.

> 2025-04-24 10:31:28,805 # vfs ls /sd0/data
2025-04-24 10:31:28,821 # RPOS.TXT	4 B
2025-04-24 10:31:28,821 # 0.CNK	5559 B
2025-04-24 10:31:28,821 # 1.CNK	4181 B
2025-04-24 10:31:28,821 # 2.CNK	696 B
2025-04-24 10:31:28,821 # total 4 files
> 2025-04-24 10:31:28,837 # vfs ls /sd0/data
2025-04-24 10:31:30,853 # [ERROR] [sdmmc] Card not present
2025-04-24 10:31:30,853 # vfs_opendir error: -EIO
> 2025-04-24 10:31:30,853 # vfs ls /sd0/data
2025-04-24 10:31:30,853 # vfs_opendir error: -EIO
> 2025-04-24 10:31:30,869 # vfs ls /sd0/data
2025-04-24 10:31:31,077 # RPOS.TXT	4 B
2025-04-24 10:31:31,093 # 0.CNK	5559 B
2025-04-24 10:31:31,093 # 1.CNK	4181 B
2025-04-24 10:31:31,093 # 2.CNK	1385 B
2025-04-24 10:31:31,094 # total 4 files
> 2025-04-24 10:31:31,094 # vfs ls /sd0/data
2025-04-24 10:31:31,094 # RPOS.TXT	4 B
2025-04-24 10:31:31,109 # 0.CNK	5559 B
2025-04-24 10:31:31,109 # 1.CNK	4181 B
2025-04-24 10:31:31,124 # 2.CNK	1385 B
2025-04-24 10:31:31,125 # total 4 files
> 2025-04-24 10:31:31,125 # vfs ls /sd0/dvfs ls /sd0/data

Without the change it was stalling. With this change it keeps working afterwards.

Issues/PRs references

@github-actions github-actions bot added the Area: drivers Area: Device drivers label Apr 24, 2025
crasbe
crasbe reviewed Apr 24, 2025
View reviewed changes
drivers/sdmmc/sdmmc_sdhc.c Outdated
Comment on lines 82 to 83
#define _ZTIMER_CLOCK ZTIMER_MSEC
#define _ZTIMER_TICKS_PER_MS 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#define _ZTIMER_CLOCK ZTIMER_MSEC
#define _ZTIMER_TICKS_PER_MS 1
# define _ZTIMER_CLOCK ZTIMER_MSEC
# define _ZTIMER_TICKS_PER_MS 1

drivers/sdmmc/sdmmc_sdhc.c Outdated
Comment on lines 86 to 87
#define _ZTIMER_CLOCK ZTIMER_USEC
#define _ZTIMER_TICKS_PER_MS US_PER_MS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#define _ZTIMER_CLOCK ZTIMER_USEC
#define _ZTIMER_TICKS_PER_MS US_PER_MS
# define _ZTIMER_CLOCK ZTIMER_USEC
# define _ZTIMER_TICKS_PER_MS US_PER_MS

I'm not sure if it makes sense to partially apply the indentation style now?
OR you could use the opportunity to apply it to the other #if-#else statements as well.

Ping @maribu?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @maribu

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm always happy when we add the indent. But as long as the CI does not enforce this, it is not really mandatory.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I would follow @crasbe's suggestion to use indentation at least for new #if ... #endif directives. If the file is touched anyway, it would be easy for this file to introduce indentation also for existing #if ... #endif directives, but I would leave that decision to @fabian18.

@crasbe
Copy link
Contributor

crasbe commented Apr 24, 2025

One small thing: The title and commit message should have a slash instead of colon. So drivers/sdmmc_sdhc: add timeout to wait for ISR mutex unlock.

@fabian18 fabian18 changed the title drivers:sdmmc_sdhc: add timeout to wait for ISR mutex unlock drivers/sdmmc_sdhc: add timeout to wait for ISR mutex unlock Apr 24, 2025
@fabian18 fabian18 force-pushed the fix/sdmmc_sdhc_irq_timeout branch from 48b1ae9 to 7ffe014 Compare April 24, 2025 12:20
@crasbe crasbe added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 24, 2025
@benpicco benpicco requested a review from gschorcht April 24, 2025 13:08
@riot-ci
Copy link

riot-ci commented Apr 24, 2025 •
edited
Loading

Murdock results

✔️ PASSED

94286a3 drivers/sdmmc_sdhc: preprocessor directive indentation

Success Failures Total Runtime
10380 0 10382 14m:46s

Artifacts

crasbe
crasbe reviewed May 14, 2025
View reviewed changes
drivers/sdmmc/sdmmc_sdhc.c Outdated
@@ -832,7 +841,7 @@ static int _sdhc_to_sdmmc_err_code(uint16_t code)
static void _isr(sdhc_dev_t *sdhc_dev)
{
sdhc_t *sdhc = sdhc_dev->conf->sdhc;

DEBUG_PUTS("[sdmmc] IRQ");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably a leftover from debugging, but it probably shouldn't stay.

gschorcht
gschorcht reviewed May 23, 2025
View reviewed changes
drivers/sdmmc/sdmmc_sdhc.c Outdated
Comment on lines 760 to 768
if (timeout || (sdhc_dev->error & error_mask)) {
if (IS_USED(ENABLE_DEBUG)) {
if (timeout) {
DEBUG("[sdmmc] IRQ wait timeout\n");
}
DEBUG("[sdmmc] SDHC error: EISTR=%04x, ", sdhc_dev->error);
switch (reset) {
case SDHC_SRR_SWRSTCMD:
Copy link
Contributor

@gschorcht gschorcht May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

???

Suggested change
if (timeout || (sdhc_dev->error & error_mask)) {
if (IS_USED(ENABLE_DEBUG)) {
if (timeout) {
DEBUG("[sdmmc] IRQ wait timeout\n");
}
DEBUG("[sdmmc] SDHC error: EISTR=%04x, ", sdhc_dev->error);
switch (reset) {
case SDHC_SRR_SWRSTCMD:
if (timeout || (sdhc_dev->error & error_mask)) {
if (IS_USED(ENABLE_DEBUG)) {
if (timeout) {
DEBUG("[sdmmc] IRQ wait timeout\n");
}
else {
DEBUG("[sdmmc] SDHC error: EISTR=%04x, ", sdhc_dev->error);
switch (reset) {
case SDHC_SRR_SWRSTCMD:
...

Either a timeout happend or an SDHC error happened, right? If so, the DEBUG message for SDHC error shouldn't be generated in case of timeout error.

@gschorcht gschorcht added the State: waiting for author State: Action by the author of the PR is required label May 23, 2025
@crasbe crasbe removed the State: waiting for author State: Action by the author of the PR is required label May 29, 2025
gschorcht
gschorcht approved these changes May 30, 2025
View reviewed changes
Copy link
Contributor

@gschorcht gschorcht left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, please quash.

@fabian18 fabian18 force-pushed the fix/sdmmc_sdhc_irq_timeout branch from 562958b to 94286a3 Compare June 2, 2025 07:27
@fabian18 fabian18 added this pull request to the merge queue Jun 3, 2025
@crasbe crasbe removed this pull request from the merge queue due to a manual request Jun 3, 2025
@crasbe crasbe added this pull request to the merge queue Jun 3, 2025
Merged via the queue into RIOT-OS:master with commit e567db6 Jun 3, 2025
25 checks passed
@Teufelchen1 Teufelchen1 added this to the Release 2025.07 milestone Jul 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crasbe crasbe crasbe left review comments

@maribu maribu maribu left review comments

@gschorcht gschorcht gschorcht approved these changes

Assignees
No one assigned
Labels
Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2025.07
Development

Successfully merging this pull request may close these issues.
6 participants
@fabian18 @crasbe @riot-ci @maribu @gschorcht @Teufelchen1