Re: [PATCH v6 4/6] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC)

[Vincent Mailhol <mailhol.vincent@wanadoo.fr>]

Hi Geert,

On 30/05/2025 at 20:44, Geert Uytterhoeven wrote:
> Hi Vincent,
> 
> 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@wanadoo.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.

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

> 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.


Yours sincerely,
Vincent Mailhol