xiaoxiang781216 commented on code in PR #18759:
URL: https://github.com/apache/nuttx/pull/18759#discussion_r3109472674
##########
drivers/can/can.c:
##########
@@ -305,29 +320,43 @@ static int can_close(FAR struct file *filep)
flags = enter_critical_section(); /* Disable interrupts */
- list_for_every(&dev->cd_readers, node)
Review Comment:
why change? the behavour is same before/after modfication
##########
drivers/can/can.c:
##########
@@ -341,26 +370,56 @@ static int can_close(FAR struct file *filep)
goto errout;
}
+ /* Last client: release the critical section before draining TX.
+ * Long sleeps with IRQs masked can prevent the TX-complete ISR
+ * from running and stall the SW queue; the same applies when
+ * the controller retries on NACK.
+ */
+
+ leave_critical_section(flags);
+
/* Stop accepting input */
dev_rxint(dev, false);
/* Now we wait for the sender to clear */
- while (!TX_EMPTY(&dev->cd_sender))
{
- nxsched_usleep(HALF_SECOND_USEC);
+ unsigned int n = 0;
Review Comment:
move n declaration to the beginning, and
`for (n = 0; !TX_EMPTY(&dev->cd_sender) && n < CAN_CLOSE_DRAIN_LOOPS; n++)`
##########
drivers/can/can.c:
##########
@@ -341,26 +370,56 @@ static int can_close(FAR struct file *filep)
goto errout;
}
+ /* Last client: release the critical section before draining TX.
Review Comment:
but new client may open device again after leaving critical section, how to
handle this case
##########
drivers/can/can.c:
##########
@@ -341,26 +370,56 @@ static int can_close(FAR struct file *filep)
goto errout;
}
+ /* Last client: release the critical section before draining TX.
+ * Long sleeps with IRQs masked can prevent the TX-complete ISR
+ * from running and stall the SW queue; the same applies when
+ * the controller retries on NACK.
+ */
+
+ leave_critical_section(flags);
+
/* Stop accepting input */
dev_rxint(dev, false);
/* Now we wait for the sender to clear */
- while (!TX_EMPTY(&dev->cd_sender))
{
- nxsched_usleep(HALF_SECOND_USEC);
+ unsigned int n = 0;
+
+ while (!TX_EMPTY(&dev->cd_sender) && n < CAN_CLOSE_DRAIN_LOOPS)
+ {
+ nxsched_usleep(HALF_SECOND_USEC);
+ n++;
+ }
+
+ if (!TX_EMPTY(&dev->cd_sender))
+ {
+ canerr("CAN close: SW TX queue still not empty after timeout\n");
+ }
}
/* And wait for the hardware sender to drain */
- while (!dev_txempty(dev))
{
- nxsched_usleep(HALF_SECOND_USEC);
+ unsigned int n = 0;
+
+ while (!dev_txempty(dev) && n < CAN_CLOSE_DRAIN_LOOPS)
Review Comment:
ditto
--
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]
