can: support CAN XL

To simplify the testing in user space all struct canfd_frame's provided by the CAN subsystem of the Linux kernel now have the CANFD_FDF flag set in canfd_frame::flags.

NB: Handcrafted ETH_P_CANFD frames introduced via PF_PACKET socket might not set this bit correctly. During the check for sufficient headroom in PF_PACKET sk_buffs the uninitialized CAN sk_buff data structures are filled. In the case of a CAN FD frame the CANFD_FDF flag is set accordingly.

As the CAN frame content is already zero initialized in alloc_canfd_skb() the obsolete initialization of cf->flags in the CTU CAN FD driver has been removed as it would overwrite the already set CANFD_FDF flag.

Signed-off-by Oliver Hartkopp <[email protected]>

---
 drivers/net/can/ctucanfd/ctucanfd_base.c |  1 -
 drivers/net/can/dev/skb.c                | 11 +++++++++++
 include/uapi/linux/can.h                 |  4 ++--
 net/can/af_can.c                         |  5 +++++
 4 files changed, 18 insertions(+), 3 deletions(-)

drivers/net/can/ctucanfd/ctucanfd_base.c +0 -1

index 3c18d028bd8c..c4026712ab7d 100644
@@ -655,11 +655,10 @@ static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *c
                cf->can_id = (idw & CAN_EFF_MASK) | CAN_EFF_FLAG;
        else
                cf->can_id = (idw >> 18) & CAN_SFF_MASK;
 
        /* BRS, ESI, RTR Flags */
-       cf->flags = 0;
        if (FIELD_GET(REG_FRAME_FORMAT_W_FDF, ffw)) {
                if (FIELD_GET(REG_FRAME_FORMAT_W_BRS, ffw))
                        cf->flags |= CANFD_BRS;
                if (FIELD_GET(REG_FRAME_FORMAT_W_ESI_RSV, ffw))
                        cf->flags |= CANFD_ESI;

drivers/net/can/dev/skb.c +11 -0

index b896e1ce3b47..adb413bdd734 100644
@@ -242,10 +242,13 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
        can_skb_prv(skb)->ifindex = dev->ifindex;
        can_skb_prv(skb)->skbcnt = 0;
 
        *cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
 
+       /* set CAN FD flag by default */
+       (*cfd)->flags = CANFD_FDF;
+
        return skb;
 }
 EXPORT_SYMBOL_GPL(alloc_canfd_skb);
 
 struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf)

@@ -285,10 +288,18 @@ static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
                        skb->pkt_type = PACKET_HOST;
 
                skb_reset_mac_header(skb);
                skb_reset_network_header(skb);
                skb_reset_transport_header(skb);
+
+               /* set CANFD_FDF flag for CAN FD frames */
+               if (can_is_canfd_skb(skb)) {
+                       struct canfd_frame *cfd;
+
+                       cfd = (struct canfd_frame *)skb->data;
+                       cfd->flags |= CANFD_FDF;
+               }
        }
 
        return true;
 }
 

include/uapi/linux/can.h +2 -2

index 90801ada2bbe..7b23eeeb3273 100644
@@ -139,12 +139,12 @@ struct can_frame {
  * The struct can_frame and struct canfd_frame intentionally share the same
  * layout to be able to write CAN frame content into a CAN FD frame structure.
  * When this is done the former differentiation via CAN_MTU / CANFD_MTU gets
  * lost. CANFD_FDF allows programmers to mark CAN FD frames in the case of
  * using struct canfd_frame for mixed CAN / CAN FD content (dual use).
- * N.B. the Kernel APIs do NOT provide mixed CAN / CAN FD content inside of
- * struct canfd_frame therefore the CANFD_FDF flag is disregarded by Linux.
+ * Since the introduction of CAN XL the CANFD_FDF flag is set in all CAN FD
+ * frame structures provided by the CAN subsystem of the Linux kernel.
  */
 #define CANFD_BRS 0x01 /* bit rate switch (second bitrate for payload data) */
 #define CANFD_ESI 0x02 /* error state indicator of the transmitting node */
 #define CANFD_FDF 0x04 /* mark CAN FD for dual use of struct canfd_frame */
 

net/can/af_can.c +1 -0

index afa6c2151bc4..072a6a5c9dd1 100644
@@ -203,11 +203,16 @@ int can_send(struct sk_buff *skb, int loop)
        int err = -EINVAL;
 
        if (can_is_can_skb(skb)) {
                skb->protocol = htons(ETH_P_CAN);
        } else if (can_is_canfd_skb(skb)) {
+               struct canfd_frame *cfd = (struct canfd_frame *)skb->data;

Quoted 13 lines · Oliver Hartkopp

To simplify the testing in user space all struct canfd_frame's provided by the CAN subsystem of the Linux kernel now have the CANFD_FDF flag set in canfd_frame::flags.

NB: Handcrafted ETH_P_CANFD frames introduced via PF_PACKET socket might not set this bit correctly. During the check for sufficient headroom in PF_PACKET sk_buffs the uninitialized CAN sk_buff data structures are filled. In the case of a CAN FD frame the CANFD_FDF flag is set accordingly.

As the CAN frame content is already zero initialized in alloc_canfd_skb() the obsolete initialization of cf->flags in the CTU CAN FD driver has been removed as it would overwrite the already set CANFD_FDF flag.

Quoted 7 lines

Signed-off-by Oliver Hartkopp <[email protected]>

---
  drivers/net/can/ctucanfd/ctucanfd_base.c |  1 -
  drivers/net/can/dev/skb.c                | 11 +++++++++++
  include/uapi/linux/can.h                 |  4 ++--
  net/can/af_can.c                         |  5 +++++
  4 files changed, 18 insertions(+), 3 deletions(-)

Would it be relevant to also set the flag when the skb is cloned?

include/linux/can/skb.h +1 -0

index 182749e858b3..24de0bb7092e 100644
@@ -85,6 +85,7 @@ static inline void can_skb_set_owner(struct sk_buff
 *skb, struct sock *sk)
  static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
  {
         struct sk_buff *nskb;
+       struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
 
         nskb = skb_clone(skb, GFP_ATOMIC);

         if (unlikely(!nskb)) {

include/linux/can/skb.h +3 -0

@@ -92,6 +93,9 @@ static inline struct sk_buff
 *can_create_echo_skb(struct sk_buff *skb)
                 return NULL;
         }
 
+       if (can_is_canfd_skb(skb))
+               cfd->flags |= CANFD_FDF;
+
         can_skb_set_owner(nskb, skb->sk);
         consume_skb(skb);

         return nskb;

Quoted 92 lines

drivers/net/can/ctucanfd/ctucanfd_base.c +0 -1

index 3c18d028bd8c..c4026712ab7d 100644
@@ -655,11 +655,10 @@ static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *c
                 cf->can_id = (idw & CAN_EFF_MASK) | CAN_EFF_FLAG;
         else
                 cf->can_id = (idw >> 18) & CAN_SFF_MASK;
 
         /* BRS, ESI, RTR Flags */
-       cf->flags = 0;
         if (FIELD_GET(REG_FRAME_FORMAT_W_FDF, ffw)) {
                 if (FIELD_GET(REG_FRAME_FORMAT_W_BRS, ffw))
                         cf->flags |= CANFD_BRS;
                 if (FIELD_GET(REG_FRAME_FORMAT_W_ESI_RSV, ffw))
                         cf->flags |= CANFD_ESI;

drivers/net/can/dev/skb.c +11 -0

index b896e1ce3b47..adb413bdd734 100644
@@ -242,10 +242,13 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
         can_skb_prv(skb)->ifindex = dev->ifindex;
         can_skb_prv(skb)->skbcnt = 0;
 
         *cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
 
+       /* set CAN FD flag by default */
+       (*cfd)->flags = CANFD_FDF;
+
         return skb;
  }
  EXPORT_SYMBOL_GPL(alloc_canfd_skb);
 
  struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf)

@@ -285,10 +288,18 @@ static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
                         skb->pkt_type = PACKET_HOST;
 
                 skb_reset_mac_header(skb);
                 skb_reset_network_header(skb);
                 skb_reset_transport_header(skb);
+
+               /* set CANFD_FDF flag for CAN FD frames */
+               if (can_is_canfd_skb(skb)) {
+                       struct canfd_frame *cfd;
+
+                       cfd = (struct canfd_frame *)skb->data;
+                       cfd->flags |= CANFD_FDF;
+               }
         }
 
         return true;
  }
 

include/uapi/linux/can.h +2 -2

index 90801ada2bbe..7b23eeeb3273 100644
@@ -139,12 +139,12 @@ struct can_frame {
   * The struct can_frame and struct canfd_frame intentionally share the same
   * layout to be able to write CAN frame content into a CAN FD frame structure.
   * When this is done the former differentiation via CAN_MTU / CANFD_MTU gets
   * lost. CANFD_FDF allows programmers to mark CAN FD frames in the case of
   * using struct canfd_frame for mixed CAN / CAN FD content (dual use).
- * N.B. the Kernel APIs do NOT provide mixed CAN / CAN FD content inside of
- * struct canfd_frame therefore the CANFD_FDF flag is disregarded by Linux.
+ * Since the introduction of CAN XL the CANFD_FDF flag is set in all CAN FD
+ * frame structures provided by the CAN subsystem of the Linux kernel.
   */
  #define CANFD_BRS 0x01 /* bit rate switch (second bitrate for payload data) */
  #define CANFD_ESI 0x02 /* error state indicator of the transmitting node */
  #define CANFD_FDF 0x04 /* mark CAN FD for dual use of struct canfd_frame */
 

net/can/af_can.c +5 -0

index afa6c2151bc4..072a6a5c9dd1 100644
@@ -203,11 +203,16 @@ int can_send(struct sk_buff *skb, int loop)
         int err = -EINVAL;
 
         if (can_is_can_skb(skb)) {
                 skb->protocol = htons(ETH_P_CAN);
         } else if (can_is_canfd_skb(skb)) {
+               struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+
                 skb->protocol = htons(ETH_P_CANFD);
+
+               /* set CAN FD flag for CAN FD frames by default */
+               cfd->flags |= CANFD_FDF;
         } else {
                 goto inval_skb;
         }
 

net/can/af_can.c int can_send(struct sk_buff *skb, int loop) · line 218

Quoted 1 line

net/can/af_can.c +0 -0

         /* Make sure the CAN frame can pass the selected CAN netdevice. */

Once all the malloc and clone functions set the flag, is there still a route that would reach the can_send() without setting the CANFD_FDF flag? If the answer is supposedly no, maybe it is better to remove the check (or maybe replace it with a debug message to identify missed use cases).

Quoted 47 lines · Vincent Mailhol

Quoted 13 lines · Oliver Hartkopp

To simplify the testing in user space all struct canfd_frame's provided by the CAN subsystem of the Linux kernel now have the CANFD_FDF flag set in canfd_frame::flags.

NB: Handcrafted ETH_P_CANFD frames introduced via PF_PACKET socket might not set this bit correctly. During the check for sufficient headroom in PF_PACKET sk_buffs the uninitialized CAN sk_buff data structures are filled. In the case of a CAN FD frame the CANFD_FDF flag is set accordingly.

As the CAN frame content is already zero initialized in alloc_canfd_skb() the obsolete initialization of cf->flags in the CTU CAN FD driver has been removed as it would overwrite the already set CANFD_FDF flag.

Quoted 7 lines

Signed-off-by Oliver Hartkopp <[email protected]>

---
  drivers/net/can/ctucanfd/ctucanfd_base.c |  1 -
  drivers/net/can/dev/skb.c                | 11 +++++++++++
  include/uapi/linux/can.h                 |  4 ++--
  net/can/af_can.c                         |  5 +++++
  4 files changed, 18 insertions(+), 3 deletions(-)

Would it be relevant to also set the flag when the skb is cloned?

include/linux/can/skb.h +1 -0

index 182749e858b3..24de0bb7092e 100644
@@ -85,6 +85,7 @@ static inline void can_skb_set_owner(struct sk_buff
 *skb, struct sock *sk)
  static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
  {
         struct sk_buff *nskb;
+       struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
 
         nskb = skb_clone(skb, GFP_ATOMIC);

         if (unlikely(!nskb)) {

include/linux/can/skb.h +3 -0

@@ -92,6 +93,9 @@ static inline struct sk_buff
 *can_create_echo_skb(struct sk_buff *skb)
                 return NULL;
         }
 
+       if (can_is_canfd_skb(skb))
+               cfd->flags |= CANFD_FDF;
+
         can_skb_set_owner(nskb, skb->sk);
         consume_skb(skb);

Quoted 2 lines

         return nskb;

No, this should be obsolete.

The current patch set should set this bit at all creation/malloc places of CAN FD skbs.

Quoted 92 lines

Quoted 92 lines

drivers/net/can/ctucanfd/ctucanfd_base.c +0 -1

index 3c18d028bd8c..c4026712ab7d 100644
@@ -655,11 +655,10 @@ static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *c
                 cf->can_id = (idw & CAN_EFF_MASK) | CAN_EFF_FLAG;
         else
                 cf->can_id = (idw >> 18) & CAN_SFF_MASK;
 
         /* BRS, ESI, RTR Flags */
-       cf->flags = 0;
         if (FIELD_GET(REG_FRAME_FORMAT_W_FDF, ffw)) {
                 if (FIELD_GET(REG_FRAME_FORMAT_W_BRS, ffw))
                         cf->flags |= CANFD_BRS;
                 if (FIELD_GET(REG_FRAME_FORMAT_W_ESI_RSV, ffw))
                         cf->flags |= CANFD_ESI;

drivers/net/can/dev/skb.c +11 -0

index b896e1ce3b47..adb413bdd734 100644
@@ -242,10 +242,13 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
         can_skb_prv(skb)->ifindex = dev->ifindex;
         can_skb_prv(skb)->skbcnt = 0;
 
         *cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
 
+       /* set CAN FD flag by default */
+       (*cfd)->flags = CANFD_FDF;
+
         return skb;
  }
  EXPORT_SYMBOL_GPL(alloc_canfd_skb);
 
  struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf)

@@ -285,10 +288,18 @@ static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
                         skb->pkt_type = PACKET_HOST;
 
                 skb_reset_mac_header(skb);
                 skb_reset_network_header(skb);
                 skb_reset_transport_header(skb);
+
+               /* set CANFD_FDF flag for CAN FD frames */
+               if (can_is_canfd_skb(skb)) {
+                       struct canfd_frame *cfd;
+
+                       cfd = (struct canfd_frame *)skb->data;
+                       cfd->flags |= CANFD_FDF;
+               }
         }
 
         return true;
  }
 

include/uapi/linux/can.h +2 -2

index 90801ada2bbe..7b23eeeb3273 100644
@@ -139,12 +139,12 @@ struct can_frame {
   * The struct can_frame and struct canfd_frame intentionally share the same
   * layout to be able to write CAN frame content into a CAN FD frame structure.
   * When this is done the former differentiation via CAN_MTU / CANFD_MTU gets
   * lost. CANFD_FDF allows programmers to mark CAN FD frames in the case of
   * using struct canfd_frame for mixed CAN / CAN FD content (dual use).
- * N.B. the Kernel APIs do NOT provide mixed CAN / CAN FD content inside of
- * struct canfd_frame therefore the CANFD_FDF flag is disregarded by Linux.
+ * Since the introduction of CAN XL the CANFD_FDF flag is set in all CAN FD
+ * frame structures provided by the CAN subsystem of the Linux kernel.
   */
  #define CANFD_BRS 0x01 /* bit rate switch (second bitrate for payload data) */
  #define CANFD_ESI 0x02 /* error state indicator of the transmitting node */
  #define CANFD_FDF 0x04 /* mark CAN FD for dual use of struct canfd_frame */
 

net/can/af_can.c +5 -0

index afa6c2151bc4..072a6a5c9dd1 100644
@@ -203,11 +203,16 @@ int can_send(struct sk_buff *skb, int loop)
         int err = -EINVAL;
 
         if (can_is_can_skb(skb)) {
                 skb->protocol = htons(ETH_P_CAN);
         } else if (can_is_canfd_skb(skb)) {
+               struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+
                 skb->protocol = htons(ETH_P_CANFD);
+
+               /* set CAN FD flag for CAN FD frames by default */
+               cfd->flags |= CANFD_FDF;
         } else {
                 goto inval_skb;
         }
 

Quoted 7 lines

net/can/af_can.c int can_send(struct sk_buff *skb, int loop) · line 218

Quoted 1 line

net/can/af_can.c +0 -0

         /* Make sure the CAN frame can pass the selected CAN netdevice. */

Once all the malloc and clone functions set the flag, is there still a route that would reach the can_send() without setting the CANFD_FDF flag? If the answer is supposedly no, maybe it is better to remove the check (or maybe replace it with a debug message to identify missed use cases).

The flag IS set in all skb malloc places (and therefore does not need to be set in clone places (see my answer above).

This specific check in can_send() especially covers content that came from userspace which is not setting this bit today.

The same is done for userspace content provided by a PF_PACKET socket in can_skb_headroom_valid() above.

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 inside 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 <[email protected]>

---
 include/uapi/linux/can.h | 51 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

include/uapi/linux/can.h +16 -0

index 7b23eeeb3273..dd645ea72306 100644
@@ -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,50 @@ 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
+ *

include/uapi/linux/can.h +4 -0

+
+/**
+ * struct canxl_frame - CAN with e'X'tended frame 'L'ength frame structure
+ * @prio:  11 bit arbitration priority with zero'ed CAN_*_FLAG flags

Sorry for the late reply, I am back from holidays. My previous review was a best effort review.

Since then, I took time to read the CiA’s CAN knowledge pages of CAN XL:
https://www.can-cia.org/can-knowledge/can/can-xl/
and have a few newer comments.

Unfortunately, I still do not have access to the full specification, so please forgive if there are some silly remarks.

Quoted 6 lines

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 inside a single frame.

Notable changes from CAN FD:

Quoted 7 lines

- 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)

Quoted 72 lines

Signed-off-by Oliver Hartkopp <[email protected]>

---
  include/uapi/linux/can.h | 51 ++++++++++++++++++++++++++++++++++++++++
  1 file changed, 51 insertions(+)

include/uapi/linux/can.h +13 -0

index 7b23eeeb3273..dd645ea72306 100644
@@ -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,50 @@ struct canfd_frame {
         __u8    __res0;  /* reserved / padding */
         __u8    __res1;  /* reserved / padding */
         __u8    data[CANFD_MAX_DLEN] __attribute__((aligned(8)));
  };
 

include/uapi/linux/can.h struct canfd_frame { · line 194

Quoted 13 lines

include/uapi/linux/can.h +13 -0

+/*
+ * 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) */

Are we sure that no other flags are needed? The CiA can-knowledge mentions a FTYP (frame type) flag. Do you know what it is?

Quoted 11 lines

include/uapi/linux/can.h +11 -0

+
+/**
+ * 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.

include/uapi/linux/can.h struct canfd_frame { · line 208

Quoted 3 lines

include/uapi/linux/can.h +3 -0

+ */
+struct canxl_frame {
+       canid_t prio;  /* 11 bit priority for arbitration (canid_t) */

Does it make sense to keep the canid_t? The addressing is done through both the prio and the af fields. Also, the CAN_EFF_FLAG, CAN_RTR_FLAG and CAN_ERR_FLAG flags do not apply anymore. Finally, the prio is only 11 bits, no need to reserve 32 bits for it.

Of course, if we declare it as 16 bits, we need to add some padding so that the other flags remain at the same offset.

include/uapi/linux/can.h struct canfd_frame { · line 214

Quoted 6 lines

include/uapi/linux/can.h +6 -0

+       __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];
+};

The CiA CAN knowledge also mentions a VCID (virtual CAN network ID). Is this not needed?

include/uapi/linux/can.h struct canfd_frame { · line 226

Quoted 12 lines

include/uapi/linux/can.h +5 -0

+
  #define CAN_MTU                (sizeof(struct can_frame))
  #define CANFD_MTU      (sizeof(struct canfd_frame))
+#define CANXL_MTU      (sizeof(struct canxl_frame))
+#define CANXL_HDR_SIZE (offsetof(struct canxl_frame, data))
+#define CANXL_MIN_MTU  (CANXL_HDR_SIZE + 64)
+#define CANXL_MAX_MTU  CANXL_MTU
 
  /* 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 */

CAN XL has a resXL flag, suggesting that there may be a fourth generation of the CAN protocol. One issue in the initial socket CAN design is that we never reserved a flag for future versions. Here, we are lucky that the bit 0x80 of the length field is never set. Back to CAN XL, I would like to discuss how we will distinguish the CAN GEN4 frames from the CAN XL ones so that we do not find ourselves stuck in a couple of years because no bits are left to differentiate things.

One solution would be to set the two most significant bit:
#define CAN_GEN4 0xC0

and the test would be:
if (cf.flags & CAN_GEN4 == CAN_GEN4)

Hi Vincent,

Quoted 2 lines · Vincent Mailhol

Sorry for the late reply, I am back from holidays. My previous review was a best effort review.

Hope you had good holidays ;-)

Quoted 6 lines

Since then, I took time to read the CiA’s CAN knowledge pages of CAN XL:
https://www.can-cia.org/can-knowledge/can/can-xl/
and have a few newer comments.

Unfortunately, I still do not have access to the full specification, so please forgive if there are some silly remarks.

I think the short introduction to CAN XL from the cover letter fits quite good.

https://www.bosch-semiconductors.com/media/ip_modules/pdf_2/can_xl_1/canxl_intro_20210225.pdf

Quoted 7 lines · Oliver Hartkopp

Quoted 6 lines

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 inside a single frame.

Notable changes from CAN FD:

Quoted 8 lines

Quoted 8 lines

- 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)

Did you note this?

Because you asked about the VCID later.

There is an existing Kernel API to handle VLAN-like ID stuff. And we should simply use that existing mechanic as it has the same intention and handling for CAN XL.

Quoted 87 lines

Quoted 87 lines

Signed-off-by Oliver Hartkopp <[email protected]>

---
  include/uapi/linux/can.h | 51 ++++++++++++++++++++++++++++++++++++++++

  1 file changed, 51 insertions(+)

include/uapi/linux/can.h +13 -0

index 7b23eeeb3273..dd645ea72306 100644
@@ -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,50 @@ 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.

Quoted 8 lines

Quoted 5 lines

+ */
+#define CANXL_XLF 0x80 /* mandatory CAN XL frame flag (must always be set!) */
+#define CANXL_SEC 0x01 /* Simple Extended Content (security/segmentation) */

Are we sure that no other flags are needed? The CiA can-knowledge mentions a FTYP (frame type) flag. Do you know what it is?

The LLC frame fields in https://www.can-cia.org/can-knowledge/can/can-xl/ are a mix(!) of Classical CAN, CAN FD and CAN XL content (e.g. ESI is only CAN FD).

The frame type flags an abstraction of
(none) -> Classical CAN
(FDF) -> CAN FD
(XLF) -> CAN XL

Quoted 13 lines

Quoted 13 lines

include/uapi/linux/can.h +3 -0

+
+/**
+ * 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.

Quoted 8 lines

Quoted 3 lines

+ */
+struct canxl_frame {
+       canid_t prio;  /* 11 bit priority for arbitration (canid_t) */

Does it make sense to keep the canid_t? The addressing is done through both the prio and the af fields. Also, the CAN_EFF_FLAG, CAN_RTR_FLAG and CAN_ERR_FLAG flags do not apply anymore. Finally, the prio is only 11 bits, no need to reserve 32 bits for it.

The CAN-ID was (due to its arbitration nature) always also a priority field.

So nothing changed here. There is no RTR and no 29-bit priority anymore now. The RTR flag has already been disabled for CAN FD (see presentation at the end of this mail).

Therefore it makes sense to handle the SFF 11-bit prio analogue to the former CAN-ID - and also use canid_t to simply keep the entire CAN filter handling in af_can.c !

The new idea in CAN XL is to split the priority from the content identification which has been named 'acceptance field' (AC).

The functionality of AC depends on SDT. Don't know how this will work out in the future and if we would provide some AF-specific filtering inside the kernel.

Quoted 3 lines

Of course, if we declare it as 16 bits, we need to add some padding so that the other flags remain at the same offset.

Answered above.

Quoted 9 lines

Quoted 6 lines

+       __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];
+};

The CiA CAN knowledge also mentions a VCID (virtual CAN network ID). Is this not needed?

Answered above.

Quoted 22 lines

Quoted 8 lines

+
  #define CAN_MTU                (sizeof(struct can_frame)) #define CANFD_MTU      (sizeof(struct canfd_frame))

include/uapi/linux/can.h +4 -0

+#define CANXL_MTU      (sizeof(struct canxl_frame))
+#define CANXL_HDR_SIZE (offsetof(struct canxl_frame, data))
+#define CANXL_MIN_MTU  (CANXL_HDR_SIZE + 64)
+#define CANXL_MAX_MTU  CANXL_MTU

Quoted 4 lines

  /* 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 */

CAN XL has a resXL flag, suggesting that there may be a fourth generation of the CAN protocol. One issue in the initial socket CAN design is that we never reserved a flag for future versions. Here, we are lucky that the bit 0x80 of the length field is never set. Back to CAN XL, I would like to discuss how we will distinguish the CAN GEN4 frames from the CAN XL ones so that we do not find ourselves stuck in a couple of years because no bits are left to differentiate things.

Quoted 5 lines

One solution would be to set the two most significant bit:
#define CAN_GEN4 0xC0

and the test would be:
if (cf.flags & CAN_GEN4 == CAN_GEN4)

Please do not mix up (incompatible) extensions that have been implemented in the CAN protocol through the 'wandering' reserved bit (r0, now resXL) with the CAN(FD/XL) data structure.

The first suggestion for a CAN frame data structure representation in IEEE 1722 contained the entire bitstream definition including the IDE bit. That splitted the 11 and 18 bits of the CAN ID inside the data structure :-D

I really think, that the journey is ending with CAN XL and when there would be really a new improvement, we will take a look at it. E.g. by extending the CAN XL flags from the top down (as you suggested with 0xC0).

But who knows how much of the CAN XL frame layout would be re-used by CAN XY ?? ¯\_(ツ)_/¯

Btw. I uploaded a presentation which shows the way from classical CAN to CAN XL which might depict some relations of the bits in a clearer way:

https://github.com/linux-can/can-doc/blob/master/presentations/CAN-XL-Intro.pdf

Best regards,
Oliver

Hi Vincent,

any update on this or are you fine with the patch set now?

Best regards,
Oliver

Quoted 7 lines · Oliver Hartkopp

Hi Vincent,

Quoted 2 lines · Vincent Mailhol

Sorry for the late reply, I am back from holidays. My previous review was a best effort review.

Hope you had good holidays ;-)

Quoted 11 lines

Quoted 6 lines

Since then, I took time to read the CiA’s CAN knowledge pages of CAN XL:
https://www.can-cia.org/can-knowledge/can/can-xl/
and have a few newer comments.

Unfortunately, I still do not have access to the full specification, so please forgive if there are some silly remarks.

I think the short introduction to CAN XL from the cover letter fits quite good.

https://www.bosch-semiconductors.com/media/ip_modules/pdf_2/can_xl_1/canxl_intro_20210225.pdf

Quoted 18 lines

Quoted 17 lines

Quoted 15 lines · 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 inside 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)

Quoted 7 lines

Did you note this?

Because you asked about the VCID later.

There is an existing Kernel API to handle VLAN-like ID stuff. And we should simply use that existing mechanic as it has the same intention and handling for CAN XL.

Quoted 99 lines

Quoted 98 lines

Quoted 87 lines

Signed-off-by Oliver Hartkopp <[email protected]>

---
  include/uapi/linux/can.h | 51 ++++++++++++++++++++++++++++++++++++++++
  1 file changed, 51 insertions(+)

include/uapi/linux/can.h +3 -0

index 7b23eeeb3273..dd645ea72306 100644
@@ -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

include/uapi/linux/can.h +10 -0

+/*
+ * 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)

include/uapi/linux/can.h +4 -0

@@ -164,12 +177,50 @@ 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.

Quoted 8 lines

+ *
+ * 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) */

Are we sure that no other flags are needed? The CiA can-knowledge mentions a FTYP (frame type) flag. Do you know what it is?

Quoted 8 lines

The LLC frame fields in
https://www.can-cia.org/can-knowledge/can/can-xl/ are a mix(!) of Classical CAN, CAN FD and CAN XL content (e.g. ESI is only CAN FD).

The frame type flags an abstraction of
(none) -> Classical CAN
(FDF) -> CAN FD
(XLF) -> CAN XL

Quoted 22 lines

Quoted 21 lines

Quoted 14 lines

include/uapi/linux/can.h +3 -0

+
+/**
+ * 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.
+ */

include/uapi/linux/can.h struct canfd_frame { · line 208

Quoted 2 lines

include/uapi/linux/can.h +2 -0

+struct canxl_frame {
+       canid_t prio;  /* 11 bit priority for arbitration (canid_t) */

Does it make sense to keep the canid_t? The addressing is done through both the prio and the af fields. Also, the CAN_EFF_FLAG, CAN_RTR_FLAG and CAN_ERR_FLAG flags do not apply anymore. Finally, the prio is only 11 bits, no need to reserve 32 bits for it.

Quoted 2 lines

The CAN-ID was (due to its arbitration nature) always also a priority field.

Quoted 7 lines

So nothing changed here. There is no RTR and no 29-bit priority anymore now. The RTR flag has already been disabled for CAN FD (see presentation at the end of this mail).

Therefore it makes sense to handle the SFF 11-bit prio analogue to the former CAN-ID - and also use canid_t to simply keep the entire CAN filter handling in af_can.c !

Quoted 6 lines

The new idea in CAN XL is to split the priority from the content identification which has been named 'acceptance field' (AC).

The functionality of AC depends on SDT. Don't know how this will work out in the future and if we would provide some AF-specific filtering inside the kernel.

Quoted 13 lines

Quoted 3 lines

Of course, if we declare it as 16 bits, we need to add some padding so that the other flags remain at the same offset.

Answered above.

Quoted 7 lines

Quoted 6 lines

include/uapi/linux/can.h +6 -0

+       __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];
+};

Quoted 4 lines

Quoted 2 lines

The CiA CAN knowledge also mentions a VCID (virtual CAN network ID). Is this not needed?

Answered above.

Quoted 41 lines

Quoted 22 lines

include/uapi/linux/can.h struct canfd_frame { · line 226

Quoted 12 lines

include/uapi/linux/can.h +5 -0

+
  #define CAN_MTU                (sizeof(struct can_frame))
  #define CANFD_MTU      (sizeof(struct canfd_frame))
+#define CANXL_MTU      (sizeof(struct canxl_frame))
+#define CANXL_HDR_SIZE (offsetof(struct canxl_frame, data))
+#define CANXL_MIN_MTU  (CANXL_HDR_SIZE + 64)
+#define CANXL_MAX_MTU  CANXL_MTU
 
  /* 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 */

CAN XL has a resXL flag, suggesting that there may be a fourth generation of the CAN protocol. One issue in the initial socket CAN design is that we never reserved a flag for future versions. Here, we are lucky that the bit 0x80 of the length field is never set. Back to CAN XL, I would like to discuss how we will distinguish the CAN GEN4 frames from the CAN XL ones so that we do not find ourselves stuck in a couple of years because no bits are left to differentiate things.

Quoted 5 lines

One solution would be to set the two most significant bit:
#define CAN_GEN4 0xC0

and the test would be:
if (cf.flags & CAN_GEN4 == CAN_GEN4)

Please do not mix up (incompatible) extensions that have been implemented in the CAN protocol through the 'wandering' reserved bit (r0, now resXL) with the CAN(FD/XL) data structure.

The first suggestion for a CAN frame data structure representation in IEEE 1722 contained the entire bitstream definition including the IDE bit. That splitted the 11 and 18 bits of the CAN ID inside the data structure :-D

I really think, that the journey is ending with CAN XL and when there would be really a new improvement, we will take a look at it. E.g. by extending the CAN XL flags from the top down (as you suggested with 0xC0).

Quoted 7 lines

But who knows how much of the CAN XL frame layout would be re-used by CAN XY ?? ¯\_(ツ)_/¯

Btw. I uploaded a presentation which shows the way from classical CAN to CAN XL which might depict some relations of the bits in a clearer way:

https://github.com/linux-can/can-doc/blob/master/presentations/CAN-XL-Intro.pdf

On Thu. 8 Sep. 2022 at 16:24, Oliver Hartkopp <[email protected]> wrote:

Quoted 2 lines

Quoted 2 lines

The CAN-ID was (due to its arbitration nature) always also a priority field.

The CAN-(FD) ID holds two roles: priority and ID. For CAN XL, it is only ID.

While I agree that both have the priority attributes, my concerns are on the semantics. The type is canid_t, not canprio_t. I just want to point that it is weird to had ID in the type when that field doesn't have an ID property anymore.

Quoted 7 lines

Quoted 7 lines

So nothing changed here. There is no RTR and no 29-bit priority anymore now. The RTR flag has already been disabled for CAN FD (see presentation at the end of this mail).

Therefore it makes sense to handle the SFF 11-bit prio analogue to the former CAN-ID - and also use canid_t to simply keep the entire CAN filter handling in af_can.c !

This is a good argument to keep using the canid_t. If we make it a u16, we lose the alignment (unless we add dirty endianness preprocessing macros).

Quoted 4 lines

Quoted 4 lines

Btw. I uploaded a presentation which shows the way from classical CAN to CAN XL which might depict some relations of the bits in a clearer way:

https://github.com/linux-can/can-doc/blob/master/presentations/CAN-XL-Intro.pdf

Thanks. With the Bosch PDF now returning error 404, I suggest replacing the link in the cover letter with your link (or the CIA knowledge page).

Hi Vincent,

Quoted 1 line · Vincent Mailhol

On Thu. 8 Sep. 2022 at 16:24, Oliver Hartkopp <[email protected]> wrote:

(..)

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

The CAN-ID was (due to its arbitration nature) always also a priority field.

The CAN-(FD) ID holds two roles: priority and ID. For CAN XL, it is only ID.

Typo? It is only the priority ;-)

Quoted 4 lines

While I agree that both have the priority attributes, my concerns are on the semantics. The type is canid_t, not canprio_t. I just want to point that it is weird to had ID in the type when that field doesn't have an ID property anymore.

CAN XL moves away from the two roles combined in the CAN(FD) Identifier. The main focus is on arbitration now (CAN XL is like CAN arbitration with Ethernet data).

But in the end the CAN bitstream at the beginning of the frame including the arbitration is completely identical for all CAN variants.

Even when this arbitration field is now named priority for CAN XL it is still the exact mechanism of the CAN identifier (what we introduces canid_t for).

Therefore having canid_t for can_id and prio seems appropriate.

Quoted 11 lines

Quoted 7 lines

Quoted 7 lines

So nothing changed here. There is no RTR and no 29-bit priority anymore now. The RTR flag has already been disabled for CAN FD (see presentation at the end of this mail).

Therefore it makes sense to handle the SFF 11-bit prio analogue to the former CAN-ID - and also use canid_t to simply keep the entire CAN filter handling in af_can.c !

This is a good argument to keep using the canid_t. If we make it a u16, we lose the alignment (unless we add dirty endianness preprocessing macros).

Yep.

(..)

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Btw. I uploaded a presentation which shows the way from classical CAN to CAN XL which might depict some relations of the bits in a clearer way:

Quoted 5 lines

Quoted 1 line

Quoted 1 line

https://github.com/linux-can/can-doc/blob/master/presentations/CAN-XL-Intro.pdf

Thanks. With the Bosch PDF now returning error 404, I suggest replacing the link in the cover letter with your link (or the CIA knowledge page).

Ok, will do.

- add new ETH_P_CANXL ethernet protocol type
- update skb checks for CAN XL
- add alloc_canxl_skb() which now needs a data length parameter
- introduce init_can_skb_reserve() to reduce code duplication

Signed-off-by Oliver Hartkopp <[email protected]>

---
 drivers/net/can/dev/skb.c     | 72 ++++++++++++++++++++++++++---------
 include/linux/can/skb.h       | 23 ++++++++++-
 include/uapi/linux/if_ether.h |  1 +
 net/can/af_can.c              | 25 +++++++++++-
 4 files changed, 101 insertions(+), 20 deletions(-)

drivers/net/can/dev/skb.c +54 -18

index adb413bdd734..f2ec20d80aba 100644
@@ -185,10 +185,24 @@ void can_free_echo_skb(struct net_device *dev, unsigned int idx,
                priv->echo_skb[idx] = NULL;
        }
 }
 EXPORT_SYMBOL_GPL(can_free_echo_skb);
 
+/* fill common values for CAN sk_buffs */
+static void init_can_skb_reserve(struct sk_buff *skb)
+{
+       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)->skbcnt = 0;
+}
+
 struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
 {
        struct sk_buff *skb;
 
        skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +

@@ -198,20 +212,12 @@ struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)
 
                return NULL;
        }
 
        skb->protocol = htons(ETH_P_CAN);
-       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);
+       init_can_skb_reserve(skb);
        can_skb_prv(skb)->ifindex = dev->ifindex;
-       can_skb_prv(skb)->skbcnt = 0;
 
        *cf = skb_put_zero(skb, sizeof(struct can_frame));
 
        return skb;
 }

@@ -229,30 +235,55 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
 
                return NULL;
        }
 
        skb->protocol = htons(ETH_P_CANFD);
-       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);
+       init_can_skb_reserve(skb);
        can_skb_prv(skb)->ifindex = dev->ifindex;
-       can_skb_prv(skb)->skbcnt = 0;
 
        *cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
 
        /* set CAN FD flag by default */
        (*cfd)->flags = CANFD_FDF;
 
        return skb;
 }
 EXPORT_SYMBOL_GPL(alloc_canfd_skb);
 
+struct sk_buff *alloc_canxl_skb(struct net_device *dev,
+                               struct canxl_frame **cfx,
+                               unsigned int data_len)
+{
+       struct sk_buff *skb;
+
+       if (data_len < CANXL_MIN_DLEN || data_len > CANXL_MAX_DLEN)
+               goto out_error;
+
+       skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +
+                              CANXL_HDR_SIZE + data_len);
+       if (unlikely(!skb))
+               goto out_error;
+
+       skb->protocol = htons(ETH_P_CANXL);
+       init_can_skb_reserve(skb);
+       can_skb_prv(skb)->ifindex = dev->ifindex;
+
+       *cfx = skb_put_zero(skb, CANXL_HDR_SIZE + data_len);
+
+       /* set CAN XL flag and length information by default */
+       (*cfx)->flags = CANXL_XLF;
+       (*cfx)->len = data_len;
+
+       return skb;
+
+out_error:
+       *cfx = NULL;
+
+       return NULL;
+}
+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);

@@ -317,10 +348,15 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb)
        case ETH_P_CANFD:
                if (!can_is_canfd_skb(skb))
                        goto inval_skb;
                break;
 
+       case ETH_P_CANXL:
+               if (!can_is_canxl_skb(skb))
+                       goto inval_skb;
+               break;
+
        default:
                goto inval_skb;
        }
 
        if (!can_skb_headroom_valid(dev, skb)) {

include/linux/can/skb.h +12 -1

index ddffc2fc008c..01c01b1261fa 100644
@@ -28,10 +28,13 @@ unsigned int __must_check can_get_echo_skb(struct net_device *dev,
 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,
+                               unsigned int data_len);
 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);
 
 /*

@@ -112,15 +115,33 @@ 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 && cfd->len <= CANFD_MAX_DLEN);
 }
 
-/* get length element value from can[fd]_frame structure */
+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_HDR_SIZE + CANXL_MIN_DLEN || skb->len > CANXL_MTU)
+               return false;
+
+       /* this also checks valid CAN XL data length boundaries */
+       if (skb->len != CANXL_HDR_SIZE + cfx->len)

include/linux/can/skb.h +3 -0

+       if (can_is_canxl_skb(skb))
+               return cfx->len;
+
        return cfd->len;
 }
 
 /* get needed data length inside CAN frame for all frame types (RTR aware) */
 static inline unsigned int can_skb_get_data_len(struct sk_buff *skb)

include/uapi/linux/if_ether.h +1 -0

index d370165bc621..69e0457eb200 100644
@@ -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 ([email protected])       */
 #define ETH_P_CONTROL  0x0016          /* Card specific control frames */
 #define ETH_P_IRDA     0x0017          /* Linux-IrDA                   */

net/can/af_can.c +23 -1

index 072a6a5c9dd1..9503ab10f9b8 100644
@@ -200,11 +200,13 @@ int can_send(struct sk_buff *skb, int loop)
 {
        struct sk_buff *newskb = NULL;
        struct can_pkg_stats *pkg_stats = dev_net(skb->dev)->can.pkg_stats;
        int err = -EINVAL;
 
-       if (can_is_can_skb(skb)) {
+       if (can_is_canxl_skb(skb)) {
+               skb->protocol = htons(ETH_P_CANXL);
+       } else if (can_is_can_skb(skb)) {
                skb->protocol = htons(ETH_P_CAN);
        } else if (can_is_canfd_skb(skb)) {
                struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
 
                skb->protocol = htons(ETH_P_CANFD);

@@ -700,10 +702,25 @@ static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
 
        can_receive(skb, dev);
        return NET_RX_SUCCESS;
 }
 
+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

@@ -824,10 +841,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,
 };

@@ -863,10 +885,11 @@ static __init int can_init(void)
        if (err)

On Tue. 2 Aug. 2022 at 04:02, Oliver Hartkopp <[email protected]> wrote:

include/linux/can/skb.h static inline bool can_is_canfd_skb(const struct sk_buff *skb) · line 138

Quoted 5 lines

include/linux/can/skb.h +2 -0

+/* 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;

Nitpick: what would be the acronyms for cfx and cfd? I thought that cfd was for *C*AN-*FD* frame, and thus I would expect cxl instead of cfx for *C*AN-*XL* frame.
On the contrary, if cfx stands for *C*AN *F*rame *X*L, then for CAN-FD, the acronym should be cff (*C*AN *f*rame *F*D).

Quoted 1 line · Vincent Mailhol

On Tue. 2 Aug. 2022 at 04:02, Oliver Hartkopp <[email protected]> wrote:

(..)

Quoted 11 lines

include/linux/can/skb.h static inline bool can_is_canfd_skb(const struct sk_buff *skb) · line 138

Quoted 5 lines

include/linux/can/skb.h +2 -0

+/* 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;

Nitpick: what would be the acronyms for cfx and cfd? I thought that cfd was for *C*AN-*FD* frame, and thus I would expect cxl instead of cfx for *C*AN-*XL* frame.
On the contrary, if cfx stands for *C*AN *F*rame *X*L, then for CAN-FD, the acronym should be cff (*C*AN *f*rame *F*D).

You need to start from the original

struct can_frame cf; *C*AN *F*RAME

Then CAN FD showed up and the naming moved from 'cf' to 'cfd' for *C*AN *FD* FRAME where is was not forseable that there ever would be CAN XL.

For me it is more intuitive to generally name CAN frames 'cf<whatever>'.

cf -> cfd -> cfx

So it is about 'cf' with an extra attribute and not an abbreviation of CAN variants.

Quoted 24 lines · Oliver Hartkopp

Quoted 1 line · Vincent Mailhol

On Tue. 2 Aug. 2022 at 04:02, Oliver Hartkopp <[email protected]> wrote:

(..)

Quoted 11 lines

include/linux/can/skb.h static inline bool can_is_canfd_skb(const struct sk_buff *skb) · line 138

Quoted 5 lines

include/linux/can/skb.h +2 -0

+/* 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;

Nitpick: what would be the acronyms for cfx and cfd? I thought that cfd was for *C*AN-*FD* frame, and thus I would expect cxl instead of cfx for *C*AN-*XL* frame.
On the contrary, if cfx stands for *C*AN *F*rame *X*L, then for CAN-FD, the acronym should be cff (*C*AN *f*rame *F*D).

You need to start from the original

struct can_frame cf; *C*AN *F*RAME

Then CAN FD showed up and the naming moved from 'cf' to 'cfd' for *C*AN *FD* FRAME where is was not forseable that there ever would be CAN XL.

Quoted 6 lines

For me it is more intuitive to generally name CAN frames 'cf<whatever>'.

cf -> cfd -> cfx

So it is about 'cf' with an extra attribute and not an abbreviation of CAN variants.

I still disagree on that one. For me:
* Classical CAN frames: ccf (or the legacy cf before introduction of CAN-FD)
* CAN FD frames: cfd
* CAN XL frames: cxl
is the most consistent.

cfx is not consistent with the cfd acronym and it is out of order: the structure name is canxl_frame, not can_frame_xl.
At least, that should be cxf for struct *c*an*x*l_*f*rame. CAN frame XL just sounds odd to me.

Regardless, this remains a nitpick and I will not NACK the series for that. So do as you prefer in the v9, my Acked-by remains valid.

Yours sincerely,
Vincent Mailhol

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())

V6:

- rework an separate skb identification and length helpers
- add CANFD_FDF flag in all CAN FD frame structures
- simplify patches for infrastructure and raw sockets
- add vxcan support in virtual CAN interface patch

V7:

- fixed indention as remarked by Marc
- set CANFD_FDF flag when detecting CAN FD frames generated by PF_PACKET
- Allow to use variable CAN XL MTU sizes to enforce real time requirements on CAN XL segments (e.g. to support of CAN CiA segmentation concept)

V8:

- fixed typo as remarked by Vincent
- rebased to latest can-next/net-next tree

Oliver Hartkopp (7):
can: skb: unify skb CAN frame identification helpers
can: skb: add skb CAN frame data length helpers
can: set CANFD_FDF flag in all CAN FD frame structures
can: canxl: introduce CAN XL data structure
can: canxl: update CAN infrastructure for CAN XL frames
can: dev: add CAN XL support to virtual CAN
can: raw: add CAN XL support

  drivers/net/can/ctucanfd/ctucanfd_base.c |   1 -
  drivers/net/can/dev/rx-offload.c         |   2 +-
  drivers/net/can/dev/skb.c                | 113 ++++++++++++++++-------
  drivers/net/can/vcan.c                   |  12 +--
  drivers/net/can/vxcan.c                  |   8 +-
  include/linux/can/dev.h                  |   5 +
  include/linux/can/skb.h                  |  57 +++++++++++-
  include/uapi/linux/can.h                 |  55 ++++++++++-
  include/uapi/linux/can/raw.h             |   1 +
  include/uapi/linux/if_ether.h            |   1 +
  net/can/af_can.c                         |  76 ++++++++-------
  net/can/bcm.c                            |   9 +-
  net/can/gw.c                             |   4 +-
  net/can/isotp.c                          |   2 +-
  net/can/j1939/main.c                     |   4 +
  net/can/raw.c                            |  55 ++++++++---
  16 files changed, 299 insertions(+), 106 deletions(-)

Hi Vincent,

I have discussed with Marc about this patch set and after our discussion about len/flags he was missing an ACK from your side:

https://lore.kernel.org/linux-can/[email protected]/T/#u

Are you fine with this V8 patch set now?

I already created some CAN CiA p-o-c code for CAN XL segmentation:

https://github.com/hartkopp/can-cia-613-3-poc

When the patch set is committed and the definitions are fixed I would also start with some documentation.

Quoted 48 lines

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())

V6:

- rework an separate skb identification and length helpers
- add CANFD_FDF flag in all CAN FD frame structures
- simplify patches for infrastructure and raw sockets
- add vxcan support in virtual CAN interface patch

V7:

- fixed indention as remarked by Marc
- set CANFD_FDF flag when detecting CAN FD frames generated by PF_PACKET
- Allow to use variable CAN XL MTU sizes to enforce real time requirements on CAN XL segments (e.g. to support of CAN CiA segmentation concept)

V8:

Quoted 8 lines

- fixed typo as remarked by Vincent
- rebased to latest can-next/net-next tree

Oliver Hartkopp (7):
can: skb: unify skb CAN frame identification helpers
can: skb: add skb CAN frame data length helpers
can: set CANFD_FDF flag in all CAN FD frame structures
can: canxl: introduce CAN XL data structure

Quoted 3 lines

can: canxl: update CAN infrastructure for CAN XL frames
can: dev: add CAN XL support to virtual CAN
can: raw: add CAN XL support

Quoted 9 lines

  drivers/net/can/ctucanfd/ctucanfd_base.c |   1 -
  drivers/net/can/dev/rx-offload.c         |   2 +-
  drivers/net/can/dev/skb.c                | 113 ++++++++++++++++-------
  drivers/net/can/vcan.c                   |  12 +--
  drivers/net/can/vxcan.c                  |   8 +-
  include/linux/can/dev.h                  |   5 +
  include/linux/can/skb.h                  |  57 +++++++++++-
  include/uapi/linux/can.h                 |  55 ++++++++++-
  include/uapi/linux/can/raw.h             |   1 +

Quoted 8 lines

  include/uapi/linux/if_ether.h            |   1 +
  net/can/af_can.c                         |  76 ++++++++-------
  net/can/bcm.c                            |   9 +-
  net/can/gw.c                             |   4 +-
  net/can/isotp.c                          |   2 +-
  net/can/j1939/main.c                     |   4 +
  net/can/raw.c                            |  55 ++++++++---
  16 files changed, 299 insertions(+), 106 deletions(-)

Hi Oliver,

Quoted 7 lines · Oliver Hartkopp

Hi Vincent,

I have discussed with Marc about this patch set and after our discussion about len/flags he was missing an ACK from your side:

https://lore.kernel.org/linux-can/[email protected]/T/#u

Quoted 8 lines

Are you fine with this V8 patch set now?

I already created some CAN CiA p-o-c code for CAN XL segmentation:

https://github.com/hartkopp/can-cia-613-3-poc

When the patch set is committed and the definitions are fixed I would also start with some documentation.

Sorry for the delay, it was tough to find time for the final review.

Quoted 49 lines · Oliver Hartkopp

Quoted 48 lines

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())

V6:

- rework an separate skb identification and length helpers
- add CANFD_FDF flag in all CAN FD frame structures
- simplify patches for infrastructure and raw sockets
- add vxcan support in virtual CAN interface patch

V7:

- fixed indention as remarked by Marc
- set CANFD_FDF flag when detecting CAN FD frames generated by PF_PACKET
- Allow to use variable CAN XL MTU sizes to enforce real time requirements on CAN XL segments (e.g. to support of CAN CiA segmentation concept)

V8:

Quoted 8 lines

Quoted 8 lines

- fixed typo as remarked by Vincent
- rebased to latest can-next/net-next tree

Oliver Hartkopp (7):
can: skb: unify skb CAN frame identification helpers
can: skb: add skb CAN frame data length helpers
can: set CANFD_FDF flag in all CAN FD frame structures
can: canxl: introduce CAN XL data structure

As pointed out in the past, I still prefer to make canxl_fame::data a flexible array (data[]) instead of specifying the maximum length (i.e. data[CANXL_MAX_DLEN]). But as no one else manifested any objections on the mailing, I conclude that the majority is either fine with the data[CANXL_MAX_DLEN] or indifferent. Thus, I will acknowledge this patch despite having a different personal opinion on the topic.

Quoted 3 lines

Quoted 3 lines

can: canxl: update CAN infrastructure for CAN XL frames
can: dev: add CAN XL support to virtual CAN
can: raw: add CAN XL support

I send two additional comments for patches 3 and 5. Those two last comments being minor ones, I let you directly handle those. With that said:

Acked-by Vincent Mailhol <[email protected]>

(valid for the full series, you can directly add this to the v9).

Quoted 10 lines

Quoted 10 lines

   drivers/net/can/ctucanfd/ctucanfd_base.c |   1 -
   drivers/net/can/dev/rx-offload.c         |   2 +-
   drivers/net/can/dev/skb.c                | 113 ++++++++++++++++-------
   drivers/net/can/vcan.c                   |  12 +--
   drivers/net/can/vxcan.c                  |   8 +-
   include/linux/can/dev.h                  |   5 +
   include/linux/can/skb.h                  |  57 +++++++++++-
   include/uapi/linux/can.h                 |  55 ++++++++++-
   include/uapi/linux/can/raw.h             |   1 +

Quoted 8 lines

Quoted 8 lines

   include/uapi/linux/if_ether.h            |   1 +
   net/can/af_can.c                         |  76 ++++++++-------
   net/can/bcm.c                            |   9 +-
   net/can/gw.c                             |   4 +-
   net/can/isotp.c                          |   2 +-
   net/can/j1939/main.c                     |   4 +
   net/can/raw.c                            |  55 ++++++++---
   16 files changed, 299 insertions(+), 106 deletions(-)

Yours sincerely,
Vincent Mailhol