On 12.11.2024 17:31:38, 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?
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:
signature.asc
Description: PGP signature
