Skip to contentSkip to conversation

can: isotp: add module parameter for maximum pdu size

RFC PATCH Superseded Oliver Hartkopp 2 participants 3 messages Mar 11, 2023

Subsystems (2)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Oliver Hartkopp

author maintainer
#0 lore raw

RFC PATCH 1/1 can: isotp: add module parameter for maximum pdu size

With ISO 15765-2:2016 the PDU size is not limited to 2^12 - 1 (4095) bytes but can be represented as a 32 bit unsigned integer value which allows 2^32 - 1 bytes (~4GB). The use-cases like automotive unified diagnostic services (UDS) and flashing of ECUs still use the small static buffers which are provided at socket creation time.

When a use-case requires to transfer PDUs up to 1025 kByte the maximum PDU size can now be extended by setting the module parameter max_pdu_size. The extended size buffers are only allocated on a per-socket/connection base when needed at run-time.

---
 net/can/isotp.c | 59 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 8 deletions(-)
net/can/isotp.c +51 -8
index 9bc344851704..3f052deeaaa0 100644
@@ -83,14 +83,22 @@ MODULE_ALIAS("can-proto-6");
 			 (CAN_EFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG) : \
 			 (CAN_SFF_MASK | CAN_EFF_FLAG | CAN_RTR_FLAG))
 
 /* ISO 15765-2:2016 supports more than 4095 byte per ISO PDU as the FF_DL can
  * take full 32 bit values (4 Gbyte). We would need some good concept to handle
- * this between user space and kernel space. For now increase the static buffer
- * to something about 64 kbyte to be able to test this new functionality.
+ * this between user space and kernel space. For now set the static buffer to
+ * something about 8 kbyte to be able to test this new functionality.
  */
-#define MAX_MSG_LENGTH 66000
+#define DEFAULT_MAX_PDU_SIZE 8300
+
+/* limit the isotp pdu size from the optional module parameter to 1MByte */
+#define MAX_PDU_SIZE (1025 * 1024U)
+
+static unsigned int max_pdu_size __read_mostly = DEFAULT_MAX_PDU_SIZE;
+module_param(max_pdu_size, uint, 0444);
+MODULE_PARM_DESC(max_pdu_size, "maximum isotp pdu size (default "
+		 __stringify(DEFAULT_MAX_PDU_SIZE) ")");
 
 /* N_PCI type values in bits 7-4 of N_PCI bytes */
 #define N_PCI_SF 0x00	/* single frame */
 #define N_PCI_FF 0x10	/* first frame */
 #define N_PCI_CF 0x20	/* consecutive frame */
@@ -121,17 +129,19 @@ enum {
 	ISOTP_WAIT_DATA,
 	ISOTP_SENDING
 };
 
 struct tpcon {
-	unsigned int idx;
+	u8 *buf;
+	unsigned int buflen;
 	unsigned int len;
+	unsigned int idx;
 	u32 state;
 	u8 bs;
 	u8 sn;
 	u8 ll_dl;
-	u8 buf[MAX_MSG_LENGTH + 1];
+	u8 sbuf[DEFAULT_MAX_PDU_SIZE];
 };
 
 struct isotp_sock {
 	struct sock sk;
 	int bound;
@@ -501,11 +511,21 @@ static int isotp_rcv_ff(struct sock *sk, struct canfd_frame *cf, int ae)
 	off = (so->rx.ll_dl > CAN_MAX_DLEN) ? 1 : 0;
 
 	if (so->rx.len + ae + off + ff_pci_sz < so->rx.ll_dl)
 		return 1;
 
-	if (so->rx.len > MAX_MSG_LENGTH) {
+	/* PDU size > default => try max_pdu_size */
+	if (so->rx.len > so->rx.buflen && so->rx.buflen < max_pdu_size) {
+		u8 *newbuf = kmalloc(max_pdu_size, GFP_ATOMIC);
+
+		if (newbuf) {
+			so->rx.buf = newbuf;
+			so->rx.buflen = max_pdu_size;
+		}
+	}
+
+	if (so->rx.len > so->rx.buflen) {
 		/* send FC frame with overflow status */
 		isotp_send_fc(sk, ae, ISOTP_FC_OVFLW);
 		return 1;
 	}
 
@@ -945,11 +965,21 @@ static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 			goto err_out;
 
 		so->tx.state = ISOTP_SENDING;
 	}
 
-	if (!size || size > MAX_MSG_LENGTH) {
+	/* PDU size > default => try max_pdu_size */
+	if (size > so->tx.buflen && so->tx.buflen < max_pdu_size) {
+		u8 *newbuf = kmalloc(max_pdu_size, GFP_KERNEL);
+
+		if (newbuf) {
+			so->tx.buf = newbuf;
+			so->tx.buflen = max_pdu_size;
+		}
+	}
+
+	if (!size || size > so->tx.buflen) {
 		err = -EINVAL;
 		goto err_out_drop;
 	}
 
 	/* take care of a potential SF_DL ESC offset for TX_DL > 8 */
@@ -1193,10 +1223,16 @@ static int isotp_release(struct socket *sock)
 	hrtimer_cancel(&so->rxtimer);
 
 	so->ifindex = 0;
 	so->bound = 0;
 
+	if (so->rx.buf != so->rx.sbuf)
+		kfree(so->rx.buf);
+
+	if (so->tx.buf != so->tx.sbuf)
+		kfree(so->tx.buf);
+
 	sock_orphan(sk);
 	sock->sk = NULL;
 
 	release_sock(sk);
 	sock_put(sk);
@@ -1589,10 +1625,15 @@ static int isotp_init(struct sock *sk)
 	so->tx.ll_dl = so->ll.tx_dl;
 
 	so->rx.state = ISOTP_IDLE;
 	so->tx.state = ISOTP_IDLE;
 
+	so->rx.buf = so->rx.sbuf;
+	so->tx.buf = so->tx.sbuf;
+	so->rx.buflen = DEFAULT_MAX_PDU_SIZE;
+	so->tx.buflen = DEFAULT_MAX_PDU_SIZE;
+
 	hrtimer_init(&so->rxtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
 	so->rxtimer.function = isotp_rx_timer_handler;
 	hrtimer_init(&so->txtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
 	so->txtimer.function = isotp_tx_timer_handler;
 	hrtimer_init(&so->txfrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT);
@@ -1656,11 +1697,13 @@ static struct notifier_block canisotp_notifier = {
 
 static __init int isotp_module_init(void)
 {
 	int err;
 
-	pr_info("can: isotp protocol\n");
+	max_pdu_size = min(max_pdu_size, (unsigned int)MAX_PDU_SIZE);
+
+	pr_info("can: isotp protocol (max_pdu_size %d)\n", max_pdu_size);
 
 	err = can_proto_register(&isotp_can_proto);
 	if (err < 0)
 		pr_err("can: registration of isotp protocol failed %pe\n", ERR_PTR(err));
 	else
Signature · 1 line
2.30.2
#1 lore raw

Hello,

IMO it is the right direction but it is just half the story.

I remember having problems with the txqueuelen even with 4KB PDUs originally and I'm not sure if that has been improved already.

https://gitlab.com/Menschel/socketcan#can-isotp-overflows-when-the-tx-queue-is-too-small

It would be great to have all of those size options in one place and some intelligent IO buffer.

Apart from that the PDU size does not matter for UDS, because UDS splits the data into chunks that are sent via transfer_data service and concatenated on the receiver end again, sort of the protocol's built-in IO buffer.

https://gitlab.com/Menschel/socketcan-uds/-/blob/master/uds/programmer.py#L495

BR,
Patrick
--

Am 11.03.23 um 15:34 schrieb Oliver Hartkopp:

Quoted 13 lines

With ISO 15765-2:2016 the PDU size is not limited to 2^12 - 1 (4095) bytes but can be represented as a 32 bit unsigned integer value which allows 2^32 - 1 bytes (~4GB). The use-cases like automotive unified diagnostic services (UDS) and flashing of ECUs still use the small static buffers which are provided at socket creation time.

When a use-case requires to transfer PDUs up to 1025 kByte the maximum PDU size can now be extended by setting the module parameter max_pdu_size. The extended size buffers are only allocated on a per-socket/connection base when needed at run-time.

author maintainer
#2 lore raw

Hi Patrick,

Quoted 7 lines · Patrick Menschel

IMO it is the right direction but it is just half the story.

I remember having problems with the txqueuelen even with 4KB PDUs originally and I'm not sure if that has been improved already.

https://gitlab.com/Menschel/socketcan#can-isotp-overflows-when-the-tx-queue-is-too-small

It would be great to have all of those size options in one place and some intelligent IO buffer.

I solved that problem since Linux 6.0 \o/

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/net/can/isotp.c?id=4b7fe92c06901f4563af0e36d25223a5ab343782

Quoted 3 lines

Apart from that the PDU size does not matter for UDS, because UDS splits the data into chunks that are sent via transfer_data service and concatenated on the receiver end again, sort of the protocol's built-in IO buffer.

https://gitlab.com/Menschel/socketcan-uds/-/blob/master/uds/programmer.py#L495

Yes. It is not needed for UDS as you stated correctly.

The reason for the increased PDU length was discussed when we created the CAN FD support for ISO TP in 2014. Some guy talked about the flashing of bootloader firmware "in one piece". The firmware was about 64kbyte but then we decided to go for a 32 bit value. IMHO 24 bits would have made it too :-D

Best regards,
Oliver

Quoted 17 lines
Signature · 1 line
Am 11.03.23 um 15:34 schrieb Oliver Hartkopp:
Quoted 13 lines

With ISO 15765-2:2016 the PDU size is not limited to 2^12 - 1 (4095) bytes but can be represented as a 32 bit unsigned integer value which allows 2^32 - 1 bytes (~4GB). The use-cases like automotive unified diagnostic services (UDS) and flashing of ECUs still use the small static buffers which are provided at socket creation time.

When a use-case requires to transfer PDUs up to 1025 kByte the maximum PDU size can now be extended by setting the module parameter max_pdu_size. The extended size buffers are only allocated on a per-socket/connection base when needed at run-time.