On 12.11.24 09:31, Vincent Mailhol wrote:
On Tue. 12 Nov. 2024 at 17:09, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
On 11.11.2024 00:56:01, Vincent Mailhol wrote:
Add the netlink interface for CAN XL.
Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
---
drivers/net/can/dev/netlink.c | 78 +++++++++++++++++++++++++++++---
include/linux/can/bittiming.h | 2 +
include/linux/can/dev.h | 13 ++++--
include/uapi/linux/can/netlink.h | 7 +++
4 files changed, 90 insertions(+), 10 deletions(-)
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 6c3fa5aa22cf..3c89b304c5b8 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -22,6 +22,9 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
[IFLA_CAN_TERMINATION] = { .type = NLA_U16 },
[IFLA_CAN_TDC] = { .type = NLA_NESTED },
[IFLA_CAN_CTRLMODE_EXT] = { .type = NLA_NESTED },
+ [IFLA_CAN_XL_DATA_BITTIMING] = { .len = sizeof(struct can_bittiming) },
+ [IFLA_CAN_XL_DATA_BITTIMING_CONST] = { .len = sizeof(struct can_bittiming_const) },
+ [IFLA_CAN_XL_TDC] = { .type = NLA_NESTED },
I haven't looked at the can_xl IP-core docs yet.
I don't want to pass "struct can_bittiming_const" via netlink to user
space. It's not sufficient to fully describe the CAN-FD controllers, as
tseg1_min cannot equally divided into prop_seg and phase_seg1.
Better make it a NLA_NESTED, as you did for the TDC.
I considered this point. The fact is that the code to handle the "struct
can_bittiming_const" already exists. And so here, I am left with two
choices:
- small code refactor and reuse the existing
- rewrite the full thing just for CAN XL and have two different ways
to handle the constant bittiming: one for Classical CAN and CAN FD
and the other for CAN XL.
For consistency, I chose the former approach which is the least
disruptive. If you want this nested, what about an in-between
solution:
IFLA_CAN_XL /* NLA_NESTED */
+ IFLA_CAN_DATA_BITTIMING /* struct can_bittiming */
+ IFLA_CAN_DATA_BITTIMING_CONST /* struct can_bittiming */
+ IFLA_CAN_TDC /* NLA_NESTED */
+ IFLA_CAN_TDC_TDCV_MIN
+ IFLA_CAN_TDC_TDCV_MAX
+ (all other TDC nested values)...
This way, we can still keep most of the current CAN(-FD) logic, and if
we want to add one value, we can add it as a standalone within the
IFLA_CAN_XL nest.
Also, the main reason for not creating the nest was that I thought
that the current bittiming API was stable. I was not aware of the
current flaw on how to divide tseg1_min. Maybe we should first discuss
how to solve this issue for CAN FD?
I like the current way how you added the CAN XL support.
It maintains the known usage pattern - and the way how CAN XL bit
timings are defined is identical to CAN FD (including TDC).
Is the separation of propseg and tseg1 that relevant?
Does it really need to be exposed to the user?
Best regards,
Oliver
