On 14.07.22 11:12, Vincent Mailhol wrote:
On Thu. 14 Jul. 2022 at 15:11, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> 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
