[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMuHMdWDUpkwPVCm2Dha04F59MES4QvKFUVfvg70x5GPZHsxDA@mail.gmail.com>
Date: Mon, 2 Jun 2025 09:23:19 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Vincent Mailhol <mailhol.vincent@...adoo.fr>
Cc: Marc Kleine-Budde <mkl@...gutronix.de>, linux-can@...r.kernel.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, Stefan Mätje <Stefan.Maetje@....eu>
Subject: Re: [PATCH v6 4/6] can: netlink: add interface for CAN-FD Transmitter
Delay Compensation (TDC)
Hi Vincent,
On Sat, 31 May 2025 at 10:25, Vincent Mailhol
<mailhol.vincent@...adoo.fr> wrote:
> On 30/05/2025 at 20:44, Geert Uytterhoeven wrote:
> > Thanks for your patch, which is now commit d99755f71a80df33
> > ("can: netlink: add interface for CAN-FD Transmitter Delay
> > Compensation (TDC)") in v5.16.
> >
> > On Sat, 18 Sept 2021 at 20:23, Vincent Mailhol
> > <mailhol.vincent@...adoo.fr> wrote:
> >> Add the netlink interface for TDC parameters of struct can_tdc_const
> >> and can_tdc.
> >>
> >> Contrary to the can_bittiming(_const) structures for which there is
> >> just a single IFLA_CAN(_DATA)_BITTMING(_CONST) entry per structure,
> >> here, we create a nested entry IFLA_CAN_TDC. Within this nested entry,
> >> additional IFLA_CAN_TDC_TDC* entries are added for each of the TDC
> >> parameters of the newly introduced struct can_tdc_const and struct
> >> can_tdc.
> >>
> >> For struct can_tdc_const, these are:
> >> IFLA_CAN_TDC_TDCV_MIN
> >> IFLA_CAN_TDC_TDCV_MAX
> >> IFLA_CAN_TDC_TDCO_MIN
> >> IFLA_CAN_TDC_TDCO_MAX
> >> IFLA_CAN_TDC_TDCF_MIN
> >> IFLA_CAN_TDC_TDCF_MAX
> >>
> >> For struct can_tdc, these are:
> >> IFLA_CAN_TDC_TDCV
> >> IFLA_CAN_TDC_TDCO
> >> IFLA_CAN_TDC_TDCF
> >>
> >> This is done so that changes can be applied in the future to the
> >> structures without breaking the netlink interface.
> >>
> >> The TDC netlink logic works as follow:
> >>
> >> * CAN_CTRLMODE_FD is not provided:
> >> - if any TDC parameters are provided: error.
> >>
> >> - TDC parameters not provided: TDC parameters unchanged.
> >>
> >> * CAN_CTRLMODE_FD is provided and is false:
> >> - TDC is deactivated: both the structure and the
> >> CAN_CTRLMODE_TDC_{AUTO,MANUAL} flags are flushed.
> >>
> >> * CAN_CTRLMODE_FD provided and is true:
> >> - CAN_CTRLMODE_TDC_{AUTO,MANUAL} and tdc{v,o,f} not provided: call
> >> can_calc_tdco() to automatically decide whether TDC should be
> >> activated and, if so, set CAN_CTRLMODE_TDC_AUTO and uses the
> >> calculated tdco value.
> >
> > This is not reflected in the code (see below).
>
> Let me first repost what I wrote but this time using numerals and letters
> instead of the bullet points:
>
> The TDC netlink logic works as follow:
>
> 1. CAN_CTRLMODE_FD is not provided:
> a) if any TDC parameters are provided: error.
>
> b) TDC parameters not provided: TDC parameters unchanged.
>
> 2. CAN_CTRLMODE_FD is provided and is false:
> a) TDC is deactivated: both the structure and the
> CAN_CTRLMODE_TDC_{AUTO,MANUAL} flags are flushed.
>
> 3. CAN_CTRLMODE_FD provided and is true:
> a) CAN_CTRLMODE_TDC_{AUTO,MANUAL} and tdc{v,o,f} not provided: call
> can_calc_tdco() to automatically decide whether TDC should be
> activated and, if so, set CAN_CTRLMODE_TDC_AUTO and uses the
> calculated tdco value.
>
> b) CAN_CTRLMODE_TDC_AUTO and tdco provided: set
> CAN_CTRLMODE_TDC_AUTO and use the provided tdco value. Here,
> tdcv is illegal and tdcf is optional.
>
> c) CAN_CTRLMODE_TDC_MANUAL and both of tdcv and tdco provided: set
> CAN_CTRLMODE_TDC_MANUAL and use the provided tdcv and tdco
> value. Here, tdcf is optional.
>
> d) CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually exclusive. Whenever
> one flag is turned on, the other will automatically be turned
> off. Providing both returns an error.
>
> e) Combination other than the one listed above are illegal and will
> return an error.
>
> You can double check that it is the exact same as before.
>
> > By default, a CAN-FD interface comes up in TDC-AUTO mode (if supported),
> > using a calculated tdco value. However, enabling "tdc-mode auto"
> > explicitly from userland requires also specifying an explicit tdco
> > value. I.e.
> >
> > ip link set can0 type can bitrate 500000 dbitrate 8000000 fd on
> ^^^^^
> Here:
>
> - CAN_CTRLMODE_FD provided and is true: so we are in close 3.
>
> - CAN_CTRLMODE_TDC_{AUTO,MANUAL} and tdc{v,o,f} not provided: so we *are* in
> sub-clause a)
>
> 3.a) tells that the framework will decide whether or not TDC should be
> activated, and if activated, will set the TDCO.
>
> > gives "can <FD,TDC-AUTO>" and "tdcv 0 tdco 3", while
>
> Looks perfectly coherent with 3.a)
>
> Note that with lower data bitrate, the framework might have decided to set TDC off.
Yes, that case is fine for sure.
> > ip link set can0 type can bitrate 500000 dbitrate 8000000 fd on
> > tdc-mode auto
>
> This time:
>
> - CAN_CTRLMODE_FD provided and is true: so we are in close 3.
>
> - CAN_CTRLMODE_TDC_AUTO is provided, we are *not* in sub-clause a)
>
> - tdco is not provided.
>
> No explicit clauses matches this pattern so it defaults to the last
> sub-clause: e), which means an error.
>
> > gives:
> >
> > tdc-mode auto: RTNETLINK answers: Operation not supported
>
> Looks perfectly coherent with 3.e)
Thanks, I misread this as clause 3.a being applicable (hasn't NOT a
higher precedence than AND? ;-)
> > unless I add an explicit "tdco 3".
>
> Yes, if you provide tcdo 3, then you are under 3.b).
>
> > According to your commit description, this is not the expected behavior?
> > Thanks!
>
> Looking back to my commit, I admit that the explanation is convoluted and could
> be hard to digest, but I do not see a mismatch between the description and the
> behaviour.
OK, so the description and the behaviour do match.
However, I still find it a bit counter-intuitive that
CAN_CTRLMODE_TDC_AUTO is not fully automatic, but automatic-with-one-
manual-knob.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Powered by blists - more mailing lists