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.

[zariiii9003]

ping @eldadcool

[eldadcool]

ping @eldadcool

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

[zariiii9003]

@lumagi feel free to approve and merge

[lumagi]

I took another final look at it, and I have one last request:
This feature only concerns CAN 2.0 so could you add another condition to this if clause that check that we're dealing with a non-FD frame?

[eldadcool]

Amm yes you are correct, maybe I should add tests for this case as well