nuttx
nuttx copied to clipboard
apache
arch/arm: Remove the critical section protection in up_invalidate_dcache_all
Summary
since l2cc_invalidate_all already do the protection
Impact
code refactor
Testing
pass CI
Here is the reason:
- All l2cc_xxx hold the critical section internally
- All other up_xxx functions in the same file doesn't hold the critical section
I do not have a strong opinion on this. The PR description states "since l2cc_invalidate_all already do the protection" ("callee" direction), but discussion goes into "caller" direction, so that is a bit confusing. I think that critical section here is done to ensure that invalidation sequence is not interrupted, so removing of a critical section allows an interruption, so calling it from a critical section becomes a caller responsibility. I think this is what @hartmannathan is talking about and it sounds reasonable to at least specify this in comment
I do not have a strong opinion on this. The PR description states "since l2cc_invalidate_all already do the protection" ("callee" direction), but discussion goes into "caller" direction, so that is a bit confusing. I think that critical section here is done to ensure that invalidation sequence is not interrupted, so removing of a critical section allows an interruption, so calling it from a critical section becomes a caller responsibility. I think this is what @hartmannathan is talking about and it sounds reasonable to at least specify this in comment
The execution can be interrupted between cp15 and l2cc, if you go through the whole file, you will find that all other functions don't hold the critical section at all. If you look at the assemble code, all cp15 cache operation can work without critical section too, only l2cc need to protect.
But there is a note in function description:
* NOTE: This function forces L1 and L2 cache operations to be atomic
* by disabling interrupts.
If this is a requirement, all other cache function should be protected too, I think.
But there is a note in function description:
* NOTE: This function forces L1 and L2 cache operations to be atomic * by disabling interrupts.
@pkarashchenko The wrong comment is removed.
But there is a note in function description:
* NOTE: This function forces L1 and L2 cache operations to be atomic * by disabling interrupts.@pkarashchenko The wrong comment is removed.
If this is well covered by testing on your side, then I do not have any objections.
@hartmannathan do you want to take a look?
@pkarashchenko yes, looking now...
@xiaoxiang781216 @pkarashchenko I studied this quite a bit. I see that l2cc_invalidate_all() is doing its own critical section.
cp15_invalidate_dcache_all() (which is assembly code) is not doing any critical section. It runs in a loop and clears one way at a time. I can't seem to find information from ARM that explains whether interrupts should be disabled when invalidating the D Cache. Is it safe to clear D Cache with interrupts enabled? If cp15_invalidate_dcache_all() is interrupted, and the interrupt reads or writes data that happens to be in the D Cache, will we get corruption?
I don't know the answer to above question.
If you are sure that this change is safe, then I'm OK with it.
The loop only write not modify(read then write) CP15 register, so it's safe without the critical section.
@xiaoxiang781216 @pkarashchenko I studied this quite a bit. I see that l2cc_invalidate_all() is doing its own critical section. cp15_invalidate_dcache_all() (which is assembly code) is not doing any critical section. It runs in a loop and clears one way at a time. I can't seem to find information from ARM that explains whether interrupts should be disabled when invalidating the D Cache. Is it safe to clear D Cache with interrupts enabled? If cp15_invalidate_dcache_all() is interrupted, and the interrupt reads or writes data that happens to be in the D Cache, will we get corruption? I don't know the answer to above question. If you are sure that this change is safe, then I'm OK with it.
The loop only write not modify(read then write) CP15 register, so it's safe without the critical section.
@xiaoxiang781216 What about the contents of the cache itself?
I also do not know the correct answer. I'm just thinking not from a read-modify-store perspective, but from a cache operation perspective. I mean is it safe if any other CP15 cache access occurs after CP15 invalidate, but before L2CC invalidate? Can we lose memory data integrity at this point?
I mean is it safe if any other CP15 cache access occurs after CP15 invalidate, but before L2CC invalidate?
Or during the middle of CP15 invalidate?
@xiaoxiang781216 @pkarashchenko I studied this quite a bit. I see that l2cc_invalidate_all() is doing its own critical section. cp15_invalidate_dcache_all() (which is assembly code) is not doing any critical section. It runs in a loop and clears one way at a time. I can't seem to find information from ARM that explains whether interrupts should be disabled when invalidating the D Cache. Is it safe to clear D Cache with interrupts enabled? If cp15_invalidate_dcache_all() is interrupted, and the interrupt reads or writes data that happens to be in the D Cache, will we get corruption? I don't know the answer to above question. If you are sure that this change is safe, then I'm OK with it.
The loop only write not modify(read then write) CP15 register, so it's safe without the critical section.
@xiaoxiang781216 What about the contents of the cache itself?
One cache line will invalidate(discard) in each loop.
I also do not know the correct answer. I'm just thinking not from a read-modify-store perspective, but from a cache operation perspective. I mean is it safe if any other CP15 cache access occurs after CP15 invalidate, but before L2CC invalidate? Can we lose memory data integrity at this point?
It isn't the responsibility of cache layer, but the caller should take care. We do cache operation just because we want to sync with hardware which has DMA capability, not another thread. The critical section can't do any protection in this case.