| 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
| ||
|
Message-ID: <CAMuHMdVEBLoG084rhBtELcFO+3cA9_UrZrUfspOeLNo80zyb9g@mail.gmail.com>
Date: Fri, 30 May 2025 13:44:31 +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,
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).
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
gives "can <FD,TDC-AUTO>" and "tdcv 0 tdco 3", while
ip link set can0 type can bitrate 500000 dbitrate 8000000 fd on
tdc-mode auto
gives:
tdc-mode auto: RTNETLINK answers: Operation not supported
unless I add an explicit "tdco 3".
According to your commit description, this is not the expected behavior?
Thanks!
>
> - 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.
>
> - 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.
>
> - 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.
>
> - Combination other than the one listed above are illegal and will
> return an error.
>
> N.B. above rules mean that whenever CAN_CTRLMODE_FD is provided, the
> previous TDC values will be overwritten. The only option to reuse
> previous TDC value is to not provide CAN_CTRLMODE_FD.
>
>
> All the new parameters are defined as u32. This arbitrary choice is
> done to mimic the other bittiming values with are also all of type
> u32. An u16 would have been sufficient to hold the TDC values.
>
> This patch completes below series (c.f. [1]):
> - commit 289ea9e4ae59 ("can: add new CAN FD bittiming parameters:
> Transmitter Delay Compensation (TDC)")
> - commit c25cc7993243 ("can: bittiming: add calculation for CAN FD
> Transmitter Delay Compensation (TDC)")
>
> [1] https://lore.kernel.org/linux-can/20210224002008.4158-1-mailhol.vincent@wanadoo.fr/T/#t
>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c
> @@ -37,8 +52,43 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
>
> if (data[IFLA_CAN_CTRLMODE]) {
> struct can_ctrlmode *cm = nla_data(data[IFLA_CAN_CTRLMODE]);
> + u32 tdc_flags = cm->flags & CAN_CTRLMODE_TDC_MASK;
>
> is_can_fd = cm->flags & cm->mask & CAN_CTRLMODE_FD;
> +
> + /* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually exclusive */
> + if (tdc_flags == CAN_CTRLMODE_TDC_MASK)
> + return -EOPNOTSUPP;
> + /* If one of the CAN_CTRLMODE_TDC_* flag is set then
> + * TDC must be set and vice-versa
> + */
> + if (!!tdc_flags != !!data[IFLA_CAN_TDC])
> + return -EOPNOTSUPP;
CAN_CTRLMODE_TDC_{AUTO,MANUAL} and none of tdc{v,o,f} provided is
rejected.
> + /* If providing TDC parameters, at least TDCO is
> + * needed. TDCV is needed if and only if
> + * CAN_CTRLMODE_TDC_MANUAL is set
> + */
> + if (data[IFLA_CAN_TDC]) {
> + struct nlattr *tb_tdc[IFLA_CAN_TDC_MAX + 1];
> + int err;
> +
> + err = nla_parse_nested(tb_tdc, IFLA_CAN_TDC_MAX,
> + data[IFLA_CAN_TDC],
> + can_tdc_policy, extack);
> + if (err)
> + return err;
> +
> + if (tb_tdc[IFLA_CAN_TDC_TDCV]) {
> + if (tdc_flags & CAN_CTRLMODE_TDC_AUTO)
> + return -EOPNOTSUPP;
> + } else {
> + if (tdc_flags & CAN_CTRLMODE_TDC_MANUAL)
> + return -EOPNOTSUPP;
> + }
> +
> + if (!tb_tdc[IFLA_CAN_TDC_TDCO])
> + return -EOPNOTSUPP;
CAN_CTRLMODE_TDC_AUTO and tdco not provided is rejected.
> + }
> }
>
> if (is_can_fd) {
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