Hi Oliver, 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. On Tue. 2 Aug 2022 at 04:02, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote: > This patch adds defines for data structures and length information for > CAN XL (CAN with eXtended data Length) which can transfer up to 2048 > byte 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 <socketcan@xxxxxxxxxxxx> > --- > include/uapi/linux/can.h | 51 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h > index 7b23eeeb3273..dd645ea72306 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,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. > + */ > +#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? > + > +/** > + * 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) */ 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. > + __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? > + > #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) Does anyone have a better idea? Yours sincerely, Vincent Mailhol
