drivers: can: remove run-time RTR filtering, add build-time RTR filter

A growing number of CAN controllers do not have support for individual RX
hardware filters based on the Remote Transmission Request (RTR) bit. This
leads to various work-arounds on the driver level mixing hardware and
software filtering.

As the use of RTR frames is discouraged by CAN in Automation (CiA) - and
not even supported by newer standards, e.g. CAN FD - this often leads to
unnecessary overhead, added complexity, and worst-case to non-portable
behavior between various CAN controller drivers.

Instead, move to a simpler approach where the ability to accept/reject RTR
frames is globally configured via Kconfig. By default, all incoming RTR
frames are rejected at the driver level, a setting which can be supported
in hardware by most in-tree CAN controllers drivers.

Legacy applications or protocol implementations, where RTR reception is
required, can now select CONFIG_CAN_ACCEPT_RTR to accept incoming RTR
frames matching added CAN filters. These applications or protocols will
need to distinguish between RTR and data frames in their respective CAN RX
frame handling routines.

Signed-off-by: Henrik Brix Andersen <hebad@vestas.com>

doc/hardware/peripherals/can/controller.rst

@@ -203,7 +203,7 @@ The filter for this example is configured to match the identifier 0x123 exactly.
 .. code-block:: C
 
   const struct can_filter my_filter = {
-          .flags = CAN_FILTER_DATA,
+          .flags = 0U,
           .id = 0x123,
           .mask = CAN_STD_ID_MASK
   };
@@ -226,7 +226,7 @@ The filter for this example is configured to match the extended identifier
 .. code-block:: C
 
   const struct can_filter my_filter = {
-          .flags = CAN_FILTER_DATA | CAN_FILTER_IDE,
+          .flags = CAN_FILTER_IDE,
           .id = 0x1234567,
           .mask = CAN_EXT_ID_MASK
   };

drivers/can/Kconfig

@@ -54,6 +54,13 @@ config CAN_STATS
 	help
 	  Enable CAN controller device statistics.
 
+config CAN_ACCEPT_RTR
+	bool "Accept Remote Transmission Requests (RTR) frames"
+	help
+	  Accept incoming Remote Transmission Request (RTR) frames matching CAN RX filters. Unless
+	  enabled, all incoming Remote Transmission Request (RTR) frames are rejected at the driver
+	  level.
+
 config CAN_FD_MODE
 	bool "CAN FD"
 	help

drivers/can/can_loopback.c

@@ -81,6 +81,12 @@ static void tx_thread(void *arg1, void *arg2, void *arg3)
 			continue;
 		}
 
+#ifndef CONFIG_CAN_ACCEPT_RTR
+		if ((frame.frame.flags & CAN_FRAME_RTR) != 0U) {
+			continue;
+		}
+#endif /* !CONFIG_CAN_ACCEPT_RTR */
+
 		k_mutex_lock(&data->mtx, K_FOREVER);
 
 		for (int i = 0; i < CONFIG_CAN_MAX_FILTER; i++) {
@@ -177,10 +183,9 @@ static int can_loopback_add_rx_filter(const struct device *dev, can_rx_callback_
 	LOG_DBG("Setting filter ID: 0x%x, mask: 0x%x", filter->id, filter->mask);
 
 #ifdef CONFIG_CAN_FD_MODE
-	if ((filter->flags & ~(CAN_FILTER_IDE | CAN_FILTER_DATA |
-							CAN_FILTER_RTR | CAN_FILTER_FDF)) != 0) {
+	if ((filter->flags & ~(CAN_FILTER_IDE | CAN_FILTER_FDF)) != 0) {
 #else
-	if ((filter->flags & ~(CAN_FILTER_IDE | CAN_FILTER_DATA | CAN_FILTER_RTR)) != 0) {
+	if ((filter->flags & ~CAN_FILTER_IDE) != 0) {
 #endif
 		LOG_ERR("unsupported CAN filter flags 0x%02x", filter->flags);
 		return -ENOTSUP;

drivers/can/can_mcan.c

@@ -672,16 +672,6 @@ static void can_mcan_get_message(const struct device *dev, uint16_t fifo_offset,
 			flags = cbs->std[filt_idx].flags;
 		}
 
-		if (((frame.flags & CAN_FRAME_RTR) == 0U && (flags & CAN_FILTER_DATA) == 0U) ||
-		    ((frame.flags & CAN_FRAME_RTR) != 0U && (flags & CAN_FILTER_RTR) == 0U)) {
-			/* RTR bit does not match filter, drop frame */
-			err = can_mcan_write_reg(dev, fifo_ack_reg, get_idx);
-			if (err != 0) {
-				return;
-			}
-			goto ack;
-		}
-
 		if (((frame.flags & CAN_FRAME_FDF) != 0U && (flags & CAN_FILTER_FDF) == 0U) ||
 		    ((frame.flags & CAN_FRAME_FDF) == 0U && (flags & CAN_FILTER_FDF) != 0U)) {
 			/* FDF bit does not match filter, drop frame */
@@ -1121,10 +1111,9 @@ int can_mcan_add_rx_filter(const struct device *dev, can_rx_callback_t callback,
 	}
 
 #ifdef CONFIG_CAN_FD_MODE
-	if ((filter->flags &
-	     ~(CAN_FILTER_IDE | CAN_FILTER_DATA | CAN_FILTER_RTR | CAN_FILTER_FDF)) != 0U) {
+	if ((filter->flags & ~(CAN_FILTER_IDE | CAN_FILTER_FDF)) != 0U) {
 #else  /* CONFIG_CAN_FD_MODE */
-	if ((filter->flags & ~(CAN_FILTER_IDE | CAN_FILTER_DATA | CAN_FILTER_RTR)) != 0U) {
+	if ((filter->flags & ~(CAN_FILTER_IDE)) != 0U) {
 #endif /* !CONFIG_CAN_FD_MODE */
 		LOG_ERR("unsupported CAN filter flags 0x%02x", filter->flags);
 		return -ENOTSUP;
@@ -1444,6 +1433,9 @@ int can_mcan_init(const struct device *dev)
 	}
 
 	reg |= FIELD_PREP(CAN_MCAN_GFC_ANFE, 0x2) | FIELD_PREP(CAN_MCAN_GFC_ANFS, 0x2);
+	if (!IS_ENABLED(CONFIG_CAN_ACCEPT_RTR)) {
+		reg |= CAN_MCAN_GFC_RRFS | CAN_MCAN_GFC_RRFE;
+	}
 
 	err = can_mcan_write_reg(dev, CAN_MCAN_GFC, reg);
 	if (err != 0) {

drivers/can/can_mcp2515.c

@@ -629,7 +629,7 @@ static int mcp2515_add_rx_filter(const struct device *dev,
 
 	__ASSERT(rx_cb != NULL, "response_ptr can not be null");
 
-	if ((filter->flags & ~(CAN_FILTER_IDE | CAN_FILTER_DATA | CAN_FILTER_RTR)) != 0) {
+	if ((filter->flags & ~(CAN_FILTER_IDE)) != 0) {
 		LOG_ERR("unsupported CAN filter flags 0x%02x", filter->flags);
 		return -ENOTSUP;
 	}
@@ -691,6 +691,12 @@ static void mcp2515_rx_filter(const struct device *dev,
 	can_rx_callback_t callback;
 	struct can_frame tmp_frame;
 
+#ifndef CONFIG_CAN_ACCEPT_RTR
+	if ((frame->flags & CAN_FRAME_RTR) != 0U) {
+		return;
+	}
+#endif /* !CONFIG_CAN_ACCEPT_RTR */
+
 	k_mutex_lock(&dev_data->mutex, K_FOREVER);
 
 	for (; filter_id < CONFIG_CAN_MAX_FILTER; filter_id++) {

drivers/can/can_mcp251xfd.c

@@ -576,11 +576,6 @@ static int mcp251xfd_add_rx_filter(const struct device *dev, can_rx_callback_t r
 		goto done;
 	}
 
-	if ((filter->flags & CAN_FILTER_RTR) != 0) {
-		filter_idx = -ENOTSUP;
-		goto done;
-	}
-
 	reg = mcp251xfd_get_spi_buf_ptr(dev);
 
 	if ((filter->flags & CAN_FILTER_IDE) != 0) {
@@ -1266,6 +1261,12 @@ static void mcp251xfd_rx_fifo_handler(const struct device *dev, void *data)
 
 	mcp251xfd_rxobj_to_canframe(rxobj, &dst);
 
+#ifndef CONFIG_CAN_ACCEPT_RTR
+	if ((dst.flags & CAN_FRAME_RTR) != 0U) {
+		return;
+	}
+#endif /* !CONFIG_CAN_ACCEPT_RTR */
+
 	filhit = FIELD_GET(MCP251XFD_OBJ_FILHIT_MASK, rxobj->flags);
 	if ((dev_data->filter_usage & BIT(filhit)) != 0) {
 		LOG_DBG("Received msg CAN id: 0x%x", dst.id);

drivers/can/can_mcux_flexcan.c

@@ -636,8 +636,7 @@ static void mcux_flexcan_can_filter_to_mbconfig(const struct can_filter *src,
 						uint32_t *mask)
 {
 	static const uint32_t ide_mask = 1U;
-	uint32_t rtr_mask = (src->flags & (CAN_FILTER_DATA | CAN_FILTER_RTR)) !=
-		(CAN_FILTER_DATA | CAN_FILTER_RTR) ? 1U : 0U;
+	static const uint32_t rtr_mask = !IS_ENABLED(CONFIG_CAN_ACCEPT_RTR);
 
 	if ((src->flags & CAN_FILTER_IDE) != 0) {
 		dest->format = kFLEXCAN_FrameFormatExtend;
@@ -649,11 +648,7 @@ static void mcux_flexcan_can_filter_to_mbconfig(const struct can_filter *src,
 		*mask = FLEXCAN_RX_MB_STD_MASK(src->mask, rtr_mask, ide_mask);
 	}
 
-	if ((src->flags & CAN_FILTER_RTR) != 0) {
-		dest->type = kFLEXCAN_FrameTypeRemote;
-	} else {
-		dest->type = kFLEXCAN_FrameTypeData;
-	}
+	dest->type = kFLEXCAN_FrameTypeData;
 }
 
 static int mcux_flexcan_get_state(const struct device *dev, enum can_state *state,
@@ -796,7 +791,7 @@ static int mcux_flexcan_add_rx_filter(const struct device *dev,
 				      void *user_data,
 				      const struct can_filter *filter)
 {
-	uint8_t supported = CAN_FILTER_IDE | CAN_FILTER_DATA | CAN_FILTER_RTR;
+	uint8_t supported = CAN_FILTER_IDE;
 	const struct mcux_flexcan_config *config = dev->config;
 	struct mcux_flexcan_data *data = dev->data;
 	status_t status;

drivers/can/can_native_linux.c

@@ -105,6 +105,12 @@ static void rx_thread(void *arg1, void *arg2, void *arg3)
 
 			socketcan_to_can_frame(&sframe, &frame);
 
+#ifndef CONFIG_CAN_ACCEPT_RTR
+			if ((frame.flags & CAN_FRAME_RTR) != 0U) {
+				continue;
+			}
+#endif /* !CONFIG_CAN_ACCEPT_RTR*/
+
 			LOG_DBG("Received %d bytes. Id: 0x%x, ID type: %s %s",
 				frame.dlc, frame.id,
 				(frame.flags & CAN_FRAME_IDE) != 0 ? "extended" : "standard",
@@ -198,10 +204,9 @@ static int can_native_linux_add_rx_filter(const struct device *dev, can_rx_callb
 		filter->mask);
 
 #ifdef CONFIG_CAN_FD_MODE
-	if ((filter->flags & ~(CAN_FILTER_IDE | CAN_FILTER_DATA |
-							CAN_FILTER_RTR | CAN_FILTER_FDF)) != 0) {
+	if ((filter->flags & ~(CAN_FILTER_IDE | CAN_FILTER_FDF)) != 0) {
 #else
-	if ((filter->flags & ~(CAN_FILTER_IDE | CAN_FILTER_DATA | CAN_FILTER_RTR)) != 0) {
+	if ((filter->flags & ~(CAN_FILTER_IDE)) != 0) {
 #endif
 		LOG_ERR("unsupported CAN filter flags 0x%02x", filter->flags);
 		return -ENOTSUP;

drivers/can/can_nxp_s32_canxl.c

@@ -431,9 +431,9 @@ static int can_nxp_s32_add_rx_filter(const struct device *dev,
 
 	__ASSERT_NO_MSG(callback != NULL);
 #if defined(CONFIG_CAN_FD_MODE)
-	if ((filter->flags & ~(CAN_FILTER_IDE | CAN_FILTER_DATA | CAN_FILTER_FDF)) != 0) {
+	if ((filter->flags & ~(CAN_FILTER_IDE | CAN_FILTER_FDF)) != 0) {
 #else
-	if ((filter->flags & ~(CAN_FILTER_IDE | CAN_FILTER_DATA)) != 0) {
+	if ((filter->flags & ~(CAN_FILTER_IDE)) != 0) {
 #endif
 		LOG_ERR("unsupported CAN filter flags 0x%02x", filter->flags);
 		return -ENOTSUP;
@@ -474,6 +474,10 @@ static int can_nxp_s32_add_rx_filter(const struct device *dev,
 		mask = ((filter->mask << CANXL_IP_ID_STD_SHIFT) & CANXL_IP_ID_STD_MASK);
 	}
 
+#ifndef CONFIG_CAN_ACCEPT_RTR
+	mask |= CANXL_MSG_DESCRIPTORS_MDFLT1FD_RTRMSK_MASK;
+#endif /* !CONFIG_CAN_ACCEPT_RTR */
+
 	Canexcel_Ip_EnterFreezeMode(config->instance);
 
 	Canexcel_Ip_SetRxIndividualMask(config->instance, mb_indx,

drivers/can/can_rcar.c

@@ -367,6 +367,12 @@ static void can_rcar_rx_filter_isr(const struct device *dev,
 	struct can_frame tmp_frame;
 	uint8_t i;
 
+#ifndef CONFIG_CAN_ACCEPT_RTR
+	if ((frame->flags & CAN_FRAME_RTR) != 0U) {
+		return;
+	}
+#endif /* !CONFIG_CAN_ACCEPT_RTR */
+
 	for (i = 0; i < CONFIG_CAN_RCAR_MAX_FILTER; i++) {
 		if (data->rx_callback[i] == NULL) {
 			continue;
@@ -960,7 +966,7 @@ static int can_rcar_add_rx_filter(const struct device *dev, can_rx_callback_t cb
 	struct can_rcar_data *data = dev->data;
 	int filter_id;
 
-	if ((filter->flags & ~(CAN_FILTER_IDE | CAN_FILTER_DATA)) != 0) {
+	if ((filter->flags & ~(CAN_FILTER_IDE)) != 0) {
 		LOG_ERR("unsupported CAN filter flags 0x%02x", filter->flags);
 		return -ENOTSUP;
 	}

drivers/can/can_shell.c

@@ -801,7 +801,7 @@ static int cmd_can_filter_add(const struct shell *sh, size_t argc, char **argv)
 
 	/* Defaults */
 	max_id = CAN_MAX_STD_ID;
-	filter.flags = CAN_FILTER_DATA;
+	filter.flags = 0U;
 
 	/* Parse options */
 	while (argidx < argc && strncmp(argv[argidx], "-", 1) == 0) {
@@ -815,13 +815,6 @@ static int cmd_can_filter_add(const struct shell *sh, size_t argc, char **argv)
 		} else if (strcmp(argv[argidx], "-f") == 0) {
 			filter.flags |= CAN_FILTER_FDF;
 			argidx++;
-		} else if (strcmp(argv[argidx], "-r") == 0) {
-			filter.flags |= CAN_FILTER_RTR;
-			argidx++;
-		} else if (strcmp(argv[argidx], "-R") == 0) {
-			filter.flags &= ~(CAN_FILTER_DATA);
-			filter.flags |= CAN_FILTER_RTR;
-			argidx++;
 		} else {
 			shell_error(sh, "unsupported argument %s", argv[argidx]);
 			shell_help(sh);
@@ -878,13 +871,11 @@ static int cmd_can_filter_add(const struct shell *sh, size_t argc, char **argv)
 	}
 
 	shell_print(sh, "adding filter with %s (%d-bit) CAN ID 0x%0*x, "
-		    "CAN ID mask 0x%0*x, data frames %d, RTR frames %d, CAN FD frames %d",
+		    "CAN ID mask 0x%0*x, CAN FD frames %d",
 		    (filter.flags & CAN_FILTER_IDE) != 0 ? "extended" : "standard",
 		    (filter.flags & CAN_FILTER_IDE) != 0 ? 29 : 11,
 		    (filter.flags & CAN_FILTER_IDE) != 0 ? 8 : 3, filter.id,
 		    (filter.flags & CAN_FILTER_IDE) != 0 ? 8 : 3, filter.mask,
-		    (filter.flags & CAN_FILTER_DATA) != 0 ? 1 : 0,
-		    (filter.flags & CAN_FILTER_RTR) != 0 ? 1 : 0,
 		    (filter.flags & CAN_FILTER_FDF) != 0 ? 1 : 0);
 
 	err = can_add_rx_filter_msgq(dev, &can_shell_rx_msgq, &filter);
@@ -1003,12 +994,10 @@ SHELL_DYNAMIC_CMD_CREATE(dsub_can_device_name_mode, cmd_can_device_name_mode);
 SHELL_STATIC_SUBCMD_SET_CREATE(sub_can_filter_cmds,
 	SHELL_CMD_ARG(add, &dsub_can_device_name,
 		"Add rx filter\n"
-		"Usage: can filter add <device> [-e] [-f] [-r] [-R] <CAN ID> [CAN ID mask]\n"
+		"Usage: can filter add <device> [-e] [-f] <CAN ID> [CAN ID mask]\n"
 		"-e  use extended (29-bit) CAN ID/CAN ID mask\n"
-		"-f  match CAN FD format frames\n"
-		"-r  also match Remote Transmission Request (RTR) frames\n"
-		"-R  only match Remote Transmission Request (RTR) frames",
-		cmd_can_filter_add, 3, 5),
+		"-f  match CAN FD format frames\n",
+		cmd_can_filter_add, 3, 3),
 	SHELL_CMD_ARG(remove, &dsub_can_device_name,
 		"Remove rx filter\n"
 		"Usage: can filter remove <device> <filter_id>",

drivers/can/can_sja1000.c

@@ -427,7 +427,7 @@ int can_sja1000_add_rx_filter(const struct device *dev, can_rx_callback_t callba
 	int filter_id = -ENOSPC;
 	int i;
 
-	if ((filter->flags & ~(CAN_FILTER_IDE | CAN_FILTER_DATA | CAN_FILTER_RTR)) != 0) {
+	if ((filter->flags & ~(CAN_FILTER_IDE)) != 0) {
 		LOG_ERR("unsupported CAN filter flags 0x%02x", filter->flags);
 		return -ENOTSUP;
 	}
@@ -569,18 +569,24 @@ static void can_sja1000_handle_receive_irq(const struct device *dev)
 	do {
 		can_sja1000_read_frame(dev, &frame);
 
-		for (i = 0; i < ARRAY_SIZE(data->filters); i++) {
-			if (!atomic_test_bit(data->rx_allocs, i)) {
-				continue;
-			}
+#ifndef CONFIG_CAN_ACCEPT_RTR
+		if ((frame.flags & CAN_FRAME_RTR) == 0U) {
+#endif /* !CONFIG_CAN_ACCEPT_RTR */
+			for (i = 0; i < ARRAY_SIZE(data->filters); i++) {
+				if (!atomic_test_bit(data->rx_allocs, i)) {
+					continue;
+				}
 
-			if (can_frame_matches_filter(&frame, &data->filters[i].filter)) {
-				callback = data->filters[i].callback;
-				if (callback != NULL) {
-					callback(dev, &frame, data->filters[i].user_data);
+				if (can_frame_matches_filter(&frame, &data->filters[i].filter)) {
+					callback = data->filters[i].callback;
+					if (callback != NULL) {
+						callback(dev, &frame, data->filters[i].user_data);
+					}
 				}
 			}
+#ifndef CONFIG_CAN_ACCEPT_RTR
 		}
+#endif /* !CONFIG_CAN_ACCEPT_RTR */
 
 		can_sja1000_write_reg(dev, CAN_SJA1000_CMR, CAN_SJA1000_CMR_RRB);
 		sr = can_sja1000_read_reg(dev, CAN_SJA1000_SR);

drivers/can/can_stm32_bxcan.c

@@ -891,8 +891,7 @@ static void can_stm32_set_filter_bank(int filter_id, CAN_FilterRegister_TypeDef
 
 static inline uint32_t can_stm32_filter_to_std_mask(const struct can_filter *filter)
 {
-	uint32_t rtr_mask = (filter->flags & (CAN_FILTER_DATA | CAN_FILTER_RTR)) !=
-		(CAN_FILTER_DATA | CAN_FILTER_RTR) ? 1U : 0U;
+	uint32_t rtr_mask = !IS_ENABLED(CONFIG_CAN_ACCEPT_RTR);
 
 	return  (filter->mask << CAN_STM32_FIRX_STD_ID_POS) |
 		(rtr_mask << CAN_STM32_FIRX_STD_RTR_POS) |
@@ -901,8 +900,7 @@ static inline uint32_t can_stm32_filter_to_std_mask(const struct can_filter *fil
 
 static inline uint32_t can_stm32_filter_to_ext_mask(const struct can_filter *filter)
 {
-	uint32_t rtr_mask = (filter->flags & (CAN_FILTER_DATA | CAN_FILTER_RTR)) !=
-		(CAN_FILTER_DATA | CAN_FILTER_RTR) ? 1U : 0U;
+	uint32_t rtr_mask = !IS_ENABLED(CONFIG_CAN_ACCEPT_RTR);
 
 	return  (filter->mask << CAN_STM32_FIRX_EXT_EXT_ID_POS) |
 		(rtr_mask << CAN_STM32_FIRX_EXT_RTR_POS) |
@@ -911,15 +909,12 @@ static inline uint32_t can_stm32_filter_to_ext_mask(const struct can_filter *fil
 
 static inline uint32_t can_stm32_filter_to_std_id(const struct can_filter *filter)
 {
-	return  (filter->id  << CAN_STM32_FIRX_STD_ID_POS) |
-		(((filter->flags & CAN_FILTER_RTR) != 0) ? (1U << CAN_STM32_FIRX_STD_RTR_POS) : 0U);
+	return  (filter->id  << CAN_STM32_FIRX_STD_ID_POS);
 }
 
 static inline uint32_t can_stm32_filter_to_ext_id(const struct can_filter *filter)
 {
 	return  (filter->id << CAN_STM32_FIRX_EXT_EXT_ID_POS) |
-		(((filter->flags & CAN_FILTER_RTR) != 0) ?
-		(1U << CAN_STM32_FIRX_EXT_RTR_POS) : 0U) |
 		(1U << CAN_STM32_FIRX_EXT_IDE_POS);
 }
 
@@ -1000,7 +995,7 @@ static int can_stm32_add_rx_filter(const struct device *dev, can_rx_callback_t c
 	struct can_stm32_data *data = dev->data;
 	int filter_id;
 
-	if ((filter->flags & ~(CAN_FILTER_IDE | CAN_FILTER_DATA | CAN_FILTER_RTR)) != 0) {
+	if ((filter->flags & ~(CAN_FILTER_IDE)) != 0) {
 		LOG_ERR("unsupported CAN filter flags 0x%02x", filter->flags);
 		return -ENOTSUP;
 	}