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

[RFC PATCH v5 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

V2: Major rework after discussion and feedback on Linux-CAN ML

- rework of struct canxl_frame
- CANXL_XLF flag is now the switch between CAN XL and CAN/CANFD
- variable length in r/w operations for CAN XL frames
- write CAN XL frame to raw socket enforces size <-> canxl_frame.len sync

V3: Fix length for CAN XL frames inside the sk_buff

- extend the CAN_RAW sockopt to handle fixed/truncated read/write operations

V4: Fix patch 5 (can: raw: add CAN XL support)

- fix return value (move 'err = -EINVAL' in raw_sendmsg())
- add CAN XL frame handling in can_rcv()
- change comment for CAN_RAW_XL_[RT]X_DYN definition (allow -> enable)

V5: Remove CAN_RAW_XL_[RT]X_DYN definition again

- CAN_RAW_XL_[RT]X_DYN (truncated data) feature is now enabled by default
- use CANXL_MIN_DLEN instead of '1' in canxl_frame definition
- add missing 'err = -EINVAL' initialization in raw_sendmsg())

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        | 49 ++++++++++++++----
 drivers/net/can/vcan.c           | 11 ++--
 include/linux/can/skb.h          | 44 +++++++++++++++-
 include/uapi/linux/can.h         | 49 ++++++++++++++++++
 include/uapi/linux/can/raw.h     |  1 +
 include/uapi/linux/if_ether.h    |  1 +
 net/can/af_can.c                 | 32 ++++++++++--
 net/can/raw.c                    | 89 +++++++++++++++++++++++++++-----
 9 files changed, 242 insertions(+), 36 deletions(-)

-- 
2.30.2

[RFC PATCH v5 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 | 49 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
index 90801ada2bbe..c91988402f25 100644
--- a/include/uapi/linux/can.h
+++ b/include/uapi/linux/can.h
@@ -46,10 +46,11 @@
 #ifndef _UAPI_CAN_H
 #define _UAPI_CAN_H
 
 #include <linux/types.h>
 #include <linux/socket.h>
+#include <linux/stddef.h> /* for offsetof */
 
 /* controller area network (CAN) kernel definitions */
 
 /* special address description flags for the CAN_ID */
 #define CAN_EFF_FLAG 0x80000000U /* EFF/SFF is set in the MSB */
@@ -58,10 +59,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 +73,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 +92,20 @@ 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_MIN_DLC 0
+#define CANXL_MAX_DLC 2047
+#define CANXL_MAX_DLC_MASK 0x07FF
+#define CANXL_MIN_DLEN 1
+#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)
@@ -164,12 +177,48 @@ struct canfd_frame {
 	__u8    __res0;  /* reserved / padding */
 	__u8    __res1;  /* reserved / padding */
 	__u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
 };
 
+/*
+ * defined bits for canxl_frame.flags
+ *
+ * The canxl_frame.flags element contains two bits CANXL_XLF and CANXL_SEC
+ * and shares the relative position of the struct can[fd]_frame.len element.
+ * The CANXL_XLF bit ALWAYS needs to be set to indicate a valid CAN XL frame.
+ * As a side effect setting this bit intentionally breaks the length checks
+ * for Classical CAN and CAN FD frames.
+ *
+ * Undefined bits in canxl_frame.flags are reserved and shall be set to zero.
+ */
+#define CANXL_XLF 0x80 /* mandatory CAN XL frame flag (must always be set!) */
+#define CANXL_SEC 0x01 /* Simple Extended Content (security/segmentation) */
+
+/**
+ * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
+ * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags
+ * @flags: additional flags for CAN XL
+ * @sdt:   SDU (service data unit) type
+ * @len:   frame payload length in byte (CANXL_MIN_DLEN .. CANXL_MAX_DLEN)
+ * @af:    acceptance field
+ * @data:  CAN XL frame payload (CANXL_MIN_DLEN .. CANXL_MAX_DLEN byte)
+ *
+ * @prio shares the same position as @can_id from struct can[fd]_frame.
+ */
+struct canxl_frame {
+	canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
+	__u8    flags; /* additional flags for CAN XL */
+	__u8    sdt;   /* SDU (service data unit) type */
+	__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))
+#define CANXL_HDR_SZ	(offsetof(struct canxl_frame, data))
 
 /* 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 v5 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 three new
helper functions have been 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       | 39 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/if_ether.h |  1 +
 net/can/af_can.c              | 32 +++++++++++++++++++++++-----
 3 files changed, 67 insertions(+), 5 deletions(-)

diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index 182749e858b3..51481f5afe62 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -101,6 +101,45 @@ 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;
 }
 
+static inline bool can_is_canxl_skb(const struct sk_buff *skb)
+{
+	const struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
+
+	if (skb->len != CANXL_MTU)
+		return false;
+
+	/* check valid CAN XL data length boundaries */
+	if (cfx->len < CANXL_MIN_DLEN || cfx->len > CANXL_MAX_DLEN)
+		return false;
+
+	return cfx->flags & CANXL_XLF;
+}
+
+/* get length element value from can[|fd|xl]_frame structure */
+static inline unsigned int can_skb_get_len_val(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 (can_is_canxl_skb(skb))
+		return cfx->len;
+
+	return cfd->len;
+}
+
+/* get needed data length inside of CAN frame for all frame types (RTR aware) */
+static inline unsigned int can_skb_get_data_len(struct sk_buff *skb)
+{
+	unsigned int len = can_skb_get_len_val(skb);
+	const struct can_frame *cf = (struct can_frame *)skb->data;
+
+	/* RTR frames have an actual length of zero */
+	if (skb->len == CAN_MTU && cf->can_id & CAN_RTR_FLAG)
+		return 0;
+
+	return 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..23e56e4e2457 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -209,19 +209,20 @@ int can_send(struct sk_buff *skb, int loop)
 			goto inval_skb;
 	} else if (skb->len == CANFD_MTU) {
 		skb->protocol = htons(ETH_P_CANFD);
 		if (unlikely(cfd->len > CANFD_MAX_DLEN))
 			goto inval_skb;
+	} else if (skb->len == CANXL_MTU) {
+		skb->protocol = htons(ETH_P_CANXL);
+		if (unlikely(!can_is_canxl_skb(skb)))
+			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,25 @@ 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)
+{
+	if (unlikely(dev->type != ARPHRD_CAN || (!can_is_canxl_skb(skb)))) {
+		pr_warn_once("PF_CAN: dropped non conform CAN XL skbuff: dev type %d, len %d\n",
+			     dev->type, skb->len);
+
+		kfree_skb(skb);
+		return NET_RX_DROP;
+	}
+
+	can_receive(skb, dev);
+	return NET_RX_SUCCESS;
+}
+
 /* af_can protocol functions */
 
 /**
  * can_proto_register - register CAN transport protocol
  * @cp: pointer to CAN protocol structure
@@ -849,10 +865,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 +909,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 v5 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        | 49 ++++++++++++++++++++++++++------
 include/linux/can/skb.h          |  5 +++-
 3 files changed, 45 insertions(+), 11 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..8531e0c39d1c 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",
@@ -103,17 +104,13 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
 		/* Using "struct canfd_frame::len" for the frame
 		 * 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;
 
 		/* 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 = can_skb_get_data_len(skb);
 
 		if (frame_len_ptr)
 			*frame_len_ptr = can_skb_priv->frame_len;
 
 		priv->echo_skb[idx] = NULL;
@@ -139,11 +136,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 +241,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);
@@ -302,10 +330,13 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
 			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(!can_is_canxl_skb(skb)))
+			goto inval_skb;
 	} else {
 		goto inval_skb;
 	}
 
 	if (!can_skb_headroom_valid(dev, skb)) {
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index 51481f5afe62..9972c9bd73bc 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -18,19 +18,22 @@
 
 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);
 struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf);
 struct sk_buff *alloc_canfd_skb(struct net_device *dev,
 				struct canfd_frame **cfd);
+struct sk_buff *alloc_canxl_skb(struct net_device *dev,
+				struct canxl_frame **cfx);
 struct sk_buff *alloc_can_err_skb(struct net_device *dev,
 				  struct can_frame **cf);
 bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb);
 
 /*
-- 
2.30.2

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

[Vincent Mailhol]

On Tue. 19 Jul. 2022, 20:44, Oliver Hartkopp <socketcan@hartkopp.net> 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        | 49 ++++++++++++++++++++++++++------
>  include/linux/can/skb.h          |  5 +++-
>  3 files changed, 45 insertions(+), 11 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..8531e0c39d1c 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",
> @@ -103,17 +104,13 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
>                 /* Using "struct canfd_frame::len" for the frame
>                  * 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;
>
>                 /* 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 = can_skb_get_data_len(skb);
>
>                 if (frame_len_ptr)
>                         *frame_len_ptr = can_skb_priv->frame_len;
>
>                 priv->echo_skb[idx] = NULL;
> @@ -139,11 +136,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 +241,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));

I am confused. In this message:
https://lore.kernel.org/linux-can/3dcc85f8-2c02-dfe5-de23-d022f3fc56be@hartkopp.net/
you said that you "vote for selecting the 'truncated' option only"
(which is OK with me). But here, you are allocating the full size. If
we always want truncated frames, shouldn't we allocate the exact size
frame length here?

> +       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);
> @@ -302,10 +330,13 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
>                         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)) {

Nitpick but with the growing list, I would see a switch
(skb->protocol) rather than the cascade of if.

> +               if (unlikely(!can_is_canxl_skb(skb)))
> +                       goto inval_skb;
>         } else {

default:

>                 goto inval_skb;
>         }
>
>         if (!can_skb_headroom_valid(dev, skb)) {
> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> index 51481f5afe62..9972c9bd73bc 100644
> --- a/include/linux/can/skb.h
> +++ b/include/linux/can/skb.h
> @@ -18,19 +18,22 @@
>
>  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);
>  struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf);
>  struct sk_buff *alloc_canfd_skb(struct net_device *dev,
>                                 struct canfd_frame **cfd);
> +struct sk_buff *alloc_canxl_skb(struct net_device *dev,
> +                               struct canxl_frame **cfx);
>  struct sk_buff *alloc_can_err_skb(struct net_device *dev,
>                                   struct can_frame **cf);
>  bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb);

Yours sincerely,
Vincent Mailhol

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

[Oliver Hartkopp]

On 19.07.22 16:11, Vincent Mailhol wrote:
> On Tue. 19 Jul. 2022, 20:44, Oliver Hartkopp <socketcan@hartkopp.net> 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        | 49 ++++++++++++++++++++++++++------
>>   include/linux/can/skb.h          |  5 +++-
>>   3 files changed, 45 insertions(+), 11 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..8531e0c39d1c 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",
>> @@ -103,17 +104,13 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
>>                  /* Using "struct canfd_frame::len" for the frame
>>                   * 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;
>>
>>                  /* 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 = can_skb_get_data_len(skb);
>>
>>                  if (frame_len_ptr)
>>                          *frame_len_ptr = can_skb_priv->frame_len;
>>
>>                  priv->echo_skb[idx] = NULL;
>> @@ -139,11 +136,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 +241,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));
> 
> I am confused. In this message:
> https://lore.kernel.org/linux-can/3dcc85f8-2c02-dfe5-de23-d022f3fc56be@hartkopp.net/
> you said that you "vote for selecting the 'truncated' option only"
> (which is OK with me). But here, you are allocating the full size. If
> we always want truncated frames, shouldn't we allocate the exact size
> frame length here?

No confusion.

The API to the user space is 'truncated' option only.

The data structure inside the kernel sizeof(struct can[|fd|xl]_frame).

See:
https://lore.kernel.org/linux-can/4c79233f-1301-d5c7-7698-38d39d8702aa@hartkopp.net/

---8<---

As the sk_buffs are only allocated once and are not copied inside the
kernel there should be no remarkable drawbacks whether we allocate
CAN_MTU skbs or CANXL_MTU skbs.

AFAICS both skbs will fit into a single page.

---8<---

It is just easier and safe.

(..)

>> @@ -302,10 +330,13 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
>>                          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)) {
> 
> Nitpick but with the growing list, I would see a switch
> (skb->protocol) rather than the cascade of if.
> 
>> +               if (unlikely(!can_is_canxl_skb(skb)))
>> +                       goto inval_skb;
>>          } else {
> 
> default:
> 
>>                  goto inval_skb;
>>          }

Yes. Good idea!

Will change that.


Best regards,
Oliver

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

[Vincent Mailhol]

On Tue 19 Jul. 2022 at 23:38, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 19.07.22 16:11, Vincent Mailhol wrote:
> > On Tue. 19 Jul. 2022, 20:44, Oliver Hartkopp <socketcan@hartkopp.net> 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        | 49 ++++++++++++++++++++++++++------
> >>   include/linux/can/skb.h          |  5 +++-
> >>   3 files changed, 45 insertions(+), 11 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..8531e0c39d1c 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",
> >> @@ -103,17 +104,13 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, u8 *len_ptr,
> >>                  /* Using "struct canfd_frame::len" for the frame
> >>                   * 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;
> >>
> >>                  /* 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 = can_skb_get_data_len(skb);
> >>
> >>                  if (frame_len_ptr)
> >>                          *frame_len_ptr = can_skb_priv->frame_len;
> >>
> >>                  priv->echo_skb[idx] = NULL;
> >> @@ -139,11 +136,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 +241,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));
> >
> > I am confused. In this message:
> > https://lore.kernel.org/linux-can/3dcc85f8-2c02-dfe5-de23-d022f3fc56be@hartkopp.net/
> > you said that you "vote for selecting the 'truncated' option only"
> > (which is OK with me). But here, you are allocating the full size. If
> > we always want truncated frames, shouldn't we allocate the exact size
> > frame length here?
>
> No confusion.
>
> The API to the user space is 'truncated' option only.
>
> The data structure inside the kernel sizeof(struct can[|fd|xl]_frame).
>
> See:
> https://lore.kernel.org/linux-can/4c79233f-1301-d5c7-7698-38d39d8702aa@hartkopp.net/
>
> ---8<---
>
> As the sk_buffs are only allocated once and are not copied inside the
> kernel there should be no remarkable drawbacks whether we allocate
> CAN_MTU skbs or CANXL_MTU skbs.
>
> AFAICS both skbs will fit into a single page.

This is true. A page is at least 4k. So the 2k + alpha will easily fit.
But the page is not the smallest chunk that can return malloc, c.f.
KMALLOC_MIN_SIZE:
https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h#L279

Also, more than the page size, my concern are the cache misses,
especially when memsetting to zero the full canxl frame. As far as I
understand, cloning an skb does not copy the payload (only increment a
reference to it) so the echo_skb thing should not be impacted.
That said, I am not able to tell whether or not this will have a
noticeable impact (I have some feeling it might but can not assert
it). If this looks good for the people who have the expertise in
kernel performances, then I am fine.

> ---8<---
>
> It is just easier and safe.
>
> (..)
>
> >> @@ -302,10 +330,13 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
> >>                          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)) {
> >
> > Nitpick but with the growing list, I would see a switch
> > (skb->protocol) rather than the cascade of if.
> >
> >> +               if (unlikely(!can_is_canxl_skb(skb)))
> >> +                       goto inval_skb;
> >>          } else {
> >
> > default:
> >
> >>                  goto inval_skb;
> >>          }
>
> Yes. Good idea!
>
> Will change that.

Maybe even better:

        switch(ntohs(skb->protocol)) {
        case ETH_P_CAN:
               /* ... */
        case ETH_P_CANFD:
               /* ... */
        case ETH_P_CANXL:
                /* ... */
        default:
                /* ... */
        }

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

[Oliver Hartkopp]

On 19.07.22 17:16, Vincent Mailhol wrote:
> On Tue 19 Jul. 2022 at 23:38, Oliver Hartkopp <socketcan@hartkopp.net> wrote:


>> No confusion.
>>
>> The API to the user space is 'truncated' option only.
>>
>> The data structure inside the kernel sizeof(struct can[|fd|xl]_frame).
>>
>> See:
>> https://lore.kernel.org/linux-can/4c79233f-1301-d5c7-7698-38d39d8702aa@hartkopp.net/
>>
>> ---8<---
>>
>> As the sk_buffs are only allocated once and are not copied inside the
>> kernel there should be no remarkable drawbacks whether we allocate
>> CAN_MTU skbs or CANXL_MTU skbs.
>>
>> AFAICS both skbs will fit into a single page.
> 
> This is true. A page is at least 4k. So the 2k + alpha will easily fit.
> But the page is not the smallest chunk that can return malloc, c.f.
> KMALLOC_MIN_SIZE:
> https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h#L279
> 
> Also, more than the page size, my concern are the cache misses,
> especially when memsetting to zero the full canxl frame. As far as I
> understand, cloning an skb does not copy the payload (only increment a
> reference to it) so the echo_skb thing should not be impacted.
> That said, I am not able to tell whether or not this will have a
> noticeable impact (I have some feeling it might but can not assert
> it). If this looks good for the people who have the expertise in
> kernel performances, then I am fine.

The more I think about our discussion and your remark that we were 
somehow going back to the V2 patch set the more I wonder if this would 
be a good idea.

IMO using the struct canxl_frame (including 2048 byte) and allowing 
truncated sizes can be handled inside the kernel safely.

And with V2 we only allocate the needed size for the sk_buff - without 
any memsetting.

When user space gets a truncated frame -> fine

When the user forges some CAN XL frame where the length value does not 
match the spec and the size does not fit the length -> -EINVAL

So there is no uninitialized data also.

And even the user space side to handle a mix of CAN frame types is 
pretty simple IMO:

union {
         struct can_frame cc;
         struct canfd_frame fd;
         struct canxl_frame xl;
} can;

nbytes = read(s, &can.xl, sizeof(struct canxl_frame));
if (nbytes < 0) {
         perror("read");
         return 1;
}
printf("nbytes = %d\n", nbytes);

if (nbytes < CANXL_HDR_SZ + CANXL_MIN_DLEN) {
         fprintf(stderr, "read: no CAN frame\n");
         return 1;
}

if (can.xl.flags & CANXL_XLF) {
          if (nbytes != CANXL_HDR_SZ + can.xl.len) {
                 printf("nbytes = %d\n", nbytes);
                 perror("read canxl_frame");
                 continue;
          }
          /* process CAN XL frames */
          printf("canxl frame prio %03X len %d flags %d\n",
                  can.xl.prio, can.xl.len, can.xl.flags);
          continue;
}

if (nbytes != sizeof(struct can_frame) &&
     nbytes != sizeof(struct canfd_frame)) {
         fprintf(stderr, "read: incomplete CAN(FD) frame\n");
         return 1;
}

/* process CAN/FD frames */

Working with partially filled data structures is a known pattern for CAN 
and CAN FD.

We only optimize the transfer from/to kernel space with truncated 
read/write operations.

¯\_(ツ)_/¯

> 
> Maybe even better:
> 
>          switch(ntohs(skb->protocol)) {
>          case ETH_P_CAN:
>                 /* ... */
>          case ETH_P_CANFD:
>                 /* ... */
>          case ETH_P_CANXL:
>                  /* ... */
>          default:
>                  /* ... */
>          }

Yes ... updated my next patch.

Best regards,
Oliver

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

[Vincent Mailhol]

On Wed. 21 Jul. 2022 at 01:43, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 19.07.22 17:16, Vincent Mailhol wrote:
> > On Tue 19 Jul. 2022 at 23:38, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >> No confusion.
> >>
> >> The API to the user space is 'truncated' option only.
> >>
> >> The data structure inside the kernel sizeof(struct can[|fd|xl]_frame).
> >>
> >> See:
> >> https://lore.kernel.org/linux-can/4c79233f-1301-d5c7-7698-38d39d8702aa@hartkopp.net/
> >>
> >> ---8<---
> >>
> >> As the sk_buffs are only allocated once and are not copied inside the
> >> kernel there should be no remarkable drawbacks whether we allocate
> >> CAN_MTU skbs or CANXL_MTU skbs.
> >>
> >> AFAICS both skbs will fit into a single page.
> >
> > This is true. A page is at least 4k. So the 2k + alpha will easily fit.
> > But the page is not the smallest chunk that can return malloc, c.f.
> > KMALLOC_MIN_SIZE:
> > https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h#L279
> >
> > Also, more than the page size, my concern are the cache misses,
> > especially when memsetting to zero the full canxl frame. As far as I
> > understand, cloning an skb does not copy the payload (only increment a
> > reference to it) so the echo_skb thing should not be impacted.
> > That said, I am not able to tell whether or not this will have a
> > noticeable impact (I have some feeling it might but can not assert
> > it). If this looks good for the people who have the expertise in
> > kernel performances, then I am fine.
>
> The more I think about our discussion and your remark that we were
> somehow going back to the V2 patch set the more I wonder if this would
> be a good idea.

I quite liked v2. My comments on the v2 were mostly to argue on the
data[CANXL_MAX_DLEN] vs the flexible member array, but aside from
that, it looked pretty good.

> IMO using the struct canxl_frame (including 2048 byte) and allowing
> truncated sizes can be handled inside the kernel safely.
>
> And with V2 we only allocate the needed size for the sk_buff - without
> any memsetting.
>
> When user space gets a truncated frame -> fine
>
> When the user forges some CAN XL frame where the length value does not
> match the spec and the size does not fit the length -> -EINVAL
>
> So there is no uninitialized data also.

So basically, forcing the truncation everywhere (TX, RX both userland
and kernel), correct? i.e. the skb length shall always be equal to
CANXL_HEADER_SIZE + canxl_frame::len.

I think this is good. As I stated before, getting -EINVAL is benign.
If developers are doing crazy things because they did not read the
doc, it is better to fail them early. If we go for truncation then
always truncating is the safest: less option -> less confusion.

> And even the user space side to handle a mix of CAN frame types is
> pretty simple IMO:
>
> union {
>          struct can_frame cc;
>          struct canfd_frame fd;
>          struct canxl_frame xl;
> } can;

Do you want to add this union in the kernel uapi or is it just a
userland example?

> nbytes = read(s, &can.xl, sizeof(struct canxl_frame));
> if (nbytes < 0) {
>          perror("read");
>          return 1;
> }
> printf("nbytes = %d\n", nbytes);

Does read() guarantee atomicity? From "man 2 read":

| It is not an error if [the return value] is smaller than the number
| of bytes requested; this may happen for example because fewer bytes
| are actually available right now (maybe because we were close to
| end-of-file, or because we are reading from a pipe, or from a
| terminal), *or because read() was interrupted by a signal*.

I think the answer is yes, but I prefer to double check (I am
especially concerned by the signal interrupts).

>
> if (nbytes < CANXL_HDR_SZ + CANXL_MIN_DLEN) {
>          fprintf(stderr, "read: no CAN frame\n");
>          return 1;
> }
>
> if (can.xl.flags & CANXL_XLF) {
>           if (nbytes != CANXL_HDR_SZ + can.xl.len) {
>                  printf("nbytes = %d\n", nbytes);
>                  perror("read canxl_frame");

ACK.

>                  continue;
>           }
>           /* process CAN XL frames */
>           printf("canxl frame prio %03X len %d flags %d\n",
>                   can.xl.prio, can.xl.len, can.xl.flags);
>           continue;
> }
>
> if (nbytes != sizeof(struct can_frame) &&
>      nbytes != sizeof(struct canfd_frame)) {

On the first read, I thought you meant "else if", then, I saw the
"continue" on the previous line.

>          fprintf(stderr, "read: incomplete CAN(FD) frame\n");
>          return 1;
> }
>
> /* process CAN/FD frames */
>
> Working with partially filled data structures is a known pattern for CAN
> and CAN FD.

Except that for CAN-(FD), truncation is not possible.

> We only optimize the transfer from/to kernel space with truncated
> read/write operations.
>
> ¯\_(ツ)_/¯

Yes, this full discussion is about optimizations…

> >
> > Maybe even better:
> >
> >          switch(ntohs(skb->protocol)) {
> >          case ETH_P_CAN:
> >                 /* ... */
> >          case ETH_P_CANFD:
> >                 /* ... */
> >          case ETH_P_CANXL:
> >                  /* ... */
> >          default:
> >                  /* ... */
> >          }
>
> Yes ... updated my next patch.
>
> Best regards,
> Oliver

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

[Vincent Mailhol]

On Thu. 21 juil. 2022 at 11:37, Vincent Mailhol
<vincent.mailhol@gmail.com> wrote:
> On Wed. 21 Jul. 2022 at 01:43, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> > On 19.07.22 17:16, Vincent Mailhol wrote:
> > > On Tue 19 Jul. 2022 at 23:38, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> > >> No confusion.
> > >>
> > >> The API to the user space is 'truncated' option only.
> > >>
> > >> The data structure inside the kernel sizeof(struct can[|fd|xl]_frame).
> > >>
> > >> See:
> > >> https://lore.kernel.org/linux-can/4c79233f-1301-d5c7-7698-38d39d8702aa@hartkopp.net/
> > >>
> > >> ---8<---
> > >>
> > >> As the sk_buffs are only allocated once and are not copied inside the
> > >> kernel there should be no remarkable drawbacks whether we allocate
> > >> CAN_MTU skbs or CANXL_MTU skbs.
> > >>
> > >> AFAICS both skbs will fit into a single page.
> > >
> > > This is true. A page is at least 4k. So the 2k + alpha will easily fit.
> > > But the page is not the smallest chunk that can return malloc, c.f.
> > > KMALLOC_MIN_SIZE:
> > > https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h#L279
> > >
> > > Also, more than the page size, my concern are the cache misses,
> > > especially when memsetting to zero the full canxl frame. As far as I
> > > understand, cloning an skb does not copy the payload (only increment a
> > > reference to it) so the echo_skb thing should not be impacted.
> > > That said, I am not able to tell whether or not this will have a
> > > noticeable impact (I have some feeling it might but can not assert
> > > it). If this looks good for the people who have the expertise in
> > > kernel performances, then I am fine.
> >
> > The more I think about our discussion and your remark that we were
> > somehow going back to the V2 patch set the more I wonder if this would
> > be a good idea.
>
> I quite liked v2. My comments on the v2 were mostly to argue on the
> data[CANXL_MAX_DLEN] vs the flexible member array, but aside from
> that, it looked pretty good.
>
> > IMO using the struct canxl_frame (including 2048 byte) and allowing
> > truncated sizes can be handled inside the kernel safely.
> >
> > And with V2 we only allocate the needed size for the sk_buff - without
> > any memsetting.
> >
> > When user space gets a truncated frame -> fine
> >
> > When the user forges some CAN XL frame where the length value does not
> > match the spec and the size does not fit the length -> -EINVAL
> >
> > So there is no uninitialized data also.
>
> So basically, forcing the truncation everywhere (TX, RX both userland
> and kernel), correct? i.e. the skb length shall always be equal to
> CANXL_HEADER_SIZE + canxl_frame::len.
>
> I think this is good. As I stated before, getting -EINVAL is benign.
> If developers are doing crazy things because they did not read the
> doc, it is better to fail them early. If we go for truncation then
> always truncating is the safest: less option -> less confusion.
>
> > And even the user space side to handle a mix of CAN frame types is
> > pretty simple IMO:
> >
> > union {
> >          struct can_frame cc;
> >          struct canfd_frame fd;
> >          struct canxl_frame xl;
> > } can;
>
> Do you want to add this union in the kernel uapi or is it just a
> userland example?

More brainstorming. If we want to introduce a generic can structure in
linux/can.h, we could  do:

struct canxl_frame {
        canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
        __u8    xl_flags; /* additional flags for CAN XL */
        __u8    fd_flags; /* CAN(-FD) flags */
        __u16   len;   /* frame payload length in byte */
        __u32   af;    /* acceptance field */
        __u8    sdt;   /* SDU (service data unit) type */
        __u8    __res0;  /* reserved / padding */
        __u8    __res1;  /* reserved / padding */
        __u8    __res2;  /* reserved / padding */
        __u8    data[CANXL_MAX_DLEN] __attribute__((aligned(8)));
};

union can_generic_frame {
         struct {
                union {
                       canid_t can_id;
                       canid_t prio;
                };
                union {
                        __u16 type;
                         struct {
                                __u8 xl_flags;
                                __u8 fd_flags;
                        } __attribute__((packed));
                } __attribute__((packed));
         };
         struct can_frame cc;
         struct canfd_frame fd;
         struct canxl_frame xl;
};

#define CANXL_XLF 0x80 /* apply to canxl_frame::xl_flags */

#define CAN_TYPE_CC 0
#ifdef __LITTLE_ENDIAN
#define CAN_TYPE_FD (CANFD_FDF << 8)
#define CAN_TYPE_XL (CANXL_XLF)
#else /* __BIG_ENDIAN */
#define CAN_TYPE_FD (CANFD_FDF)
#define CAN_TYPE_XL (CANXL_XLF << 8)
#endif

#define CAN_TYPE_MASK (CAN_TYPE_FD | CAN_TYPE_XL)

Because can_generic_frame::type overlaps with the can(fd)_frame::len,
it will contain garbage and thus it is necessary to mask it with
CAN_TYPE_MASK.
The CANFD_FDF is only set in the rx path. In the tx path it is simply
ignored. This done, we can use it as below when *receiving* can
frames:

int foo()
{
  union can_generic_frame can;

  /* receive a frame */

  switch (can.type & CAN_TYPE_MASK) {
  case CAN_TYPE_CC:
    printf("Received classical CAN Frame\n");
    break;

  case CAN_TYPE_FD:
    printf("Received CAN FD Frame\n");
    break;

  case CAN_TYPE_XL:
    printf("Received CAN XL Frame\n");
    break;

  default:
    fprintf(stderr, "Unknown type: %x\n", can.type & CAN_TYPE_MASK);
  }

  return EXIT_SUCCESS;
}


Yours sincerely,
Vincent Mailhol

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

[Oliver Hartkopp]

On 21.07.22 05:13, Vincent Mailhol wrote:
> On Thu. 21 juil. 2022 at 11:37, Vincent Mailhol
> <vincent.mailhol@gmail.com> wrote:
>> On Wed. 21 Jul. 2022 at 01:43, Oliver Hartkopp <socketcan@hartkopp.net> wrote:


>>> And even the user space side to handle a mix of CAN frame types is
>>> pretty simple IMO:
>>>
>>> union {
>>>           struct can_frame cc;
>>>           struct canfd_frame fd;
>>>           struct canxl_frame xl;
>>> } can;
>>
>> Do you want to add this union in the kernel uapi or is it just a
>> userland example?
> 
> More brainstorming. If we want to introduce a generic can structure in
> linux/can.h, we could  do:
> 
> struct canxl_frame {
>          canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
>          __u8    xl_flags; /* additional flags for CAN XL */
>          __u8    fd_flags; /* CAN(-FD) flags */
>          __u16   len;   /* frame payload length in byte */
>          __u32   af;    /* acceptance field */
>          __u8    sdt;   /* SDU (service data unit) type */
>          __u8    __res0;  /* reserved / padding */
>          __u8    __res1;  /* reserved / padding */
>          __u8    __res2;  /* reserved / padding */
>          __u8    data[CANXL_MAX_DLEN] __attribute__((aligned(8)));
> };
> 
> union can_generic_frame {
>           struct {
>                  union {
>                         canid_t can_id;
>                         canid_t prio;
>                  };
>                  union {
>                          __u16 type;
>                           struct {
>                                  __u8 xl_flags;
>                                  __u8 fd_flags;
>                          } __attribute__((packed));
>                  } __attribute__((packed));
>           };
>           struct can_frame cc;
>           struct canfd_frame fd;
>           struct canxl_frame xl;
> };
> 
> #define CANXL_XLF 0x80 /* apply to canxl_frame::xl_flags */
> 
> #define CAN_TYPE_CC 0
> #ifdef __LITTLE_ENDIAN
> #define CAN_TYPE_FD (CANFD_FDF << 8)
> #define CAN_TYPE_XL (CANXL_XLF)
> #else /* __BIG_ENDIAN */
> #define CAN_TYPE_FD (CANFD_FDF)
> #define CAN_TYPE_XL (CANXL_XLF << 8)
> #endif
> 
> #define CAN_TYPE_MASK (CAN_TYPE_FD | CAN_TYPE_XL)
> 
> Because can_generic_frame::type overlaps with the can(fd)_frame::len,
> it will contain garbage and thus it is necessary to mask it with
> CAN_TYPE_MASK.
> The CANFD_FDF is only set in the rx path. In the tx path it is simply
> ignored. This done, we can use it as below when *receiving* can
> frames:

No problem to set CANFD_FDF in raw_sendmsg() when we process a CAN FD 
frame in the tx path ...

> 
> int foo()
> {
>    union can_generic_frame can;
> 
>    /* receive a frame */
> 
>    switch (can.type & CAN_TYPE_MASK) {
>    case CAN_TYPE_CC:
>      printf("Received classical CAN Frame\n");
>      break;
> 
>    case CAN_TYPE_FD:
>      printf("Received CAN FD Frame\n");
>      break;
> 
>    case CAN_TYPE_XL:
>      printf("Received CAN XL Frame\n");
>      break;
> 
>    default:
>      fprintf(stderr, "Unknown type: %x\n", can.type & CAN_TYPE_MASK);
>    }
> 
>    return EXIT_SUCCESS;
> }
> 

If you just want to get rid of the nbytes checking and we make sure 
CANFD_FDF is properly set in the future we are not far from such an easy 
check - even without moving the sdt element or endian magic:


if (can.xl_flags & CANXL_XLF) {
     printf("Received CAN XL Frame\n");
} else if (can.fd_flags & CANFD_FDF) {
     printf("Received CAN FD Frame\n");
} else {
     printf("Received classical CAN Frame\n");
}

Regards,
Oliver

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

[Vincent Mailhol]

On Fri. 22 Jul. 2022 at 05:26, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 21.07.22 05:13, Vincent Mailhol wrote:
> > On Thu. 21 juil. 2022 at 11:37, Vincent Mailhol
> > <vincent.mailhol@gmail.com> wrote:
> >> On Wed. 21 Jul. 2022 at 01:43, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>
>
> >>> And even the user space side to handle a mix of CAN frame types is
> >>> pretty simple IMO:
> >>>
> >>> union {
> >>>           struct can_frame cc;
> >>>           struct canfd_frame fd;
> >>>           struct canxl_frame xl;
> >>> } can;
> >>
> >> Do you want to add this union in the kernel uapi or is it just a
> >> userland example?
> >
> > More brainstorming. If we want to introduce a generic can structure in
> > linux/can.h, we could  do:
> >
> > struct canxl_frame {
> >          canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
> >          __u8    xl_flags; /* additional flags for CAN XL */
> >          __u8    fd_flags; /* CAN(-FD) flags */
> >          __u16   len;   /* frame payload length in byte */
> >          __u32   af;    /* acceptance field */
> >          __u8    sdt;   /* SDU (service data unit) type */
> >          __u8    __res0;  /* reserved / padding */
> >          __u8    __res1;  /* reserved / padding */
> >          __u8    __res2;  /* reserved / padding */
> >          __u8    data[CANXL_MAX_DLEN] __attribute__((aligned(8)));
> > };
> >
> > union can_generic_frame {
> >           struct {
> >                  union {
> >                         canid_t can_id;
> >                         canid_t prio;
> >                  };
> >                  union {
> >                          __u16 type;
> >                           struct {
> >                                  __u8 xl_flags;
> >                                  __u8 fd_flags;
> >                          } __attribute__((packed));
> >                  } __attribute__((packed));
> >           };
> >           struct can_frame cc;
> >           struct canfd_frame fd;
> >           struct canxl_frame xl;
> > };
> >
> > #define CANXL_XLF 0x80 /* apply to canxl_frame::xl_flags */
> >
> > #define CAN_TYPE_CC 0
> > #ifdef __LITTLE_ENDIAN
> > #define CAN_TYPE_FD (CANFD_FDF << 8)
> > #define CAN_TYPE_XL (CANXL_XLF)
> > #else /* __BIG_ENDIAN */
> > #define CAN_TYPE_FD (CANFD_FDF)
> > #define CAN_TYPE_XL (CANXL_XLF << 8)
> > #endif
> >
> > #define CAN_TYPE_MASK (CAN_TYPE_FD | CAN_TYPE_XL)
> >
> > Because can_generic_frame::type overlaps with the can(fd)_frame::len,
> > it will contain garbage and thus it is necessary to mask it with
> > CAN_TYPE_MASK.
> > The CANFD_FDF is only set in the rx path. In the tx path it is simply
> > ignored. This done, we can use it as below when *receiving* can
> > frames:
>
> No problem to set CANFD_FDF in raw_sendmsg() when we process a CAN FD
> frame in the tx path ...

ACK.

> >
> > int foo()
> > {
> >    union can_generic_frame can;
> >
> >    /* receive a frame */
> >
> >    switch (can.type & CAN_TYPE_MASK) {
> >    case CAN_TYPE_CC:
> >      printf("Received classical CAN Frame\n");
> >      break;
> >
> >    case CAN_TYPE_FD:
> >      printf("Received CAN FD Frame\n");
> >      break;
> >
> >    case CAN_TYPE_XL:
> >      printf("Received CAN XL Frame\n");
> >      break;
> >
> >    default:
> >      fprintf(stderr, "Unknown type: %x\n", can.type & CAN_TYPE_MASK);
> >    }
> >
> >    return EXIT_SUCCESS;
> > }
> >
>
> If you just want to get rid of the nbytes checking and we make sure
> CANFD_FDF is properly set in the future we are not far from such an easy
> check - even without moving the sdt element or endian magic:

ACK. I did not like the mix between the CANXL_XLF and the CAN*_MTU
checks. Would be great to have a consistent method to check the type.

>
> if (can.xl_flags & CANXL_XLF) {
>      printf("Received CAN XL Frame\n");
> } else if (can.fd_flags & CANFD_FDF) {
>      printf("Received CAN FD Frame\n");
> } else {
>      printf("Received classical CAN Frame\n");
> }

ACK. I like this approach :)

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

[Oliver Hartkopp]

On 21.07.22 04:37, Vincent Mailhol wrote:
> On Wed. 21 Jul. 2022 at 01:43, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> On 19.07.22 17:16, Vincent Mailhol wrote:
>>> On Tue 19 Jul. 2022 at 23:38, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>>> No confusion.
>>>>
>>>> The API to the user space is 'truncated' option only.
>>>>
>>>> The data structure inside the kernel sizeof(struct can[|fd|xl]_frame).
>>>>
>>>> See:
>>>> https://lore.kernel.org/linux-can/4c79233f-1301-d5c7-7698-38d39d8702aa@hartkopp.net/
>>>>
>>>> ---8<---
>>>>
>>>> As the sk_buffs are only allocated once and are not copied inside the
>>>> kernel there should be no remarkable drawbacks whether we allocate
>>>> CAN_MTU skbs or CANXL_MTU skbs.
>>>>
>>>> AFAICS both skbs will fit into a single page.
>>>
>>> This is true. A page is at least 4k. So the 2k + alpha will easily fit.
>>> But the page is not the smallest chunk that can return malloc, c.f.
>>> KMALLOC_MIN_SIZE:
>>> https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h#L279
>>>
>>> Also, more than the page size, my concern are the cache misses,
>>> especially when memsetting to zero the full canxl frame. As far as I
>>> understand, cloning an skb does not copy the payload (only increment a
>>> reference to it) so the echo_skb thing should not be impacted.
>>> That said, I am not able to tell whether or not this will have a
>>> noticeable impact (I have some feeling it might but can not assert
>>> it). If this looks good for the people who have the expertise in
>>> kernel performances, then I am fine.
>>
>> The more I think about our discussion and your remark that we were
>> somehow going back to the V2 patch set the more I wonder if this would
>> be a good idea.
> 
> I quite liked v2. My comments on the v2 were mostly to argue on the
> data[CANXL_MAX_DLEN] vs the flexible member array, but aside from
> that, it looked pretty good.
> 
>> IMO using the struct canxl_frame (including 2048 byte) and allowing
>> truncated sizes can be handled inside the kernel safely.
>>
>> And with V2 we only allocate the needed size for the sk_buff - without
>> any memsetting.
>>
>> When user space gets a truncated frame -> fine
>>
>> When the user forges some CAN XL frame where the length value does not
>> match the spec and the size does not fit the length -> -EINVAL
>>
>> So there is no uninitialized data also.
> 
> So basically, forcing the truncation everywhere (TX, RX both userland
> and kernel), correct? i.e. the skb length shall always be equal to
> CANXL_HEADER_SIZE + canxl_frame::len.

Yes!

Btw. How should we finally name the 'non data' header of CAN XL?

1. CANXL_HEADER_SIZE
2. CANXL_HEAD_SIZE
3. CANXL_HDR_SIZE
4. CANXL_HDR_SZ <- currently in the patches
5. CANXL_HD_SZ

I think it has to be 'head' and not 'header'.

In skbs we also have head and tail.

So I would vote for 2 or 5 with a tendency to 5.

> I think this is good. As I stated before, getting -EINVAL is benign.
> If developers are doing crazy things because they did not read the
> doc, it is better to fail them early. If we go for truncation then
> always truncating is the safest: less option -> less confusion.

ACK

>> And even the user space side to handle a mix of CAN frame types is
>> pretty simple IMO:
>>
>> union {
>>           struct can_frame cc;
>>           struct canfd_frame fd;
>>           struct canxl_frame xl;
>> } can;
> 
> Do you want to add this union in the kernel uapi or is it just a
> userland example?

Yes, that was just a userland example to read and write some CAN XL 
frames along with CAN/CANFD frames with the 'new' CAN_RAW socket.

I just wanted to get an impression whether it is handy to program this 
extended API or not.

>> nbytes = read(s, &can.xl, sizeof(struct canxl_frame));
>> if (nbytes < 0) {
>>           perror("read");
>>           return 1;
>> }
>> printf("nbytes = %d\n", nbytes);
> 
> Does read() guarantee atomicity? From "man 2 read":
> | It is not an error if [the return value] is smaller than the number
> | of bytes requested; this may happen for example because fewer bytes
> | are actually available right now (maybe because we were close to
> | end-of-file, or because we are reading from a pipe, or from a
> | terminal), *or because read() was interrupted by a signal*.
> 
> I think the answer is yes, but I prefer to double check (I am
> especially concerned by the signal interrupts).

Hm, we are not reading from a file but from a socket here that provide 
chunks in the form of struct can_frame in raw_recvmsg(). You only get a 
MSG_TRUNC there when you provide a (buffer)size in userspace that's to 
small.

I've never got any error reports on CAN_RAW reading (over 16 years) and 
all the examples contain a test for sizeof(struct can_frame) like this:

 >> if (nbytes != sizeof(struct can_frame) &&
 >>       nbytes != sizeof(struct canfd_frame)) {

So we either have an error or an incomplete CAN frame which becomes an 
error too.

Do you think this is still worth an investigation?

>>
>> if (nbytes < CANXL_HDR_SZ + CANXL_MIN_DLEN) {
>>           fprintf(stderr, "read: no CAN frame\n");
>>           return 1;
>> }
>>
>> if (can.xl.flags & CANXL_XLF) {
>>            if (nbytes != CANXL_HDR_SZ + can.xl.len) {
>>                   printf("nbytes = %d\n", nbytes);
>>                   perror("read canxl_frame");
> 
> ACK.

When we checked for a complete header this really seems to be simple. 
And we directly prove CANXL_MIN_DLEN and CANXL_MAX_DLEN is ensured by 
read().

> 
>>                   continue;
>>            }
>>            /* process CAN XL frames */
>>            printf("canxl frame prio %03X len %d flags %d\n",
>>                    can.xl.prio, can.xl.len, can.xl.flags);
>>            continue;
>> }
>>
>> if (nbytes != sizeof(struct can_frame) &&
>>       nbytes != sizeof(struct canfd_frame)) {
> 
> On the first read, I thought you meant "else if", then, I saw the
> "continue" on the previous line.

Yes. I just wanted to look if I got a CAN XL frame and print its attributes.

>>           fprintf(stderr, "read: incomplete CAN(FD) frame\n");
>>           return 1;
>> }
>>
>> /* process CAN/FD frames */
>>
>> Working with partially filled data structures is a known pattern for CAN
>> and CAN FD.
> 
> Except that for CAN-(FD), truncation is not possible.

Exactly. CAN/FD uses the fixed structure sizes to distinguish the frame 
types.

>> We only optimize the transfer from/to kernel space with truncated
>> read/write operations.
>>
>> ¯\_(ツ)_/¯
> 
> Yes, this full discussion is about optimizations…

Optimization and STYLE ;-)

Thanks for your feedback!

Best regards,
Oliver

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

[Marc Kleine-Budde]

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

On 21.07.2022 09:36:21, Oliver Hartkopp wrote:
> Btw. How should we finally name the 'non data' header of CAN XL?
> 
> 1. CANXL_HEADER_SIZE
> 2. CANXL_HEAD_SIZE
> 3. CANXL_HDR_SIZE
> 4. CANXL_HDR_SZ <- currently in the patches
> 5. CANXL_HD_SZ
> 
> I think it has to be 'head' and not 'header'.

Header! Header is in front of data.

> In skbs we also have head and tail.

Yes, but they point at the head or tail of the buffer allocated with the
skb.

> So I would vote for 2 or 5 with a tendency to 5.

3, 1, 4

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 v5 3/5] can: dev: add CAN XL support

[Vincent Mailhol]

On Thu. 21 Jul. 2022 at 16:53, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 21.07.2022 09:36:21, Oliver Hartkopp wrote:
> > Btw. How should we finally name the 'non data' header of CAN XL?
> >
> > 1. CANXL_HEADER_SIZE
> > 2. CANXL_HEAD_SIZE
> > 3. CANXL_HDR_SIZE
> > 4. CANXL_HDR_SZ <- currently in the patches
> > 5. CANXL_HD_SZ
> >
> > I think it has to be 'head' and not 'header'.
>
> Header! Header is in front of data.

I am also part of the header team! By analogy with:
https://en.wikipedia.org/wiki/IPv4#Header

> > In skbs we also have head and tail.
>
> Yes, but they point at the head or tail of the buffer allocated with the
> skb.
>
> > So I would vote for 2 or 5 with a tendency to 5.
>
> 3, 1, 4

My top vote goes to:
6. No macro, instead use flexible array member and do sizeof(struct canxl_frame)

I do not like the SZ abbreviation either, so my next choices will be 3 then 1.

To recap: 6, 3, 1.

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

[Oliver Hartkopp]

On 21.07.22 10:14, Vincent Mailhol wrote:
> On Thu. 21 Jul. 2022 at 16:53, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>> On 21.07.2022 09:36:21, Oliver Hartkopp wrote:
>>> Btw. How should we finally name the 'non data' header of CAN XL?
>>>
>>> 1. CANXL_HEADER_SIZE
>>> 2. CANXL_HEAD_SIZE
>>> 3. CANXL_HDR_SIZE
>>> 4. CANXL_HDR_SZ <- currently in the patches
>>> 5. CANXL_HD_SZ
>>>
>>> I think it has to be 'head' and not 'header'.
>>
>> Header! Header is in front of data.
> 
> I am also part of the header team! By analogy with:
> https://en.wikipedia.org/wiki/IPv4#Header
> 
>>> In skbs we also have head and tail.
>>
>> Yes, but they point at the head or tail of the buffer allocated with the
>> skb.
>>
>>> So I would vote for 2 or 5 with a tendency to 5.
>>
>> 3, 1, 4
> 
> My top vote goes to:
> 6. No macro, instead use flexible array member and do sizeof(struct canxl_frame)

This is no sizeof(struct canxl_frame) anymore with the use of a flexible 
array because a valid "CAN XL frame" has data (at least one byte and up 
to 2048 byte).

You might name it

struct canxl_header {
         canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
         __u8    flags; /* additional flags for CAN XL */
         __u8    sdt;   /* SDU (service data unit) type */
         __u16   len;   /* frame payload length in byte */
         __u32   af;    /* acceptance field */
         __u8    data[];
};

But then you can't build a struct canxl_frame anymore in the way that 
you can access the elements as you can do it today:

struct canxl_frame {
          struct canxl_header xldhr;
          data[CANXL_MAX_DLEN];
};

struct canxl_frame cfx;

=> cfx.xlhdr.len

Which is not cfx.len anymore what is a known pattern from struct 
can[fd]_frame from CAN application programmers and simple to use.

The only new thing is the possibility to handle a truncated data[] 
section. That should be feasible to learn.

> I do not like the SZ abbreviation either, so my next choices will be 3 then 1.
> 
> To recap: 6, 3, 1.

Then CANXL_HDR_SIZE wins :-)

Regards,
Oliver

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

[Vincent Mailhol]

On Fri. 22 Jul. 2022 at 04:10, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 21.07.22 10:14, Vincent Mailhol wrote:
> > On Thu. 21 Jul. 2022 at 16:53, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >> On 21.07.2022 09:36:21, Oliver Hartkopp wrote:
> >>> Btw. How should we finally name the 'non data' header of CAN XL?
> >>>
> >>> 1. CANXL_HEADER_SIZE
> >>> 2. CANXL_HEAD_SIZE
> >>> 3. CANXL_HDR_SIZE
> >>> 4. CANXL_HDR_SZ <- currently in the patches
> >>> 5. CANXL_HD_SZ
> >>>
> >>> I think it has to be 'head' and not 'header'.
> >>
> >> Header! Header is in front of data.
> >
> > I am also part of the header team! By analogy with:
> > https://en.wikipedia.org/wiki/IPv4#Header
> >
> >>> In skbs we also have head and tail.
> >>
> >> Yes, but they point at the head or tail of the buffer allocated with the
> >> skb.
> >>
> >>> So I would vote for 2 or 5 with a tendency to 5.
> >>
> >> 3, 1, 4
> >
> > My top vote goes to:
> > 6. No macro, instead use flexible array member and do sizeof(struct canxl_frame)
>
> This is no sizeof(struct canxl_frame) anymore with the use of a flexible
> array because a valid "CAN XL frame" has data (at least one byte and up
> to 2048 byte).
>
> You might name it
>
> struct canxl_header {
>          canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
>          __u8    flags; /* additional flags for CAN XL */
>          __u8    sdt;   /* SDU (service data unit) type */
>          __u16   len;   /* frame payload length in byte */
>          __u32   af;    /* acceptance field */
>          __u8    data[];
> };
>
> But then you can't build a struct canxl_frame anymore in the way that
> you can access the elements as you can do it today:
>
> struct canxl_frame {
>           struct canxl_header xldhr;
>           data[CANXL_MAX_DLEN];
> };
>
> struct canxl_frame cfx;
>
> => cfx.xlhdr.len
>
> Which is not cfx.len anymore what is a known pattern from struct
> can[fd]_frame from CAN application programmers and simple to use.
>
> The only new thing is the possibility to handle a truncated data[]
> section. That should be feasible to learn.

I do not buy your argument that you can not do sizeof(struct
canxl_frame) because of naming.

With a flexible array member, I can do:

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

void foo (int s)
{
         struct canxl_frame cxl_hdr = { 0 };
         struct canxl_frame *cxl1, *cxl2;
         size_t cxl2_data_len, cxl2_frame_len;

         /* read header */
         assert(read(s, &cxl_hdr, sizeof(cxl_hdr)) == sizeof(cxl_hdr));
         cxl1 = malloc(sizeof(*cxl1) + cxl_hdr.len);
         memcpy(cxl1, &cxl_hdr, sizeof(cxl_hdr));
         /* read remaining data */
         assert(read(s, cxl1->data, cxl1->len) == cxl1->len);

         cxl2_data_len = 512; /* arbitrary value for the example */
         cxl2_frame_len = sizeof(*cxl2) + cxl2_data_len;
         cxl2 = malloc(cxl2_frame_len);
         memset(cxl2, cxl2_frame_len, 0);
         cxl2->len = cxl2_data_len;
         cxl2->flags = CANXL_XLF;
         /* fill prio, data, ect... */
         assert(write(s, cxl2, cxl2_frame_len) == cxl2_frame_len);
}

This is not a fantasy from myself. Flexible array members are used a
lot in the uapi and do not define a XXX_HDR_SIZE macro.

I can send again the exemple from ethtool which I shared before:
  * kernel definition:
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1163
  * userland code:
https://kernel.googlesource.com/pub/scm/network/ethtool/ethtool/+/refs/heads/ethtool-3.4.y/ethtool.c#2989

In the userland code of the above example, you can see that the same
structure (without a "header" suffix) is used to manage both headers
and full messages. Extract:
struct ethtool_rxfh_indir indir_head;
struct ethtool_rxfh_indir *indir;

Even the ones which add the "header" suffix do not declare the
XXX_HDR_SIZE macro. Example: struct ip_auth_hdr from linux/ip.h:
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ip.h#L109
Furthermore, in this struct ip_auth_hdr, the auth_data field is
specified to be at least 4 bytes, which clearly contradict your
argument that "This is no sizeof(struct canxl_frame) anymore with the
use of a flexible array because a valid CAN XL frame has data".

The flexible array member is the standard method to handle structure
of variable length. And by standard, I literally mean so (c.f. ISO
C99, section 6.7.2.1 paragraph 16). And people have been happily using
sizeof(struct foo) to get the header size for decades. I do not see
why canxl_frame should be different.

This is basically what I meant by saying that the flexible array
members are more idiomatic. It is closer to what other people have
been doing in the uapi so far. As you said, CANXL is closer to
Ethernet than CAN. So I do not see the point to force some
similarities between the struct can(fd)_frame and the struct
canxl_frame.

I looked for counterexamples in the uapi headers that would prove me
wrong, but didn't find any.


Yours sincerely,
Vincent Mailhol

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

[Marc Kleine-Budde]

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

On 22.07.2022 12:20:43, Vincent Mailhol wrote:
> I do not buy your argument that you can not do sizeof(struct
> canxl_frame) because of naming.
> 
> With a flexible array member, I can do:
> 
> struct canxl_frame {
>          canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
>          __u8    flags; /* additional flags for CAN XL */
>          __u8    sdt;   /* SDU (service data unit) type */
>          __u16   len;   /* frame payload length in byte */
>          __u32   af;    /* acceptance field */
>          __u8    data[];
> };
> 
> void foo (int s)
> {
>          struct canxl_frame cxl_hdr = { 0 };
>          struct canxl_frame *cxl1, *cxl2;
>          size_t cxl2_data_len, cxl2_frame_len;
> 
>          /* read header */
>          assert(read(s, &cxl_hdr, sizeof(cxl_hdr)) == sizeof(cxl_hdr));
>          cxl1 = malloc(sizeof(*cxl1) + cxl_hdr.len);
>          memcpy(cxl1, &cxl_hdr, sizeof(cxl_hdr));
>          /* read remaining data */
>          assert(read(s, cxl1->data, cxl1->len) == cxl1->len);

For performance reasons you probable don't want to split the read of a
single CAN frame over 2 read() syscalls. Also the CAN_RAW doesn't
support "split"-read now, but could be extended.

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 v5 3/5] can: dev: add CAN XL support

[Vincent Mailhol]

On Fri. 22 Jul. 2022 at 16:27, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 22.07.2022 12:20:43, Vincent Mailhol wrote:
> > I do not buy your argument that you can not do sizeof(struct
> > canxl_frame) because of naming.
> >
> > With a flexible array member, I can do:
> >
> > struct canxl_frame {
> >          canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
> >          __u8    flags; /* additional flags for CAN XL */
> >          __u8    sdt;   /* SDU (service data unit) type */
> >          __u16   len;   /* frame payload length in byte */
> >          __u32   af;    /* acceptance field */
> >          __u8    data[];
> > };
> >
> > void foo (int s)
> > {
> >          struct canxl_frame cxl_hdr = { 0 };
> >          struct canxl_frame *cxl1, *cxl2;
> >          size_t cxl2_data_len, cxl2_frame_len;
> >
> >          /* read header */
> >          assert(read(s, &cxl_hdr, sizeof(cxl_hdr)) == sizeof(cxl_hdr));
> >          cxl1 = malloc(sizeof(*cxl1) + cxl_hdr.len);
> >          memcpy(cxl1, &cxl_hdr, sizeof(cxl_hdr));
> >          /* read remaining data */
> >          assert(read(s, cxl1->data, cxl1->len) == cxl1->len);
>
> For performance reasons you probable don't want to split the read of a
> single CAN frame over 2 read() syscalls.

ACK. I wrote the code to illustrate how to do header manipulations.
The goal of this example was not to be optimal but to show off how
sizeof(struct canxl_frame) could be used. Sorry if the example was not
natural and a bit too forced.

> Also the CAN_RAW doesn't
> support "split"-read now, but could be extended.

Interesting! I naively thought that split read() was handled at a
higher level of the socket API. I did not know that it was per
protocol.

It could make sense to allow such split read() for CANXL. One example
I can directly think of is the Packet API. Correct me if I am wrong
but if writing generic code to dump packets on any interfaces, you do
not know in advance the MTU. And so, I guess you have to provide a
buffer of an arbitrary size. A generic program using a buffer of, let
say, 2048 bytes (one page) will not be enough to get a CANXL frame in
one single shot.

Yours sincerely,
Vincent Mailhol

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

[Marc Kleine-Budde]

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

On 22.07.2022 17:33:08, Vincent Mailhol wrote:
> On Fri. 22 Jul. 2022 at 16:27, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 22.07.2022 12:20:43, Vincent Mailhol wrote:
> > > I do not buy your argument that you can not do sizeof(struct
> > > canxl_frame) because of naming.
> > >
> > > With a flexible array member, I can do:
> > >
> > > struct canxl_frame {
> > >          canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
> > >          __u8    flags; /* additional flags for CAN XL */
> > >          __u8    sdt;   /* SDU (service data unit) type */
> > >          __u16   len;   /* frame payload length in byte */
> > >          __u32   af;    /* acceptance field */
> > >          __u8    data[];
> > > };
> > >
> > > void foo (int s)
> > > {
> > >          struct canxl_frame cxl_hdr = { 0 };
> > >          struct canxl_frame *cxl1, *cxl2;
> > >          size_t cxl2_data_len, cxl2_frame_len;
> > >
> > >          /* read header */
> > >          assert(read(s, &cxl_hdr, sizeof(cxl_hdr)) == sizeof(cxl_hdr));
> > >          cxl1 = malloc(sizeof(*cxl1) + cxl_hdr.len);
> > >          memcpy(cxl1, &cxl_hdr, sizeof(cxl_hdr));
> > >          /* read remaining data */
> > >          assert(read(s, cxl1->data, cxl1->len) == cxl1->len);
> >
> > For performance reasons you probable don't want to split the read of a
> > single CAN frame over 2 read() syscalls.
> 
> ACK. I wrote the code to illustrate how to do header manipulations.
> The goal of this example was not to be optimal but to show off how
> sizeof(struct canxl_frame) could be used. Sorry if the example was not
> natural and a bit too forced.
> 
> > Also the CAN_RAW doesn't
> > support "split"-read now, but could be extended.
> 
> Interesting! I naively thought that split read() was handled at a
> higher level of the socket API. I did not know that it was per
> protocol.

The CAN_RAW protocol implements raw_recvmsg():

| https://elixir.bootlin.com/linux/latest/source/net/can/raw.c#L843

> It could make sense to allow such split read() for CANXL.

Then we have to track the number of already read bytes inside the
socket. The POSIX API offers some interesting flags to recvmsg(). With
MSG_PEEK you can read but not remove the read data from the queue and/or
MSG_TRUNC to get the total size of the CAN frame.

I have not tested these flags, but I think support for them has to be
implemented in CAN_RAW, too.

Also, we should have a look at the UDP code.

> One example
> I can directly think of is the Packet API. Correct me if I am wrong
> but if writing generic code to dump packets on any interfaces, you do
> not know in advance the MTU. And so, I guess you have to provide a
> buffer of an arbitrary size. A generic program using a buffer of, let
> say, 2048 bytes (one page) will not be enough to get a CANXL frame in
> one single shot.

Nitpick: the page size is arch and/or kernel config dependent and the
smallest page size that Linux supports is 4k.

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 v5 3/5] can: dev: add CAN XL support

[Vincent Mailhol]

On Fri. 22 Jul. 2022 at 18:58, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 22.07.2022 17:33:08, Vincent Mailhol wrote:
> > On Fri. 22 Jul. 2022 at 16:27, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > On 22.07.2022 12:20:43, Vincent Mailhol wrote:
> > > > I do not buy your argument that you can not do sizeof(struct
> > > > canxl_frame) because of naming.
> > > >
> > > > With a flexible array member, I can do:
> > > >
> > > > struct canxl_frame {
> > > >          canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
> > > >          __u8    flags; /* additional flags for CAN XL */
> > > >          __u8    sdt;   /* SDU (service data unit) type */
> > > >          __u16   len;   /* frame payload length in byte */
> > > >          __u32   af;    /* acceptance field */
> > > >          __u8    data[];
> > > > };
> > > >
> > > > void foo (int s)
> > > > {
> > > >          struct canxl_frame cxl_hdr = { 0 };
> > > >          struct canxl_frame *cxl1, *cxl2;
> > > >          size_t cxl2_data_len, cxl2_frame_len;
> > > >
> > > >          /* read header */
> > > >          assert(read(s, &cxl_hdr, sizeof(cxl_hdr)) == sizeof(cxl_hdr));
> > > >          cxl1 = malloc(sizeof(*cxl1) + cxl_hdr.len);
> > > >          memcpy(cxl1, &cxl_hdr, sizeof(cxl_hdr));
> > > >          /* read remaining data */
> > > >          assert(read(s, cxl1->data, cxl1->len) == cxl1->len);
> > >
> > > For performance reasons you probable don't want to split the read of a
> > > single CAN frame over 2 read() syscalls.
> >
> > ACK. I wrote the code to illustrate how to do header manipulations.
> > The goal of this example was not to be optimal but to show off how
> > sizeof(struct canxl_frame) could be used. Sorry if the example was not
> > natural and a bit too forced.
> >
> > > Also the CAN_RAW doesn't
> > > support "split"-read now, but could be extended.
> >
> > Interesting! I naively thought that split read() was handled at a
> > higher level of the socket API. I did not know that it was per
> > protocol.
>
> The CAN_RAW protocol implements raw_recvmsg():
>
> | https://elixir.bootlin.com/linux/latest/source/net/can/raw.c#L843
>
> > It could make sense to allow such split read() for CANXL.
>
> Then we have to track the number of already read bytes inside the
> socket. The POSIX API offers some interesting flags to recvmsg(). With
> MSG_PEEK you can read but not remove the read data from the queue and/or
> MSG_TRUNC to get the total size of the CAN frame.
>
> I have not tested these flags, but I think support for them has to be
> implemented in CAN_RAW, too.
>
> Also, we should have a look at the UDP code.

Here it is:
https://elixir.bootlin.com/linux/latest/source/net/ipv4/udp.c#L1846

The relevant function is sk_peek_offset:
https://elixir.bootlin.com/linux/latest/source/include/net/sock.h#L617

And to finish, there is an nearly eponym field in struct sock: sk_peek_off
https://elixir.bootlin.com/linux/latest/source/include/net/sock.h#L457

> > One example
> > I can directly think of is the Packet API. Correct me if I am wrong
> > but if writing generic code to dump packets on any interfaces, you do
> > not know in advance the MTU. And so, I guess you have to provide a
> > buffer of an arbitrary size. A generic program using a buffer of, let
> > say, 2048 bytes (one page) will not be enough to get a CANXL frame in
> > one single shot.
>
> Nitpick: the page size is arch and/or kernel config dependent and the
> smallest page size that Linux supports is 4k.

Arg, I did not have enough sleep.

By the way, do you have a preference between the flexible array member
and the data[CANXL_MAX_DLEN]? So far, I have been pushing the idea of
the flexible array member but if I am the only one to support this
idea, we can call it a day and use the data[CANXL_MAX_DLEN] approach.


Yours sincerely,
Vincent Mailhol

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

[Oliver Hartkopp]

On 22.07.22 12:54, Vincent Mailhol wrote:
> On Fri. 22 Jul. 2022 at 18:58, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>> On 22.07.2022 17:33:08, Vincent Mailhol wrote:
>>> On Fri. 22 Jul. 2022 at 16:27, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>>> On 22.07.2022 12:20:43, Vincent Mailhol wrote:
>>>>> I do not buy your argument that you can not do sizeof(struct
>>>>> canxl_frame) because of naming.
>>>>>
>>>>> With a flexible array member, I can do:
>>>>>
>>>>> struct canxl_frame {
>>>>>           canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
>>>>>           __u8    flags; /* additional flags for CAN XL */
>>>>>           __u8    sdt;   /* SDU (service data unit) type */
>>>>>           __u16   len;   /* frame payload length in byte */
>>>>>           __u32   af;    /* acceptance field */
>>>>>           __u8    data[];
>>>>> };
>>>>>
>>>>> void foo (int s)
>>>>> {
>>>>>           struct canxl_frame cxl_hdr = { 0 };
>>>>>           struct canxl_frame *cxl1, *cxl2;
>>>>>           size_t cxl2_data_len, cxl2_frame_len;
>>>>>
>>>>>           /* read header */
>>>>>           assert(read(s, &cxl_hdr, sizeof(cxl_hdr)) == sizeof(cxl_hdr));
>>>>>           cxl1 = malloc(sizeof(*cxl1) + cxl_hdr.len);
>>>>>           memcpy(cxl1, &cxl_hdr, sizeof(cxl_hdr));
>>>>>           /* read remaining data */
>>>>>           assert(read(s, cxl1->data, cxl1->len) == cxl1->len);
>>>>
>>>> For performance reasons you probable don't want to split the read of a
>>>> single CAN frame over 2 read() syscalls.

Yes, two read() syscalls (with testing/asserting for the right size), 
one malloc() syscall, one memcpy().

Alternatively I can offer ONE syscall with ONE single copy operation 
from kernel to userspace \o/

read(s, &can.xl, sizeof(struct canxl_frame));

The more I read about splitting/peeking/testing, calculating of sizes, 
etc the more I like the simple struct canxl_frame with 2048 bytes of 
data that is specified to be truncated by definition.

E.g. for candump or other usual CAN applications you can run with *one* 
static struct without any malloc(). This is not only a bridge for 
application programmers that have experiences in programming SocketCAN 
applications - it is just a safe and simple pattern that I would not 
like to give up for which improvement?

In fact I don't know any SocketCAN application that uses malloc for CAN 
frames which likely introduces lags and affects the performance.

Best regards,
Oliver

>>>
>>> ACK. I wrote the code to illustrate how to do header manipulations.
>>> The goal of this example was not to be optimal but to show off how
>>> sizeof(struct canxl_frame) could be used. Sorry if the example was not
>>> natural and a bit too forced.
>>>
>>>> Also the CAN_RAW doesn't
>>>> support "split"-read now, but could be extended.
>>>
>>> Interesting! I naively thought that split read() was handled at a
>>> higher level of the socket API. I did not know that it was per
>>> protocol.
>>
>> The CAN_RAW protocol implements raw_recvmsg():
>>
>> | https://elixir.bootlin.com/linux/latest/source/net/can/raw.c#L843
>>
>>> It could make sense to allow such split read() for CANXL.
>>
>> Then we have to track the number of already read bytes inside the
>> socket. The POSIX API offers some interesting flags to recvmsg(). With
>> MSG_PEEK you can read but not remove the read data from the queue and/or
>> MSG_TRUNC to get the total size of the CAN frame.
>>
>> I have not tested these flags, but I think support for them has to be
>> implemented in CAN_RAW, too.
>>
>> Also, we should have a look at the UDP code.
> 
> Here it is:
> https://elixir.bootlin.com/linux/latest/source/net/ipv4/udp.c#L1846
> 
> The relevant function is sk_peek_offset:
> https://elixir.bootlin.com/linux/latest/source/include/net/sock.h#L617
> 
> And to finish, there is an nearly eponym field in struct sock: sk_peek_off
> https://elixir.bootlin.com/linux/latest/source/include/net/sock.h#L457
> 
>>> One example
>>> I can directly think of is the Packet API. Correct me if I am wrong
>>> but if writing generic code to dump packets on any interfaces, you do
>>> not know in advance the MTU. And so, I guess you have to provide a
>>> buffer of an arbitrary size. A generic program using a buffer of, let
>>> say, 2048 bytes (one page) will not be enough to get a CANXL frame in
>>> one single shot.
>>
>> Nitpick: the page size is arch and/or kernel config dependent and the
>> smallest page size that Linux supports is 4k.
> 
> Arg, I did not have enough sleep.
> 
> By the way, do you have a preference between the flexible array member
> and the data[CANXL_MAX_DLEN]? So far, I have been pushing the idea of
> the flexible array member but if I am the only one to support this
> idea, we can call it a day and use the data[CANXL_MAX_DLEN] approach.
> 
> 
> Yours sincerely,
> Vincent Mailhol

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

[Vincent Mailhol]

On Sat. 23 Jul. 2022 at 00:30, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 22.07.22 12:54, Vincent Mailhol wrote:
> > On Fri. 22 Jul. 2022 at 18:58, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >> On 22.07.2022 17:33:08, Vincent Mailhol wrote:
> >>> On Fri. 22 Jul. 2022 at 16:27, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >>>> On 22.07.2022 12:20:43, Vincent Mailhol wrote:
> >>>>> I do not buy your argument that you can not do sizeof(struct
> >>>>> canxl_frame) because of naming.
> >>>>>
> >>>>> With a flexible array member, I can do:
> >>>>>
> >>>>> struct canxl_frame {
> >>>>>           canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
> >>>>>           __u8    flags; /* additional flags for CAN XL */
> >>>>>           __u8    sdt;   /* SDU (service data unit) type */
> >>>>>           __u16   len;   /* frame payload length in byte */
> >>>>>           __u32   af;    /* acceptance field */
> >>>>>           __u8    data[];
> >>>>> };
> >>>>>
> >>>>> void foo (int s)
> >>>>> {
> >>>>>           struct canxl_frame cxl_hdr = { 0 };
> >>>>>           struct canxl_frame *cxl1, *cxl2;
> >>>>>           size_t cxl2_data_len, cxl2_frame_len;
> >>>>>
> >>>>>           /* read header */
> >>>>>           assert(read(s, &cxl_hdr, sizeof(cxl_hdr)) == sizeof(cxl_hdr));
> >>>>>           cxl1 = malloc(sizeof(*cxl1) + cxl_hdr.len);
> >>>>>           memcpy(cxl1, &cxl_hdr, sizeof(cxl_hdr));
> >>>>>           /* read remaining data */
> >>>>>           assert(read(s, cxl1->data, cxl1->len) == cxl1->len);
> >>>>
> >>>> For performance reasons you probable don't want to split the read of a
> >>>> single CAN frame over 2 read() syscalls.
>
> Yes, two read() syscalls (with testing/asserting for the right size),
> one malloc() syscall, one memcpy().
>
> Alternatively I can offer ONE syscall with ONE single copy operation
> from kernel to userspace \o/
>
> read(s, &can.xl, sizeof(struct canxl_frame));

You just ignored what I wrote just below:
| I wrote the code to illustrate how to do header
| manipulations. The goal of this example was not to be optimal
| but to show off how sizeof(struct canxl_frame) could be used.

I was answering your "This is no sizeof(struct canxl_frame) anymore" comment.

You can do:
| struct canxl_frame *cxl = malloc(CANXL_MTU);
| read(s, cxl, CANXL_MTU);

In fact, the sizeof() will be mostly useful in the tx path when doing
write(), not in the rx when you will likely always provide a buffer of
maximum size. Now I regret sending my example.

> The more I read about splitting/peeking/testing, calculating of sizes,
> etc the more I like the simple struct canxl_frame with 2048 bytes of
> data that is specified to be truncated by definition.
>
> E.g. for candump or other usual CAN applications you can run with *one*
> static struct without any malloc(). This is not only a bridge for
> application programmers that have experiences in programming SocketCAN
> applications - it is just a safe and simple pattern that I would not
> like to give up for which improvement?

The possibility to do static declaration is the strongest argument in
your favor.

There is no easy way to do this with flexible array member aside maybe
of the convoluted:
| char buf[CANXL_MTU];
| struct canxl_frame *cxl = &(struct canxl_frame)buf;

But I do not see the problem of using dynamic memory. For a 2
kilobytes buffer, dynamic memory looks pretty standard to me. And one
more time, is there any precedent in the kernel uapi of not using
flexible array members for structures meant to hold more than one
kilobyte? For the tiny CAN(-FD) it made sense, the same argument does
not translate so easily when going from ~64B to ~2KB.

My main point is that your approach does not follow what I could
witness so far in the UAPI. You have not yet answered this point.

> In fact I don't know any SocketCAN application that uses malloc for CAN
> frames which likely introduces lags and affects the performance.

This argument is invalid. You can malloc() once at the beginning of
the program and reuse it for the entire lifetime (please forget my
previous code snippet which was not meant to be performant).
There is no way that a single malloc can introduce noticeable lags
compared to a static allocation.

> >>>
> >>> ACK. I wrote the code to illustrate how to do header manipulations.
> >>> The goal of this example was not to be optimal but to show off how
> >>> sizeof(struct canxl_frame) could be used. Sorry if the example was not
> >>> natural and a bit too forced.
> >>>
> >>>> Also the CAN_RAW doesn't
> >>>> support "split"-read now, but could be extended.
> >>>
> >>> Interesting! I naively thought that split read() was handled at a
> >>> higher level of the socket API. I did not know that it was per
> >>> protocol.
> >>
> >> The CAN_RAW protocol implements raw_recvmsg():
> >>
> >> | https://elixir.bootlin.com/linux/latest/source/net/can/raw.c#L843
> >>
> >>> It could make sense to allow such split read() for CANXL.
> >>
> >> Then we have to track the number of already read bytes inside the
> >> socket. The POSIX API offers some interesting flags to recvmsg(). With
> >> MSG_PEEK you can read but not remove the read data from the queue and/or
> >> MSG_TRUNC to get the total size of the CAN frame.
> >>
> >> I have not tested these flags, but I think support for them has to be
> >> implemented in CAN_RAW, too.
> >>
> >> Also, we should have a look at the UDP code.
> >
> > Here it is:
> > https://elixir.bootlin.com/linux/latest/source/net/ipv4/udp.c#L1846
> >
> > The relevant function is sk_peek_offset:
> > https://elixir.bootlin.com/linux/latest/source/include/net/sock.h#L617
> >
> > And to finish, there is an nearly eponym field in struct sock: sk_peek_off
> > https://elixir.bootlin.com/linux/latest/source/include/net/sock.h#L457
> >
> >>> One example
> >>> I can directly think of is the Packet API. Correct me if I am wrong
> >>> but if writing generic code to dump packets on any interfaces, you do
> >>> not know in advance the MTU. And so, I guess you have to provide a
> >>> buffer of an arbitrary size. A generic program using a buffer of, let
> >>> say, 2048 bytes (one page) will not be enough to get a CANXL frame in
> >>> one single shot.
> >>
> >> Nitpick: the page size is arch and/or kernel config dependent and the
> >> smallest page size that Linux supports is 4k.
> >
> > Arg, I did not have enough sleep.
> >
> > By the way, do you have a preference between the flexible array member
> > and the data[CANXL_MAX_DLEN]? So far, I have been pushing the idea of
> > the flexible array member but if I am the only one to support this
> > idea, we can call it a day and use the data[CANXL_MAX_DLEN] approach.
> >
> >
> > Yours sincerely,
> > Vincent Mailhol

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

[Oliver Hartkopp]

On 22.07.22 18:32, Vincent Mailhol wrote:
> On Sat. 23 Jul. 2022 at 00:30, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> On 22.07.22 12:54, Vincent Mailhol wrote:
>>> On Fri. 22 Jul. 2022 at 18:58, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>>> On 22.07.2022 17:33:08, Vincent Mailhol wrote:
>>>>> On Fri. 22 Jul. 2022 at 16:27, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>>>>> On 22.07.2022 12:20:43, Vincent Mailhol wrote:
>>>>>>> I do not buy your argument that you can not do sizeof(struct
>>>>>>> canxl_frame) because of naming.
>>>>>>>
>>>>>>> With a flexible array member, I can do:
>>>>>>>
>>>>>>> struct canxl_frame {
>>>>>>>            canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
>>>>>>>            __u8    flags; /* additional flags for CAN XL */
>>>>>>>            __u8    sdt;   /* SDU (service data unit) type */
>>>>>>>            __u16   len;   /* frame payload length in byte */
>>>>>>>            __u32   af;    /* acceptance field */
>>>>>>>            __u8    data[];
>>>>>>> };
>>>>>>>
>>>>>>> void foo (int s)
>>>>>>> {
>>>>>>>            struct canxl_frame cxl_hdr = { 0 };
>>>>>>>            struct canxl_frame *cxl1, *cxl2;
>>>>>>>            size_t cxl2_data_len, cxl2_frame_len;
>>>>>>>
>>>>>>>            /* read header */
>>>>>>>            assert(read(s, &cxl_hdr, sizeof(cxl_hdr)) == sizeof(cxl_hdr));
>>>>>>>            cxl1 = malloc(sizeof(*cxl1) + cxl_hdr.len);
>>>>>>>            memcpy(cxl1, &cxl_hdr, sizeof(cxl_hdr));
>>>>>>>            /* read remaining data */
>>>>>>>            assert(read(s, cxl1->data, cxl1->len) == cxl1->len);
>>>>>>
>>>>>> For performance reasons you probable don't want to split the read of a
>>>>>> single CAN frame over 2 read() syscalls.
>>
>> Yes, two read() syscalls (with testing/asserting for the right size),
>> one malloc() syscall, one memcpy().
>>
>> Alternatively I can offer ONE syscall with ONE single copy operation
>> from kernel to userspace \o/
>>
>> read(s, &can.xl, sizeof(struct canxl_frame));
> 
> You just ignored what I wrote just below:
> | I wrote the code to illustrate how to do header
> | manipulations. The goal of this example was not to be optimal
> | but to show off how sizeof(struct canxl_frame) could be used.
> 
> I was answering your "This is no sizeof(struct canxl_frame) anymore" comment.
> 
> You can do:
> | struct canxl_frame *cxl = malloc(CANXL_MTU);
> | read(s, cxl, CANXL_MTU);
> 
> In fact, the sizeof() will be mostly useful in the tx path when doing
> write(), not in the rx when you will likely always provide a buffer of
> maximum size. Now I regret sending my example.
> 
>> The more I read about splitting/peeking/testing, calculating of sizes,
>> etc the more I like the simple struct canxl_frame with 2048 bytes of
>> data that is specified to be truncated by definition.
>>
>> E.g. for candump or other usual CAN applications you can run with *one*
>> static struct without any malloc(). This is not only a bridge for
>> application programmers that have experiences in programming SocketCAN
>> applications - it is just a safe and simple pattern that I would not
>> like to give up for which improvement?
> 
> The possibility to do static declaration is the strongest argument in
> your favor.

Yes. And it is a good argument.

> There is no easy way to do this with flexible array member aside maybe
> of the convoluted:
> | char buf[CANXL_MTU];
> | struct canxl_frame *cxl = &(struct canxl_frame)buf;
> 
> But I do not see the problem of using dynamic memory. For a 2
> kilobytes buffer, dynamic memory looks pretty standard to me.

I do see a problem with it and definitely have another personal 
preference. So far I did not need dynamic memory to process CAN content 
in CAN applications which is a good (and required) pattern for embedded 
automotive applications and allows to port concepts and code between 
these two worlds.

> And one
> more time, is there any precedent in the kernel uapi of not using
> flexible array members for structures meant to hold more than one
> kilobyte? For the tiny CAN(-FD) it made sense, the same argument does
> not translate so easily when going from ~64B to ~2KB.

Because of what?

IMO it is a proper follow up of the current CAN_RAW API.

> My main point is that your approach does not follow what I could
> witness so far in the UAPI. You have not yet answered this point.

There is an ioctl:

https://manpages.debian.org/testing/manpages-de-dev/ioctl_list.2.de.html

0x4FA44308 	SNDCTL_COPR_SENDMSG 	const struct copr_msg *
0x8FA44309 	SNDCTL_COPR_RCVMSG 	struct copr_msg *

include/uapi/linux/soundcard.h

typedef struct copr_msg {
                 int len;
                 unsigned char data[4000];
         } copr_msg;


>> In fact I don't know any SocketCAN application that uses malloc for CAN
>> frames which likely introduces lags and affects the performance.
> 
> This argument is invalid. You can malloc() once at the beginning of
> the program and reuse it for the entire lifetime (please forget my
> previous code snippet which was not meant to be performant).
> There is no way that a single malloc can introduce noticeable lags
> compared to a static allocation.

As described above omitting dynamic memory as a whole is a valid 
pattern. And so far no one needs dynamic memory or has to be encouraged 
to use it.

With the API of truncated CAN XL frames you can do whatever you want 
(even make use of malloc()) on the user space side.

I don't see any real argument to leave the established pattern.

You probably might feel that I don't like dynamic arrays or have some 
problems of making use of it. This is not the case:

include/uapi/linux/can/bcm.h

struct bcm_msg_head {
         __u32 opcode;
         __u32 flags;
         __u32 count;
         struct bcm_timeval ival1, ival2;
         canid_t can_id;
         __u32 nframes;
         struct can_frame frames[0];
};


Which leads to:

linux-can/can-tests/bcm/tst-bcm-filter.c

struct {
         struct bcm_msg_head msg_head;
         struct can_frame frame[4];
} txmsg, rxmsg;


txmsg.msg_head.opcode  = RX_SETUP;
txmsg.msg_head.can_id  = 0x042;
txmsg.msg_head.flags   = SETTIMER|RX_FILTER_ID;
txmsg.msg_head.ival1.tv_sec = 1;
txmsg.msg_head.ival1.tv_usec = 0;
txmsg.msg_head.ival2.tv_sec = 0;
txmsg.msg_head.ival2.tv_usec = 0;
txmsg.msg_head.nframes = 0;

But this is something different to me than having a struct canxl_frame 
without data - especially as it requires at least one data byte 
(CANXL_MIN_DLEN) to be a valid CAN XL frame.

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

A truncated CAN XL frame structure with a data section that contains as 
much valid bytes as given in the len element is the exact representation 
of what you will see on the wire.

For some reason you seem to stick on this (even invalid) "UAPI is using 
dynamic arrays" stuff instead of enhancing and improving the established 
handy and safe pattern for CAN frames.

Regards,
Oliver

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

[Vincent Mailhol]

On Tue. 23 Jul. 2022 à 03:52, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 22.07.22 18:32, Vincent Mailhol wrote:
> > On Sat. 23 Jul. 2022 at 00:30, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >> On 22.07.22 12:54, Vincent Mailhol wrote:
> >>> On Fri. 22 Jul. 2022 at 18:58, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >>>> On 22.07.2022 17:33:08, Vincent Mailhol wrote:
> >>>>> On Fri. 22 Jul. 2022 at 16:27, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >>>>>> On 22.07.2022 12:20:43, Vincent Mailhol wrote:
> >>>>>>> I do not buy your argument that you can not do sizeof(struct
> >>>>>>> canxl_frame) because of naming.
> >>>>>>>
> >>>>>>> With a flexible array member, I can do:
> >>>>>>>
> >>>>>>> struct canxl_frame {
> >>>>>>>            canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
> >>>>>>>            __u8    flags; /* additional flags for CAN XL */
> >>>>>>>            __u8    sdt;   /* SDU (service data unit) type */
> >>>>>>>            __u16   len;   /* frame payload length in byte */
> >>>>>>>            __u32   af;    /* acceptance field */
> >>>>>>>            __u8    data[];
> >>>>>>> };
> >>>>>>>
> >>>>>>> void foo (int s)
> >>>>>>> {
> >>>>>>>            struct canxl_frame cxl_hdr = { 0 };
> >>>>>>>            struct canxl_frame *cxl1, *cxl2;
> >>>>>>>            size_t cxl2_data_len, cxl2_frame_len;
> >>>>>>>
> >>>>>>>            /* read header */
> >>>>>>>            assert(read(s, &cxl_hdr, sizeof(cxl_hdr)) == sizeof(cxl_hdr));
> >>>>>>>            cxl1 = malloc(sizeof(*cxl1) + cxl_hdr.len);
> >>>>>>>            memcpy(cxl1, &cxl_hdr, sizeof(cxl_hdr));
> >>>>>>>            /* read remaining data */
> >>>>>>>            assert(read(s, cxl1->data, cxl1->len) == cxl1->len);
> >>>>>>
> >>>>>> For performance reasons you probable don't want to split the read of a
> >>>>>> single CAN frame over 2 read() syscalls.
> >>
> >> Yes, two read() syscalls (with testing/asserting for the right size),
> >> one malloc() syscall, one memcpy().
> >>
> >> Alternatively I can offer ONE syscall with ONE single copy operation
> >> from kernel to userspace \o/
> >>
> >> read(s, &can.xl, sizeof(struct canxl_frame));
> >
> > You just ignored what I wrote just below:
> > | I wrote the code to illustrate how to do header
> > | manipulations. The goal of this example was not to be optimal
> > | but to show off how sizeof(struct canxl_frame) could be used.
> >
> > I was answering your "This is no sizeof(struct canxl_frame) anymore" comment.
> >
> > You can do:
> > | struct canxl_frame *cxl = malloc(CANXL_MTU);
> > | read(s, cxl, CANXL_MTU);
> >
> > In fact, the sizeof() will be mostly useful in the tx path when doing
> > write(), not in the rx when you will likely always provide a buffer of
> > maximum size. Now I regret sending my example.
> >
> >> The more I read about splitting/peeking/testing, calculating of sizes,
> >> etc the more I like the simple struct canxl_frame with 2048 bytes of
> >> data that is specified to be truncated by definition.
> >>
> >> E.g. for candump or other usual CAN applications you can run with *one*
> >> static struct without any malloc(). This is not only a bridge for
> >> application programmers that have experiences in programming SocketCAN
> >> applications - it is just a safe and simple pattern that I would not
> >> like to give up for which improvement?
> >
> > The possibility to do static declaration is the strongest argument in
> > your favor.
>
> Yes. And it is a good argument.
>
> > There is no easy way to do this with flexible array member aside maybe
> > of the convoluted:
> > | char buf[CANXL_MTU];
> > | struct canxl_frame *cxl = &(struct canxl_frame)buf;
> >
> > But I do not see the problem of using dynamic memory. For a 2
> > kilobytes buffer, dynamic memory looks pretty standard to me.
>
> I do see a problem with it and definitely have another personal
> preference. So far I did not need dynamic memory to process CAN content
> in CAN applications which is a good (and required) pattern for embedded
> automotive applications and allows to port concepts and code between
> these two worlds.

ACK that for safety application (as of ISO 26262) static declaration
are prefered (and nearly mandatory at ASIL C and D levels) but the
Linux kernel is not designed for safety.
If your kernel uses dynamic memory allocations, then your application
will not be safer (as of ISO 26262) with static declarations.

I do not believe it will be a problem when porting code from an
embedded application to Socket CAN. It is not the same structure, not
the same field names, not the same API so you have to rewrite the code
related to the declaration, initialization and the read()/write()
regardless. The only part which is portable is the processing of
canxl_frame::data and this part could be ported regardless if you use
dynamic or static memory application.

> > And one
> > more time, is there any precedent in the kernel uapi of not using
> > flexible array members for structures meant to hold more than one
> > kilobyte? For the tiny CAN(-FD) it made sense, the same argument does
> > not translate so easily when going from ~64B to ~2KB.
>
> Because of what?

Because of consistency.

> IMO it is a proper follow up of the current CAN_RAW API.
>
> > My main point is that your approach does not follow what I could
> > witness so far in the UAPI. You have not yet answered this point.
>
> There is an ioctl:
>
> https://manpages.debian.org/testing/manpages-de-dev/ioctl_list.2.de.html
>
> 0x4FA44308      SNDCTL_COPR_SENDMSG     const struct copr_msg *
> 0x8FA44309      SNDCTL_COPR_RCVMSG      struct copr_msg *
>
> include/uapi/linux/soundcard.h
>
> typedef struct copr_msg {
>                  int len;
>                  unsigned char data[4000];
>          } copr_msg;

OK. So the best example you found was introduced in Linux 1.x (around
end of 1995!) and already violates the Linux coding guidelines by
using typedef.
Ref: https://elixir.bootlin.com/linux/1.3.9/source/include/linux/soundcard.h#L638
Ref: https://www.kernel.org/doc/html/latest/process/coding-style.html#typedefs

So I guess you just found the exception that proves the rule.

> >> In fact I don't know any SocketCAN application that uses malloc for CAN
> >> frames which likely introduces lags and affects the performance.
> >
> > This argument is invalid. You can malloc() once at the beginning of
> > the program and reuse it for the entire lifetime (please forget my
> > previous code snippet which was not meant to be performant).
> > There is no way that a single malloc can introduce noticeable lags
> > compared to a static allocation.
>
> As described above omitting dynamic memory as a whole is a valid
> pattern. And so far no one needs dynamic memory or has to be encouraged
> to use it.
>
> With the API of truncated CAN XL frames you can do whatever you want
> (even make use of malloc()) on the user space side.
>
> I don't see any real argument to leave the established pattern.
>
> You probably might feel that I don't like dynamic arrays or have some
> problems of making use of it. This is not the case:
>
> include/uapi/linux/can/bcm.h
>
> struct bcm_msg_head {
>          __u32 opcode;
>          __u32 flags;
>          __u32 count;
>          struct bcm_timeval ival1, ival2;
>          canid_t can_id;
>          __u32 nframes;
>          struct can_frame frames[0];
> };
>
>
> Which leads to:
>
> linux-can/can-tests/bcm/tst-bcm-filter.c
>
> struct {
>          struct bcm_msg_head msg_head;
>          struct can_frame frame[4];
> } txmsg, rxmsg;
>
>
> txmsg.msg_head.opcode  = RX_SETUP;
> txmsg.msg_head.can_id  = 0x042;
> txmsg.msg_head.flags   = SETTIMER|RX_FILTER_ID;
> txmsg.msg_head.ival1.tv_sec = 1;
> txmsg.msg_head.ival1.tv_usec = 0;
> txmsg.msg_head.ival2.tv_sec = 0;
> txmsg.msg_head.ival2.tv_usec = 0;
> txmsg.msg_head.nframes = 0;
>
> But this is something different to me than having a struct canxl_frame
> without data - especially as it requires at least one data byte
> (CANXL_MIN_DLEN) to be a valid CAN XL frame.
>
> struct canxl_frame {
>          canid_t prio;  /* 11 bit priority for arbitration (canid_t) */
>          __u8    flags; /* additional flags for CAN XL */
>          __u8    sdt;   /* SDU (service data unit) type */
>          __u16   len;   /* frame payload length in byte */
>          __u32   af;    /* acceptance field */
>          __u8    data[CANXL_MIN_DLEN];
> };
>
> A truncated CAN XL frame structure with a data section that contains as
> much valid bytes as given in the len element is the exact representation
> of what you will see on the wire.
>
> For some reason you seem to stick on this (even invalid) "UAPI is using
> dynamic arrays" stuff instead of enhancing and improving the established
> handy and safe pattern for CAN frames.

Please define what you mean as "safe". SocketCAN is not safe by ISO
26262 standard. And in a non safety critical context, static memory
allocation is equally as safe as dynamic memory allocation (i.e. both
are not safe): if you have no memory left, an application, even if
using static declaration, will not start.

This debate is going to a dead end. I do not see one of us fully
convincing the other. As I stated before, the data[CANXL_MIN_DLEN] is
not bad and has the big merit of allowing static declaration. While my
preference definitely goes to the flexible array member, I will not
veto your idea. I think it was important to have this debate.
With that said, I leave the final decision to the mailing list. If
other people prefer the data[CANXL_MIN_DLEN] over the flexible array
member, or if no one other than the two of us care, then let's use
your approach!


Yours sincerely,
Vincent Mailhol

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

[Oliver Hartkopp]

On 23.07.22 04:39, Vincent Mailhol wrote:

> This debate is going to a dead end. I do not see one of us fully
> convincing the other. As I stated before, the data[CANXL_MIN_DLEN] is
> not bad and has the big merit of allowing static declaration. While my
> preference definitely goes to the flexible array member, I will not
> veto your idea. I think it was important to have this debate.
> With that said, I leave the final decision to the mailing list. If
> other people prefer the data[CANXL_MIN_DLEN] over the flexible array
> member, or if no one other than the two of us care, then let's use
> your approach!

Thanks Vincent!

I really think it is the right approach to maintain the current CAN_RAW 
API patterns for all CAN flavours. It provides consistency for 
programmers and tools and really only affects the CAN_RAW socket.

When it comes to Ethernet traffic encapsulation inside CAN XL this can 
be seen as a break to the CAN stuff and I wonder if we should create 
another 'Ethernet like' network device then.

E.g. an Ethernet device, that is able to handle/convert ETH_P_CANXL 
frames and offers additional attributes (setting the prio and defining 
the CAN XL encapsulation schema etc). This will kick us into the 
Ethernet world with all the common APIs - and will definitely fuel a new 
and interesting discussion ;-)

Best regards,
Oliver

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

[Vincent Mailhol]

On Thu. 21 Jul. 2022 at 16:36, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> On 21.07.22 04:37, Vincent Mailhol wrote:
> > On Wed. 21 Jul. 2022 at 01:43, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> >> On 19.07.22 17:16, Vincent Mailhol wrote:
> >>> On Tue 19 Jul. 2022 at 23:38, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
(...)
> I just wanted to get an impression whether it is handy to program this
> extended API or not.
>
> >> nbytes = read(s, &can.xl, sizeof(struct canxl_frame));
> >> if (nbytes < 0) {
> >>           perror("read");
> >>           return 1;
> >> }
> >> printf("nbytes = %d\n", nbytes);
> >
> > Does read() guarantee atomicity? From "man 2 read":
> > | It is not an error if [the return value] is smaller than the number
> > | of bytes requested; this may happen for example because fewer bytes
> > | are actually available right now (maybe because we were close to
> > | end-of-file, or because we are reading from a pipe, or from a
> > | terminal), *or because read() was interrupted by a signal*.
> >
> > I think the answer is yes, but I prefer to double check (I am
> > especially concerned by the signal interrupts).
>
> Hm, we are not reading from a file but from a socket here that provide
> chunks in the form of struct can_frame in raw_recvmsg(). You only get a
> MSG_TRUNC there when you provide a (buffer)size in userspace that's to
> small.
>
> I've never got any error reports on CAN_RAW reading (over 16 years) and
> all the examples contain a test for sizeof(struct can_frame) like this:
>
>  >> if (nbytes != sizeof(struct can_frame) &&
>  >>       nbytes != sizeof(struct canfd_frame)) {
>
> So we either have an error or an incomplete CAN frame which becomes an
> error too.
>
> Do you think this is still worth an investigation?

My concern is whether or not read() can really be interrupted by a signal?
But actually, even if read() were not atomic, it would still be
usable: you just have to continue reading until you get the first 64
bits, then you have all the information needed to determine the type
and the total length.

So I do not think this is a valid concern, but even if it is, it does
not invalidate current design. So OK to move on. Maybe it is just
something to add to the TODO list of the "things I yet have to
understand".


Yours sincerely,
Vincent Mailhol

[RFC PATCH v5 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, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
index a15619d883ec..d72c0727b440 100644
--- a/drivers/net/can/vcan.c
+++ b/drivers/net/can/vcan.c
@@ -68,33 +68,32 @@ 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;
 	struct net_device_stats *stats = &dev->stats;
 
 	stats->rx_packets++;
-	stats->rx_bytes += cfd->len;
+	stats->rx_bytes += can_skb_get_data_len(skb);
 
 	skb->pkt_type  = PACKET_BROADCAST;
 	skb->dev       = dev;
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 	netif_rx(skb);
 }
 
 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;
+	int loop;
 
 	if (can_dropped_invalid_skb(dev, skb))
 		return NETDEV_TX_OK;
 
-	len = cfd->can_id & CAN_RTR_FLAG ? 0 : cfd->len;
+	len = can_skb_get_data_len(skb);
 	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 +131,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 v5 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                | 89 ++++++++++++++++++++++++++++++------
 2 files changed, 76 insertions(+), 14 deletions(-)

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..0090fb785480 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;
@@ -127,12 +128,13 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
 
 	/* check the received tx sock reference */
 	if (!ro->recv_own_msgs && oskb->sk == sk)
 		return;
 
-	/* do not pass non-CAN2.0 frames to a legacy socket */
-	if (!ro->fd_frames && oskb->len != CAN_MTU)
+	/* make sure to not pass oversized frames to the socket */
+	if ((oskb->len > CAN_MTU && !ro->fd_frames && !ro->xl_frames) ||
+	    (oskb->len == CANXL_MTU && !ro->xl_frames))
 		return;
 
 	/* eliminate multiple filter matches for the same skb */
 	if (this_cpu_ptr(ro->uniq)->skb == oskb &&
 	    this_cpu_ptr(ro->uniq)->skbcnt == can_skb_prv(oskb)->skbcnt) {
@@ -344,10 +346,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 +670,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 +761,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;
@@ -773,12 +791,17 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	struct sock *sk = sock->sk;
 	struct raw_sock *ro = raw_sk(sk);
 	struct sockcm_cookie sockc;
 	struct sk_buff *skb;
 	struct net_device *dev;
+	unsigned int skb_sz = size;
 	int ifindex;
-	int err;
+	int err = -EINVAL;
+
+	/* valid CAN frame sizes */
+	if (size < CANXL_HDR_SZ + CANXL_MIN_DLEN || size > CANXL_MTU)
+		return -EINVAL;
 
 	if (msg->msg_name) {
 		DECLARE_SOCKADDR(struct sockaddr_can *, addr, msg->msg_name);
 
 		if (msg->msg_namelen < RAW_MIN_NAMELEN)
@@ -794,20 +817,15 @@ 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 (unlikely(size != CANFD_MTU && size != CAN_MTU))
-			goto put_dev;
-	} else {
-		if (unlikely(size != CAN_MTU))
-			goto put_dev;
-	}
+	/* alloc full CAN XL space */
+	if (ro->xl_frames && dev->mtu == CANXL_MTU)
+		skb_sz = CANXL_MTU;
 
-	skb = sock_alloc_send_skb(sk, size + sizeof(struct can_skb_priv),
+	skb = sock_alloc_send_skb(sk, skb_sz + sizeof(struct can_skb_priv),
 				  msg->msg_flags & MSG_DONTWAIT, &err);
 	if (!skb)
 		goto put_dev;
 
 	can_skb_reserve(skb);
@@ -816,10 +834,43 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 
 	err = memcpy_from_msg(skb_put(skb, size), msg, size);
 	if (err < 0)
 		goto free_skb;
 
+	err = -EINVAL;
+
+	if (ro->xl_frames && dev->mtu == CANXL_MTU) {
+		struct canxl_frame *cfx = (struct canxl_frame *)skb->data;
+
+		if (cfx->flags & CANXL_XLF) {
+			/* check for valid CAN XL frame */
+			if (cfx->len < CANXL_MIN_DLEN ||
+			    cfx->len > CANXL_MAX_DLEN)
+			goto free_skb;
+
+			/* check truncated CAN XL frame structure */
+			if (size != cfx->len + CANXL_HDR_SZ)
+				goto free_skb;
+
+			/* push skb->len to CANXL_MTU */
+			if (size < CANXL_MTU)
+				skb_put_zero(skb, CANXL_MTU - size);
+		} else {
+			/* check for CAN FD and Classical CAN */
+			if (unlikely(size != CANFD_MTU && size != CAN_MTU))
+				goto free_skb;
+		}
+	} else if (ro->fd_frames && dev->mtu == CANFD_MTU) {
+		/* CAN FD and Classical CAN */
+		if (unlikely(size != CANFD_MTU && size != CAN_MTU))
+			goto free_skb;
+	} else {
+		/* Classical CAN */
+		if (unlikely(size != CAN_MTU))
+			goto free_skb;
+	}
+
 	sockcm_init(&sockc, sk);
 	if (msg->msg_controllen) {
 		err = sock_cmsg_send(sk, msg, &sockc);
 		if (unlikely(err))
 			goto free_skb;
@@ -850,25 +901,35 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 
 static int raw_recvmsg(struct socket *sock, struct msghdr *msg, size_t size,
 		       int flags)
 {
 	struct sock *sk = sock->sk;
+	struct raw_sock *ro = raw_sk(sk);
 	struct sk_buff *skb;
+	struct canxl_frame *cfx;
+	unsigned int rx_len;
 	int err = 0;
 
 	if (flags & MSG_ERRQUEUE)
 		return sock_recv_errqueue(sk, msg, size,
 					  SOL_CAN_RAW, SCM_CAN_RAW_ERRQUEUE);
 
 	skb = skb_recv_datagram(sk, flags, &err);
 	if (!skb)
 		return err;
 
-	if (size < skb->len)
+	rx_len = skb->len;
+	if (rx_len == CANXL_MTU) {
+		cfx = (struct canxl_frame *)skb->data;
+		if (cfx->len >= CANXL_MIN_DLEN && cfx->len < CANXL_MAX_DLEN)
+			rx_len = CANXL_HDR_SZ + cfx->len;
+	}
+
+	if (size < rx_len)
 		msg->msg_flags |= MSG_TRUNC;
 	else
-		size = skb->len;
+		size = rx_len;
 
 	err = memcpy_to_msg(msg, skb->data, size);
 	if (err < 0) {
 		skb_free_datagram(sk, skb);
 		return err;
-- 
2.30.2