From a94e88cd5081148a8bcc99c5123337b94612f995 Mon Sep 17 00:00:00 2001 From: Eduardo Montoya Date: Wed, 11 Aug 2021 03:17:40 +0200 Subject: [PATCH] [mac] add `mIsHeaderUpdated` frame flag (#6849) When a platform provides `OT_RADIO_CAPS_TRANSMIT_SEC` capability, it updates the frame header with the dynamic data (frame counter, key id...) and returns it back to MAC together with the transmission result. So far the `mIsARetx` flag was being used to instruct the radio whether to update the header or not, but there was no mean to know if the radio was able to do so. This commit introduces the `mIsHeaderUpdated` flag which is used in both directions, so the MAC layer can react to cases in which the frame header was not properly updated by the radio platform. The usage of mIsARetx remains restricted now to two cases: - When a SSED decides to re-synchronize CSL parameters after a retransmission. - To check if sequence number was set for CSL/indirect retransmissions. --- examples/platforms/simulation/radio.c | 4 +-- include/openthread/instance.h | 2 +- include/openthread/platform/radio.h | 22 ++++++++------ src/core/mac/data_poll_handler.cpp | 5 ++-- src/core/mac/mac.cpp | 14 ++++----- src/core/mac/mac_frame.cpp | 2 ++ src/core/mac/mac_frame.hpp | 17 +++++++++++ src/core/mac/mac_links.hpp | 2 ++ src/core/mac/sub_mac.cpp | 4 +-- src/core/thread/csl_tx_scheduler.cpp | 5 ++-- src/lib/spinel/radio_spinel_impl.hpp | 43 +++++++++++++++++---------- src/lib/spinel/spinel.h | 4 +-- src/ncp/ncp_base_radio.cpp | 12 ++++++-- 13 files changed, 90 insertions(+), 46 deletions(-) diff --git a/examples/platforms/simulation/radio.c b/examples/platforms/simulation/radio.c index 7874ad5a5..4bf353ae8 100644 --- a/examples/platforms/simulation/radio.c +++ b/examples/platforms/simulation/radio.c @@ -643,7 +643,7 @@ static otError radioProcessTransmitSecurity(otRadioFrame *aFrame) aFrame->mInfo.mTxInfo.mAesKey = key; - if (!aFrame->mInfo.mTxInfo.mIsARetx) + if (!aFrame->mInfo.mTxInfo.mIsHeaderUpdated) { otMacFrameSetKeyId(aFrame, keyId); otMacFrameSetFrameCounter(aFrame, sMacFrameCounter++); @@ -678,7 +678,7 @@ void radioSendMessage(otInstance *aInstance) #endif // OPENTHREAD_CONFIG_MAC_HEADER_IE_SUPPORT && OPENTHREAD_CONFIG_TIME_SYNC_ENABLE #if OPENTHREAD_CONFIG_MAC_CSL_RECEIVER_ENABLE - if (sCslPeriod > 0 && !sTransmitFrame.mInfo.mTxInfo.mIsARetx) + if (sCslPeriod > 0 && !sTransmitFrame.mInfo.mTxInfo.mIsHeaderUpdated) { otMacFrameSetCslIe(&sTransmitFrame, (uint16_t)sCslPeriod, getCslPhase()); } diff --git a/include/openthread/instance.h b/include/openthread/instance.h index 498ba5c54..1a1477f28 100644 --- a/include/openthread/instance.h +++ b/include/openthread/instance.h @@ -53,7 +53,7 @@ extern "C" { * @note This number versions both OpenThread platform and user APIs. * */ -#define OPENTHREAD_API_VERSION (144) +#define OPENTHREAD_API_VERSION (145) /** * @addtogroup api-instance diff --git a/include/openthread/platform/radio.h b/include/openthread/platform/radio.h index f0b21c378..822d91184 100644 --- a/include/openthread/platform/radio.h +++ b/include/openthread/platform/radio.h @@ -239,24 +239,28 @@ typedef struct otRadioFrame uint8_t mMaxFrameRetries; ///< Maximum number of retries allowed after a transmission failure. /** - * Indicates whether the frame is a retransmission or not. + * Indicates whether frame counter and CSL IEs are properly updated in the header. * * If the platform layer does not provide `OT_RADIO_CAPS_TRANSMIT_SEC` capability, it can ignore this flag. * * If the platform provides `OT_RADIO_CAPS_TRANSMIT_SEC` capability, then platform is expected to handle tx * security processing and assignment of frame counter. In this case the following behavior is expected: * - * When `mIsARetx` is set, it indicates that OpenThread core has already set the frame counter and key id - * (if security is enabled) in the prepared frame. The counter is ensured to match the counter value from - * the previous attempts of the same frame. The platform should not assign or change the frame counter (but - * may still need to perform security processing depending on `mIsSecurityProcessed` flag). + * When `mIsHeaderUpdated` is set, it indicates that OpenThread core has already set the frame counter and + * CSL IEs (if security is enabled) in the prepared frame. The counter is ensured to match the counter value + * from the previous attempts of the same frame. The platform should not assign or change the frame counter + * (but may still need to perform security processing depending on `mIsSecurityProcessed` flag). * - * If `mIsARetx` is not set, then the frame counter and key id are not set in the frame by OpenThread core - * and it is the responsibility of the radio platform to assign them. The platform should update the frame - * (assign counter and key id) even if the transmission gets aborted or fails (e.g., channel access error). + * If `mIsHeaderUpdated` is not set, then the frame counter and key CSL IE not set in the frame by + * OpenThread core and it is the responsibility of the radio platform to assign them. The platform + * must update the frame header (assign counter and CSL IE values) before sending the frame over the air, + * however if the the transmission gets aborted and the frame is never sent over the air (e.g., channel + * access error) the platform may choose to not update the header. If the platform updates the header, + * it must also set this flag before passing the frame back from the `otPlatRadioTxDone()` callback. * */ - bool mIsARetx : 1; + bool mIsHeaderUpdated : 1; + bool mIsARetx : 1; ///< Indicates whether the frame is a retransmission or not. bool mCsmaCaEnabled : 1; ///< Set to true to enable CSMA-CA for this packet, false otherwise. bool mCslPresent : 1; ///< Set to true if CSL header IE is present. bool mIsSecurityProcessed : 1; ///< True if SubMac should skip the AES processing of this frame. diff --git a/src/core/mac/data_poll_handler.cpp b/src/core/mac/data_poll_handler.cpp index f4d61eac8..81299cbff 100644 --- a/src/core/mac/data_poll_handler.cpp +++ b/src/core/mac/data_poll_handler.cpp @@ -242,8 +242,9 @@ void DataPollHandler::HandleSentFrame(const Mac::TxFrame &aFrame, Error aError, break; case kErrorNoAck: - aChild.IncrementIndirectTxAttempts(); + OT_ASSERT(!aFrame.GetSecurityEnabled() || aFrame.IsHeaderUpdated()); + aChild.IncrementIndirectTxAttempts(); otLogInfoMac("Indirect tx to child %04x failed, attempt %d/%d", aChild.GetRloc16(), aChild.GetIndirectTxAttempts(), kMaxPollTriggeredTxAttempts); @@ -268,7 +269,7 @@ void DataPollHandler::HandleSentFrame(const Mac::TxFrame &aFrame, Error aError, aChild.SetIndirectDataSequenceNumber(aFrame.GetSequence()); - if (aFrame.GetSecurityEnabled()) + if (aFrame.GetSecurityEnabled() && aFrame.IsHeaderUpdated()) { uint32_t frameCounter; uint8_t keyId; diff --git a/src/core/mac/mac.cpp b/src/core/mac/mac.cpp index 59a60aebf..5a74c3ad3 100644 --- a/src/core/mac/mac.cpp +++ b/src/core/mac/mac.cpp @@ -950,7 +950,7 @@ void Mac::ProcessTransmitSecurity(TxFrame &aFrame) aFrame.SetAesKey(keyManager.GetKek()); extAddress = &GetExtAddress(); - if (!aFrame.IsARetransmission()) + if (!aFrame.IsHeaderUpdated()) { aFrame.SetFrameCounter(keyManager.GetKekFrameCounter()); keyManager.IncrementKekFrameCounter(); @@ -974,13 +974,13 @@ void Mac::ProcessTransmitSecurity(TxFrame &aFrame) aFrame.SetAesKey(*mLinks.GetCurrentMacKey(aFrame)); extAddress = &GetExtAddress(); - // If the frame is marked as a retransmission, `MeshForwarder` which + // If the frame header is marked as updated, `MeshForwarder` which // prepared the frame should set the frame counter and key id to the - // same values used in the earlier transmit attempt. For a new frame (not - // a retransmission), we get a new frame counter and key id from the key + // same values used in the earlier transmit attempt. For a new frame (header + // not updated), we get a new frame counter and key id from the key // manager. - if (!aFrame.IsARetransmission()) + if (!aFrame.IsHeaderUpdated()) { mLinks.SetMacFrameCounter(aFrame); aFrame.SetKeyId((keyManager.GetCurrentKeySequence() & 0x7f) + 1); @@ -1086,7 +1086,7 @@ void Mac::BeginTransmit(void) frame = Get().HandleFrameRequest(txFrames); VerifyOrExit(frame != nullptr); - // If the frame is marked as a retransmission, then data sequence number is already set. + // If the frame is marked as retransmission, then data sequence number is already set. if (!frame->IsARetransmission()) { frame->SetSequence(mDataSequence++); @@ -1100,7 +1100,7 @@ void Mac::BeginTransmit(void) frame = Get().HandleFrameRequest(txFrames); VerifyOrExit(frame != nullptr); - // If the frame is marked as a retransmission, then data sequence number is already set. + // If the frame is marked as retransmission, then data sequence number is already set. if (!frame->IsARetransmission()) { frame->SetSequence(mDataSequence++); diff --git a/src/core/mac/mac_frame.cpp b/src/core/mac/mac_frame.cpp index cd1be5b10..c51e52336 100644 --- a/src/core/mac/mac_frame.cpp +++ b/src/core/mac/mac_frame.cpp @@ -509,6 +509,8 @@ void Frame::SetFrameCounter(uint32_t aFrameCounter) index += kSecurityControlSize; WriteUint32(aFrameCounter, &mPsdu[index]); + + static_cast(this)->SetIsHeaderUpdated(true); } const uint8_t *Frame::GetKeySource(void) const diff --git a/src/core/mac/mac_frame.hpp b/src/core/mac/mac_frame.hpp index ec111a76f..4fbaab821 100644 --- a/src/core/mac/mac_frame.hpp +++ b/src/core/mac/mac_frame.hpp @@ -1336,6 +1336,23 @@ public: mInfo.mTxInfo.mIsSecurityProcessed = aIsSecurityProcessed; } + /** + * This method indicates whether or not the frame header is updated. + * + * @retval TRUE The frame already has the header updated. + * @retval FALSE The frame does not have the header updated. + * + */ + bool IsHeaderUpdated(void) const { return mInfo.mTxInfo.mIsHeaderUpdated; } + + /** + * This method sets the header updated flag attribute. + * + * @param[in] aIsHeaderUpdated TRUE if the frame header is updated. + * + */ + void SetIsHeaderUpdated(bool aIsHeaderUpdated) { mInfo.mTxInfo.mIsHeaderUpdated = aIsHeaderUpdated; } + #if OPENTHREAD_CONFIG_TIME_SYNC_ENABLE /** * This method sets the Time IE offset. diff --git a/src/core/mac/mac_links.hpp b/src/core/mac/mac_links.hpp index 967c2227c..21b0694e3 100644 --- a/src/core/mac/mac_links.hpp +++ b/src/core/mac/mac_links.hpp @@ -180,6 +180,7 @@ public: mTxFrame802154.SetIsARetransmission(false); mTxFrame802154.SetIsSecurityProcessed(false); mTxFrame802154.SetCsmaCaEnabled(true); // Set to true by default, only set to `false` for CSL transmission + mTxFrame802154.SetIsHeaderUpdated(false); #if OPENTHREAD_FTD && OPENTHREAD_CONFIG_MAC_CSL_TRANSMITTER_ENABLE mTxFrame802154.SetTxDelay(0); mTxFrame802154.SetTxDelayBaseTime(0); @@ -190,6 +191,7 @@ public: mTxFrameTrel.SetIsARetransmission(false); mTxFrameTrel.SetIsSecurityProcessed(false); mTxFrameTrel.SetCsmaCaEnabled(true); + mTxFrameTrel.SetIsHeaderUpdated(false); #endif #if OPENTHREAD_CONFIG_MULTI_RADIO diff --git a/src/core/mac/sub_mac.cpp b/src/core/mac/sub_mac.cpp index a84696235..0217a2d50 100644 --- a/src/core/mac/sub_mac.cpp +++ b/src/core/mac/sub_mac.cpp @@ -333,7 +333,7 @@ void SubMac::ProcessTransmitSecurity(void) SuccessOrExit(mTransmitFrame.GetKeyIdMode(keyIdMode)); - if (!mTransmitFrame.IsARetransmission()) + if (!mTransmitFrame.IsHeaderUpdated()) { mTransmitFrame.SetKeyId(mKeyId); } @@ -343,7 +343,7 @@ void SubMac::ProcessTransmitSecurity(void) mTransmitFrame.SetAesKey(GetCurrentMacKey()); - if (!mTransmitFrame.IsARetransmission()) + if (!mTransmitFrame.IsHeaderUpdated()) { uint32_t frameCounter = GetFrameCounter(); diff --git a/src/core/thread/csl_tx_scheduler.cpp b/src/core/thread/csl_tx_scheduler.cpp index b70a895f0..552a429ed 100644 --- a/src/core/thread/csl_tx_scheduler.cpp +++ b/src/core/thread/csl_tx_scheduler.cpp @@ -249,8 +249,9 @@ void CslTxScheduler::HandleSentFrame(const Mac::TxFrame &aFrame, Error aError, C break; case kErrorNoAck: - aChild.IncrementCslTxAttempts(); + OT_ASSERT(!aFrame.GetSecurityEnabled() || aFrame.IsHeaderUpdated()); + aChild.IncrementCslTxAttempts(); otLogInfoMac("CSL tx to child %04x failed, attempt %d/%d", aChild.GetRloc16(), aChild.GetCslTxAttempts(), kMaxCslTriggeredTxAttempts); @@ -274,7 +275,7 @@ void CslTxScheduler::HandleSentFrame(const Mac::TxFrame &aFrame, Error aError, C { aChild.SetIndirectDataSequenceNumber(aFrame.GetSequence()); - if (aFrame.GetSecurityEnabled()) + if (aFrame.GetSecurityEnabled() && aFrame.IsHeaderUpdated()) { uint32_t frameCounter; uint8_t keyId; diff --git a/src/lib/spinel/radio_spinel_impl.hpp b/src/lib/spinel/radio_spinel_impl.hpp index dfeee4b55..29ee3d874 100644 --- a/src/lib/spinel/radio_spinel_impl.hpp +++ b/src/lib/spinel/radio_spinel_impl.hpp @@ -1843,9 +1843,10 @@ void RadioSpinel::HandleTransmitDone(uint32_t const uint8_t * aBuffer, uint16_t aLength) { - otError error = OT_ERROR_NONE; - spinel_status_t status = SPINEL_STATUS_OK; - bool framePending = false; + otError error = OT_ERROR_NONE; + spinel_status_t status = SPINEL_STATUS_OK; + bool framePending = false; + bool headerUpdated = false; spinel_ssize_t unpacked; VerifyOrExit(aCommand == SPINEL_CMD_PROP_VALUE_IS && aKey == SPINEL_PROP_LAST_STATUS, error = OT_ERROR_FAILED); @@ -1862,6 +1863,12 @@ void RadioSpinel::HandleTransmitDone(uint32_t aBuffer += unpacked; aLength -= static_cast(unpacked); + unpacked = spinel_datatype_unpack(aBuffer, aLength, SPINEL_DATATYPE_BOOL_S, &headerUpdated); + VerifyOrExit(unpacked > 0, error = OT_ERROR_PARSE); + + aBuffer += unpacked; + aLength -= static_cast(unpacked); + if (status == SPINEL_STATUS_OK) { SuccessOrExit(error = ParseRadioFrame(mAckRadioFrame, aBuffer, aLength, unpacked)); @@ -1873,7 +1880,10 @@ void RadioSpinel::HandleTransmitDone(uint32_t error = SpinelStatusToOtError(status); } - if ((mRadioCaps & OT_RADIO_CAPS_TRANSMIT_SEC) && static_cast(mTransmitFrame)->GetSecurityEnabled()) + static_cast(mTransmitFrame)->SetIsHeaderUpdated(headerUpdated); + + if ((mRadioCaps & OT_RADIO_CAPS_TRANSMIT_SEC) && headerUpdated && + static_cast(mTransmitFrame)->GetSecurityEnabled()) { uint8_t keyId; uint32_t frameCounter; @@ -1905,20 +1915,21 @@ otError RadioSpinel::Transmit(otRadioFrame &a otPlatRadioTxStarted(mInstance, mTransmitFrame); error = Request(SPINEL_CMD_PROP_VALUE_SET, SPINEL_PROP_STREAM_RAW, - SPINEL_DATATYPE_DATA_WLEN_S // Frame data - SPINEL_DATATYPE_UINT8_S // Channel - SPINEL_DATATYPE_UINT8_S // MaxCsmaBackoffs - SPINEL_DATATYPE_UINT8_S // MaxFrameRetries - SPINEL_DATATYPE_BOOL_S // CsmaCaEnabled - SPINEL_DATATYPE_BOOL_S // IsARetx - SPINEL_DATATYPE_BOOL_S // SkipAes - SPINEL_DATATYPE_UINT32_S // TxDelay - SPINEL_DATATYPE_UINT32_S, // TxDelayBaseTime + SPINEL_DATATYPE_DATA_WLEN_S // Frame data + SPINEL_DATATYPE_UINT8_S // Channel + SPINEL_DATATYPE_UINT8_S // MaxCsmaBackoffs + SPINEL_DATATYPE_UINT8_S // MaxFrameRetries + SPINEL_DATATYPE_BOOL_S // CsmaCaEnabled + SPINEL_DATATYPE_BOOL_S // IsHeaderUpdated + SPINEL_DATATYPE_BOOL_S // IsARetx + SPINEL_DATATYPE_BOOL_S // SkipAes + SPINEL_DATATYPE_UINT32_S // TxDelay + SPINEL_DATATYPE_UINT32_S, // TxDelayBaseTime mTransmitFrame->mPsdu, mTransmitFrame->mLength, mTransmitFrame->mChannel, mTransmitFrame->mInfo.mTxInfo.mMaxCsmaBackoffs, mTransmitFrame->mInfo.mTxInfo.mMaxFrameRetries, - mTransmitFrame->mInfo.mTxInfo.mCsmaCaEnabled, mTransmitFrame->mInfo.mTxInfo.mIsARetx, - mTransmitFrame->mInfo.mTxInfo.mIsSecurityProcessed, mTransmitFrame->mInfo.mTxInfo.mTxDelay, - mTransmitFrame->mInfo.mTxInfo.mTxDelayBaseTime); + mTransmitFrame->mInfo.mTxInfo.mCsmaCaEnabled, mTransmitFrame->mInfo.mTxInfo.mIsHeaderUpdated, + mTransmitFrame->mInfo.mTxInfo.mIsARetx, mTransmitFrame->mInfo.mTxInfo.mIsSecurityProcessed, + mTransmitFrame->mInfo.mTxInfo.mTxDelay, mTransmitFrame->mInfo.mTxInfo.mTxDelayBaseTime); if (error == OT_ERROR_NONE) { diff --git a/src/lib/spinel/spinel.h b/src/lib/spinel/spinel.h index 9ddf6a969..77f98c785 100644 --- a/src/lib/spinel/spinel.h +++ b/src/lib/spinel/spinel.h @@ -377,7 +377,7 @@ * Please see section "Spinel definition compatibility guideline" for more details. * */ -#define SPINEL_RCP_API_VERSION 3 +#define SPINEL_RCP_API_VERSION 4 /** * @def SPINEL_MIN_HOST_SUPPORTED_RCP_API_VERSION @@ -389,7 +389,7 @@ * Please see section "Spinel definition compatibility guideline" for more details. * */ -#define SPINEL_MIN_HOST_SUPPORTED_RCP_API_VERSION 1 +#define SPINEL_MIN_HOST_SUPPORTED_RCP_API_VERSION 4 /** * @def SPINEL_FRAME_MAX_SIZE diff --git a/src/ncp/ncp_base_radio.cpp b/src/ncp/ncp_base_radio.cpp index 3be08286f..b53a0db2c 100644 --- a/src/ncp/ncp_base_radio.cpp +++ b/src/ncp/ncp_base_radio.cpp @@ -148,8 +148,9 @@ void NcpBase::LinkRawTransmitDone(otRadioFrame *aFrame, otRadioFrame *aAckFrame, if (mCurTransmitTID) { - uint8_t header = SPINEL_HEADER_FLAG | SPINEL_HEADER_IID_0 | mCurTransmitTID; - bool framePending = (aAckFrame != nullptr && static_cast(aAckFrame)->GetFramePending()); + uint8_t header = SPINEL_HEADER_FLAG | SPINEL_HEADER_IID_0 | mCurTransmitTID; + bool framePending = (aAckFrame != nullptr && static_cast(aAckFrame)->GetFramePending()); + bool headerUpdated = static_cast(aFrame)->IsHeaderUpdated(); // Clear cached transmit TID mCurTransmitTID = 0; @@ -157,13 +158,14 @@ void NcpBase::LinkRawTransmitDone(otRadioFrame *aFrame, otRadioFrame *aAckFrame, SuccessOrExit(mEncoder.BeginFrame(header, SPINEL_CMD_PROP_VALUE_IS, SPINEL_PROP_LAST_STATUS)); SuccessOrExit(mEncoder.WriteUintPacked(ThreadErrorToSpinelStatus(aError))); SuccessOrExit(mEncoder.WriteBool(framePending)); + SuccessOrExit(mEncoder.WriteBool(headerUpdated)); if (aError == OT_ERROR_NONE) { SuccessOrExit(PackRadioFrame(aAckFrame, aError)); } - if (static_cast(aFrame)->GetSecurityEnabled()) + if (static_cast(aFrame)->GetSecurityEnabled() && headerUpdated) { uint8_t keyId; uint32_t frameCounter; @@ -386,6 +388,7 @@ otError NcpBase::DecodeStreamRawTxRequest(otRadioFrame &aFrame) uint16_t payloadLen; bool csmaEnable; bool isARetx; + bool isHeaderUpdated; bool isSecurityProcessed; SuccessOrExit(error = mDecoder.ReadDataWithLen(payloadPtr, payloadLen)); @@ -403,6 +406,7 @@ otError NcpBase::DecodeStreamRawTxRequest(otRadioFrame &aFrame) aFrame.mInfo.mTxInfo.mMaxCsmaBackoffs = OPENTHREAD_CONFIG_MAC_MAX_CSMA_BACKOFFS_DIRECT; aFrame.mInfo.mTxInfo.mMaxFrameRetries = OPENTHREAD_CONFIG_MAC_DEFAULT_MAX_FRAME_RETRIES_DIRECT; aFrame.mInfo.mTxInfo.mCsmaCaEnabled = true; + aFrame.mInfo.mTxInfo.mIsHeaderUpdated = false; aFrame.mInfo.mTxInfo.mIsARetx = false; aFrame.mInfo.mTxInfo.mIsSecurityProcessed = false; aFrame.mInfo.mTxInfo.mTxDelay = 0; @@ -416,11 +420,13 @@ otError NcpBase::DecodeStreamRawTxRequest(otRadioFrame &aFrame) SuccessOrExit(mDecoder.ReadUint8(aFrame.mInfo.mTxInfo.mMaxCsmaBackoffs)); SuccessOrExit(mDecoder.ReadUint8(aFrame.mInfo.mTxInfo.mMaxFrameRetries)); SuccessOrExit(mDecoder.ReadBool(csmaEnable)); + SuccessOrExit(mDecoder.ReadBool(isHeaderUpdated)); SuccessOrExit(mDecoder.ReadBool(isARetx)); SuccessOrExit(mDecoder.ReadBool(isSecurityProcessed)); SuccessOrExit(mDecoder.ReadUint32(aFrame.mInfo.mTxInfo.mTxDelay)); SuccessOrExit(mDecoder.ReadUint32(aFrame.mInfo.mTxInfo.mTxDelayBaseTime)); aFrame.mInfo.mTxInfo.mCsmaCaEnabled = csmaEnable; + aFrame.mInfo.mTxInfo.mIsHeaderUpdated = isHeaderUpdated; aFrame.mInfo.mTxInfo.mIsARetx = isARetx; aFrame.mInfo.mTxInfo.mIsSecurityProcessed = isSecurityProcessed;