Hi Vincent,
On 2024-01-08 09:26, Oliver Hartkopp wrote:
On 08.01.24 06:46, Vincent MAILHOL wrote:
I have not yet reviewed the net/can/raw.c part. It will take me more
time. I will do it later (we are the first day of the merge window, so
that leaves me time!)
The raw.c changes have become very easy to review since the introduction
of the vcid element - no hurry ;-)
Some time has already passed now. Did you find some time to take a look
at the v4 patch?
https://lore.kernel.org/linux-can/20240108170644.1887-1-socketcan@xxxxxxxxxxxx/
Best regards,
Oliver
Question: do you think this should also go to stable?
I would have no objections, but I'm not sure whether we can convince the
stable maintainers to change the UAPI and slightly extend the ABI in
stable kernels? Let's see.
More a matter of principle, but you should
#include <asm/byteorder.h>
c.f.
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ip.h#L22
Yes. I already prepared this change for the next patch cycle yesterday:
https://github.com/hartkopp/canxl-utils/commit/9ac1b75b59ccbf668fec4bd5c846542ad0073da0
@@ -193,26 +193,39 @@ struct canfd_frame {
#define CANXL_XLF 0x80 /* mandatory CAN XL frame flag (must always
be set!) */
#define CANXL_SEC 0x01 /* Simple Extended Content
(security/segmentation) */
/**
* struct canxl_frame - CAN with e'X'tended frame 'L'ength frame
structure
- * @prio: 11 bit arbitration priority with zero'ed CAN_*_FLAG flags
- * @flags: additional flags for CAN XL
- * @sdt: SDU (service data unit) type
- * @len: frame payload length in byte (CANXL_MIN_DLEN ..
CANXL_MAX_DLEN)
- * @af: acceptance field
- * @data: CAN XL frame payload (CANXL_MIN_DLEN .. CANXL_MAX_DLEN byte)
+ * @prio: 11 bit arbitration priority with zero'ed CAN_*_FLAG flags
Now that prio is a __u16, the CAN_*_FLAG flags fall outside prio width
(in __res0 as you mentioned below). So I would rather remove the
latter part of this sentence. You may instead specify that bits
outside of the CANXL_PRIO_MASK shall be zeroed.
Right. Will change that.
+ * @vcid: virtual CAN network identifier
Maybe say that this field is valid only if enabled through
CAN_RAW_XL_VCID_OPTS, else should be zero.
No. This is a data structure definition and not the CAN_RAW socket
documentation. But it has definitely to be mentioned somewhere.
- * @prio shares the same position as @can_id from struct can[fd]_frame.
+ * @prio shares the lower 16 bits of @can_id from struct can[fd]_frame.
+ * @__res0 holds the @can_id flags CAN_xxx_FLAG and has to be set to
zero
Just realized now, but I would have personally preferred to merge
those two sentences within their respective field documentation. Just
feels odd to me to have the information on the prio and __res0 field
are scattered in two different locations. Well this is just a nitpick,
at the end, just choose which one seems good to you.
Ok. I always tried to get only one line for the element description. But
this could be changed. No problem.
+struct can_raw_vcid_options {
+
+ __u8 flags; /* flags for vcid (filter) behaviour */
+ __u8 tx_vcid; /* VCID value set into
canxl_frame.prio */
+ __u8 rx_vcid; /* VCID value for VCID filter */
+ __u8 rx_vcid_mask; /* VCID mask for VCID filter */
+
Why the newlines before and after the struct members? This style is
not consistent with the other CAN headers.
Ok. Will remove them.
/* check for correct padding to be able to use the structs
similarly */
BUILD_BUG_ON(offsetof(struct can_frame, len) !=
offsetof(struct canfd_frame, len) ||
+ offsetof(struct can_frame, len) !=
+ offsetof(struct canxl_frame, flags) ||
offsetof(struct can_frame, data) !=
offsetof(struct canfd_frame, data));
Nitpick: I would expect to see the code structured in this order:
check Classical CAN first, then CAN-FD and finally CAN-XL. Not sure
why the CAN-XL is in the middle. If the only reason is to minimize the
patch diff, then no.
The reason is to check the offset to struct can_frame.len - so all these
checks are sorted by the first element to be checked.
Thanks for the feedback!
Oliver
