search

atmelcorp / atmel-software-package

Atmel Software Package
Other
110 stars 76 forks source link

Correct mutex implementation #76

Closed johannesacco closed 5 years ago

johannesacco commented 5 years ago

The strex instruction will return 1 if an interrupt occurred between the ldrex and strex instruction. This change make sure that the atomic sequence is retried and only fail after the ldrex instruction. This is in line with how ARM recommends implementing a mutex. For more information see http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dht0008a/ch01s03s02.html

junminlin commented 5 years ago

Thanks for drawing our attention to the point, but, looks like your commits can't merge as it is. Do you plan to rework?

johannesacco commented 5 years ago

Thanks for drawing our attention to the point, but, looks like your commits can't merge as it is. Do you plan to rework?

I can't see any merge conflicts. Github also says "This branch has no conflicts with the base branch"?

lfazio commented 5 years ago

For what I can see, mutex_try_lock() is not blocking => so the while (true) you added as no reason to be added. You also forget the return value of the function mutex_try_lock().

The function you may want to use is the mutex_lock() and not mutex_try_lock()?

johannesacco commented 5 years ago

It seems like the return value was lost when cherry-picking from our internal branch to this. I will update soon. While this function is called "mutex_try_lock()" I don't think it should fail to lock the mutex unless it is already locked. Without this change the function will return false if an interrupt occurs (which will change the state of the exclusive lock) between the ldrex and strex instruction. This will cause spid_transfer(...), uartd_transfer(...) etc to fail intermittently even though there is no reason for it. The while loop will only loop until the ldrex and strex sequence has executed successfully (i.e. the exclusive monitor allows the store) or if the mutex is already locked (ldrex reads a value != MUTEX_UNLOCKED).

TonyHan11 commented 5 years ago

Hi johannesacco, The patch has been merged to the master branch in commit df8bff2 and will effect in v2.17, thank your very much.