drivers: can: mcux: flexcan: fix MB overrun when CAN FD enabled #95097

william-tang914 · 2025-08-28T08:32:21Z

Fix message buffer (MB) overrun when CAN FD enabled by dynamically calculate the number of MB based on payload size and CAN instance capability. Maximum number of MB is less than classical CAN when CAN FD enabled.

Add new data structure member 'tx_mb_number' and 'rx_mb_number', which reflect real TX MB and RX MB per FlexCAN instance can accommodate.

Use 'tx_mb_number' and 'rx_mb_number' to define maximum semaphore count and traverse atomic bitmap, ensure safe concurrent access to shared resources on different FlexCAN instance.

This commit assumes 'tx_mb_number' less than or equal to 'MCUX_FLEXCAN_MAX_TX'. For S32 platform, it requires suitable Kconfig symbol 'CAN_MAX_MB' value to ensure above relation. MCUX platform can ignore this limit.

Fixes #92798

github-actions · 2025-08-28T08:33:02Z

Hello @william-tang914, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

zejiang0jason · 2025-08-28T13:22:28Z

This commit assumes 'tx_mb_number' less than or equal to 'MCUX_FLEXCAN_MAX_TX'. For S32 platform, it requires suitable Kconfig symbol 'CAN_MAX_MB' value to ensure above relation. MCUX platform can ignore this limit.

From the MCUX_FLEXCAN_MAX_MB implementation, seems CAN_MAX_MB must be defined for the SOCs which have different MB number by FlexCAN instance, because FSL_FEATURE_FLEXCAN_HAS_MESSAGE_BUFFER_MAX_NUMBERn(0) returns -1 for them, not only S32.

zephyr/drivers/can/can_mcux_flexcan.c

Lines 37 to 41 in 5eeddab

    
           #ifdef CONFIG_CAN_MAX_MB 
        
           #define MCUX_FLEXCAN_MAX_MB CONFIG_CAN_MAX_MB 
        
           #else 
        
           #define MCUX_FLEXCAN_MAX_MB FSL_FEATURE_FLEXCAN_HAS_MESSAGE_BUFFER_MAX_NUMBERn(0) 
        
           #endif

william-tang914 · 2025-08-29T10:37:06Z

Hi @zejiang0jason

For the SOCs which have different MB number by FlexCAN instance, user should configure CAN_MAX_MB by themselves.
This update was introduced in #60200.

I insert a guard clause for MCUX_FLEXCAN_MAX_RX overflow real accommodated MB max_mb.

zephyr/drivers/can/can_mcux_flexcan.c

Lines 1188 to 1191 in 533681f

    
           if (max_mb < MCUX_FLEXCAN_MAX_RX) { 
        
           	LOG_ERR("Not enough message buffers for RX"); 
        
           	return -ENODEV; 
        
           }

Another guard clause for tx_mb_number overflow MCUX_FLEXCAN_MAX_TX, which will result traverse atomic bitmap out of boundary.

zephyr/drivers/can/can_mcux_flexcan.c

Lines 1196 to 1199 in 533681f

    
           if (data->tx_mb_number > MCUX_FLEXCAN_MAX_TX) { 
        
           	LOG_ERR("Not enough message buffers for TX"); 
        
           	return -ENODEV; 
        
           }

dleach02

@henrikbrixandersen can you provide a review. This LGTM

str4t0m

I couldn't test it on hardware, but I think I needs a few changes to be able to run on hw where instances have a different number of message buffers.

str4t0m · 2025-09-17T17:30:00Z

drivers/can/can_mcux_flexcan.c

+	data->tx_mb_number = max_mb - MCUX_FLEXCAN_MAX_RX;
+	data->rx_mb_number = MCUX_FLEXCAN_MAX_RX;
+
+	if (data->tx_mb_number > MCUX_FLEXCAN_MAX_TX) {


As tx_mb_number is scaled for canfd I think we need to adopt the MCUX_FLEXCAN_MAX_TX as well don't we?

It seems that MCUX_FLEXCAN_MAX_TX is static, unless update MCUX_FLEXCAN_MAX_MB by Kconfig.

str4t0m · 2025-09-17T17:37:51Z

drivers/can/can_mcux_flexcan.c

+		return -ENODEV;
+	}
+
+	data->tx_mb_number = max_mb - MCUX_FLEXCAN_MAX_RX;


So here we will always use all physically available mb of the CAN instance. Doesn't that make the CONFIG_CAN_MAX_MB kind of unneeded, as it will also have to be set to the max value to avoid getting an error in the following tx_mb_number check. And consequently I think it makes it impossible to use two CAN instances of differing max mb number, as we always assign the same value to flexcan_config.maxMbNum(reg MCR.MAXMB).

In the current implementation, CONFIG_CAN_MAX_MB is used to statically define the size of tx_cbs and tx_allocs via MCUX_FLEXCAN_MAX_TX. If we remove this macro, we can no longer rely on compile-time constants for these arrays, and would need to switch to dynamic initialization instead.

While dynamic allocation is technically feasible, I'm not sure if it's appropriate to introduce such a change within this pull request, as it affects memory layout and initialization logic. It might be better to keep the static allocation for now, or handle the transition to dynamic allocation in a separate PR to keep changes scoped and easier to review.

zephyr/drivers/can/can_mcux_flexcan.c

Lines 119 to 126 in 533681f

ATOMIC_DEFINE(rx_allocs, MCUX_FLEXCAN_MAX_RX);

struct k_mutex rx_mutex;

struct mcux_flexcan_rx_callback rx_cbs[MCUX_FLEXCAN_MAX_RX];

ATOMIC_DEFINE(tx_allocs, MCUX_FLEXCAN_MAX_TX);

struct k_sem tx_allocs_sem;

struct k_mutex tx_mutex;

struct mcux_flexcan_tx_callback tx_cbs[MCUX_FLEXCAN_MAX_TX];

I will update flexcan_config.maxMbNum = MCUX_FLEXCAN_MAX_MB to flexcan_config.maxMbNum = max_mb.

I wouldn't want go toward dynamic allocation either.
My concern is just that in case FSL_FEATURE_FLEXCAN_HAS_MESSAGE_BUFFER_MAX_NUMBERn(x) and MCUX_FLEXCAN_MAX_MB differ we might use more Buffers than configured in MCR.MAXMB.
And the following tx_mb_number check will not be triggered in FD mode.
I'll have to check this in more detail and will try to clarify what I meant, when I have more time, hopefully in the evening.

Hello @str4t0m , let me introduce intent of the Pull Request.

The maximum number of FlexCAN message buffers (mailboxes) is determined by the following factors:

Whether CAN FD is enabled.

If CAN FD is enabled, the amount of memory allocated per mailbox (in this case, 64 bytes).

The number of memory blocks available to each FlexCAN instance. Each instance's memory blocks is different on some SoCs (e.g., the S32K3 series).

Based on these three factors, I calculate the actual maximum number of mailboxes for each FlexCAN instance. Then, using the number of receive mailboxes configured via Kconfig, I compute the remaining available transmit mailboxes. Finally, I assign the actual maximum mailbox count to MCR[MAXMB].(will update)

The check max_mb < MCUX_FLEXCAN_MAX_RX ensures that the user does not configure more receive mailboxes than the hardware can support.

The check data->tx_mb_number > MCUX_FLEXCAN_MAX_TX prevents out-of-bounds access when iterating over the tx_cbs array. Expanding this condition leads to max_mb > MCUX_FLEXCAN_MAX_MB, which can only occur if:

The number of memory blocks differs between FlexCAN instances, and

The user incorrectly sets MCUX_FLEXCAN_MAX_MB via Kconfig.

This PR aims to make the mailbox allocation more robust and adaptable to different SoC configurations, while also preventing misconfiguration-related issues.

To fully resolve this issue, the driver would need to dynamically calculate the actual maximum number of mailboxes for each FlexCAN instance, or alternatively, this information could be provided via the device tree.

I think each SoC should disclose the amount of mailboxes for each FlexCAN instance in the SoC device tree. Similar how it's done for the FIFO sizes for LPSPI.
https://docs.zephyrproject.org/latest/build/dts/api/bindings/spi/nxp%2Clpspi.html

I agree, device tree shall provide memory blocks each FlexCAN instance support, then calculate amount of mailboxes in driver. Payload size determines maximum number of message buffers per RAM block.
I'm not sure if it's appropriate to introduce device tree update in this PR considering granularity.

I've been meaning to introduce dts properties for this for quite some time (it's also needed by the Zephyr-native FlexCAN driver, I am working on), but I have not gotten around to it yet.

Hello @henrikbrixandersen , I'm glad you are trying to add mailboxes property for Zephyr-native CAN driver. Would you mind sharing me your plan or timeline to fix this issue?

Hello @henrikbrixandersen , shall we apply a workaround before introduce mailbox to device tree?
On many platforms, there are only 7 mailboxes for CAN FD. So this pull request is necessary for those platfroms. Thanks a lot.
#91138

Fix message buffer (MB) overrun when CAN FD enabled by dynamically calculate the number of MB based on payload size and CAN instance capability. Maximum number of MB is less than classical CAN when CAN FD enabled. Add new data structure member 'tx_mb_number' and 'rx_mb_number', which reflect real TX MB and RX MB per FlexCAN instance can accommodate. Use 'tx_mb_number' and 'rx_mb_number' to define maximum semaphore count and traverse atomic bitmap, ensure safe concurrent access to shared resources on different FlexCAN instance. This commit assumes 'tx_mb_number' less than or equal to 'MCUX_FLEXCAN_MAX_TX'. For S32 platform, it requires suitable Kconfig symbol 'CAN_MAX_MB' value to ensure above relation. MCUX platform can ignore this limit. Fixes zephyrproject-rtos#92798 Signed-off-by: William Tang <[email protected]>

sonarqubecloud · 2025-09-27T02:30:52Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
3.9% Duplication on New Code

See analysis details on SonarQube Cloud

dleach02 · 2025-10-21T17:54:58Z

What is the plan for this PR

zephyrbot added area: CAN platform: NXP Drivers NXP Semiconductors, drivers labels Aug 28, 2025

zephyrbot requested review from alexanderwachter, dbaluta, decsny, dleach02, henrikbrixandersen, manuargue, martinjaeger, mmahadevan108 and str4t0m August 28, 2025 08:33

zephyrbot assigned dleach02 and mmahadevan108 Aug 28, 2025

henrikbrixandersen self-assigned this Aug 28, 2025

dleach02 previously approved these changes Sep 17, 2025

View reviewed changes

str4t0m requested changes Sep 17, 2025

View reviewed changes

william-tang914 dismissed dleach02’s stale review via 16ba31a September 27, 2025 02:26

william-tang914 force-pushed the bugfix/can-mcux-flexcan-allows-mailbox-overrun branch from 533681f to 16ba31a Compare September 27, 2025 02:26

zephyrbot added the platform: NXP NXP label Sep 27, 2025

vChavezB mentioned this pull request Sep 27, 2025

can: s32k1 flexcan fd driver max message buffers not checked #88069

Open

str4t0m self-requested a review October 16, 2025 10:57

PetervdPerk-NXP mentioned this pull request Oct 22, 2025

Add mcxaxx6 flexcan support #97929

Open

dleach02 added the bug The issue is a bug, or the PR is fixing a bug label Oct 24, 2025

	ATOMIC_DEFINE(rx_allocs, MCUX_FLEXCAN_MAX_RX);
	struct k_mutex rx_mutex;
	struct mcux_flexcan_rx_callback rx_cbs[MCUX_FLEXCAN_MAX_RX];

	ATOMIC_DEFINE(tx_allocs, MCUX_FLEXCAN_MAX_TX);
	struct k_sem tx_allocs_sem;
	struct k_mutex tx_mutex;
	struct mcux_flexcan_tx_callback tx_cbs[MCUX_FLEXCAN_MAX_TX];

drivers: can: mcux: flexcan: fix MB overrun when CAN FD enabled #95097

Are you sure you want to change the base?

drivers: can: mcux: flexcan: fix MB overrun when CAN FD enabled #95097

Conversation

william-tang914 commented Aug 28, 2025

Uh oh!

github-actions bot commented Aug 28, 2025

Uh oh!

zejiang0jason commented Aug 28, 2025

Uh oh!

william-tang914 commented Aug 29, 2025

Uh oh!

dleach02 left a comment

Choose a reason for hiding this comment

Uh oh!

str4t0m left a comment

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!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Sep 27, 2025

Quality Gate passed

Uh oh!

dleach02 commented Oct 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants