From b2702415c969815d06e4a13ba455a7e4a9f2b773 Mon Sep 17 00:00:00 2001 From: Jukka Laitinen Date: Tue, 3 Dec 2024 14:56:03 +0200 Subject: [PATCH] arch/arm64/src/imx9/imx9_usdhc.c: Simplify eventwait logic and remove race condition There is a race condition when timeout and completion interrupts occur at the same time. Fix this and simplify the eventwait code. Signed-off-by: Jukka Laitinen --- arch/arm64/src/imx9/imx9_usdhc.c | 93 ++++++++++++++------------------ 1 file changed, 39 insertions(+), 54 deletions(-) diff --git a/arch/arm64/src/imx9/imx9_usdhc.c b/arch/arm64/src/imx9/imx9_usdhc.c index 157cc6d52dc65..75700999c60f4 100644 --- a/arch/arm64/src/imx9/imx9_usdhc.c +++ b/arch/arm64/src/imx9/imx9_usdhc.c @@ -1114,7 +1114,7 @@ static void imx9_recvdma(struct imx9_dev_s *priv) * None * * Assumptions: - * Always called from the interrupt level with interrupts disabled. + * None * ****************************************************************************/ @@ -1133,6 +1133,14 @@ static void imx9_eventtimeout(wdparm_t arg) imx9_sample(priv, SAMPLENDX_END_TRANSFER); + /* Disable wait interrupts */ + + imx9_configwaitints(priv, 0, 0, SDIOWAIT_TIMEOUT); + + /* Clear pending interrupt status on all wait interrupts */ + + putreg32(USDHC_WAITALL_INTS, priv->addr + IMX9_USDHC_IRQSTAT_OFFSET); + /* Wake up any waiting threads */ imx9_endwait(priv, SDIOWAIT_TIMEOUT); @@ -1159,17 +1167,15 @@ static void imx9_eventtimeout(wdparm_t arg) ****************************************************************************/ static void imx9_endwait(struct imx9_dev_s *priv, - sdio_eventset_t wkupevent) + sdio_eventset_t wkupevent) { /* Cancel the watchdog timeout */ wd_cancel(&priv->waitwdog); - /* Disable event-related interrupts */ - - imx9_configwaitints(priv, 0, 0, wkupevent); - - /* Wake up the waiting thread */ + /* Wake up the waiting thread. Avoid double post in case interrupt and + * wdog trigger at the same time + */ nxsem_post(&priv->waitsem); } @@ -2384,7 +2390,8 @@ static int imx9_cancel(struct sdio_dev_s *dev) /* Disable all transfer- and event- related interrupts */ - imx9_configxfrints(priv, 0); imx9_configwaitints(priv, 0, 0, 0); + imx9_configxfrints(priv, 0); + imx9_configwaitints(priv, 0, 0, 0); /* Clearing pending interrupt status on all transfer- and event- related * interrupts @@ -2813,9 +2820,6 @@ static void imx9_waitenable(struct sdio_dev_s *dev, * * Input Parameters: * dev - An instance of the SDIO device interface - * timeout - Maximum time in milliseconds to wait. Zero means immediate - * timeout with no wait. The timeout value is ignored if - * SDIOWAIT_TIMEOUT is not included in the waited-for eventset. * * Returned Value: * Event set containing the event(s) that ended the wait. Should always @@ -2826,65 +2830,46 @@ static void imx9_waitenable(struct sdio_dev_s *dev, static sdio_eventset_t imx9_eventwait(struct sdio_dev_s *dev) { struct imx9_dev_s *priv = (struct imx9_dev_s *)dev; - sdio_eventset_t wkupevent = 0; int ret; - - /* There is a race condition here... the event may have completed before - * we get here. In this case waitevents will be zero, but wkupevents - * will be non-zero (and, hopefully, the semaphore count will also be - * non-zero. - */ - - DEBUGASSERT((priv->waitevents != 0 && priv->wkupevent == 0) || - (priv->waitevents == 0 && priv->wkupevent != 0)); + int ret; - /* Loop until the event (or the timeout occurs). Race conditions are - * avoided by calling imx9_waitenable prior to triggering the logic - * that will cause the wait to terminate. Under certain race - * conditions, the waited-for may have already occurred before this - * function was called! + /* Wait for an event in event set to occur. If this the event has + * already occurred, then the semaphore will already have been + * incremented and there will be no wait. */ - for (; ; ) + ret = nxsem_wait_uninterruptible(&priv->waitsem); + if (ret < 0) { - /* Wait for an event in event set to occur. If this the event has - * already occurred, then the semaphore will already have been - * incremented and there will be no wait. - */ + /* Disable wait interrupts */ - ret = nxsem_wait_uninterruptible(&priv->waitsem); - if (ret < 0) - { - /* Task canceled. Cancel the wdog (assuming it was started) and - * return an SDIO error. - */ + imx9_configwaitints(priv, 0, 0, SDIOWAIT_ERROR); - wd_cancel(&priv->waitwdog); - return SDIOWAIT_ERROR; - } + /* Clear pending interrupt status on all wait interrupts */ - wkupevent = priv->wkupevent; + putreg32(USDHC_WAITALL_INTS, priv->addr + IMX9_USDHC_IRQSTAT_OFFSET); - /* Check if the event has occurred. When the event has occurred, then - * evenset will be set to 0 and wkupevent will be set to a non-zero - * value. - */ + /* Cancel wdog */ - if (wkupevent != 0) - { - /* Yes... break out of the loop with wkupevent non-zero */ - - break; - } + wd_cancel(&priv->waitwdog); } - /* Disable event-related interrupts */ + /* In case of timeout or task cancellation, we need to reset the semaphore; + * it might have been double-posted if interrupt occured at the same time + */ + + if (ret < 0 || + priv->wkupevent == SDIOWAIT_TIMEOUT) + { + nxsem_reset(&priv->waitsem, 0); + } - imx9_configwaitints(priv, 0, 0, 0); #ifdef CONFIG_IMX9_USDHC_DMA priv->xfrflags = 0; #endif + imx9_dumpsamples(priv); - return wkupevent; + + return priv->wkupevent; } /****************************************************************************