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

[Oliver Hartkopp <socketcan@hartkopp.net>]

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


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

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

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

ACK.

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

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

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

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

I would suggest the following:

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

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

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

Best regards,
Oliver