[RFC PATCH 0/5] can: support CAN XL

[RFC PATCH 0/5] can: support CAN XL

[Oliver Hartkopp]

The CAN with eXtended data Length (CAN XL) is a new CAN protocol with a
10Mbit/s data transfer with a new physical layer transceiver (for this
data section). CAN XL allows up to 2048 byte of payload and shares the
arbitration principle (11 bit priority) known from Classical CAN and
CAN FD. RTR and 29 bit identifiers are not implemented in CAN XL.

A short introdution to CAN XL can be found here:
https://www.bosch-semiconductors.com/media/ip_modules/pdf_2/can_xl_1/canxl_intro_20210225.pdf

Oliver Hartkopp (5):
  can: canxl: introduce CAN XL data structure
  can: canxl: introduce ETH_P_CANXL ethernet protocol handling
  can: dev: add CAN XL support
  can: vcan: add CAN XL support
  can: raw: add CAN XL support

 drivers/net/can/dev/rx-offload.c |  2 +-
 drivers/net/can/dev/skb.c        | 53 +++++++++++++++++++++++++++-----
 drivers/net/can/vcan.c           | 11 ++++---
 include/linux/can/skb.h          | 17 +++++++++-
 include/uapi/linux/can.h         | 38 +++++++++++++++++++++++
 include/uapi/linux/can/raw.h     |  1 +
 include/uapi/linux/if_ether.h    |  1 +
 net/can/af_can.c                 | 49 ++++++++++++++++++++++++-----
 net/can/raw.c                    | 26 +++++++++++++++-
 9 files changed, 174 insertions(+), 24 deletions(-)

-- 
2.30.2

[RFC PATCH 1/5] can: canxl: introduce CAN XL data structure

[Oliver Hartkopp]

This patch adds defines for data structures and length information for
CAN XL (CAN with eXtended data Length) which can transfer up to 2048
byte insinde a single frame.

Notable changes from CAN FD:

- the 11 bit arbitration field is now named 'priority' instead of 'can_id'
  (there are no 29 bit identifiers nor RTR frames anymore)
- the data length needs a uint16 value to cover up to 2048 byte
  (the length element position is different to struct can[fd]_frame)
- new fields (SDT, AF) and a SEC bit have been introduced
- the virtual CAN interface identifier is not part if the CAN XL frame
  struct as this VCID value is stored in struct skbuff (analog to vlan id)

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 include/uapi/linux/can.h | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
index 90801ada2bbe..9f97a5d06f3b 100644
--- a/include/uapi/linux/can.h
+++ b/include/uapi/linux/can.h
@@ -58,10 +58,11 @@
 
 /* valid bits in CAN ID for frame formats */
 #define CAN_SFF_MASK 0x000007FFU /* standard frame format (SFF) */
 #define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) */
 #define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */
+#define CANXL_PRIO_MASK CAN_SFF_MASK /* 11 bit priority mask */
 
 /*
  * Controller Area Network Identifier structure
  *
  * bit 0-28	: CAN identifier (11/29 bit)
@@ -71,10 +72,11 @@
  */
 typedef __u32 canid_t;
 
 #define CAN_SFF_ID_BITS		11
 #define CAN_EFF_ID_BITS		29
+#define CANXL_PRIO_BITS		CAN_SFF_ID_BITS
 
 /*
  * Controller Area Network Error Message Frame Mask structure
  *
  * bit 0-28	: error class mask (see include/uapi/linux/can/error.h)
@@ -89,10 +91,18 @@ typedef __u32 can_err_mask_t;
 
 /* CAN FD payload length and DLC definitions according to ISO 11898-7 */
 #define CANFD_MAX_DLC 15
 #define CANFD_MAX_DLEN 64
 
+/*
+ * CAN XL payload length and DLC definitions according to ISO 11898-1
+ * CAN XL DLC ranges from 0 .. 2047 => data length from 1 .. 2048 byte
+ */
+#define CANXL_MAX_DLC 2047
+#define CANXL_MAX_DLC_MASK 0x07FF
+#define CANXL_MAX_DLEN 2048
+
 /**
  * struct can_frame - Classical CAN frame structure (aka CAN 2.0B)
  * @can_id:   CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
  * @len:      CAN frame payload length in byte (0 .. 8)
  * @can_dlc:  deprecated name for CAN frame payload length in byte (0 .. 8)
@@ -141,14 +151,20 @@ struct can_frame {
  * When this is done the former differentiation via CAN_MTU / CANFD_MTU gets
  * lost. CANFD_FDF allows programmers to mark CAN FD frames in the case of
  * using struct canfd_frame for mixed CAN / CAN FD content (dual use).
  * N.B. the Kernel APIs do NOT provide mixed CAN / CAN FD content inside of
  * struct canfd_frame therefore the CANFD_FDF flag is disregarded by Linux.
+ * Same applies to the CANXL_XLF bit.
+ *
+ * For CAN XL the SEC bit has been added to the flags field which shares the
+ * same position in struct can[fd|xl]_frame.
  */
 #define CANFD_BRS 0x01 /* bit rate switch (second bitrate for payload data) */
 #define CANFD_ESI 0x02 /* error state indicator of the transmitting node */
 #define CANFD_FDF 0x04 /* mark CAN FD for dual use of struct canfd_frame */
+#define CANXL_XLF 0x08 /* mark CAN XL for dual use of struct canfd_frame */
+#define CANXL_SEC 0x10 /* Simple Extended Content (security/segmentation) */
 
 /**
  * struct canfd_frame - CAN flexible data rate frame structure
  * @can_id: CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
  * @len:    frame payload length in byte (0 .. CANFD_MAX_DLEN)
@@ -164,12 +180,34 @@ struct canfd_frame {
 	__u8    __res0;  /* reserved / padding */
 	__u8    __res1;  /* reserved / padding */
 	__u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
 };
 
+/**
+ * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
+ * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags
+ * @sdt:   SDU (service data unit) type
+ * @flags: additional flags for CAN XL
+ * @len:   frame payload length in byte (1 .. CANXL_MAX_DLEN)
+ * @af:    acceptance field
+ * @data:  CAN XL frame payload (up to CANXL_MAX_DLEN byte)
+ *
+ * @prio shares the same position as @can_id from struct canfd_frame.
+ * Same applies to the relative position and length of @flags.
+ */
+struct canxl_frame {
+	canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
+	__u8    sdt;   /* SDU (service data unit) type */
+	__u8    flags; /* additional flags for CAN XL */
+	__u16   len;   /* frame payload length in byte */
+	__u32   af;    /* acceptance field */
+	__u8    data[CANXL_MAX_DLEN];
+};
+
 #define CAN_MTU		(sizeof(struct can_frame))
 #define CANFD_MTU	(sizeof(struct canfd_frame))
+#define CANXL_MTU	(sizeof(struct canxl_frame))
 
 /* particular protocols of the protocol family PF_CAN */
 #define CAN_RAW		1 /* RAW sockets */
 #define CAN_BCM		2 /* Broadcast Manager */
 #define CAN_TP16	3 /* VAG Transport Protocol v1.6 */
-- 
2.30.2

[RFC PATCH 2/5] can: canxl: introduce ETH_P_CANXL ethernet protocol handling

[Oliver Hartkopp]

Enable the PF_CAN infrastructure to handle CAN XL frames. A new ethernet
protocol type ETH_P_CANXL is defined to tag skbuffs containing the CAN XL
frame data structure.

As the length information is now a uint16 value for CAN XL a new helper
function can_get_data_len() is introduced to retrieve the data length
from all types of CAN frames.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 include/linux/can/skb.h       | 14 ++++++++++
 include/uapi/linux/if_ether.h |  1 +
 net/can/af_can.c              | 49 +++++++++++++++++++++++++++++------
 3 files changed, 56 insertions(+), 8 deletions(-)

diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index 182749e858b3..d043bc4afd6d 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -101,6 +101,20 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
 {
 	/* the CAN specific type of skb is identified by its data length */
 	return skb->len == CANFD_MTU;
 }
 
+/* get data length inside of CAN frame for all frame types */
+static inline unsigned int can_get_data_len(struct sk_buff *skb)
+{
+	if(skb->len == CANXL_MTU) {
+		const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
+
+		return cfx->len;
+	} else {
+		const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+
+		return cfd->len;
+	}
+}
+
 #endif /* !_CAN_SKB_H */
diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
index d370165bc621..69e0457eb200 100644
--- a/include/uapi/linux/if_ether.h
+++ b/include/uapi/linux/if_ether.h
@@ -136,10 +136,11 @@
 #define ETH_P_WAN_PPP   0x0007          /* Dummy type for WAN PPP frames*/
 #define ETH_P_PPP_MP    0x0008          /* Dummy type for PPP MP frames */
 #define ETH_P_LOCALTALK 0x0009		/* Localtalk pseudo type 	*/
 #define ETH_P_CAN	0x000C		/* CAN: Controller Area Network */
 #define ETH_P_CANFD	0x000D		/* CANFD: CAN flexible data rate*/
+#define ETH_P_CANXL	0x000E		/* CANXL: eXtended frame Length */
 #define ETH_P_PPPTALK	0x0010		/* Dummy type for Atalk over PPP*/
 #define ETH_P_TR_802_2	0x0011		/* 802.2 frames 		*/
 #define ETH_P_MOBITEX	0x0015		/* Mobitex (kaz@cafe.net)	*/
 #define ETH_P_CONTROL	0x0016		/* Card specific control frames */
 #define ETH_P_IRDA	0x0017		/* Linux-IrDA			*/
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 1fb49d51b25d..2c9f48aa5f1f 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -197,31 +197,32 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
  *  -EINVAL when the skb->data does not contain a valid CAN frame
  */
 int can_send(struct sk_buff *skb, int loop)
 {
 	struct sk_buff *newskb = NULL;
-	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+	unsigned int len = can_get_data_len(skb);
 	struct can_pkg_stats *pkg_stats = dev_net(skb->dev)->can.pkg_stats;
 	int err = -EINVAL;
 
 	if (skb->len == CAN_MTU) {
 		skb->protocol = htons(ETH_P_CAN);
-		if (unlikely(cfd->len > CAN_MAX_DLEN))
+		if (unlikely(len > CAN_MAX_DLEN))
 			goto inval_skb;
 	} else if (skb->len == CANFD_MTU) {
 		skb->protocol = htons(ETH_P_CANFD);
-		if (unlikely(cfd->len > CANFD_MAX_DLEN))
+		if (unlikely(len > CANFD_MAX_DLEN))
+			goto inval_skb;
+	} else if (skb->len == CANXL_MTU) {
+		skb->protocol = htons(ETH_P_CANXL);
+		if (unlikely(len > CANXL_MAX_DLEN || len == 0))
 			goto inval_skb;
 	} else {
 		goto inval_skb;
 	}
 
-	/* Make sure the CAN frame can pass the selected CAN netdevice.
-	 * As structs can_frame and canfd_frame are similar, we can provide
-	 * CAN FD frames to legacy CAN drivers as long as the length is <= 8
-	 */
-	if (unlikely(skb->len > skb->dev->mtu && cfd->len > CAN_MAX_DLEN)) {
+	/* Make sure the CAN frame can pass the selected CAN netdevice */
+	if (unlikely(skb->len > skb->dev->mtu)) {
 		err = -EMSGSIZE;
 		goto inval_skb;
 	}
 
 	if (unlikely(skb->dev->type != ARPHRD_CAN)) {
@@ -725,10 +726,36 @@ static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
 free_skb:
 	kfree_skb(skb);
 	return NET_RX_DROP;
 }
 
+static int canxl_rcv(struct sk_buff *skb, struct net_device *dev,
+		     struct packet_type *pt, struct net_device *orig_dev)
+{
+	struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
+
+	if (unlikely(dev->type != ARPHRD_CAN || skb->len != CANXL_MTU)) {
+		pr_warn_once("PF_CAN: dropped non conform CAN XL skbuff: dev type %d, len %d\n",
+			     dev->type, skb->len);
+		goto free_skb;
+	}
+
+	/* This check is made separately since cfx->len would be uninitialized if skb->len = 0. */
+	if (unlikely(cfx->len > CANXL_MAX_DLEN || cfx->len == 0)) {
+		pr_warn_once("PF_CAN: dropped non conform CAN XL skbuff: dev type %d, len %d, datalen %d\n",
+			     dev->type, skb->len, cfx->len);
+		goto free_skb;
+	}
+
+	can_receive(skb, dev);
+	return NET_RX_SUCCESS;
+
+free_skb:
+	kfree_skb(skb);
+	return NET_RX_DROP;
+}
+
 /* af_can protocol functions */
 
 /**
  * can_proto_register - register CAN transport protocol
  * @cp: pointer to CAN protocol structure
@@ -849,10 +876,15 @@ static struct packet_type can_packet __read_mostly = {
 static struct packet_type canfd_packet __read_mostly = {
 	.type = cpu_to_be16(ETH_P_CANFD),
 	.func = canfd_rcv,
 };
 
+static struct packet_type canxl_packet __read_mostly = {
+	.type = cpu_to_be16(ETH_P_CANXL),
+	.func = canxl_rcv,
+};
+
 static const struct net_proto_family can_family_ops = {
 	.family = PF_CAN,
 	.create = can_create,
 	.owner  = THIS_MODULE,
 };
@@ -888,10 +920,11 @@ static __init int can_init(void)
 	if (err)
 		goto out_sock;
 
 	dev_add_pack(&can_packet);
 	dev_add_pack(&canfd_packet);
+	dev_add_pack(&canxl_packet);
 
 	return 0;
 
 out_sock:
 	unregister_pernet_subsys(&can_pernet_ops);
-- 
2.30.2

[RFC PATCH 3/5] can: dev: add CAN XL support

[Oliver Hartkopp]

Extend the CAN device driver infrastructure to handle CAN XL frames.
This especially addresses the increased data length which is extended
to uint16 for CAN XL.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev/rx-offload.c |  2 +-
 drivers/net/can/dev/skb.c        | 53 +++++++++++++++++++++++++++-----
 include/linux/can/skb.h          |  3 +-
 3 files changed, 48 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
index a32a01c172d4..8505e547e922 100644
--- a/drivers/net/can/dev/rx-offload.c
+++ b/drivers/net/can/dev/rx-offload.c
@@ -245,11 +245,11 @@ unsigned int can_rx_offload_get_echo_skb(struct can_rx_offload *offload,
 					 unsigned int *frame_len_ptr)
 {
 	struct net_device *dev = offload->dev;
 	struct net_device_stats *stats = &dev->stats;
 	struct sk_buff *skb;
-	u8 len;
+	unsigned int len;
 	int err;
 
 	skb = __can_get_echo_skb(dev, idx, &len, frame_len_ptr);
 	if (!skb)
 		return 0;
diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 8bb62dd864c8..a849f503dcff 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -53,11 +53,12 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 	BUG_ON(idx >= priv->echo_skb_max);
 
 	/* check flag whether this packet has to be looped back */
 	if (!(dev->flags & IFF_ECHO) ||
 	    (skb->protocol != htons(ETH_P_CAN) &&
-	     skb->protocol != htons(ETH_P_CANFD))) {
+	     skb->protocol != htons(ETH_P_CANFD) &&
+	     skb->protocol != htons(ETH_P_CANXL))) {
 		kfree_skb(skb);
 		return 0;
 	}
 
 	if (!priv->echo_skb[idx]) {
@@ -86,12 +87,12 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(can_put_echo_skb);
 
 struct sk_buff *
-__can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
-		   unsigned int *frame_len_ptr)
+__can_get_echo_skb(struct net_device *dev, unsigned int idx,
+		   unsigned int *len_ptr, unsigned int *frame_len_ptr)
 {
 	struct can_priv *priv = netdev_priv(dev);
 
 	if (idx >= priv->echo_skb_max) {
 		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
@@ -104,16 +105,17 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
 		 * length is supported on both CAN and CANFD frames.
 		 */
 		struct sk_buff *skb = priv->echo_skb[idx];
 		struct can_skb_priv *can_skb_priv = can_skb_prv(skb);
 		struct canfd_frame *cf = (struct canfd_frame *)skb->data;
+		unsigned int len = can_get_data_len(skb);
 
 		/* get the real payload length for netdev statistics */
 		if (cf->can_id & CAN_RTR_FLAG)
 			*len_ptr = 0;
 		else
-			*len_ptr = cf->len;
+			*len_ptr = len;
 
 		if (frame_len_ptr)
 			*frame_len_ptr = can_skb_priv->frame_len;
 
 		priv->echo_skb[idx] = NULL;
@@ -139,11 +141,11 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
  */
 unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx,
 			      unsigned int *frame_len_ptr)
 {
 	struct sk_buff *skb;
-	u8 len;
+	unsigned int len;
 
 	skb = __can_get_echo_skb(dev, idx, &len, frame_len_ptr);
 	if (!skb)
 		return 0;
 
@@ -244,10 +246,41 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
 
 	return skb;
 }
 EXPORT_SYMBOL_GPL(alloc_canfd_skb);
 
+struct sk_buff *alloc_canxl_skb(struct net_device *dev,
+				struct canxl_frame **cfx)
+{
+	struct sk_buff *skb;
+
+	skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
+			       sizeof(struct canxl_frame));
+	if (unlikely(!skb)) {
+		*cfx = NULL;
+
+		return NULL;
+	}
+
+	skb->protocol = htons(ETH_P_CANXL);
+	skb->pkt_type = PACKET_BROADCAST;
+	skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+	skb_reset_mac_header(skb);
+	skb_reset_network_header(skb);
+	skb_reset_transport_header(skb);
+
+	can_skb_reserve(skb);
+	can_skb_prv(skb)->ifindex = dev->ifindex;
+	can_skb_prv(skb)->skbcnt = 0;
+
+	*cfx = skb_put_zero(skb, sizeof(struct canxl_frame));
+
+	return skb;
+}
+EXPORT_SYMBOL_GPL(alloc_canxl_skb);
+
 struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf)
 {
 	struct sk_buff *skb;
 
 	skb = alloc_can_skb(dev, cf);
@@ -291,20 +324,24 @@ static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
 }
 
 /* Drop a given socketbuffer if it does not contain a valid CAN frame. */
 bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
 {
-	const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
 	struct can_priv *priv = netdev_priv(dev);
+	unsigned int len = can_get_data_len(skb);
 
 	if (skb->protocol == htons(ETH_P_CAN)) {
 		if (unlikely(skb->len != CAN_MTU ||
-			     cfd->len > CAN_MAX_DLEN))
+			     len > CAN_MAX_DLEN))
 			goto inval_skb;
 	} else if (skb->protocol == htons(ETH_P_CANFD)) {
 		if (unlikely(skb->len != CANFD_MTU ||
-			     cfd->len > CANFD_MAX_DLEN))
+			     len > CANFD_MAX_DLEN))
+			goto inval_skb;
+	} else if (skb->protocol == htons(ETH_P_CANXL)) {
+		if (unlikely(skb->len != CANXL_MTU ||
+			     len > CANXL_MAX_DLEN || len == 0))
 			goto inval_skb;
 	} else {
 		goto inval_skb;
 	}
 
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index d043bc4afd6d..72ee887a783a 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -18,11 +18,12 @@
 
 void can_flush_echo_skb(struct net_device *dev);
 int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 		     unsigned int idx, unsigned int frame_len);
 struct sk_buff *__can_get_echo_skb(struct net_device *dev, unsigned int idx,
-				   u8 *len_ptr, unsigned int *frame_len_ptr);
+				   unsigned int *len_ptr,
+				   unsigned int *frame_len_ptr);
 unsigned int __must_check can_get_echo_skb(struct net_device *dev,
 					   unsigned int idx,
 					   unsigned int *frame_len_ptr);
 void can_free_echo_skb(struct net_device *dev, unsigned int idx,
 		       unsigned int *frame_len_ptr);
-- 
2.30.2

[RFC PATCH 4/5] can: vcan: add CAN XL support

[Oliver Hartkopp]

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/vcan.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
index a15619d883ec..a8de50fbfe3d 100644
--- a/drivers/net/can/vcan.c
+++ b/drivers/net/can/vcan.c
@@ -68,15 +68,15 @@ static bool echo; /* echo testing. Default: 0 (Off) */
 module_param(echo, bool, 0444);
 MODULE_PARM_DESC(echo, "Echo sent frames (for testing). Default: 0 (Off)");
 
 static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
 {
-	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+	unsigned int len = can_get_data_len(skb);
 	struct net_device_stats *stats = &dev->stats;
 
 	stats->rx_packets++;
-	stats->rx_bytes += cfd->len;
+	stats->rx_bytes += len;
 
 	skb->pkt_type  = PACKET_BROADCAST;
 	skb->dev       = dev;
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
 
@@ -85,16 +85,17 @@ static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
 
 static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
 {
 	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
 	struct net_device_stats *stats = &dev->stats;
-	int loop, len;
+	unsigned int len = can_get_data_len(skb);
+	int loop;
 
 	if (can_dropped_invalid_skb(dev, skb))
 		return NETDEV_TX_OK;
 
-	len = cfd->can_id & CAN_RTR_FLAG ? 0 : cfd->len;
+	len = cfd->can_id & CAN_RTR_FLAG ? 0 : len;
 	stats->tx_packets++;
 	stats->tx_bytes += len;
 
 	/* set flag whether this packet has to be looped back */
 	loop = skb->pkt_type == PACKET_LOOPBACK;
@@ -132,11 +133,11 @@ static int vcan_change_mtu(struct net_device *dev, int new_mtu)
 {
 	/* Do not allow changing the MTU while running */
 	if (dev->flags & IFF_UP)
 		return -EBUSY;
 
-	if (new_mtu != CAN_MTU && new_mtu != CANFD_MTU)
+	if (new_mtu != CAN_MTU && new_mtu != CANFD_MTU && new_mtu != CANXL_MTU)
 		return -EINVAL;
 
 	dev->mtu = new_mtu;
 	return 0;
 }
-- 
2.30.2

[RFC PATCH 5/5] can: raw: add CAN XL support

[Oliver Hartkopp]

Enable CAN_RAW sockets to read and write CAN XL frames analogue to the
CAN FD extension (new CAN_RAW_XL_FRAMES sockopt).

A CAN XL network interface is capable to handle Classical CAN, CAN FD and
CAN XL frames. When CAN_RAW_XL_FRAMES is enabled, the CAN_RAW socket checks
whether the addressed CAN network interface is capable to handle the
provided CAN frame.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 include/uapi/linux/can/raw.h |  1 +
 net/can/raw.c                | 26 +++++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/can/raw.h b/include/uapi/linux/can/raw.h
index 3386aa81fdf2..ff12f525c37c 100644
--- a/include/uapi/linux/can/raw.h
+++ b/include/uapi/linux/can/raw.h
@@ -60,8 +60,9 @@ enum {
 	CAN_RAW_ERR_FILTER,	/* set filter for error frames       */
 	CAN_RAW_LOOPBACK,	/* local loopback (default:on)       */
 	CAN_RAW_RECV_OWN_MSGS,	/* receive my own msgs (default:off) */
 	CAN_RAW_FD_FRAMES,	/* allow CAN FD frames (default:off) */
 	CAN_RAW_JOIN_FILTERS,	/* all filters must match to trigger */
+	CAN_RAW_XL_FRAMES,	/* allow CAN XL frames (default:off) */
 };
 
 #endif /* !_UAPI_CAN_RAW_H */
diff --git a/net/can/raw.c b/net/can/raw.c
index d1bd9cc51ebe..c036de63fcfa 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -85,10 +85,11 @@ struct raw_sock {
 	int ifindex;
 	struct list_head notifier;
 	int loopback;
 	int recv_own_msgs;
 	int fd_frames;
+	int xl_frames;
 	int join_filters;
 	int count;                 /* number of active filters */
 	struct can_filter dfilter; /* default/single filter */
 	struct can_filter *filter; /* pointer to filter(s) */
 	can_err_mask_t err_mask;
@@ -344,10 +345,11 @@ static int raw_init(struct sock *sk)
 
 	/* set default loopback behaviour */
 	ro->loopback         = 1;
 	ro->recv_own_msgs    = 0;
 	ro->fd_frames        = 0;
+	ro->xl_frames        = 0;
 	ro->join_filters     = 0;
 
 	/* alloc_percpu provides zero'ed memory */
 	ro->uniq = alloc_percpu(struct uniqframe);
 	if (unlikely(!ro->uniq))
@@ -667,10 +669,19 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 		if (copy_from_sockptr(&ro->fd_frames, optval, optlen))
 			return -EFAULT;
 
 		break;
 
+	case CAN_RAW_XL_FRAMES:
+		if (optlen != sizeof(ro->xl_frames))
+			return -EINVAL;
+
+		if (copy_from_sockptr(&ro->xl_frames, optval, optlen))
+			return -EFAULT;
+
+		break;
+
 	case CAN_RAW_JOIN_FILTERS:
 		if (optlen != sizeof(ro->join_filters))
 			return -EINVAL;
 
 		if (copy_from_sockptr(&ro->join_filters, optval, optlen))
@@ -749,10 +760,16 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
 		if (len > sizeof(int))
 			len = sizeof(int);
 		val = &ro->fd_frames;
 		break;
 
+	case CAN_RAW_XL_FRAMES:
+		if (len > sizeof(int))
+			len = sizeof(int);
+		val = &ro->xl_frames;
+		break;
+
 	case CAN_RAW_JOIN_FILTERS:
 		if (len > sizeof(int))
 			len = sizeof(int);
 		val = &ro->join_filters;
 		break;
@@ -795,14 +812,21 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	dev = dev_get_by_index(sock_net(sk), ifindex);
 	if (!dev)
 		return -ENXIO;
 
 	err = -EINVAL;
-	if (ro->fd_frames && dev->mtu == CANFD_MTU) {
+	if (ro->xl_frames && dev->mtu == CANXL_MTU) {
+		/* CAN XL, CAN FD and Classical CAN */
+		if (unlikely(size != CANXL_MTU && size != CANFD_MTU &&
+			     size != CAN_MTU))
+			goto put_dev;
+	} else if (ro->fd_frames && dev->mtu == CANFD_MTU) {
+		/* CAN FD and Classical CAN */
 		if (unlikely(size != CANFD_MTU && size != CAN_MTU))
 			goto put_dev;
 	} else {
+		/* Classical CAN */
 		if (unlikely(size != CAN_MTU))
 			goto put_dev;
 	}
 
 	skb = sock_alloc_send_skb(sk, size + sizeof(struct can_skb_priv),
-- 
2.30.2

Re: [RFC PATCH 2/5] can: canxl: introduce ETH_P_CANXL ethernet protocol handling

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1569 bytes --]

On 11.07.2022 20:34:23, Oliver Hartkopp wrote:
> Enable the PF_CAN infrastructure to handle CAN XL frames. A new ethernet
> protocol type ETH_P_CANXL is defined to tag skbuffs containing the CAN XL
> frame data structure.
> 
> As the length information is now a uint16 value for CAN XL a new helper
> function can_get_data_len() is introduced to retrieve the data length
> from all types of CAN frames.
> 
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>  include/linux/can/skb.h       | 14 ++++++++++
>  include/uapi/linux/if_ether.h |  1 +
>  net/can/af_can.c              | 49 +++++++++++++++++++++++++++++------
>  3 files changed, 56 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> index 182749e858b3..d043bc4afd6d 100644
> --- a/include/linux/can/skb.h
> +++ b/include/linux/can/skb.h
> @@ -101,6 +101,20 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
>  {
>  	/* the CAN specific type of skb is identified by its data length */
>  	return skb->len == CANFD_MTU;
>  }
>  
> +/* get data length inside of CAN frame for all frame types */
> +static inline unsigned int can_get_data_len(struct sk_buff *skb)
> +{
> +	if(skb->len == CANXL_MTU) {
         ^^^ add space

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 2/5] can: canxl: introduce ETH_P_CANXL ethernet protocol handling

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 5515 bytes --]

On 11.07.2022 20:34:23, Oliver Hartkopp wrote:
> Enable the PF_CAN infrastructure to handle CAN XL frames. A new ethernet
> protocol type ETH_P_CANXL is defined to tag skbuffs containing the CAN XL
> frame data structure.
> 
> As the length information is now a uint16 value for CAN XL a new helper
> function can_get_data_len() is introduced to retrieve the data length
> from all types of CAN frames.
> 
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>  include/linux/can/skb.h       | 14 ++++++++++
>  include/uapi/linux/if_ether.h |  1 +
>  net/can/af_can.c              | 49 +++++++++++++++++++++++++++++------
>  3 files changed, 56 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> index 182749e858b3..d043bc4afd6d 100644
> --- a/include/linux/can/skb.h
> +++ b/include/linux/can/skb.h
> @@ -101,6 +101,20 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
>  {
>  	/* the CAN specific type of skb is identified by its data length */
>  	return skb->len == CANFD_MTU;
>  }
>  
> +/* get data length inside of CAN frame for all frame types */
> +static inline unsigned int can_get_data_len(struct sk_buff *skb)
> +{
> +	if(skb->len == CANXL_MTU) {
> +		const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> +
> +		return cfx->len;
> +	} else {
> +		const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> +
> +		return cfd->len;
> +	}
> +}
> +
>  #endif /* !_CAN_SKB_H */
> diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
> index d370165bc621..69e0457eb200 100644
> --- a/include/uapi/linux/if_ether.h
> +++ b/include/uapi/linux/if_ether.h
> @@ -136,10 +136,11 @@
>  #define ETH_P_WAN_PPP   0x0007          /* Dummy type for WAN PPP frames*/
>  #define ETH_P_PPP_MP    0x0008          /* Dummy type for PPP MP frames */
>  #define ETH_P_LOCALTALK 0x0009		/* Localtalk pseudo type 	*/
>  #define ETH_P_CAN	0x000C		/* CAN: Controller Area Network */
>  #define ETH_P_CANFD	0x000D		/* CANFD: CAN flexible data rate*/
> +#define ETH_P_CANXL	0x000E		/* CANXL: eXtended frame Length */
>  #define ETH_P_PPPTALK	0x0010		/* Dummy type for Atalk over PPP*/
>  #define ETH_P_TR_802_2	0x0011		/* 802.2 frames 		*/
>  #define ETH_P_MOBITEX	0x0015		/* Mobitex (kaz@cafe.net)	*/
>  #define ETH_P_CONTROL	0x0016		/* Card specific control frames */
>  #define ETH_P_IRDA	0x0017		/* Linux-IrDA			*/

This file doesn't change that often I suppose. Or does it make sense to
send this change mainline as soon as possible?

> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 1fb49d51b25d..2c9f48aa5f1f 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -197,31 +197,32 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
>   *  -EINVAL when the skb->data does not contain a valid CAN frame
>   */
>  int can_send(struct sk_buff *skb, int loop)
>  {
>  	struct sk_buff *newskb = NULL;
> -	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> +	unsigned int len = can_get_data_len(skb);
>  	struct can_pkg_stats *pkg_stats = dev_net(skb->dev)->can.pkg_stats;
>  	int err = -EINVAL;
>  
>  	if (skb->len == CAN_MTU) {
>  		skb->protocol = htons(ETH_P_CAN);
> -		if (unlikely(cfd->len > CAN_MAX_DLEN))
> +		if (unlikely(len > CAN_MAX_DLEN))
>  			goto inval_skb;
>  	} else if (skb->len == CANFD_MTU) {
>  		skb->protocol = htons(ETH_P_CANFD);
> -		if (unlikely(cfd->len > CANFD_MAX_DLEN))
> +		if (unlikely(len > CANFD_MAX_DLEN))
> +			goto inval_skb;
> +	} else if (skb->len == CANXL_MTU) {
> +		skb->protocol = htons(ETH_P_CANXL);
> +		if (unlikely(len > CANXL_MAX_DLEN || len == 0))

Do we need a helper for the > CANXL_MAX_DLEN || == 0 check?

>  			goto inval_skb;
>  	} else {
>  		goto inval_skb;
>  	}
>  
> -	/* Make sure the CAN frame can pass the selected CAN netdevice.
> -	 * As structs can_frame and canfd_frame are similar, we can provide
> -	 * CAN FD frames to legacy CAN drivers as long as the length is <= 8
> -	 */
> -	if (unlikely(skb->len > skb->dev->mtu && cfd->len > CAN_MAX_DLEN)) {
> +	/* Make sure the CAN frame can pass the selected CAN netdevice */
> +	if (unlikely(skb->len > skb->dev->mtu)) {
>  		err = -EMSGSIZE;
>  		goto inval_skb;
>  	}
>  
>  	if (unlikely(skb->dev->type != ARPHRD_CAN)) {
> @@ -725,10 +726,36 @@ static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
>  free_skb:
>  	kfree_skb(skb);
>  	return NET_RX_DROP;
>  }
>  
> +static int canxl_rcv(struct sk_buff *skb, struct net_device *dev,
> +		     struct packet_type *pt, struct net_device *orig_dev)
> +{
> +	struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> +
> +	if (unlikely(dev->type != ARPHRD_CAN || skb->len != CANXL_MTU)) {
> +		pr_warn_once("PF_CAN: dropped non conform CAN XL skbuff: dev type %d, len %d\n",
> +			     dev->type, skb->len);
> +		goto free_skb;
> +	}
> +
> +	/* This check is made separately since cfx->len would be uninitialized if skb->len = 0. */
> +	if (unlikely(cfx->len > CANXL_MAX_DLEN || cfx->len == 0)) {
                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 3/5] can: dev: add CAN XL support

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4111 bytes --]

On 11.07.2022 20:34:24, Oliver Hartkopp wrote:
> Extend the CAN device driver infrastructure to handle CAN XL frames.
> This especially addresses the increased data length which is extended
> to uint16 for CAN XL.
> 
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>  drivers/net/can/dev/rx-offload.c |  2 +-
>  drivers/net/can/dev/skb.c        | 53 +++++++++++++++++++++++++++-----
>  include/linux/can/skb.h          |  3 +-
>  3 files changed, 48 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
> index a32a01c172d4..8505e547e922 100644
> --- a/drivers/net/can/dev/rx-offload.c
> +++ b/drivers/net/can/dev/rx-offload.c
> @@ -245,11 +245,11 @@ unsigned int can_rx_offload_get_echo_skb(struct can_rx_offload *offload,
>  					 unsigned int *frame_len_ptr)
>  {
>  	struct net_device *dev = offload->dev;
>  	struct net_device_stats *stats = &dev->stats;
>  	struct sk_buff *skb;
> -	u8 len;
> +	unsigned int len;
>  	int err;
>  
>  	skb = __can_get_echo_skb(dev, idx, &len, frame_len_ptr);
>  	if (!skb)
>  		return 0;
> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
> index 8bb62dd864c8..a849f503dcff 100644
> --- a/drivers/net/can/dev/skb.c
> +++ b/drivers/net/can/dev/skb.c
> @@ -53,11 +53,12 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>  	BUG_ON(idx >= priv->echo_skb_max);
>  
>  	/* check flag whether this packet has to be looped back */
>  	if (!(dev->flags & IFF_ECHO) ||
>  	    (skb->protocol != htons(ETH_P_CAN) &&
> -	     skb->protocol != htons(ETH_P_CANFD))) {
> +	     skb->protocol != htons(ETH_P_CANFD) &&
> +	     skb->protocol != htons(ETH_P_CANXL))) {
>  		kfree_skb(skb);
>  		return 0;
>  	}
>  
>  	if (!priv->echo_skb[idx]) {
> @@ -86,12 +87,12 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(can_put_echo_skb);
>  
>  struct sk_buff *
> -__can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
> -		   unsigned int *frame_len_ptr)
> +__can_get_echo_skb(struct net_device *dev, unsigned int idx,
> +		   unsigned int *len_ptr, unsigned int *frame_len_ptr)
>  {
>  	struct can_priv *priv = netdev_priv(dev);
>  
>  	if (idx >= priv->echo_skb_max) {
>  		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
> @@ -104,16 +105,17 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
>  		 * length is supported on both CAN and CANFD frames.
>  		 */
>  		struct sk_buff *skb = priv->echo_skb[idx];
>  		struct can_skb_priv *can_skb_priv = can_skb_prv(skb);
>  		struct canfd_frame *cf = (struct canfd_frame *)skb->data;
> +		unsigned int len = can_get_data_len(skb);
>  
>  		/* get the real payload length for netdev statistics */
>  		if (cf->can_id & CAN_RTR_FLAG)
>  			*len_ptr = 0;
>  		else
> -			*len_ptr = cf->len;
> +			*len_ptr = len;
>  
>  		if (frame_len_ptr)
>  			*frame_len_ptr = can_skb_priv->frame_len;
>  
>  		priv->echo_skb[idx] = NULL;
> @@ -139,11 +141,11 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
>   */
>  unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx,
>  			      unsigned int *frame_len_ptr)
>  {
>  	struct sk_buff *skb;
> -	u8 len;
> +	unsigned int len;
>  
>  	skb = __can_get_echo_skb(dev, idx, &len, frame_len_ptr);
>  	if (!skb)
>  		return 0;
>  
> @@ -244,10 +246,41 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
>  
>  	return skb;
>  }
>  EXPORT_SYMBOL_GPL(alloc_canfd_skb);
>  
> +struct sk_buff *alloc_canxl_skb(struct net_device *dev,
> +				struct canxl_frame **cfx)

The prototype has to be added to a header file

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 1/5] can: canxl: introduce CAN XL data structure

[Vincent Mailhol]

On Tue. 12 Jul. 2022 at 03:44, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>
> This patch adds defines for data structures and length information for
> CAN XL (CAN with eXtended data Length) which can transfer up to 2048
> byte insinde a single frame.
>
> Notable changes from CAN FD:
>
> - the 11 bit arbitration field is now named 'priority' instead of 'can_id'
>   (there are no 29 bit identifiers nor RTR frames anymore)
> - the data length needs a uint16 value to cover up to 2048 byte
>   (the length element position is different to struct can[fd]_frame)
> - new fields (SDT, AF) and a SEC bit have been introduced
> - the virtual CAN interface identifier is not part if the CAN XL frame
>   struct as this VCID value is stored in struct skbuff (analog to vlan id)
>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>  include/uapi/linux/can.h | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
> index 90801ada2bbe..9f97a5d06f3b 100644
> --- a/include/uapi/linux/can.h
> +++ b/include/uapi/linux/can.h
> @@ -58,10 +58,11 @@
>
>  /* valid bits in CAN ID for frame formats */
>  #define CAN_SFF_MASK 0x000007FFU /* standard frame format (SFF) */
>  #define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) */
>  #define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */
> +#define CANXL_PRIO_MASK CAN_SFF_MASK /* 11 bit priority mask */
>
>  /*
>   * Controller Area Network Identifier structure
>   *
>   * bit 0-28    : CAN identifier (11/29 bit)
> @@ -71,10 +72,11 @@
>   */
>  typedef __u32 canid_t;
>
>  #define CAN_SFF_ID_BITS                11
>  #define CAN_EFF_ID_BITS                29
> +#define CANXL_PRIO_BITS                CAN_SFF_ID_BITS
>
>  /*
>   * Controller Area Network Error Message Frame Mask structure
>   *
>   * bit 0-28    : error class mask (see include/uapi/linux/can/error.h)
> @@ -89,10 +91,18 @@ typedef __u32 can_err_mask_t;
>
>  /* CAN FD payload length and DLC definitions according to ISO 11898-7 */
>  #define CANFD_MAX_DLC 15
>  #define CANFD_MAX_DLEN 64
>
> +/*
> + * CAN XL payload length and DLC definitions according to ISO 11898-1
> + * CAN XL DLC ranges from 0 .. 2047 => data length from 1 .. 2048 byte
> + */
> +#define CANXL_MAX_DLC 2047
> +#define CANXL_MAX_DLC_MASK 0x07FF
> +#define CANXL_MAX_DLEN 2048
> +
>  /**
>   * struct can_frame - Classical CAN frame structure (aka CAN 2.0B)
>   * @can_id:   CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
>   * @len:      CAN frame payload length in byte (0 .. 8)
>   * @can_dlc:  deprecated name for CAN frame payload length in byte (0 .. 8)
> @@ -141,14 +151,20 @@ struct can_frame {
>   * When this is done the former differentiation via CAN_MTU / CANFD_MTU gets
>   * lost. CANFD_FDF allows programmers to mark CAN FD frames in the case of
>   * using struct canfd_frame for mixed CAN / CAN FD content (dual use).
>   * N.B. the Kernel APIs do NOT provide mixed CAN / CAN FD content inside of
>   * struct canfd_frame therefore the CANFD_FDF flag is disregarded by Linux.
> + * Same applies to the CANXL_XLF bit.
> + *
> + * For CAN XL the SEC bit has been added to the flags field which shares the
> + * same position in struct can[fd|xl]_frame.
>   */
>  #define CANFD_BRS 0x01 /* bit rate switch (second bitrate for payload data) */
>  #define CANFD_ESI 0x02 /* error state indicator of the transmitting node */
>  #define CANFD_FDF 0x04 /* mark CAN FD for dual use of struct canfd_frame */
> +#define CANXL_XLF 0x08 /* mark CAN XL for dual use of struct canfd_frame */
> +#define CANXL_SEC 0x10 /* Simple Extended Content (security/segmentation) */
>
>  /**
>   * struct canfd_frame - CAN flexible data rate frame structure
>   * @can_id: CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
>   * @len:    frame payload length in byte (0 .. CANFD_MAX_DLEN)
> @@ -164,12 +180,34 @@ struct canfd_frame {
>         __u8    __res0;  /* reserved / padding */
>         __u8    __res1;  /* reserved / padding */
>         __u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
>  };
>
> +/**
> + * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
> + * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags
> + * @sdt:   SDU (service data unit) type
> + * @flags: additional flags for CAN XL
> + * @len:   frame payload length in byte (1 .. CANXL_MAX_DLEN)
> + * @af:    acceptance field
> + * @data:  CAN XL frame payload (up to CANXL_MAX_DLEN byte)
> + *
> + * @prio shares the same position as @can_id from struct canfd_frame.
> + * Same applies to the relative position and length of @flags.
> + */
> +struct canxl_frame {
> +       canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
> +       __u8    sdt;   /* SDU (service data unit) type */
> +       __u8    flags; /* additional flags for CAN XL */
> +       __u16   len;   /* frame payload length in byte */
> +       __u32   af;    /* acceptance field */
> +       __u8    data[CANXL_MAX_DLEN];

__u8 data[];

2 kilobytes start to be a significant size. Would it make sense to use
a flexible array member to minimize the copies? A bit like the TCP/IP
stack where you do not have to allocate the MTU but just what is
actually needed for the headers and your payload.

Of course this is a tradeoff. It will add some complexity. The first
impact is that the skb's data length might be smaller than the MTU and
thus any logic using the MTU to differentiate between Classic CAN,
CAN-FD or CAN XL would have to be adjusted.

Also, are we fine to drop the __attribute__((aligned(8)))? If I
understand correctly, we moved from a 8 bytes alignment in struct
can(fd)_frame to a 4 bytes alignment in struct canxl_frame.

>
> +};
> +
>  #define CAN_MTU                (sizeof(struct can_frame))
>  #define CANFD_MTU      (sizeof(struct canfd_frame))
> +#define CANXL_MTU      (sizeof(struct canxl_frame))

#define CANXL_MTU      (sizeof(struct canxl_frame) + CANXL_MAX_DLEN)

>  /* particular protocols of the protocol family PF_CAN */
>  #define CAN_RAW                1 /* RAW sockets */
>  #define CAN_BCM                2 /* Broadcast Manager */
>  #define CAN_TP16       3 /* VAG Transport Protocol v1.6 */


Yours sincerely,
Vincent Mailhol

Re: [RFC PATCH 2/5] can: canxl: introduce ETH_P_CANXL ethernet protocol handling

[Vincent Mailhol]

On Tue. 12 Jul. 2022 at 03:44, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> Enable the PF_CAN infrastructure to handle CAN XL frames. A new ethernet
> protocol type ETH_P_CANXL is defined to tag skbuffs containing the CAN XL
> frame data structure.
>
> As the length information is now a uint16 value for CAN XL a new helper
> function can_get_data_len() is introduced to retrieve the data length
> from all types of CAN frames.
>
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>  include/linux/can/skb.h       | 14 ++++++++++
>  include/uapi/linux/if_ether.h |  1 +
>  net/can/af_can.c              | 49 +++++++++++++++++++++++++++++------
>  3 files changed, 56 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> index 182749e858b3..d043bc4afd6d 100644
> --- a/include/linux/can/skb.h
> +++ b/include/linux/can/skb.h
> @@ -101,6 +101,20 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
>  {
>         /* the CAN specific type of skb is identified by its data length */
>         return skb->len == CANFD_MTU;
>  }
>
> +/* get data length inside of CAN frame for all frame types */> +static inline unsigned int can_get_data_len(struct sk_buff *skb)
> +{
> +       if(skb->len == CANXL_MTU) {
> +               const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> +
> +               return cfx->len;
> +       } else {
> +               const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> +
> +               return cfd->len;
> +       }
> +}

What about the RTR frames?

If there are cases in which we intentionally want the declared length
and not the actual length, it might be good to have two distinct
helper functions.

/* get data length inside of CAN frame for all frame types. For
 * RTR frames, return zero. */
static inline unsigned int can_get_actual_len(struct sk_buff *skb)
{
       const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
       const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;

       if (skb->len == CANXL_MTU)
               return cfx->len;

       /* RTR frames have an actual length of zero */
       if (skb->len == CAN_MTU && cfd->flags & CAN_RTR_FLAG)
               return 0;

       return cfd->len;
}


/* get data length inside of CAN frame for all frame types. For
 * RTR frames, return requested length. */
static inline unsigned int can_get_declared_len(struct sk_buff *skb)
{
       const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
       const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;

       if (skb->len == CANXL_MTU)
               return cfx->len;

       return cfd->len;
}


> +
>  #endif /* !_CAN_SKB_H */
> diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
> index d370165bc621..69e0457eb200 100644
> --- a/include/uapi/linux/if_ether.h
> +++ b/include/uapi/linux/if_ether.h
> @@ -136,10 +136,11 @@
>  #define ETH_P_WAN_PPP   0x0007          /* Dummy type for WAN PPP frames*/
>  #define ETH_P_PPP_MP    0x0008          /* Dummy type for PPP MP frames */
>  #define ETH_P_LOCALTALK 0x0009         /* Localtalk pseudo type        */
>  #define ETH_P_CAN      0x000C          /* CAN: Controller Area Network */
>  #define ETH_P_CANFD    0x000D          /* CANFD: CAN flexible data rate*/
> +#define ETH_P_CANXL    0x000E          /* CANXL: eXtended frame Length */
>  #define ETH_P_PPPTALK  0x0010          /* Dummy type for Atalk over PPP*/
>  #define ETH_P_TR_802_2 0x0011          /* 802.2 frames                 */
>  #define ETH_P_MOBITEX  0x0015          /* Mobitex (kaz@cafe.net)       */
>  #define ETH_P_CONTROL  0x0016          /* Card specific control frames */
>  #define ETH_P_IRDA     0x0017          /* Linux-IrDA                   */
> diff --git a/net/can/af_can.c b/net/can/af_can.c
> index 1fb49d51b25d..2c9f48aa5f1f 100644
> --- a/net/can/af_can.c
> +++ b/net/can/af_can.c
> @@ -197,31 +197,32 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
>   *  -EINVAL when the skb->data does not contain a valid CAN frame
>   */
>  int can_send(struct sk_buff *skb, int loop)
>  {
>         struct sk_buff *newskb = NULL;
> -       struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> +       unsigned int len = can_get_data_len(skb);
>         struct can_pkg_stats *pkg_stats = dev_net(skb->dev)->can.pkg_stats;
>         int err = -EINVAL;
>
>         if (skb->len == CAN_MTU) {
>                 skb->protocol = htons(ETH_P_CAN);
> -               if (unlikely(cfd->len > CAN_MAX_DLEN))
> +               if (unlikely(len > CAN_MAX_DLEN))
>                         goto inval_skb;
>         } else if (skb->len == CANFD_MTU) {
>                 skb->protocol = htons(ETH_P_CANFD);
> -               if (unlikely(cfd->len > CANFD_MAX_DLEN))
> +               if (unlikely(len > CANFD_MAX_DLEN))
> +                       goto inval_skb;
> +       } else if (skb->len == CANXL_MTU) {
> +               skb->protocol = htons(ETH_P_CANXL);
> +               if (unlikely(len > CANXL_MAX_DLEN || len == 0))
>                         goto inval_skb;
>         } else {
>                 goto inval_skb;
>         }
>
> -       /* Make sure the CAN frame can pass the selected CAN netdevice.
> -        * As structs can_frame and canfd_frame are similar, we can provide
> -        * CAN FD frames to legacy CAN drivers as long as the length is <= 8
> -        */
> -       if (unlikely(skb->len > skb->dev->mtu && cfd->len > CAN_MAX_DLEN)) {
> +       /* Make sure the CAN frame can pass the selected CAN netdevice */
> +       if (unlikely(skb->len > skb->dev->mtu)) {
>                 err = -EMSGSIZE;
>                 goto inval_skb;
>         }
>
>         if (unlikely(skb->dev->type != ARPHRD_CAN)) {
> @@ -725,10 +726,36 @@ static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
>  free_skb:
>         kfree_skb(skb);
>         return NET_RX_DROP;
>  }
>
> +static int canxl_rcv(struct sk_buff *skb, struct net_device *dev,
> +                    struct packet_type *pt, struct net_device *orig_dev)
> +{
> +       struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> +
> +       if (unlikely(dev->type != ARPHRD_CAN || skb->len != CANXL_MTU)) {
> +               pr_warn_once("PF_CAN: dropped non conform CAN XL skbuff: dev type %d, len %d\n",
> +                            dev->type, skb->len);
> +               goto free_skb;
> +       }
> +
> +       /* This check is made separately since cfx->len would be uninitialized if skb->len = 0. */
> +       if (unlikely(cfx->len > CANXL_MAX_DLEN || cfx->len == 0)) {
> +               pr_warn_once("PF_CAN: dropped non conform CAN XL skbuff: dev type %d, len %d, datalen %d\n",
> +                            dev->type, skb->len, cfx->len);
> +               goto free_skb;
> +       }
> +
> +       can_receive(skb, dev);
> +       return NET_RX_SUCCESS;
> +
> +free_skb:
> +       kfree_skb(skb);
> +       return NET_RX_DROP;
> +}
> +
>  /* af_can protocol functions */
>
>  /**
>   * can_proto_register - register CAN transport protocol
>   * @cp: pointer to CAN protocol structure
> @@ -849,10 +876,15 @@ static struct packet_type can_packet __read_mostly = {
>  static struct packet_type canfd_packet __read_mostly = {
>         .type = cpu_to_be16(ETH_P_CANFD),
>         .func = canfd_rcv,
>  };
>
> +static struct packet_type canxl_packet __read_mostly = {
> +       .type = cpu_to_be16(ETH_P_CANXL),
> +       .func = canxl_rcv,
> +};
> +
>  static const struct net_proto_family can_family_ops = {
>         .family = PF_CAN,
>         .create = can_create,
>         .owner  = THIS_MODULE,
>  };
> @@ -888,10 +920,11 @@ static __init int can_init(void)
>         if (err)
>                 goto out_sock;
>
>         dev_add_pack(&can_packet);
>         dev_add_pack(&canfd_packet);
> +       dev_add_pack(&canxl_packet);
>
>         return 0;
>
>  out_sock:
>         unregister_pernet_subsys(&can_pernet_ops);
> --
> 2.30.2
>

Re: [RFC PATCH 3/5] can: dev: add CAN XL support

[Oliver Hartkopp]

On 11.07.22 21:46, Marc Kleine-Budde wrote:
> On 11.07.2022 20:34:24, Oliver Hartkopp wrote:
>> Extend the CAN device driver infrastructure to handle CAN XL frames.
>> This especially addresses the increased data length which is extended
>> to uint16 for CAN XL.
>>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> ---
>>   drivers/net/can/dev/rx-offload.c |  2 +-
>>   drivers/net/can/dev/skb.c        | 53 +++++++++++++++++++++++++++-----
>>   include/linux/can/skb.h          |  3 +-
>>   3 files changed, 48 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/can/dev/rx-offload.c b/drivers/net/can/dev/rx-offload.c
>> index a32a01c172d4..8505e547e922 100644
>> --- a/drivers/net/can/dev/rx-offload.c
>> +++ b/drivers/net/can/dev/rx-offload.c
>> @@ -245,11 +245,11 @@ unsigned int can_rx_offload_get_echo_skb(struct can_rx_offload *offload,
>>   					 unsigned int *frame_len_ptr)
>>   {
>>   	struct net_device *dev = offload->dev;
>>   	struct net_device_stats *stats = &dev->stats;
>>   	struct sk_buff *skb;
>> -	u8 len;
>> +	unsigned int len;
>>   	int err;
>>   
>>   	skb = __can_get_echo_skb(dev, idx, &len, frame_len_ptr);
>>   	if (!skb)
>>   		return 0;
>> diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
>> index 8bb62dd864c8..a849f503dcff 100644
>> --- a/drivers/net/can/dev/skb.c
>> +++ b/drivers/net/can/dev/skb.c
>> @@ -53,11 +53,12 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>>   	BUG_ON(idx >= priv->echo_skb_max);
>>   
>>   	/* check flag whether this packet has to be looped back */
>>   	if (!(dev->flags & IFF_ECHO) ||
>>   	    (skb->protocol != htons(ETH_P_CAN) &&
>> -	     skb->protocol != htons(ETH_P_CANFD))) {
>> +	     skb->protocol != htons(ETH_P_CANFD) &&
>> +	     skb->protocol != htons(ETH_P_CANXL))) {
>>   		kfree_skb(skb);
>>   		return 0;
>>   	}
>>   
>>   	if (!priv->echo_skb[idx]) {
>> @@ -86,12 +87,12 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>>   	return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(can_put_echo_skb);
>>   
>>   struct sk_buff *
>> -__can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
>> -		   unsigned int *frame_len_ptr)
>> +__can_get_echo_skb(struct net_device *dev, unsigned int idx,
>> +		   unsigned int *len_ptr, unsigned int *frame_len_ptr)
>>   {
>>   	struct can_priv *priv = netdev_priv(dev);
>>   
>>   	if (idx >= priv->echo_skb_max) {
>>   		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
>> @@ -104,16 +105,17 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
>>   		 * length is supported on both CAN and CANFD frames.
>>   		 */
>>   		struct sk_buff *skb = priv->echo_skb[idx];
>>   		struct can_skb_priv *can_skb_priv = can_skb_prv(skb);
>>   		struct canfd_frame *cf = (struct canfd_frame *)skb->data;
>> +		unsigned int len = can_get_data_len(skb);
>>   
>>   		/* get the real payload length for netdev statistics */
>>   		if (cf->can_id & CAN_RTR_FLAG)
>>   			*len_ptr = 0;
>>   		else
>> -			*len_ptr = cf->len;
>> +			*len_ptr = len;
>>   
>>   		if (frame_len_ptr)
>>   			*frame_len_ptr = can_skb_priv->frame_len;
>>   
>>   		priv->echo_skb[idx] = NULL;
>> @@ -139,11 +141,11 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
>>    */
>>   unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx,
>>   			      unsigned int *frame_len_ptr)
>>   {
>>   	struct sk_buff *skb;
>> -	u8 len;
>> +	unsigned int len;
>>   
>>   	skb = __can_get_echo_skb(dev, idx, &len, frame_len_ptr);
>>   	if (!skb)
>>   		return 0;
>>   
>> @@ -244,10 +246,41 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
>>   
>>   	return skb;
>>   }
>>   EXPORT_SYMBOL_GPL(alloc_canfd_skb);
>>   
>> +struct sk_buff *alloc_canxl_skb(struct net_device *dev,
>> +				struct canxl_frame **cfx)
> 
> The prototype has to be added to a header file

Yes! Seen that when looking through the patches on the mailing list once 
more m(

:-D

Will add it to skb.h

Best,
Oliver

Re: [RFC PATCH 2/5] can: canxl: introduce ETH_P_CANXL ethernet protocol handling

[Oliver Hartkopp]

On 11.07.22 21:41, Marc Kleine-Budde wrote:
> On 11.07.2022 20:34:23, Oliver Hartkopp wrote:
>> Enable the PF_CAN infrastructure to handle CAN XL frames. A new ethernet
>> protocol type ETH_P_CANXL is defined to tag skbuffs containing the CAN XL
>> frame data structure.
>>
>> As the length information is now a uint16 value for CAN XL a new helper
>> function can_get_data_len() is introduced to retrieve the data length
>> from all types of CAN frames.
>>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> ---
>>   include/linux/can/skb.h       | 14 ++++++++++
>>   include/uapi/linux/if_ether.h |  1 +
>>   net/can/af_can.c              | 49 +++++++++++++++++++++++++++++------
>>   3 files changed, 56 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
>> index 182749e858b3..d043bc4afd6d 100644
>> --- a/include/linux/can/skb.h
>> +++ b/include/linux/can/skb.h
>> @@ -101,6 +101,20 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
>>   {
>>   	/* the CAN specific type of skb is identified by its data length */
>>   	return skb->len == CANFD_MTU;
>>   }
>>   
>> +/* get data length inside of CAN frame for all frame types */
>> +static inline unsigned int can_get_data_len(struct sk_buff *skb)
>> +{
>> +	if(skb->len == CANXL_MTU) {
>> +		const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
>> +
>> +		return cfx->len;
>> +	} else {
>> +		const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>> +
>> +		return cfd->len;
>> +	}
>> +}
>> +
>>   #endif /* !_CAN_SKB_H */
>> diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
>> index d370165bc621..69e0457eb200 100644
>> --- a/include/uapi/linux/if_ether.h
>> +++ b/include/uapi/linux/if_ether.h
>> @@ -136,10 +136,11 @@
>>   #define ETH_P_WAN_PPP   0x0007          /* Dummy type for WAN PPP frames*/
>>   #define ETH_P_PPP_MP    0x0008          /* Dummy type for PPP MP frames */
>>   #define ETH_P_LOCALTALK 0x0009		/* Localtalk pseudo type 	*/
>>   #define ETH_P_CAN	0x000C		/* CAN: Controller Area Network */
>>   #define ETH_P_CANFD	0x000D		/* CANFD: CAN flexible data rate*/
>> +#define ETH_P_CANXL	0x000E		/* CANXL: eXtended frame Length */
>>   #define ETH_P_PPPTALK	0x0010		/* Dummy type for Atalk over PPP*/
>>   #define ETH_P_TR_802_2	0x0011		/* 802.2 frames 		*/
>>   #define ETH_P_MOBITEX	0x0015		/* Mobitex (kaz@cafe.net)	*/
>>   #define ETH_P_CONTROL	0x0016		/* Card specific control frames */
>>   #define ETH_P_IRDA	0x0017		/* Linux-IrDA			*/
> 
> This file doesn't change that often I suppose. Or does it make sense to
> send this change mainline as soon as possible?

AFAIK you only can reserve these values when you have a reference that 
uses it. I don't seen any additional pressure here.

>> diff --git a/net/can/af_can.c b/net/can/af_can.c
>> index 1fb49d51b25d..2c9f48aa5f1f 100644
>> --- a/net/can/af_can.c
>> +++ b/net/can/af_can.c
>> @@ -197,31 +197,32 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
>>    *  -EINVAL when the skb->data does not contain a valid CAN frame
>>    */
>>   int can_send(struct sk_buff *skb, int loop)
>>   {
>>   	struct sk_buff *newskb = NULL;
>> -	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>> +	unsigned int len = can_get_data_len(skb);
>>   	struct can_pkg_stats *pkg_stats = dev_net(skb->dev)->can.pkg_stats;
>>   	int err = -EINVAL;
>>   
>>   	if (skb->len == CAN_MTU) {
>>   		skb->protocol = htons(ETH_P_CAN);
>> -		if (unlikely(cfd->len > CAN_MAX_DLEN))
>> +		if (unlikely(len > CAN_MAX_DLEN))
>>   			goto inval_skb;
>>   	} else if (skb->len == CANFD_MTU) {
>>   		skb->protocol = htons(ETH_P_CANFD);
>> -		if (unlikely(cfd->len > CANFD_MAX_DLEN))
>> +		if (unlikely(len > CANFD_MAX_DLEN))
>> +			goto inval_skb;
>> +	} else if (skb->len == CANXL_MTU) {
>> +		skb->protocol = htons(ETH_P_CANXL);
>> +		if (unlikely(len > CANXL_MAX_DLEN || len == 0))
> 
> Do we need a helper for the > CANXL_MAX_DLEN || == 0 check?

Some can_xl_is_valid_data_size(unsigned int len) ?

Fine for me.

Does the name fit for you?

Regards,
Oliver


> 
>>   			goto inval_skb;
>>   	} else {
>>   		goto inval_skb;
>>   	}
>>   
>> -	/* Make sure the CAN frame can pass the selected CAN netdevice.
>> -	 * As structs can_frame and canfd_frame are similar, we can provide
>> -	 * CAN FD frames to legacy CAN drivers as long as the length is <= 8
>> -	 */
>> -	if (unlikely(skb->len > skb->dev->mtu && cfd->len > CAN_MAX_DLEN)) {
>> +	/* Make sure the CAN frame can pass the selected CAN netdevice */
>> +	if (unlikely(skb->len > skb->dev->mtu)) {
>>   		err = -EMSGSIZE;
>>   		goto inval_skb;
>>   	}
>>   
>>   	if (unlikely(skb->dev->type != ARPHRD_CAN)) {
>> @@ -725,10 +726,36 @@ static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
>>   free_skb:
>>   	kfree_skb(skb);
>>   	return NET_RX_DROP;
>>   }
>>   
>> +static int canxl_rcv(struct sk_buff *skb, struct net_device *dev,
>> +		     struct packet_type *pt, struct net_device *orig_dev)
>> +{
>> +	struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
>> +
>> +	if (unlikely(dev->type != ARPHRD_CAN || skb->len != CANXL_MTU)) {
>> +		pr_warn_once("PF_CAN: dropped non conform CAN XL skbuff: dev type %d, len %d\n",
>> +			     dev->type, skb->len);
>> +		goto free_skb;
>> +	}
>> +
>> +	/* This check is made separately since cfx->len would be uninitialized if skb->len = 0. */
>> +	if (unlikely(cfx->len > CANXL_MAX_DLEN || cfx->len == 0)) {
>                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> regards,
> Marc
> 

Re: [RFC PATCH 2/5] can: canxl: introduce ETH_P_CANXL ethernet protocol handling

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4920 bytes --]

On 12.07.2022 09:12:49, Oliver Hartkopp wrote:
> 
> 
> On 11.07.22 21:41, Marc Kleine-Budde wrote:
> > On 11.07.2022 20:34:23, Oliver Hartkopp wrote:
> > > Enable the PF_CAN infrastructure to handle CAN XL frames. A new ethernet
> > > protocol type ETH_P_CANXL is defined to tag skbuffs containing the CAN XL
> > > frame data structure.
> > > 
> > > As the length information is now a uint16 value for CAN XL a new helper
> > > function can_get_data_len() is introduced to retrieve the data length
> > > from all types of CAN frames.
> > > 
> > > Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> > > ---
> > >   include/linux/can/skb.h       | 14 ++++++++++
> > >   include/uapi/linux/if_ether.h |  1 +
> > >   net/can/af_can.c              | 49 +++++++++++++++++++++++++++++------
> > >   3 files changed, 56 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> > > index 182749e858b3..d043bc4afd6d 100644
> > > --- a/include/linux/can/skb.h
> > > +++ b/include/linux/can/skb.h
> > > @@ -101,6 +101,20 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
> > >   {
> > >   	/* the CAN specific type of skb is identified by its data length */
> > >   	return skb->len == CANFD_MTU;
> > >   }
> > > +/* get data length inside of CAN frame for all frame types */
> > > +static inline unsigned int can_get_data_len(struct sk_buff *skb)
> > > +{
> > > +	if(skb->len == CANXL_MTU) {
> > > +		const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> > > +
> > > +		return cfx->len;
> > > +	} else {
> > > +		const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> > > +
> > > +		return cfd->len;
> > > +	}
> > > +}
> > > +
> > >   #endif /* !_CAN_SKB_H */
> > > diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
> > > index d370165bc621..69e0457eb200 100644
> > > --- a/include/uapi/linux/if_ether.h
> > > +++ b/include/uapi/linux/if_ether.h
> > > @@ -136,10 +136,11 @@
> > >   #define ETH_P_WAN_PPP   0x0007          /* Dummy type for WAN PPP frames*/
> > >   #define ETH_P_PPP_MP    0x0008          /* Dummy type for PPP MP frames */
> > >   #define ETH_P_LOCALTALK 0x0009		/* Localtalk pseudo type 	*/
> > >   #define ETH_P_CAN	0x000C		/* CAN: Controller Area Network */
> > >   #define ETH_P_CANFD	0x000D		/* CANFD: CAN flexible data rate*/
> > > +#define ETH_P_CANXL	0x000E		/* CANXL: eXtended frame Length */
> > >   #define ETH_P_PPPTALK	0x0010		/* Dummy type for Atalk over PPP*/
> > >   #define ETH_P_TR_802_2	0x0011		/* 802.2 frames 		*/
> > >   #define ETH_P_MOBITEX	0x0015		/* Mobitex (kaz@cafe.net)	*/
> > >   #define ETH_P_CONTROL	0x0016		/* Card specific control frames */
> > >   #define ETH_P_IRDA	0x0017		/* Linux-IrDA			*/
> > 
> > This file doesn't change that often I suppose. Or does it make sense to
> > send this change mainline as soon as possible?
> 
> AFAIK you only can reserve these values when you have a reference that uses
> it. I don't seen any additional pressure here.

There have been added other types that are not used in the kernel (yet).

> > > diff --git a/net/can/af_can.c b/net/can/af_can.c
> > > index 1fb49d51b25d..2c9f48aa5f1f 100644
> > > --- a/net/can/af_can.c
> > > +++ b/net/can/af_can.c
> > > @@ -197,31 +197,32 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
> > >    *  -EINVAL when the skb->data does not contain a valid CAN frame
> > >    */
> > >   int can_send(struct sk_buff *skb, int loop)
> > >   {
> > >   	struct sk_buff *newskb = NULL;
> > > -	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> > > +	unsigned int len = can_get_data_len(skb);
> > >   	struct can_pkg_stats *pkg_stats = dev_net(skb->dev)->can.pkg_stats;
> > >   	int err = -EINVAL;
> > >   	if (skb->len == CAN_MTU) {
> > >   		skb->protocol = htons(ETH_P_CAN);
> > > -		if (unlikely(cfd->len > CAN_MAX_DLEN))
> > > +		if (unlikely(len > CAN_MAX_DLEN))
> > >   			goto inval_skb;
> > >   	} else if (skb->len == CANFD_MTU) {
> > >   		skb->protocol = htons(ETH_P_CANFD);
> > > -		if (unlikely(cfd->len > CANFD_MAX_DLEN))
> > > +		if (unlikely(len > CANFD_MAX_DLEN))
> > > +			goto inval_skb;
> > > +	} else if (skb->len == CANXL_MTU) {
> > > +		skb->protocol = htons(ETH_P_CANXL);
> > > +		if (unlikely(len > CANXL_MAX_DLEN || len == 0))
> > 
> > Do we need a helper for the > CANXL_MAX_DLEN || == 0 check?
> 
> Some can_xl_is_valid_data_size(unsigned int len) ?

In other location you're using "canxl" not "can_xl".
What about: canxl_valid_data_size()

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 1/5] can: canxl: introduce CAN XL data structure

[Oliver Hartkopp]

On 12.07.22 02:36, Vincent Mailhol wrote:
> On Tue. 12 Jul. 2022 at 03:44, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>
>> This patch adds defines for data structures and length information for
>> CAN XL (CAN with eXtended data Length) which can transfer up to 2048
>> byte insinde a single frame.
>>
>> Notable changes from CAN FD:
>>
>> - the 11 bit arbitration field is now named 'priority' instead of 'can_id'
>>    (there are no 29 bit identifiers nor RTR frames anymore)
>> - the data length needs a uint16 value to cover up to 2048 byte
>>    (the length element position is different to struct can[fd]_frame)
>> - new fields (SDT, AF) and a SEC bit have been introduced
>> - the virtual CAN interface identifier is not part if the CAN XL frame
>>    struct as this VCID value is stored in struct skbuff (analog to vlan id)
>>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> ---
>>   include/uapi/linux/can.h | 38 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 38 insertions(+)
>>
>> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
>> index 90801ada2bbe..9f97a5d06f3b 100644
>> --- a/include/uapi/linux/can.h
>> +++ b/include/uapi/linux/can.h
>> @@ -58,10 +58,11 @@
>>
>>   /* valid bits in CAN ID for frame formats */
>>   #define CAN_SFF_MASK 0x000007FFU /* standard frame format (SFF) */
>>   #define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) */
>>   #define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */
>> +#define CANXL_PRIO_MASK CAN_SFF_MASK /* 11 bit priority mask */
>>
>>   /*
>>    * Controller Area Network Identifier structure
>>    *
>>    * bit 0-28    : CAN identifier (11/29 bit)
>> @@ -71,10 +72,11 @@
>>    */
>>   typedef __u32 canid_t;
>>
>>   #define CAN_SFF_ID_BITS                11
>>   #define CAN_EFF_ID_BITS                29
>> +#define CANXL_PRIO_BITS                CAN_SFF_ID_BITS
>>
>>   /*
>>    * Controller Area Network Error Message Frame Mask structure
>>    *
>>    * bit 0-28    : error class mask (see include/uapi/linux/can/error.h)
>> @@ -89,10 +91,18 @@ typedef __u32 can_err_mask_t;
>>
>>   /* CAN FD payload length and DLC definitions according to ISO 11898-7 */
>>   #define CANFD_MAX_DLC 15
>>   #define CANFD_MAX_DLEN 64
>>
>> +/*
>> + * CAN XL payload length and DLC definitions according to ISO 11898-1
>> + * CAN XL DLC ranges from 0 .. 2047 => data length from 1 .. 2048 byte
>> + */
>> +#define CANXL_MAX_DLC 2047
>> +#define CANXL_MAX_DLC_MASK 0x07FF
>> +#define CANXL_MAX_DLEN 2048
>> +
>>   /**
>>    * struct can_frame - Classical CAN frame structure (aka CAN 2.0B)
>>    * @can_id:   CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
>>    * @len:      CAN frame payload length in byte (0 .. 8)
>>    * @can_dlc:  deprecated name for CAN frame payload length in byte (0 .. 8)
>> @@ -141,14 +151,20 @@ struct can_frame {
>>    * When this is done the former differentiation via CAN_MTU / CANFD_MTU gets
>>    * lost. CANFD_FDF allows programmers to mark CAN FD frames in the case of
>>    * using struct canfd_frame for mixed CAN / CAN FD content (dual use).
>>    * N.B. the Kernel APIs do NOT provide mixed CAN / CAN FD content inside of
>>    * struct canfd_frame therefore the CANFD_FDF flag is disregarded by Linux.
>> + * Same applies to the CANXL_XLF bit.
>> + *
>> + * For CAN XL the SEC bit has been added to the flags field which shares the
>> + * same position in struct can[fd|xl]_frame.
>>    */
>>   #define CANFD_BRS 0x01 /* bit rate switch (second bitrate for payload data) */
>>   #define CANFD_ESI 0x02 /* error state indicator of the transmitting node */
>>   #define CANFD_FDF 0x04 /* mark CAN FD for dual use of struct canfd_frame */
>> +#define CANXL_XLF 0x08 /* mark CAN XL for dual use of struct canfd_frame */
>> +#define CANXL_SEC 0x10 /* Simple Extended Content (security/segmentation) */
>>
>>   /**
>>    * struct canfd_frame - CAN flexible data rate frame structure
>>    * @can_id: CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
>>    * @len:    frame payload length in byte (0 .. CANFD_MAX_DLEN)
>> @@ -164,12 +180,34 @@ struct canfd_frame {
>>          __u8    __res0;  /* reserved / padding */
>>          __u8    __res1;  /* reserved / padding */
>>          __u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
>>   };
>>
>> +/**
>> + * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
>> + * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags
>> + * @sdt:   SDU (service data unit) type
>> + * @flags: additional flags for CAN XL
>> + * @len:   frame payload length in byte (1 .. CANXL_MAX_DLEN)
>> + * @af:    acceptance field
>> + * @data:  CAN XL frame payload (up to CANXL_MAX_DLEN byte)
>> + *
>> + * @prio shares the same position as @can_id from struct canfd_frame.
>> + * Same applies to the relative position and length of @flags.
>> + */
>> +struct canxl_frame {
>> +       canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
>> +       __u8    sdt;   /* SDU (service data unit) type */
>> +       __u8    flags; /* additional flags for CAN XL */
>> +       __u16   len;   /* frame payload length in byte */
>> +       __u32   af;    /* acceptance field */
>> +       __u8    data[CANXL_MAX_DLEN];
> 
> __u8 data[];
> 
> 2 kilobytes start to be a significant size. Would it make sense to use
> a flexible array member to minimize the copies? A bit like the TCP/IP
> stack where you do not have to allocate the MTU but just what is
> actually needed for the headers and your payload.
> 
> Of course this is a tradeoff. It will add some complexity. The first
> impact is that the skb's data length might be smaller than the MTU and
> thus any logic using the MTU to differentiate between Classic CAN,
> CAN-FD or CAN XL would have to be adjusted.

Yes, I've thought about that myself but I wanted a simple start for our 
discussion to think about improvements in the team.

I implemented this first:

  /* Drop a given socketbuffer if it does not contain a valid CAN frame. */
  bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
  {
-       const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+       unsigned int len = can_get_data_len(skb);
         struct can_priv *priv = netdev_priv(dev);

         if (skb->protocol == htons(ETH_P_CAN)) {
                 if (unlikely(skb->len != CAN_MTU ||
-                            cfd->len > CAN_MAX_DLEN))
+                            len > CAN_MAX_DLEN))
                         goto inval_skb;
         } else if (skb->protocol == htons(ETH_P_CANFD)) {
                 if (unlikely(skb->len != CANFD_MTU ||
-                            cfd->len > CANFD_MAX_DLEN))
+                            len > CANFD_MAX_DLEN))
+                       goto inval_skb;
+       } else if (skb->protocol == htons(ETH_P_CANXL)) {
+               if (unlikely(skb->len < CANXL_MINTU ||
+                            skb->len > CANXL_MTU ||
+                            len > CANXL_MAX_DLEN || len == 0))
                         goto inval_skb;
         } else {
                 goto inval_skb;
         }

(..)

+/* truncated CAN XL structs must contain at least 64 data bytes */
+#define CANXL_MINTU    (CANXL_MTU - CANXL_MAX_DLEN + CANFD_MAX_DLEN)

So the idea was to define a CAN XL skb->len which is clearly above 
CANFD_MTU to distinguish it from the other CAN MTUs.

But as the skbuff is zerocopy inside the kernel, it probably makes sense 
to stay with the full CANXL_MTU inside the kernel and allow to crop the 
data structure for CAN_RAW socket interactions from/to user space down 
to CANXL_MINTU ?!?

> Also, are we fine to drop the __attribute__((aligned(8)))? If I
> understand correctly, we moved from a 8 bytes alignment in struct
> can(fd)_frame to a 4 bytes alignment in struct canxl_frame.

Yes. I hassled with the alignment too.

The big cool thing about the 64 bit alignment was the filter and 
modification efficiency in bcm.c and gw.c

I wonder if this is still a relevant use case with CAN XL.

Currently the SDU type SDT=0x03 defines a Classical CAN and CAN FD 
'tunneling' for CAN XL (in CiA 611-1 document).

For this SDT=0x03 the CAN ID (and EFF/RTR/FD flags) are placed in the AF 
element.

And then the first data[0] byte will contain ESI/BRS/DLC and starting 
with data[1] the CAN frame data content will start.

So at least this spec will horribly break and 64 bit access to CAN data 
content.

I've been thinking about creating a 'normal' Classical CAN / CAN FD 
virtual CAN interface that feels for the user like a standard CAN 
interface with struct can[fd]_frame - but inside interacts with CAN XL 
frames with SDT=0x03 ...

Don't know if users really will need such stuff with CAN XL as there are 
other PDU tunneling mechanics already specified.

For that reason I would not take the 64 bit alignment as a strong 
requirement. With the current struct canxl_frame layout the data[] will 
start at a 32 bit boundary.

At the end I would see CAN XL as some Ethernet implementation with a 
cool arbitration concept from CAN that assures CSMA/C[AR] instead of 
CSMA/CD ;-)

Best regards,
Oliver

> 
>>
>> +};
>> +
>>   #define CAN_MTU                (sizeof(struct can_frame))
>>   #define CANFD_MTU      (sizeof(struct canfd_frame))
>> +#define CANXL_MTU      (sizeof(struct canxl_frame))
> 
> #define CANXL_MTU      (sizeof(struct canxl_frame) + CANXL_MAX_DLEN)
> 
>>   /* particular protocols of the protocol family PF_CAN */
>>   #define CAN_RAW                1 /* RAW sockets */
>>   #define CAN_BCM                2 /* Broadcast Manager */
>>   #define CAN_TP16       3 /* VAG Transport Protocol v1.6 */
> 
> 
> Yours sincerely,
> Vincent Mailhol

Re: [RFC PATCH 2/5] can: canxl: introduce ETH_P_CANXL ethernet protocol handling

[Oliver Hartkopp]

On 12.07.22 09:17, Marc Kleine-Budde wrote:
> On 12.07.2022 09:12:49, Oliver Hartkopp wrote:
>>
>>
>> On 11.07.22 21:41, Marc Kleine-Budde wrote:
>>> On 11.07.2022 20:34:23, Oliver Hartkopp wrote:
>>>> Enable the PF_CAN infrastructure to handle CAN XL frames. A new ethernet
>>>> protocol type ETH_P_CANXL is defined to tag skbuffs containing the CAN XL
>>>> frame data structure.
>>>>
>>>> As the length information is now a uint16 value for CAN XL a new helper
>>>> function can_get_data_len() is introduced to retrieve the data length
>>>> from all types of CAN frames.
>>>>
>>>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>>>> ---
>>>>    include/linux/can/skb.h       | 14 ++++++++++
>>>>    include/uapi/linux/if_ether.h |  1 +
>>>>    net/can/af_can.c              | 49 +++++++++++++++++++++++++++++------
>>>>    3 files changed, 56 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
>>>> index 182749e858b3..d043bc4afd6d 100644
>>>> --- a/include/linux/can/skb.h
>>>> +++ b/include/linux/can/skb.h
>>>> @@ -101,6 +101,20 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
>>>>    {
>>>>    	/* the CAN specific type of skb is identified by its data length */
>>>>    	return skb->len == CANFD_MTU;
>>>>    }
>>>> +/* get data length inside of CAN frame for all frame types */
>>>> +static inline unsigned int can_get_data_len(struct sk_buff *skb)
>>>> +{
>>>> +	if(skb->len == CANXL_MTU) {
>>>> +		const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
>>>> +
>>>> +		return cfx->len;
>>>> +	} else {
>>>> +		const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>>>> +
>>>> +		return cfd->len;
>>>> +	}
>>>> +}
>>>> +
>>>>    #endif /* !_CAN_SKB_H */
>>>> diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
>>>> index d370165bc621..69e0457eb200 100644
>>>> --- a/include/uapi/linux/if_ether.h
>>>> +++ b/include/uapi/linux/if_ether.h
>>>> @@ -136,10 +136,11 @@
>>>>    #define ETH_P_WAN_PPP   0x0007          /* Dummy type for WAN PPP frames*/
>>>>    #define ETH_P_PPP_MP    0x0008          /* Dummy type for PPP MP frames */
>>>>    #define ETH_P_LOCALTALK 0x0009		/* Localtalk pseudo type 	*/
>>>>    #define ETH_P_CAN	0x000C		/* CAN: Controller Area Network */
>>>>    #define ETH_P_CANFD	0x000D		/* CANFD: CAN flexible data rate*/
>>>> +#define ETH_P_CANXL	0x000E		/* CANXL: eXtended frame Length */
>>>>    #define ETH_P_PPPTALK	0x0010		/* Dummy type for Atalk over PPP*/
>>>>    #define ETH_P_TR_802_2	0x0011		/* 802.2 frames 		*/
>>>>    #define ETH_P_MOBITEX	0x0015		/* Mobitex (kaz@cafe.net)	*/
>>>>    #define ETH_P_CONTROL	0x0016		/* Card specific control frames */
>>>>    #define ETH_P_IRDA	0x0017		/* Linux-IrDA			*/
>>>
>>> This file doesn't change that often I suppose. Or does it make sense to
>>> send this change mainline as soon as possible?
>>
>> AFAIK you only can reserve these values when you have a reference that uses
>> it. I don't seen any additional pressure here.
> 
> There have been added other types that are not used in the kernel (yet).
> 

Ok?!?

I remembered some discussion about PF_FLEXRAY which bounced some years ago.

But I think it should be fine if we get this small patchset discussed 
and upstreamed to net-next in this round, right?

>>>> diff --git a/net/can/af_can.c b/net/can/af_can.c
>>>> index 1fb49d51b25d..2c9f48aa5f1f 100644
>>>> --- a/net/can/af_can.c
>>>> +++ b/net/can/af_can.c
>>>> @@ -197,31 +197,32 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
>>>>     *  -EINVAL when the skb->data does not contain a valid CAN frame
>>>>     */
>>>>    int can_send(struct sk_buff *skb, int loop)
>>>>    {
>>>>    	struct sk_buff *newskb = NULL;
>>>> -	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>>>> +	unsigned int len = can_get_data_len(skb);
>>>>    	struct can_pkg_stats *pkg_stats = dev_net(skb->dev)->can.pkg_stats;
>>>>    	int err = -EINVAL;
>>>>    	if (skb->len == CAN_MTU) {
>>>>    		skb->protocol = htons(ETH_P_CAN);
>>>> -		if (unlikely(cfd->len > CAN_MAX_DLEN))
>>>> +		if (unlikely(len > CAN_MAX_DLEN))
>>>>    			goto inval_skb;
>>>>    	} else if (skb->len == CANFD_MTU) {
>>>>    		skb->protocol = htons(ETH_P_CANFD);
>>>> -		if (unlikely(cfd->len > CANFD_MAX_DLEN))
>>>> +		if (unlikely(len > CANFD_MAX_DLEN))
>>>> +			goto inval_skb;
>>>> +	} else if (skb->len == CANXL_MTU) {
>>>> +		skb->protocol = htons(ETH_P_CANXL);
>>>> +		if (unlikely(len > CANXL_MAX_DLEN || len == 0))
>>>
>>> Do we need a helper for the > CANXL_MAX_DLEN || == 0 check?
>>
>> Some can_xl_is_valid_data_size(unsigned int len) ?
> 
> In other location you're using "canxl" not "can_xl".

I just thought about can_blablabla() as common namespace for our functions.

But canxl_whatever() is also safe ;-)

> What about: canxl_valid_data_size()

Fine for me.

Thanks,
Oliver

Re: [RFC PATCH 2/5] can: canxl: introduce ETH_P_CANXL ethernet protocol handling

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3889 bytes --]

On 12.07.2022 10:02:28, Oliver Hartkopp wrote:
> > > > > --- a/include/uapi/linux/if_ether.h
> > > > > +++ b/include/uapi/linux/if_ether.h
> > > > > @@ -136,10 +136,11 @@
> > > > >    #define ETH_P_WAN_PPP   0x0007          /* Dummy type for WAN PPP frames*/
> > > > >    #define ETH_P_PPP_MP    0x0008          /* Dummy type for PPP MP frames */
> > > > >    #define ETH_P_LOCALTALK 0x0009		/* Localtalk pseudo type 	*/
> > > > >    #define ETH_P_CAN	0x000C		/* CAN: Controller Area Network */
> > > > >    #define ETH_P_CANFD	0x000D		/* CANFD: CAN flexible data rate*/
> > > > > +#define ETH_P_CANXL	0x000E		/* CANXL: eXtended frame Length */
> > > > >    #define ETH_P_PPPTALK	0x0010		/* Dummy type for Atalk over PPP*/
> > > > >    #define ETH_P_TR_802_2	0x0011		/* 802.2 frames 		*/
> > > > >    #define ETH_P_MOBITEX	0x0015		/* Mobitex (kaz@cafe.net)	*/
> > > > >    #define ETH_P_CONTROL	0x0016		/* Card specific control frames */
> > > > >    #define ETH_P_IRDA	0x0017		/* Linux-IrDA			*/
> > > > 
> > > > This file doesn't change that often I suppose. Or does it make sense to
> > > > send this change mainline as soon as possible?
> > > 
> > > AFAIK you only can reserve these values when you have a reference that uses
> > > it. I don't seen any additional pressure here.
> > 
> > There have been added other types that are not used in the kernel (yet).
> > 
> 
> Ok?!?
> 
> I remembered some discussion about PF_FLEXRAY which bounced some years ago.
> 
> But I think it should be fine if we get this small patchset discussed and
> upstreamed to net-next in this round, right?

ACK

> > > > > diff --git a/net/can/af_can.c b/net/can/af_can.c
> > > > > index 1fb49d51b25d..2c9f48aa5f1f 100644
> > > > > --- a/net/can/af_can.c
> > > > > +++ b/net/can/af_can.c
> > > > > @@ -197,31 +197,32 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
> > > > >     *  -EINVAL when the skb->data does not contain a valid CAN frame
> > > > >     */
> > > > >    int can_send(struct sk_buff *skb, int loop)
> > > > >    {
> > > > >    	struct sk_buff *newskb = NULL;
> > > > > -	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> > > > > +	unsigned int len = can_get_data_len(skb);
> > > > >    	struct can_pkg_stats *pkg_stats = dev_net(skb->dev)->can.pkg_stats;
> > > > >    	int err = -EINVAL;
> > > > >    	if (skb->len == CAN_MTU) {
> > > > >    		skb->protocol = htons(ETH_P_CAN);
> > > > > -		if (unlikely(cfd->len > CAN_MAX_DLEN))
> > > > > +		if (unlikely(len > CAN_MAX_DLEN))
> > > > >    			goto inval_skb;
> > > > >    	} else if (skb->len == CANFD_MTU) {
> > > > >    		skb->protocol = htons(ETH_P_CANFD);
> > > > > -		if (unlikely(cfd->len > CANFD_MAX_DLEN))
> > > > > +		if (unlikely(len > CANFD_MAX_DLEN))
> > > > > +			goto inval_skb;
> > > > > +	} else if (skb->len == CANXL_MTU) {
> > > > > +		skb->protocol = htons(ETH_P_CANXL);
> > > > > +		if (unlikely(len > CANXL_MAX_DLEN || len == 0))
> > > > 
> > > > Do we need a helper for the > CANXL_MAX_DLEN || == 0 check?
> > > 
> > > Some can_xl_is_valid_data_size(unsigned int len) ?
> > 
> > In other location you're using "canxl" not "can_xl".

...but that's the struct canxl_frame and alloc_canxl_skb().

> I just thought about can_blablabla() as common namespace for our functions.

ACK it is. There are:

include/linux/can/length.h:160:u8 can_fd_dlc2len(u8 dlc);
include/linux/can/length.h:163:u8 can_fd_len2dlc(u8 len);

> But canxl_whatever() is also safe ;-)
> 
> > What about: canxl_valid_data_size()

Sorry for the noise,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 1/5] can: canxl: introduce CAN XL data structure

[Vincent Mailhol]

On Tue. 12 juil. 2022 at 16:55, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 12.07.22 02:36, Vincent Mailhol wrote:
> > On Tue. 12 Jul. 2022 at 03:44, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >>
> >> This patch adds defines for data structures and length information for
> >> CAN XL (CAN with eXtended data Length) which can transfer up to 2048
> >> byte insinde a single frame.
> >>
> >> Notable changes from CAN FD:
> >>
> >> - the 11 bit arbitration field is now named 'priority' instead of 'can_id'
> >>    (there are no 29 bit identifiers nor RTR frames anymore)
> >> - the data length needs a uint16 value to cover up to 2048 byte
> >>    (the length element position is different to struct can[fd]_frame)
> >> - new fields (SDT, AF) and a SEC bit have been introduced
> >> - the virtual CAN interface identifier is not part if the CAN XL frame
> >>    struct as this VCID value is stored in struct skbuff (analog to vlan id)
> >>
> >> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> >> ---
> >>   include/uapi/linux/can.h | 38 ++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 38 insertions(+)
> >>
> >> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
> >> index 90801ada2bbe..9f97a5d06f3b 100644
> >> --- a/include/uapi/linux/can.h
> >> +++ b/include/uapi/linux/can.h
> >> @@ -58,10 +58,11 @@
> >>
> >>   /* valid bits in CAN ID for frame formats */
> >>   #define CAN_SFF_MASK 0x000007FFU /* standard frame format (SFF) */
> >>   #define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) */
> >>   #define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */
> >> +#define CANXL_PRIO_MASK CAN_SFF_MASK /* 11 bit priority mask */
> >>
> >>   /*
> >>    * Controller Area Network Identifier structure
> >>    *
> >>    * bit 0-28    : CAN identifier (11/29 bit)
> >> @@ -71,10 +72,11 @@
> >>    */
> >>   typedef __u32 canid_t;
> >>
> >>   #define CAN_SFF_ID_BITS                11
> >>   #define CAN_EFF_ID_BITS                29
> >> +#define CANXL_PRIO_BITS                CAN_SFF_ID_BITS
> >>
> >>   /*
> >>    * Controller Area Network Error Message Frame Mask structure
> >>    *
> >>    * bit 0-28    : error class mask (see include/uapi/linux/can/error.h)
> >> @@ -89,10 +91,18 @@ typedef __u32 can_err_mask_t;
> >>
> >>   /* CAN FD payload length and DLC definitions according to ISO 11898-7 */
> >>   #define CANFD_MAX_DLC 15
> >>   #define CANFD_MAX_DLEN 64
> >>
> >> +/*
> >> + * CAN XL payload length and DLC definitions according to ISO 11898-1
> >> + * CAN XL DLC ranges from 0 .. 2047 => data length from 1 .. 2048 byte
> >> + */
> >> +#define CANXL_MAX_DLC 2047
> >> +#define CANXL_MAX_DLC_MASK 0x07FF
> >> +#define CANXL_MAX_DLEN 2048
> >> +
> >>   /**
> >>    * struct can_frame - Classical CAN frame structure (aka CAN 2.0B)
> >>    * @can_id:   CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
> >>    * @len:      CAN frame payload length in byte (0 .. 8)
> >>    * @can_dlc:  deprecated name for CAN frame payload length in byte (0 .. 8)
> >> @@ -141,14 +151,20 @@ struct can_frame {
> >>    * When this is done the former differentiation via CAN_MTU / CANFD_MTU gets
> >>    * lost. CANFD_FDF allows programmers to mark CAN FD frames in the case of
> >>    * using struct canfd_frame for mixed CAN / CAN FD content (dual use).
> >>    * N.B. the Kernel APIs do NOT provide mixed CAN / CAN FD content inside of
> >>    * struct canfd_frame therefore the CANFD_FDF flag is disregarded by Linux.
> >> + * Same applies to the CANXL_XLF bit.
> >> + *
> >> + * For CAN XL the SEC bit has been added to the flags field which shares the
> >> + * same position in struct can[fd|xl]_frame.
> >>    */
> >>   #define CANFD_BRS 0x01 /* bit rate switch (second bitrate for payload data) */
> >>   #define CANFD_ESI 0x02 /* error state indicator of the transmitting node */
> >>   #define CANFD_FDF 0x04 /* mark CAN FD for dual use of struct canfd_frame */
> >> +#define CANXL_XLF 0x08 /* mark CAN XL for dual use of struct canfd_frame */
> >> +#define CANXL_SEC 0x10 /* Simple Extended Content (security/segmentation) */
> >>
> >>   /**
> >>    * struct canfd_frame - CAN flexible data rate frame structure
> >>    * @can_id: CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
> >>    * @len:    frame payload length in byte (0 .. CANFD_MAX_DLEN)
> >> @@ -164,12 +180,34 @@ struct canfd_frame {
> >>          __u8    __res0;  /* reserved / padding */
> >>          __u8    __res1;  /* reserved / padding */
> >>          __u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
> >>   };
> >>
> >> +/**
> >> + * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
> >> + * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags
> >> + * @sdt:   SDU (service data unit) type
> >> + * @flags: additional flags for CAN XL
> >> + * @len:   frame payload length in byte (1 .. CANXL_MAX_DLEN)
> >> + * @af:    acceptance field
> >> + * @data:  CAN XL frame payload (up to CANXL_MAX_DLEN byte)
> >> + *
> >> + * @prio shares the same position as @can_id from struct canfd_frame.
> >> + * Same applies to the relative position and length of @flags.
> >> + */
> >> +struct canxl_frame {
> >> +       canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
> >> +       __u8    sdt;   /* SDU (service data unit) type */
> >> +       __u8    flags; /* additional flags for CAN XL */
> >> +       __u16   len;   /* frame payload length in byte */
> >> +       __u32   af;    /* acceptance field */
> >> +       __u8    data[CANXL_MAX_DLEN];
> >
> > __u8 data[];
> >
> > 2 kilobytes start to be a significant size. Would it make sense to use
> > a flexible array member to minimize the copies? A bit like the TCP/IP
> > stack where you do not have to allocate the MTU but just what is
> > actually needed for the headers and your payload.
> >
> > Of course this is a tradeoff. It will add some complexity. The first
> > impact is that the skb's data length might be smaller than the MTU and
> > thus any logic using the MTU to differentiate between Classic CAN,
> > CAN-FD or CAN XL would have to be adjusted.
>
> Yes, I've thought about that myself but I wanted a simple start for our
> discussion to think about improvements in the team.
>
> I implemented this first:
>
>   /* Drop a given socketbuffer if it does not contain a valid CAN frame. */
>   bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
>   {
> -       const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> +       unsigned int len = can_get_data_len(skb);

It is premature to use can_get_data_len() here. You have not yet
confirmed the skb’s length so this could be an out of band read.

>          struct can_priv *priv = netdev_priv(dev);
>
>          if (skb->protocol == htons(ETH_P_CAN)) {
>                  if (unlikely(skb->len != CAN_MTU ||
> -                            cfd->len > CAN_MAX_DLEN))
> +                            len > CAN_MAX_DLEN))
>                          goto inval_skb;
>          } else if (skb->protocol == htons(ETH_P_CANFD)) {
>                  if (unlikely(skb->len != CANFD_MTU ||
> -                            cfd->len > CANFD_MAX_DLEN))
> +                            len > CANFD_MAX_DLEN))
> +                       goto inval_skb;
> +       } else if (skb->protocol == htons(ETH_P_CANXL)) {
> +               if (unlikely(skb->len < CANXL_MINTU ||
> +                            skb->len > CANXL_MTU ||
> +                            len > CANXL_MAX_DLEN || len == 0))
>                          goto inval_skb;
>          } else {
>                  goto inval_skb;
>          }

I suggest this:

  /* Drop a given socket buffer if it does not contain a valid CAN frame. */
  bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
  {
         const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+        const struct canxl_frame *cxl = (struct canxl_frame *)skb->data;
         struct can_priv *priv = netdev_priv(dev);

         if (skb->protocol == htons(ETH_P_CAN)) {
                 if (unlikely(skb->len != CAN_MTU ||
                              cfd->len > CAN_MAX_DLEN))
                         goto inval_skb;
         } else if (skb->protocol == htons(ETH_P_CANFD)) {
                 if (unlikely(skb->len != CANFD_MTU ||
                              cfd->len > CANFD_MAX_DLEN))
                         goto inval_skb;
+       } else if (skb->protocol == htons(ETH_P_CANXL)) {
+               if (unlikely(skb->len < sizeof(struct canxl_frame) ||
+                            skb->len > CANXL_MTU ||
+                            cxl->len > CANXL_MAX_DLEN || cxl->len == 0))
+                        goto inval_skb;
         } else {
                 goto inval_skb;
         }

Once can_dropped_invalid_skb() succeeds, calls to can_get_data_len()
will be safe.

> +/* truncated CAN XL structs must contain at least 64 data bytes */
> +#define CANXL_MINTU    (CANXL_MTU - CANXL_MAX_DLEN + CANFD_MAX_DLEN)

I did not get the concept of the "truncated CAN XL structs". The valid
data field lengths are 1 to 2048, right? I did not get where this 64
comes from.
Your formula is equivalent to
#define CANXL_MINTU    (sizeof(struct canxl_frame) + CANFD_MAX_DLEN)

I would have just expected:
#define CANXL_MINTU    (sizeof(struct canxl_frame))

Or maybe no macro at all, the sizeof is more explicit to me.

> So the idea was to define a CAN XL skb->len which is clearly above
> CANFD_MTU to distinguish it from the other CAN MTUs.
>
> But as the skbuff is zerocopy inside the kernel,

Inside the kernel yes, but there is still one copy between userspace
and kernel land with the full width.

> it probably makes sense
> to stay with the full CANXL_MTU inside the kernel and allow to crop the
> data structure for CAN_RAW socket interactions from/to user space down
> to CANXL_MINTU ?!?

My guts would tell me to crop it from the initial malloc in userland.
Not sure what would be the impact in terms of performances.

> > Also, are we fine to drop the __attribute__((aligned(8)))? If I
> > understand correctly, we moved from a 8 bytes alignment in struct
> > can(fd)_frame to a 4 bytes alignment in struct canxl_frame.
>
> Yes. I hassled with the alignment too.
>
> The big cool thing about the 64 bit alignment was the filter and
> modification efficiency in bcm.c and gw.c
>
> I wonder if this is still a relevant use case with CAN XL.
>
> Currently the SDU type SDT=0x03 defines a Classical CAN and CAN FD
> 'tunneling' for CAN XL (in CiA 611-1 document).
>
> For this SDT=0x03 the CAN ID (and EFF/RTR/FD flags) are placed in the AF
> element.
>
> And then the first data[0] byte will contain ESI/BRS/DLC and starting
> with data[1] the CAN frame data content will start.
>
> So at least this spec will horribly break and 64 bit access to CAN data
> content.
>
> I've been thinking about creating a 'normal' Classical CAN / CAN FD
> virtual CAN interface that feels for the user like a standard CAN
> interface with struct can[fd]_frame - but inside interacts with CAN XL
> frames with SDT=0x03 ...

Here, you lost me. The only reference document I have is the Bosch
presentation you linked in the cover letter. I would need to get a
copy of the specification first to follow up on this level of details.

> Don't know if users really will need such stuff with CAN XL as there are
> other PDU tunneling mechanics already specified.
>
> For that reason I would not take the 64 bit alignment as a strong
> requirement. With the current struct canxl_frame layout the data[] will
> start at a 32 bit boundary.

ACK. The 32 bit alignment is totally acceptable I think. In the worst
case, on 64 bits architecture, when the payload is a perfect multiple
of 64 bits, we might lose a couple of assembly instructions but I
think that would be acceptable.

> At the end I would see CAN XL as some Ethernet implementation with a
> cool arbitration concept from CAN that assures CSMA/C[AR] instead of
> CSMA/CD ;-)
>
> Best regards,
> Oliver
>
> >
> >>
> >> +};
> >> +
> >>   #define CAN_MTU                (sizeof(struct can_frame))
> >>   #define CANFD_MTU      (sizeof(struct canfd_frame))
> >> +#define CANXL_MTU      (sizeof(struct canxl_frame))
> >
> > #define CANXL_MTU      (sizeof(struct canxl_frame) + CANXL_MAX_DLEN)
> >
> >>   /* particular protocols of the protocol family PF_CAN */
> >>   #define CAN_RAW                1 /* RAW sockets */
> >>   #define CAN_BCM                2 /* Broadcast Manager */
> >>   #define CAN_TP16       3 /* VAG Transport Protocol v1.6 */
> >
> >
> > Yours sincerely,
> > Vincent Mailhol

Re: [RFC PATCH 1/5] can: canxl: introduce CAN XL data structure

[Oliver Hartkopp]

On 12.07.22 10:40, Vincent Mailhol wrote:
> On Tue. 12 juil. 2022 at 16:55, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> On 12.07.22 02:36, Vincent Mailhol wrote:
>>> On Tue. 12 Jul. 2022 at 03:44, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>>>
>>>> This patch adds defines for data structures and length information for
>>>> CAN XL (CAN with eXtended data Length) which can transfer up to 2048
>>>> byte insinde a single frame.
>>>>
>>>> Notable changes from CAN FD:
>>>>
>>>> - the 11 bit arbitration field is now named 'priority' instead of 'can_id'
>>>>     (there are no 29 bit identifiers nor RTR frames anymore)
>>>> - the data length needs a uint16 value to cover up to 2048 byte
>>>>     (the length element position is different to struct can[fd]_frame)
>>>> - new fields (SDT, AF) and a SEC bit have been introduced
>>>> - the virtual CAN interface identifier is not part if the CAN XL frame
>>>>     struct as this VCID value is stored in struct skbuff (analog to vlan id)
>>>>
>>>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>>>> ---
>>>>    include/uapi/linux/can.h | 38 ++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 38 insertions(+)
>>>>
>>>> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
>>>> index 90801ada2bbe..9f97a5d06f3b 100644
>>>> --- a/include/uapi/linux/can.h
>>>> +++ b/include/uapi/linux/can.h
>>>> @@ -58,10 +58,11 @@
>>>>
>>>>    /* valid bits in CAN ID for frame formats */
>>>>    #define CAN_SFF_MASK 0x000007FFU /* standard frame format (SFF) */
>>>>    #define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) */
>>>>    #define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */
>>>> +#define CANXL_PRIO_MASK CAN_SFF_MASK /* 11 bit priority mask */
>>>>
>>>>    /*
>>>>     * Controller Area Network Identifier structure
>>>>     *
>>>>     * bit 0-28    : CAN identifier (11/29 bit)
>>>> @@ -71,10 +72,11 @@
>>>>     */
>>>>    typedef __u32 canid_t;
>>>>
>>>>    #define CAN_SFF_ID_BITS                11
>>>>    #define CAN_EFF_ID_BITS                29
>>>> +#define CANXL_PRIO_BITS                CAN_SFF_ID_BITS
>>>>
>>>>    /*
>>>>     * Controller Area Network Error Message Frame Mask structure
>>>>     *
>>>>     * bit 0-28    : error class mask (see include/uapi/linux/can/error.h)
>>>> @@ -89,10 +91,18 @@ typedef __u32 can_err_mask_t;
>>>>
>>>>    /* CAN FD payload length and DLC definitions according to ISO 11898-7 */
>>>>    #define CANFD_MAX_DLC 15
>>>>    #define CANFD_MAX_DLEN 64
>>>>
>>>> +/*
>>>> + * CAN XL payload length and DLC definitions according to ISO 11898-1
>>>> + * CAN XL DLC ranges from 0 .. 2047 => data length from 1 .. 2048 byte
>>>> + */
>>>> +#define CANXL_MAX_DLC 2047
>>>> +#define CANXL_MAX_DLC_MASK 0x07FF
>>>> +#define CANXL_MAX_DLEN 2048
>>>> +
>>>>    /**
>>>>     * struct can_frame - Classical CAN frame structure (aka CAN 2.0B)
>>>>     * @can_id:   CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
>>>>     * @len:      CAN frame payload length in byte (0 .. 8)
>>>>     * @can_dlc:  deprecated name for CAN frame payload length in byte (0 .. 8)
>>>> @@ -141,14 +151,20 @@ struct can_frame {
>>>>     * When this is done the former differentiation via CAN_MTU / CANFD_MTU gets
>>>>     * lost. CANFD_FDF allows programmers to mark CAN FD frames in the case of
>>>>     * using struct canfd_frame for mixed CAN / CAN FD content (dual use).
>>>>     * N.B. the Kernel APIs do NOT provide mixed CAN / CAN FD content inside of
>>>>     * struct canfd_frame therefore the CANFD_FDF flag is disregarded by Linux.
>>>> + * Same applies to the CANXL_XLF bit.
>>>> + *
>>>> + * For CAN XL the SEC bit has been added to the flags field which shares the
>>>> + * same position in struct can[fd|xl]_frame.
>>>>     */
>>>>    #define CANFD_BRS 0x01 /* bit rate switch (second bitrate for payload data) */
>>>>    #define CANFD_ESI 0x02 /* error state indicator of the transmitting node */
>>>>    #define CANFD_FDF 0x04 /* mark CAN FD for dual use of struct canfd_frame */
>>>> +#define CANXL_XLF 0x08 /* mark CAN XL for dual use of struct canfd_frame */
>>>> +#define CANXL_SEC 0x10 /* Simple Extended Content (security/segmentation) */
>>>>
>>>>    /**
>>>>     * struct canfd_frame - CAN flexible data rate frame structure
>>>>     * @can_id: CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
>>>>     * @len:    frame payload length in byte (0 .. CANFD_MAX_DLEN)
>>>> @@ -164,12 +180,34 @@ struct canfd_frame {
>>>>           __u8    __res0;  /* reserved / padding */
>>>>           __u8    __res1;  /* reserved / padding */
>>>>           __u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
>>>>    };
>>>>
>>>> +/**
>>>> + * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
>>>> + * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags
>>>> + * @sdt:   SDU (service data unit) type
>>>> + * @flags: additional flags for CAN XL
>>>> + * @len:   frame payload length in byte (1 .. CANXL_MAX_DLEN)
>>>> + * @af:    acceptance field
>>>> + * @data:  CAN XL frame payload (up to CANXL_MAX_DLEN byte)
>>>> + *
>>>> + * @prio shares the same position as @can_id from struct canfd_frame.
>>>> + * Same applies to the relative position and length of @flags.
>>>> + */
>>>> +struct canxl_frame {
>>>> +       canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
>>>> +       __u8    sdt;   /* SDU (service data unit) type */
>>>> +       __u8    flags; /* additional flags for CAN XL */
>>>> +       __u16   len;   /* frame payload length in byte */
>>>> +       __u32   af;    /* acceptance field */
>>>> +       __u8    data[CANXL_MAX_DLEN];
>>>
>>> __u8 data[];
>>>
>>> 2 kilobytes start to be a significant size. Would it make sense to use
>>> a flexible array member to minimize the copies? A bit like the TCP/IP
>>> stack where you do not have to allocate the MTU but just what is
>>> actually needed for the headers and your payload.
>>>
>>> Of course this is a tradeoff. It will add some complexity. The first
>>> impact is that the skb's data length might be smaller than the MTU and
>>> thus any logic using the MTU to differentiate between Classic CAN,
>>> CAN-FD or CAN XL would have to be adjusted.
>>
>> Yes, I've thought about that myself but I wanted a simple start for our
>> discussion to think about improvements in the team.
>>
>> I implemented this first:
>>
>>    /* Drop a given socketbuffer if it does not contain a valid CAN frame. */
>>    bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
>>    {
>> -       const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>> +       unsigned int len = can_get_data_len(skb);
> 
> It is premature to use can_get_data_len() here. You have not yet
> confirmed the skb’s length so this could be an out of band read.
> 
>>           struct can_priv *priv = netdev_priv(dev);
>>
>>           if (skb->protocol == htons(ETH_P_CAN)) {
>>                   if (unlikely(skb->len != CAN_MTU ||
>> -                            cfd->len > CAN_MAX_DLEN))
>> +                            len > CAN_MAX_DLEN))
>>                           goto inval_skb;
>>           } else if (skb->protocol == htons(ETH_P_CANFD)) {
>>                   if (unlikely(skb->len != CANFD_MTU ||
>> -                            cfd->len > CANFD_MAX_DLEN))
>> +                            len > CANFD_MAX_DLEN))
>> +                       goto inval_skb;
>> +       } else if (skb->protocol == htons(ETH_P_CANXL)) {
>> +               if (unlikely(skb->len < CANXL_MINTU ||
>> +                            skb->len > CANXL_MTU ||
>> +                            len > CANXL_MAX_DLEN || len == 0))
>>                           goto inval_skb;
>>           } else {
>>                   goto inval_skb;
>>           }
> 
> I suggest this:
> 
>    /* Drop a given socket buffer if it does not contain a valid CAN frame. */
>    bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
>    {
>           const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> +        const struct canxl_frame *cxl = (struct canxl_frame *)skb->data;
>           struct can_priv *priv = netdev_priv(dev);
> 
>           if (skb->protocol == htons(ETH_P_CAN)) {
>                   if (unlikely(skb->len != CAN_MTU ||
>                                cfd->len > CAN_MAX_DLEN))
>                           goto inval_skb;
>           } else if (skb->protocol == htons(ETH_P_CANFD)) {
>                   if (unlikely(skb->len != CANFD_MTU ||
>                                cfd->len > CANFD_MAX_DLEN))
>                           goto inval_skb;
> +       } else if (skb->protocol == htons(ETH_P_CANXL)) {
> +               if (unlikely(skb->len < sizeof(struct canxl_frame) ||
> +                            skb->len > CANXL_MTU ||
> +                            cxl->len > CANXL_MAX_DLEN || cxl->len == 0))
> +                        goto inval_skb;
>           } else {
>                   goto inval_skb;
>           }
> 
> Once can_dropped_invalid_skb() succeeds, calls to can_get_data_len()
> will be safe.

Agreed. Will change that.

> 
>> +/* truncated CAN XL structs must contain at least 64 data bytes */
>> +#define CANXL_MINTU    (CANXL_MTU - CANXL_MAX_DLEN + CANFD_MAX_DLEN)
> 
> I did not get the concept of the "truncated CAN XL structs". The valid
> data field lengths are 1 to 2048, right? I did not get where this 64
> comes from.
> Your formula is equivalent to
> #define CANXL_MINTU    (sizeof(struct canxl_frame) + CANFD_MAX_DLEN)

No. CANXL_MINTU becomes sizeof(struct canfd_frame) + sizeof(af)

So I wanted some size value that is 'more than' CANFD_MTU so that you 
know that you have read a CANXL frame.

Even if the cxf->len would be 14 you would at least copy a struct 
canxl_frame with data[64].

> 
> I would have just expected:
> #define CANXL_MINTU    (sizeof(struct canxl_frame))

That is CANXL_MTU (max transfer unit).

> Or maybe no macro at all, the sizeof is more explicit to me.
> 
>> So the idea was to define a CAN XL skb->len which is clearly above
>> CANFD_MTU to distinguish it from the other CAN MTUs.
>>
>> But as the skbuff is zerocopy inside the kernel,
> 
> Inside the kernel yes, but there is still one copy between userspace
> and kernel land with the full width.

Yes. I hope the explanation above made it clearer now.

> 
>> it probably makes sense
>> to stay with the full CANXL_MTU inside the kernel and allow to crop the
>> data structure for CAN_RAW socket interactions from/to user space down
>> to CANXL_MINTU ?!?
> 
> My guts would tell me to crop it from the initial malloc in userland.
> Not sure what would be the impact in terms of performances.
> 
>>> Also, are we fine to drop the __attribute__((aligned(8)))? If I
>>> understand correctly, we moved from a 8 bytes alignment in struct
>>> can(fd)_frame to a 4 bytes alignment in struct canxl_frame.
>>
>> Yes. I hassled with the alignment too.
>>
>> The big cool thing about the 64 bit alignment was the filter and
>> modification efficiency in bcm.c and gw.c
>>
>> I wonder if this is still a relevant use case with CAN XL.
>>
>> Currently the SDU type SDT=0x03 defines a Classical CAN and CAN FD
>> 'tunneling' for CAN XL (in CiA 611-1 document).
>>
>> For this SDT=0x03 the CAN ID (and EFF/RTR/FD flags) are placed in the AF
>> element.
>>
>> And then the first data[0] byte will contain ESI/BRS/DLC and starting
>> with data[1] the CAN frame data content will start.
>>
>> So at least this spec will horribly break and 64 bit access to CAN data
>> content.
>>
>> I've been thinking about creating a 'normal' Classical CAN / CAN FD
>> virtual CAN interface that feels for the user like a standard CAN
>> interface with struct can[fd]_frame - but inside interacts with CAN XL
>> frames with SDT=0x03 ...
> 
> Here, you lost me. The only reference document I have is the Bosch
> presentation you linked in the cover letter. I would need to get a
> copy of the specification first to follow up on this level of details.

There is a Special Interest Group for CAN XL at CAN in Automation 
(can-cia.org) and these doscuments are currently under development.

With the current approach SDT=3 to 'tunnel' CAN/CANFD frames the aligned 
access to data[] into the struct canxl_frame is at least not possible.

>> Don't know if users really will need such stuff with CAN XL as there are
>> other PDU tunneling mechanics already specified.
>>
>> For that reason I would not take the 64 bit alignment as a strong
>> requirement. With the current struct canxl_frame layout the data[] will
>> start at a 32 bit boundary.
> 
> ACK. The 32 bit alignment is totally acceptable I think. In the worst
> case, on 64 bits architecture, when the payload is a perfect multiple
> of 64 bits, we might lose a couple of assembly instructions but I
> think that would be acceptable.

+1

Best,
Oliver

> 
>> At the end I would see CAN XL as some Ethernet implementation with a
>> cool arbitration concept from CAN that assures CSMA/C[AR] instead of
>> CSMA/CD ;-)
>>
>> Best regards,
>> Oliver
>>
>>>
>>>>
>>>> +};
>>>> +
>>>>    #define CAN_MTU                (sizeof(struct can_frame))
>>>>    #define CANFD_MTU      (sizeof(struct canfd_frame))
>>>> +#define CANXL_MTU      (sizeof(struct canxl_frame))
>>>
>>> #define CANXL_MTU      (sizeof(struct canxl_frame) + CANXL_MAX_DLEN)
>>>
>>>>    /* particular protocols of the protocol family PF_CAN */
>>>>    #define CAN_RAW                1 /* RAW sockets */
>>>>    #define CAN_BCM                2 /* Broadcast Manager */
>>>>    #define CAN_TP16       3 /* VAG Transport Protocol v1.6 */
>>>
>>>
>>> Yours sincerely,
>>> Vincent Mailhol

Re: [RFC PATCH 1/5] can: canxl: introduce CAN XL data structure

[Vincent Mailhol]

On Tue. 12 Jul. 2022 at 18:31, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 12.07.22 10:40, Vincent Mailhol wrote:
> > On Tue. 12 juil. 2022 at 16:55, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >> On 12.07.22 02:36, Vincent Mailhol wrote:
> >>> On Tue. 12 Jul. 2022 at 03:44, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >>>>
> >>>> This patch adds defines for data structures and length information for
> >>>> CAN XL (CAN with eXtended data Length) which can transfer up to 2048
> >>>> byte insinde a single frame.
> >>>>
> >>>> Notable changes from CAN FD:
> >>>>
> >>>> - the 11 bit arbitration field is now named 'priority' instead of 'can_id'
> >>>>     (there are no 29 bit identifiers nor RTR frames anymore)
> >>>> - the data length needs a uint16 value to cover up to 2048 byte
> >>>>     (the length element position is different to struct can[fd]_frame)
> >>>> - new fields (SDT, AF) and a SEC bit have been introduced
> >>>> - the virtual CAN interface identifier is not part if the CAN XL frame
> >>>>     struct as this VCID value is stored in struct skbuff (analog to vlan id)
> >>>>
> >>>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> >>>> ---
> >>>>    include/uapi/linux/can.h | 38 ++++++++++++++++++++++++++++++++++++++
> >>>>    1 file changed, 38 insertions(+)
> >>>>
> >>>> diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
> >>>> index 90801ada2bbe..9f97a5d06f3b 100644
> >>>> --- a/include/uapi/linux/can.h
> >>>> +++ b/include/uapi/linux/can.h
> >>>> @@ -58,10 +58,11 @@
> >>>>
> >>>>    /* valid bits in CAN ID for frame formats */
> >>>>    #define CAN_SFF_MASK 0x000007FFU /* standard frame format (SFF) */
> >>>>    #define CAN_EFF_MASK 0x1FFFFFFFU /* extended frame format (EFF) */
> >>>>    #define CAN_ERR_MASK 0x1FFFFFFFU /* omit EFF, RTR, ERR flags */
> >>>> +#define CANXL_PRIO_MASK CAN_SFF_MASK /* 11 bit priority mask */
> >>>>
> >>>>    /*
> >>>>     * Controller Area Network Identifier structure
> >>>>     *
> >>>>     * bit 0-28    : CAN identifier (11/29 bit)
> >>>> @@ -71,10 +72,11 @@
> >>>>     */
> >>>>    typedef __u32 canid_t;
> >>>>
> >>>>    #define CAN_SFF_ID_BITS                11
> >>>>    #define CAN_EFF_ID_BITS                29
> >>>> +#define CANXL_PRIO_BITS                CAN_SFF_ID_BITS
> >>>>
> >>>>    /*
> >>>>     * Controller Area Network Error Message Frame Mask structure
> >>>>     *
> >>>>     * bit 0-28    : error class mask (see include/uapi/linux/can/error.h)
> >>>> @@ -89,10 +91,18 @@ typedef __u32 can_err_mask_t;
> >>>>
> >>>>    /* CAN FD payload length and DLC definitions according to ISO 11898-7 */
> >>>>    #define CANFD_MAX_DLC 15
> >>>>    #define CANFD_MAX_DLEN 64
> >>>>
> >>>> +/*
> >>>> + * CAN XL payload length and DLC definitions according to ISO 11898-1
> >>>> + * CAN XL DLC ranges from 0 .. 2047 => data length from 1 .. 2048 byte
> >>>> + */
> >>>> +#define CANXL_MAX_DLC 2047
> >>>> +#define CANXL_MAX_DLC_MASK 0x07FF
> >>>> +#define CANXL_MAX_DLEN 2048
> >>>> +
> >>>>    /**
> >>>>     * struct can_frame - Classical CAN frame structure (aka CAN 2.0B)
> >>>>     * @can_id:   CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
> >>>>     * @len:      CAN frame payload length in byte (0 .. 8)
> >>>>     * @can_dlc:  deprecated name for CAN frame payload length in byte (0 .. 8)
> >>>> @@ -141,14 +151,20 @@ struct can_frame {
> >>>>     * When this is done the former differentiation via CAN_MTU / CANFD_MTU gets
> >>>>     * lost. CANFD_FDF allows programmers to mark CAN FD frames in the case of
> >>>>     * using struct canfd_frame for mixed CAN / CAN FD content (dual use).
> >>>>     * N.B. the Kernel APIs do NOT provide mixed CAN / CAN FD content inside of
> >>>>     * struct canfd_frame therefore the CANFD_FDF flag is disregarded by Linux.
> >>>> + * Same applies to the CANXL_XLF bit.
> >>>> + *
> >>>> + * For CAN XL the SEC bit has been added to the flags field which shares the
> >>>> + * same position in struct can[fd|xl]_frame.
> >>>>     */
> >>>>    #define CANFD_BRS 0x01 /* bit rate switch (second bitrate for payload data) */
> >>>>    #define CANFD_ESI 0x02 /* error state indicator of the transmitting node */
> >>>>    #define CANFD_FDF 0x04 /* mark CAN FD for dual use of struct canfd_frame */
> >>>> +#define CANXL_XLF 0x08 /* mark CAN XL for dual use of struct canfd_frame */
> >>>> +#define CANXL_SEC 0x10 /* Simple Extended Content (security/segmentation) */
> >>>>
> >>>>    /**
> >>>>     * struct canfd_frame - CAN flexible data rate frame structure
> >>>>     * @can_id: CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
> >>>>     * @len:    frame payload length in byte (0 .. CANFD_MAX_DLEN)
> >>>> @@ -164,12 +180,34 @@ struct canfd_frame {
> >>>>           __u8    __res0;  /* reserved / padding */
> >>>>           __u8    __res1;  /* reserved / padding */
> >>>>           __u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
> >>>>    };
> >>>>
> >>>> +/**
> >>>> + * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
> >>>> + * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags
> >>>> + * @sdt:   SDU (service data unit) type
> >>>> + * @flags: additional flags for CAN XL
> >>>> + * @len:   frame payload length in byte (1 .. CANXL_MAX_DLEN)
> >>>> + * @af:    acceptance field
> >>>> + * @data:  CAN XL frame payload (up to CANXL_MAX_DLEN byte)
> >>>> + *
> >>>> + * @prio shares the same position as @can_id from struct canfd_frame.
> >>>> + * Same applies to the relative position and length of @flags.
> >>>> + */
> >>>> +struct canxl_frame {
> >>>> +       canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
> >>>> +       __u8    sdt;   /* SDU (service data unit) type */
> >>>> +       __u8    flags; /* additional flags for CAN XL */
> >>>> +       __u16   len;   /* frame payload length in byte */
> >>>> +       __u32   af;    /* acceptance field */
> >>>> +       __u8    data[CANXL_MAX_DLEN];
> >>>
> >>> __u8 data[];
> >>>
> >>> 2 kilobytes start to be a significant size. Would it make sense to use
> >>> a flexible array member to minimize the copies? A bit like the TCP/IP
> >>> stack where you do not have to allocate the MTU but just what is
> >>> actually needed for the headers and your payload.
> >>>
> >>> Of course this is a tradeoff. It will add some complexity. The first
> >>> impact is that the skb's data length might be smaller than the MTU and
> >>> thus any logic using the MTU to differentiate between Classic CAN,
> >>> CAN-FD or CAN XL would have to be adjusted.
> >>
> >> Yes, I've thought about that myself but I wanted a simple start for our
> >> discussion to think about improvements in the team.
> >>
> >> I implemented this first:
> >>
> >>    /* Drop a given socketbuffer if it does not contain a valid CAN frame. */
> >>    bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
> >>    {
> >> -       const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> >> +       unsigned int len = can_get_data_len(skb);
> >
> > It is premature to use can_get_data_len() here. You have not yet
> > confirmed the skb’s length so this could be an out of band read.
> >
> >>           struct can_priv *priv = netdev_priv(dev);
> >>
> >>           if (skb->protocol == htons(ETH_P_CAN)) {
> >>                   if (unlikely(skb->len != CAN_MTU ||
> >> -                            cfd->len > CAN_MAX_DLEN))
> >> +                            len > CAN_MAX_DLEN))
> >>                           goto inval_skb;
> >>           } else if (skb->protocol == htons(ETH_P_CANFD)) {
> >>                   if (unlikely(skb->len != CANFD_MTU ||
> >> -                            cfd->len > CANFD_MAX_DLEN))
> >> +                            len > CANFD_MAX_DLEN))
> >> +                       goto inval_skb;
> >> +       } else if (skb->protocol == htons(ETH_P_CANXL)) {
> >> +               if (unlikely(skb->len < CANXL_MINTU ||
> >> +                            skb->len > CANXL_MTU ||
> >> +                            len > CANXL_MAX_DLEN || len == 0))
> >>                           goto inval_skb;
> >>           } else {
> >>                   goto inval_skb;
> >>           }
> >
> > I suggest this:
> >
> >    /* Drop a given socket buffer if it does not contain a valid CAN frame. */
> >    bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
> >    {
> >           const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> > +        const struct canxl_frame *cxl = (struct canxl_frame *)skb->data;
> >           struct can_priv *priv = netdev_priv(dev);
> >
> >           if (skb->protocol == htons(ETH_P_CAN)) {
> >                   if (unlikely(skb->len != CAN_MTU ||
> >                                cfd->len > CAN_MAX_DLEN))
> >                           goto inval_skb;
> >           } else if (skb->protocol == htons(ETH_P_CANFD)) {
> >                   if (unlikely(skb->len != CANFD_MTU ||
> >                                cfd->len > CANFD_MAX_DLEN))
> >                           goto inval_skb;
> > +       } else if (skb->protocol == htons(ETH_P_CANXL)) {
> > +               if (unlikely(skb->len < sizeof(struct canxl_frame) ||
> > +                            skb->len > CANXL_MTU ||
> > +                            cxl->len > CANXL_MAX_DLEN || cxl->len == 0))
> > +                        goto inval_skb;
> >           } else {
> >                   goto inval_skb;
> >           }
> >
> > Once can_dropped_invalid_skb() succeeds, calls to can_get_data_len()
> > will be safe.
>
> Agreed. Will change that.
>
> >
> >> +/* truncated CAN XL structs must contain at least 64 data bytes */
> >> +#define CANXL_MINTU    (CANXL_MTU - CANXL_MAX_DLEN + CANFD_MAX_DLEN)
> >
> > I did not get the concept of the "truncated CAN XL structs". The valid
> > data field lengths are 1 to 2048, right? I did not get where this 64
> > comes from.
> > Your formula is equivalent to
> > #define CANXL_MINTU    (sizeof(struct canxl_frame) + CANFD_MAX_DLEN)
>
> No. CANXL_MINTU becomes sizeof(struct canfd_frame) + sizeof(af)
>
> So I wanted some size value that is 'more than' CANFD_MTU so that you
> know that you have read a CANXL frame.
>
> Even if the cxf->len would be 14 you would at least copy a struct
> canxl_frame with data[64].

OK, I finally got your point. Your concern is that if skb->len could
be equal or less than CANFD_MTU, then there would be a collision.

My approach here would be to stop using the MTU correlation to
differentiate between CAN(-FD) and CANXL. Instead, I suggest using
can{fd,xl}_frame::flags. If can{fd,xl}_frame has a CANXL flag set,
then it is a CANXL frame regardless of the value of skb->len. If the
CANXL flag is not set, then skb->len is used to differentiate between
Classic CAN and CAN FD (so that we remain compatible with the
existing). That way, no need to impose a minimum length of
CANFD_MAX_DLEN.

> >
> > I would have just expected:
> > #define CANXL_MINTU    (sizeof(struct canxl_frame))
>
> That is CANXL_MTU (max transfer unit).

I was writing while thinking that canxl_frame::data was a flexible
array member as suggested in this thread.
In that case canxl_frame::data counts as zero when doing sizeof(struct
canxl_frame). And so sizeof(struct canxl_frame) == sizeof(struct
canfd_frame) + sizeof(af).

Actually, thinking twice, the Minimum transfer unit would be:
#define CANXL_MINLEN 1
#define CANXL_MINTU    (sizeof(struct canxl_frame) + CANXL_MINLEN)

(I forgot that the length can not be zero anymore in CANXL...)

> > Or maybe no macro at all, the sizeof is more explicit to me.
> >
> >> So the idea was to define a CAN XL skb->len which is clearly above
> >> CANFD_MTU to distinguish it from the other CAN MTUs.
> >>
> >> But as the skbuff is zerocopy inside the kernel,
> >
> > Inside the kernel yes, but there is still one copy between userspace
> > and kernel land with the full width.
>
> Yes. I hope the explanation above made it clearer now.
>
> >
> >> it probably makes sense
> >> to stay with the full CANXL_MTU inside the kernel and allow to crop the
> >> data structure for CAN_RAW socket interactions from/to user space down
> >> to CANXL_MINTU ?!?
> >
> > My guts would tell me to crop it from the initial malloc in userland.
> > Not sure what would be the impact in terms of performances.
> >
> >>> Also, are we fine to drop the __attribute__((aligned(8)))? If I
> >>> understand correctly, we moved from a 8 bytes alignment in struct
> >>> can(fd)_frame to a 4 bytes alignment in struct canxl_frame.
> >>
> >> Yes. I hassled with the alignment too.
> >>
> >> The big cool thing about the 64 bit alignment was the filter and
> >> modification efficiency in bcm.c and gw.c
> >>
> >> I wonder if this is still a relevant use case with CAN XL.
> >>
> >> Currently the SDU type SDT=0x03 defines a Classical CAN and CAN FD
> >> 'tunneling' for CAN XL (in CiA 611-1 document).
> >>
> >> For this SDT=0x03 the CAN ID (and EFF/RTR/FD flags) are placed in the AF
> >> element.
> >>
> >> And then the first data[0] byte will contain ESI/BRS/DLC and starting
> >> with data[1] the CAN frame data content will start.
> >>
> >> So at least this spec will horribly break and 64 bit access to CAN data
> >> content.
> >>
> >> I've been thinking about creating a 'normal' Classical CAN / CAN FD
> >> virtual CAN interface that feels for the user like a standard CAN
> >> interface with struct can[fd]_frame - but inside interacts with CAN XL
> >> frames with SDT=0x03 ...
> >
> > Here, you lost me. The only reference document I have is the Bosch
> > presentation you linked in the cover letter. I would need to get a
> > copy of the specification first to follow up on this level of details.
>
> There is a Special Interest Group for CAN XL at CAN in Automation
> (can-cia.org) and these doscuments are currently under development.

I wonder how hard it is to join the group. Right now, I was thinking
of waiting for the ISO Final Draft International Standard (FDIS) to
deep dive in CANXL.

> With the current approach SDT=3 to 'tunnel' CAN/CANFD frames the aligned
> access to data[] into the struct canxl_frame is at least not possible.
>
> >> Don't know if users really will need such stuff with CAN XL as there are
> >> other PDU tunneling mechanics already specified.
> >>
> >> For that reason I would not take the 64 bit alignment as a strong
> >> requirement. With the current struct canxl_frame layout the data[] will
> >> start at a 32 bit boundary.
> >
> > ACK. The 32 bit alignment is totally acceptable I think. In the worst
> > case, on 64 bits architecture, when the payload is a perfect multiple
> > of 64 bits, we might lose a couple of assembly instructions but I
> > think that would be acceptable.
>
> +1
>
> Best,
> Oliver
>
> >
> >> At the end I would see CAN XL as some Ethernet implementation with a
> >> cool arbitration concept from CAN that assures CSMA/C[AR] instead of
> >> CSMA/CD ;-)
> >>
> >> Best regards,
> >> Oliver
> >>
> >>>
> >>>>
> >>>> +};
> >>>> +
> >>>>    #define CAN_MTU                (sizeof(struct can_frame))
> >>>>    #define CANFD_MTU      (sizeof(struct canfd_frame))
> >>>> +#define CANXL_MTU      (sizeof(struct canxl_frame))
> >>>
> >>> #define CANXL_MTU      (sizeof(struct canxl_frame) + CANXL_MAX_DLEN)
> >>>
> >>>>    /* particular protocols of the protocol family PF_CAN */
> >>>>    #define CAN_RAW                1 /* RAW sockets */
> >>>>    #define CAN_BCM                2 /* Broadcast Manager */
> >>>>    #define CAN_TP16       3 /* VAG Transport Protocol v1.6 */
> >>>
> >>>
> >>> Yours sincerely,
> >>> Vincent Mailhol

Re: [RFC PATCH 1/5] can: canxl: introduce CAN XL data structure

[Oliver Hartkopp]

On 12.07.22 12:19, Vincent Mailhol wrote:
> On Tue. 12 Jul. 2022 at 18:31, Oliver Hartkopp <socketcan@hartkopp.net> wrote:

>>>> +/* truncated CAN XL structs must contain at least 64 data bytes */
>>>> +#define CANXL_MINTU    (CANXL_MTU - CANXL_MAX_DLEN + CANFD_MAX_DLEN)
>>>
>>> I did not get the concept of the "truncated CAN XL structs". The valid
>>> data field lengths are 1 to 2048, right? I did not get where this 64
>>> comes from.
>>> Your formula is equivalent to
>>> #define CANXL_MINTU    (sizeof(struct canxl_frame) + CANFD_MAX_DLEN)
>>
>> No. CANXL_MINTU becomes sizeof(struct canfd_frame) + sizeof(af)
>>
>> So I wanted some size value that is 'more than' CANFD_MTU so that you
>> know that you have read a CANXL frame.
>>
>> Even if the cxf->len would be 14 you would at least copy a struct
>> canxl_frame with data[64].
> 
> OK, I finally got your point. Your concern is that if skb->len could
> be equal or less than CANFD_MTU, then there would be a collision.
> 
> My approach here would be to stop using the MTU correlation to
> differentiate between CAN(-FD) and CANXL. Instead, I suggest using
> can{fd,xl}_frame::flags. If can{fd,xl}_frame has a CANXL flag set,
> then it is a CANXL frame regardless of the value of skb->len. If the
> CANXL flag is not set, then skb->len is used to differentiate between
> Classic CAN and CAN FD (so that we remain compatible with the
> existing). That way, no need to impose a minimum length of
> CANFD_MAX_DLEN.

Hm, that sounds interesting! I like that as it looks clear and simple.

Need to pick a coffee to think about potential (security) effects ... ;-)

E.g. we would need to keep skb->len and canxl_frame::len in sync now.

> 
>>>
>>> I would have just expected:
>>> #define CANXL_MINTU    (sizeof(struct canxl_frame))
>>
>> That is CANXL_MTU (max transfer unit).
> 
> I was writing while thinking that canxl_frame::data was a flexible
> array member as suggested in this thread.
> In that case canxl_frame::data counts as zero when doing sizeof(struct
> canxl_frame). And so sizeof(struct canxl_frame) == sizeof(struct
> canfd_frame) + sizeof(af).
> 
> Actually, thinking twice, the Minimum transfer unit would be:
> #define CANXL_MINLEN 1
> #define CANXL_MINTU    (sizeof(struct canxl_frame) + CANXL_MINLEN)
> 
> (I forgot that the length can not be zero anymore in CANXL...)

I still would suggest to have the struct canxl_frame contain the 2048 
byte of data (data[CANXL_MAXDLEN]) - as the entire CAN XL frame is 
defined like this in the CAN XL spec. This would be also in common with 
CAN/CANFD.

E.g. when reading into the struct canxl_frame you always have a defined 
data structure which can contain a complete CAN XL frame.

But if you get or send less than that size (when reading/writing) this 
would be ok now (with your idea with CANXL_XLF set).


E.g.

#define CANXL_MINDLEN 1
#define CANXL_MAXDLEN 2048

#define CANXL_MTU sizeof(struct canxl_frame)
#define CANXL_HEAD_SZ (CANXL_MTU - CANXL_MAXDLEN)
#define CANXL_MINTU (CANXL_HEAD_SZ + CANXL_MINDLEN)

(..)

>>> Here, you lost me. The only reference document I have is the Bosch
>>> presentation you linked in the cover letter. I would need to get a
>>> copy of the specification first to follow up on this level of details.
>>
>> There is a Special Interest Group for CAN XL at CAN in Automation
>> (can-cia.org) and these doscuments are currently under development.
> 
> I wonder how hard it is to join the group. Right now, I was thinking
> of waiting for the ISO Final Draft International Standard (FDIS) to
> deep dive in CANXL.

I'm not sure if the SDT definitions will be part of an ISO draft as 
defining the content inside a CAN XL frame is probably not ISO standard 
relevant.

The content definitions are industrial recommendations to increase 
interoperability. You are always free to put into the CAN XL frame 
whatever you want.

E.g. CAN-CiA defined the pinout of the SUB-D9 connectors for CAN (Pin 
2/7) which is not defines in ISO too.

Best regards,
Oliver

Re: [RFC PATCH 1/5] can: canxl: introduce CAN XL data structure

[Vincent Mailhol]

On Tue. 12 Jul. 2022 at 21:30, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 12.07.22 12:19, Vincent Mailhol wrote:
> > On Tue. 12 Jul. 2022 at 18:31, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>
> >>>> +/* truncated CAN XL structs must contain at least 64 data bytes */
> >>>> +#define CANXL_MINTU    (CANXL_MTU - CANXL_MAX_DLEN + CANFD_MAX_DLEN)
> >>>
> >>> I did not get the concept of the "truncated CAN XL structs". The valid
> >>> data field lengths are 1 to 2048, right? I did not get where this 64
> >>> comes from.
> >>> Your formula is equivalent to
> >>> #define CANXL_MINTU    (sizeof(struct canxl_frame) + CANFD_MAX_DLEN)
> >>
> >> No. CANXL_MINTU becomes sizeof(struct canfd_frame) + sizeof(af)
> >>
> >> So I wanted some size value that is 'more than' CANFD_MTU so that you
> >> know that you have read a CANXL frame.
> >>
> >> Even if the cxf->len would be 14 you would at least copy a struct
> >> canxl_frame with data[64].
> >
> > OK, I finally got your point. Your concern is that if skb->len could
> > be equal or less than CANFD_MTU, then there would be a collision.
> >
> > My approach here would be to stop using the MTU correlation to
> > differentiate between CAN(-FD) and CANXL. Instead, I suggest using
> > can{fd,xl}_frame::flags. If can{fd,xl}_frame has a CANXL flag set,
> > then it is a CANXL frame regardless of the value of skb->len. If the
> > CANXL flag is not set, then skb->len is used to differentiate between
> > Classic CAN and CAN FD (so that we remain compatible with the
> > existing). That way, no need to impose a minimum length of
> > CANFD_MAX_DLEN.
>
> Hm, that sounds interesting! I like that as it looks clear and simple.
>
> Need to pick a coffee

Two years ago, it was a beer:
https://lore.kernel.org/linux-can/a9605011-2674-dc73-111c-8ebf724a13ac@hartkopp.net/

Now it is coffee. Seems that you change your drinking habits (just kidding) ;-)

> to think about potential (security) effects ... ;-)

If we require a socket option as you already did (c.f.
CAN_RAW_XL_FRAMES), I do not see a security risk.

If not using the socket option, then a user who activates CAN_XL at
the netlink level and who runs a legacy application in which struct
can_frame is declared on the stack and not initialised would be at
risk. can_frame::__pad could contain garbage data which could be
interpreted as a valid CAN_XL flag. From that point, the garbage
values in can_frame::__res0 and can_frame::len8_dlc would be
interpreted as an CANXL length.

The only requirement is that all applications which will use a mix of
CANXL and non-CANXL frames shall make sure that no garbage values are
present in can_frame::__pad or canfd_frame::flags. Coding guidelines
such as MISRA and good static analyzers should also be able to catch
this.

> E.g. we would need to keep skb->len and canxl_frame::len in sync now.

Not necessarily. The only strong condition is that:
    skb_buff::len + sizeof(struct canxl_frame) < canxl_frame::len

If it is equal, then perfect, we are optimal. If greater, it just
means that there is some garbage data. If the condition is not met, we
just drop the skb of course.

Technically, we could remove canxl_frame::len and use below formula to
derive the data length:
    len = skb_buff::len - sizeof(struct canxl_frame)

In that case, the user must do all the length calculations correctly.
This would be close to how TCP/IP frames are managed. But personally,
I do not recommend removing canxl_frame::len.

Having both skb_buff::len and canxl_frame::len in sync is a design
choice, not a necessity. I am still thinking of the implications and
what is best between allowing garbage or forcing the two lengths to be
in sync.

> >
> >>>
> >>> I would have just expected:
> >>> #define CANXL_MINTU    (sizeof(struct canxl_frame))
> >>
> >> That is CANXL_MTU (max transfer unit).
> >
> > I was writing while thinking that canxl_frame::data was a flexible
> > array member as suggested in this thread.
> > In that case canxl_frame::data counts as zero when doing sizeof(struct
> > canxl_frame). And so sizeof(struct canxl_frame) == sizeof(struct
> > canfd_frame) + sizeof(af).
> >
> > Actually, thinking twice, the Minimum transfer unit would be:
> > #define CANXL_MINLEN 1
> > #define CANXL_MINTU    (sizeof(struct canxl_frame) + CANXL_MINLEN)
> >
> > (I forgot that the length can not be zero anymore in CANXL...)
>
> I still would suggest to have the struct canxl_frame contain the 2048
> byte of data (data[CANXL_MAXDLEN]) - as the entire CAN XL frame is
> defined like this in the CAN XL spec. This would be also in common with
> CAN/CANFD.
>
> E.g. when reading into the struct canxl_frame you always have a defined
> data structure which can contain a complete CAN XL frame.

If we go this way, then I would allow the user to put garbage (i.e.
not having the two lengths in sync).
The rationale would be that we actually already allow such garbage
values for CAN and CAN-FD. Also, this way, the user who has zero clues
about the flexible array member property would simply do:

   struct canxl_frame cxl = { 0 };
    /* ... */
    write(socket, &cxl, sizeof(cxl));

and it would work. The advanced user who understand what is going on
can still do:

   struct canxl_frame cxl = { 0 };
    /* ... */
    write(socket, &cxl, CANXL_HEAD_SZ + cxl.len);

> But if you get or send less than that size (when reading/writing) this
> would be ok now (with your idea with CANXL_XLF set).
>
>
> E.g.
>
> #define CANXL_MINDLEN 1
> #define CANXL_MAXDLEN 2048

To be consistent with CAN_DLEN and CANFD_DLEN names:
#define CANXL_DLEN 2048

> #define CANXL_MTU sizeof(struct canxl_frame)
> #define CANXL_HEAD_SZ (CANXL_MTU - CANXL_MAXDLEN)
> #define CANXL_MINTU (CANXL_HEAD_SZ + CANXL_MINDLEN)

I need to think twice about all that, all the different alternatives
(allow or not garbage, data as flexible member array vs.
data[CANXL_DLEN]). Now it is a bit late, so I will continue to think
about all that tomorrow.
But overall, I like the direction this thread is taking ;-)

> >>> Here, you lost me. The only reference document I have is the Bosch
> >>> presentation you linked in the cover letter. I would need to get a
> >>> copy of the specification first to follow up on this level of details.
> >>
> >> There is a Special Interest Group for CAN XL at CAN in Automation
> >> (can-cia.org) and these doscuments are currently under development.
> >
> > I wonder how hard it is to join the group. Right now, I was thinking
> > of waiting for the ISO Final Draft International Standard (FDIS) to
> > deep dive in CANXL.
>
> I'm not sure if the SDT definitions will be part of an ISO draft as
> defining the content inside a CAN XL frame is probably not ISO standard
> relevant.
>
> The content definitions are industrial recommendations to increase
> interoperability. You are always free to put into the CAN XL frame
> whatever you want.

But then, should this be another protocol? Similar to how ISO-TP is
not part of CAN_RAW but its own protocol? (this is a naive question, I
am out of context here).

> E.g. CAN-CiA defined the pinout of the SUB-D9 connectors for CAN (Pin
> 2/7) which is not defines in ISO too.

ACK.

Unrelated to this thread but I was wondering if you saw that my
comments on the can_get_data_len() helper macro:
https://lore.kernel.org/linux-can/20220711183426.96446-1-socketcan@hartkopp.net/T/#m01645d364e92681e5b889ca5d3c9f501e5d33dc3

Yours sincerely,
Vincent Mailhol

Re: [RFC PATCH 1/5] can: canxl: introduce CAN XL data structure

[Oliver Hartkopp]

On 12.07.22 16:31, Vincent Mailhol wrote:
> On Tue. 12 Jul. 2022 at 21:30, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> On 12.07.22 12:19, Vincent Mailhol wrote:
>>> On Tue. 12 Jul. 2022 at 18:31, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>
>>>>>> +/* truncated CAN XL structs must contain at least 64 data bytes */
>>>>>> +#define CANXL_MINTU    (CANXL_MTU - CANXL_MAX_DLEN + CANFD_MAX_DLEN)
>>>>>
>>>>> I did not get the concept of the "truncated CAN XL structs". The valid
>>>>> data field lengths are 1 to 2048, right? I did not get where this 64
>>>>> comes from.
>>>>> Your formula is equivalent to
>>>>> #define CANXL_MINTU    (sizeof(struct canxl_frame) + CANFD_MAX_DLEN)
>>>>
>>>> No. CANXL_MINTU becomes sizeof(struct canfd_frame) + sizeof(af)
>>>>
>>>> So I wanted some size value that is 'more than' CANFD_MTU so that you
>>>> know that you have read a CANXL frame.
>>>>
>>>> Even if the cxf->len would be 14 you would at least copy a struct
>>>> canxl_frame with data[64].
>>>
>>> OK, I finally got your point. Your concern is that if skb->len could
>>> be equal or less than CANFD_MTU, then there would be a collision.
>>>
>>> My approach here would be to stop using the MTU correlation to
>>> differentiate between CAN(-FD) and CANXL. Instead, I suggest using
>>> can{fd,xl}_frame::flags. If can{fd,xl}_frame has a CANXL flag set,
>>> then it is a CANXL frame regardless of the value of skb->len. If the
>>> CANXL flag is not set, then skb->len is used to differentiate between
>>> Classic CAN and CAN FD (so that we remain compatible with the
>>> existing). That way, no need to impose a minimum length of
>>> CANFD_MAX_DLEN.
>>
>> Hm, that sounds interesting! I like that as it looks clear and simple.
>>
>> Need to pick a coffee
> 
> Two years ago, it was a beer:
> https://lore.kernel.org/linux-can/a9605011-2674-dc73-111c-8ebf724a13ac@hartkopp.net/
> 
> Now it is coffee. Seems that you change your drinking habits (just kidding) ;-)

That depends on the daytime ;-)

In Germany there's a saying:
"Kein Bier vor vier!"
(no beer before 4pm)

But a friend of my dad has a clock in his garage with only '4's on the 
watchface to make this test always return true :-D

>> to think about potential (security) effects ... ;-)
> 
> If we require a socket option as you already did (c.f.
> CAN_RAW_XL_FRAMES), I do not see a security risk.
> 
> If not using the socket option, then a user who activates CAN_XL at
> the netlink level and who runs a legacy application in which struct
> can_frame is declared on the stack and not initialised would be at
> risk. can_frame::__pad could contain garbage data which could be
> interpreted as a valid CAN_XL flag. From that point, the garbage
> values in can_frame::__res0 and can_frame::len8_dlc would be
> interpreted as an CANXL length.
> 
> The only requirement is that all applications which will use a mix of
> CANXL and non-CANXL frames shall make sure that no garbage values are
> present in can_frame::__pad or canfd_frame::flags. Coding guidelines
> such as MISRA and good static analyzers should also be able to catch
> this.

Yes. This was exactly my concern. But we can not assume that user space 
application is friendly or follows any MISRA guidelines.

Following your flexible length with CANXL_XLF idea you may forge a CAN 
XL frame inside a Classical CAN frame. But then we simply need to treat 
it as CAN XL frame any apply the other sanity checks.

>> E.g. we would need to keep skb->len and canxl_frame::len in sync now.
> 
> Not necessarily. The only strong condition is that:
>      skb_buff::len + sizeof(struct canxl_frame) < canxl_frame::len
> 
> If it is equal, then perfect, we are optimal. If greater, it just
> means that there is some garbage data. If the condition is not met, we
> just drop the skb of course.
> 
> Technically, we could remove canxl_frame::len and use below formula to
> derive the data length:
>      len = skb_buff::len - sizeof(struct canxl_frame)
> 
> In that case, the user must do all the length calculations correctly.
> This would be close to how TCP/IP frames are managed. But personally,
> I do not recommend removing canxl_frame::len.
> 
> Having both skb_buff::len and canxl_frame::len in sync is a design
> choice, not a necessity. I am still thinking of the implications and
> what is best between allowing garbage or forcing the two lengths to be
> in sync.

Ok.

(..)

>> I still would suggest to have the struct canxl_frame contain the 2048
>> byte of data (data[CANXL_MAXDLEN]) - as the entire CAN XL frame is
>> defined like this in the CAN XL spec. This would be also in common with
>> CAN/CANFD.
>>
>> E.g. when reading into the struct canxl_frame you always have a defined
>> data structure which can contain a complete CAN XL frame.
> 
> If we go this way, then I would allow the user to put garbage (i.e.
> not having the two lengths in sync).
> The rationale would be that we actually already allow such garbage
> values for CAN and CAN-FD. Also, this way, the user who has zero clues
> about the flexible array member property would simply do:
> 
>     struct canxl_frame cxl = { 0 };
>      /* ... */

(len and CANXL_XLF would have needed to be set here)

>      write(socket, &cxl, sizeof(cxl));
> 
> and it would work. 

Yes.


> The advanced user who understand what is going on
> can still do:
> 
>     struct canxl_frame cxl = { 0 };
>      /* ... */
>      write(socket, &cxl, CANXL_HEAD_SZ + cxl.len);
> 

ACK. I think this is feasible and easy to understand.

>> But if you get or send less than that size (when reading/writing) this
>> would be ok now (with your idea with CANXL_XLF set).
>>
>>
>> E.g.
>>
>> #define CANXL_MINDLEN 1
>> #define CANXL_MAXDLEN 2048
> 
> To be consistent with CAN_DLEN and CANFD_DLEN names:
> #define CANXL_DLEN 2048

You are right about the inconsistency. But it needs to be

#define CANXL_MIN_DLEN 1
#define CANXL_MAX_DLEN 2048

to fit the CAN/CANFD definitions.

>> #define CANXL_MTU sizeof(struct canxl_frame)
>> #define CANXL_HEAD_SZ (CANXL_MTU - CANXL_MAXDLEN)
>> #define CANXL_MINTU (CANXL_HEAD_SZ + CANXL_MINDLEN)
> 
> I need to think twice about all that, all the different alternatives
> (allow or not garbage, data as flexible member array vs.
> data[CANXL_DLEN]). Now it is a bit late, so I will continue to think
> about all that tomorrow.

I would suggest to not allow garbage and have skbuff::len and 
canxl_frame::len in sync.

When a userspace application writes more bytes than needed by 
canxl_frame::len, then that garbage is cropped to the needed size.

When less data is provided than required by canxl_frame::len this could 
lead to an error.

> But overall, I like the direction this thread is taking ;-)

Yep!

(..)

> Unrelated to this thread but I was wondering if you saw that my
> comments on the can_get_data_len() helper macro:
> https://lore.kernel.org/linux-can/20220711183426.96446-1-socketcan@hartkopp.net/T/#m01645d364e92681e5b889ca5d3c9f501e5d33dc3

Will answer on that thread.

Best,
Oliver

Re: [RFC PATCH 2/5] can: canxl: introduce ETH_P_CANXL ethernet protocol handling

[Oliver Hartkopp]

On 12.07.22 03:23, Vincent Mailhol wrote:
> On Tue. 12 Jul. 2022 at 03:44, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> Enable the PF_CAN infrastructure to handle CAN XL frames. A new ethernet
>> protocol type ETH_P_CANXL is defined to tag skbuffs containing the CAN XL
>> frame data structure.
>>
>> As the length information is now a uint16 value for CAN XL a new helper
>> function can_get_data_len() is introduced to retrieve the data length
>> from all types of CAN frames.
>>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>> ---
>>   include/linux/can/skb.h       | 14 ++++++++++
>>   include/uapi/linux/if_ether.h |  1 +
>>   net/can/af_can.c              | 49 +++++++++++++++++++++++++++++------
>>   3 files changed, 56 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
>> index 182749e858b3..d043bc4afd6d 100644
>> --- a/include/linux/can/skb.h
>> +++ b/include/linux/can/skb.h
>> @@ -101,6 +101,20 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
>>   {
>>          /* the CAN specific type of skb is identified by its data length */
>>          return skb->len == CANFD_MTU;
>>   }
>>
>> +/* get data length inside of CAN frame for all frame types */> +static inline unsigned int can_get_data_len(struct sk_buff *skb)
>> +{
>> +       if(skb->len == CANXL_MTU) {
>> +               const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
>> +
>> +               return cfx->len;
>> +       } else {
>> +               const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>> +
>> +               return cfd->len;
>> +       }
>> +}
> 
> What about the RTR frames?
> 
> If there are cases in which we intentionally want the declared length
> and not the actual length, it might be good to have two distinct
> helper functions.

Good idea.

> /* get data length inside of CAN frame for all frame types. For
>   * RTR frames, return zero. */
> static inline unsigned int can_get_actual_len(struct sk_buff *skb)

I would name this one can_get_data_len()

> {
>         const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
>         const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> 
>         if (skb->len == CANXL_MTU)
>                 return cfx->len;
> 
>         /* RTR frames have an actual length of zero */
>         if (skb->len == CAN_MTU && cfd->flags & CAN_RTR_FLAG)
>                 return 0;
> 
>         return cfd->len;
> }
> 
> 
> /* get data length inside of CAN frame for all frame types. For
>   * RTR frames, return requested length. */
> static inline unsigned int can_get_declared_len(struct sk_buff *skb)

I would name this one can_get_len()

> {
>         const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
>         const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> 
>         if (skb->len == CANXL_MTU)
>                 return cfx->len;
> 
>         return cfd->len;
> }
> 

Best regards,
Oliver

Re: [RFC PATCH 2/5] can: canxl: introduce ETH_P_CANXL ethernet protocol handling

[Vincent Mailhol]

On Wed. 13 Jul. 2022 at 05:20, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 12.07.22 03:23, Vincent Mailhol wrote:
> > On Tue. 12 Jul. 2022 at 03:44, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >> Enable the PF_CAN infrastructure to handle CAN XL frames. A new ethernet
> >> protocol type ETH_P_CANXL is defined to tag skbuffs containing the CAN XL
> >> frame data structure.
> >>
> >> As the length information is now a uint16 value for CAN XL a new helper
> >> function can_get_data_len() is introduced to retrieve the data length
> >> from all types of CAN frames.
> >>
> >> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> >> ---
> >>   include/linux/can/skb.h       | 14 ++++++++++
> >>   include/uapi/linux/if_ether.h |  1 +
> >>   net/can/af_can.c              | 49 +++++++++++++++++++++++++++++------
> >>   3 files changed, 56 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> >> index 182749e858b3..d043bc4afd6d 100644
> >> --- a/include/linux/can/skb.h
> >> +++ b/include/linux/can/skb.h
> >> @@ -101,6 +101,20 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
> >>   {
> >>          /* the CAN specific type of skb is identified by its data length */
> >>          return skb->len == CANFD_MTU;
> >>   }
> >>
> >> +/* get data length inside of CAN frame for all frame types */> +static inline unsigned int can_get_data_len(struct sk_buff *skb)
> >> +{
> >> +       if(skb->len == CANXL_MTU) {
> >> +               const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> >> +
> >> +               return cfx->len;
> >> +       } else {
> >> +               const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> >> +
> >> +               return cfd->len;
> >> +       }
> >> +}
> >
> > What about the RTR frames?
> >
> > If there are cases in which we intentionally want the declared length
> > and not the actual length, it might be good to have two distinct
> > helper functions.
>
> Good idea.
>
> > /* get data length inside of CAN frame for all frame types. For
> >   * RTR frames, return zero. */
> > static inline unsigned int can_get_actual_len(struct sk_buff *skb)
>
> I would name this one can_get_data_len()

ACK.

> > {
> >         const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> >         const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> >
> >         if (skb->len == CANXL_MTU)
> >                 return cfx->len;
> >
> >         /* RTR frames have an actual length of zero */
> >         if (skb->len == CAN_MTU && cfd->flags & CAN_RTR_FLAG)
> >                 return 0;
> >
> >         return cfd->len;
> > }
> >
> >
> > /* get data length inside of CAN frame for all frame types. For
> >   * RTR frames, return requested length. */
> > static inline unsigned int can_get_declared_len(struct sk_buff *skb)
>
> I would name this one can_get_len()

I anticipate that most of the time, developers do not want to get the
RTR length but the actual length (e.g. to memcpy data[] or to increase
statistics). People will get confused between can_get_data_len() and
can_get_len() due to the similar names. So I would suggest a more
explicit name to point out that this one is probably not the one you
want to use.
Candidates name I can think of:
  * can_get_raw_len()
  * can_get_advertised_len()
 * can_get_rtr_len()

The only time you want to access the raw len (with real RTR value) is
in the TX path when you fill your device's structures. But here the
can_get_cc_dlc() is a better helper function which is already RTR
aware.

> > {
> >         const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> >         const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> >
> >         if (skb->len == CANXL_MTU)
> >                 return cfx->len;
> >
> >         return cfd->len;
> > }

Yours sincerely,
Vincent Mailhol

Re: [RFC PATCH 1/5] can: canxl: introduce CAN XL data structure

[Vincent Mailhol]

On Wed. 13 Jul. 2022 at 04:24, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 12.07.22 16:31, Vincent Mailhol wrote:
> > On Tue. 12 Jul. 2022 at 21:30, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >> On 12.07.22 12:19, Vincent Mailhol wrote:
> >>> On Tue. 12 Jul. 2022 at 18:31, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >>
> >>>>>> +/* truncated CAN XL structs must contain at least 64 data bytes */
> >>>>>> +#define CANXL_MINTU    (CANXL_MTU - CANXL_MAX_DLEN + CANFD_MAX_DLEN)
> >>>>>
> >>>>> I did not get the concept of the "truncated CAN XL structs". The valid
> >>>>> data field lengths are 1 to 2048, right? I did not get where this 64
> >>>>> comes from.
> >>>>> Your formula is equivalent to
> >>>>> #define CANXL_MINTU    (sizeof(struct canxl_frame) + CANFD_MAX_DLEN)
> >>>>
> >>>> No. CANXL_MINTU becomes sizeof(struct canfd_frame) + sizeof(af)
> >>>>
> >>>> So I wanted some size value that is 'more than' CANFD_MTU so that you
> >>>> know that you have read a CANXL frame.
> >>>>
> >>>> Even if the cxf->len would be 14 you would at least copy a struct
> >>>> canxl_frame with data[64].
> >>>
> >>> OK, I finally got your point. Your concern is that if skb->len could
> >>> be equal or less than CANFD_MTU, then there would be a collision.
> >>>
> >>> My approach here would be to stop using the MTU correlation to
> >>> differentiate between CAN(-FD) and CANXL. Instead, I suggest using
> >>> can{fd,xl}_frame::flags. If can{fd,xl}_frame has a CANXL flag set,
> >>> then it is a CANXL frame regardless of the value of skb->len. If the
> >>> CANXL flag is not set, then skb->len is used to differentiate between
> >>> Classic CAN and CAN FD (so that we remain compatible with the
> >>> existing). That way, no need to impose a minimum length of
> >>> CANFD_MAX_DLEN.
> >>
> >> Hm, that sounds interesting! I like that as it looks clear and simple.
> >>
> >> Need to pick a coffee
> >
> > Two years ago, it was a beer:
> > https://lore.kernel.org/linux-can/a9605011-2674-dc73-111c-8ebf724a13ac@hartkopp.net/
> >
> > Now it is coffee. Seems that you change your drinking habits (just kidding) ;-)
>
> That depends on the daytime ;-)
>
> In Germany there's a saying:
> "Kein Bier vor vier!"
> (no beer before 4pm)
>
> But a friend of my dad has a clock in his garage with only '4's on the
> watchface to make this test always return true :-D

He, he, he! I had an argument with a German friend a long time ago. We
concluded that I was always past 4 p.m. somewhere in the world. This
works especially great for imported beers which you can drink at the
foreign timezone (to pay homage of course!)

> >> to think about potential (security) effects ... ;-)
> >
> > If we require a socket option as you already did (c.f.
> > CAN_RAW_XL_FRAMES), I do not see a security risk.
> >
> > If not using the socket option, then a user who activates CAN_XL at
> > the netlink level and who runs a legacy application in which struct
> > can_frame is declared on the stack and not initialised would be at
> > risk. can_frame::__pad could contain garbage data which could be
> > interpreted as a valid CAN_XL flag. From that point, the garbage
> > values in can_frame::__res0 and can_frame::len8_dlc would be
> > interpreted as an CANXL length.
> >
> > The only requirement is that all applications which will use a mix of
> > CANXL and non-CANXL frames shall make sure that no garbage values are
> > present in can_frame::__pad or canfd_frame::flags. Coding guidelines
> > such as MISRA and good static analyzers should also be able to catch
> > this.
>
> Yes. This was exactly my concern. But we can not assume that user space
> application is friendly or follows any MISRA guidelines.

Well, if we document than can_frame::__pad shall be zero for mix
usages (i.e. comments in struct can_frame and updated kernel doc),
then we would have done our due diligence. From that point, if people
ignore the documentation *and* do not follow best practices for safety
application development, I wouldn't cry.

If we absolutely want to prevent struct can_frame to be interpreted as
a canxl_frame due to some stack garbage, we can add one
padding/reserved field like that:

struct canxl_frame {
        canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
        __u8    sdt;   /* SDU (service data unit) type */
        __u8    flags; /* additional flags for CAN XL */
        __u16   len;   /* frame payload length in byte */
        __u32   af;    /* acceptance field */
        __u32 __res; /* reserved field. Shall be zero */
        __u8    data[] __attribute__((aligned(8)));
};

This way, the minimum transfer unit of CANXL is 17 bytes (16 for
header and 1 for data) which is exactly one byte more than can_frame
(and we get back the 8 bytes alignment \o/)

This would only leave the risk of having some garbage in
canfd_frame::flags, e.g. if user does:

        struct canfd_frame cfd; /* declared on the stack and not initialized */
        cfd.flags |= <some_flags> /* use |= instead of = */

But this is already risky for plain CAN-FD.

> Following your flexible length with CANXL_XLF idea you may forge a CAN
> XL frame inside a Classical CAN frame. But then we simply need to treat
> it as CAN XL frame any apply the other sanity checks.
>
> >> E.g. we would need to keep skb->len and canxl_frame::len in sync now.
> >
> > Not necessarily. The only strong condition is that:
> >      skb_buff::len + sizeof(struct canxl_frame) < canxl_frame::len
> >
> > If it is equal, then perfect, we are optimal. If greater, it just
> > means that there is some garbage data. If the condition is not met, we
> > just drop the skb of course.
> >
> > Technically, we could remove canxl_frame::len and use below formula to
> > derive the data length:
> >      len = skb_buff::len - sizeof(struct canxl_frame)
> >
> > In that case, the user must do all the length calculations correctly.
> > This would be close to how TCP/IP frames are managed. But personally,
> > I do not recommend removing canxl_frame::len.
> >
> > Having both skb_buff::len and canxl_frame::len in sync is a design
> > choice, not a necessity. I am still thinking of the implications and
> > what is best between allowing garbage or forcing the two lengths to be
> > in sync.
>
> Ok.
>
> (..)
>
> >> I still would suggest to have the struct canxl_frame contain the 2048
> >> byte of data (data[CANXL_MAXDLEN]) - as the entire CAN XL frame is
> >> defined like this in the CAN XL spec. This would be also in common with
> >> CAN/CANFD.
> >>
> >> E.g. when reading into the struct canxl_frame you always have a defined
> >> data structure which can contain a complete CAN XL frame.
> >
> > If we go this way, then I would allow the user to put garbage (i.e.
> > not having the two lengths in sync).
> > The rationale would be that we actually already allow such garbage
> > values for CAN and CAN-FD. Also, this way, the user who has zero clues
> > about the flexible array member property would simply do:
> >
> >     struct canxl_frame cxl = { 0 };
> >      /* ... */
>
> (len and CANXL_XLF would have needed to be set here)
ACK.
> >      write(socket, &cxl, sizeof(cxl));
> >
> > and it would work.
>
> Yes.
>
>
> > The advanced user who understand what is going on
> > can still do:
> >
> >     struct canxl_frame cxl = { 0 };
> >      /* ... */
> >      write(socket, &cxl, CANXL_HEAD_SZ + cxl.len);
> >
>
> ACK. I think this is feasible and easy to understand.
>
> >> But if you get or send less than that size (when reading/writing) this
> >> would be ok now (with your idea with CANXL_XLF set).
> >>
> >>
> >> E.g.
> >>
> >> #define CANXL_MINDLEN 1
> >> #define CANXL_MAXDLEN 2048
> >
> > To be consistent with CAN_DLEN and CANFD_DLEN names:
> > #define CANXL_DLEN 2048
>
> You are right about the inconsistency. But it needs to be
>
> #define CANXL_MIN_DLEN 1
> #define CANXL_MAX_DLEN 2048
>
> to fit the CAN/CANFD definitions.

ACK.

> >> #define CANXL_MTU sizeof(struct canxl_frame)
> >> #define CANXL_HEAD_SZ (CANXL_MTU - CANXL_MAXDLEN)
> >> #define CANXL_MINTU (CANXL_HEAD_SZ + CANXL_MINDLEN)
> >
> > I need to think twice about all that, all the different alternatives
> > (allow or not garbage, data as flexible member array vs.
> > data[CANXL_DLEN]). Now it is a bit late, so I will continue to think
> > about all that tomorrow.
>
> I would suggest to not allow garbage and have skbuff::len and
> canxl_frame::len in sync.
>
> When a userspace application writes more bytes than needed by
> canxl_frame::len, then that garbage is cropped to the needed size.

Cropped by whom? My point is to allow userspace to leave garbage. I am
totally OK with cropping the frames as soon as they enter the kernel.
On the other hand, the kernel shall always return cropped frames to
the userland. To summarize, skb and data len synchronisation is
optional for userland but mandatory for kernel drivers.

> When less data is provided than required by canxl_frame::len this could
> lead to an error.
>
> > But overall, I like the direction this thread is taking ;-)
>
> Yep!
>
> (..)

Yours sincerely,
Vincent Mailhol

Re: [RFC PATCH 2/5] can: canxl: introduce ETH_P_CANXL ethernet protocol handling

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4475 bytes --]

On 13.07.2022 08:58:26, Vincent Mailhol wrote:
> On Wed. 13 Jul. 2022 at 05:20, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> > On 12.07.22 03:23, Vincent Mailhol wrote:
> > > On Tue. 12 Jul. 2022 at 03:44, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> > >> Enable the PF_CAN infrastructure to handle CAN XL frames. A new ethernet
> > >> protocol type ETH_P_CANXL is defined to tag skbuffs containing the CAN XL
> > >> frame data structure.
> > >>
> > >> As the length information is now a uint16 value for CAN XL a new helper
> > >> function can_get_data_len() is introduced to retrieve the data length
> > >> from all types of CAN frames.
> > >>
> > >> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> > >> ---
> > >>   include/linux/can/skb.h       | 14 ++++++++++
> > >>   include/uapi/linux/if_ether.h |  1 +
> > >>   net/can/af_can.c              | 49 +++++++++++++++++++++++++++++------
> > >>   3 files changed, 56 insertions(+), 8 deletions(-)
> > >>
> > >> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> > >> index 182749e858b3..d043bc4afd6d 100644
> > >> --- a/include/linux/can/skb.h
> > >> +++ b/include/linux/can/skb.h
> > >> @@ -101,6 +101,20 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
> > >>   {
> > >>          /* the CAN specific type of skb is identified by its data length */
> > >>          return skb->len == CANFD_MTU;
> > >>   }
> > >>
> > >> +/* get data length inside of CAN frame for all frame types */> +static inline unsigned int can_get_data_len(struct sk_buff *skb)
> > >> +{
> > >> +       if(skb->len == CANXL_MTU) {
> > >> +               const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> > >> +
> > >> +               return cfx->len;
> > >> +       } else {
> > >> +               const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> > >> +
> > >> +               return cfd->len;
> > >> +       }
> > >> +}
> > >
> > > What about the RTR frames?
> > >
> > > If there are cases in which we intentionally want the declared length
> > > and not the actual length, it might be good to have two distinct
> > > helper functions.
> >
> > Good idea.
> >
> > > /* get data length inside of CAN frame for all frame types. For
> > >   * RTR frames, return zero. */
> > > static inline unsigned int can_get_actual_len(struct sk_buff *skb)
> >
> > I would name this one can_get_data_len()
> 
> ACK.
> 
> > > {
> > >         const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> > >         const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> > >
> > >         if (skb->len == CANXL_MTU)
> > >                 return cfx->len;
> > >
> > >         /* RTR frames have an actual length of zero */
> > >         if (skb->len == CAN_MTU && cfd->flags & CAN_RTR_FLAG)
> > >                 return 0;
> > >
> > >         return cfd->len;
> > > }
> > >
> > >
> > > /* get data length inside of CAN frame for all frame types. For
> > >   * RTR frames, return requested length. */
> > > static inline unsigned int can_get_declared_len(struct sk_buff *skb)
> >
> > I would name this one can_get_len()
> 
> I anticipate that most of the time, developers do not want to get the
> RTR length but the actual length (e.g. to memcpy data[] or to increase
> statistics). People will get confused between can_get_data_len() and
> can_get_len() due to the similar names. So I would suggest a more
> explicit name to point out that this one is probably not the one you
> want to use.
> Candidates name I can think of:
>  * can_get_raw_len()
>  * can_get_advertised_len()
>  * can_get_rtr_len()
> 
> The only time you want to access the raw len (with real RTR value) is
> in the TX path when you fill your device's structures. But here the
> can_get_cc_dlc() is a better helper function which is already RTR
> aware.

There also is

| unsigned int can_skb_get_frame_len(const struct sk_buff *skb)

It gives the length of the frame on the wire in bytes. We should use a
common naming scheme. Let's always use can_skb_ as a prefix or drop the
skb_ from this function.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 2/5] can: canxl: introduce ETH_P_CANXL ethernet protocol handling

[Vincent Mailhol]

On Wed. 13 Jul. 2022 at 16:02, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 13.07.2022 08:58:26, Vincent Mailhol wrote:
> > On Wed. 13 Jul. 2022 at 05:20, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> > > On 12.07.22 03:23, Vincent Mailhol wrote:
> > > > On Tue. 12 Jul. 2022 at 03:44, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> > > >> Enable the PF_CAN infrastructure to handle CAN XL frames. A new ethernet
> > > >> protocol type ETH_P_CANXL is defined to tag skbuffs containing the CAN XL
> > > >> frame data structure.
> > > >>
> > > >> As the length information is now a uint16 value for CAN XL a new helper
> > > >> function can_get_data_len() is introduced to retrieve the data length
> > > >> from all types of CAN frames.
> > > >>
> > > >> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> > > >> ---
> > > >>   include/linux/can/skb.h       | 14 ++++++++++
> > > >>   include/uapi/linux/if_ether.h |  1 +
> > > >>   net/can/af_can.c              | 49 +++++++++++++++++++++++++++++------
> > > >>   3 files changed, 56 insertions(+), 8 deletions(-)
> > > >>
> > > >> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> > > >> index 182749e858b3..d043bc4afd6d 100644
> > > >> --- a/include/linux/can/skb.h
> > > >> +++ b/include/linux/can/skb.h
> > > >> @@ -101,6 +101,20 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
> > > >>   {
> > > >>          /* the CAN specific type of skb is identified by its data length */
> > > >>          return skb->len == CANFD_MTU;
> > > >>   }
> > > >>
> > > >> +/* get data length inside of CAN frame for all frame types */> +static inline unsigned int can_get_data_len(struct sk_buff *skb)
> > > >> +{
> > > >> +       if(skb->len == CANXL_MTU) {
> > > >> +               const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> > > >> +
> > > >> +               return cfx->len;
> > > >> +       } else {
> > > >> +               const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> > > >> +
> > > >> +               return cfd->len;
> > > >> +       }
> > > >> +}
> > > >
> > > > What about the RTR frames?
> > > >
> > > > If there are cases in which we intentionally want the declared length
> > > > and not the actual length, it might be good to have two distinct
> > > > helper functions.
> > >
> > > Good idea.
> > >
> > > > /* get data length inside of CAN frame for all frame types. For
> > > >   * RTR frames, return zero. */
> > > > static inline unsigned int can_get_actual_len(struct sk_buff *skb)
> > >
> > > I would name this one can_get_data_len()
> >
> > ACK.

So, according to Marc's comments (c.f. below):
can_skb_get_data_len()

> > > > {
> > > >         const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> > > >         const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> > > >
> > > >         if (skb->len == CANXL_MTU)
> > > >                 return cfx->len;
> > > >
> > > >         /* RTR frames have an actual length of zero */
> > > >         if (skb->len == CAN_MTU && cfd->flags & CAN_RTR_FLAG)
> > > >                 return 0;
> > > >
> > > >         return cfd->len;
> > > > }
> > > >
> > > >
> > > > /* get data length inside of CAN frame for all frame types. For
> > > >   * RTR frames, return requested length. */
> > > > static inline unsigned int can_get_declared_len(struct sk_buff *skb)
> > >
> > > I would name this one can_get_len()
> >
> > I anticipate that most of the time, developers do not want to get the
> > RTR length but the actual length (e.g. to memcpy data[] or to increase
> > statistics). People will get confused between can_get_data_len() and
> > can_get_len() due to the similar names. So I would suggest a more
> > explicit name to point out that this one is probably not the one you
> > want to use.
> > Candidates name I can think of:
> >  * can_get_raw_len()
> >  * can_get_advertised_len()
> >  * can_get_rtr_len()

So according to Marc's comments (c.f. below), candidates become:
  * can_skb_get_raw_len()
  * can_skb_get_advertised_len()
  * can_skb_get_rtr_len()

> > The only time you want to access the raw len (with real RTR value) is
> > in the TX path when you fill your device's structures. But here the
> > can_get_cc_dlc() is a better helper function which is already RTR
> > aware.
>
> There also is
>
> | unsigned int can_skb_get_frame_len(const struct sk_buff *skb)

I totally forgot about that one, despite the fact that I co-developed
it with you.

> It gives the length of the frame on the wire in bytes. We should use a
> common naming scheme. Let's always use can_skb_ as a prefix or drop the
> skb_ from this function.

You are right. The idea was to use the can_skb_ prefix because the
argument of the function was a struct skb_buff. Same applies here.

Re: [RFC PATCH 1/5] can: canxl: introduce CAN XL data structure

[Oliver Hartkopp]

On 13.07.22 03:07, Vincent Mailhol wrote:
> On Wed. 13 Jul. 2022 at 04:24, Oliver Hartkopp <socketcan@hartkopp.net> wrote:


> Well, if we document than can_frame::__pad shall be zero for mix
> usages (i.e. comments in struct can_frame and updated kernel doc),
> then we would have done our due diligence. From that point, if people
> ignore the documentation *and* do not follow best practices for safety
> application development, I wouldn't cry.

Right. As long as we did not enable CAN_XL we do not need to take care. 
And introducing the restriction to set __pad to zero together with the 
new CAN_XL option seems legit.

> If we absolutely want to prevent struct can_frame to be interpreted as
> a canxl_frame due to some stack garbage, we can add one
> padding/reserved field like that:
> 
> struct canxl_frame {
>          canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
>          __u8    sdt;   /* SDU (service data unit) type */
>          __u8    flags; /* additional flags for CAN XL */
>          __u16   len;   /* frame payload length in byte */
>          __u32   af;    /* acceptance field */
>          __u32 __res; /* reserved field. Shall be zero */
>          __u8    data[] __attribute__((aligned(8)));
> };
> 
> This way, the minimum transfer unit of CANXL is 17 bytes (16 for
> header and 1 for data) which is exactly one byte more than can_frame
> (and we get back the 8 bytes alignment \o/)
> 
> This would only leave the risk of having some garbage in
> canfd_frame::flags, e.g. if user does:
> 
>          struct canfd_frame cfd; /* declared on the stack and not initialized */
>          cfd.flags |= <some_flags> /* use |= instead of = */
> 
> But this is already risky for plain CAN-FD.

Hm, this brings me to an even more weird idea:

struct canxl_frame {
         canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
         __u8    sec:1, /* SEC bit */
                 xltag:7; /* all bits set */
         __u8    sdt;   /* SDU (service data unit) type */
         __u16   len;   /* frame payload length in byte */
         __u32   af;    /* acceptance field */
         __u8    data[CANXL_MAX_DLEN];
};

We could set the first byte (former len element) to a value of 0x7F and 
define the SEC bit as 0x80.

This would mean that we use the former length element to indicate the 
CAN XL frame as no CAN/CANFD frame can have a length of 127 and above.

Not sure if it makes sense to define some bits as depicted above, or if 
we should define a __u8 xlsec ?

Where we define

#define CANXL_TAG 0x7F
#define CANXL_SEC 0x80

which has to be set in the xlsec element then.

With this move we get rid of any flags problems (we only need the SEC 
bit anyway) and define a clear 'escape value' in the former length element.

(..)

>>
>> You are right about the inconsistency. But it needs to be
>>
>> #define CANXL_MIN_DLEN 1
>> #define CANXL_MAX_DLEN 2048
>>
>> to fit the CAN/CANFD definitions.
> 
> ACK.
> 
>>>> #define CANXL_MTU sizeof(struct canxl_frame)
>>>> #define CANXL_HEAD_SZ (CANXL_MTU - CANXL_MAXDLEN)
>>>> #define CANXL_MINTU (CANXL_HEAD_SZ + CANXL_MINDLEN)
>>>
>>> I need to think twice about all that, all the different alternatives
>>> (allow or not garbage, data as flexible member array vs.
>>> data[CANXL_DLEN]). Now it is a bit late, so I will continue to think
>>> about all that tomorrow.
>>
>> I would suggest to not allow garbage and have skbuff::len and
>> canxl_frame::len in sync.
>>
>> When a userspace application writes more bytes than needed by
>> canxl_frame::len, then that garbage is cropped to the needed size.
> 
> Cropped by whom? My point is to allow userspace to leave garbage. I am
> totally OK with cropping the frames as soon as they enter the kernel.

That was exactly what I wanted to say :-D

- be very strict inside the kernel
- and crop the userspace garbage away when entering the kernel

> On the other hand, the kernel shall always return cropped frames to
> the userland. To summarize, skb and data len synchronisation is
> optional for userland but mandatory for kernel drivers.

ACK!

Best regards,
Oliver

Re: [RFC PATCH 2/5] can: canxl: introduce ETH_P_CANXL ethernet protocol handling

[Oliver Hartkopp]

On 13.07.22 09:15, Vincent Mailhol wrote:
> On Wed. 13 Jul. 2022 at 16:02, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>> On 13.07.2022 08:58:26, Vincent Mailhol wrote:
>>> On Wed. 13 Jul. 2022 at 05:20, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>>> On 12.07.22 03:23, Vincent Mailhol wrote:
>>>>> On Tue. 12 Jul. 2022 at 03:44, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>>>>> Enable the PF_CAN infrastructure to handle CAN XL frames. A new ethernet
>>>>>> protocol type ETH_P_CANXL is defined to tag skbuffs containing the CAN XL
>>>>>> frame data structure.
>>>>>>
>>>>>> As the length information is now a uint16 value for CAN XL a new helper
>>>>>> function can_get_data_len() is introduced to retrieve the data length
>>>>>> from all types of CAN frames.
>>>>>>
>>>>>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>>>>>> ---
>>>>>>    include/linux/can/skb.h       | 14 ++++++++++
>>>>>>    include/uapi/linux/if_ether.h |  1 +
>>>>>>    net/can/af_can.c              | 49 +++++++++++++++++++++++++++++------
>>>>>>    3 files changed, 56 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
>>>>>> index 182749e858b3..d043bc4afd6d 100644
>>>>>> --- a/include/linux/can/skb.h
>>>>>> +++ b/include/linux/can/skb.h
>>>>>> @@ -101,6 +101,20 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
>>>>>>    {
>>>>>>           /* the CAN specific type of skb is identified by its data length */
>>>>>>           return skb->len == CANFD_MTU;
>>>>>>    }
>>>>>>
>>>>>> +/* get data length inside of CAN frame for all frame types */> +static inline unsigned int can_get_data_len(struct sk_buff *skb)
>>>>>> +{
>>>>>> +       if(skb->len == CANXL_MTU) {
>>>>>> +               const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
>>>>>> +
>>>>>> +               return cfx->len;
>>>>>> +       } else {
>>>>>> +               const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>>>>>> +
>>>>>> +               return cfd->len;
>>>>>> +       }
>>>>>> +}
>>>>>
>>>>> What about the RTR frames?
>>>>>
>>>>> If there are cases in which we intentionally want the declared length
>>>>> and not the actual length, it might be good to have two distinct
>>>>> helper functions.
>>>>
>>>> Good idea.
>>>>
>>>>> /* get data length inside of CAN frame for all frame types. For
>>>>>    * RTR frames, return zero. */
>>>>> static inline unsigned int can_get_actual_len(struct sk_buff *skb)
>>>>
>>>> I would name this one can_get_data_len()
>>>
>>> ACK.
> 
> So, according to Marc's comments (c.f. below):
> can_skb_get_data_len()
> 
>>>>> {
>>>>>          const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
>>>>>          const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
>>>>>
>>>>>          if (skb->len == CANXL_MTU)
>>>>>                  return cfx->len;
>>>>>
>>>>>          /* RTR frames have an actual length of zero */
>>>>>          if (skb->len == CAN_MTU && cfd->flags & CAN_RTR_FLAG)
>>>>>                  return 0;
>>>>>
>>>>>          return cfd->len;
>>>>> }
>>>>>
>>>>>
>>>>> /* get data length inside of CAN frame for all frame types. For
>>>>>    * RTR frames, return requested length. */
>>>>> static inline unsigned int can_get_declared_len(struct sk_buff *skb)
>>>>
>>>> I would name this one can_get_len()
>>>
>>> I anticipate that most of the time, developers do not want to get the
>>> RTR length but the actual length (e.g. to memcpy data[] or to increase
>>> statistics). People will get confused between can_get_data_len() and
>>> can_get_len() due to the similar names. So I would suggest a more
>>> explicit name to point out that this one is probably not the one you
>>> want to use.
>>> Candidates name I can think of:
>>>   * can_get_raw_len()
>>>   * can_get_advertised_len()
>>>   * can_get_rtr_len()

But these three functions still bconfuse me.

IMO we need two values:

- the data byte length (RTR aware)
- the (raw) length value

My suggestion for a naming would be:

- can_skb_get_data_len()
- can_skb_get_len_value()

This also fits to the existing
can_skb_get_frame_len().

> So according to Marc's comments (c.f. below), candidates become:
>    * can_skb_get_raw_len()
>    * can_skb_get_advertised_len()
>    * can_skb_get_rtr_len()

Right.

>>> The only time you want to access the raw len (with real RTR value) is
>>> in the TX path when you fill your device's structures. But here the
>>> can_get_cc_dlc() is a better helper function which is already RTR
>>> aware.
>>
>> There also is
>>
>> | unsigned int can_skb_get_frame_len(const struct sk_buff *skb)
> 
> I totally forgot about that one, despite the fact that I co-developed
> it with you.
> 
>> It gives the length of the frame on the wire in bytes. We should use a
>> common naming scheme. Let's always use can_skb_ as a prefix or drop the
>> skb_ from this function.
> 
> You are right. The idea was to use the can_skb_ prefix because the
> argument of the function was a struct skb_buff. Same applies here.

Yes, that's fine!

Best regards,
Oliver

Re: [RFC PATCH 1/5] can: canxl: introduce CAN XL data structure

[Vincent Mailhol]

On Thu. 14 Jul. 2022 at 05:02, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 13.07.22 03:07, Vincent Mailhol wrote:
> > On Wed. 13 Jul. 2022 at 04:24, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> > Well, if we document than can_frame::__pad shall be zero for mix
> > usages (i.e. comments in struct can_frame and updated kernel doc),
> > then we would have done our due diligence. From that point, if people
> > ignore the documentation *and* do not follow best practices for safety
> > application development, I wouldn't cry.
>
> Right. As long as we did not enable CAN_XL we do not need to take care.
> And introducing the restriction to set __pad to zero together with the
> new CAN_XL option seems legit.

ACK. I draw the line between the already existing applications and the
yet to be written ones. If we are safe for the existing ones, then I
think we are good to go.

> > If we absolutely want to prevent struct can_frame to be interpreted as
> > a canxl_frame due to some stack garbage, we can add one
> > padding/reserved field like that:
> >
> > struct canxl_frame {
> >          canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
> >          __u8    sdt;   /* SDU (service data unit) type */
> >          __u8    flags; /* additional flags for CAN XL */
> >          __u16   len;   /* frame payload length in byte */
> >          __u32   af;    /* acceptance field */
> >          __u32 __res; /* reserved field. Shall be zero */
> >          __u8    data[] __attribute__((aligned(8)));
> > };
> >
> > This way, the minimum transfer unit of CANXL is 17 bytes (16 for
> > header and 1 for data) which is exactly one byte more than can_frame
> > (and we get back the 8 bytes alignment \o/)
> >
> > This would only leave the risk of having some garbage in
> > canfd_frame::flags, e.g. if user does:
> >
> >          struct canfd_frame cfd; /* declared on the stack and not initialized */
> >          cfd.flags |= <some_flags> /* use |= instead of = */
> >
> > But this is already risky for plain CAN-FD.
>
> Hm, this brings me to an even more weird idea:
>
> struct canxl_frame {
>          canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
>          __u8    sec:1, /* SEC bit */
>                  xltag:7; /* all bits set */
>          __u8    sdt;   /* SDU (service data unit) type */
>          __u16   len;   /* frame payload length in byte */
>          __u32   af;    /* acceptance field */
>          __u8    data[CANXL_MAX_DLEN];
> };
>
> We could set the first byte (former len element) to a value of 0x7F and
> define the SEC bit as 0x80.

Beware! You just stepped in the realm of undefined behaviours! The
order of the bitfields is implementation specific...

Ref: ISO/IEC 9899:1999 section 6.7.2.1 "Structure and union
specifiers" paragraph 10:

| The order of allocation of bit-fields within a unit (high-order to
| low-order or low-order to high-order) is implementation-defined.

Example, on my x86_64 machine, this code:

/*********** begin **********/
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>

union foo {
        struct {
                uint8_t sec:1, /* SEC bit */
                        xltag:7; /* all bits set */
        };
        uint8_t raw;
};

int main()
{
        union foo foo;

        foo.sec = 1;
        printf("foo.raw: 0x%x\n", foo.raw);

        return EXIT_SUCCESS;
}
/*********** end ************/

will return: "foo.raw: 0x1" instead of the expected 0x80.

The correct declaration is:

/*********** begin **********/
#include <asm/byteorder.h>

struct canxl_frame {
        canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
#if defined(__LITTLE_ENDIAN_BITFIELD)
        __u8    xltag:7, /* all bits set */
                sec:1; /* SEC bit */
#else /* __BIG_ENDIAN_BITFIELD */
        __u8    sec:1, /* SEC bit */
                xltag:7; /* all bits set */
#endif
        __u8    sdt;   /* SDU (service data unit) type */
        __u16   len;   /* frame payload length in byte */
        __u32   af;    /* acceptance field */
        __u8    data[CANXL_MAX_DLEN];
};
/*********** end ************/

And this starts to be a really convoluted solution.

> This would mean that we use the former length element to indicate the
> CAN XL frame as no CAN/CANFD frame can have a length of 127 and above.
>
> Not sure if it makes sense to define some bits as depicted above, or if
> we should define a __u8 xlsec ?

If we follow your idea, use __u8 xlsec in order to avoid undefined behaviours.

> Where we define
>
> #define CANXL_TAG 0x7F

Here, you "burn" all the flags for future usage.
FYI, this doesn't have to be 0x7F. It could be any of the length
values not allowed by CAN-FD, namely (in decimal): 9-11, 13-15, 17-19,
21-23, 25-31, 33-47, 49-63

> #define CANXL_SEC 0x80

I did not get why CANXL_XLF isn't needed anymore.

> which has to be set in the xlsec element then.
>
> With this move we get rid of any flags problems (we only need the SEC
> bit anyway) and define a clear 'escape value' in the former length element.

If I try to bounce on your idea, I will propose:

/*********** begin **********/
struct canxl_frame {
        canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
        __u8    flags; /* additional flags for CAN XL. MSB must be set */
        __u8    sdt;   /* SDU (service data unit) type */
        __u16   len;   /* frame payload length in bytes */
        __u32   af;    /* acceptance field */
        __u8    data[];
};

#define CANXL_XLF 0x01 /* mark CAN XL for dual use of struct canfd_frame */
#define CANXL_SEC 0x02 /* Simple Extended Content (security/segmentation) */
#define CANXL_DISCRIMINATOR 0x80; /* Mandatory to distinguish between
CAN(-FD) and XL frames */
/*********** end ************/

This has no undefined behaviour and we still have five flags (0x04 to
0x40) for future use.


Yours sincerely,
Vincent Mailhol

Re: [RFC PATCH 2/5] can: canxl: introduce ETH_P_CANXL ethernet protocol handling

[Vincent Mailhol]

On Thu. 14 Jul. 2022 at 05:27, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 13.07.22 09:15, Vincent Mailhol wrote:
> > On Wed. 13 Jul. 2022 at 16:02, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >> On 13.07.2022 08:58:26, Vincent Mailhol wrote:
> >>> On Wed. 13 Jul. 2022 at 05:20, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >>>> On 12.07.22 03:23, Vincent Mailhol wrote:
> >>>>> On Tue. 12 Jul. 2022 at 03:44, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >>>>>> Enable the PF_CAN infrastructure to handle CAN XL frames. A new ethernet
> >>>>>> protocol type ETH_P_CANXL is defined to tag skbuffs containing the CAN XL
> >>>>>> frame data structure.
> >>>>>>
> >>>>>> As the length information is now a uint16 value for CAN XL a new helper
> >>>>>> function can_get_data_len() is introduced to retrieve the data length
> >>>>>> from all types of CAN frames.
> >>>>>>
> >>>>>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> >>>>>> ---
> >>>>>>    include/linux/can/skb.h       | 14 ++++++++++
> >>>>>>    include/uapi/linux/if_ether.h |  1 +
> >>>>>>    net/can/af_can.c              | 49 +++++++++++++++++++++++++++++------
> >>>>>>    3 files changed, 56 insertions(+), 8 deletions(-)
> >>>>>>
> >>>>>> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> >>>>>> index 182749e858b3..d043bc4afd6d 100644
> >>>>>> --- a/include/linux/can/skb.h
> >>>>>> +++ b/include/linux/can/skb.h
> >>>>>> @@ -101,6 +101,20 @@ static inline bool can_is_canfd_skb(const struct sk_buff *skb)
> >>>>>>    {
> >>>>>>           /* the CAN specific type of skb is identified by its data length */
> >>>>>>           return skb->len == CANFD_MTU;
> >>>>>>    }
> >>>>>>
> >>>>>> +/* get data length inside of CAN frame for all frame types */> +static inline unsigned int can_get_data_len(struct sk_buff *skb)
> >>>>>> +{
> >>>>>> +       if(skb->len == CANXL_MTU) {
> >>>>>> +               const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> >>>>>> +
> >>>>>> +               return cfx->len;
> >>>>>> +       } else {
> >>>>>> +               const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> >>>>>> +
> >>>>>> +               return cfd->len;
> >>>>>> +       }
> >>>>>> +}
> >>>>>
> >>>>> What about the RTR frames?
> >>>>>
> >>>>> If there are cases in which we intentionally want the declared length
> >>>>> and not the actual length, it might be good to have two distinct
> >>>>> helper functions.
> >>>>
> >>>> Good idea.
> >>>>
> >>>>> /* get data length inside of CAN frame for all frame types. For
> >>>>>    * RTR frames, return zero. */
> >>>>> static inline unsigned int can_get_actual_len(struct sk_buff *skb)
> >>>>
> >>>> I would name this one can_get_data_len()
> >>>
> >>> ACK.
> >
> > So, according to Marc's comments (c.f. below):
> > can_skb_get_data_len()
> >
> >>>>> {
> >>>>>          const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
> >>>>>          const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> >>>>>
> >>>>>          if (skb->len == CANXL_MTU)
> >>>>>                  return cfx->len;
> >>>>>
> >>>>>          /* RTR frames have an actual length of zero */
> >>>>>          if (skb->len == CAN_MTU && cfd->flags & CAN_RTR_FLAG)
> >>>>>                  return 0;
> >>>>>
> >>>>>          return cfd->len;
> >>>>> }
> >>>>>
> >>>>>
> >>>>> /* get data length inside of CAN frame for all frame types. For
> >>>>>    * RTR frames, return requested length. */
> >>>>> static inline unsigned int can_get_declared_len(struct sk_buff *skb)
> >>>>
> >>>> I would name this one can_get_len()
> >>>
> >>> I anticipate that most of the time, developers do not want to get the
> >>> RTR length but the actual length (e.g. to memcpy data[] or to increase
> >>> statistics). People will get confused between can_get_data_len() and
> >>> can_get_len() due to the similar names. So I would suggest a more
> >>> explicit name to point out that this one is probably not the one you
> >>> want to use.
> >>> Candidates name I can think of:
> >>>   * can_get_raw_len()
> >>>   * can_get_advertised_len()
> >>>   * can_get_rtr_len()
>
> But these three functions still confuse me.
>
> IMO we need two values:
>
> - the data byte length (RTR aware)
> - the (raw) length value
>
> My suggestion for a naming would be:
>
> - can_skb_get_data_len()
> - can_skb_get_len_value()

Hmm, you did not fully convince me but at the same time, your solution
is okayish to me.
My main point was to correctly manage the length according to the RTR
flag and this concern is now fully addressed. I am fine to go on with
your naming suggestions.

Yours sincerely,
Vincent Mailhol

Re: [RFC PATCH 1/5] can: canxl: introduce CAN XL data structure

[Oliver Hartkopp]

On 14.07.22 03:23, Vincent Mailhol wrote:
> On Thu. 14 Jul. 2022 at 05:02, Oliver Hartkopp <socketcan@hartkopp.net> wrote:


> If we follow your idea, use __u8 xlsec in order to avoid undefined behaviours.
> 
>> Where we define
>>
>> #define CANXL_TAG 0x7F
> 
> Here, you "burn" all the flags for future usage.
> FYI, this doesn't have to be 0x7F. It could be any of the length
> values not allowed by CAN-FD, namely (in decimal): 9-11, 13-15, 17-19,
> 21-23, 25-31, 33-47, 49-63

Yes, I detected this issue too when waking up this morning ...

>> #define CANXL_SEC 0x80
> 
> I did not get why CANXL_XLF isn't needed anymore.
> 
>> which has to be set in the xlsec element then.
>>
>> With this move we get rid of any flags problems (we only need the SEC
>> bit anyway) and define a clear 'escape value' in the former length element.
> 
> If I try to bounce on your idea, I will propose:
> 
> /*********** begin **********/
> struct canxl_frame {
>          canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
>          __u8    flags; /* additional flags for CAN XL. MSB must be set */
>          __u8    sdt;   /* SDU (service data unit) type */
>          __u16   len;   /* frame payload length in bytes */
>          __u32   af;    /* acceptance field */
>          __u8    data[];
> };

ACK.

> #define CANXL_XLF 0x01 /* mark CAN XL for dual use of struct canfd_frame */

IMO the dual use stuff between CAN FD & CAN XL is not working anymore 
and became obsolete here.

CAN_XLF needs to be a hard switch for CAN XL - no optional thing.

> #define CANXL_SEC 0x02 /* Simple Extended Content (security/segmentation) */
> #define CANXL_DISCRIMINATOR 0x80; /* Mandatory to distinguish between
> CAN(-FD) and XL frames */
> /*********** end ************/
> 
> This has no undefined behaviour and we still have five flags (0x04 to
> 0x40) for future use.
> 

I would suggest the following:

#define CANXL_XLF 0x80 /* mark CAN XL frame (must be set) */
#define CANXL_SEC 0x40 /* Simple Extended Content (security/segmentation) */

And the rest of the bits are reserved (shall be set to zero).

This way the CAN_XLF value would make the former 'len' element 128 - 
which is a proper distinction for CAN XL frames as such length value 
surely bounces on CAN/CANFD frames.

Best regards,
Oliver

Re: [RFC PATCH 1/5] can: canxl: introduce CAN XL data structure

[Vincent Mailhol]

On Thu. 14 Jul. 2022 at 15:11, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 14.07.22 03:23, Vincent Mailhol wrote:
> > On Thu. 14 Jul. 2022 at 05:02, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> > If we follow your idea, use __u8 xlsec in order to avoid undefined behaviours.
> >
> >> Where we define
> >>
> >> #define CANXL_TAG 0x7F
> >
> > Here, you "burn" all the flags for future usage.
> > FYI, this doesn't have to be 0x7F. It could be any of the length
> > values not allowed by CAN-FD, namely (in decimal): 9-11, 13-15, 17-19,
> > 21-23, 25-31, 33-47, 49-63
>
> Yes, I detected this issue too when waking up this morning ...
>
> >> #define CANXL_SEC 0x80
> >
> > I did not get why CANXL_XLF isn't needed anymore.
> >
> >> which has to be set in the xlsec element then.
> >>
> >> With this move we get rid of any flags problems (we only need the SEC
> >> bit anyway) and define a clear 'escape value' in the former length element.
> >
> > If I try to bounce on your idea, I will propose:
> >
> > /*********** begin **********/
> > struct canxl_frame {
> >          canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
> >          __u8    flags; /* additional flags for CAN XL. MSB must be set */
> >          __u8    sdt;   /* SDU (service data unit) type */
> >          __u16   len;   /* frame payload length in bytes */
> >          __u32   af;    /* acceptance field */
> >          __u8    data[];
> > };
>
> ACK.
>
> > #define CANXL_XLF 0x01 /* mark CAN XL for dual use of struct canfd_frame */
>
> IMO the dual use stuff between CAN FD & CAN XL is not working anymore
> and became obsolete here.
>
> CAN_XLF needs to be a hard switch for CAN XL - no optional thing.
>
> > #define CANXL_SEC 0x02 /* Simple Extended Content (security/segmentation) */
> > #define CANXL_DISCRIMINATOR 0x80; /* Mandatory to distinguish between
> > CAN(-FD) and XL frames */
> > /*********** end ************/
> >
> > This has no undefined behaviour and we still have five flags (0x04 to
> > 0x40) for future use.
> >
>
> I would suggest the following:
>
> #define CANXL_XLF 0x80 /* mark CAN XL frame (must be set) */
> #define CANXL_SEC 0x40 /* Simple Extended Content (security/segmentation) */

OK. Having CANXL_SEC on the most significant bit (0x40) or instead of
the least significant bit (0x01) works for me.

> And the rest of the bits are reserved (shall be set to zero).

ACK.

> This way the CAN_XLF value would make the former 'len' element 128 -
> which is a proper distinction for CAN XL frames as such length value
> surely bounces on CAN/CANFD frames.

The purpose of the CAN_XLF is still unclear to me. In your initial
patch, you wrote that CAN_XLF was to "mark CAN XL for dual use of
struct canfd_frame". So are we really OK to specify that CAN_XLF is
always set? How are we going to tag dual use?

But my main point was to always set 0x80 to differentiate between
CAN(-FD) and CANXL, and we are aligned on this. :D


The last topic remaining is the data[] vs. data[CANXL_MAX_DLEN]. I
thought about it but can not find any killer arguments for either
solution.
The biggest difference is that for data[] we can do sizeof(struct
canxl_frame) to get the header file whereas for data[CANXL_MAX_DLEN],
we need to introduce the CANXL_HEADER_SIZE macro.
My preference still goes toward the data[] because I see flexible
member arrays as being more idiomatic C. But it is more for personal
taste than anything else…


Yours sincerely,
Vincent Mailhol

Re: [RFC PATCH 1/5] can: canxl: introduce CAN XL data structure

[Oliver Hartkopp]

On 14.07.22 11:12, Vincent Mailhol wrote:
> On Thu. 14 Jul. 2022 at 15:11, Oliver Hartkopp <socketcan@hartkopp.net> wrote:



>> I would suggest the following:
>>
>> #define CANXL_XLF 0x80 /* mark CAN XL frame (must be set) */
>> #define CANXL_SEC 0x40 /* Simple Extended Content (security/segmentation) */
> 
> OK. Having CANXL_SEC on the most significant bit (0x40) or instead of
> the least significant bit (0x01) works for me.

Hm. This is a really iterative process ;-)

Maybe

#define CANXL_XLF 0x80 /* mark CAN XL frame (must be set) */
#define CANXL_SEC 0x01 /* Simple Extended Content (security/segmentation) */

looks even more natural:

The MSB is set and the other bits start from LSB as usual.

>> And the rest of the bits are reserved (shall be set to zero).
> 
> ACK.
> 
>> This way the CAN_XLF value would make the former 'len' element 128 -
>> which is a proper distinction for CAN XL frames as such length value
>> surely bounces on CAN/CANFD frames.
> 
> The purpose of the CAN_XLF is still unclear to me. In your initial
> patch, you wrote that CAN_XLF was to "mark CAN XL for dual use of
> struct canfd_frame". So are we really OK to specify that CAN_XLF is
> always set? How are we going to tag dual use?

I think the dual-use pattern does not make sense anymore as either the 
flags element and the len element have been moved away from the struct 
can[fd]_frame layout.

There is no layout match between CAN XL and CAN/CANFD anymore.

> But my main point was to always set 0x80 to differentiate between
> CAN(-FD) and CANXL, and we are aligned on this. :D

ACK.

IMO this 0x80 bit at this specific position is an excellent flag to 
introduce CAN XL frames and to provide a proper break with CAN/CANFD 
data structures.

> The last topic remaining is the data[] vs. data[CANXL_MAX_DLEN]. I
> thought about it but can not find any killer arguments for either
> solution.
> The biggest difference is that for data[] we can do sizeof(struct
> canxl_frame) to get the header file whereas for data[CANXL_MAX_DLEN],
> we need to introduce the CANXL_HEADER_SIZE macro.
> My preference still goes toward the data[] because I see flexible
> member arrays as being more idiomatic C. But it is more for personal
> taste than anything else…

I see it as a practical thing for programmers (also in userspace) to 
allocate sizeof(struct canxl_frame) and do all the checks and operations 
similar to struct can[fd]_frame (and the ISO CAN Spec).

It just makes it very clear what we are talking about.

The fact that we can reduce the content size whenever we transfer 
content from/to kernel space or inside the kernel is just a nice 
optimization IMO.

> Yours sincerely,
> Vincent Mailhol

Thanks for your feedback! I think I'll post an updated patch set later 
today to continue the discussion on a new code basis.

Best regards,
Oliver