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@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?
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
