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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 04.12.24 12:15, Marc Kleine-Budde wrote:
> On 04.12.2024 11:56:02, Oliver Hartkopp wrote:
>>
>>
>> On 12.11.24 09:31, 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?
>>
>> I like the current way how you added the CAN XL support.
>> It maintains the known usage pattern - and the way how CAN XL bit timings
>> are defined is identical to CAN FD (including TDC).
>>
>> Is the separation of propseg and tseg1 that relevant?
>> Does it really need to be exposed to the user?
> 
> There are IIRC at least 2 CAN-FD cores where the prop segment and phase
> segment 1 for the data bit timing have not the same width. This means we
> have to change the bittiming_const in the kernel.
> 
> A struct in netlink means we cannot change it.

But are we not already in this situation with CAN FD that we can not 
change the bittiming (const) in the userspace API?

Best regards,
Oliver


> It makes IMHO no sense to
> me to add any new structs into netlink, especially if we know the
> bittiming struct is going to change.
> 
> regards,
> Marc
>