Hi Vincent,

On Sat, 31 May 2025 at 10:25, Vincent Mailhol
[off-list ref] wrote:

On 30/05/2025 at 20:44, Geert Uytterhoeven wrote:

quoted

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
[off-list ref] wrote:

quoted

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.

quoted

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.

quoted

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.

quoted

    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.

quoted

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? ;-)

quoted

unless I add an explicit "tdco 3".

Yes, if you provide tcdo 3, then you are under 3.b).

quoted

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