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? Yours sincerely, Vincent Mailhol