neilabc wrote on Friday, November 24, 2017:
Hi, I’m using the FreeRTOS 8.2.3 port for the microblaze.
I’m concerned that a context switch at the wrong time could cause an interrupt enable or disable to be lost.
vPortEnableInterrupt calls XIntc_Enable without calling portENTER_CRITICAL.
Within XIntc_Enable, there is a read-modify-write of the IER interrupt enable register.
Should this be a critical section? If not, can you explain?
Cheers,
Neil
rtel wrote on Friday, November 24, 2017:
Just from your explanation, without looking at the code, I don’t think a
critical section is not required because the task takes a copy of the
IER register in the read part of the read modify write. If it were to
then be switched out before the write part then it could only run again
if the interrupt enable/state was the same as when it read the register.
Eg. If interrupts were enabled when it read the register, then it was
swapped out and the next task to run disabled interrupts, then a context
switch back to the original task could not take place until interrupts
were enabled again, at which time the IER register would be back to the
value the original task has already read (at least the interrupt enable
bit).
Some ports take a copy of the interrupt enable state during a context
switch anyway, and restore it before the task runs again - depends on
how the port is implemented.
neilabc wrote on Friday, November 24, 2017:
I’m not sure I explained it well, here’s a more specific example:
Two tasks are both attempting to enable a different interrupt in the same interrupt controller.
Task1 is preempted in the middle of its call to XIntc_Enable (from intc_v3_4)
Task1: Read IER <-- 0x80 (initial value in IER from interrupt controller)
Task1: Or bit 2 in local copy --> 0x84
… context switch due to, say a timer event
Task2: Read IER <-- 0x80 (initial value in IER)
Task2: Or bit 3 in local copy --> 0x88 // task enables IRQ3
Task2: Write IER <-- 0x88
… context switch back to Task1
Task1: Write IER <-- 0x84 * problem, Task2’s enable got lost
From XIntc_Enable, the context switch would have to occur between these two lines:
CurrentIER = XIntc_In32(InstancePtr->BaseAddress + XIN_IER_OFFSET);
XIntc_Out32(InstancePtr->BaseAddress + XIN_IER_OFFSET, (CurrentIER | Mask));
rtel wrote on Friday, November 24, 2017:
Task1: Read IER ← 0x80 (initial value in IER from interrupt controller)
Task1: Or bit 2 in local copy → 0x84
Is this bit in the RTOS code? Probably in the portDISABLE_INTERRUPTS macro.
… context switch due to, say a timer event
Task2: Read IER ← 0x80 (initial value in IER)
Task2: Or bit 3 in local copy → 0x88 // task enables IRQ3
Task2: Write IER ← 0x88
Is this bit in user code (that is, outside of the RTOS code)?
… context switch back to Task1
Task1: Write IER ← 0x84 * problem, Task2’s enable got lost
If the answer to both these questions is ‘yes’ then the user can wrap
their access to the IER register in a critical section.
If both the accesses are within the RTOS code then (again from your
description rather than by looking at the code) I would agree the
enabling of the IRQ should be in a critical section.
neilabc wrote on Saturday, November 25, 2017:
Yes, those bits are in the Xilinx interrupt controller driver, which is called from the RTOS code.
How should I go about reporting this possible bug to FreeRTOS?
rtel wrote on Saturday, November 25, 2017:
How should I go about reporting this possible bug to FreeRTOS?
You already have ;o)
neilabc wrote on Monday, November 27, 2017:
Thanks!
Would you please look at the code and confirm that there really is a problem that needs to be fixed?
I proposed a code change in port.c to fix this, but I’m getting some pushback from my colleagues who find it hard to believe the Microblaze FreeRTOS port would have this problem.
rtel wrote on Monday, November 27, 2017:
Will do, but won’t be able to do so immediately. I’ll report back here.
rtel wrote on Tuesday, January 30, 2018:
A fix that just wraps the single call to XIntc_Enable() inside a critical section was pushed to the public SVN repository today.