tonton81/FlexCAN_T4

Interrupt race condition on RX buffer

AzwarPrihastomo opened this issue · 3 comments

I found issue when using multiple can port to listen bus at high rate
i found that the way interrupt store data using this

FCTP_FUNC void FCTP_OPT::struct2queueRx(const CAN_message_t &msg) {
uint8_t buf[sizeof(CAN_message_t)];
memmove(buf, &msg, sizeof(msg));
rxBuffer.push_back(buf, sizeof(CAN_message_t));
}

the problem is when we calling FCTP_FUNC uint64_t FCTP_OPT::events() this function is accessing rxBuffer may make race condition

so i'm propose to add guard when we access rxBuffer by disabling the interrupt
like below

FCTP_FUNC uint64_t FCTP_OPT::events() {
if ( rxBuffer.size() ) {
CAN_message_t frame;
uint8_t buf[sizeof(CAN_message_t)];
NVIC_DISABLE_IRQ(nvicIrq);
rxBuffer.pop_front(buf, sizeof(CAN_message_t));
NVIC_ENABLE_IRQ(nvicIrq);
memmove(&frame, buf, sizeof(frame));
mbCallbacks((FLEXCAN_MAILBOX)frame.mb, frame);
}
if ( txBuffer.size() ) {
CAN_message_t frame;
uint8_t buf[sizeof(CAN_message_t)];
txBuffer.peek_front(buf, sizeof(CAN_message_t));
memmove(&frame, buf, sizeof(frame));
if ( write((FLEXCAN_MAILBOX)getFirstTxBox(), frame) ) txBuffer.pop_front();
}
return (uint64_t)(rxBuffer.size() << 12) | txBuffer.size();
}

it's a FIFO stack, interrupts queue to end, events pulls from front, no locks are necessary

the buffer that you use is not thread safe, so the head and tail index could be prone to race condition because of this interrupt

it's not thread safe, it's context safe, no need to block interrupts. each bus has their own rx buffer. the interrupt writes to the back of the queue, the callback pulls from the front, thats how the FIFO queue works