canfd: add new data structures and constants

include/linux/can.h typedef __u32 canid_t; · line 50

Quoted 8 lines · Oliver Hartkopp

include/linux/can.h +2 -0

@@ -46,18 +46,65 @@ typedef __u32 canid_t;
  */
 typedef __u32 can_err_mask_t;
 +#define CAN_MAX_DLC 8
+#define CAN_MAX_DLEN 8
+

Hm looks broken - but the patch and the repo content is ok.

Oh, well.

Anyone an idea how to set up imap with strato.de for git send-imap with a debian system ??

- add new struct canfd_frame
- check identical element offsets in struct can_frame and struct canfd_frame
- new ETH_P_CANFD definition to tag CAN FD skbs correctly
- add CAN_MTU and CANFD_MTU definitions for easy frame and mode detection
- add CAN[FD]_MAX_[DLC|DLEN] helper constants to remove hard coded values

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

---
 include/linux/can.h      |   55 ++++++++++++++++++++++++++++++++++++++++++----
 include/linux/if_ether.h |    3 ++-
 net/can/af_can.c         |    7 ++++++
 3 files changed, 60 insertions(+), 5 deletions(-)

include/linux/can.h +6 -3

index 17334c0..25265e7 100644
@@ -46,18 +46,65 @@ typedef __u32 canid_t;
  */
 typedef __u32 can_err_mask_t;
 +#define CAN_MAX_DLC 8
+#define CAN_MAX_DLEN 8
+
+#define CANFD_MAX_DLC 15
+#define CANFD_MAX_DLEN 64
+
 /**
  * struct can_frame - basic CAN frame structure
- * @can_id:  the CAN ID of the frame and CAN_*_FLAG flags, see above.
- * @can_dlc: the data length field of the CAN frame
- * @data:    the CAN frame payload.
+ * @can_id:  CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition

include/linux/can.h typedef __u32 canid_t; · line 65

Quoted 8 lines · Oliver Hartkopp

- add new struct canfd_frame

struct can_frame {

include/linux/can.h +1 -1

 	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
-	__u8    can_dlc; /* data length code: 0 .. 8 */
+	__u8    can_dlc; /* frame payload length in byte (0 .. 8) */
 	__u8    data[8] __attribute__((aligned(8)));

checkpatch complains:

WARNING: __aligned(size) is preferred over __attribute__((aligned(size))) #84: FILE: include/linux/can.h:102:
+ __u8 data[64] __attribute__((aligned(8)));

total: 0 errors, 1 warnings, 97 lines checked

Review

Quoted 2 lines

(quoted diff) +1 -1

- 	__u8    data[8] __attribute__((aligned(8)));
+ 	__u8    data[8] __aligned(8);

Quoted 15 lines · Marc Kleine-Budde

include/linux/can.h typedef __u32 canid_t; · line 65

Quoted 7 lines · Oliver Hartkopp

- add new struct canfd_frame

struct can_frame {

include/linux/can.h +1 -1

 	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
-	__u8    can_dlc; /* data length code: 0 .. 8 */
+	__u8    can_dlc; /* frame payload length in byte (0 .. 8) */
 	__u8    data[8] __attribute__((aligned(8)));

checkpatch complains:

WARNING: __aligned(size) is preferred over __attribute__((aligned(size))) #84: FILE: include/linux/can.h:102:
+ __u8 data[64] __attribute__((aligned(8)));

Quoted 1 line

total: 0 errors, 1 warnings, 97 lines checked

Hm, yes. I wanted to have it the same way as in struct can_frame.

I wonder if "__aligened(size)" is also compatible for backports - or alternative if we should also modify the original can_frame:

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +1 -1

- 	__u8    data[8] __attribute__((aligned(8)));
+ 	__u8    data[8] __aligned(8);

If you want to be super clean you can first make a patch that changes the existing __attribute__((aligned(8))) to __aligned(8), and then add your canfd series. If not, just mention it in the patch description (While being here change can_fd to __aligned(8)).

Err..this is a userspace header, I just tested it, headers_install doesn't remove it. And userspace fails to compile with this. For now stick to __attribute__((aligned(8))).

Quoted 5 lines · Marc Kleine-Budde

If you want to be super clean you can first make a patch that changes the existing __attribute__((aligned(8))) to __aligned(8), and then add your canfd series. If not, just mention it in the patch description (While being here change can_fd to __aligned(8)).

Quoted 3 lines

Err..this is a userspace header, I just tested it, headers_install doesn't remove it. And userspace fails to compile with this. For now stick to __attribute__((aligned(8))).

BTW: and it fails to compile due to a __kernel_sa_family_t which isn't defined on my ubuntu with 2.6.32er headers

Quoted 6 lines

Quoted 3 lines

Err..this is a userspace header, I just tested it, headers_install doesn't remove it. And userspace fails to compile with this. For now stick to __attribute__((aligned(8))).

BTW: and it fails to compile due to a __kernel_sa_family_t which isn't defined on my ubuntu with 2.6.32er headers

__kernel_sa_family_t is defined in include/linux/socket.h but with

#ifdef __KERNEL__

around it.

typedef __kernel_sa_family_t sa_family_t;

What do you compile?

If you compile a kernel -> it is defined. If you compile an application -> it is not needed.

- add new struct canfd_frame
- check identical element offsets in struct can_frame and struct canfd_frame
- new ETH_P_CANFD definition to tag CAN FD skbs correctly
- add CAN_MTU and CANFD_MTU definitions for easy frame and mode detection
- add CAN[FD]_MAX_[DLC|DLEN] helper constants to remove hard coded values

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

---
 include/linux/can.h      |   55 ++++++++++++++++++++++++++++++++++++++++++----
 include/linux/if_ether.h |    3 ++-
 net/can/af_can.c         |    7 ++++++
 3 files changed, 60 insertions(+), 5 deletions(-)

include/linux/can.h +34 -4

index 17334c0..25265e7 100644
@@ -46,18 +46,65 @@ typedef __u32 canid_t;
  */
 typedef __u32 can_err_mask_t;
 +#define CAN_MAX_DLC 8
+#define CAN_MAX_DLEN 8
+
+#define CANFD_MAX_DLC 15
+#define CANFD_MAX_DLEN 64
+
 /**
  * struct can_frame - basic CAN frame structure
- * @can_id:  the CAN ID of the frame and CAN_*_FLAG flags, see above.
- * @can_dlc: the data length field of the CAN frame
- * @data:    the CAN frame payload.
+ * @can_id:  CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
+ * @can_dlc: frame payload length in byte (0 .. 8) aka data length code
+ *           N.B. the DLC field from ISO 11898-1 Chapter 8.4.2.3 has a 1:1
+ *           mapping of the 'data length code' to the real payload length
+ * @data:    CAN frame payload (up to 8 byte)
  */
 struct can_frame {
 	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
-	__u8    can_dlc; /* data length code: 0 .. 8 */
+	__u8    can_dlc; /* frame payload length in byte (0 .. 8) */
 	__u8    data[8] __attribute__((aligned(8)));
 };
 +/*
+ * defined bits for canfd_frame.flags
+ *
+ * As the default for CAN FD should be to support the high data rate in the
+ * payload section of the frame (HDR) and to support up to 64 byte in the
+ * data section (EDL) the bits are only set in the non-default case.
+ * Btw. as long as there's no real implementation for CAN FD network driver
+ * these bits are only preliminary.
+ *
+ * RX: NOHDR/NOEDL - info about received CAN FD frame
+ *     ESI         - bit from originating CAN controller
+ * TX: NOHDR/NOEDL - control per-frame settings if supported by CAN controller
+ *     ESI         - bit is set by local CAN controller
+ */
+#define CANFD_NOHDR 0x01 /* frame without high data rate */
+#define CANFD_NOEDL 0x02 /* frame without extended data length */
+#define CANFD_ESI   0x04 /* error state indicator */
+
+/**
+ * struct canfd_frame - CAN flexible data rate frame structure
+ * @can_id: CAN ID of the frame and CAN_*_FLAG flags, see canid_t definition
+ * @len:    frame payload length in byte (0 .. 64)
+ * @flags:  additional flags for CAN FD
+ * @__res0: reserved / padding

include/linux/can.h +5 -0

+};
+
+#define CAN_MTU		(sizeof(struct can_frame))
+#define CANFD_MTU	(sizeof(struct canfd_frame))
+
 /* particular protocols of the protocol family PF_CAN */
 #define CAN_RAW		1 /* RAW sockets */
 #define CAN_BCM		2 /* Broadcast Manager */

include/linux/if_ether.h +0 -0

index 56d907a..260138b 100644

include/linux/if_ether.h +0 -0

 #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])	*/

net/can/af_can.c +7 -0

index 6efcd37..c96140a 100644
@@ -41,6 +41,7 @@
  */
  #include <linux/module.h>
+#include <linux/stddef.h>
 #include <linux/init.h>
 #include <linux/kmod.h>
 #include <linux/slab.h>

@@ -824,6 +825,12 @@ static struct notifier_block can_netdev_notifier __read_mostly = {
  static __init int can_init(void)
 {
+	/* check for correct padding to be able to use the structs similarly */
+	BUILD_BUG_ON(offsetof(struct can_frame, can_dlc) !=
+		     offsetof(struct canfd_frame, len) ||
+		     offsetof(struct can_frame, data) !=
+		     offsetof(struct canfd_frame, data));
+
 	printk(banner);
  	memset(&can_rx_alldev_list, 0, sizeof(can_rx_alldev_list));

Hi Oliver,

Quoted 26 lines · Oliver Hartkopp

- add new struct canfd_frame
- check identical element offsets in struct can_frame and struct canfd_frame
- new ETH_P_CANFD definition to tag CAN FD skbs correctly
- add CAN_MTU and CANFD_MTU definitions for easy frame and mode detection
- add CAN[FD]_MAX_[DLC|DLEN] helper constants to remove hard coded values

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

---
 include/linux/can.h      |   55 ++++++++++++++++++++++++++++++++++++++++++----
 include/linux/if_ether.h |    3 ++-
 net/can/af_can.c         |    7 ++++++
 3 files changed, 60 insertions(+), 5 deletions(-)

include/linux/can.h +5 -0

index 17334c0..25265e7 100644
@@ -46,18 +46,65 @@ typedef __u32 canid_t;
  */
 typedef __u32 can_err_mask_t;
 +#define CAN_MAX_DLC 8
+#define CAN_MAX_DLEN 8
+
+#define CANFD_MAX_DLC 15
+#define CANFD_MAX_DLEN 64
+

Quoted 6 lines

struct can_frame {

include/linux/can.h +1 -1

 	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
-	__u8    can_dlc; /* data length code: 0 .. 8 */
+	__u8    can_dlc; /* frame payload length in byte (0 .. 8) */
 	__u8    data[8] __attribute__((aligned(8)));
 };

Quoted 4 lines

+ * @data: CAN FD frame payload (up to 64 byte)
+ */

include/linux/can.h +2 -0

+struct canfd_frame {
+	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */

include/linux/can.h typedef __u32 canid_t; · line 100

Quoted 5 lines

include/linux/can.h +5 -0

+	__u8    len;     /* frame payload length in byte (0 .. 64) */
+	__u8    flags;   /* additional flags for CAN FD */
+	__u8    __res0;  /* reserved / padding */
+	__u8    __res1;  /* reserved / padding */
+	__u8    data[64] __attribute__((aligned(8)));

Shouldn't we use "CANFD_MAX_DLEN" here (... and maybe above as well)?. IIRC, CANFD_MAX_DLEN might be even 128 in the finalized standard.

include/linux/can.h

Quoted 3 lines

include/linux/can.h +2 -1

-#define ETH_P_CAN	0x000C		/* Controller Area Network      */
+#define ETH_P_CAN	0x000C		/* Controller Area Network (CAN)*/
+#define ETH_P_CANFD	0x000D		/* CAN FD 64 byte payload frames*/

Then hardcoding 64 here is also not be nice.

Quoted 29 lines · Wolfgang Grandegger

Hi Oliver,

Quoted 26 lines · Oliver Hartkopp

- add new struct canfd_frame
- check identical element offsets in struct can_frame and struct canfd_frame
- new ETH_P_CANFD definition to tag CAN FD skbs correctly
- add CAN_MTU and CANFD_MTU definitions for easy frame and mode detection
- add CAN[FD]_MAX_[DLC|DLEN] helper constants to remove hard coded values

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

---
 include/linux/can.h      |   55 ++++++++++++++++++++++++++++++++++++++++++----
 include/linux/if_ether.h |    3 ++-
 net/can/af_can.c         |    7 ++++++
 3 files changed, 60 insertions(+), 5 deletions(-)

include/linux/can.h +5 -0

index 17334c0..25265e7 100644
@@ -46,18 +46,65 @@ typedef __u32 canid_t;
  */
 typedef __u32 can_err_mask_t;
 +#define CAN_MAX_DLC 8
+#define CAN_MAX_DLEN 8
+
+#define CANFD_MAX_DLC 15
+#define CANFD_MAX_DLEN 64
+

(..)

Quoted 6 lines

Quoted 6 lines

struct can_frame {

include/linux/can.h +1 -1

 	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
-	__u8    can_dlc; /* data length code: 0 .. 8 */
+	__u8    can_dlc; /* frame payload length in byte (0 .. 8) */
 	__u8    data[8] __attribute__((aligned(8)));
 };

(..)

Quoted 4 lines

Quoted 4 lines

+ * @data: CAN FD frame payload (up to 64 byte)
+ */

include/linux/can.h +2 -0

+struct canfd_frame {
+	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */

Quoted 9 lines

include/linux/can.h typedef __u32 canid_t; · line 100

Quoted 5 lines

include/linux/can.h +5 -0

+	__u8    len;     /* frame payload length in byte (0 .. 64) */
+	__u8    flags;   /* additional flags for CAN FD */
+	__u8    __res0;  /* reserved / padding */
+	__u8    __res1;  /* reserved / padding */
+	__u8    data[64] __attribute__((aligned(8)));

Shouldn't we use "CANFD_MAX_DLEN" here (... and maybe above as well)?. IIRC, CANFD_MAX_DLEN might be even 128 in the finalized standard.

Yes. Good point.

(..)

Quoted 5 lines

include/linux/can.h

Quoted 3 lines

include/linux/can.h +2 -1

-#define ETH_P_CAN	0x000C		/* Controller Area Network      */
+#define ETH_P_CAN	0x000C		/* Controller Area Network (CAN)*/
+#define ETH_P_CANFD	0x000D		/* CAN FD 64 byte payload frames*/

Then hardcoding 64 here is also not be nice.

Yes. Will change that too.
Even if there's a little chance to have more than 64 bytes it's not the right place for this kind of information anyway.

I would wait until this evening if there are more of these remarks and send a v2 then. A review of the documentation would be nice - if everything seems clear to you.

Quoted 4 lines

I would wait until this evening if there are more of these remarks and send a v2 then. A review of the documentation would be nice - if everything seems clear to you.

Where can I pull/fetch the patches from? IIUC, the latest patch series you sent is broken.

Quoted 4 lines · Wolfgang Grandegger

Quoted 4 lines

+ * @data: CAN FD frame payload (up to 64 byte)
+ */

(quoted diff) +2 -0

+struct canfd_frame {
+	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */

Quoted 9 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	__u8    len;     /* frame payload length in byte (0 .. 64) */
+	__u8    flags;   /* additional flags for CAN FD */
+	__u8    __res0;  /* reserved / padding */
+	__u8    __res1;  /* reserved / padding */
+	__u8    data[64] __attribute__((aligned(8)));

Shouldn't we use "CANFD_MAX_DLEN" here (... and maybe above as well)?. IIRC, CANFD_MAX_DLEN might be even 128 in the finalized standard.

Yes. Good point.

(..)

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +2 -1

-#define ETH_P_CAN	0x000C		/* Controller Area Network      */
+#define ETH_P_CAN	0x000C		/* Controller Area Network (CAN)*/
+#define ETH_P_CANFD	0x000D		/* CAN FD 64 byte payload frames*/

Then hardcoding 64 here is also not be nice.

Yes. Will change that too.
Even if there's a little chance to have more than 64 bytes it's not the right place for this kind of information anyway.