You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "shizacat (via GitHub)" <gi...@apache.org> on 2026/04/18 18:36:08 UTC
[PR] drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header [nuttx]
shizacat opened a new pull request, #18759:
URL: https://github.com/apache/nuttx/pull/18759
## Summary
This PR hardens the generic CAN character driver (drivers/can/can.c) and the STM32 bxCAN backend (arch/arm/src/stm32/stm32_can.c) for write-only use, safe shutdown, and consistent RX headers.
- Per-file context on every open: init_can_reader() runs for all successful opens so write-only fds get msgalign and ioctl-related state. Readers are still only queued on cd_readers when O_RDOK is set.
- can_close: frees that per-file context when the fd was write-only (never on cd_readers), fixing a leak.
- Last close / TX drain: leaves the critical section before waiting so TX-complete ISRs can run; bounds SW-queue and H/W drain waits (20 × 500 ms per stage) so close() cannot hang forever when the controller retries without ACK or dev_txempty() never clears.
- STM32 RX path: zeroes the full can_hdr before filling it so padding/reserved fields are not stack garbage—avoids false memcmp mismatches vs TX (e.g. examples/can with CONFIG_CAN_LOOPBACK).
- Debug: stm32can_vputreg() log uses value, not an undefined val.
## Impact
- Loopback / full-header compare: Without clearing the RX header, apps that compare the whole header (loopback tests) could see spurious failures; this change makes RX headers deterministic.
- close(): Unbounded waits on TX drain could freeze the app on a silent bus or endless retries; timeouts avoid infinite blocking while still calling dev_shutdown afterward.
- Write-only open: Previously f_priv could stay NULL, breaking can_write (msgalign) and some ioctls—risk of fault/panic; allocating can_reader_s for every open fixes that.
## Testing
Manual testing on stm32f103-minimum with a custom config (CAN loopback / Example CAN / app scenario as applicable).
--
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
Re: [PR] drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header [nuttx]
Posted by "shizacat (via GitHub)" <gi...@apache.org>.
shizacat commented on code in PR #18759:
URL: https://github.com/apache/nuttx/pull/18759#discussion_r3113510675
##########
arch/arm/src/stm32/stm32_can.c:
##########
@@ -1533,6 +1533,14 @@ static int stm32can_rxinterrupt(struct can_dev_s *dev, int rxmb)
DEBUGASSERT(dev != NULL && dev->cd_priv != NULL);
priv = dev->cd_priv;
+ /* Clear full header:
+ * padding and optional fields (timestamp, etc.) must
+ * not contain stack garbage or memcmp() in apps (e.g. examples/can with
+ * CONFIG_CAN_LOOPBACK) will falsely fail vs. zero-initialized TX header.
+ */
+
+ memset(&hdr, 0, sizeof(hdr));
Review Comment:
Ok
--
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
Re: [PR] drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header [nuttx]
Posted by "shizacat (via GitHub)" <gi...@apache.org>.
shizacat commented on code in PR #18759:
URL: https://github.com/apache/nuttx/pull/18759#discussion_r3133320744
##########
drivers/can/can.c:
##########
@@ -347,16 +376,33 @@ static int can_close(FAR struct file *filep)
/* Now we wait for the sender to clear */
- while (!TX_EMPTY(&dev->cd_sender))
{
Review Comment:
done
##########
drivers/can/can.c:
##########
@@ -347,16 +376,33 @@ static int can_close(FAR struct file *filep)
/* Now we wait for the sender to clear */
- while (!TX_EMPTY(&dev->cd_sender))
{
- nxsched_usleep(HALF_SECOND_USEC);
+ for (n = 0;
+ !TX_EMPTY(&dev->cd_sender) && n < CAN_CLOSE_DRAIN_LOOPS;
+ n++)
+ {
+ nxsched_usleep(HALF_SECOND_USEC);
+ }
+
+ 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))
{
Review Comment:
done
--
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
Re: [PR] drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header [nuttx]
Posted by "shizacat (via GitHub)" <gi...@apache.org>.
shizacat commented on code in PR #18759:
URL: https://github.com/apache/nuttx/pull/18759#discussion_r3113183410
##########
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:
done
##########
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:
done
--
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
Re: [PR] drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #18759:
URL: https://github.com/apache/nuttx/pull/18759#discussion_r3130885702
##########
drivers/can/can.c:
##########
@@ -305,12 +323,15 @@ static int can_close(FAR struct file *filep)
flags = enter_critical_section(); /* Disable interrupts */
+ priv = (FAR struct can_reader_s *)filep->f_priv;
Review Comment:
merge to line 09 and 310
##########
drivers/can/can.c:
##########
@@ -347,16 +376,33 @@ static int can_close(FAR struct file *filep)
/* Now we wait for the sender to clear */
- while (!TX_EMPTY(&dev->cd_sender))
{
- nxsched_usleep(HALF_SECOND_USEC);
+ for (n = 0;
+ !TX_EMPTY(&dev->cd_sender) && n < CAN_CLOSE_DRAIN_LOOPS;
+ n++)
+ {
+ nxsched_usleep(HALF_SECOND_USEC);
+ }
+
+ 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))
{
Review Comment:
remove {}
##########
drivers/can/can.c:
##########
@@ -347,16 +376,33 @@ static int can_close(FAR struct file *filep)
/* Now we wait for the sender to clear */
- while (!TX_EMPTY(&dev->cd_sender))
{
Review Comment:
remove {}
--
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
Re: [PR] drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header [nuttx]
Posted by "shizacat (via GitHub)" <gi...@apache.org>.
shizacat commented on code in PR #18759:
URL: https://github.com/apache/nuttx/pull/18759#discussion_r3133321637
##########
drivers/can/can.c:
##########
@@ -305,12 +323,15 @@ static int can_close(FAR struct file *filep)
flags = enter_critical_section(); /* Disable interrupts */
+ priv = (FAR struct can_reader_s *)filep->f_priv;
Review Comment:
done
--
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
Re: [PR] drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header [nuttx]
Posted by "raiden00pl (via GitHub)" <gi...@apache.org>.
raiden00pl commented on code in PR #18759:
URL: https://github.com/apache/nuttx/pull/18759#discussion_r3109587634
##########
arch/arm/src/stm32/stm32_can.c:
##########
@@ -1533,6 +1533,14 @@ static int stm32can_rxinterrupt(struct can_dev_s *dev, int rxmb)
DEBUGASSERT(dev != NULL && dev->cd_priv != NULL);
priv = dev->cd_priv;
+ /* Clear full header:
+ * padding and optional fields (timestamp, etc.) must
+ * not contain stack garbage or memcmp() in apps (e.g. examples/can with
+ * CONFIG_CAN_LOOPBACK) will falsely fail vs. zero-initialized TX header.
+ */
+
+ memset(&hdr, 0, sizeof(hdr));
Review Comment:
I think that only the timestamp is not set and can be garbage. If the problem is only with the timestamp, it may be better to zero its fields directly rather than using memset. I guess we'll save a few CPU cycles.
This problem can occur for other chips, not only stm32. The PR that added timestamp support for CAN character driver should reset this data filed for all lower CAN drivers.
--
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
Re: [PR] drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header [nuttx]
Posted by "shizacat (via GitHub)" <gi...@apache.org>.
shizacat commented on code in PR #18759:
URL: https://github.com/apache/nuttx/pull/18759#discussion_r3107147017
##########
arch/arm/src/stm32/stm32_can.c:
##########
@@ -1533,6 +1533,14 @@ static int stm32can_rxinterrupt(struct can_dev_s *dev, int rxmb)
DEBUGASSERT(dev != NULL && dev->cd_priv != NULL);
priv = dev->cd_priv;
+ /* Clear full header:
+ * padding and optional fields (timestamp, etc.) must
+ * not contain stack garbage or memcmp() in apps (e.g. examples/can with
+ * CONFIG_CAN_LOOPBACK) will falsely fail vs. zero-initialized TX header.
+ */
+
+ memset(&hdr, 0, sizeof(hdr));
Review Comment:
I can’t reproduce this anymore with the fully modified code, so the issue might not be easily observable...
Do you think we should explicitly initialize the entire struct can_hdr_s (e.g., with memset), or rely on all fields being set individually before use?
---
Looking at the code
CAN, need fix every place that builds a struct can_hdr_s: stm32can_sceinterrupt and stm32can_rxinterrupt:
- stm32f7/stm32_can.c — yes
- stm32l4/stm32l4_can.c — yes
For FDCAN, the same idea applies to functions: fdcan_receive and fdcan_error:
- stm32h5/stm32_fdcan.c - yes
- stm32f0l0g0/stm32_fdcan.c - yes
--
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
Re: [PR] drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header [nuttx]
Posted by "shizacat (via GitHub)" <gi...@apache.org>.
shizacat commented on code in PR #18759:
URL: https://github.com/apache/nuttx/pull/18759#discussion_r3113360868
##########
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:
... yep, you're right. I returned it as it was
--
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
Re: [PR] drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header [nuttx]
Posted by "shizacat (via GitHub)" <gi...@apache.org>.
shizacat commented on PR #18759:
URL: https://github.com/apache/nuttx/pull/18759#issuecomment-4332306122
@acassis What to do also need for MR?
--
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
Re: [PR] drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header [nuttx]
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on code in PR #18759:
URL: https://github.com/apache/nuttx/pull/18759#discussion_r3105899378
##########
arch/arm/src/stm32/stm32_can.c:
##########
@@ -1533,6 +1533,14 @@ static int stm32can_rxinterrupt(struct can_dev_s *dev, int rxmb)
DEBUGASSERT(dev != NULL && dev->cd_priv != NULL);
priv = dev->cd_priv;
+ /* Clear full header:
+ * padding and optional fields (timestamp, etc.) must
+ * not contain stack garbage or memcmp() in apps (e.g. examples/can with
+ * CONFIG_CAN_LOOPBACK) will falsely fail vs. zero-initialized TX header.
+ */
+
+ memset(&hdr, 0, sizeof(hdr));
Review Comment:
@shizacat could you please verify if stm32f7, h7, and others using the same stm32_can.c driver need this fix too?
##########
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)
{
- if (((FAR struct can_reader_s *)node) ==
- ((FAR struct can_reader_s *)filep->f_priv))
+ FAR struct can_reader_s *priv =
+ (FAR struct can_reader_s *)filep->f_priv;
+ bool onlist = false;
Review Comment:
Please fix the alignment, the "=" are not at same column, and in this case could be better just use "bool onlist = false;"
--
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
Re: [PR] drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header [nuttx]
Posted by "shizacat (via GitHub)" <gi...@apache.org>.
shizacat commented on code in PR #18759:
URL: https://github.com/apache/nuttx/pull/18759#discussion_r3107005123
##########
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)
{
- if (((FAR struct can_reader_s *)node) ==
- ((FAR struct can_reader_s *)filep->f_priv))
+ FAR struct can_reader_s *priv =
+ (FAR struct can_reader_s *)filep->f_priv;
+ bool onlist = false;
Review Comment:
Fix
--
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
Re: [PR] drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header [nuttx]
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #18759:
URL: https://github.com/apache/nuttx/pull/18759#issuecomment-4335700205
> @acassis What to do also need for MR?
Everything is fine from my side, I was waiting for other to review too, but since nobody show up, I will merge now.
--
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
Re: [PR] drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header [nuttx]
Posted by "shizacat (via GitHub)" <gi...@apache.org>.
shizacat commented on PR #18759:
URL: https://github.com/apache/nuttx/pull/18759#issuecomment-4341545252
@acassis Thank you!
--
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
Re: [PR] drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header [nuttx]
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis merged PR #18759:
URL: https://github.com/apache/nuttx/pull/18759
--
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
Re: [PR] drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header [nuttx]
Posted by "shizacat (via GitHub)" <gi...@apache.org>.
shizacat commented on code in PR #18759:
URL: https://github.com/apache/nuttx/pull/18759#discussion_r3113695781
##########
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:
Rewritten this place as was
--
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