[PATCH v5 0/7] add the netlink interface for CAN-FD Transmitter Delay Compensation (TDC)

[PATCH v5 0/7] add the netlink interface for CAN-FD Transmitter Delay Compensation (TDC)

[Vincent Mailhol]

The main goal of this series is to add a netlink interface for the TDC
parameters using netlink nested attributes. The series also contains a
few fix on the current TDC implementation.

This series completes the implementation of the Transmitter Delay
Compensation (TDC) in the kernel which started in March with those two
patches:
  - 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)")

The netlink interface was missing from this series because the initial
patch needed rework in order to make it more flexible for future
changes.

At that time, Marc suggested to take inspiration from the recently
released ethtool-netlink interface (Ref [1]).

ethtool uses nested attributes (c.f. NLA_NESTED type in validation
policy). A bit of trivia: the NLA_NESTED type was introduced in
version 2.6.15 of the kernel and thus actually predates Socket CAN.
Ref: commit bfa83a9e03cf ("[NETLINK]: Type-safe netlink messages/attributes interface")

Since then, Stephan shared additional remarks for improvement which
are addressed in revision v4 of this series [2].

The first patch allow a user to turn off a feature even if not
supported (e.g. allow "ip link set can0 type can bitrate 500000 fd
off" even if fd is not supported). This feature will be used later by
the fifth patch of the series.

The second patch allows TDCV and TDCO to be zero (previously, those
values were assigned a special meaning).

The third patch fixes the unit of the TDC parameters. In fact, those
should be measured in clock period (a.k.a. minimum time quantum) and
not in time quantum as it is done in current implementation.

The fourth patch addresses the concern of Marc and Stefan concerning
some devices which use a TDCO value relative to the Sample Point (so
far, the TDC implementation used the formula SSP = TDCV + TDCO). To do
so, an helper function to convert TDCO from an absolute value to a
value relative to the sample point is added.

The fifth patch is the real thing: the TDC netlink interface.

The sixth patch allows to retrieve TDCV from the device through a
callback function.

The seventh and last patch does a bit of cleanup in the ES58x driver
to remove some redundant TDC information from the documentation.

[1] https://lore.kernel.org/linux-can/20210407081557.m3sotnepbgasarri@pengutronix.de/
[2] https://lore.kernel.org/linux-can/75fa87a71a3f5fd7d7407a2c514b71690e56eb4e.camel@esd.eu/


** Changelog **

v4 -> v5:
  - move to can_validate() all of the checks on TDC parameters that do
    not need access to priv.
  - in can_tdc_get_size(), field IFLA_CAN_TDCV was not taken in
    account for size calculation when in automatic mode.
  - if can_tdc_is_enabled(priv) returns true, we know that one of
    either CAN_CTRLMODE_TDC_AUTO or CAN_CTRLMODE_TDC_MANUAL is
    set. After confirming that CAN_CTRLMODE_TDC_MANUAL if off, we then
    implicitly know that CAN_CTRLMODE_TDC_AUTO is set and do not need
    to check it again. Remove such redundant check in
    can_tdc_fill_info().

v3 -> v4:
  - add a patch to the series to change the unit from time quantum to
    clock period (a.k.a. minimum time quantum).
  - add a callback function to retrieve TDCV from the device.

v2 -> v3:
  - allows both TDCV and TDCO to be zero.
  - introduce the can_tdc_get_relative_tdco() helper function
  - other path of the series modified accordingly to above changes.

RFC v1 -> v2:
  The v2 fixes several issue in can_tdc_get_size() and
  can_tdc_fill_info(). Namely: can_tdc_get_size() returned an
  incorrect size if TDC was not implemented and can_tdc_fill_info()
  did not include a fail path with nla_nest_cancel().


Vincent Mailhol (7):
  can: netlink: allow user to turn off unsupported features
  can: bittiming: allow TDC{V,O} to be zero and add
    can_tdc_const::tdc{v,o,f}_min
  can: bittiming: change unit of TDC parameters to clock periods
  can: dev: add can_tdc_get_relative_tdco() helper function
  can: netlink: add interface for CAN-FD Transmitter Delay Compensation
    (TDC)
  can: netlink: add can_priv::do_get_auto_tdcv() to retrieve tdcv from
    device
  can: etas_es58x: clean-up documentation of struct es58x_fd_tx_conf_msg

 drivers/net/can/dev/bittiming.c           |  19 +-
 drivers/net/can/dev/netlink.c             | 213 +++++++++++++++++++++-
 drivers/net/can/usb/etas_es58x/es58x_fd.c |   7 +-
 drivers/net/can/usb/etas_es58x/es58x_fd.h |  23 +--
 include/linux/can/bittiming.h             |  80 +++++---
 include/linux/can/dev.h                   |  34 ++++
 include/uapi/linux/can/netlink.h          |  31 +++-
 7 files changed, 353 insertions(+), 54 deletions(-)

-- 
2.31.1

[PATCH v5 1/7] can: netlink: allow user to turn off unsupported features

[Vincent Mailhol]

The sanity checks on the control modes will reject any request related
to an unsupported features, even turning it off.

Example on an interface which does not support CAN-FD:

$ ip link set can0 type can bitrate 500000 fd off
RTNETLINK answers: Operation not supported

This patch lets such command go through (but requests to turn on an
unsupported feature are, of course, still denied).

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev/netlink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 147c23d7dab7..80425636049d 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -116,7 +116,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		maskedflags = cm->flags & cm->mask;
 
 		/* check whether provided bits are allowed to be passed */
-		if (cm->mask & ~(priv->ctrlmode_supported | ctrlstatic))
+		if (maskedflags & ~(priv->ctrlmode_supported | ctrlstatic))
 			return -EOPNOTSUPP;
 
 		/* do not check for static fd-non-iso if 'fd' is disabled */
-- 
2.31.1

[PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Vincent Mailhol]

ISO 11898-1 specifies in section 11.3.3 "Transmitter delay
compensation" that "the configuration range for [the] SSP position
shall be at least 0 to 63 minimum time quanta."

Because SSP = TDCV + TDCO, it means that we should allow both TDCV and
TDCO to hold zero value in order to honor SSP's minimum possible
value.

However, current implementation assigned special meaning to TDCV and
TDCO's zero values:
  * TDCV = 0 -> TDCV is automatically measured by the transceiver.
  * TDCO = 0 -> TDC is off.

In order to allow for those values to really be zero and to maintain
current features, we introduce two new flags:
  * CAN_CTRLMODE_TDC_AUTO indicates that the controller support
    automatic measurement of TDCV.
  * CAN_CTRLMODE_TDC_MANUAL indicates that the controller support
    manual configuration of TDCV. N.B.: current implementation failed
    to provide an option for the driver to indicate that only manual
    mode was supported.

TDC is disabled if both CAN_CTRLMODE_TDC_AUTO and
CAN_CTRLMODE_TDC_MANUAL flags are off, c.f. the helper function
can_tdc_is_enabled() which is also introduced in this patch.

Also, this patch adds three fields: tdcv_min, tdco_min and tdcf_min to
struct can_tdc_const. While we are not convinced that those three
fields could be anything else than zero, we can imagine that some
controllers might specify a lower bound on these. Thus, those minimums
are really added "just in case".

Comments of struct can_tdc and can_tdc_const are updated accordingly.

Finally, the changes are applied to the etas_es58x driver.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev/bittiming.c           | 10 ++--
 drivers/net/can/usb/etas_es58x/es58x_fd.c |  7 ++-
 include/linux/can/bittiming.h             | 64 +++++++++++++++++------
 include/linux/can/dev.h                   |  4 ++
 include/uapi/linux/can/netlink.h          |  2 +
 5 files changed, 65 insertions(+), 22 deletions(-)

diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index f49170eadd54..7dbda411915a 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -182,9 +182,12 @@ void can_calc_tdco(struct net_device *dev)
 	struct can_tdc *tdc = &priv->tdc;
 	const struct can_tdc_const *tdc_const = priv->tdc_const;
 
-	if (!tdc_const)
+	if (!tdc_const ||
+	    !(priv->ctrlmode_supported & CAN_CTRLMODE_TDC_AUTO))
 		return;
 
+	priv->ctrlmode &= ~CAN_CTRLMODE_TDC_MASK;
+
 	/* As specified in ISO 11898-1 section 11.3.3 "Transmitter
 	 * delay compensation" (TDC) is only applicable if data BRP is
 	 * one or two.
@@ -193,9 +196,10 @@ void can_calc_tdco(struct net_device *dev)
 		/* Reuse "normal" sample point and convert it to time quanta */
 		u32 sample_point_in_tq = can_bit_time(dbt) * dbt->sample_point / 1000;
 
+		if (sample_point_in_tq < tdc_const->tdco_min)
+			return;
 		tdc->tdco = min(sample_point_in_tq, tdc_const->tdco_max);
-	} else {
-		tdc->tdco = 0;
+		priv->ctrlmode |= CAN_CTRLMODE_TDC_AUTO;
 	}
 }
 #endif /* CONFIG_CAN_CALC_BITTIMING */
diff --git a/drivers/net/can/usb/etas_es58x/es58x_fd.c b/drivers/net/can/usb/etas_es58x/es58x_fd.c
index af042aa55f59..4f0cae29f4d8 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_fd.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_fd.c
@@ -428,7 +428,7 @@ static int es58x_fd_enable_channel(struct es58x_priv *priv)
 		es58x_fd_convert_bittiming(&tx_conf_msg.data_bittiming,
 					   &priv->can.data_bittiming);
 
-		if (priv->can.tdc.tdco) {
+		if (can_tdc_is_enabled(&priv->can)) {
 			tx_conf_msg.tdc_enabled = 1;
 			tx_conf_msg.tdco = cpu_to_le16(priv->can.tdc.tdco);
 			tx_conf_msg.tdcf = cpu_to_le16(priv->can.tdc.tdcf);
@@ -505,8 +505,11 @@ static const struct can_bittiming_const es58x_fd_data_bittiming_const = {
  * Register" from Microchip.
  */
 static const struct can_tdc_const es58x_tdc_const = {
+	.tdcv_min = 0,
 	.tdcv_max = 0, /* Manual mode not supported. */
+	.tdco_min = 0,
 	.tdco_max = 127,
+	.tdcf_min = 0,
 	.tdcf_max = 127
 };
 
@@ -523,7 +526,7 @@ const struct es58x_parameters es58x_fd_param = {
 	.clock = {.freq = 80 * CAN_MHZ},
 	.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_LISTENONLY |
 	    CAN_CTRLMODE_3_SAMPLES | CAN_CTRLMODE_FD | CAN_CTRLMODE_FD_NON_ISO |
-	    CAN_CTRLMODE_CC_LEN8_DLC,
+	    CAN_CTRLMODE_CC_LEN8_DLC | CAN_CTRLMODE_TDC_AUTO,
 	.tx_start_of_frame = 0xCEFA,	/* FACE in little endian */
 	.rx_start_of_frame = 0xFECA,	/* CAFE in little endian */
 	.tx_urb_cmd_max_len = ES58X_FD_TX_URB_CMD_MAX_LEN,
diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
index 9de6e9053e34..9e20260611cc 100644
--- a/include/linux/can/bittiming.h
+++ b/include/linux/can/bittiming.h
@@ -19,6 +19,9 @@
 /* Megahertz */
 #define CAN_MHZ 1000000UL
 
+#define CAN_CTRLMODE_TDC_MASK					\
+	(CAN_CTRLMODE_TDC_AUTO | CAN_CTRLMODE_TDC_MANUAL)
+
 /*
  * struct can_tdc - CAN FD Transmission Delay Compensation parameters
  *
@@ -33,29 +36,43 @@
  *
  * This structure contains the parameters to calculate that SSP.
  *
- * @tdcv: Transmitter Delay Compensation Value. Distance, in time
- *	quanta, from when the bit is sent on the TX pin to when it is
- *	received on the RX pin of the transmitter. Possible options:
+ * -+----------- one bit ----------+-- TX pin
+ *  |<--- Sample Point --->|
+ *
+ *                         --+----------- one bit ----------+-- RX pin
+ *  |<-------- TDCV -------->|
+ *                           |<------- TDCO ------->|
+ *  |<----------- Secondary Sample Point ---------->|
+ *
+ * @tdcv: Transmitter Delay Compensation Value. The time needed for
+ *	the signal to propagate, i.e. the distance, in time quanta,
+ *	from the start of the bit on the TX pin to when it is received
+ *	on the RX pin. @tdcv depends on the controller modes:
+ *
+ *	  CAN_CTRLMODE_TDC_AUTO is set: The transceiver dynamically
+ *	  measures @tdcv for each transmitted CAN FD frame and the
+ *	  value provided here should be ignored.
  *
- *	  0: automatic mode. The controller dynamically measures @tdcv
- *	  for each transmitted CAN FD frame.
+ *	  CAN_CTRLMODE_TDC_MANUAL is set: use the fixed provided @tdcv
+ *	  value.
  *
- *	  Other values: manual mode. Use the fixed provided value.
+ *	N.B. CAN_CTRLMODE_TDC_AUTO and CAN_CTRLMODE_TDC_MANUAL are
+ *	mutually exclusive. Only one can be set at a time. If both
+ *	CAN_TDC_CTRLMODE_AUTO and CAN_TDC_CTRLMODE_MANUAL are unset,
+ *	TDC is disabled and all the values of this structure should be
+ *	ignored.
  *
  * @tdco: Transmitter Delay Compensation Offset. Offset value, in time
  *	quanta, defining the distance between the start of the bit
  *	reception on the RX pin of the transceiver and the SSP
  *	position such that SSP = @tdcv + @tdco.
  *
- *	If @tdco is zero, then TDC is disabled and both @tdcv and
- *	@tdcf should be ignored.
- *
  * @tdcf: Transmitter Delay Compensation Filter window. Defines the
- *	minimum value for the SSP position in time quanta. If SSP is
- *	less than @tdcf, then no delay compensations occur and the
- *	normal sampling point is used instead. The feature is enabled
- *	if and only if @tdcv is set to zero (automatic mode) and @tdcf
- *	is configured to a value greater than @tdco.
+ *	minimum value for the SSP position in time quanta. If the SSP
+ *	position is less than @tdcf, then no delay compensations occur
+ *	and the normal sampling point is used instead. The feature is
+ *	enabled if and only if @tdcv is set to zero (automatic mode)
+ *	and @tdcf is configured to a value greater than @tdco.
  */
 struct can_tdc {
 	u32 tdcv;
@@ -67,19 +84,32 @@ struct can_tdc {
  * struct can_tdc_const - CAN hardware-dependent constant for
  *	Transmission Delay Compensation
  *
- * @tdcv_max: Transmitter Delay Compensation Value maximum value.
- *	Should be set to zero if the controller does not support
- *	manual mode for tdcv.
+ * @tdcv_min: Transmitter Delay Compensation Value minimum value. If
+ *	the controller does not support manual mode for tdcv
+ *	(c.f. flag CAN_CTRLMODE_TDC_MANUAL) then this value is
+ *	ignored.
+ * @tdcv_max: Transmitter Delay Compensation Value maximum value. If
+ *	the controller does not support manual mode for tdcv
+ *	(c.f. flag CAN_CTRLMODE_TDC_MANUAL) then this value is
+ *	ignored.
+ *
+ * @tdco_min: Transmitter Delay Compensation Offset minimum value.
  * @tdco_max: Transmitter Delay Compensation Offset maximum value.
  *	Should not be zero. If the controller does not support TDC,
  *	then the pointer to this structure should be NULL.
+ *
+ * @tdcf_min: Transmitter Delay Compensation Filter window minimum
+ *	value. If @tdcf_max is zero, this value is ignored.
  * @tdcf_max: Transmitter Delay Compensation Filter window maximum
  *	value. Should be set to zero if the controller does not
  *	support this feature.
  */
 struct can_tdc_const {
+	u32 tdcv_min;
 	u32 tdcv_max;
+	u32 tdco_min;
 	u32 tdco_max;
+	u32 tdcf_min;
 	u32 tdcf_max;
 };
 
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 27b275e463da..0be982fd22fb 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -88,6 +88,10 @@ struct can_priv {
 #endif
 };
 
+static inline bool can_tdc_is_enabled(const struct can_priv *priv)
+{
+	return !!(priv->ctrlmode & CAN_CTRLMODE_TDC_MASK);
+}
 
 /* helper to define static CAN controller features at device creation time */
 static inline void can_set_static_ctrlmode(struct net_device *dev,
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index f730d443b918..004cd09a7d49 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -101,6 +101,8 @@ struct can_ctrlmode {
 #define CAN_CTRLMODE_PRESUME_ACK	0x40	/* Ignore missing CAN ACKs */
 #define CAN_CTRLMODE_FD_NON_ISO		0x80	/* CAN FD in non-ISO mode */
 #define CAN_CTRLMODE_CC_LEN8_DLC	0x100	/* Classic CAN DLC option */
+#define CAN_CTRLMODE_TDC_AUTO		0x200	/* CAN transiver automatically calculates TDCV */
+#define CAN_CTRLMODE_TDC_MANUAL		0x400	/* TDCV is manually set up by user */
 
 /*
  * CAN device statistics
-- 
2.31.1

[PATCH v5 3/7] can: bittiming: change unit of TDC parameters to clock periods

[Vincent Mailhol]

In the current implementation, all Transmission Delay Compensation
(TDC) parameters are expressed in time quantum. However, ISO 11898-1
actually specifies that these should be expressed in *minimum* time
quantum.

Furthermore, the minimum time quantum is specified to be "one node
clock period long" (c.f. paragraph 11.3.1.1 "Bit time"). For sake of
simplicity, we prefer to use the "clock period" term instead of
"minimum time quantum" because we believe that it is more broadly
understood.

This patch fixes that discrepancy by updating the documentation and
the formula for TDCO calculation.

N.B. In can_calc_tdco(), the sample point (in time quantum) was
calculated using a division, thus introducing a risk of rounding and
truncation errors. On top of changing the unit to clock period, we
also modified the formula to use only additions.

Suggested-by: Stefan Mätje <Stefan.Maetje@esd.eu>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev/bittiming.c |  9 +++++----
 include/linux/can/bittiming.h   | 28 +++++++++++++++++-----------
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index 7dbda411915a..582463bc15f7 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -193,12 +193,13 @@ void can_calc_tdco(struct net_device *dev)
 	 * one or two.
 	 */
 	if (dbt->brp == 1 || dbt->brp == 2) {
-		/* Reuse "normal" sample point and convert it to time quanta */
-		u32 sample_point_in_tq = can_bit_time(dbt) * dbt->sample_point / 1000;
+		/* Sample point in clock periods */
+		u32 sample_point_in_tc = (CAN_SYNC_SEG + dbt->prop_seg +
+					  dbt->phase_seg1) * dbt->brp;
 
-		if (sample_point_in_tq < tdc_const->tdco_min)
+		if (sample_point_in_tc < tdc_const->tdco_min)
 			return;
-		tdc->tdco = min(sample_point_in_tq, tdc_const->tdco_max);
+		tdc->tdco = min(sample_point_in_tc, tdc_const->tdco_max);
 		priv->ctrlmode |= CAN_CTRLMODE_TDC_AUTO;
 	}
 }
diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
index 9e20260611cc..aebbe65dab7e 100644
--- a/include/linux/can/bittiming.h
+++ b/include/linux/can/bittiming.h
@@ -31,8 +31,8 @@
  *
  * To solve this issue, ISO 11898-1 introduces in section 11.3.3
  * "Transmitter delay compensation" a SSP (Secondary Sample Point)
- * equal to the distance, in time quanta, from the start of the bit
- * time on the TX pin to the actual measurement on the RX pin.
+ * equal to the distance from the start of the bit time on the TX pin
+ * to the actual measurement on the RX pin.
  *
  * This structure contains the parameters to calculate that SSP.
  *
@@ -44,8 +44,13 @@
  *                           |<------- TDCO ------->|
  *  |<----------- Secondary Sample Point ---------->|
  *
+ * To increase precision, contrary to the other bittiming parameters
+ * which are measured in time quanta, the TDC parameters are measured
+ * in clock periods (also referred as "minimum time quantum" in ISO
+ * 11898-1).
+ *
  * @tdcv: Transmitter Delay Compensation Value. The time needed for
- *	the signal to propagate, i.e. the distance, in time quanta,
+ *	the signal to propagate, i.e. the distance, in clock periods,
  *	from the start of the bit on the TX pin to when it is received
  *	on the RX pin. @tdcv depends on the controller modes:
  *
@@ -62,17 +67,18 @@
  *	TDC is disabled and all the values of this structure should be
  *	ignored.
  *
- * @tdco: Transmitter Delay Compensation Offset. Offset value, in time
- *	quanta, defining the distance between the start of the bit
- *	reception on the RX pin of the transceiver and the SSP
+ * @tdco: Transmitter Delay Compensation Offset. Offset value, in
+ *	clock periods, defining the distance between the start of the
+ *	bit reception on the RX pin of the transceiver and the SSP
  *	position such that SSP = @tdcv + @tdco.
  *
  * @tdcf: Transmitter Delay Compensation Filter window. Defines the
- *	minimum value for the SSP position in time quanta. If the SSP
- *	position is less than @tdcf, then no delay compensations occur
- *	and the normal sampling point is used instead. The feature is
- *	enabled if and only if @tdcv is set to zero (automatic mode)
- *	and @tdcf is configured to a value greater than @tdco.
+ *	minimum value for the SSP position in clock periods. If the
+ *	SSP position is less than @tdcf, then no delay compensations
+ *	occur and the normal sampling point is used instead. The
+ *	feature is enabled if and only if @tdcv is set to zero
+ *	(automatic mode) and @tdcf is configured to a value greater
+ *	than @tdco.
  */
 struct can_tdc {
 	u32 tdcv;
-- 
2.31.1

[PATCH v5 4/7] can: dev: add can_tdc_get_relative_tdco() helper function

[Vincent Mailhol]

struct can_tdc::tdco represents the absolute offset from TDCV. Some
controllers use instead an offset relative to the Sample Point (SP)
such that:
| SSP = TDCV + absolute TDCO
|     = TDCV + SP + relative TDCO

Consequently:
| relative TDCO = absolute TDCO - SP

The function can_tdc_get_relative_tdco() allow to retrieve this
relative TDCO value.

CC: Stefan Mätje <Stefan.Maetje@esd.eu>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 include/linux/can/dev.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 0be982fd22fb..fa75e29182a3 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -93,6 +93,35 @@ static inline bool can_tdc_is_enabled(const struct can_priv *priv)
 	return !!(priv->ctrlmode & CAN_CTRLMODE_TDC_MASK);
 }
 
+/*
+ * can_get_relative_tdco() - TDCO relative to the sample point
+ *
+ * struct can_tdc::tdco represents the absolute offset from TDCV. Some
+ * controllers use instead an offset relative to the Sample Point (SP)
+ * such that:
+ *
+ * SSP = TDCV + absolute TDCO
+ *     = TDCV + SP + relative TDCO
+ *
+ * -+----------- one bit ----------+-- TX pin
+ *  |<--- Sample Point --->|
+ *
+ *                         --+----------- one bit ----------+-- RX pin
+ *  |<-------- TDCV -------->|
+ *                           |<------------------------>| absolute TDCO
+ *                           |<--- Sample Point --->|
+ *                           |                      |<->| relative TDCO
+ *  |<------------- Secondary Sample Point ------------>|
+ */
+static inline s32 can_get_relative_tdco(const struct can_priv *priv)
+{
+	const struct can_bittiming *dbt = &priv->data_bittiming;
+	s32 sample_point_in_tc =  (CAN_SYNC_SEG + dbt->prop_seg +
+				   dbt->phase_seg1) * dbt->brp;
+
+	return (s32)priv->tdc.tdco - sample_point_in_tc;
+}
+
 /* helper to define static CAN controller features at device creation time */
 static inline void can_set_static_ctrlmode(struct net_device *dev,
 					   u32 static_mode)
-- 
2.31.1

[PATCH v5 5/7] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC)

[Vincent Mailhol]

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.

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@wanadoo.fr>
---
 drivers/net/can/dev/netlink.c    | 202 ++++++++++++++++++++++++++++++-
 include/uapi/linux/can/netlink.h |  29 ++++-
 2 files changed, 225 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 80425636049d..f05745c96b9c 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -2,6 +2,7 @@
 /* Copyright (C) 2005 Marc Kleine-Budde, Pengutronix
  * Copyright (C) 2006 Andrey Volkov, Varma Electronics
  * Copyright (C) 2008-2009 Wolfgang Grandegger <wg@grandegger.com>
+ * Copyright (C) 2021 Vincent Mailhol <mailhol.vincent@wanadoo.fr>
  */
 
 #include <linux/can/dev.h>
@@ -19,6 +20,19 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
 	[IFLA_CAN_DATA_BITTIMING] = { .len = sizeof(struct can_bittiming) },
 	[IFLA_CAN_DATA_BITTIMING_CONST]	= { .len = sizeof(struct can_bittiming_const) },
 	[IFLA_CAN_TERMINATION] = { .type = NLA_U16 },
+	[IFLA_CAN_TDC] = { .type = NLA_NESTED },
+};
+
+static const struct nla_policy can_tdc_policy[IFLA_CAN_TDC_MAX + 1] = {
+	[IFLA_CAN_TDC_TDCV_MIN] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCV_MAX] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCO_MIN] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCO_MAX] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCF_MIN] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCF_MAX] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCV] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCO] = { .type = NLA_U32 },
+	[IFLA_CAN_TDC_TDCF] = { .type = NLA_U32 },
 };
 
 static int can_validate(struct nlattr *tb[], struct nlattr *data[],
@@ -30,6 +44,7 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 	 * - nominal/arbitration bittiming
 	 * - data bittiming
 	 * - control mode with CAN_CTRLMODE_FD set
+	 * - TDC parameters are coherent (details below)
 	 */
 
 	if (!data)
@@ -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;
+		/* 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;
+		}
 	}
 
 	if (is_can_fd) {
@@ -46,7 +96,7 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 			return -EOPNOTSUPP;
 	}
 
-	if (data[IFLA_CAN_DATA_BITTIMING]) {
+	if (data[IFLA_CAN_DATA_BITTIMING] || data[IFLA_CAN_TDC]) {
 		if (!is_can_fd)
 			return -EOPNOTSUPP;
 	}
@@ -54,11 +104,62 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 	return 0;
 }
 
+static int can_tdc_changelink(struct net_device *dev, const struct nlattr *nla,
+			      struct netlink_ext_ack *extack)
+{
+	struct nlattr *tb_tdc[IFLA_CAN_TDC_MAX + 1];
+	struct can_priv *priv = netdev_priv(dev);
+	struct can_tdc *tdc = &priv->tdc;
+	const struct can_tdc_const *tdc_const = priv->tdc_const;
+	int err;
+
+	if (!tdc_const || !can_tdc_is_enabled(priv))
+		return -EOPNOTSUPP;
+
+	if (dev->flags & IFF_UP)
+		return -EBUSY;
+
+	err = nla_parse_nested(tb_tdc, IFLA_CAN_TDC_MAX, nla,
+			       can_tdc_policy, extack);
+	if (err)
+		return err;
+
+	if (tb_tdc[IFLA_CAN_TDC_TDCV]) {
+		u32 tdcv = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCV]);
+
+		if (tdcv < tdc_const->tdcv_min || tdcv > tdc_const->tdcv_max)
+			return -EINVAL;
+
+		tdc->tdcv = tdcv;
+	}
+
+	if (tb_tdc[IFLA_CAN_TDC_TDCO]) {
+		u32 tdco = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCO]);
+
+		if (tdco < tdc_const->tdco_min || tdco > tdc_const->tdco_max)
+			return -EINVAL;
+
+		tdc->tdco = tdco;
+	}
+
+	if (tb_tdc[IFLA_CAN_TDC_TDCF]) {
+		u32 tdcf = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCF]);
+
+		if (tdcf < tdc_const->tdcf_min || tdcf > tdc_const->tdcf_max)
+			return -EINVAL;
+
+		tdc->tdcf = tdcf;
+	}
+
+	return 0;
+}
+
 static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 			  struct nlattr *data[],
 			  struct netlink_ext_ack *extack)
 {
 	struct can_priv *priv = netdev_priv(dev);
+	u32 tdc_mask = 0;
 	int err;
 
 	/* We need synchronization with dev->stop() */
@@ -138,7 +239,11 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 			dev->mtu = CAN_MTU;
 			memset(&priv->data_bittiming, 0,
 			       sizeof(priv->data_bittiming));
+			memset(&priv->tdc, 0, sizeof(priv->tdc));
+			priv->ctrlmode &= ~CAN_CTRLMODE_TDC_MASK;
 		}
+
+		tdc_mask = cm->mask & CAN_CTRLMODE_TDC_MASK;
 	}
 
 	if (data[IFLA_CAN_RESTART_MS]) {
@@ -189,8 +294,6 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 
 		memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
 
-		can_calc_tdco(dev);
-
 		if (priv->do_set_data_bittiming) {
 			/* Finally, set the bit-timing registers */
 			err = priv->do_set_data_bittiming(dev);
@@ -223,9 +326,52 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		priv->termination = termval;
 	}
 
+	if (data[IFLA_CAN_TDC]) {
+		/* Use the provided TDC parameters */
+		err = can_tdc_changelink(dev, data[IFLA_CAN_TDC], extack);
+		if (err)
+			return err;
+	} else if (!tdc_mask) {
+		/* Both TDC parameters and flags not provided: do calculation */
+		can_calc_tdco(dev);
+	} /* else: both CAN_CTRLMODE_TDC_{AUTO,MANUAL} are explicitly
+	   * turned off. TDC is disabled: do nothing
+	   */
+
 	return 0;
 }
 
+static size_t can_tdc_get_size(const struct net_device *dev)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	size_t size;
+
+	if (!priv->tdc_const)
+		return 0;
+
+	size = nla_total_size(0);			/* nest IFLA_CAN_TDC */
+	if (priv->ctrlmode_supported & CAN_CTRLMODE_TDC_MANUAL) {
+		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCV_MIN */
+		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCV_MAX */
+	}
+	size += nla_total_size(sizeof(u32));		/* IFLA_CAN_TDCO_MIN */
+	size += nla_total_size(sizeof(u32));		/* IFLA_CAN_TDCO_MAX */
+	if (priv->tdc_const->tdcf_max) {
+		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCF_MIN */
+		size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCF_MAX */
+	}
+
+	if (can_tdc_is_enabled(priv)) {
+		if (priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL)
+			size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCV */
+		size += nla_total_size(sizeof(u32));		/* IFLA_CAN_TDCO */
+		if (priv->tdc_const->tdcf_max)
+			size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCF */
+	}
+
+	return size;
+}
+
 static size_t can_get_size(const struct net_device *dev)
 {
 	struct can_priv *priv = netdev_priv(dev);
@@ -257,10 +403,56 @@ static size_t can_get_size(const struct net_device *dev)
 		size += nla_total_size(sizeof(*priv->data_bitrate_const) *
 				       priv->data_bitrate_const_cnt);
 	size += sizeof(priv->bitrate_max);			/* IFLA_CAN_BITRATE_MAX */
+	size += can_tdc_get_size(dev);				/* IFLA_CAN_TDC */
 
 	return size;
 }
 
+static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
+{
+	struct nlattr *nest;
+	struct can_priv *priv = netdev_priv(dev);
+	struct can_tdc *tdc = &priv->tdc;
+	const struct can_tdc_const *tdc_const = priv->tdc_const;
+
+	if (!tdc_const)
+		return 0;
+
+	nest = nla_nest_start(skb, IFLA_CAN_TDC);
+	if (!nest)
+		return -EMSGSIZE;
+
+	if (priv->ctrlmode_supported & CAN_CTRLMODE_TDC_MANUAL &&
+	    (nla_put_u32(skb, IFLA_CAN_TDC_TDCV_MIN, tdc_const->tdcv_min) ||
+	     nla_put_u32(skb, IFLA_CAN_TDC_TDCV_MAX, tdc_const->tdcv_max)))
+		goto err_cancel;
+	if (nla_put_u32(skb, IFLA_CAN_TDC_TDCO_MIN, tdc_const->tdco_min) ||
+	    nla_put_u32(skb, IFLA_CAN_TDC_TDCO_MAX, tdc_const->tdco_max))
+		goto err_cancel;
+	if (tdc_const->tdcf_max &&
+	    (nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MIN, tdc_const->tdcf_min) ||
+	     nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MAX, tdc_const->tdcf_max)))
+		goto err_cancel;
+
+	if (can_tdc_is_enabled(priv)) {
+		if (priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL &&
+		    nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdc->tdcv))
+			goto err_cancel;
+		if (nla_put_u32(skb, IFLA_CAN_TDC_TDCO, tdc->tdco))
+			goto err_cancel;
+		if (tdc_const->tdcf_max &&
+		    nla_put_u32(skb, IFLA_CAN_TDC_TDCF, tdc->tdcf))
+			goto err_cancel;
+	}
+
+	nla_nest_end(skb, nest);
+	return 0;
+
+err_cancel:
+	nla_nest_cancel(skb, nest);
+	return -EMSGSIZE;
+}
+
 static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 {
 	struct can_priv *priv = netdev_priv(dev);
@@ -318,7 +510,9 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 
 	    (nla_put(skb, IFLA_CAN_BITRATE_MAX,
 		     sizeof(priv->bitrate_max),
-		     &priv->bitrate_max))
+		     &priv->bitrate_max)) ||
+
+	    (can_tdc_fill_info(skb, dev))
 	    )
 
 		return -EMSGSIZE;
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index 004cd09a7d49..75b85c60efb2 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -136,10 +136,35 @@ enum {
 	IFLA_CAN_BITRATE_CONST,
 	IFLA_CAN_DATA_BITRATE_CONST,
 	IFLA_CAN_BITRATE_MAX,
-	__IFLA_CAN_MAX
+	IFLA_CAN_TDC,
+
+	/* add new constants above here */
+	__IFLA_CAN_MAX,
+	IFLA_CAN_MAX = __IFLA_CAN_MAX - 1
 };
 
-#define IFLA_CAN_MAX	(__IFLA_CAN_MAX - 1)
+/*
+ * CAN FD Transmitter Delay Compensation (TDC)
+ *
+ * Please refer to struct can_tdc_const and can_tdc in
+ * include/linux/can/bittiming.h for further details.
+ */
+enum {
+	IFLA_CAN_TDC_UNSPEC,
+	IFLA_CAN_TDC_TDCV_MIN,	/* u32 */
+	IFLA_CAN_TDC_TDCV_MAX,	/* u32 */
+	IFLA_CAN_TDC_TDCO_MIN,	/* u32 */
+	IFLA_CAN_TDC_TDCO_MAX,	/* u32 */
+	IFLA_CAN_TDC_TDCF_MIN,	/* u32 */
+	IFLA_CAN_TDC_TDCF_MAX,	/* u32 */
+	IFLA_CAN_TDC_TDCV,	/* u32 */
+	IFLA_CAN_TDC_TDCO,	/* u32 */
+	IFLA_CAN_TDC_TDCF,	/* u32 */
+
+	/* add new constants above here */
+	__IFLA_CAN_TDC,
+	IFLA_CAN_TDC_MAX = __IFLA_CAN_TDC - 1
+};
 
 /* u16 termination range: 1..65535 Ohms */
 #define CAN_TERMINATION_DISABLED 0
-- 
2.31.1

[PATCH v5 6/7] can: netlink: add can_priv::do_get_auto_tdcv() to retrieve tdcv from device

[Vincent Mailhol]

Some CAN device can measure the TDCV (Transmission Delay Compensation
Value) automatically for each transmitted CAN frames.

A callback function do_get_auto_tdcv() is added to retrieve that
value. This function is used only if CAN_CTRLMODE_TDC_AUTO is enabled
(if CAN_CTRLMODE_TDC_MANUAL is selected, the TDCV value is provided by
the user).

If the device does not support reporting of TDCV, do_get_auto_tdcv()
should be set to NULL and TDCV will not be reported by the netlink
interface.

On success, do_get_auto_tdcv() shall return 0. If the value can not be
measured by the device, for example because network is down or because
no frames were transmitted yet, can_priv::do_get_auto_tdcv() shall
return a negative error code (e.g. -EINVAL) to signify that the value
is not yet available. In such cases, TDCV is not reported by the
netlink interface.

CC: Stefan Mätje <stefan.maetje@esd.eu>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev/netlink.c | 15 ++++++++++++---
 include/linux/can/dev.h       |  1 +
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index f05745c96b9c..776284593b5d 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -362,7 +362,8 @@ static size_t can_tdc_get_size(const struct net_device *dev)
 	}
 
 	if (can_tdc_is_enabled(priv)) {
-		if (priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL)
+		if (priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL ||
+		    priv->do_get_auto_tdcv)
 			size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCV */
 		size += nla_total_size(sizeof(u32));		/* IFLA_CAN_TDCO */
 		if (priv->tdc_const->tdcf_max)
@@ -435,8 +436,16 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
 		goto err_cancel;
 
 	if (can_tdc_is_enabled(priv)) {
-		if (priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL &&
-		    nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdc->tdcv))
+		u32 tdcv;
+		int err = -EINVAL;
+
+		if (priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL) {
+			tdcv = tdc->tdcv;
+			err = 0;
+		} else if (priv->do_get_auto_tdcv) {
+			err = priv->do_get_auto_tdcv(dev, &tdcv);
+		}
+		if (!err && nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdcv))
 			goto err_cancel;
 		if (nla_put_u32(skb, IFLA_CAN_TDC_TDCO, tdc->tdco))
 			goto err_cancel;
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index fa75e29182a3..266d0fe9de9d 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -74,6 +74,7 @@ struct can_priv {
 			    enum can_state *state);
 	int (*do_get_berr_counter)(const struct net_device *dev,
 				   struct can_berr_counter *bec);
+	int (*do_get_auto_tdcv)(const struct net_device *dev, u32 *tdcv);
 
 	unsigned int echo_skb_max;
 	struct sk_buff **echo_skb;
-- 
2.31.1

[PATCH v5 7/7] can: etas_es58x: clean-up documentation of struct es58x_fd_tx_conf_msg

[Vincent Mailhol]

The documentation of struct es58x_fd_tx_conf_msg explains in details
the different TDC parameters. However, those description are redundant
with the documentation of struct can_tdc.

Remove most of the description.

Also, fixes a typo in the reference to the datasheet (E701 -> E70).

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/usb/etas_es58x/es58x_fd.h | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/net/can/usb/etas_es58x/es58x_fd.h b/drivers/net/can/usb/etas_es58x/es58x_fd.h
index ee18a87e40c0..a191891b8777 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_fd.h
+++ b/drivers/net/can/usb/etas_es58x/es58x_fd.h
@@ -96,23 +96,14 @@ struct es58x_fd_bittiming {
  * @ctrlmode: type enum es58x_fd_ctrlmode.
  * @canfd_enabled: boolean (0: Classical CAN, 1: CAN and/or CANFD).
  * @data_bittiming: Bittiming for flexible data-rate transmission.
- * @tdc_enabled: Transmitter Delay Compensation switch (0: disabled,
- *	1: enabled). On very high bitrates, the delay between when the
- *	bit is sent and received on the CANTX and CANRX pins of the
- *	transceiver start to be significant enough for errors to occur
- *	and thus need to be compensated.
- * @tdco: Transmitter Delay Compensation Offset. Offset value, in time
- *	quanta, defining the delay between the start of the bit
- *	reception on the CANRX pin of the transceiver and the SSP
- *	(Secondary Sample Point). Valid values: 0 to 127.
- * @tdcf: Transmitter Delay Compensation Filter window. Defines the
- *	minimum value for the SSP position, in time quanta. The
- *	feature is enabled when TDCF is configured to a value greater
- *	than TDCO. Valid values: 0 to 127.
+ * @tdc_enabled: Transmitter Delay Compensation switch (0: TDC is
+ *	disabled, 1: TDC is enabled).
+ * @tdco: Transmitter Delay Compensation Offset.
+ * @tdcf: Transmitter Delay Compensation Filter window.
  *
- * Please refer to the microcontroller datasheet: "SAM
- * E701/S70/V70/V71 Family" section 49 "Controller Area Network
- * (MCAN)" for additional information.
+ * Please refer to the microcontroller datasheet: "SAM E70/S70/V70/V71
+ * Family" section 49 "Controller Area Network (MCAN)" for additional
+ * information.
  */
 struct es58x_fd_tx_conf_msg {
 	struct es58x_fd_bittiming nominal_bittiming;
-- 
2.31.1

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2074 bytes --]

On 15.08.2021 12:32:43, Vincent Mailhol wrote:
> ISO 11898-1 specifies in section 11.3.3 "Transmitter delay
> compensation" that "the configuration range for [the] SSP position
> shall be at least 0 to 63 minimum time quanta."
> 
> Because SSP = TDCV + TDCO, it means that we should allow both TDCV and
> TDCO to hold zero value in order to honor SSP's minimum possible
> value.
> 
> However, current implementation assigned special meaning to TDCV and
> TDCO's zero values:
>   * TDCV = 0 -> TDCV is automatically measured by the transceiver.
>   * TDCO = 0 -> TDC is off.
> 
> In order to allow for those values to really be zero and to maintain
> current features, we introduce two new flags:
>   * CAN_CTRLMODE_TDC_AUTO indicates that the controller support
>     automatic measurement of TDCV.
>   * CAN_CTRLMODE_TDC_MANUAL indicates that the controller support
>     manual configuration of TDCV. N.B.: current implementation failed
>     to provide an option for the driver to indicate that only manual
>     mode was supported.
> 
> TDC is disabled if both CAN_CTRLMODE_TDC_AUTO and
> CAN_CTRLMODE_TDC_MANUAL flags are off, c.f. the helper function
> can_tdc_is_enabled() which is also introduced in this patch.

Nitpick: We can only say that TDC is disabled, if the driver supports
the TDC interface at all, which is the case if tdc_const is set.

> Also, this patch adds three fields: tdcv_min, tdco_min and tdcf_min to
> struct can_tdc_const. While we are not convinced that those three
> fields could be anything else than zero, we can imagine that some
> controllers might specify a lower bound on these. Thus, those minimums
> are really added "just in case".

I'm not sure, if we talked about the mcp251xfd's tcdo, valid values are
-64...63.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Vincent MAILHOL]

On Mon. 16 Aug 2021 at 17:42, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 15.08.2021 12:32:43, Vincent Mailhol wrote:
> > ISO 11898-1 specifies in section 11.3.3 "Transmitter delay
> > compensation" that "the configuration range for [the] SSP position
> > shall be at least 0 to 63 minimum time quanta."
> >
> > Because SSP = TDCV + TDCO, it means that we should allow both TDCV and
> > TDCO to hold zero value in order to honor SSP's minimum possible
> > value.
> >
> > However, current implementation assigned special meaning to TDCV and
> > TDCO's zero values:
> >   * TDCV = 0 -> TDCV is automatically measured by the transceiver.
> >   * TDCO = 0 -> TDC is off.
> >
> > In order to allow for those values to really be zero and to maintain
> > current features, we introduce two new flags:
> >   * CAN_CTRLMODE_TDC_AUTO indicates that the controller support
> >     automatic measurement of TDCV.
> >   * CAN_CTRLMODE_TDC_MANUAL indicates that the controller support
> >     manual configuration of TDCV. N.B.: current implementation failed
> >     to provide an option for the driver to indicate that only manual
> >     mode was supported.
> >
> > TDC is disabled if both CAN_CTRLMODE_TDC_AUTO and
> > CAN_CTRLMODE_TDC_MANUAL flags are off, c.f. the helper function
> > can_tdc_is_enabled() which is also introduced in this patch.
>
> Nitpick: We can only say that TDC is disabled, if the driver supports
> the TDC interface at all, which is the case if tdc_const is set.

I would argue that saying that a device does not support TDC is
equivalent to saying that TDC is always disabled for that device.
Especially, the function can_tdc_is_enabled() can be used even if
the device does not support TDC (even if there is no benefit
doing so).

Do you still want me to rephrase this part?

> > Also, this patch adds three fields: tdcv_min, tdco_min and tdcf_min to
> > struct can_tdc_const. While we are not convinced that those three
> > fields could be anything else than zero, we can imagine that some
> > controllers might specify a lower bound on these. Thus, those minimums
> > are really added "just in case".
>
> I'm not sure, if we talked about the mcp251xfd's tcdo, valid values are
> -64...63.

Yes! Stefan shed some light on this. The mcp251xfd uses a tdco
value which is relative to the sample point.
| SSP = TDCV + absolute TDCO
|     = TDCV + SP + relative TDCO

Consequently:
| relative TDCO = absolute TDCO - SP

Which is also why TDCO can be negative.

I added an helper function can_tdc_get_relative_tdco() in the
fourth path of this series:
https://lore.kernel.org/linux-can/20210814091750.73931-5-mailhol.vincent@wanadoo.fr/T/#u

Devices which use the absolute TDCO can directly use
can_priv->tdc.tdco. Devices which use the relative TDCO such as
the mcp251xfd should use this helper function instead.

However, you will still need to convert the TDCO valid range from
relative values to absolute ones. In your case 0..127.


Yours sincerely,
Vincent

Re: [PATCH v5 4/7] can: dev: add can_tdc_get_relative_tdco() helper function

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2659 bytes --]

On 15.08.2021 12:32:45, Vincent Mailhol wrote:
> struct can_tdc::tdco represents the absolute offset from TDCV. Some
> controllers use instead an offset relative to the Sample Point (SP)
> such that:
> | SSP = TDCV + absolute TDCO
> |     = TDCV + SP + relative TDCO
> 
> Consequently:
> | relative TDCO = absolute TDCO - SP
> 
> The function can_tdc_get_relative_tdco() allow to retrieve this
> relative TDCO value.
> 
> CC: Stefan Mätje <Stefan.Maetje@esd.eu>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  include/linux/can/dev.h | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
> index 0be982fd22fb..fa75e29182a3 100644
> --- a/include/linux/can/dev.h
> +++ b/include/linux/can/dev.h
> @@ -93,6 +93,35 @@ static inline bool can_tdc_is_enabled(const struct can_priv *priv)
>  	return !!(priv->ctrlmode & CAN_CTRLMODE_TDC_MASK);
>  }
>  
> +/*
> + * can_get_relative_tdco() - TDCO relative to the sample point
> + *
> + * struct can_tdc::tdco represents the absolute offset from TDCV. Some
> + * controllers use instead an offset relative to the Sample Point (SP)
> + * such that:
> + *
> + * SSP = TDCV + absolute TDCO
> + *     = TDCV + SP + relative TDCO
> + *
> + * -+----------- one bit ----------+-- TX pin
> + *  |<--- Sample Point --->|
> + *
> + *                         --+----------- one bit ----------+-- RX pin
> + *  |<-------- TDCV -------->|
> + *                           |<------------------------>| absolute TDCO
> + *                           |<--- Sample Point --->|
> + *                           |                      |<->| relative TDCO
> + *  |<------------- Secondary Sample Point ------------>|
> + */
> +static inline s32 can_get_relative_tdco(const struct can_priv *priv)
> +{
> +	const struct can_bittiming *dbt = &priv->data_bittiming;
> +	s32 sample_point_in_tc =  (CAN_SYNC_SEG + dbt->prop_seg +
                                ^^

Only one space. I'll fix the while applying.

> +				   dbt->phase_seg1) * dbt->brp;
> +
> +	return (s32)priv->tdc.tdco - sample_point_in_tc;
> +}
> +
>  /* helper to define static CAN controller features at device creation time */
>  static inline void can_set_static_ctrlmode(struct net_device *dev,
>  					   u32 static_mode)

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4917 bytes --]

On 16.08.2021 19:24:43, Vincent MAILHOL wrote:
> On Mon. 16 Aug 2021 at 17:42, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 15.08.2021 12:32:43, Vincent Mailhol wrote:
> > > ISO 11898-1 specifies in section 11.3.3 "Transmitter delay
> > > compensation" that "the configuration range for [the] SSP position
> > > shall be at least 0 to 63 minimum time quanta."
> > >
> > > Because SSP = TDCV + TDCO, it means that we should allow both TDCV and
> > > TDCO to hold zero value in order to honor SSP's minimum possible
> > > value.
> > >
> > > However, current implementation assigned special meaning to TDCV and
> > > TDCO's zero values:
> > >   * TDCV = 0 -> TDCV is automatically measured by the transceiver.
> > >   * TDCO = 0 -> TDC is off.
> > >
> > > In order to allow for those values to really be zero and to maintain
> > > current features, we introduce two new flags:
> > >   * CAN_CTRLMODE_TDC_AUTO indicates that the controller support
> > >     automatic measurement of TDCV.
> > >   * CAN_CTRLMODE_TDC_MANUAL indicates that the controller support
> > >     manual configuration of TDCV. N.B.: current implementation failed
> > >     to provide an option for the driver to indicate that only manual
> > >     mode was supported.
> > >
> > > TDC is disabled if both CAN_CTRLMODE_TDC_AUTO and
> > > CAN_CTRLMODE_TDC_MANUAL flags are off, c.f. the helper function
> > > can_tdc_is_enabled() which is also introduced in this patch.
> >
> > Nitpick: We can only say that TDC is disabled, if the driver supports
> > the TDC interface at all, which is the case if tdc_const is set.
> 
> I would argue that saying that a device does not support TDC is
> equivalent to saying that TDC is always disabled for that device.
> Especially, the function can_tdc_is_enabled() can be used even if
> the device does not support TDC (even if there is no benefit
> doing so).
> 
> Do you still want me to rephrase this part?
> 
> > > Also, this patch adds three fields: tdcv_min, tdco_min and tdcf_min to
> > > struct can_tdc_const. While we are not convinced that those three
> > > fields could be anything else than zero, we can imagine that some
> > > controllers might specify a lower bound on these. Thus, those minimums
> > > are really added "just in case".
> >
> > I'm not sure, if we talked about the mcp251xfd's tcdo, valid values are
> > -64...63.
> 
> Yes! Stefan shed some light on this. The mcp251xfd uses a tdco
> value which is relative to the sample point.

I don't read the documentation this way....

> | SSP = TDCV + absolute TDCO
> |     = TDCV + SP + relative TDCO
> 
> Consequently:
> | relative TDCO = absolute TDCO - SP

In the mcp15xxfd family manual
(http://ww1.microchip.com/downloads/en/DeviceDoc/MCP251XXFD-CAN-FD-Controller-Module-Family-Reference-Manual-20005678B.pdf)
in the 2mbit/s data bit rate example in table 3-5 (page 21) it says:

| DTSEG1  15 DTQ
| DTSEG2   4 DTQ
| TDCO    15 DTQ

The mcp251xfd driver uses 15, the framework calculates 16 (== Sync Seg+
tseg1, which is correct), and relative tdco would be 0:

| mcp251xfd_set_bittiming: tdco=15, priv->tdc.tdc=16, relative_tdco=0

Here the output with the patched ip tool:

| 4: mcp251xfd0: <NOARP,UP,LOWER_UP,ECHO> mtu 72 qdisc pfifo_fast state UP mode DEFAULT group default qlen 10
|     link/can  promiscuity 0 minmtu 0 maxmtu 0 
|     can <FD,TDC_AUTO> state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 100 
|           bitrate 500000 sample-point 0.875
|           tq 25 prop-seg 34 phase-seg1 35 phase-seg2 10 sjw 1 brp 1
|           mcp251xfd: tseg1 2..256 tseg2 1..128 sjw 1..128 brp 1..256 brp_inc 1
|           dbitrate 2000000 dsample-point 0.750
|           dtq 25 dprop-seg 7 dphase-seg1 7 dphase-seg2 5 dsjw 1 dbrp 1
|           tdco 15
|           mcp251xfd: dtseg1 1..32 dtseg2 1..16 dsjw 1..16 dbrp 1..256 dbrp_inc 1
|           tdco 0..127
|           clock 40000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 parentbus spi parentdev spi0.0


> Which is also why TDCO can be negative.
> 
> I added an helper function can_tdc_get_relative_tdco() in the
> fourth path of this series:
> https://lore.kernel.org/linux-can/20210814091750.73931-5-mailhol.vincent@wanadoo.fr/T/#u
> 
> Devices which use the absolute TDCO can directly use
> can_priv->tdc.tdco. Devices which use the relative TDCO such as
> the mcp251xfd should use this helper function instead.

Don't think so....

> However, you will still need to convert the TDCO valid range from
> relative values to absolute ones. In your case 0..127.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2260 bytes --]

On 16.08.2021 14:25:19, Marc Kleine-Budde wrote:
> > > I'm not sure, if we talked about the mcp251xfd's tcdo, valid values are
> > > -64...63.
> > 
> > Yes! Stefan shed some light on this. The mcp251xfd uses a tdco
> > value which is relative to the sample point.
> 
> I don't read the documentation this way....
> 
> > | SSP = TDCV + absolute TDCO
> > |     = TDCV + SP + relative TDCO
> > 
> > Consequently:
> > | relative TDCO = absolute TDCO - SP
> 
> In the mcp15xxfd family manual
> (http://ww1.microchip.com/downloads/en/DeviceDoc/MCP251XXFD-CAN-FD-Controller-Module-Family-Reference-Manual-20005678B.pdf)
> in the 2mbit/s data bit rate example in table 3-5 (page 21) it says:
> 
> | DTSEG1  15 DTQ
> | DTSEG2   4 DTQ
> | TDCO    15 DTQ
> 
> The mcp251xfd driver uses 15, the framework calculates 16 (== Sync Seg+
> tseg1, which is correct), and relative tdco would be 0:
> 
> | mcp251xfd_set_bittiming: tdco=15, priv->tdc.tdc=16, relative_tdco=0
> 
> Here the output with the patched ip tool:

Sorry, the previous output was not using the sample points of the
example in the data sheet, this is the fixed output:

| 6: mcp251xfd0: <NOARP,UP,LOWER_UP,ECHO> mtu 72 qdisc pfifo_fast state UP mode DEFAULT group default qlen 10
|     link/can  promiscuity 0 minmtu 0 maxmtu 0 
|     can <FD,TDC_AUTO> state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 100 
|           bitrate 500000 sample-point 0.800
|           tq 25 prop-seg 31 phase-seg1 32 phase-seg2 16 sjw 1 brp 1
|           mcp251xfd: tseg1 2..256 tseg2 1..128 sjw 1..128 brp 1..256 brp_inc 1
|           dbitrate 2000000 dsample-point 0.800
|           dtq 25 dprop-seg 7 dphase-seg1 8 dphase-seg2 4 dsjw 1 dbrp 1
|           tdco 16
|           mcp251xfd: dtseg1 1..32 dtseg2 1..16 dsjw 1..16 dbrp 1..256 dbrp_inc 1
|           tdco 0..127
|           clock 40000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 parentbus spi parentdev spi0.0 

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 6095 bytes --]

On 16.08.2021 14:33:09, Marc Kleine-Budde wrote:
> On 16.08.2021 14:25:19, Marc Kleine-Budde wrote:
> > > > I'm not sure, if we talked about the mcp251xfd's tcdo, valid values are
> > > > -64...63.
> > > 
> > > Yes! Stefan shed some light on this. The mcp251xfd uses a tdco
> > > value which is relative to the sample point.
> > 
> > I don't read the documentation this way....
> > 
> > > | SSP = TDCV + absolute TDCO
> > > |     = TDCV + SP + relative TDCO
> > > 
> > > Consequently:
> > > | relative TDCO = absolute TDCO - SP
> > 
> > In the mcp15xxfd family manual
> > (http://ww1.microchip.com/downloads/en/DeviceDoc/MCP251XXFD-CAN-FD-Controller-Module-Family-Reference-Manual-20005678B.pdf)
> > in the 2mbit/s data bit rate example in table 3-5 (page 21) it says:
> > 
> > | DTSEG1  15 DTQ
> > | DTSEG2   4 DTQ
> > | TDCO    15 DTQ
> > 
> > The mcp251xfd driver uses 15, the framework calculates 16 (== Sync Seg+
> > tseg1, which is correct), and relative tdco would be 0:
> > 
> > | mcp251xfd_set_bittiming: tdco=15, priv->tdc.tdc=16, relative_tdco=0
> > 
> > Here the output with the patched ip tool:
> 
> Sorry, the previous output was not using the sample points of the
> example in the data sheet, this is the fixed output:
> 
> | 6: mcp251xfd0: <NOARP,UP,LOWER_UP,ECHO> mtu 72 qdisc pfifo_fast state UP mode DEFAULT group default qlen 10
> |     link/can  promiscuity 0 minmtu 0 maxmtu 0 
> |     can <FD,TDC_AUTO> state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 100 
> |           bitrate 500000 sample-point 0.800
> |           tq 25 prop-seg 31 phase-seg1 32 phase-seg2 16 sjw 1 brp 1
> |           mcp251xfd: tseg1 2..256 tseg2 1..128 sjw 1..128 brp 1..256 brp_inc 1
> |           dbitrate 2000000 dsample-point 0.800
> |           dtq 25 dprop-seg 7 dphase-seg1 8 dphase-seg2 4 dsjw 1 dbrp 1
> |           tdco 16
> |           mcp251xfd: dtseg1 1..32 dtseg2 1..16 dsjw 1..16 dbrp 1..256 dbrp_inc 1
> |           tdco 0..127
> |           clock 40000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 parentbus spi parentdev spi0.0 

The following sequence doesn't clear "tdcv" properly:

| ip link set dev mcp251xfd0 down; \
| ip link set mcp251xfd0 txqueuelen 10 up type can \
|     sample-point 0.8 bitrate 500000  \
|     dsample-point 0.8 dbitrate 2000000 fd on \
|     tdc-mode manual tdco 11 tdcv 22
| 
| ip link set dev mcp251xfd0 down; \
| ip link set mcp251xfd0 txqueuelen 10 up type can \
|     sample-point 0.8 bitrate 500000  \
|     dsample-point 0.8 dbitrate 2000000 fd on

| Aug 16 15:10:43 rpi4b8 kernel: mcp251xfd spi0.0 mcp251xfd0: mcp251xfd_set_bittiming: tdco=11 tdcv=22 mode=manual
| Aug 16 15:10:43 rpi4b8 kernel: IPv6: ADDRCONF(NETDEV_CHANGE): mcp251xfd0: link becomes ready
| Aug 16 15:10:59 rpi4b8 kernel: mcp251xfd spi0.0 mcp251xfd0: mcp251xfd_set_bittiming: tdco=16 tdcv=22 mode=automatic
| Aug 16 15:10:59 rpi4b8 kernel: IPv6: ADDRCONF(NETDEV_CHANGE): mcp251xfd0: link becomes ready

neither does "tdc-mode off":

| Aug 16 15:12:18 rpi4b8 kernel: mcp251xfd spi0.0 mcp251xfd0: mcp251xfd_set_bittiming: tdco=11 tdcv=22 mode=off
| Aug 16 15:12:18 rpi4b8 kernel: IPv6: ADDRCONF(NETDEV_CHANGE): mcp251xfd0: link becomes ready

---

We have 4 operations:
- tdc-mode off                  switch off tdc altogether
- tdc-mode manual tdco X tdcv Y configure X and Y for tdco and tdcv
- tdc-mode auto tdco X          configure X tdco and
                                controller measures tdcv automatically
- /* nothing */                 configure default value for tdco
                                controller measures tdcv automatically

The /* nothing */ operation is what the old "ip" tool does, so we're
backwards compatible here (using the old "ip" tool on an updated
kernel/driver).

At first glance it's a bit non-intuitive that you have to specify
nothing for the "full" automatic mode.

Oh, I just noticed:

| ip link set dev mcp251xfd0 down; \
| ip link set mcp251xfd0 txqueuelen 10 up type can \
|     sample-point 0.8 bitrate 500000  \
|     dsample-point 0.8 dbitrate 2000000 fd on \
|     tdc-mode manual tdco 11 tdcv 22

followed by:

| ip link set dev mcp251xfd0 down; \
| ip link set mcp251xfd0 txqueuelen 10 up type can

We stay in manual mode:

| Aug 16 15:27:47 rpi4b8 kernel: mcp251xfd spi0.0 mcp251xfd0: mcp251xfd_set_bittiming: tdco=11 tdcv=22 mode=manual

| 8: mcp251xfd0: <NOARP,UP,LOWER_UP,ECHO> mtu 72 qdisc pfifo_fast state UP mode DEFAULT group default qlen 10
|     link/can  promiscuity 0 minmtu 0 maxmtu 0 
|     can <FD,TDC_AUTO,TDC_MANUAL> state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 100 
|           bitrate 500000 sample-point 0.800
|           tq 25 prop-seg 31 phase-seg1 32 phase-seg2 16 sjw 1 brp 1
|           mcp251xfd: tseg1 2..256 tseg2 1..128 sjw 1..128 brp 1..256 brp_inc 1
|           dbitrate 2000000 dsample-point 0.800
|           dtq 25 dprop-seg 7 dphase-seg1 8 dphase-seg2 4 dsjw 1 dbrp 1
|           tdcv 22 tdco 11
|           mcp251xfd: dtseg1 1..32 dtseg2 1..16 dsjw 1..16 dbrp 1..256 dbrp_inc 1
|           tdcv 0..63 tdco 0..63
|           clock 40000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 parentbus spi parentdev spi0.0 

I have to give "fd on" + the bit timing parameters to go to the full
automatic mode again:

| Aug 16 15:32:46 rpi4b8 kernel: mcp251xfd spi0.0 mcp251xfd0: mcp251xfd_set_bittiming: tdco=16 tdcv=22 mode=automatic


Does it make sense to let "mode auto" without a tdco value switch the
controller into full automatic mode and /* nothing */ not tough the tdc
config at all? If the full automatic mode is power on default mode, then
new kernel/drivers are compatible with old the old ip tool.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Stefan Mätje]

Hi Vincent,

I would like to add some comments below:

Am Montag, den 16.08.2021, 14:25 +0200 schrieb Marc Kleine-Budde:
> On 16.08.2021 19:24:43, Vincent MAILHOL wrote:
> > On Mon. 16 Aug 2021 at 17:42, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > On 15.08.2021 12:32:43, Vincent Mailhol wrote:
> > > > ISO 11898-1 specifies in section 11.3.3 "Transmitter delay
> > > > compensation" that "the configuration range for [the] SSP position
> > > > shall be at least 0 to 63 minimum time quanta."
> > > > 
> > > > Because SSP = TDCV + TDCO, it means that we should allow both TDCV and
> > > > TDCO to hold zero value in order to honor SSP's minimum possible
> > > > value.
> > > > 
> > > > However, current implementation assigned special meaning to TDCV and
> > > > TDCO's zero values:
> > > >   * TDCV = 0 -> TDCV is automatically measured by the transceiver.
> > > >   * TDCO = 0 -> TDC is off.
> > > > 
> > > > In order to allow for those values to really be zero and to maintain
> > > > current features, we introduce two new flags:
> > > >   * CAN_CTRLMODE_TDC_AUTO indicates that the controller support
> > > >     automatic measurement of TDCV.
> > > >   * CAN_CTRLMODE_TDC_MANUAL indicates that the controller support
> > > >     manual configuration of TDCV. N.B.: current implementation failed
> > > >     to provide an option for the driver to indicate that only manual
> > > >     mode was supported.
> > > > 
> > > > TDC is disabled if both CAN_CTRLMODE_TDC_AUTO and
> > > > CAN_CTRLMODE_TDC_MANUAL flags are off, c.f. the helper function
> > > > can_tdc_is_enabled() which is also introduced in this patch.
> > > 
> > > Nitpick: We can only say that TDC is disabled, if the driver supports
> > > the TDC interface at all, which is the case if tdc_const is set.
> > 
> > I would argue that saying that a device does not support TDC is
> > equivalent to saying that TDC is always disabled for that device.
> > Especially, the function can_tdc_is_enabled() can be used even if
> > the device does not support TDC (even if there is no benefit
> > doing so).
> > 
> > Do you still want me to rephrase this part?
> > 
> > > > Also, this patch adds three fields: tdcv_min, tdco_min and tdcf_min to
> > > > struct can_tdc_const. While we are not convinced that those three
> > > > fields could be anything else than zero, we can imagine that some
> > > > controllers might specify a lower bound on these. Thus, those minimums
> > > > are really added "just in case".
> > > 
> > > I'm not sure, if we talked about the mcp251xfd's tcdo, valid values are
> > > -64...63.
> > 
> > Yes! Stefan shed some light on this. The mcp251xfd uses a tdco
> > value which is relative to the sample point.
> 
> I don't read the documentation this way....

@Vincent: I have to agree with Marc here. Perhaps my email
https://lore.kernel.org/linux-can/094d8a2eab2177e5a5143f96cf745b26897e1793.camel@esd.eu/
was also misleading. I also referred there to a MicroChip Excel sheet
(https://ww1.microchip.com/downloads/en/DeviceDoc/MCP2517FD%20Bit%20Time%20Calculations%20-%20UG.xlsx)
that describes the calculation of the bit timing and the TDCO. The values calculated 
there correspond to the SPO from the above email. Microchip calculates the TDCO as
TDCO = (DPRSEG + DPH1SEG) * DBRP.
Thus, as already discussed, negative values are not purposeful.

Sorry, that that email was misleading. So far I've seen now only the ESDACC 
controller has a "relative" TDCO register value where a negative value may
be sensible.

> > > SSP = TDCV + absolute TDCO
> > >     = TDCV + SP + relative TDCO
> > 
> > Consequently:
> > > relative TDCO = absolute TDCO - SP
> 
> In the mcp15xxfd family manual
> (http://ww1.microchip.com/downloads/en/DeviceDoc/MCP251XXFD-CAN-FD-Controller-Module-Family-Reference-Manual-20005678B.pdf)
> in the 2mbit/s data bit rate example in table 3-5 (page 21) it says:
> 
> > DTSEG1  15 DTQ
> > DTSEG2   4 DTQ
> > TDCO    15 DTQ
> 
> The mcp251xfd driver uses 15, the framework calculates 16 (== Sync Seg+
> tseg1, which is correct), and relative tdco would be 0:
> 
> > mcp251xfd_set_bittiming: tdco=15, priv->tdc.tdc=16, relative_tdco=0
> 
> Here the output with the patched ip tool:
> 
> > 4: mcp251xfd0: <NOARP,UP,LOWER_UP,ECHO> mtu 72 qdisc pfifo_fast state UP mode DEFAULT group default qlen 10
> >     link/can  promiscuity 0 minmtu 0 maxmtu 0 
> >     can <FD,TDC_AUTO> state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 100 
> >           bitrate 500000 sample-point 0.875
> >           tq 25 prop-seg 34 phase-seg1 35 phase-seg2 10 sjw 1 brp 1
> >           mcp251xfd: tseg1 2..256 tseg2 1..128 sjw 1..128 brp 1..256 brp_inc 1
> >           dbitrate 2000000 dsample-point 0.750
> >           dtq 25 dprop-seg 7 dphase-seg1 7 dphase-seg2 5 dsjw 1 dbrp 1
> >           tdco 15
> >           mcp251xfd: dtseg1 1..32 dtseg2 1..16 dsjw 1..16 dbrp 1..256 dbrp_inc 1
> >           tdco 0..127
> >           clock 40000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 parentbus spi parentdev spi0.0
> 
> 
> > Which is also why TDCO can be negative.
> > 
> > I added an helper function can_tdc_get_relative_tdco() in the
> > fourth path of this series:
> > https://lore.kernel.org/linux-can/20210814091750.73931-5-mailhol.vincent@wanadoo.fr/T/#u
> > 
> > Devices which use the absolute TDCO can directly use
> > can_priv->tdc.tdco. Devices which use the relative TDCO such as
> > the mcp251xfd should use this helper function instead.
> 
> Don't think so....

@Vincent: Perhaps you should not implement this helper function as it is only needed
for the ESDACC so far.

> > However, you will still need to convert the TDCO valid range from
> > relative values to absolute ones. In your case 0..127.
> 
> Marc
> 

Stefan

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Vincent MAILHOL]

On Mon. 16 Aug 2021 at 21:33, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 16.08.2021 14:25:19, Marc Kleine-Budde wrote:
> > > > I'm not sure, if we talked about the mcp251xfd's tcdo, valid values are
> > > > -64...63.
> > >
> > > Yes! Stefan shed some light on this. The mcp251xfd uses a tdco
> > > value which is relative to the sample point.
> >
> > I don't read the documentation this way....
> >
> > > | SSP = TDCV + absolute TDCO
> > > |     = TDCV + SP + relative TDCO
> > >
> > > Consequently:
> > > | relative TDCO = absolute TDCO - SP
> >
> > In the mcp15xxfd family manual
> > (http://ww1.microchip.com/downloads/en/DeviceDoc/MCP251XXFD-CAN-FD-Controller-Module-Family-Reference-Manual-20005678B.pdf)
> > in the 2mbit/s data bit rate example in table 3-5 (page 21) it says:
> >
> > | DTSEG1  15 DTQ
> > | DTSEG2   4 DTQ
> > | TDCO    15 DTQ
> >
> > The mcp251xfd driver uses 15, the framework calculates 16 (== Sync Seg+
> > tseg1, which is correct), and relative tdco would be 0:
> >
> > | mcp251xfd_set_bittiming: tdco=15, priv->tdc.tdc=16, relative_tdco=0
> >
> > Here the output with the patched ip tool:
>
> Sorry, the previous output was not using the sample points of the
> example in the data sheet, this is the fixed output:
>
> | 6: mcp251xfd0: <NOARP,UP,LOWER_UP,ECHO> mtu 72 qdisc pfifo_fast state UP mode DEFAULT group default qlen 10
> |     link/can  promiscuity 0 minmtu 0 maxmtu 0
> |     can <FD,TDC_AUTO> state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 100
> |           bitrate 500000 sample-point 0.800
> |           tq 25 prop-seg 31 phase-seg1 32 phase-seg2 16 sjw 1 brp 1
> |           mcp251xfd: tseg1 2..256 tseg2 1..128 sjw 1..128 brp 1..256 brp_inc 1
> |           dbitrate 2000000 dsample-point 0.800
> |           dtq 25 dprop-seg 7 dphase-seg1 8 dphase-seg2 4 dsjw 1 dbrp 1
> |           tdco 16
> |           mcp251xfd: dtseg1 1..32 dtseg2 1..16 dsjw 1..16 dbrp 1..256 dbrp_inc 1
> |           tdco 0..127
> |           clock 40000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 parentbus spi parentdev spi0.0

After the discussion I had with Stefan, I assumed mcp251xxfd also
used relative TDCO.  However, in the mcp15xxfd family manual,
Equation 3-10: "Secondary Sample Point" on page 18 states that:

| SSP = TDCV + TDCO

As I commented above, this is the formula of the absolute
TDCO. Furthermore, in the example you shared, TDCO is
16 (absolute), not 0 (relative).

*BUT*, if this is the absolute TDCO, I just do not get how it can
be negative (I already elaborated on this in the past: if you
subtract from TDCV, you are measuring the previous bit...)

Another thing which is misleading to me is that the mcp15xxfd
family manual lists the min and max values for most of the
bittiming parameters but not for TDCO.

Finally, I did a bit of research and found that:
http://ww1.microchip.com/downloads/en/DeviceDoc/Section_56_Controller_Area_Network_with_Flexible_Data_rate_DS60001549A.pdf

This is *not* the mcp25xxfd datasheet but it is still from
Microship and as you will see, it is mostly similar to the
mcp25xxfd except for, you guessed it, the TDCO.

It reads:
| TDCMOD<1:0>: Transmitter Delay Compensation Mode bits
| Secondary Sample Point (SSP).
| 10 = Auto; measure delay and add CFDxDBTCFG.TSEG1; add TDCO
| 11 = Auto; measure delay and add CFDxDBTCFG.TSEG1; add TDCO
| 01 = Manual; Do not measure, use TDCV plus TDCO from the register
| 00 = Disable

| TDCO<6:0>: Transmitter Delay Compensation Offset bits
| Secondary Sample Point (SSP). Two's complement; offset can be
positive, zero, or negative.
| 1111111 = -64 x SYSCLK
| .
| .
| .
| 0111111 = 63 x SYSCLK
| .
| .
| .
| 0000000 = 0 x SYSCLK

Here, you can clearly see that the TDCO has the exact same range
as the one of the mcp25xxfd but the description of TDCMOD
changes, telling us that:

| SSP = TDCV (measured delay) + CFDxDBTCFG.TSEG1 (sample point) + TDCO

Which means this is a relative TDCO.

I just do not get how two documents from Microchip can have the
TDCO relative range of -64..63 but use a different formula. I am
sorry but at that point, I just do not understand what is going
on with your controller...

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2720 bytes --]

On 16.08.2021 23:10:29, Vincent MAILHOL wrote:
[...]
> After the discussion I had with Stefan, I assumed mcp251xxfd also
> used relative TDCO.  However, in the mcp15xxfd family manual,
> Equation 3-10: "Secondary Sample Point" on page 18 states that:
> 
> | SSP = TDCV + TDCO
> 
> As I commented above, this is the formula of the absolute
> TDCO. Furthermore, in the example you shared, TDCO is
> 16 (absolute), not 0 (relative).

ACK

> *BUT*, if this is the absolute TDCO, I just do not get how it can
> be negative (I already elaborated on this in the past: if you
> subtract from TDCV, you are measuring the previous bit...)
> 
> Another thing which is misleading to me is that the mcp15xxfd
> family manual lists the min and max values for most of the
> bittiming parameters but not for TDCO.
> 
> Finally, I did a bit of research and found that:
> http://ww1.microchip.com/downloads/en/DeviceDoc/Section_56_Controller_Area_Network_with_Flexible_Data_rate_DS60001549A.pdf

Interesting. This data sheet is older than the one of the mcp2518fd.

> This is *not* the mcp25xxfd datasheet but it is still from
> Microship and as you will see, it is mostly similar to the
> mcp25xxfd except for, you guessed it, the TDCO.
> 
> It reads:
> | TDCMOD<1:0>: Transmitter Delay Compensation Mode bits
> | Secondary Sample Point (SSP).
> | 10 = Auto; measure delay and add CFDxDBTCFG.TSEG1; add TDCO
> | 11 = Auto; measure delay and add CFDxDBTCFG.TSEG1; add TDCO
> | 01 = Manual; Do not measure, use TDCV plus TDCO from the register
> | 00 = Disable
> 
> | TDCO<6:0>: Transmitter Delay Compensation Offset bits
> | Secondary Sample Point (SSP). Two's complement; offset can be
> positive, zero, or negative.
> | 1111111 = -64 x SYSCLK
> | .
> | .
> | .
> | 0111111 = 63 x SYSCLK
> | .
> | .
> | .
> | 0000000 = 0 x SYSCLK
> 
> Here, you can clearly see that the TDCO has the exact same range
> as the one of the mcp25xxfd but the description of TDCMOD
> changes, telling us that:
> 
> | SSP = TDCV (measured delay) + CFDxDBTCFG.TSEG1 (sample point) + TDCO
> 
> Which means this is a relative TDCO.
> 
> I just do not get how two documents from Microchip can have the
> TDCO relative range of -64..63 but use a different formula. I am
> sorry but at that point, I just do not understand what is going
> on with your controller...

Me neither. I'll ask my microchip contact.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Vincent MAILHOL]

On Mon. 16 Aug 2021 at 22:43, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 16.08.2021 14:33:09, Marc Kleine-Budde wrote:
> > On 16.08.2021 14:25:19, Marc Kleine-Budde wrote:
> > ...
> The following sequence doesn't clear "tdcv" properly:
>
> | ip link set dev mcp251xfd0 down; \
> | ip link set mcp251xfd0 txqueuelen 10 up type can \
> |     sample-point 0.8 bitrate 500000  \
> |     dsample-point 0.8 dbitrate 2000000 fd on \
> |     tdc-mode manual tdco 11 tdcv 22
> |
> | ip link set dev mcp251xfd0 down; \
> | ip link set mcp251xfd0 txqueuelen 10 up type can \
> |     sample-point 0.8 bitrate 500000  \
> |     dsample-point 0.8 dbitrate 2000000 fd on
>
> | Aug 16 15:10:43 rpi4b8 kernel: mcp251xfd spi0.0 mcp251xfd0: mcp251xfd_set_bittiming: tdco=11 tdcv=22 mode=manual
> | Aug 16 15:10:43 rpi4b8 kernel: IPv6: ADDRCONF(NETDEV_CHANGE): mcp251xfd0: link becomes ready
> | Aug 16 15:10:59 rpi4b8 kernel: mcp251xfd spi0.0 mcp251xfd0: mcp251xfd_set_bittiming: tdco=16 tdcv=22 mode=automatic
> | Aug 16 15:10:59 rpi4b8 kernel: IPv6: ADDRCONF(NETDEV_CHANGE): mcp251xfd0: link becomes ready

This is intended. Documentation of can_tdc::tdcv states that:
 *      CAN_CTRLMODE_TDC_AUTO is set: The transceiver dynamically
 *      measures @tdcv for each transmitted CAN FD frame and the
 *      value provided here should be ignored.

And indeed, here you are in automatic mode, so can_tdc::tdcv
should be ignored. And, if you do:

| ip --details link show mcp251xfd0

TDCV should not be reported (unless you implemented the callback
function do_get_auto_tdcv(), in which case, it will report
whatever value was measured by your controller).

> neither does "tdc-mode off":
>
> | Aug 16 15:12:18 rpi4b8 kernel: mcp251xfd spi0.0 mcp251xfd0: mcp251xfd_set_bittiming: tdco=11 tdcv=22 mode=off
> | Aug 16 15:12:18 rpi4b8 kernel: IPv6: ADDRCONF(NETDEV_CHANGE): mcp251xfd0: link becomes ready

Same here. can_tdc doc reads:
 *    N.B. CAN_CTRLMODE_TDC_AUTO and CAN_CTRLMODE_TDC_MANUAL are
 *    mutually exclusive. Only one can be set at a time. If both
 *    CAN_TDC_CTRLMODE_AUTO and CAN_TDC_CTRLMODE_MANUAL are unset,
 *    TDC is disabled and all the values of this structure should be
 *    ignored.

Indeed, both CAN_TDC_CTRLMODE_{AUTO,MANUAL} are unset so all the
fields of can_tdc shall be ignored. You can also confirm that the
netlink interface does not report any tof he TDC parameters.

That said, if you want me to clear the garbage values, I can do
so. I will just add some code with no actual purpose to the
netlink interface.

> ---
>
> We have 4 operations:
> - tdc-mode off                  switch off tdc altogether
> - tdc-mode manual tdco X tdcv Y configure X and Y for tdco and tdcv
> - tdc-mode auto tdco X          configure X tdco and
>                                 controller measures tdcv automatically
> - /* nothing */                 configure default value for tdco
>                                 controller measures tdcv automatically

The "nothing" does one more thing: it decides whether TDC should
be activated or not.

> The /* nothing */ operation is what the old "ip" tool does, so we're
> backwards compatible here (using the old "ip" tool on an updated
> kernel/driver).

That's true but this isn't the real intent. By doing this design,
I wanted the user to be able to transparently use TDC while
continuing to use the exact same ip commands she or he is used
to using.

> At first glance it's a bit non-intuitive that you have to specify
> nothing for the "full" automatic mode.

I have the opposite opinion: at the end, the TDC is no more than
one other bittiming parameter. The current interface requires, at
minimum, to turn "fd" flag on and to specify the dbittiming and
voila. I want to keep the number of required command line to a
minimum. This way, the TL;DR users would still get a working
environment on CAN buses with high bitrates.

For example, when you want the sample-point to be automatically
calculated, you give "nothing". You do not have to
state "sample-point auto" or anything else. I want the same for
the TDC.

> Oh, I just noticed:
>
> | ip link set dev mcp251xfd0 down; \
> | ip link set mcp251xfd0 txqueuelen 10 up type can \
> |     sample-point 0.8 bitrate 500000  \
> |     dsample-point 0.8 dbitrate 2000000 fd on \
> |     tdc-mode manual tdco 11 tdcv 22
>
> followed by:
>
> | ip link set dev mcp251xfd0 down; \
> | ip link set mcp251xfd0 txqueuelen 10 up type can
>
> We stay in manual mode:
>
> | Aug 16 15:27:47 rpi4b8 kernel: mcp251xfd spi0.0 mcp251xfd0: mcp251xfd_set_bittiming: tdco=11 tdcv=22 mode=manual
>
> | 8: mcp251xfd0: <NOARP,UP,LOWER_UP,ECHO> mtu 72 qdisc pfifo_fast state UP mode DEFAULT group default qlen 10
> |     link/can  promiscuity 0 minmtu 0 maxmtu 0
> |     can <FD,TDC_AUTO,TDC_MANUAL> state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 100

That's a bug. It should be impossible to have both TDC_AUTO and
TDC_MANUAL at the same time.

> |           bitrate 500000 sample-point 0.800
> |           tq 25 prop-seg 31 phase-seg1 32 phase-seg2 16 sjw 1 brp 1
> |           mcp251xfd: tseg1 2..256 tseg2 1..128 sjw 1..128 brp 1..256 brp_inc 1
> |           dbitrate 2000000 dsample-point 0.800
> |           dtq 25 dprop-seg 7 dphase-seg1 8 dphase-seg2 4 dsjw 1 dbrp 1
> |           tdcv 22 tdco 11
> |           mcp251xfd: dtseg1 1..32 dtseg2 1..16 dsjw 1..16 dbrp 1..256 dbrp_inc 1
> |           tdcv 0..63 tdco 0..63
> |           clock 40000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 parentbus spi parentdev spi0.0

Sorry, but can I confirm what you did here? You stated that you
did those four commands in this order:

> | ip link set dev mcp251xfd0 down; \
> | ip link set mcp251xfd0 txqueuelen 10 up type can \
> |     sample-point 0.8 bitrate 500000  \
> |     dsample-point 0.8 dbitrate 2000000 fd on \
> |     tdc-mode manual tdco 11 tdcv 22
> | ip link set dev mcp251xfd0 down; \
> | ip link set mcp251xfd0 txqueuelen 10 up type can

So now, you should be in Classical CAN (fd flag off) but the
results of iproute2 shows that FD is on... Is there one missing
step?

> I have to give "fd on" + the bit timing parameters to go to the full
> automatic mode again:
>
> | Aug 16 15:32:46 rpi4b8 kernel: mcp251xfd spi0.0 mcp251xfd0: mcp251xfd_set_bittiming: tdco=16 tdcv=22 mode=automatic
>
>
> Does it make sense to let "mode auto" without a tdco value switch the
> controller into full automatic mode and /* nothing */ not tough the tdc
> config at all? If the full automatic mode is power on default mode, then
> new kernel/drivers are compatible with old the old ip tool.

If we do so, some users will forget to turn it on. Now, this
might not be a big issue, but in the near future, when CAN buses
with high bitrate (4M or more) reach production, you will start
to see complains online of users not understanding why they get
errors on their bus (because the average user will have no clue
of what TDC is). I tried to propose the most dummy proofed
approach (the compatibility with old ip tools is not the goal).

> regards,
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde           |
> Embedded Linux                   | https://www.pengutronix.de  |
> Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Vincent MAILHOL]

On Mon. 16 Aug 2021 at 22:49, Stefan Mätje <Stefan.Maetje@esd.eu> wrote:
> Hi Vincent,
>
> I would like to add some comments below:
>
> Am Montag, den 16.08.2021, 14:25 +0200 schrieb Marc Kleine-Budde:
> > On 16.08.2021 19:24:43, Vincent MAILHOL wrote:
> > > On Mon. 16 Aug 2021 at 17:42, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > > On 15.08.2021 12:32:43, Vincent Mailhol wrote:
> > > > > ISO 11898-1 specifies in section 11.3.3 "Transmitter delay
> > > > > compensation" that "the configuration range for [the] SSP position
> > > > > shall be at least 0 to 63 minimum time quanta."
> > > > >
> > > > > Because SSP = TDCV + TDCO, it means that we should allow both TDCV and
> > > > > TDCO to hold zero value in order to honor SSP's minimum possible
> > > > > value.
> > > > >
> > > > > However, current implementation assigned special meaning to TDCV and
> > > > > TDCO's zero values:
> > > > >   * TDCV = 0 -> TDCV is automatically measured by the transceiver.
> > > > >   * TDCO = 0 -> TDC is off.
> > > > >
> > > > > In order to allow for those values to really be zero and to maintain
> > > > > current features, we introduce two new flags:
> > > > >   * CAN_CTRLMODE_TDC_AUTO indicates that the controller support
> > > > >     automatic measurement of TDCV.
> > > > >   * CAN_CTRLMODE_TDC_MANUAL indicates that the controller support
> > > > >     manual configuration of TDCV. N.B.: current implementation failed
> > > > >     to provide an option for the driver to indicate that only manual
> > > > >     mode was supported.
> > > > >
> > > > > TDC is disabled if both CAN_CTRLMODE_TDC_AUTO and
> > > > > CAN_CTRLMODE_TDC_MANUAL flags are off, c.f. the helper function
> > > > > can_tdc_is_enabled() which is also introduced in this patch.
> > > >
> > > > Nitpick: We can only say that TDC is disabled, if the driver supports
> > > > the TDC interface at all, which is the case if tdc_const is set.
> > >
> > > I would argue that saying that a device does not support TDC is
> > > equivalent to saying that TDC is always disabled for that device.
> > > Especially, the function can_tdc_is_enabled() can be used even if
> > > the device does not support TDC (even if there is no benefit
> > > doing so).
> > >
> > > Do you still want me to rephrase this part?
> > >
> > > > > Also, this patch adds three fields: tdcv_min, tdco_min and tdcf_min to
> > > > > struct can_tdc_const. While we are not convinced that those three
> > > > > fields could be anything else than zero, we can imagine that some
> > > > > controllers might specify a lower bound on these. Thus, those minimums
> > > > > are really added "just in case".
> > > >
> > > > I'm not sure, if we talked about the mcp251xfd's tcdo, valid values are
> > > > -64...63.
> > >
> > > Yes! Stefan shed some light on this. The mcp251xfd uses a tdco
> > > value which is relative to the sample point.
> >
> > I don't read the documentation this way....
>
> @Vincent: I have to agree with Marc here. Perhaps my email
> https://lore.kernel.org/linux-can/094d8a2eab2177e5a5143f96cf745b26897e1793.camel@esd.eu/
> was also misleading. I also referred there to a MicroChip Excel sheet
> (https://ww1.microchip.com/downloads/en/DeviceDoc/MCP2517FD%20Bit%20Time%20Calculations%20-%20UG.xlsx)
> that describes the calculation of the bit timing and the TDCO. The values calculated
> there correspond to the SPO from the above email. Microchip calculates the TDCO as
> TDCO = (DPRSEG + DPH1SEG) * DBRP.
> Thus, as already discussed, negative values are not purposeful.
>
> Sorry, that that email was misleading. So far I've seen now only the ESDACC
> controller has a "relative" TDCO register value where a negative value may
> be sensible.

So many misleading things on this absolute/relative TDCO topic.
But be sure of one thing: your help has been precious!

> > > > SSP = TDCV + absolute TDCO
> > > >     = TDCV + SP + relative TDCO
> > >
> > > Consequently:
> > > > relative TDCO = absolute TDCO - SP
> >
> > In the mcp15xxfd family manual
> > (http://ww1.microchip.com/downloads/en/DeviceDoc/MCP251XXFD-CAN-FD-Controller-Module-Family-Reference-Manual-20005678B.pdf)
> > in the 2mbit/s data bit rate example in table 3-5 (page 21) it says:
> >
> > > DTSEG1  15 DTQ
> > > DTSEG2   4 DTQ
> > > TDCO    15 DTQ
> >
> > The mcp251xfd driver uses 15, the framework calculates 16 (== Sync Seg+
> > tseg1, which is correct), and relative tdco would be 0:
> >
> > > mcp251xfd_set_bittiming: tdco=15, priv->tdc.tdc=16, relative_tdco=0
> >
> > Here the output with the patched ip tool:
> >
> > > 4: mcp251xfd0: <NOARP,UP,LOWER_UP,ECHO> mtu 72 qdisc pfifo_fast state UP mode DEFAULT group default qlen 10
> > >     link/can  promiscuity 0 minmtu 0 maxmtu 0
> > >     can <FD,TDC_AUTO> state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 100
> > >           bitrate 500000 sample-point 0.875
> > >           tq 25 prop-seg 34 phase-seg1 35 phase-seg2 10 sjw 1 brp 1
> > >           mcp251xfd: tseg1 2..256 tseg2 1..128 sjw 1..128 brp 1..256 brp_inc 1
> > >           dbitrate 2000000 dsample-point 0.750
> > >           dtq 25 dprop-seg 7 dphase-seg1 7 dphase-seg2 5 dsjw 1 dbrp 1
> > >           tdco 15
> > >           mcp251xfd: dtseg1 1..32 dtseg2 1..16 dsjw 1..16 dbrp 1..256 dbrp_inc 1
> > >           tdco 0..127
> > >           clock 40000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 parentbus spi parentdev spi0.0
> >
> >
> > > Which is also why TDCO can be negative.
> > >
> > > I added an helper function can_tdc_get_relative_tdco() in the
> > > fourth path of this series:
> > > https://lore.kernel.org/linux-can/20210814091750.73931-5-mailhol.vincent@wanadoo.fr/T/#u
> > >
> > > Devices which use the absolute TDCO can directly use
> > > can_priv->tdc.tdco. Devices which use the relative TDCO such as
> > > the mcp251xfd should use this helper function instead.
> >
> > Don't think so....
>
> @Vincent: Perhaps you should not implement this helper function as it is only needed
> for the ESDACC so far.

Lets first wait for the response from Microchip and if indeed
mcp25xxfd does not use a relative TDCO, I am fine to remove this
from the series. In such a case, do not hesitate to steal the patch
for your ESD driver.


Yours sincerely,
Vincent

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Vincent MAILHOL]

Hi Marc,

I answered too quickly in one paragraph.

On Tue. 17 Aug 2021 at 00:49, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> On Mon. 16 Aug 2021 at 22:43, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
...
> > Oh, I just noticed:
> >
> > | ip link set dev mcp251xfd0 down; \
> > | ip link set mcp251xfd0 txqueuelen 10 up type can \
> > |     sample-point 0.8 bitrate 500000  \
> > |     dsample-point 0.8 dbitrate 2000000 fd on \
> > |     tdc-mode manual tdco 11 tdcv 22
> >
> > followed by:
> >
> > | ip link set dev mcp251xfd0 down; \
> > | ip link set mcp251xfd0 txqueuelen 10 up type can
> >
> > We stay in manual mode:
> >
> > | Aug 16 15:27:47 rpi4b8 kernel: mcp251xfd spi0.0 mcp251xfd0: mcp251xfd_set_bittiming: tdco=11 tdcv=22 mode=manual
> >
> > | 8: mcp251xfd0: <NOARP,UP,LOWER_UP,ECHO> mtu 72 qdisc pfifo_fast state UP mode DEFAULT group default qlen 10
> > |     link/can  promiscuity 0 minmtu 0 maxmtu 0
> > |     can <FD,TDC_AUTO,TDC_MANUAL> state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 100
>
> That's a bug. It should be impossible to have both TDC_AUTO and
> TDC_MANUAL at the same time.
>
> > |           bitrate 500000 sample-point 0.800
> > |           tq 25 prop-seg 31 phase-seg1 32 phase-seg2 16 sjw 1 brp 1
> > |           mcp251xfd: tseg1 2..256 tseg2 1..128 sjw 1..128 brp 1..256 brp_inc 1
> > |           dbitrate 2000000 dsample-point 0.800
> > |           dtq 25 dprop-seg 7 dphase-seg1 8 dphase-seg2 4 dsjw 1 dbrp 1
> > |           tdcv 22 tdco 11
> > |           mcp251xfd: dtseg1 1..32 dtseg2 1..16 dsjw 1..16 dbrp 1..256 dbrp_inc 1
> > |           tdcv 0..63 tdco 0..63
> > |           clock 40000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535 parentbus spi parentdev spi0.0
>
> Sorry, but can I confirm what you did here? You stated that you
> did those four commands in this order:
>
> > | ip link set dev mcp251xfd0 down; \
> > | ip link set mcp251xfd0 txqueuelen 10 up type can \
> > |     sample-point 0.8 bitrate 500000  \
> > |     dsample-point 0.8 dbitrate 2000000 fd on \
> > |     tdc-mode manual tdco 11 tdcv 22
> > | ip link set dev mcp251xfd0 down; \
> > | ip link set mcp251xfd0 txqueuelen 10 up type can
>
> So now, you should be in Classical CAN (fd flag off) but the
> results of iproute2 shows that FD is on... Is there one missing
> step?

Please ignore this part. I misread the latest command and thought
you were configuring it as classical CAN. You just did a network
down / up.

I will troubleshoot this tomorrow.

> > I have to give "fd on" + the bit timing parameters to go to the full
> > automatic mode again:
> >
> > | Aug 16 15:32:46 rpi4b8 kernel: mcp251xfd spi0.0 mcp251xfd0: mcp251xfd_set_bittiming: tdco=16 tdcv=22 mode=automatic

Yours sincerely,
Vincent

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Vincent MAILHOL]

Hi Marc,

This patch fixes the bug you just encountered: having both TDC_AUTO
and TDC_MANUAL set at the same time. I also cleaned all garbage
data in struct can_tdc because that was trivial.

This patch is meant to be squashed into:
commit ca7200319a90 ("can: netlink: add interface for CAN-FD
Transmitter Delay Compensation (TDC)")

For now, I am just sharing it here so that you can continue your
testing. I will resend the full series after we finish current
ongoing discussion.

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/net/can/dev/netlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index f05745c96b9c..d8cefe7d354c 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -204,6 +204,7 @@ static int can_changelink(struct net_device *dev,
struct nlattr *tb[],
         }
     }

+    priv->ctrlmode &= ~CAN_CTRLMODE_TDC_MASK;
     if (data[IFLA_CAN_CTRLMODE]) {
         struct can_ctrlmode *cm;
         u32 ctrlstatic;
@@ -239,8 +240,6 @@ static int can_changelink(struct net_device *dev,
struct nlattr *tb[],
             dev->mtu = CAN_MTU;
             memset(&priv->data_bittiming, 0,
                    sizeof(priv->data_bittiming));
-            memset(&priv->tdc, 0, sizeof(priv->tdc));
-            priv->ctrlmode &= ~CAN_CTRLMODE_TDC_MASK;
         }

         tdc_mask = cm->mask & CAN_CTRLMODE_TDC_MASK;
@@ -326,6 +325,7 @@ static int can_changelink(struct net_device *dev,
struct nlattr *tb[],
         priv->termination = termval;
     }

+    memset(&priv->tdc, 0, sizeof(priv->tdc));
     if (data[IFLA_CAN_TDC]) {
         /* Use the provided TDC parameters */
         err = can_tdc_changelink(dev, data[IFLA_CAN_TDC], extack);
-- 
2.31.1

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1709 bytes --]

On 16.08.2021 16:30:52, Marc Kleine-Budde wrote:
> > Finally, I did a bit of research and found that:
> > http://ww1.microchip.com/downloads/en/DeviceDoc/Section_56_Controller_Area_Network_with_Flexible_Data_rate_DS60001549A.pdf

> > This is *not* the mcp25xxfd datasheet but it is still from
> > Microship and as you will see, it is mostly similar to the
> > mcp25xxfd except for, you guessed it, the TDCO.
> > 
> > It reads:
> > | TDCMOD<1:0>: Transmitter Delay Compensation Mode bits
> > | Secondary Sample Point (SSP).
> > | 10 = Auto; measure delay and add CFDxDBTCFG.TSEG1; add TDCO
> > | 11 = Auto; measure delay and add CFDxDBTCFG.TSEG1; add TDCO
> > | 01 = Manual; Do not measure, use TDCV plus TDCO from the register
> > | 00 = Disable
> > 
> > | TDCO<6:0>: Transmitter Delay Compensation Offset bits
> > | Secondary Sample Point (SSP). Two's complement; offset can be
> > positive, zero, or negative.
> > | 1111111 = -64 x SYSCLK
> > | .
> > | .
> > | .
> > | 0111111 = 63 x SYSCLK
> > | .
> > | .
> > | .
> > | 0000000 = 0 x SYSCLK
> > 
> > Here, you can clearly see that the TDCO has the exact same range
> > as the one of the mcp25xxfd but the description of TDCMOD
> > changes, telling us that:
> > 
> > | SSP = TDCV (measured delay) + CFDxDBTCFG.TSEG1 (sample point) + TDCO
> > 
> > Which means this is a relative TDCO.

Good catch! Microchip is investigating this.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 5/7] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC)

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2039 bytes --]

On 15.08.2021 12:32:46, Vincent Mailhol wrote:
> +static int can_tdc_changelink(struct net_device *dev, const struct nlattr *nla,
> +			      struct netlink_ext_ack *extack)
> +{
> +	struct nlattr *tb_tdc[IFLA_CAN_TDC_MAX + 1];
> +	struct can_priv *priv = netdev_priv(dev);
> +	struct can_tdc *tdc = &priv->tdc;
> +	const struct can_tdc_const *tdc_const = priv->tdc_const;
> +	int err;
> +
> +	if (!tdc_const || !can_tdc_is_enabled(priv))
> +		return -EOPNOTSUPP;
> +
> +	if (dev->flags & IFF_UP)
> +		return -EBUSY;
> +
> +	err = nla_parse_nested(tb_tdc, IFLA_CAN_TDC_MAX, nla,
> +			       can_tdc_policy, extack);
> +	if (err)
> +		return err;
> +
> +	if (tb_tdc[IFLA_CAN_TDC_TDCV]) {
> +		u32 tdcv = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCV]);
> +
> +		if (tdcv < tdc_const->tdcv_min || tdcv > tdc_const->tdcv_max)
> +			return -EINVAL;
> +
> +		tdc->tdcv = tdcv;

You have to assign to a temporary struct first, and set the priv->tdc
after complete validation, otherwise you end up with inconsistent
values.

> +	}
> +
> +	if (tb_tdc[IFLA_CAN_TDC_TDCO]) {
> +		u32 tdco = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCO]);
> +
> +		if (tdco < tdc_const->tdco_min || tdco > tdc_const->tdco_max)
> +			return -EINVAL;
> +
> +		tdc->tdco = tdco;
> +	}
> +
> +	if (tb_tdc[IFLA_CAN_TDC_TDCF]) {
> +		u32 tdcf = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCF]);
> +
> +		if (tdcf < tdc_const->tdcf_min || tdcf > tdc_const->tdcf_max)
> +			return -EINVAL;
> +
> +		tdc->tdcf = tdcf;
> +	}
> +
> +	return 0;
> +}

To reproduce (ip pseudo-code only :D ):

ip down
ip up tdc-mode manual tdco 111 tdcv 33  # 111 is out of range, 33 is valid
ip down
ip up                                   # results in tdco=0 tdcv=33 mode=manual

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1470 bytes --]

On 17.08.2021 00:49:35, Vincent MAILHOL wrote:
> > We have 4 operations:
> > - tdc-mode off                  switch off tdc altogether
> > - tdc-mode manual tdco X tdcv Y configure X and Y for tdco and tdcv
> > - tdc-mode auto tdco X          configure X tdco and
> >                                 controller measures tdcv automatically
> > - /* nothing */                 configure default value for tdco
> >                                 controller measures tdcv automatically
> 
> The "nothing" does one more thing: it decides whether TDC should
> be activated or not.
> 
> > The /* nothing */ operation is what the old "ip" tool does, so we're
> > backwards compatible here (using the old "ip" tool on an updated
> > kernel/driver).
> 
> That's true but this isn't the real intent. By doing this design,
> I wanted the user to be able to transparently use TDC while
> continuing to use the exact same ip commands she or he is used
> to using.

Backwards compatibility using an old ip tool on a new kernel/driver must
work. In case of the mcp251xfd the tdc mode must be activated and tdcv
set to the automatic calculated value and tdco automatically measured.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 5/7] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC)

[Vincent MAILHOL]

On Wed 18 Aug 2021 at 04:55, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 15.08.2021 12:32:46, Vincent Mailhol wrote:
> > +static int can_tdc_changelink(struct net_device *dev, const struct nlattr *nla,
> > +                           struct netlink_ext_ack *extack)
> > +{
> > +     struct nlattr *tb_tdc[IFLA_CAN_TDC_MAX + 1];
> > +     struct can_priv *priv = netdev_priv(dev);
> > +     struct can_tdc *tdc = &priv->tdc;
> > +     const struct can_tdc_const *tdc_const = priv->tdc_const;
> > +     int err;
> > +
> > +     if (!tdc_const || !can_tdc_is_enabled(priv))
> > +             return -EOPNOTSUPP;
> > +
> > +     if (dev->flags & IFF_UP)
> > +             return -EBUSY;
> > +
> > +     err = nla_parse_nested(tb_tdc, IFLA_CAN_TDC_MAX, nla,
> > +                            can_tdc_policy, extack);
> > +     if (err)
> > +             return err;
> > +
> > +     if (tb_tdc[IFLA_CAN_TDC_TDCV]) {
> > +             u32 tdcv = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCV]);
> > +
> > +             if (tdcv < tdc_const->tdcv_min || tdcv > tdc_const->tdcv_max)
> > +                     return -EINVAL;
> > +
> > +             tdc->tdcv = tdcv;
>
> You have to assign to a temporary struct first, and set the priv->tdc
> after complete validation, otherwise you end up with inconsistent
> values.

Actually, copying the temporary structure to priv->tdc is not an
atomic operation. Here, you are only reducing the window, not
closing it.

> > +     }
> > +
> > +     if (tb_tdc[IFLA_CAN_TDC_TDCO]) {
> > +             u32 tdco = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCO]);
> > +
> > +             if (tdco < tdc_const->tdco_min || tdco > tdc_const->tdco_max)
> > +                     return -EINVAL;
> > +
> > +             tdc->tdco = tdco;
> > +     }
> > +
> > +     if (tb_tdc[IFLA_CAN_TDC_TDCF]) {
> > +             u32 tdcf = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCF]);
> > +
> > +             if (tdcf < tdc_const->tdcf_min || tdcf > tdc_const->tdcf_max)
> > +                     return -EINVAL;
> > +
> > +             tdc->tdcf = tdcf;
> > +     }
> > +
> > +     return 0;
> > +}
>
> To reproduce (ip pseudo-code only :D ):
>
> ip down
> ip up tdc-mode manual tdco 111 tdcv 33  # 111 is out of range, 33 is valid
> ip down
> ip up                                   # results in tdco=0 tdcv=33 mode=manual

I do not think that this PoC would work because, thankfully, the
netlink interface uses a mutex to prevent this issue from
occurring.

That mutex is defined in:
https://elixir.bootlin.com/linux/latest/source/net/core/rtnetlink.c#L68

Each time a netlink message is sent to the kernel, it would be
dispatched by rtnetlink_rcv_msg() which will make sure to lock
the mutex before doing so:
https://elixir.bootlin.com/linux/latest/source/net/core/rtnetlink.c#L5551

A funny note is that because the mutex is global, if you run two
ip command in a row:

| ip link set can0 type can bitrate 500000
| ip link set can1 up

the second one will wait for the first one to finish even if it
is on a different network device.

To conclude, I do not think this needs to be fixed.


Yours sincerely,
Vincent

Re: [PATCH v5 5/7] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC)

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4180 bytes --]

On 18.08.2021 17:08:51, Vincent MAILHOL wrote:
> On Wed 18 Aug 2021 at 04:55, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 15.08.2021 12:32:46, Vincent Mailhol wrote:
> > > +static int can_tdc_changelink(struct net_device *dev, const struct nlattr *nla,
> > > +                           struct netlink_ext_ack *extack)
> > > +{
> > > +     struct nlattr *tb_tdc[IFLA_CAN_TDC_MAX + 1];
> > > +     struct can_priv *priv = netdev_priv(dev);
> > > +     struct can_tdc *tdc = &priv->tdc;
> > > +     const struct can_tdc_const *tdc_const = priv->tdc_const;
> > > +     int err;
> > > +
> > > +     if (!tdc_const || !can_tdc_is_enabled(priv))
> > > +             return -EOPNOTSUPP;
> > > +
> > > +     if (dev->flags & IFF_UP)
> > > +             return -EBUSY;
> > > +
> > > +     err = nla_parse_nested(tb_tdc, IFLA_CAN_TDC_MAX, nla,
> > > +                            can_tdc_policy, extack);
> > > +     if (err)
> > > +             return err;
> > > +
> > > +     if (tb_tdc[IFLA_CAN_TDC_TDCV]) {
> > > +             u32 tdcv = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCV]);
> > > +
> > > +             if (tdcv < tdc_const->tdcv_min || tdcv > tdc_const->tdcv_max)
> > > +                     return -EINVAL;
> > > +
> > > +             tdc->tdcv = tdcv;
> >
> > You have to assign to a temporary struct first, and set the priv->tdc
> > after complete validation, otherwise you end up with inconsistent
> > values.
> 
> Actually, copying the temporary structure to priv->tdc is not an
> atomic operation. Here, you are only reducing the window, not
> closing it.

It's not a race I'm fixing.

> 
> > > +     }
> > > +
> > > +     if (tb_tdc[IFLA_CAN_TDC_TDCO]) {
> > > +             u32 tdco = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCO]);
> > > +
> > > +             if (tdco < tdc_const->tdco_min || tdco > tdc_const->tdco_max)
> > > +                     return -EINVAL;
> > > +
> > > +             tdc->tdco = tdco;
> > > +     }
> > > +
> > > +     if (tb_tdc[IFLA_CAN_TDC_TDCF]) {
> > > +             u32 tdcf = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCF]);
> > > +
> > > +             if (tdcf < tdc_const->tdcf_min || tdcf > tdc_const->tdcf_max)
> > > +                     return -EINVAL;
> > > +
> > > +             tdc->tdcf = tdcf;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> >
> > To reproduce (ip pseudo-code only :D ):
> >
> > ip down
> > ip up tdc-mode manual tdco 111 tdcv 33  # 111 is out of range, 33 is valid
> > ip down
> > ip up                                   # results in tdco=0 tdcv=33 mode=manual
> 
> I do not think that this PoC would work because, thankfully, the
> netlink interface uses a mutex to prevent this issue from
> occurring.

It works, I've tested it :)

> That mutex is defined in:
> https://elixir.bootlin.com/linux/latest/source/net/core/rtnetlink.c#L68
> 
> Each time a netlink message is sent to the kernel, it would be
> dispatched by rtnetlink_rcv_msg() which will make sure to lock
> the mutex before doing so:
> https://elixir.bootlin.com/linux/latest/source/net/core/rtnetlink.c#L5551
> 
> A funny note is that because the mutex is global, if you run two
> ip command in a row:
> 
> | ip link set can0 type can bitrate 500000
> | ip link set can1 up
> 
> the second one will wait for the first one to finish even if it
> is on a different network device.
> 
> To conclude, I do not think this needs to be fixed.

It's not a race. Consider this command:

| ip up tdc-mode manual tdco 111 tdcv 33  # 111 is out of range, 33 is valid

tdcv is checked first and valid, then it's assigned to the priv->tdc.
tdco is checked second and invalid, then can_tdc_changelink() returns -EINVAL.

tdc ends up being half set :(

So the setting of tdc is inconsistent and when you do a "ip down" "ip
up" then it results in a tdco=0 tdcv=33 mode=manual.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 5/7] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC)

[Vincent MAILHOL]

On Wed. 18 Aug 2021 à 17:19, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 18.08.2021 17:08:51, Vincent MAILHOL wrote:
> > On Wed 18 Aug 2021 at 04:55, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > On 15.08.2021 12:32:46, Vincent Mailhol wrote:
> > > > +static int can_tdc_changelink(struct net_device *dev, const struct nlattr *nla,
> > > > +                           struct netlink_ext_ack *extack)
> > > > +{
> > > > +     struct nlattr *tb_tdc[IFLA_CAN_TDC_MAX + 1];
> > > > +     struct can_priv *priv = netdev_priv(dev);
> > > > +     struct can_tdc *tdc = &priv->tdc;
> > > > +     const struct can_tdc_const *tdc_const = priv->tdc_const;
> > > > +     int err;
> > > > +
> > > > +     if (!tdc_const || !can_tdc_is_enabled(priv))
> > > > +             return -EOPNOTSUPP;
> > > > +
> > > > +     if (dev->flags & IFF_UP)
> > > > +             return -EBUSY;
> > > > +
> > > > +     err = nla_parse_nested(tb_tdc, IFLA_CAN_TDC_MAX, nla,
> > > > +                            can_tdc_policy, extack);
> > > > +     if (err)
> > > > +             return err;
> > > > +
> > > > +     if (tb_tdc[IFLA_CAN_TDC_TDCV]) {
> > > > +             u32 tdcv = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCV]);
> > > > +
> > > > +             if (tdcv < tdc_const->tdcv_min || tdcv > tdc_const->tdcv_max)
> > > > +                     return -EINVAL;
> > > > +
> > > > +             tdc->tdcv = tdcv;
> > >
> > > You have to assign to a temporary struct first, and set the priv->tdc
> > > after complete validation, otherwise you end up with inconsistent
> > > values.
> >
> > Actually, copying the temporary structure to priv->tdc is not an
> > atomic operation. Here, you are only reducing the window, not
> > closing it.
>
> It's not a race I'm fixing.
>
> >
> > > > +     }
> > > > +
> > > > +     if (tb_tdc[IFLA_CAN_TDC_TDCO]) {
> > > > +             u32 tdco = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCO]);
> > > > +
> > > > +             if (tdco < tdc_const->tdco_min || tdco > tdc_const->tdco_max)
> > > > +                     return -EINVAL;
> > > > +
> > > > +             tdc->tdco = tdco;
> > > > +     }
> > > > +
> > > > +     if (tb_tdc[IFLA_CAN_TDC_TDCF]) {
> > > > +             u32 tdcf = nla_get_u32(tb_tdc[IFLA_CAN_TDC_TDCF]);
> > > > +
> > > > +             if (tdcf < tdc_const->tdcf_min || tdcf > tdc_const->tdcf_max)
> > > > +                     return -EINVAL;
> > > > +
> > > > +             tdc->tdcf = tdcf;
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > >
> > > To reproduce (ip pseudo-code only :D ):
> > >
> > > ip down
> > > ip up tdc-mode manual tdco 111 tdcv 33  # 111 is out of range, 33 is valid
> > > ip down
> > > ip up                                   # results in tdco=0 tdcv=33 mode=manual
> >
> > I do not think that this PoC would work because, thankfully, the
> > netlink interface uses a mutex to prevent this issue from
> > occurring.
>
> It works, I've tested it :)
>
> > That mutex is defined in:
> > https://elixir.bootlin.com/linux/latest/source/net/core/rtnetlink.c#L68
> >
> > Each time a netlink message is sent to the kernel, it would be
> > dispatched by rtnetlink_rcv_msg() which will make sure to lock
> > the mutex before doing so:
> > https://elixir.bootlin.com/linux/latest/source/net/core/rtnetlink.c#L5551
> >
> > A funny note is that because the mutex is global, if you run two
> > ip command in a row:
> >
> > | ip link set can0 type can bitrate 500000
> > | ip link set can1 up
> >
> > the second one will wait for the first one to finish even if it
> > is on a different network device.
> >
> > To conclude, I do not think this needs to be fixed.
>
> It's not a race. Consider this command:
>
> | ip up tdc-mode manual tdco 111 tdcv 33  # 111 is out of range, 33 is valid
>
> tdcv is checked first and valid, then it's assigned to the priv->tdc.
> tdco is checked second and invalid, then can_tdc_changelink() returns -EINVAL.
>
> tdc ends up being half set :(
>
> So the setting of tdc is inconsistent and when you do a "ip down" "ip
> up" then it results in a tdco=0 tdcv=33 mode=manual.

My bad. Now I understand the issue.
I was confused because tdco=111 is in the valid range of my driver...
I will squash your patch.

Actually, I think that there is one more thing which needs to be
fixed: If can_tdc_changelink() fails (e.g. value out of range),
the CAN_CTRLMODE_TDC_AUTO or CAN_CTRLMODE_TDC_MANUAL would still
be set, meaning that can_tdc_is_enabled() would return true. So I
will add a "fail" branch to clear the flags.


Yours sincerely,
Vincent

Re: [PATCH v5 5/7] can: netlink: add interface for CAN-FD Transmitter Delay Compensation (TDC)

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1294 bytes --]

On 18.08.2021 17:37:17, Vincent MAILHOL wrote:
> > It's not a race. Consider this command:
> >
> > | ip up tdc-mode manual tdco 111 tdcv 33  # 111 is out of range, 33 is valid
> >
> > tdcv is checked first and valid, then it's assigned to the priv->tdc.
> > tdco is checked second and invalid, then can_tdc_changelink() returns -EINVAL.
> >
> > tdc ends up being half set :(
> >
> > So the setting of tdc is inconsistent and when you do a "ip down" "ip
> > up" then it results in a tdco=0 tdcv=33 mode=manual.
> 
> My bad. Now I understand the issue.
> I was confused because tdco=111 is in the valid range of my driver...

:D

> I will squash your patch.
> 
> Actually, I think that there is one more thing which needs to be
> fixed: If can_tdc_changelink() fails (e.g. value out of range),
> the CAN_CTRLMODE_TDC_AUTO or CAN_CTRLMODE_TDC_MANUAL would still
> be set, meaning that can_tdc_is_enabled() would return true. So I
> will add a "fail" branch to clear the flags.

Ok.

regard,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Vincent MAILHOL]

On Wed. 18 Aug 2021 at 05:01, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 17.08.2021 00:49:35, Vincent MAILHOL wrote:
> > > We have 4 operations:
> > > - tdc-mode off                  switch off tdc altogether
> > > - tdc-mode manual tdco X tdcv Y configure X and Y for tdco and tdcv
> > > - tdc-mode auto tdco X          configure X tdco and
> > >                                 controller measures tdcv automatically
> > > - /* nothing */                 configure default value for tdco
> > >                                 controller measures tdcv automatically
> >
> > The "nothing" does one more thing: it decides whether TDC should
> > be activated or not.
> >
> > > The /* nothing */ operation is what the old "ip" tool does, so we're
> > > backwards compatible here (using the old "ip" tool on an updated
> > > kernel/driver).
> >
> > That's true but this isn't the real intent. By doing this design,
> > I wanted the user to be able to transparently use TDC while
> > continuing to use the exact same ip commands she or he is used
> > to using.
>
> Backwards compatibility using an old ip tool on a new kernel/driver must
> work.

I am not trying to argue against backward compatibility :)
My comment was just to point out that I had other intents as well.

> In case of the mcp251xfd the tdc mode must be activated and tdcv
> set to the automatic calculated value and tdco automatically measured.

Sorry but I am not sure if I will follow you. Here, do you mean
that "nothing" should do the "fully automated" calculation?

In your previous message, you said:

> Does it make sense to let "mode auto" without a tdco value switch the
> controller into full automatic mode and /* nothing */ not tough the tdc
> config at all?

So, you would like this behavior:

| "nothing" -> TDC is off (not touch the tdc config at all)
| mode auto, no tdco provided -> kernel decides between TDC_AUTO and TDC off.
| mode auto, tdco provided -> TDC_AUTO
| mode manual, tdcv and tdco provided -> TDC_MANUAL
| mode off is not needed anymore (redundant with "nothing")
(TDCF left out of the picture intentionally)

Correct?

If you do so, I see three issues:

1/ Some of the drivers already implement TDC. Those will
automatically do a calculation as long as FD is on. If "nothing"
now brings TDC off, some users will find themselves with some
error on the bus after the iproute2 update if they continue using
the same command.

2/ Users will need to read and understand how to use the TDC
parameters of iproute2. And by experience, too many people just
don't read the doc. If I can make the interface transparent and
do the correct thing by default ("nothing"), I prefer to do so.

3/ Final one is more of a nitpick. The mode auto might result in
TDC being off. If we have a TDC_AUTO flag, I would expect the
auto mode to always set that flag (unless error occurs). I see
this to be slightly counter intuitive (I recognize that my
solution also has some aspects which are not intuitive, I just
want to point here that none are perfect).


To be honest, I really preferred the v1 of this series where
there were no tdc-mode {auto,manual,off} and where the "off"
behavior was controlled by setting TDCO to zero. However, as we
realized, zero is a valid value and thus, I had to add all this
complexity just to allow that damn zero value.


Yours sincerely,
Vincent

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3766 bytes --]

On 18.08.2021 18:22:33, Vincent MAILHOL wrote:
> > Backwards compatibility using an old ip tool on a new kernel/driver must
> > work.
> 
> I am not trying to argue against backward compatibility :)
> My comment was just to point out that I had other intents as well.
> 
> > In case of the mcp251xfd the tdc mode must be activated and tdcv
> > set to the automatic calculated value and tdco automatically measured.
> 
> Sorry but I am not sure if I will follow you. Here, do you mean
> that "nothing" should do the "fully automated" calculation?

Sort of.
The use case is the old ip tool with a driver that supports tdc, for
CAN-FD to work it must be configured in fully automated mode.

> In your previous message, you said:
> 
> > Does it make sense to let "mode auto" without a tdco value switch the
> > controller into full automatic mode and /* nothing */ not tough the tdc
> > config at all?
> 
> So, you would like this behavior:
> 
> | mode auto, no tdco provided -> kernel decides between TDC_AUTO and TDC off.

NACK - mode auto, no tdco -> TDC_AUTO with tdco calculated by the kernel

> | mode auto, tdco provided -> TDC_AUTO

ACK - TDC_AUTO with user supplied tdco

> | mode manual, tdcv and tdco provided -> TDC_MANUAL

ACK - TDC_MANUAL with tdco and tdcv user provided

> | mode off is not needed anymore (redundant with "nothing")
> (TDCF left out of the picture intentionally)

NACK - TDC is switched off

> | "nothing" -> TDC is off (not touch the tdc config at all)

NACK - do not touch TDC setting, use previous setting

> Correct?

See above. Plus a change that addresses your issue 1/ from below.

If driver supports TDC it should be initially brought into TDC auto
mode, if no TDC mode is given. Maybe we need an explizit TDC off to make
that work.

> If you do so, I see three issues:
> 
> 1/ Some of the drivers already implement TDC. Those will
> automatically do a calculation as long as FD is on. If "nothing"
> now brings TDC off, some users will find themselves with some
> error on the bus after the iproute2 update if they continue using
> the same command.

Nothing would mean "do not touch" and as TDC auto is default a new ip
would work out of the box. Old ip will work, too. Just failing to decode
TDC_AUTO...

> 2/ Users will need to read and understand how to use the TDC
> parameters of iproute2. And by experience, too many people just
> don't read the doc. If I can make the interface transparent and
> do the correct thing by default ("nothing"), I prefer to do so.

ACK, see above

> 3/ Final one is more of a nitpick. The mode auto might result in
> TDC being off. If we have a TDC_AUTO flag, I would expect the
> auto mode to always set that flag (unless error occurs). I see
> this to be slightly counter intuitive (I recognize that my
> solution also has some aspects which are not intuitive, I just
> want to point here that none are perfect).

What are the corner cases where TDC_AUTO results in TDC off?

> To be honest, I really preferred the v1 of this series where
> there were no tdc-mode {auto,manual,off} and where the "off"
> behavior was controlled by setting TDCO to zero. However, as we
> realized, zero is a valid value and thus, I had to add all this
> complexity just to allow that damn zero value.

Maybe we should not put the TDC mode _not_ into ctrl-mode, but info a
dedicated tdc-mode (which is not bit-field) inside the struct tdc?

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Vincent MAILHOL]

On Wed. 18 Aug 2021 at 21:29, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 18.08.2021 18:22:33, Vincent MAILHOL wrote:
> > > Backwards compatibility using an old ip tool on a new kernel/driver must
> > > work.
> >
> > I am not trying to argue against backward compatibility :)
> > My comment was just to point out that I had other intents as well.
> >
> > > In case of the mcp251xfd the tdc mode must be activated and tdcv
> > > set to the automatic calculated value and tdco automatically measured.
> >
> > Sorry but I am not sure if I will follow you. Here, do you mean
> > that "nothing" should do the "fully automated" calculation?
>
> Sort of.
> The use case is the old ip tool with a driver that supports tdc, for
> CAN-FD to work it must be configured in fully automated mode.

The current patch does that: "nothing" means that both TDC_AUTO
and TDC_MANUAL are not set, same as what an old ip tool would
do. And that triggers the default (fully automated) mode (call
can_calc_tdco()).

> > In your previous message, you said:
> >
> > > Does it make sense to let "mode auto" without a tdco value switch the
> > > controller into full automatic mode and /* nothing */ not tough the tdc
> > > config at all?
> >
> > So, you would like this behavior:
> >
> > | mode auto, no tdco provided -> kernel decides between TDC_AUTO and TDC off.
>
> NACK - mode auto, no tdco -> TDC_AUTO with tdco calculated by the kernel

Currently, the tdco calculation is paired with the decision to
enable or not TDC. If dbrp is one or two, then do the tdco
calculation, else, TDC is off (c.f. can_calc_tdco()). This
behaviour is to follow ISO 11898-1 which states that TDC is only
applicable if data BRP is one or two. In the current patch I
allow to have TDC enabled with a dbrp greater than 2 only if the
tdco is provided by the user (i.e. I allow the user to forcefully
go against ISO but the automatic calculation guarantees
compliance).

So what do you suggest to do when drbp is greater than 2? Still
enable TDC (and violate ISO standard) or return an error
code (e.g. -ENOTSUPP)?

> > | mode auto, tdco provided -> TDC_AUTO
>
> ACK - TDC_AUTO with user supplied tdco
>
> > | mode manual, tdcv and tdco provided -> TDC_MANUAL
>
> ACK - TDC_MANUAL with tdco and tdcv user provided
>
> > | mode off is not needed anymore (redundant with "nothing")
> > (TDCF left out of the picture intentionally)
>
> NACK - TDC is switched off

Same as the current patch then :)

> > | "nothing" -> TDC is off (not touch the tdc config at all)
>
> NACK - do not touch TDC setting, use previous setting

Sorry but I still fail to understand your definition of "do not
touch".

The first time you start a device, all the structures are zeroed
out meaning that TDC is off to begin with.  So the first time the
user do something like:

| ip link set can0 type can bitrate 1000000 dbitrate 8000000 fd on

If you "do not touch" TDC it means that all TDC values stays at
zero, i.e. TDC stays off. This would contradict point 1/.

> > Correct?
>
> See above. Plus a change that addresses your issue 1/ from below.
>
> If driver supports TDC it should be initially brought into TDC auto
> mode, if no TDC mode is given. Maybe we need an explizit TDC off to make
> that work.
>
> > If you do so, I see three issues:
> >
> > 1/ Some of the drivers already implement TDC. Those will
> > automatically do a calculation as long as FD is on. If "nothing"
> > now brings TDC off, some users will find themselves with some
> > error on the bus after the iproute2 update if they continue using
> > the same command.
>
> Nothing would mean "do not touch" and as TDC auto is default a new ip
> would work out of the box. Old ip will work, too. Just failing to decode
> TDC_AUTO...

See above: if you "do not touch", my understanding is that the
old ip tool will indefinitely keep TDC to its initial value:
everything zeroed out.

To turn TDC auto, you will eventually call can_calc_tdco() and
that will touch something.

> > 2/ Users will need to read and understand how to use the TDC
> > parameters of iproute2. And by experience, too many people just
> > don't read the doc. If I can make the interface transparent and
> > do the correct thing by default ("nothing"), I prefer to do so.
>
> ACK, see above
>
> > 3/ Final one is more of a nitpick. The mode auto might result in
> > TDC being off. If we have a TDC_AUTO flag, I would expect the
> > auto mode to always set that flag (unless error occurs). I see
> > this to be slightly counter intuitive (I recognize that my
> > solution also has some aspects which are not intuitive, I just
> > want to point here that none are perfect).
>
> What are the corner cases where TDC_AUTO results in TDC off?

dbrp greater than 2 (see above).

> > To be honest, I really preferred the v1 of this series where
> > there were no tdc-mode {auto,manual,off} and where the "off"
> > behavior was controlled by setting TDCO to zero. However, as we
> > realized, zero is a valid value and thus, I had to add all this
> > complexity just to allow that damn zero value.
>
> Maybe we should not put the TDC mode _not_ into ctrl-mode, but info a
> dedicated tdc-mode (which is not bit-field) inside the struct tdc?

If you do so, then you would need both a tdcmode and a
tdcmode_supported in order for the device to announce which modes
it supports (same as the ctrlmode and ctrlmode_supported in
can_priv). I seriously thought about this option but it seemed
like reinventing the wheel for me.

Also, it needs to be bit field to differentiate between a device
which would only support manual mode, one device which would only
support auto mode and one device which would support both.


Yours sincerely,
Vincent

Re: [PATCH v5 1/7] can: netlink: allow user to turn off unsupported features

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]

On 15.08.2021 12:32:42, Vincent Mailhol wrote:
> The sanity checks on the control modes will reject any request related
> to an unsupported features, even turning it off.
> 
> Example on an interface which does not support CAN-FD:
> 
> $ ip link set can0 type can bitrate 500000 fd off
> RTNETLINK answers: Operation not supported
> 
> This patch lets such command go through (but requests to turn on an
> unsupported feature are, of course, still denied).
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

I'm planing to send a pull request to net-next today. I want to do some
more tests with this series, but this patch is more or less unrelated,
so I can take it in this PR, should I?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 1/7] can: netlink: allow user to turn off unsupported features

[Vincent MAILHOL]

On Thu. 19 Aug 2021 at 16:45, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 15.08.2021 12:32:42, Vincent Mailhol wrote:
> > The sanity checks on the control modes will reject any request related
> > to an unsupported features, even turning it off.
> >
> > Example on an interface which does not support CAN-FD:
> >
> > $ ip link set can0 type can bitrate 500000 fd off
> > RTNETLINK answers: Operation not supported
> >
> > This patch lets such command go through (but requests to turn on an
> > unsupported feature are, of course, still denied).
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>
> I'm planing to send a pull request to net-next today. I want to do some
> more tests with this series

Ack, I am also preparing a new version. But first, I am just
waiting for your reply on the tdc-mode {auto, manual, off}. :)

> but this patch is more or less unrelated,
> so I can take it in this PR, should I?

FYI, the reason to add it to the series is that when setting TDC to
off, the ip tool sets both CAN_CTRLMODE_TDC_AUTO and
CAN_CTRLMODE_TDC_MANUAL to zero (which the corresponding bits in
can_ctrlmode::mask set to 1).  Without this patch, netlink would
return -ENOTSUPP if the driver only supported one of
CAN_CTRLMODE_TDC_{AUTO,MANUAL}.

Regardless, this patch makes sense as a standalone, I am fine if
you include it in your PR.


Also, if you want, you can include the latest patch of the series as well:
https://lore.kernel.org/linux-can/20210815033248.98111-8-mailhol.vincent@wanadoo.fr/

It's a comment fix, it should be pretty harmless.


Yours sincerely,
Vincent

Re: [PATCH v5 1/7] can: netlink: allow user to turn off unsupported features

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2224 bytes --]

On 19.08.2021 18:24:27, Vincent MAILHOL wrote:
> On Thu. 19 Aug 2021 at 16:45, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 15.08.2021 12:32:42, Vincent Mailhol wrote:
> > > The sanity checks on the control modes will reject any request related
> > > to an unsupported features, even turning it off.
> > >
> > > Example on an interface which does not support CAN-FD:
> > >
> > > $ ip link set can0 type can bitrate 500000 fd off
> > > RTNETLINK answers: Operation not supported
> > >
> > > This patch lets such command go through (but requests to turn on an
> > > unsupported feature are, of course, still denied).
> > >
> > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >
> > I'm planing to send a pull request to net-next today. I want to do some
> > more tests with this series
> 
> Ack, I am also preparing a new version. But first, I am just
> waiting for your reply on the tdc-mode {auto, manual, off}. :)

I want to do some proper testing, if it's now working as I'm expecting,
before continuing the discussion. :D

> > but this patch is more or less unrelated,
> > so I can take it in this PR, should I?
> 
> FYI, the reason to add it to the series is that when setting TDC to
> off, the ip tool sets both CAN_CTRLMODE_TDC_AUTO and
> CAN_CTRLMODE_TDC_MANUAL to zero (which the corresponding bits in
> can_ctrlmode::mask set to 1).  Without this patch, netlink would
> return -ENOTSUPP if the driver only supported one of
> CAN_CTRLMODE_TDC_{AUTO,MANUAL}.

Oh, I see.

> Regardless, this patch makes sense as a standalone, I am fine if
> you include it in your PR.

Why not, reduces the patch amount on your side :)

> Also, if you want, you can include the latest patch of the series as well:
> https://lore.kernel.org/linux-can/20210815033248.98111-8-mailhol.vincent@wanadoo.fr/
> 
> It's a comment fix, it should be pretty harmless.

Ok, makes sense.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 7/7] can: etas_es58x: clean-up documentation of struct es58x_fd_tx_conf_msg

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 694 bytes --]

On 15.08.2021 12:32:48, Vincent Mailhol wrote:
> The documentation of struct es58x_fd_tx_conf_msg explains in details
> the different TDC parameters. However, those description are redundant
> with the documentation of struct can_tdc.
> 
> Remove most of the description.
> 
> Also, fixes a typo in the reference to the datasheet (E701 -> E70).

As suggested, applied to linux-can-next/testing.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 2/7] can: bittiming: allow TDC{V,O} to be zero and add can_tdc_const::tdc{v,o,f}_min

[Vincent MAILHOL]

On Wed. 18 Aug 2021 at 23:17, Vincent MAILHOL
<mailhol.vincent@wanadoo.fr> wrote:
> On Wed. 18 Aug 2021 at 21:29, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 18.08.2021 18:22:33, Vincent MAILHOL wrote:
> > > > Backwards compatibility using an old ip tool on a new kernel/driver must
> > > > work.
> > >
> > > I am not trying to argue against backward compatibility :)
> > > My comment was just to point out that I had other intents as well.
> > >
> > > > In case of the mcp251xfd the tdc mode must be activated and tdcv
> > > > set to the automatic calculated value and tdco automatically measured.
> > >
> > > Sorry but I am not sure if I will follow you. Here, do you mean
> > > that "nothing" should do the "fully automated" calculation?
> >
> > Sort of.
> > The use case is the old ip tool with a driver that supports tdc, for
> > CAN-FD to work it must be configured in fully automated mode.
>
> The current patch does that: "nothing" means that both TDC_AUTO
> and TDC_MANUAL are not set, same as what an old ip tool would
> do. And that triggers the default (fully automated) mode (call
> can_calc_tdco()).
>
> > > In your previous message, you said:
> > >
> > > > Does it make sense to let "mode auto" without a tdco value switch the
> > > > controller into full automatic mode and /* nothing */ not tough the tdc
> > > > config at all?
> > >
> > > So, you would like this behavior:
> > >
> > > | mode auto, no tdco provided -> kernel decides between TDC_AUTO and TDC off.
> >
> > NACK - mode auto, no tdco -> TDC_AUTO with tdco calculated by the kernel
>
> Currently, the tdco calculation is paired with the decision to
> enable or not TDC. If dbrp is one or two, then do the tdco
> calculation, else, TDC is off (c.f. can_calc_tdco()). This
> behaviour is to follow ISO 11898-1 which states that TDC is only
> applicable if data BRP is one or two. In the current patch I
> allow to have TDC enabled with a dbrp greater than 2 only if the
> tdco is provided by the user (i.e. I allow the user to forcefully
> go against ISO but the automatic calculation guarantees
> compliance).
>
> So what do you suggest to do when drbp is greater than 2? Still
> enable TDC (and violate ISO standard) or return an error
> code (e.g. -ENOTSUPP)?
>
> > > | mode auto, tdco provided -> TDC_AUTO
> >
> > ACK - TDC_AUTO with user supplied tdco
> >
> > > | mode manual, tdcv and tdco provided -> TDC_MANUAL
> >
> > ACK - TDC_MANUAL with tdco and tdcv user provided
> >
> > > | mode off is not needed anymore (redundant with "nothing")
> > > (TDCF left out of the picture intentionally)
> >
> > NACK - TDC is switched off
>
> Same as the current patch then :)
>
> > > | "nothing" -> TDC is off (not touch the tdc config at all)
> >
> > NACK - do not touch TDC setting, use previous setting
>
> Sorry but I still fail to understand your definition of "do not
> touch".
>
> The first time you start a device, all the structures are zeroed
> out meaning that TDC is off to begin with.  So the first time the
> user do something like:
>
> | ip link set can0 type can bitrate 1000000 dbitrate 8000000 fd on
>
> If you "do not touch" TDC it means that all TDC values stays at
> zero, i.e. TDC stays off. This would contradict point 1/.
>
> > > Correct?
> >
> > See above. Plus a change that addresses your issue 1/ from below.
> >
> > If driver supports TDC it should be initially brought into TDC auto
> > mode, if no TDC mode is given. Maybe we need an explizit TDC off to make
> > that work.
> >
> > > If you do so, I see three issues:
> > >
> > > 1/ Some of the drivers already implement TDC. Those will
> > > automatically do a calculation as long as FD is on. If "nothing"
> > > now brings TDC off, some users will find themselves with some
> > > error on the bus after the iproute2 update if they continue using
> > > the same command.
> >
> > Nothing would mean "do not touch" and as TDC auto is default a new ip
> > would work out of the box. Old ip will work, too. Just failing to decode
> > TDC_AUTO...
>
> See above: if you "do not touch", my understanding is that the
> old ip tool will indefinitely keep TDC to its initial value:
> everything zeroed out.
>
> To turn TDC auto, you will eventually call can_calc_tdco() and
> that will touch something.
>
> > > 2/ Users will need to read and understand how to use the TDC
> > > parameters of iproute2. And by experience, too many people just
> > > don't read the doc. If I can make the interface transparent and
> > > do the correct thing by default ("nothing"), I prefer to do so.
> >
> > ACK, see above
> >
> > > 3/ Final one is more of a nitpick. The mode auto might result in
> > > TDC being off. If we have a TDC_AUTO flag, I would expect the
> > > auto mode to always set that flag (unless error occurs). I see
> > > this to be slightly counter intuitive (I recognize that my
> > > solution also has some aspects which are not intuitive, I just
> > > want to point here that none are perfect).
> >
> > What are the corner cases where TDC_AUTO results in TDC off?
>
> dbrp greater than 2 (see above).
>
> > > To be honest, I really preferred the v1 of this series where
> > > there were no tdc-mode {auto,manual,off} and where the "off"
> > > behavior was controlled by setting TDCO to zero. However, as we
> > > realized, zero is a valid value and thus, I had to add all this
> > > complexity just to allow that damn zero value.
> >
> > Maybe we should not put the TDC mode _not_ into ctrl-mode, but info a
> > dedicated tdc-mode (which is not bit-field) inside the struct tdc?
>
> If you do so, then you would need both a tdcmode and a
> tdcmode_supported in order for the device to announce which modes
> it supports (same as the ctrlmode and ctrlmode_supported in
> can_priv). I seriously thought about this option but it seemed
> like reinventing the wheel for me.
>
> Also, it needs to be bit field to differentiate between a device
> which would only support manual mode, one device which would only
> support auto mode and one device which would support both.

I just realized something. If the user first sets the TDC
parameters and then does another command without any data
bittiming parameters provided, then the TDC parameters will be
recalculated but the other data bittiming parameters would remain
unchanged.

Example:
$ ip link set can0 type can bitrate 1000000 dbitrate 8000000 fd on
tdcv 33 tdco 16 tdc-mode manual
$ ip link set can0 type can bitrate 500000

Here, can_calc_tdco() will be triggered resulting in a switch to
TDC_AUTO mode.
In this scenario, it is not normal to only have the TDC
recalculated but not the other data bittiming parameters. Is it
what you were trying to explain when saying "do not touch"?

I am preparing a new series with below behavior:

* data bittiming not provided: TDC parameters unchanged
* data bittiming provided: (unchanged from current behavior)
    - tdc-mode not provided: do can_calc_tdco (fully automated)
    - tdc-mode auto and tdco provided: TDC_AUTO
    - tdc-mode manual and both of tdcv and tdco provided: TDC_MANUAL

N.B. TDC parameters must be provided together with data bittiming
parameters, e.g. data bittiming not provided + TDC parameters is
an invalid command.

Does that make more sense?


Yours sincerely,
Vinent