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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 12.07.22 12:19, Vincent Mailhol wrote:
> On Tue. 12 Jul. 2022 at 18:31, Oliver Hartkopp <socketcan@hartkopp.net> wrote:

>>>> +/* truncated CAN XL structs must contain at least 64 data bytes */
>>>> +#define CANXL_MINTU    (CANXL_MTU - CANXL_MAX_DLEN + CANFD_MAX_DLEN)
>>>
>>> I did not get the concept of the "truncated CAN XL structs". The valid
>>> data field lengths are 1 to 2048, right? I did not get where this 64
>>> comes from.
>>> Your formula is equivalent to
>>> #define CANXL_MINTU    (sizeof(struct canxl_frame) + CANFD_MAX_DLEN)
>>
>> No. CANXL_MINTU becomes sizeof(struct canfd_frame) + sizeof(af)
>>
>> So I wanted some size value that is 'more than' CANFD_MTU so that you
>> know that you have read a CANXL frame.
>>
>> Even if the cxf->len would be 14 you would at least copy a struct
>> canxl_frame with data[64].
> 
> OK, I finally got your point. Your concern is that if skb->len could
> be equal or less than CANFD_MTU, then there would be a collision.
> 
> My approach here would be to stop using the MTU correlation to
> differentiate between CAN(-FD) and CANXL. Instead, I suggest using
> can{fd,xl}_frame::flags. If can{fd,xl}_frame has a CANXL flag set,
> then it is a CANXL frame regardless of the value of skb->len. If the
> CANXL flag is not set, then skb->len is used to differentiate between
> Classic CAN and CAN FD (so that we remain compatible with the
> existing). That way, no need to impose a minimum length of
> CANFD_MAX_DLEN.

Hm, that sounds interesting! I like that as it looks clear and simple.

Need to pick a coffee to think about potential (security) effects ... ;-)

E.g. we would need to keep skb->len and canxl_frame::len in sync now.

> 
>>>
>>> I would have just expected:
>>> #define CANXL_MINTU    (sizeof(struct canxl_frame))
>>
>> That is CANXL_MTU (max transfer unit).
> 
> I was writing while thinking that canxl_frame::data was a flexible
> array member as suggested in this thread.
> In that case canxl_frame::data counts as zero when doing sizeof(struct
> canxl_frame). And so sizeof(struct canxl_frame) == sizeof(struct
> canfd_frame) + sizeof(af).
> 
> Actually, thinking twice, the Minimum transfer unit would be:
> #define CANXL_MINLEN 1
> #define CANXL_MINTU    (sizeof(struct canxl_frame) + CANXL_MINLEN)
> 
> (I forgot that the length can not be zero anymore in CANXL...)

I still would suggest to have the struct canxl_frame contain the 2048 
byte of data (data[CANXL_MAXDLEN]) - as the entire CAN XL frame is 
defined like this in the CAN XL spec. This would be also in common with 
CAN/CANFD.

E.g. when reading into the struct canxl_frame you always have a defined 
data structure which can contain a complete CAN XL frame.

But if you get or send less than that size (when reading/writing) this 
would be ok now (with your idea with CANXL_XLF set).


E.g.

#define CANXL_MINDLEN 1
#define CANXL_MAXDLEN 2048

#define CANXL_MTU sizeof(struct canxl_frame)
#define CANXL_HEAD_SZ (CANXL_MTU - CANXL_MAXDLEN)
#define CANXL_MINTU (CANXL_HEAD_SZ + CANXL_MINDLEN)

(..)

>>> Here, you lost me. The only reference document I have is the Bosch
>>> presentation you linked in the cover letter. I would need to get a
>>> copy of the specification first to follow up on this level of details.
>>
>> There is a Special Interest Group for CAN XL at CAN in Automation
>> (can-cia.org) and these doscuments are currently under development.
> 
> I wonder how hard it is to join the group. Right now, I was thinking
> of waiting for the ISO Final Draft International Standard (FDIS) to
> deep dive in CANXL.

I'm not sure if the SDT definitions will be part of an ISO draft as 
defining the content inside a CAN XL frame is probably not ISO standard 
relevant.

The content definitions are industrial recommendations to increase 
interoperability. You are always free to put into the CAN XL frame 
whatever you want.

E.g. CAN-CiA defined the pinout of the SUB-D9 connectors for CAN (Pin 
2/7) which is not defines in ISO too.

Best regards,
Oliver