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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 14.07.22 11:12, Vincent Mailhol wrote:
> On Thu. 14 Jul. 2022 at 15:11, Oliver Hartkopp <socketcan@hartkopp.net> wrote:



>> 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) */
> 
> OK. Having CANXL_SEC on the most significant bit (0x40) or instead of
> the least significant bit (0x01) works for me.

Hm. This is a really iterative process ;-)

Maybe

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

looks even more natural:

The MSB is set and the other bits start from LSB as usual.

>> And the rest of the bits are reserved (shall be set to zero).
> 
> ACK.
> 
>> 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.
> 
> The purpose of the CAN_XLF is still unclear to me. In your initial
> patch, you wrote that CAN_XLF was to "mark CAN XL for dual use of
> struct canfd_frame". So are we really OK to specify that CAN_XLF is
> always set? How are we going to tag dual use?

I think the dual-use pattern does not make sense anymore as either the 
flags element and the len element have been moved away from the struct 
can[fd]_frame layout.

There is no layout match between CAN XL and CAN/CANFD anymore.

> But my main point was to always set 0x80 to differentiate between
> CAN(-FD) and CANXL, and we are aligned on this. :D

ACK.

IMO this 0x80 bit at this specific position is an excellent flag to 
introduce CAN XL frames and to provide a proper break with CAN/CANFD 
data structures.

> The last topic remaining is the data[] vs. data[CANXL_MAX_DLEN]. I
> thought about it but can not find any killer arguments for either
> solution.
> The biggest difference is that for data[] we can do sizeof(struct
> canxl_frame) to get the header file whereas for data[CANXL_MAX_DLEN],
> we need to introduce the CANXL_HEADER_SIZE macro.
> My preference still goes toward the data[] because I see flexible
> member arrays as being more idiomatic C. But it is more for personal
> taste than anything else…

I see it as a practical thing for programmers (also in userspace) to 
allocate sizeof(struct canxl_frame) and do all the checks and operations 
similar to struct can[fd]_frame (and the ISO CAN Spec).

It just makes it very clear what we are talking about.

The fact that we can reduce the content size whenever we transfer 
content from/to kernel space or inside the kernel is just a nice 
optimization IMO.

> Yours sincerely,
> Vincent Mailhol

Thanks for your feedback! I think I'll post an updated patch set later 
today to continue the discussion on a new code basis.

Best regards,
Oliver