pkarashchenko commented on a change in pull request #5504:
URL: https://github.com/apache/incubator-nuttx/pull/5504#discussion_r806748839
##########
File path: mm/mm_heap/mm_sem.c
##########
@@ -104,9 +104,23 @@ bool mm_takesemaphore(FAR struct mm_heap_s *heap)
#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
else if (sched_idletask())
{
- /* Try to take the semaphore */
+ return false;
+ }
+ else if (up_interrupt_context())
+ {
+ int val;
+
+#ifdef CONFIG_SMP
+ return false;
+#endif
+
+ /* Check the semaphore value, if held by someone, then return false.
+ * Else, we can take it, return true.
+ */
- return _SEM_TRYWAIT(&heap->mm_semaphore) >= 0;
+ _SEM_GETVALUE(&heap->mm_semaphore, &val);
+
+ return val > 0;
Review comment:
```suggestion
else if (up_interrupt_context())
{
#ifdef CONFIG_SMP
return false;
#else
int val;
/* Check the semaphore value, if held by someone, then return false.
* Else, we can take it, return true.
*/
_SEM_GETVALUE(&heap->mm_semaphore, &val);
return val > 0;
#endif
}
```
or
```suggestion
else if (up_interrupt_context())
{
int val = 0;
#ifndef CONFIG_SMP
/* Check the semaphore value, if held by someone, then return false.
* Else, we can take it, return true.
*/
_SEM_GETVALUE(&heap->mm_semaphore, &val);
#endif
return val > 0;
}
```
##########
File path: mm/mm_heap/mm_sem.c
##########
@@ -104,9 +104,23 @@ bool mm_takesemaphore(FAR struct mm_heap_s *heap)
#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
else if (sched_idletask())
{
- /* Try to take the semaphore */
+ return false;
+ }
+ else if (up_interrupt_context())
+ {
+ int val;
+
+#ifdef CONFIG_SMP
+ return false;
+#endif
+
+ /* Check the semaphore value, if held by someone, then return false.
+ * Else, we can take it, return true.
+ */
- return _SEM_TRYWAIT(&heap->mm_semaphore) >= 0;
+ _SEM_GETVALUE(&heap->mm_semaphore, &val);
+
+ return val > 0;
Review comment:
I agree to use it. both of my suggestions actually do it. I'm not sure
about the exact compilation options, but some compilers might generate
"unreachable code" warning in case if next construction is met:
```
int f(int a, int b)
{
int c;
return 0;
/* some code here */
c = a + b;
return c;
}
```
##########
File path: mm/mm_heap/mm_sem.c
##########
@@ -104,9 +104,23 @@ bool mm_takesemaphore(FAR struct mm_heap_s *heap)
#if defined(CONFIG_BUILD_FLAT) || defined(__KERNEL__)
else if (sched_idletask())
{
- /* Try to take the semaphore */
+ return false;
+ }
+ else if (up_interrupt_context())
+ {
+ int val;
+
+#ifdef CONFIG_SMP
+ return false;
+#endif
+
+ /* Check the semaphore value, if held by someone, then return false.
+ * Else, we can take it, return true.
+ */
- return _SEM_TRYWAIT(&heap->mm_semaphore) >= 0;
+ _SEM_GETVALUE(&heap->mm_semaphore, &val);
+
+ return val > 0;
Review comment:
I agree to use it. both of my suggestions actually do it. I'm not sure
about the exact compilation options, but some compilers might generate
"unreachable code" warning in case if next construction is met:
```
int f(int a, int b)
{
int c;
return 0;
/* some code here */
c = a + b;
return c;
}
```
I just propose some logical code restructuring.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
