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
