Re: [RFC PATCH 12/14] can: netlink: add CAN XL support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Automotive Discussions]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [CAN Bus]

  Powered by Linux