Skip to content

Commit

Permalink
Fixes race condition in event wait logic of SDMMC driver.
Browse files Browse the repository at this point in the history 
This change makes it so that the timeout is set as part of the SDIO_WAITENABLE call instead of the SDIO_EVENTWAIT call. By doing so, you eliminate all opportunity for a race condition.

stm32h7:sdmmc Check if busy ended early
  • Loading branch information
@antmerlino @davids5
antmerlino authored and davids5 committed Apr 5, 2021
1 parent 807d449 commit 8632a38
Show file tree
Hide file tree
Showing 17 changed files with 509 additions and 534 deletions.
60 changes: 30 additions & 30 deletions arch/arm/src/cxd56xx/cxd56_sdhci.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -398,9 +398,8 @@ static int cxd56_sdio_recvshort(FAR struct sdio_dev_s *dev, uint32_t cmd,
/* EVENT handler */

static void cxd56_sdio_waitenable(FAR struct sdio_dev_s *dev,
sdio_eventset_t eventset);
static sdio_eventset_t cxd56_sdio_eventwait(FAR struct sdio_dev_s *dev,
uint32_t timeout);
sdio_eventset_t eventset, uint32_t timeout);
static sdio_eventset_t cxd56_sdio_eventwait(FAR struct sdio_dev_s *dev);
static void cxd56_sdio_callbackenable(FAR struct sdio_dev_s *dev,
sdio_eventset_t eventset);
static int cxd56_sdio_registercallback(FAR struct sdio_dev_s *dev,
Expand Down Expand Up @@ -2494,7 +2493,7 @@ static int cxd56_sdio_recvshort(FAR struct sdio_dev_s *dev, uint32_t cmd,
****************************************************************************/

static void cxd56_sdio_waitenable(FAR struct sdio_dev_s *dev,
sdio_eventset_t eventset)
sdio_eventset_t eventset, uint32_t timeout)
{
struct cxd56_sdiodev_s *priv = (struct cxd56_sdiodev_s *)dev;
uint32_t waitints;
Expand Down Expand Up @@ -2523,6 +2522,32 @@ static void cxd56_sdio_waitenable(FAR struct sdio_dev_s *dev,
/* Enable event-related interrupts */

cxd56_configwaitints(priv, waitints, eventset, 0);

/* Check if the timeout event is specified in the event set */

if ((priv->waitevents & SDIOWAIT_TIMEOUT) != 0)
{
int delay;
int ret;

/* Yes.. Handle a corner case */

if (!timeout)
{
priv->wkupevent = SDIOWAIT_TIMEOUT;
return;
}

/* Start the watchdog timer */

delay = MSEC2TICK(timeout);
ret = wd_start(&priv->waitwdog, delay,
cxd56_eventtimeout, (wdparm_t)priv);
if (ret != OK)
{
mcerr("ERROR: wd_start failed: %d\n", ret);
}
}
}

/****************************************************************************
Expand All @@ -2546,8 +2571,7 @@ static void cxd56_sdio_waitenable(FAR struct sdio_dev_s *dev,
*
****************************************************************************/

static sdio_eventset_t cxd56_sdio_eventwait(FAR struct sdio_dev_s *dev,
uint32_t timeout)
static sdio_eventset_t cxd56_sdio_eventwait(FAR struct sdio_dev_s *dev)
{
struct cxd56_sdiodev_s *priv = (struct cxd56_sdiodev_s *)dev;
sdio_eventset_t wkupevent = 0;
Expand All @@ -2561,30 +2585,6 @@ static sdio_eventset_t cxd56_sdio_eventwait(FAR struct sdio_dev_s *dev,
DEBUGASSERT((priv->waitevents != 0 && priv->wkupevent == 0) ||
(priv->waitevents == 0 && priv->wkupevent != 0));

/* Check if the timeout event is specified in the event set */

if ((priv->waitevents & SDIOWAIT_TIMEOUT) != 0)
{
int delay;

/* Yes.. Handle a corner case */

if (!timeout)
{
return SDIOWAIT_TIMEOUT;
}

/* Start the watchdog timer */

delay = MSEC2TICK(timeout);
ret = wd_start(&priv->waitwdog, delay,
cxd56_eventtimeout, (wdparm_t)priv);
if (ret != OK)
{
mcerr("ERROR: wd_start failed: %d\n", ret);
}
}

/* Loop until the event (or the timeout occurs). Race conditions are
* avoided by calling cxd56_waitenable prior to triggering the logic that
* will cause the wait to terminate. Under certain race conditions, the
Expand Down
62 changes: 31 additions & 31 deletions arch/arm/src/imxrt/imxrt_usdhc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,9 +350,8 @@ static int imxrt_recvshort(FAR struct sdio_dev_s *dev, uint32_t cmd,
/* EVENT handler */

static void imxrt_waitenable(FAR struct sdio_dev_s *dev,
sdio_eventset_t eventset);
static sdio_eventset_t imxrt_eventwait(FAR struct sdio_dev_s *dev,
uint32_t timeout);
sdio_eventset_t eventset, uint32_t timeout);
static sdio_eventset_t imxrt_eventwait(FAR struct sdio_dev_s *dev);
static void imxrt_callbackenable(FAR struct sdio_dev_s *dev,
sdio_eventset_t eventset);
static int imxrt_registercallback(FAR struct sdio_dev_s *dev,
Expand Down Expand Up @@ -2627,7 +2626,7 @@ static int imxrt_recvshort(FAR struct sdio_dev_s *dev, uint32_t cmd,
****************************************************************************/

static void imxrt_waitenable(FAR struct sdio_dev_s *dev,
sdio_eventset_t eventset)
sdio_eventset_t eventset, uint32_t timeout)
{
struct imxrt_dev_s *priv = (struct imxrt_dev_s *)dev;
uint32_t waitints;
Expand Down Expand Up @@ -2660,6 +2659,33 @@ static void imxrt_waitenable(FAR struct sdio_dev_s *dev,
/* Enable event-related interrupts */

imxrt_configwaitints(priv, waitints, eventset, 0);

/* Check if the timeout event is specified in the event set */

if ((priv->waitevents & SDIOWAIT_TIMEOUT) != 0)
{
int delay;
int ret;

/* Yes.. Handle a corner case */

if (!timeout)
{
priv->wkupevent = SDIOWAIT_TIMEOUT;
return;
}

/* Start the watchdog timer */

delay = MSEC2TICK(timeout);
ret = wd_start(&priv->waitwdog, delay,
imxrt_eventtimeout, (wdparm_t)priv);

if (ret < 0)
{
mcerr("ERROR: wd_start failed: %d\n", ret);
}
}
}

/****************************************************************************
Expand All @@ -2683,8 +2709,7 @@ static void imxrt_waitenable(FAR struct sdio_dev_s *dev,
*
****************************************************************************/

static sdio_eventset_t imxrt_eventwait(FAR struct sdio_dev_s *dev,
uint32_t timeout)
static sdio_eventset_t imxrt_eventwait(FAR struct sdio_dev_s *dev)
{
struct imxrt_dev_s *priv = (struct imxrt_dev_s *)dev;
sdio_eventset_t wkupevent = 0; int ret;
Expand All @@ -2698,31 +2723,6 @@ static sdio_eventset_t imxrt_eventwait(FAR struct sdio_dev_s *dev,
DEBUGASSERT((priv->waitevents != 0 && priv->wkupevent == 0) ||
(priv->waitevents == 0 && priv->wkupevent != 0));

/* Check if the timeout event is specified in the event set */

if ((priv->waitevents & SDIOWAIT_TIMEOUT) != 0)
{
int delay;

/* Yes.. Handle a corner case */

if (!timeout)
{
return SDIOWAIT_TIMEOUT;
}

/* Start the watchdog timer */

delay = MSEC2TICK(timeout);
ret = wd_start(&priv->waitwdog, delay,
imxrt_eventtimeout, (wdparm_t)priv);

if (ret < 0)
{
mcerr("ERROR: wd_start failed: %d\n", ret);
}
}

/* Loop until the event (or the timeout occurs). Race conditions are
* avoided by calling imxrt_waitenable prior to triggering the logic
* that will cause the wait to terminate. Under certain race
Expand Down
60 changes: 30 additions & 30 deletions arch/arm/src/kinetis/kinetis_sdhc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -329,9 +329,8 @@ static int kinetis_recvnotimpl(FAR struct sdio_dev_s *dev, uint32_t cmd,
/* EVENT handler */

static void kinetis_waitenable(FAR struct sdio_dev_s *dev,
sdio_eventset_t eventset);
static sdio_eventset_t
kinetis_eventwait(FAR struct sdio_dev_s *dev, uint32_t timeout);
sdio_eventset_t eventset, uint32_t timeout);
static sdio_eventset_t kinetis_eventwait(FAR struct sdio_dev_s *dev);
static void kinetis_callbackenable(FAR struct sdio_dev_s *dev,
sdio_eventset_t eventset);
static int kinetis_registercallback(FAR struct sdio_dev_s *dev,
Expand Down Expand Up @@ -2430,7 +2429,7 @@ static int kinetis_recvnotimpl(FAR struct sdio_dev_s *dev, uint32_t cmd,
****************************************************************************/

static void kinetis_waitenable(FAR struct sdio_dev_s *dev,
sdio_eventset_t eventset)
sdio_eventset_t eventset, uint32_t timeout)
{
struct kinetis_dev_s *priv = (struct kinetis_dev_s *)dev;
uint32_t waitints;
Expand Down Expand Up @@ -2459,6 +2458,32 @@ static void kinetis_waitenable(FAR struct sdio_dev_s *dev,
/* Enable event-related interrupts */

kinetis_configwaitints(priv, waitints, eventset, 0);

/* Check if the timeout event is specified in the event set */

if ((priv->waitevents & SDIOWAIT_TIMEOUT) != 0)
{
int delay;
int ret;

/* Yes.. Handle a corner case */

if (!timeout)
{
priv->wkupevent = SDIOWAIT_TIMEOUT;
return;
}

/* Start the watchdog timer */

delay = MSEC2TICK(timeout);
ret = wd_start(&priv->waitwdog, delay,
kinetis_eventtimeout, (wdparm_t)priv);
if (ret < 0)
{
mcerr("ERROR: wd_start failed: %d\n", ret);
}
}
}

/****************************************************************************
Expand All @@ -2482,8 +2507,7 @@ static void kinetis_waitenable(FAR struct sdio_dev_s *dev,
*
****************************************************************************/

static sdio_eventset_t kinetis_eventwait(FAR struct sdio_dev_s *dev,
uint32_t timeout)
static sdio_eventset_t kinetis_eventwait(FAR struct sdio_dev_s *dev)
{
struct kinetis_dev_s *priv = (struct kinetis_dev_s *)dev;
sdio_eventset_t wkupevent = 0;
Expand All @@ -2497,30 +2521,6 @@ static sdio_eventset_t kinetis_eventwait(FAR struct sdio_dev_s *dev,
DEBUGASSERT((priv->waitevents != 0 && priv->wkupevent == 0) ||
(priv->waitevents == 0 && priv->wkupevent != 0));

/* Check if the timeout event is specified in the event set */

if ((priv->waitevents & SDIOWAIT_TIMEOUT) != 0)
{
int delay;

/* Yes.. Handle a corner case */

if (!timeout)
{
return SDIOWAIT_TIMEOUT;
}

/* Start the watchdog timer */

delay = MSEC2TICK(timeout);
ret = wd_start(&priv->waitwdog, delay,
kinetis_eventtimeout, (wdparm_t)priv);
if (ret < 0)
{
mcerr("ERROR: wd_start failed: %d\n", ret);
}
}

/* Loop until the event (or the timeout occurs). Race conditions are
* avoided by calling kinetis_waitenable prior to triggering the logic
* that will cause the wait to terminate. Under certain race conditions,
Expand Down
68 changes: 32 additions & 36 deletions arch/arm/src/lpc17xx_40xx/lpc17_40_sdcard.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -384,9 +384,8 @@ static int lpc17_40_recvnotimpl(FAR struct sdio_dev_s *dev, uint32_t cmd,
/* EVENT handler */

static void lpc17_40_waitenable(FAR struct sdio_dev_s *dev,
sdio_eventset_t eventset);
static sdio_eventset_t
lpc17_40_eventwait(FAR struct sdio_dev_s *dev, uint32_t timeout);
sdio_eventset_t eventset, uint32_t timeout);
static sdio_eventset_t lpc17_40_eventwait(FAR struct sdio_dev_s *dev);
static void lpc17_40_callbackenable(FAR struct sdio_dev_s *dev,
sdio_eventset_t eventset);
static int lpc17_40_registercallback(FAR struct sdio_dev_s *dev,
Expand Down Expand Up @@ -2237,10 +2236,11 @@ static int lpc17_40_recvnotimpl(FAR struct sdio_dev_s *dev, uint32_t cmd,
****************************************************************************/

static void lpc17_40_waitenable(FAR struct sdio_dev_s *dev,
sdio_eventset_t eventset)
sdio_eventset_t eventset, uint32_t timeout)
{
struct lpc17_40_dev_s *priv = (struct lpc17_40_dev_s *)dev;
uint32_t waitmask;
int ret;

DEBUGASSERT(priv != NULL);

Expand Down Expand Up @@ -2272,6 +2272,33 @@ static void lpc17_40_waitenable(FAR struct sdio_dev_s *dev,

putreg32(SDCARD_WAITALL_ICR, LPC17_40_SDCARD_CLEAR);
lpc17_40_configwaitints(priv, waitmask, eventset, 0);

/* Check if the timeout event is specified in the event set */

if ((priv->waitevents & SDIOWAIT_TIMEOUT) != 0)
{
int delay;

/* Yes.. Handle a cornercase: The user request a timeout event but
* with timeout == 0?
*/

if (!timeout)
{
priv->wkupevent = SDIOWAIT_TIMEOUT;
return;
}

/* Start the watchdog timer */

delay = MSEC2TICK(timeout);
ret = wd_start(&priv->waitwdog, delay,
lpc17_40_eventtimeout, (wdparm_t)priv);
if (ret < 0)
{
mcerr("ERROR: wd_start failed: %d\n", ret);
}
}
}

/****************************************************************************
Expand All @@ -2295,8 +2322,7 @@ static void lpc17_40_waitenable(FAR struct sdio_dev_s *dev,
*
****************************************************************************/

static sdio_eventset_t lpc17_40_eventwait(FAR struct sdio_dev_s *dev,
uint32_t timeout)
static sdio_eventset_t lpc17_40_eventwait(FAR struct sdio_dev_s *dev)
{
struct lpc17_40_dev_s *priv = (struct lpc17_40_dev_s *)dev;
sdio_eventset_t wkupevent = 0;
Expand All @@ -2311,35 +2337,6 @@ static sdio_eventset_t lpc17_40_eventwait(FAR struct sdio_dev_s *dev,
flags = enter_critical_section();
DEBUGASSERT(priv->waitevents != 0 || priv->wkupevent != 0);

/* Check if the timeout event is specified in the event set */

if ((priv->waitevents & SDIOWAIT_TIMEOUT) != 0)
{
int delay;

/* Yes.. Handle a cornercase: The user request a timeout event but
* with timeout == 0?
*/

if (!timeout)
{
/* Then just tell the caller that we already timed out */

wkupevent = SDIOWAIT_TIMEOUT;
goto errout;
}

/* Start the watchdog timer */

delay = MSEC2TICK(timeout);
ret = wd_start(&priv->waitwdog, delay,
lpc17_40_eventtimeout, (wdparm_t)priv);
if (ret < 0)
{
mcerr("ERROR: wd_start failed: %d\n", ret);
}
}

/* Loop until the event (or the timeout occurs). Race conditions are
* avoided by calling lpc17_40_waitenable prior to triggering the logic
* that will cause the wait to terminate. Under certain race conditions,
Expand Down Expand Up @@ -2388,7 +2385,6 @@ static sdio_eventset_t lpc17_40_eventwait(FAR struct sdio_dev_s *dev,
priv->xfrflags = 0;
#endif

errout:
leave_critical_section(flags);
lpc17_40_dumpsamples(priv);
return wkupevent;
Expand Down
Loading

0 comments on commit 8632a38

Please sign in to comment.