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

[Vincent Mailhol]

On 02/06/2025 at 16:23, Geert Uytterhoeven wrote:
> Hi Vincent,
> 
> On Sat, 31 May 2025 at 10:25, Vincent Mailhol
> <mailhol.vincent@xxxxxxxxxx> wrote:
>> On 30/05/2025 at 20:44, Geert Uytterhoeven wrote:

(...)

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

Now I see where your confusion comes from.

But if I read:

  Paul and Mary not present

I understand that both are absent. I do not see an ambiguity that Paul may be
present and only Mary absent.

Well, I guess this is what occurs when you write in English instead of C.

And I just realize that there was an actual mistake in my description: I forgot
to state the obvious and I omitted to mention that TDC_OFF means that TDC is
forcefully deactivated.

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

Fair enough. In truth, TDC_AUTO and TDC_MANUAL means respectively
TDC_ON_WITH_TDCV_AUTO and TDC_ON_WITH_TDCV_MANUAL.

The problem is that it makes close to no sense to allow the user to ask to have
TDC explicitly ON and TDCO automatically calculated. And if I recall, TDC_AUTO
and TDC_MANUAL is also consistent with what I found in the datasheets.

On a side note, I still have to update Documentation/networking/can.rst to add
some TDC explanations. I will of course not change the naming (this is part of
the UAPI, so now it is set in stone), but I will try to clarify the concerns you
raised here so that the documentation is more clear than my patch comments.

Also, I am not writing this documentation any soon. CAN-XL has a higher priority
in my TODO list :)


Yours sincerely,
Vincent Mailhol