From e9e986e64cec72eb833b749ac805d4ecd1180a5b Mon Sep 17 00:00:00 2001 From: DipSwitch Date: Wed, 31 May 2017 14:06:23 +0200 Subject: [PATCH 1/2] driver/at86rf2xx: Fix possible race condition where at86rf2xx_configure returns to the wrong state if waited on finished state in at86rf2xx_setstate --- drivers/at86rf2xx/at86rf2xx_getset.c | 18 ++++++++++-------- drivers/at86rf2xx/at86rf2xx_internal.c | 2 +- drivers/include/at86rf2xx.h | 4 +++- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/drivers/at86rf2xx/at86rf2xx_getset.c b/drivers/at86rf2xx/at86rf2xx_getset.c index 761f97825202..67760ba8ddcb 100644 --- a/drivers/at86rf2xx/at86rf2xx_getset.c +++ b/drivers/at86rf2xx/at86rf2xx_getset.c @@ -444,17 +444,12 @@ static inline void _set_state(at86rf2xx_t *dev, uint8_t state, uint8_t cmd) dev->state = state; } -void at86rf2xx_set_state(at86rf2xx_t *dev, uint8_t state) +uint8_t at86rf2xx_set_state(at86rf2xx_t *dev, uint8_t state) { uint8_t old_state = at86rf2xx_get_status(dev); if (state == old_state) { - return; - } - - if (state == AT86RF2XX_STATE_FORCE_TRX_OFF) { - _set_state(dev, AT86RF2XX_STATE_TRX_OFF, state); - return; + return old_state; } /* make sure there is no ongoing transmission, or state transition already @@ -465,8 +460,13 @@ void at86rf2xx_set_state(at86rf2xx_t *dev, uint8_t state) old_state = at86rf2xx_get_status(dev); } + if (state == AT86RF2XX_STATE_FORCE_TRX_OFF) { + _set_state(dev, AT86RF2XX_STATE_TRX_OFF, state); + return old_state; + } + if (state == old_state) { - return; + return old_state; } /* we need to go via PLL_ON if we are moving between RX_AACK_ON <-> TX_ARET_ON */ @@ -493,6 +493,8 @@ void at86rf2xx_set_state(at86rf2xx_t *dev, uint8_t state) } else { _set_state(dev, state, state); } + + return old_state; } void at86rf2xx_reset_state_machine(at86rf2xx_t *dev) diff --git a/drivers/at86rf2xx/at86rf2xx_internal.c b/drivers/at86rf2xx/at86rf2xx_internal.c index 0265ef6f54bf..806bc288c13b 100644 --- a/drivers/at86rf2xx/at86rf2xx_internal.c +++ b/drivers/at86rf2xx/at86rf2xx_internal.c @@ -150,7 +150,7 @@ void at86rf2xx_configure_phy(at86rf2xx_t *dev) uint8_t state = at86rf2xx_get_status(dev); /* we must be in TRX_OFF before changing the PHY configuration */ - at86rf2xx_set_state(dev, AT86RF2XX_STATE_TRX_OFF); + state = at86rf2xx_set_state(dev, AT86RF2XX_STATE_TRX_OFF); #ifdef MODULE_AT86RF212B /* The TX power register must be updated after changing the channel if diff --git a/drivers/include/at86rf2xx.h b/drivers/include/at86rf2xx.h index 3afee08987fc..9e3453f2d807 100644 --- a/drivers/include/at86rf2xx.h +++ b/drivers/include/at86rf2xx.h @@ -385,8 +385,10 @@ void at86rf2xx_set_option(at86rf2xx_t *dev, uint16_t option, bool state); * * @param[in] dev device to change state of * @param[in] state the targeted new state + * + * @return the previous error before the new state was set */ -void at86rf2xx_set_state(at86rf2xx_t *dev, uint8_t state); +uint8_t at86rf2xx_set_state(at86rf2xx_t *dev, uint8_t state); /** * @brief Reset the internal state machine to TRX_OFF mode. From 8557fab603bff802a9593b79c3e18c1d3fccd9ec Mon Sep 17 00:00:00 2001 From: Thomas Eichinger Date: Mon, 19 Jun 2017 16:15:35 -0700 Subject: [PATCH 2/2] drivers/at86rf2xx: simplify code When checking for busy states or state transissions in the state changing function checks before calling the function can be removed. --- drivers/at86rf2xx/at86rf2xx.c | 14 +++----------- drivers/at86rf2xx/at86rf2xx_getset.c | 21 +++++---------------- drivers/at86rf2xx/at86rf2xx_internal.c | 2 +- drivers/include/at86rf2xx.h | 2 +- 4 files changed, 10 insertions(+), 29 deletions(-) diff --git a/drivers/at86rf2xx/at86rf2xx.c b/drivers/at86rf2xx/at86rf2xx.c index 1e17acd326e1..727ae8bb54fc 100644 --- a/drivers/at86rf2xx/at86rf2xx.c +++ b/drivers/at86rf2xx/at86rf2xx.c @@ -136,18 +136,10 @@ size_t at86rf2xx_send(at86rf2xx_t *dev, uint8_t *data, size_t len) void at86rf2xx_tx_prepare(at86rf2xx_t *dev) { - uint8_t state; - dev->pending_tx++; - /* make sure ongoing transmissions are finished */ - do { - state = at86rf2xx_get_status(dev); - } while (state == AT86RF2XX_STATE_BUSY_RX_AACK || - state == AT86RF2XX_STATE_BUSY_TX_ARET); - if (state != AT86RF2XX_STATE_TX_ARET_ON) { - dev->idle_state = state; - } - at86rf2xx_set_state(dev, AT86RF2XX_STATE_TX_ARET_ON); + + dev->idle_state = at86rf2xx_set_state(dev, AT86RF2XX_STATE_TX_ARET_ON); + dev->tx_frame_len = IEEE802154_FCS_LEN; } diff --git a/drivers/at86rf2xx/at86rf2xx_getset.c b/drivers/at86rf2xx/at86rf2xx_getset.c index 67760ba8ddcb..c22521b0fb53 100644 --- a/drivers/at86rf2xx/at86rf2xx_getset.c +++ b/drivers/at86rf2xx/at86rf2xx_getset.c @@ -446,19 +446,15 @@ static inline void _set_state(at86rf2xx_t *dev, uint8_t state, uint8_t cmd) uint8_t at86rf2xx_set_state(at86rf2xx_t *dev, uint8_t state) { - uint8_t old_state = at86rf2xx_get_status(dev); - - if (state == old_state) { - return old_state; - } + uint8_t old_state; /* make sure there is no ongoing transmission, or state transition already * in progress */ - while (old_state == AT86RF2XX_STATE_BUSY_RX_AACK || - old_state == AT86RF2XX_STATE_BUSY_TX_ARET || - old_state == AT86RF2XX_STATE_IN_PROGRESS) { + do { old_state = at86rf2xx_get_status(dev); - } + } while (old_state == AT86RF2XX_STATE_BUSY_RX_AACK || + old_state == AT86RF2XX_STATE_BUSY_TX_ARET || + old_state == AT86RF2XX_STATE_IN_PROGRESS); if (state == AT86RF2XX_STATE_FORCE_TRX_OFF) { _set_state(dev, AT86RF2XX_STATE_TRX_OFF, state); @@ -499,14 +495,7 @@ uint8_t at86rf2xx_set_state(at86rf2xx_t *dev, uint8_t state) void at86rf2xx_reset_state_machine(at86rf2xx_t *dev) { - uint8_t old_state; - at86rf2xx_assert_awake(dev); - /* Wait for any state transitions to complete before forcing TRX_OFF */ - do { - old_state = at86rf2xx_get_status(dev); - } while (old_state == AT86RF2XX_STATE_IN_PROGRESS); - at86rf2xx_set_state(dev, AT86RF2XX_STATE_FORCE_TRX_OFF); } diff --git a/drivers/at86rf2xx/at86rf2xx_internal.c b/drivers/at86rf2xx/at86rf2xx_internal.c index 806bc288c13b..095ce8059d80 100644 --- a/drivers/at86rf2xx/at86rf2xx_internal.c +++ b/drivers/at86rf2xx/at86rf2xx_internal.c @@ -147,7 +147,7 @@ void at86rf2xx_hardware_reset(at86rf2xx_t *dev) void at86rf2xx_configure_phy(at86rf2xx_t *dev) { - uint8_t state = at86rf2xx_get_status(dev); + uint8_t state; /* we must be in TRX_OFF before changing the PHY configuration */ state = at86rf2xx_set_state(dev, AT86RF2XX_STATE_TRX_OFF); diff --git a/drivers/include/at86rf2xx.h b/drivers/include/at86rf2xx.h index 9e3453f2d807..708164364952 100644 --- a/drivers/include/at86rf2xx.h +++ b/drivers/include/at86rf2xx.h @@ -386,7 +386,7 @@ void at86rf2xx_set_option(at86rf2xx_t *dev, uint16_t option, bool state); * @param[in] dev device to change state of * @param[in] state the targeted new state * - * @return the previous error before the new state was set + * @return the previous state before the new state was set */ uint8_t at86rf2xx_set_state(at86rf2xx_t *dev, uint8_t state);