2021-08-14 09:20:27

by Vincent MAILHOL

[permalink] [raw]
Subject: [PATCH v4 0/7] add the netlink interface for CAN-FD Transmitter Delay Compensation (TDC)

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: https://lore.kernel.org/linux-can/[email protected]/

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:
Ref: https://lore.kernel.org/linux-can/[email protected]/

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.

** Changelog **

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 | 192 +++++++++++++++++++++-
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, 332 insertions(+), 54 deletions(-)

--
2.31.1


2021-08-14 09:20:30

by Vincent MAILHOL

[permalink] [raw]
Subject: [PATCH v4 1/7] can: netlink: allow user to turn off unsupported features

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 <[email protected]>
---
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

2021-08-14 09:20:33

by Vincent MAILHOL

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

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 <[email protected]>
---
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

2021-08-14 09:20:57

by Vincent MAILHOL

[permalink] [raw]
Subject: [PATCH v4 3/7] can: bittiming: change unit of TDC parameters to clock periods

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 <[email protected]>
Signed-off-by: Vincent Mailhol <[email protected]>
---
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

2021-08-14 09:21:23

by Vincent MAILHOL

[permalink] [raw]
Subject: [PATCH v4 4/7] can: dev: add can_tdc_get_relative_tdco() helper function

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 <[email protected]>
Signed-off-by: Vincent Mailhol <[email protected]>
---
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

2021-08-14 09:21:39

by Vincent MAILHOL

[permalink] [raw]
Subject: [PATCH v4 6/7] can: netlink: add can_priv::do_get_auto_tdcv() to retrieve tdcv from device

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 <[email protected]>
Signed-off-by: Vincent Mailhol <[email protected]>
---
drivers/net/can/dev/netlink.c | 13 +++++++++++--
include/linux/can/dev.h | 1 +
2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 735b9e36202e..9164decf2ddb 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -414,8 +414,17 @@ 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->ctrlmode & CAN_CTRLMODE_TDC_AUTO &&
+ 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

2021-08-14 09:22:08

by Vincent MAILHOL

[permalink] [raw]
Subject: [PATCH v4 7/7] can: etas_es58x: clean-up documentation of struct es58x_fd_tx_conf_msg

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 <[email protected]>
---
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

2021-08-14 09:22:50

by Vincent MAILHOL

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

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

Reference: https://lore.kernel.org/linux-can/[email protected]/T/#t

Signed-off-by: Vincent Mailhol <[email protected]>
---
drivers/net/can/dev/netlink.c | 181 ++++++++++++++++++++++++++++++-
include/uapi/linux/can/netlink.h | 29 ++++-
2 files changed, 204 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 80425636049d..735b9e36202e 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 <[email protected]>
+ * Copyright (C) 2021 Vincent Mailhol <[email protected]>
*/

#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[],
@@ -46,7 +60,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 +68,69 @@ 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[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, IFLA_CAN_TDC_MAX, nla,
+ can_tdc_policy, extack);
+ if (err)
+ return err;
+
+ if (tb[IFLA_CAN_TDC_TDCV]) {
+ u32 tdcv = nla_get_u32(tb[IFLA_CAN_TDC_TDCV]);
+
+ if (priv->ctrlmode & CAN_CTRLMODE_TDC_AUTO)
+ return -EOPNOTSUPP;
+
+ if (tdcv < tdc_const->tdcv_min || tdcv > tdc_const->tdcv_max)
+ return -EINVAL;
+
+ tdc->tdcv = tdcv;
+ } else if (priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL) {
+ return -EOPNOTSUPP;
+ }
+
+ if (tb[IFLA_CAN_TDC_TDCO]) {
+ u32 tdco = nla_get_u32(tb[IFLA_CAN_TDC_TDCO]);
+
+ if (tdco < tdc_const->tdco_min || tdco > tdc_const->tdco_max)
+ return -EINVAL;
+
+ tdc->tdco = tdco;
+ } else {
+ return -EOPNOTSUPP;
+ }
+
+ if (tb[IFLA_CAN_TDC_TDCF]) {
+ u32 tdcf = nla_get_u32(tb[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() */
@@ -107,6 +179,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
struct can_ctrlmode *cm;
u32 ctrlstatic;
u32 maskedflags;
+ u32 tdc_flags;

/* Do not allow changing controller mode while running */
if (dev->flags & IFF_UP)
@@ -138,7 +211,18 @@ 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_flags = cm->flags & CAN_CTRLMODE_TDC_MASK;
+ /* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually exclusive */
+ if (tdc_flags == CAN_CTRLMODE_TDC_MASK)
+ return -EOPNOTSUPP;
+ /* If one of CAN_CTRLMODE_TDC_* is set then TDC must be set */
+ if (tdc_flags && !data[IFLA_CAN_TDC])
+ return -EOPNOTSUPP;
+ tdc_mask = cm->mask & CAN_CTRLMODE_TDC_MASK;
}

if (data[IFLA_CAN_RESTART_MS]) {
@@ -189,8 +273,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 +305,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
+ */
+
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 +382,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 +489,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

2021-08-14 11:14:51

by Marc Kleine-Budde

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

On 14.08.2021 18:17:48, Vincent Mailhol wrote:
[...]

> 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() */
> @@ -107,6 +179,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
> struct can_ctrlmode *cm;
> u32 ctrlstatic;
> u32 maskedflags;
> + u32 tdc_flags;
>
> /* Do not allow changing controller mode while running */
> if (dev->flags & IFF_UP)
> @@ -138,7 +211,18 @@ 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_flags = cm->flags & CAN_CTRLMODE_TDC_MASK;
> + /* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually exclusive */
> + if (tdc_flags == CAN_CTRLMODE_TDC_MASK)
> + return -EOPNOTSUPP;
> + /* If one of CAN_CTRLMODE_TDC_* is set then TDC must be set */
> + if (tdc_flags && !data[IFLA_CAN_TDC])
> + return -EOPNOTSUPP;

These don't need information form the can_priv, right? So these checks
can be moved to can_validate()?

> + tdc_mask = cm->mask & CAN_CTRLMODE_TDC_MASK;
> }

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 |


Attachments:
(No filename) (1.70 kB)
signature.asc (499.00 B)
Download all attachments

2021-08-15 03:40:21

by Vincent MAILHOL

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

On Sat. 14 Aug 2021 at 20:12, Marc Kleine-Budde <[email protected]> wrote:
> On 14.08.2021 18:17:48, Vincent Mailhol wrote:
> [...]
>
> > 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() */
> > @@ -107,6 +179,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
> > struct can_ctrlmode *cm;
> > u32 ctrlstatic;
> > u32 maskedflags;
> > + u32 tdc_flags;
> >
> > /* Do not allow changing controller mode while running */
> > if (dev->flags & IFF_UP)
> > @@ -138,7 +211,18 @@ 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_flags = cm->flags & CAN_CTRLMODE_TDC_MASK;
> > + /* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually exclusive */
> > + if (tdc_flags == CAN_CTRLMODE_TDC_MASK)
> > + return -EOPNOTSUPP;
> > + /* If one of CAN_CTRLMODE_TDC_* is set then TDC must be set */
> > + if (tdc_flags && !data[IFLA_CAN_TDC])
> > + return -EOPNOTSUPP;
>
> These don't need information form the can_priv, right? So these checks
> can be moved to can_validate()?

ACK. Actually, this comment applies not only to can_changelink()
but also to can_tdc_changelink(). I just sent a v5 where I made
sure to move all the checks that do not rely on can_priv to
can_validate().


Yours sincerely,
Vincent