On 21.10.20 13:55, Vincent MAILHOL wrote:
On 21.10.20 18:48, Oliver Hartkopp wrote:
To be more compatible to non raw dlc CAN frame sources the tx handling
could also be like this:
if ((can_dlc == CAN_MAX_DLEN) &&
(raw_dlc >= CAN_MAX_DLC && raw_dlc <= CAN_MAX_RAW_DLC))
=> use raw_dlc
(..)
If I understand well, the idea would be not to use a setsockopt() but
instead rely on some logic on the can_dlc and raw_dlc to determine
which one to use.
Unfortunately, this approach has one issue in the TX path that could
break existing applications.
Consider below code (which I think is fairly realistic):
void send_frame (int soc, canid_t can_id, char *data, size_t len)
{
struct can_frame cf;
size_t dlc = len > sizeof(cf.data) ? sizeof(cf.data) : len;
cf.can_id = can_id;
cf.can_dlc = dlc;
memcpy(cf.data, data, dlc);
write(soc, &cf, sizeof(cf));
}
Here, the user did not initialize the can frame (cf) but assigned all
the relevant fields manually. Because cf is not initialized, the newly
introduced cf.dlc_raw field would have any of the values which was
present on the stack at the moment (rationale: the C standard does not
guarantee zero initialization). If 9 <= raw_dlc <= 15, the can frame
will be transmitted with a bad DLC value. If raw_dlc > 15, the can
frame will be discarded.
No, this is not what I wrote. With my suggestion you need to populate
both dlc elements to use the new "raw dlc" feature.
if (can_dlc == 8) && (9 <= raw_dlc <= 15)
=> put raw_dlc value into the controller register
else
=> put can_dlc value into the controller register
When you have a test system to make security tests and you enable
CAN_CTRLMODE_RAW_DLC on a specific CAN interface - which applications
would you run to send CAN frames on this interface?
I assume only the test application which is really aware of setting
can_dlc and raw_dlc correctly.
CAN_CTRLMODE_RAW_DLC is no 'standard' option. It's a testing facility
and only used, when people want to play with raw DLCs.
An option could be to introduce a sockopt CAN_RAW_RAW_DLC that only
forwards the raw_dlc element for classic CAN frames when enabled, and
clears the raw_dlc field otherwise.
But again: Who ever enables CAN_CTRLMODE_RAW_DLC has a specific use-case
in mind and knows what he's doing.
I think that it is mandatory to have the application declare that it
wants to use raw DLCs (this way, we know that the code is "DLC
aware"). I can not think of any transparent approach.
Next, we might think of using the netlink CAN_CTRLMODE_RAW_DLC and
the CAN_RAW_RAW_DLC socket option and the raw_dlc field. But I think
that this is overkill.
If not introducing new dlc_raw field, the drawback, as you pointed
out, will be that we would lose the backward compatibility of
canfd_frame with can_frame and that can_dlc field can not be used
directly as a length.
For userland, I think that this is acceptable because the very instant
the user calls setsockopt() with the CAN_RAW_RAW_DLC, he should be
aware of the consequences and should resign to use can_dlc field as a
plain length.
Not filling can_dlc would cause tons of changes for sanity checks and
feature switches.
Therefore everything should remain as-is and ONLY in the case of
CAN_CTRLMODE_RAW_DLC and can_dlc == 8 the CAN driver validates the
raw_dlc value element and uses it for transmission.
That of course means that this should be clearly
highlighted when updating the documentation. And users not interested
by this feature can continue to use it as they did before.
Inside the kernel, all drivers advertising CAN_CTRLMODE_RAW_DLC will
also resign the right to use can_dlc as plain length.
As explained before. This would cause effort without any need for the
can_dlc handling.
Drivers not
using it are safe with their existing code. Finally, the TX and RX
path would both need to be inspected in detail.
Yep.
TLDR; socket options seem mandatory in all cases. We then have to
choose between breaking the can_dlc plain length property or
introducing a new raw_dlc field. My choice goes to the first option of
breaking the can_dlc plain length property.
The 14 year old documentation in can.h says:
@can_dlc: frame payload length in byte (0 .. 8)
I will not break this established rule for a testing feature. The
question from Joakim clearly showed: Don't touch this!
At the end it would have made more sense to call it can_frame.len in the
first place. But this first came into our mind when CAN FD showed up.
The discussion about the can_dlc meaning can be closed IMO. It is a
plain length from 0..8 which is unfortunately named can_dlc.
Regards,
Oliver
