[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #2641: can: Cleanup usage of soft fifo semaphore

[GitBox <gi...@apache.org>]

xiaoxiang781216 commented on a change in pull request #2641:
URL: https://github.com/apache/incubator-nuttx/pull/2641#discussion_r773694946



##########
File path: drivers/can/can.c
##########
@@ -609,24 +609,24 @@ static ssize_t can_read(FAR struct file *filep, FAR char *buffer,
 
       fifo = &reader->fifo;
 
-      while (fifo->rx_head == fifo->rx_tail)
+      if (filep->f_oflags & O_NONBLOCK)
+        {
+          ret = nxsem_trywait(&fifo->rx_sem);

Review comment:
       should we add can_trysem or remove the null wrapper of can_takesem and can_givesem?

##########
File path: drivers/can/can.c
##########
@@ -609,24 +609,24 @@ static ssize_t can_read(FAR struct file *filep, FAR char *buffer,
 
       fifo = &reader->fifo;
 
-      while (fifo->rx_head == fifo->rx_tail)
+      if (filep->f_oflags & O_NONBLOCK)

Review comment:
       the critical section is held at line 568, why not directly check the queue empty directly instead calling nxsem_trywait?

##########
File path: include/nuttx/can/can.h
##########
@@ -491,7 +491,13 @@ begin_packed_struct struct can_msg_s
 
 struct can_rxfifo_s
 {
-  sem_t         rx_sem;                  /* Counting semaphore */
+  /* Binary semaphore. Indicates whether FIFO is available for reading
+   * AND not empty. Only take this sem inside a critical section to guarantee
+   * exclusive access to both the semaphore and the head/tail FIFO indices.
+   */
+
+  sem_t         rx_sem;

Review comment:
       but, the code still use it as the lock, not an indicator?




-- 
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: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org