drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header by shizacat · Pull Request #18759 · apache/nuttx

shizacat · 2026-04-18T18:36:04Z

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).

acassis · 2026-04-18T22:02:02Z

+   * CONFIG_CAN_LOOPBACK) will falsely fail vs. zero-initialized TX header.
+   */
+
+  memset(&hdr, 0, sizeof(hdr));


@shizacat could you please verify if stm32f7, h7, and others using the same stm32_can.c driver need this fix too?

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

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.

xiaoxiang781216 · 2026-04-20T09:04:35Z


  flags = enter_critical_section(); /* Disable interrupts */

-  list_for_every(&dev->cd_readers, node)


why change? the behavour is same before/after modfication

Rewritten this place as was

xiaoxiang781216 · 2026-04-20T09:05:42Z

-  while (!TX_EMPTY(&dev->cd_sender))
    {
-      nxsched_usleep(HALF_SECOND_USEC);
+      unsigned int n = 0;


move n declaration to the beginning, and
for (n = 0; !TX_EMPTY(&dev->cd_sender) && n < CAN_CLOSE_DRAIN_LOOPS; n++)

xiaoxiang781216 · 2026-04-20T09:06:09Z

-      nxsched_usleep(HALF_SECOND_USEC);
+      unsigned int n = 0;
+
+      while (!dev_txempty(dev) && n < CAN_CLOSE_DRAIN_LOOPS)


xiaoxiang781216 · 2026-04-20T09:07:06Z

      goto errout;
    }

+  /* Last client: release the critical section before draining TX.


but new client may open device again after leaving critical section, how to handle this case

... yep, you're right. I returned it as it was

…RX header Always allocate per-file can_reader on open so write-only descriptors get msgalign / ioctl state; free that context on close when it was never linked into cd_readers (fixes leak). Cap each drain loop (20×500 ms) so close() cannot block forever when bxCAN retries without ACK or dev_txempty never clears. Fix stm32can_vputreg debug log to print the written value (correct parameter name). Signed-off-by: Alexey Matveev <tippet@yandex.ru>

shizacat requested review from acassis, michallenc and raiden00pl as code owners April 18, 2026 18:36

github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Area: Drivers Drivers issues Size: M The size of the change in this PR is medium labels Apr 18, 2026

shizacat force-pushed the update-arch-can-driver-stm32 branch from c9537e6 to e1149b7 Compare April 18, 2026 18:44

acassis reviewed Apr 18, 2026

View reviewed changes

acassis requested review from linguini1 and xiaoxiang781216 April 18, 2026 22:02

shizacat force-pushed the update-arch-can-driver-stm32 branch from e1149b7 to 267649d Compare April 19, 2026 14:48

xiaoxiang781216 reviewed Apr 20, 2026

View reviewed changes

shizacat force-pushed the update-arch-can-driver-stm32 branch 4 times, most recently from 198c069 to c57c35d Compare April 20, 2026 20:34

github-actions bot added Size: S The size of the change in this PR is small and removed Size: M The size of the change in this PR is medium labels Apr 20, 2026

shizacat force-pushed the update-arch-can-driver-stm32 branch from c57c35d to 9254bc3 Compare April 20, 2026 21:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header#18759

drivers/can: Fix close drain, write-only reader lifecycle, and STM32 RX header#18759
shizacat wants to merge 1 commit intoapache:masterfrom
shizacat:update-arch-can-driver-stm32

shizacat commented Apr 18, 2026

Uh oh!

Uh oh!

acassis Apr 18, 2026

Uh oh!

shizacat Apr 19, 2026

Uh oh!

raiden00pl Apr 20, 2026

Uh oh!

shizacat Apr 20, 2026

Uh oh!

xiaoxiang781216 Apr 20, 2026

Uh oh!

shizacat Apr 20, 2026

Uh oh!

xiaoxiang781216 Apr 20, 2026

Uh oh!

shizacat Apr 20, 2026

Uh oh!

xiaoxiang781216 Apr 20, 2026

Uh oh!

shizacat Apr 20, 2026

Uh oh!

xiaoxiang781216 Apr 20, 2026

Uh oh!

shizacat Apr 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		flags = enter_critical_section(); /* Disable interrupts */

		list_for_every(&dev->cd_readers, node)

Conversation

shizacat commented Apr 18, 2026

Summary

Impact

Testing

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants