From 12703220b80b09852bcdde0fcc6884d177119cda Mon Sep 17 00:00:00 2001 From: gdisirio Date: Thu, 19 May 2011 18:01:11 +0000 Subject: [PATCH] Fixed bug 3303908. git-svn-id: svn://svn.code.sf.net/p/chibios/svn/branches/stable_2.2.x@2976 35acf78f-673a-0410-8e92-d51de3d6d3f4 --- os/kernel/include/chqueues.h | 102 ++++++++++++++++++--------- os/kernel/src/chqueues.c | 130 ++++++++++++++++++----------------- readme.txt | 1 + 3 files changed, 137 insertions(+), 96 deletions(-) diff --git a/os/kernel/include/chqueues.h b/os/kernel/include/chqueues.h index 7964298f1..88162fc2c 100644 --- a/os/kernel/include/chqueues.h +++ b/os/kernel/include/chqueues.h @@ -37,18 +37,11 @@ #if CH_USE_QUEUES || defined(__DOXYGEN__) -/* - * Module dependencies check. - */ -#if !CH_USE_SEMAPHORES -#error "CH_USE_QUEUES requires CH_USE_SEMAPHORES" -#endif - /** @brief Returned by the queue functions if the operation is successful.*/ #define Q_OK RDY_OK /** @brief Returned by the queue functions if a timeout occurs.*/ #define Q_TIMEOUT RDY_TIMEOUT -/** @brief Returned by the queue functions if the queue is reset.*/ +/** @brief Returned by the queue functions if the queue has been reset.*/ #define Q_RESET RDY_RESET /** @brief Returned by the queue functions if the queue is empty.*/ #define Q_EMPTY -3 @@ -73,12 +66,13 @@ typedef void (*qnotify_t)(GenericQueue *qp); * @ref system_states) and is non-blocking. */ struct GenericQueue { + ThreadsQueue q_waiting; /**< @brief Queue of waiting threads. */ + size_t q_counter; /**< @brief Resources counter. */ uint8_t *q_buffer; /**< @brief Pointer to the queue buffer.*/ uint8_t *q_top; /**< @brief Pointer to the first location after the buffer. */ uint8_t *q_wrptr; /**< @brief Write pointer. */ uint8_t *q_rdptr; /**< @brief Read pointer. */ - Semaphore q_sem; /**< @brief Counter @p Semaphore. */ qnotify_t q_notify; /**< @brief Data notification callback. */ }; @@ -90,21 +84,19 @@ struct GenericQueue { * * @iclass */ -#define chQSizeI(qp) ((qp)->q_top - (qp)->q_buffer) +#define chQSizeI(qp) ((size_t)((qp)->q_top - (qp)->q_buffer)) /** * @brief Queue space. * @details Returns the used space if used on an input queue or the empty * space if used on an output queue. - * @note The returned value can be less than zero when there are waiting - * threads on the internal semaphore. * * @param[in] qp pointer to a @p GenericQueue structure. * @return The buffer space. * * @iclass */ -#define chQSpaceI(qp) chSemGetCounterI(&(qp)->q_sem) +#define chQSpaceI(qp) ((size_t)((qp)->q_counter)) /** * @extends GenericQueue @@ -119,6 +111,28 @@ struct GenericQueue { */ typedef GenericQueue InputQueue; +/** + * @brief Returns the filled space into an input queue. + * + * @param[in] iqp pointer to an @p InputQueue structure + * @return The number of full bytes in the queue. + * @retval 0 if the queue is empty. + * + * @iclass + */ +#define chIQGetFullI(iqp) chQSpaceI(iqp) + +/** + * @brief Returns the empty space into an input queue. + * + * @param[in] iqp pointer to an @p InputQueue structure + * @return The number of empty bytes in the queue. + * @retval 0 if the queue is full. + * + * @iclass + */ +#define chIQGetEmptyI(iqp) (chQSizeI(iqp) - chQSpaceI(iqp)) + /** * @brief Evaluates to @p TRUE if the specified input queue is empty. * @@ -141,8 +155,7 @@ typedef GenericQueue InputQueue; * * @iclass */ -#define chIQIsFullI(iqp) ((bool_t)(((iqp)->q_wrptr == (iqp)->q_rdptr) && \ - !chIQIsEmptyI(iqp))) +#define chIQIsFullI(iqp) ((bool_t)(chQSpaceI(iqp) >= chQSizeI(iqp))) /** * @brief Input queue read. @@ -152,7 +165,7 @@ typedef GenericQueue InputQueue; * * @param[in] iqp pointer to an @p InputQueue structure * @return A byte value from the queue. - * @retval Q_RESET If the queue has been reset. + * @retval Q_RESET if the queue has been reset. * * @api */ @@ -168,13 +181,14 @@ typedef GenericQueue InputQueue; * @param[in] size size of the queue buffer area * @param[in] inotify input notification callback pointer */ -#define _INPUTQUEUE_DATA(name, buffer, size, inotify) { \ - (uint8_t *)(buffer), \ - (uint8_t *)(buffer) + size, \ - (uint8_t *)(buffer), \ - (uint8_t *)(buffer), \ - _SEMAPHORE_DATA(name.q_sem, 0), \ - inotify \ +#define _INPUTQUEUE_DATA(name, buffer, size, inotify) { \ + _THREADSQUEUE_DATA(name), \ + 0, \ + (uint8_t *)(buffer), \ + (uint8_t *)(buffer) + (size), \ + (uint8_t *)(buffer), \ + (uint8_t *)(buffer), \ + inotify \ } /** @@ -203,6 +217,28 @@ typedef GenericQueue InputQueue; */ typedef GenericQueue OutputQueue; + /** + * @brief Returns the filled space into an output queue. + * + * @param[in] oqp pointer to an @p OutputQueue structure + * @return The number of full bytes in the queue. + * @retval 0 if the queue is empty. + * + * @iclass + */ +#define chOQGetFullI(oqp) (chQSizeI(oqp) - chQSpaceI(oqp)) + +/** + * @brief Returns the empty space into an output queue. + * + * @param[in] iqp pointer to an @p OutputQueue structure + * @return The number of empty bytes in the queue. + * @retval 0 if the queue is full. + * + * @iclass + */ +#define chOQGetEmptyI(iqp) chQSpaceI(oqp) + /** * @brief Evaluates to @p TRUE if the specified output queue is empty. * @@ -213,8 +249,7 @@ typedef GenericQueue OutputQueue; * * @iclass */ -#define chOQIsEmptyI(oqp) ((bool_t)(((oqp)->q_wrptr == (oqp)->q_rdptr) && \ - !chOQIsFullI(oqp))) +#define chOQIsEmptyI(oqp) ((bool_t)(chQSpaceI(oqp) >= chQSizeI(oqp))) /** * @brief Evaluates to @p TRUE if the specified output queue is full. @@ -254,13 +289,14 @@ typedef GenericQueue OutputQueue; * @param[in] size size of the queue buffer area * @param[in] onotify output notification callback pointer */ -#define _OUTPUTQUEUE_DATA(name, buffer, size, onotify) { \ - (uint8_t *)(buffer), \ - (uint8_t *)(buffer) + size, \ - (uint8_t *)(buffer), \ - (uint8_t *)(buffer), \ - _SEMAPHORE_DATA(name.q_sem, size), \ - onotify \ +#define _OUTPUTQUEUE_DATA(name, buffer, size, onotify) { \ + _THREADSQUEUE_DATA(name), \ + (size), \ + (uint8_t *)(buffer), \ + (uint8_t *)(buffer) + (size), \ + (uint8_t *)(buffer), \ + (uint8_t *)(buffer), \ + onotify \ } /** @@ -280,7 +316,6 @@ typedef GenericQueue OutputQueue; extern "C" { #endif void chIQInit(InputQueue *iqp, uint8_t *bp, size_t size, qnotify_t infy); - size_t chIQGetFullI(InputQueue *iqp); void chIQResetI(InputQueue *iqp); msg_t chIQPutI(InputQueue *iqp, uint8_t b); msg_t chIQGetTimeout(InputQueue *iqp, systime_t time); @@ -288,7 +323,6 @@ extern "C" { size_t n, systime_t time); void chOQInit(OutputQueue *oqp, uint8_t *bp, size_t size, qnotify_t onfy); - size_t chOQGetFullI(OutputQueue *oqp); void chOQResetI(OutputQueue *oqp); msg_t chOQPutTimeout(OutputQueue *oqp, uint8_t b, systime_t time); msg_t chOQGetI(OutputQueue *oqp); diff --git a/os/kernel/src/chqueues.c b/os/kernel/src/chqueues.c index 537da6d7d..07f60ffe2 100644 --- a/os/kernel/src/chqueues.c +++ b/os/kernel/src/chqueues.c @@ -53,6 +53,30 @@ #if CH_USE_QUEUES || defined(__DOXYGEN__) +/** + * @brief Puts the invoking thread into the queue's threads queue. + * + * @param[out] qp pointer to an @p GenericQueue structure + * @param[in] time the number of ticks before the operation timeouts, + * the following special values are allowed: + * - @a TIME_IMMEDIATE immediate timeout. + * - @a TIME_INFINITE no timeout. + * . + * @return A message specifying how the invoking thread has been + * released from threads queue. + * @retval Q_OK is the normal exit, thread signaled. + * @retval Q_RESET if the queue has been reset. + * @retval Q_TIMEOUT if the queue operation timed out. + */ +static msg_t qwait(GenericQueue *qp, systime_t time) { + + if (TIME_IMMEDIATE == time) + return Q_TIMEOUT; + currp->p_u.wtobjp = qp; + queue_insert(currp, &qp->q_waiting); + return chSchGoSleepTimeoutS(THD_STATE_WTQUEUE, time); +} + /** * @brief Initializes an input queue. * @details A Semaphore is internally initialized and works as a counter of @@ -70,28 +94,11 @@ */ void chIQInit(InputQueue *iqp, uint8_t *bp, size_t size, qnotify_t infy) { + queue_init(&iqp->q_waiting); + iqp->q_counter = 0; iqp->q_buffer = iqp->q_rdptr = iqp->q_wrptr = bp; iqp->q_top = bp + size; iqp->q_notify = infy; - chSemInit(&iqp->q_sem, 0); -} - -/** - * @brief Returns the filled space into an input queue. - * - * @param[in] iqp pointer to an @p InputQueue structure - * @return The number of bytes in the queue. - * @retval 0 if the queue is empty. - * - * @iclass - */ -size_t chIQGetFullI(InputQueue *iqp) { - cnt_t cnt; - - cnt = chQSpaceI(iqp); - if (cnt < 0) - return 0; - return (size_t)cnt; } /** @@ -108,7 +115,9 @@ size_t chIQGetFullI(InputQueue *iqp) { void chIQResetI(InputQueue *iqp) { iqp->q_rdptr = iqp->q_wrptr = iqp->q_buffer; - chSemResetI(&iqp->q_sem, 0); + iqp->q_counter = 0; + while (notempty(&iqp->q_waiting)) + chSchReadyI(fifo_remove(&iqp->q_waiting))->p_u.rdymsg = Q_RESET; } /** @@ -129,10 +138,12 @@ msg_t chIQPutI(InputQueue *iqp, uint8_t b) { if (chIQIsFullI(iqp)) return Q_FULL; + iqp->q_counter++; *iqp->q_wrptr++ = b; if (iqp->q_wrptr >= iqp->q_top) iqp->q_wrptr = iqp->q_buffer; - chSemSignalI(&iqp->q_sem); + if (notempty(&iqp->q_waiting)) + chSchReadyI(fifo_remove(&iqp->q_waiting))->p_u.rdymsg = Q_OK; return Q_OK; } @@ -150,23 +161,27 @@ msg_t chIQPutI(InputQueue *iqp, uint8_t b) { * . * @return A byte value from the queue. * @retval Q_TIMEOUT if the specified time expired. - * @retval Q_RESET if the queue was reset. + * @retval Q_RESET if the queue has been reset. * * @api */ msg_t chIQGetTimeout(InputQueue *iqp, systime_t time) { uint8_t b; - msg_t msg; chSysLock(); + while (chIQIsEmptyI(iqp)) { + msg_t msg; - if (iqp->q_notify) - iqp->q_notify(iqp); + if (iqp->q_notify) + iqp->q_notify(iqp); - if ((msg = chSemWaitTimeoutS(&iqp->q_sem, time)) < RDY_OK) { - chSysUnlock(); - return msg; + if ((msg = qwait((GenericQueue *)iqp, time)) < Q_OK) { + chSysUnlock(); + return msg; + } } + + iqp->q_counter--; b = *iqp->q_rdptr++; if (iqp->q_rdptr >= iqp->q_top) iqp->q_rdptr = iqp->q_buffer; @@ -208,16 +223,16 @@ size_t chIQReadTimeout(InputQueue *iqp, uint8_t *bp, chSysLock(); while (TRUE) { - if (chIQIsEmptyI(iqp)) { + while (chIQIsEmptyI(iqp)) { if (nfy) nfy(iqp); - if ((chSemWaitTimeoutS(&iqp->q_sem, time) != RDY_OK)) { + if (qwait((GenericQueue *)iqp, time) != Q_OK) { chSysUnlock(); return r; } } - else - chSemFastWaitI(&iqp->q_sem); + + iqp->q_counter--; *bp++ = *iqp->q_rdptr++; if (iqp->q_rdptr >= iqp->q_top) iqp->q_rdptr = iqp->q_buffer; @@ -251,28 +266,11 @@ size_t chIQReadTimeout(InputQueue *iqp, uint8_t *bp, */ void chOQInit(OutputQueue *oqp, uint8_t *bp, size_t size, qnotify_t onfy) { + queue_init(&oqp->q_waiting); + oqp->q_counter = size; oqp->q_buffer = oqp->q_rdptr = oqp->q_wrptr = bp; oqp->q_top = bp + size; oqp->q_notify = onfy; - chSemInit(&oqp->q_sem, (cnt_t)size); -} - -/** - * @brief Returns the filled space into an output queue. - * - * @param[in] oqp pointer to an @p OutputQueue structure - * @return The number of bytes in the queue. - * @retval 0 if the queue is empty. - * - * @iclass - */ -size_t chOQGetFullI(OutputQueue *oqp) { - cnt_t cnt; - - cnt = chQSpaceI(oqp); - if (cnt < 0) - return chQSizeI(oqp); - return chQSizeI(oqp) - (size_t)cnt; } /** @@ -289,7 +287,9 @@ size_t chOQGetFullI(OutputQueue *oqp) { void chOQResetI(OutputQueue *oqp) { oqp->q_rdptr = oqp->q_wrptr = oqp->q_buffer; - chSemResetI(&oqp->q_sem, (cnt_t)(oqp->q_top - oqp->q_buffer)); + oqp->q_counter = chQSizeI(oqp); + while (notempty(&oqp->q_waiting)) + chSchReadyI(fifo_remove(&oqp->q_waiting))->p_u.rdymsg = Q_RESET; } /** @@ -308,18 +308,23 @@ void chOQResetI(OutputQueue *oqp) { * @return The operation status. * @retval Q_OK if the operation succeeded. * @retval Q_TIMEOUT if the specified time expired. - * @retval Q_RESET if the queue was reset. + * @retval Q_RESET if the queue has been reset. * * @api */ msg_t chOQPutTimeout(OutputQueue *oqp, uint8_t b, systime_t time) { - msg_t msg; chSysLock(); - if ((msg = chSemWaitTimeoutS(&oqp->q_sem, time)) < RDY_OK) { - chSysUnlock(); - return msg; + while (chOQIsFullI(oqp)) { + msg_t msg; + + if ((msg = qwait((GenericQueue *)oqp, time)) < Q_OK) { + chSysUnlock(); + return msg; + } } + + oqp->q_counter--; *oqp->q_wrptr++ = b; if (oqp->q_wrptr >= oqp->q_top) oqp->q_wrptr = oqp->q_buffer; @@ -347,10 +352,12 @@ msg_t chOQGetI(OutputQueue *oqp) { if (chOQIsEmptyI(oqp)) return Q_EMPTY; + oqp->q_counter++; b = *oqp->q_rdptr++; if (oqp->q_rdptr >= oqp->q_top) oqp->q_rdptr = oqp->q_buffer; - chSemSignalI(&oqp->q_sem); + if (notempty(&oqp->q_waiting)) + chSchReadyI(fifo_remove(&oqp->q_waiting))->p_u.rdymsg = Q_OK; return b; } @@ -387,16 +394,15 @@ size_t chOQWriteTimeout(OutputQueue *oqp, const uint8_t *bp, chSysLock(); while (TRUE) { - if (chOQIsFullI(oqp)) { + while (chOQIsFullI(oqp)) { if (nfy) nfy(oqp); - if ((chSemWaitTimeoutS(&oqp->q_sem, time) != RDY_OK)) { + if (qwait((GenericQueue *)oqp, time) != Q_OK) { chSysUnlock(); return w; } } - else - chSemFastWaitI(&oqp->q_sem); + oqp->q_counter--; *oqp->q_wrptr++ = *bp++; if (oqp->q_wrptr >= oqp->q_top) oqp->q_wrptr = oqp->q_buffer; diff --git a/readme.txt b/readme.txt index 05f836eb2..1ccc3ae6f 100644 --- a/readme.txt +++ b/readme.txt @@ -69,6 +69,7 @@ ***************************************************************************** *** 2.2.4 *** +- FIX: Race condition in output queues (bug 3303908). - FIX: Fixed CH_USE_HEAP and CH_USE_MALLOC_HEAP conflict (bug 3303841). - FIX: Fixed timeout problem in the lwIP interface layer (bug 3302420). - FIX: Fixed invalid BRR() macro in AVR serial driver (bug 3299306).