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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<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