Hi Vincent,
thanks for your review!
On 07.01.24 07:28, Vincent MAILHOL wrote:
On Sun. 7 Jan. 2024 at 04:47, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
values can be send, e.g. to replay full qualified CAN XL traffic.
^^^^
sent
ACK
provided by the CAN_RAW sockets and kernel infrastruture.
^^^^^^^^^^^^^
infrastructure
ACK
struct canxl_frame {
- canid_t prio; /* 11 bit priority for arbitration (canid_t) */
+ canid_t prio; /* 11 bit priority for arbitration / 8 bit VCID */
Isn't this a UAPI breaking change? Prior to this patch, the applications may do:
canxl_frame.prio
to get the prio, but after this patch, applications are required to do:
canxl_frame.prio & CANXL_PRIO_MASK
Not really. I also thought about it but you *only* need to take care
about the VCID content when you have enabled it explicitly with the new
sockopt. Otherwise you will never see anything beyond the 32 bit prio.
in order to mask out the VCID (currently, there are no requirements
that canxl_frame.prio must be masked before use).
In the past, I was reluctant to acknowledge the introduction of CANXL
in the kernel prior to reading the ISO standard because I was afraid
of such UAPI stability issues. Now we have to deal with it.
Yes, but that kind of extension would be backwards compatible.
What do you think of:
struct canxl_frame {
#if defined(__LITTLE_ENDIAN)
__u16 prio; /* 11 bit priority for arbitration */
__u8 vcid; /* 8 bit VCID */
__u8 __reserved; /* must be 0 */
/* ... */
#elif defined(__BIG_ENDIAN)
__u8 __reserved; /* must be 0 */
__u8 vcid; /* 8 bit VCID */
__u16 prio; /* 11 bit priority for arbitration */
#else
#error "Unknown endianness"
#endif
}
Here, canxl_frame.prio always gives a correct value without need for
CANXL_PRIO_MASK masking. The big/little endianness checks are needed
to maintain the ABI compatibility. Not yet tested, so forgive if there
is a mistake. Getting the endianness logic correct on a first try is
not easy.
Yes, I tested such approach too (with little endian only) and it worked
great - and of course looked better in the code.
Also, the VCID can now be accessed through canxl_frame.vcid instead of
relying on some mask and shift logic.
Right. That looked nice.
The drawback is that you lose the can_id type. For what I understand,
this is only used for filtering. If we absolutely need to maintain the
canid_t, then maybe:
struct canxl_frame {
union {
canid_t filter;
struct {
#if defined(__LITTLE_ENDIAN)
__u16 prio; /* 11 bit priority for arbitration */
__u8 vcid; /* 8 bit VCID */
__u8 __reserved; /* must be 0 */
#elif defined(__BIG_ENDIAN)
__u8 __reserved; /* must be 0 */
__u8 vcid; /* 8 bit VCID */
__u16 prio; /* 11 bit priority for arbitration */
#else
#error "Unknown endianness"
#endif
};
};
/* ... */
}
But I think it is better to drop it. If someone wants a canid_t, then
he or she can just cast the XL frame to either struct can_frame or
struct canfd_frame.
Though?
My only concern is that it looks really ugly :-/
The change of the prio element from u32 to u16 will also not harm anyone
as I assume to be the only person who's currently working with CAN XL
frames on virtual CAN interfaces:
https://github.com/hartkopp?tab=repositories&q=can-cia
I'll prepare a patch that picks up this suggestion of an __u16 prio etc.
Maybe we can add some compile time checks to ensure the correct struct
layout for this case.
__u8 flags; /* additional flags for CAN XL */
If CANXL_VCID is set, can vcid be zero? If not, no need for a flag.
Just need to check if canxl_frame.vcid is not zero.
This is probably indeed a leftover which can be removed with my latest
implementation. Will recheck.
Many thanks,
Oliver
