CC-2174: modify CAN frame header structure to match updated struct ca…

[eldadcool]

This fix allow to send CLASSIC CAN frames with a DLC value larger than 8 using the socketcan interface.

In addition it allow to parse incoming socketcan messages with the updated format
#1780
@hartkopp

[zariiii9003]

@lumagi could you take a look at this?

[lumagi]

I'll try to take a look over the next couple of days. @eldadcool do you happen to have a link to the patch set in the kernel?

[eldadcool]

I'll try to take a look over the next couple of days. @eldadcool do you happen to have a link to the patch set in the kernel?

@lumagi, sure:
The patch can be found at version 6.10.7 of the kernel, see patch at torvalds/linux@ea78005

To configure the bus using "ip link" command you require a version of iproute2 5.12 or higher: https://lore.kernel.org/netdev/[email protected]/

use the command

ip link set can0 down
ip link set type can cc-len8-dlc on
ip link set can0 up

note: can-utils support this only since this patch linux-can/can-utils@11edd1d

@hartkopp - please review my fix as well to make sure I did not misinterpret some edge cases regarding the new structure and that this will not break the usage of the old format.

[hartkopp]

use the command

ip link set can0 down
ip link set type can cc-len8-dlc on
ip link set can0 up

You can simply go with the latest can-utils where candump and cangen have a -8 option to create and display the DLC values over 8.
WIth the virtual CAN interfaces (e.g. vcan0) you can test the cc-len8-dlc stuff without any additional configuration. The vcan is transparent for these values.

@hartkopp - please review my fix as well to make sure I did not misinterpret some edge cases regarding the new structure and that this will not break the usage of the old format.

I'm not sure whether I can really review the patch, as I'm not that familiar with Pythons packing concept.
But if you strictly followed the data structure and the rules (len == 8 and ( len8_dlc > 8 && len8_dlc <= 0xF )) apply for valid DLC over 8 length's values - then it should be fine :-)

Many thanks,
Oliver

[eldadcool]

I'm not sure whether I can really review the patch, as I'm not that familiar with Pythons packing concept. But if you strictly followed the data structure and the rules (len == 8 and ( len8_dlc > 8 && len8_dlc <= 0xF )) apply for valid DLC over 8 length's values - then it should be fine :-)

Actually, the DLC value in python-can relate directly to the data length. and it's not even verified to be a valid message by default. So potentially this could lead to sending a message with unexpected DLC value.
This reminds me of an issue I think I found in socketcan (@hartkopp) allowing unexpected lengths for CAN FD frames (10 bytes for example...) because the length value used is socketcan also relate to the data length and not to the DLC. I think we need to be aligned regarding this and very careful because fixing it might break some existing usage of socketcan or python-can.

Note the issue with socketcan only relevant for virtual can interfaces because physical interfaces does not allow FD frames with 10 bytes of data (only <8, 8, 12, 16, 20, 24, 32, 48 and 64) but it makes virtual interfaces behave differently than physical interfeces.

[hartkopp]

This reminds me of an issue I think I found in socketcan (@hartkopp) allowing unexpected lengths for CAN FD frames (10 bytes for example...) because the length value used is socketcan also relate to the data length and not to the DLC. I think we need to be aligned regarding this and very careful because fixing it might break some existing usage of socketcan or python-can.

Note the issue with socketcan only relevant for virtual can interfaces because physical interfaces does not allow FD frames with 10 bytes of data (only <8, 8, 12, 16, 20, 24, 32, 48 and 64) but it makes virtual interfaces behave differently than physical interfeces.

Yes, this is intentional.
The idea is that the content which is sent via virtual CAN devices is not extensively sanitized to be able to test with uncertain values and just reflect them back to user space. This is very useful to test user space applications.

Once the CAN frame content is given to a real CAN interface or received from them the functions

can_fd_dlc2len() can_fd_len2dlc() canfd_sanitize_len() are used.

https://elixir.bootlin.com/linux/v6.11/source/include/linux/can/length.h#L290

[eldadcool]

Yes, this is intentional.
The idea is that the content which is sent via virtual CAN devices is not extensively sanitized to be able to test with uncertain values and just reflect them back to user space. This is very useful to test user space applications.

Cool,
In that case the current solution I proposed here sanitize the length field for FD valid values. Don't think this should lead to an issue. @lumagi, have any insights?

[lumagi]

It looks fine to me. I only had some nit-picky stuff.

[lumagi]

Is there a specific reason you don't trust the value returned by the kernel? I'm not saying it's a bad thing, just interested in why you added this.

[eldadcool]

As discussed with @hartkopp socketcan allow to send non valid FD frames with length not supported by CAN FD. For example, DLC=0x9 (data_len=12) and actual data_len=10 - this is invalid CAN FD frame but still could be received from the kernel.
To handle this case I added code to round up the amount of bytes.
Note that no read overflow will happen because the CANFD MTU is set to 72 (64 bytes of data) in the socketcan implementation so the additional bytes read from the frame will still be part of the struct and will probably contains 0x00s. If you prefer we can add padding with a defined value to be on the safe side.

[lumagi]

This is very nit-picky: I know this wasn't any better before the new PR, but I don't like that we don't check that the expected data_len is actually present in the frame. Since it's Python it will simply return less data than expected.

[eldadcool]

Sure I can add this verification to make it more robust. Should I add padding bytes , recude the data_len value or raise an exception in that case? Not sure what will be the best here...

[zariiii9003]

ping @eldadcool

[eldadcool]

ping @eldadcool

Hii sorry, I was not very available last month...
Added replies to your comments. :)