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

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3542 bytes --]

On 12.11.2024 17:31:38, Vincent Mailhol wrote:
> On Tue. 12 Nov. 2024 at 17:09, Marc Kleine-Budde <mkl@pengutronix.de> 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@wanadoo.fr>
> > > ---
> > >  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?

The bittiming API is stable. I thought of adding an additional nested
type to hold all relevant information. Luckily it's only from kernel ->
user space. So we can pass both, the old struct can_bittiming_const and
the new nested type and the user space can decide which one to use.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]