You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "davids5 (via GitHub)" <gi...@apache.org> on 2023/02/16 11:46:44 UTC
[GitHub] [nuttx] davids5 opened a new pull request, #8544: mmsd fixes to ensure CPU is not busy waited
davids5 opened a new pull request, #8544:
URL: https://github.com/apache/nuttx/pull/8544
## Summary
mmcsd_removed will be called if the card is in invalid state.
This can happen if the card is bad, or vibrations causes a power
loss.
mmcsd_removed resets:
priv->capacity = 0; /* Capacity=0 sometimes means no media */
priv->blocksize = 0;
priv->probed = false;
priv->mediachanged = false;
priv->wrbusy = false;
priv->type = MMCSD_CARDTYPE_UNKNOWN;
priv->rca = 0;
priv->selblocklen = 0;
priv->widebus = false;
If blocksize is set to 0 will cause the log2 to result
in an infinate loop in some drivers.
IS_EMPTY will check for priv->type = MMCSD_CARDTYPE_UNKNOWN
and return ENODEV.
mmcsd_sdio:Release CPU during wait for Write Completion
Even if CONFIG_MMCSD_SDIOWAIT_WRCOMPLETE is set on an error the CPU could be busy waited for a second.
## Impact
System will not hang.
## Testing
PX4 px4_fmu_v5x: While doing SD testing using `logger` or `sd_bench`, short the CMD line during a command.
Prior to this change the write would fail (22) but the close would hang indefinitely.
--
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] [nuttx] davids5 commented on a diff in pull request #8544: mmsd fixes to ensure CPU is not busy waited
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #8544:
URL: https://github.com/apache/nuttx/pull/8544#discussion_r1108846445
##########
drivers/mmcsd/mmcsd_sdio.c:
##########
@@ -1268,6 +1268,10 @@ static int mmcsd_transferready(FAR struct mmcsd_state_s *priv)
goto errorout;
}
+ /* Do not hog the CPU */
+
+ nxsig_usleep(1000);
Review Comment:
> can we change CONFIG_MMCSD_SDIOWAIT_WRCOMPLETE from bool to int? so the user could select the different sleep time.
No - it is a separate setting.
--
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] [nuttx] davids5 commented on a diff in pull request #8544: mmsd fixes to ensure CPU is not busy waited
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #8544:
URL: https://github.com/apache/nuttx/pull/8544#discussion_r1108847308
##########
drivers/mmcsd/mmcsd_sdio.c:
##########
@@ -1268,6 +1268,10 @@ static int mmcsd_transferready(FAR struct mmcsd_state_s *priv)
goto errorout;
}
+ /* Do not hog the CPU */
+
+ nxsig_usleep(1000);
Review Comment:
> I'm fine with 1ms, just felt I need to ask it since you are more familiar with that part. We can go with Kconfig value as well, but I'm not sure if that will give us significant benefit.
Let's Take in as is. We can tune it if need be later.
--
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] [nuttx] xiaoxiang781216 merged pull request #8544: mmsd fixes to ensure CPU is not busy waited
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #8544:
URL: https://github.com/apache/nuttx/pull/8544
--
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] [nuttx] pkarashchenko commented on a diff in pull request #8544: mmsd fixes to ensure CPU is not busy waited
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #8544:
URL: https://github.com/apache/nuttx/pull/8544#discussion_r1108407197
##########
drivers/mmcsd/mmcsd_sdio.c:
##########
@@ -1268,6 +1268,10 @@ static int mmcsd_transferready(FAR struct mmcsd_state_s *priv)
goto errorout;
}
+ /* Do not hog the CPU */
+
+ nxsig_usleep(1000);
Review Comment:
Will this not limit the throughput? I mean, do we have an impact estimation? Maybe it is reasonable to go to some lower values like 100us that will be rounded to system tick period eventually, maybe may be tuned by system tick period at least.
In other words: what is the usual time until transmission is complete?
--
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] [nuttx] xiaoxiang781216 commented on a diff in pull request #8544: mmsd fixes to ensure CPU is not busy waited
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8544:
URL: https://github.com/apache/nuttx/pull/8544#discussion_r1108597122
##########
drivers/mmcsd/mmcsd_sdio.c:
##########
@@ -1268,6 +1268,10 @@ static int mmcsd_transferready(FAR struct mmcsd_state_s *priv)
goto errorout;
}
+ /* Do not hog the CPU */
+
+ nxsig_usleep(1000);
Review Comment:
can we change CONFIG_MMCSD_SDIOWAIT_WRCOMPLETE from bool to int? so the user could select the different sleep time.
--
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] [nuttx] pkarashchenko commented on a diff in pull request #8544: mmsd fixes to ensure CPU is not busy waited
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #8544:
URL: https://github.com/apache/nuttx/pull/8544#discussion_r1108650369
##########
drivers/mmcsd/mmcsd_sdio.c:
##########
@@ -1268,6 +1268,10 @@ static int mmcsd_transferready(FAR struct mmcsd_state_s *priv)
goto errorout;
}
+ /* Do not hog the CPU */
+
+ nxsig_usleep(1000);
Review Comment:
I'm fine with 1ms, just felt I need to ask it since you are more familiar with that part. We can go with Kconfig value as well, but I'm not sure if that will give us significant benefit.
--
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] [nuttx] xiaoxiang781216 commented on a diff in pull request #8544: mmsd fixes to ensure CPU is not busy waited
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8544:
URL: https://github.com/apache/nuttx/pull/8544#discussion_r1109385410
##########
drivers/mmcsd/mmcsd_sdio.c:
##########
@@ -1268,6 +1268,10 @@ static int mmcsd_transferready(FAR struct mmcsd_state_s *priv)
goto errorout;
}
+ /* Do not hog the CPU */
+
+ nxsig_usleep(1000);
Review Comment:
I am fine with the current state.
--
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] [nuttx] davids5 commented on a diff in pull request #8544: mmsd fixes to ensure CPU is not busy waited
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #8544:
URL: https://github.com/apache/nuttx/pull/8544#discussion_r1108451108
##########
drivers/mmcsd/mmcsd_sdio.c:
##########
@@ -1268,6 +1268,10 @@ static int mmcsd_transferready(FAR struct mmcsd_state_s *priv)
goto errorout;
}
+ /* Do not hog the CPU */
+
+ nxsig_usleep(1000);
Review Comment:
This code is executed on the next operation after a write that has ben committed but not completed by the card yet. So there is some delay consumed by the cpu running the time to the next cycle.
Write--------------------------------V Completion
Next(a)--------------------------V short wait
Next(b)-------------------------------------V wait no wait
> In other words: what is the usual time until transmission is complete?
This is usually short 1 ms to 250 MS or longer on some cards during wear-leveling
If thruput is an issue and system performance matters in the sense you do not want to busy wait. Then CONFIG_MMCSD_SDIOWAIT_WRCOMPLETE should be used. The CPU will wait on a semaphore for the completion interrupt from D0 and not busy wait. On the D0 completion, the loop will read the status and exit. With this setting there is 0 impact on thruput this has been tested with sd_bench. In the case of an SD failure it may wait for 1 Second. But now it will not a busy wait.
If CONFIG_MMCSD_SDIOWAIT_WRCOMPLETE is not set. Then nxsig_usleep time will be at the mercy of the tick and rounding. If we just yield we are also at the the mercy of the tick and task load.
Do you feel the 100 is really an advantage over 1000 on a 10 ms system, will it be on a 1 ms with quantization?
--
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] [nuttx] davids5 commented on a diff in pull request #8544: mmsd fixes to ensure CPU is not busy waited
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #8544:
URL: https://github.com/apache/nuttx/pull/8544#discussion_r1108451108
##########
drivers/mmcsd/mmcsd_sdio.c:
##########
@@ -1268,6 +1268,10 @@ static int mmcsd_transferready(FAR struct mmcsd_state_s *priv)
goto errorout;
}
+ /* Do not hog the CPU */
+
+ nxsig_usleep(1000);
Review Comment:
This code is executed on the next operation after a write that has been committed but not completed by the card yet. So there is some delay consumed by the cpu running the time to the next cycle.
Write--------------------------------V Completion
Next(a)--------------------------V short wait
Next(b)-------------------------------------V wait no wait
> In other words: what is the usual time until transmission is complete?
This is usually short 1 ms to 250 MS or longer on some cards during wear-leveling
If thruput is an issue and system performance matters in the sense you do not want to busy wait. Then CONFIG_MMCSD_SDIOWAIT_WRCOMPLETE should be used. The CPU will wait on a semaphore for the completion interrupt from D0 and not busy wait. On the D0 completion, the loop will read the status and exit. With this setting there is 0 impact on thruput this has been tested with sd_bench. In the case of an SD failure it may wait for 1 Second. But now it will not a busy wait.
If CONFIG_MMCSD_SDIOWAIT_WRCOMPLETE is not set. Then nxsig_usleep time will be at the mercy of the tick and rounding. If we just yield we are also at the the mercy of the tick and task load.
Do you feel the 100 is really an advantage over 1000 on a 10 ms system, will it be on a 1 ms with quantization?
--
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] [nuttx] davids5 commented on a diff in pull request #8544: mmsd fixes to ensure CPU is not busy waited
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #8544:
URL: https://github.com/apache/nuttx/pull/8544#discussion_r1109379419
##########
drivers/mmcsd/mmcsd_sdio.c:
##########
@@ -1268,6 +1268,10 @@ static int mmcsd_transferready(FAR struct mmcsd_state_s *priv)
goto errorout;
}
+ /* Do not hog the CPU */
+
+ nxsig_usleep(1000);
Review Comment:
@pkarashchenko @xiaoxiang781216 can we bring this in or do you want more changes?
--
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