[PATCH 00/21] can: netlink: preparation before introduction of CAN XL step 2/2

[PATCH 00/21] can: netlink: preparation before introduction of CAN XL step 2/2

[Vincent Mailhol]

In November last year, I sent an RFC to introduce CAN XL [1]. That
RFC, despite positive feedback, was put on hold due to some unanswered
question concerning the PWM encoding [2].

While stuck, some small preparation work was done in parallel in [3]
by refactoring the struct can_priv and doing some trivial clean-up and
renaming. [3] received zero feedback but was eventually merged after
splitting it in smaller parts and resending it.

Finally, in July this year, we clarified the remaining mysteries about
PWM calculation, thus unlocking the series. Summer being a bit busy
because of some personal matters brings us to now.

After doing all the refactoring and adding all the CAN XL features,
the final result is roughly 30 patches, probably too much for a single
series. So I am splitting it in two:

  - preparation (this series)
  - CAN XL

And so, this series continues and finishes the preparation work done
in [3]. It contains all the refactoring needed to smoothly introduce
CAN XL. The goal is to:

  - split the function in smaller pieces: CAN XL will introduce a fair
    amount of code. And some functions which are already fairly long
    (86 lines for can_validate(), 216 lines for can_changelink())
    would grow to disproportionate sizes if the CAN XL logic were to
    be inlined in those functions.

  - repurpose the existing code to handle both CAN FD and CAN XL: a
    huge part of CAN XL simply reuses the CAN FD logic. All the
    existing CAN FD logic is made more generic to handle both CAN FD
    and XL.

In more details:

  - Patch #1 moves struct data_bittiming_params from dev.h to
    bittiming.h and patch #2 makes can_get_relative_tdco() FD agnostic
    before also moving it to bittiming.h.

  - Patch #3 adds some comments to netlink.h tagging which IFLA
    symbols are FD specific.

  - Patch #4 to #7 are refactoring can_validate() and
    can_validate_bittiming().

  - Patch #8 to #12 are refactoring can_changelink() and
    can_tdc_changelink().

  - Patch #13 and #14 are refactoring can_get_size() and
    can_tdc_get_size().

  - Patch #15 to #18 are refactoring can_fill_info() and
    can_tdc_fill_info().

  - Patch #19 makes can_calc_tdco() FD agnostic.

  - Patch #20 adds can_get_ctrlmode_str() which converts control mode
    flags into strings. This is done in preparation of patch #20.

  - Patch #21 is the final patch and improves the user experience by
    providing detailed error messages whenever invalid parameters are
    provided. All those error messages came into handy when debugging
    the upcoming CAN XL patches.

Aside from the last patch, the other changes do not impact any of the
existing functionalities.

The follow up series which introduces CAN XL is nearly completed but
will be sent only once this one is approved: one thing at a time, I do
not want to overwhelm people (including myself).

[1] https://lore.kernel.org/linux-can/20241110155902.72807-16-mailhol.vincent@wanadoo.fr/
[2] https://lore.kernel.org/linux-can/c4771c16-c578-4a6d-baee-918fe276dbe9@wanadoo.fr/
[3] https://lore.kernel.org/linux-can/20241110155902.72807-16-mailhol.vincent@wanadoo.fr/

To: Marc Kleine-Budde <mkl@pengutronix.de>
To: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Vincent Mailhol <mailhol@kernel.org>
Cc: Stéphane Grosjean <stephane.grosjean@hms-networks.com>
Cc: Robert Nawrath <mbro1689@gmail.com>
Cc: Minh Le <minh.le.aj@renesas.com>
Cc: Duy Nguyen <duy.nguyen.rh@renesas.com>
Cc: linux-can@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
Vincent Mailhol (21):
      can: dev: move struct data_bittiming_params to linux/can/bittiming.h
      can: dev: make can_get_relative_tdco() FD agnostic and move it to bittiming.h
      can: netlink: document which symbols are FD specific
      can: netlink: refactor can_validate_bittiming()
      can: netlink: add can_validate_tdc()
      can: netlink: add can_validate_databittiming()
      can: netlink: remove comment in can_validate()
      can: netlink: refactor CAN_CTRLMODE_TDC_{AUTO,MANUAL} flag reset logic
      can: netlink: remove useless check in can_tdc_changelink()
      can: netlink: make can_tdc_changelink() FD agnostic
      can: netlink: add can_dtb_changelink()
      can: netlink: add can_ctrlmode_changelink()
      can: netlink: make can_tdc_get_size() FD agnostic
      can: netlink: add can_data_bittiming_get_size()
      can: netlink: add can_bittiming_fill_info()
      can: netlink: add can_bittiming_const_fill_info()
      can: netlink: add can_bitrate_const_fill_info()
      can: netlink: make can_tdc_fill_info() FD agnostic
      can: calc_bittiming: make can_calc_tdco() FD agnostic
      can: dev: add can_get_ctrlmode_str()
      can: netlink: add userland error messages

 drivers/net/can/dev/calc_bittiming.c |  10 +-
 drivers/net/can/dev/dev.c            |  33 ++
 drivers/net/can/dev/netlink.c        | 607 ++++++++++++++++++++++-------------
 include/linux/can/bittiming.h        |  48 ++-
 include/linux/can/dev.h              |  42 +--
 include/uapi/linux/can/netlink.h     |  14 +-
 6 files changed, 471 insertions(+), 283 deletions(-)
---
base-commit: 2fd4161d0d2547650d9559d57fc67b4e0a26a9e3
change-id: 20250831-canxl-netlink-prep-9dbf8498fd9d

Best regards,
-- 
Vincent Mailhol <mailhol@kernel.org>

[PATCH 01/21] can: dev: move struct data_bittiming_params to linux/can/bittiming.h

[Vincent Mailhol]

In commit b803c4a4f788 ("can: dev: add struct data_bittiming_params to
group FD parameters"), struct data_bittiming_params was put into
linux/can/dev.h.

This structure being a collection of bittiming parameters, on second
thought, bittiming.h is actually a better location. This way, users of
struct data_bittiming_params will not have to forcefully include
linux/can/dev.h thus removing some complexity and reducing the risk of
circular dependencies in headers.

Move struct data_bittiming_params from linux/can/dev.h to
linux/can/bittiming.h.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 include/linux/can/bittiming.h | 11 +++++++++++
 include/linux/can/dev.h       | 11 -----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
index 5dfdbb63b1d54f3dc02170b10b73dbb9c2242851..6572ec1712ca2df8db7fe1453ae5a4d5699712b1 100644
--- a/include/linux/can/bittiming.h
+++ b/include/linux/can/bittiming.h
@@ -114,6 +114,17 @@ struct can_tdc_const {
 	u32 tdcf_max;
 };
 
+struct data_bittiming_params {
+	const struct can_bittiming_const *data_bittiming_const;
+	struct can_bittiming data_bittiming;
+	const struct can_tdc_const *tdc_const;
+	struct can_tdc tdc;
+	const u32 *data_bitrate_const;
+	unsigned int data_bitrate_const_cnt;
+	int (*do_set_data_bittiming)(struct net_device *dev);
+	int (*do_get_auto_tdcv)(const struct net_device *dev, u32 *tdcv);
+};
+
 #ifdef CONFIG_CAN_CALC_BITTIMING
 int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 		       const struct can_bittiming_const *btc, struct netlink_ext_ack *extack);
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 9a92cbe5b2cb7ccdfca3121718856d096e9ecfa6..76022f48a97673d81676c39c697eadc6d7063ff7 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -38,17 +38,6 @@ enum can_termination_gpio {
 	CAN_TERMINATION_GPIO_MAX,
 };
 
-struct data_bittiming_params {
-	const struct can_bittiming_const *data_bittiming_const;
-	struct can_bittiming data_bittiming;
-	const struct can_tdc_const *tdc_const;
-	struct can_tdc tdc;
-	const u32 *data_bitrate_const;
-	unsigned int data_bitrate_const_cnt;
-	int (*do_set_data_bittiming)(struct net_device *dev);
-	int (*do_get_auto_tdcv)(const struct net_device *dev, u32 *tdcv);
-};
-
 /*
  * CAN common private data
  */

-- 
2.49.1

[PATCH 02/21] can: dev: make can_get_relative_tdco() FD agnostic and move it to bittiming.h

[Vincent Mailhol]

can_get_relative_tdco() needs to access can_priv->fd making it
specific to CAN FD. Change the function parameter from struct can_priv
to struct data_bittiming_params. This way, the function becomes CAN FD
agnostic and can be reused later on for the CAN XL TDC.

Now that we dropped the dependency on struct can_priv, also move
can_get_relative_tdco() back to bittiming.h where it was meant to
belong to.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
Changelog:

  RFC -> v1:

   - Just pass the IFLA index instead of passing each argument
     individually. Instead, derive these as local variables depending
     on whethe the IFLA index is IFLA_CAN_TDC or IFLA_CAN_XL_TDC.
---
 include/linux/can/bittiming.h | 29 +++++++++++++++++++++++++++++
 include/linux/can/dev.h       | 29 -----------------------------
 2 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
index 6572ec1712ca2df8db7fe1453ae5a4d5699712b1..4d5f7794194ab13641c7854c2d66625c4e942f6c 100644
--- a/include/linux/can/bittiming.h
+++ b/include/linux/can/bittiming.h
@@ -160,6 +160,35 @@ int can_get_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 		      const unsigned int bitrate_const_cnt,
 		      struct netlink_ext_ack *extack);
 
+/*
+ * 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 data_bittiming_params *dbt_params)
+{
+	const struct can_bittiming *dbt = &dbt_params->data_bittiming;
+	s32 sample_point_in_tc = (CAN_SYNC_SEG + dbt->prop_seg +
+				  dbt->phase_seg1) * dbt->brp;
+
+	return (s32)dbt_params->tdc.tdco - sample_point_in_tc;
+}
+
 /*
  * can_bit_time() - Duration of one bit
  *
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 76022f48a97673d81676c39c697eadc6d7063ff7..55aaadaacf68f940fa1b71f7c438e68b84080292 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -85,35 +85,6 @@ static inline bool can_fd_tdc_is_enabled(const struct can_priv *priv)
 	return !!(priv->ctrlmode & CAN_CTRLMODE_FD_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->fd.data_bittiming;
-	s32 sample_point_in_tc = (CAN_SYNC_SEG + dbt->prop_seg +
-				  dbt->phase_seg1) * dbt->brp;
-
-	return (s32)priv->fd.tdc.tdco - sample_point_in_tc;
-}
-
 /* helper to define static CAN controller features at device creation time */
 static inline int __must_check can_set_static_ctrlmode(struct net_device *dev,
 						       u32 static_mode)

-- 
2.49.1

[PATCH 03/21] can: netlink: document which symbols are FD specific

[Vincent Mailhol]

The CAN XL netlink interface will also have data bitrate and TDC
parameters. The current FD parameters do not have a prefix in their
names to differentiate them.

Because the netlink interface is part of the UAPI, it is unfortunately
not feasible to rename the existing symbols to add an FD_ prefix. The
best alternative is to add a comment for each of the symbols to notify
the reader of which parts are CAN FD specific.

While at it, fix a typo: transiver -> transceiver.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 include/uapi/linux/can/netlink.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index 02ec32d694742a32b3a51ff8c33616e109cef9f4..ef62f56eaaefda9f2fb39345776a483734682cd0 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -101,8 +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 */
+#define CAN_CTRLMODE_TDC_AUTO		0x200	/* FD transceiver automatically calculates TDCV */
+#define CAN_CTRLMODE_TDC_MANUAL		0x400	/* FD TDCV is manually set up by user */
 
 /*
  * CAN device statistics
@@ -129,14 +129,14 @@ enum {
 	IFLA_CAN_RESTART_MS,
 	IFLA_CAN_RESTART,
 	IFLA_CAN_BERR_COUNTER,
-	IFLA_CAN_DATA_BITTIMING,
-	IFLA_CAN_DATA_BITTIMING_CONST,
+	IFLA_CAN_DATA_BITTIMING, /* FD */
+	IFLA_CAN_DATA_BITTIMING_CONST, /* FD */
 	IFLA_CAN_TERMINATION,
 	IFLA_CAN_TERMINATION_CONST,
 	IFLA_CAN_BITRATE_CONST,
-	IFLA_CAN_DATA_BITRATE_CONST,
+	IFLA_CAN_DATA_BITRATE_CONST, /* FD */
 	IFLA_CAN_BITRATE_MAX,
-	IFLA_CAN_TDC,
+	IFLA_CAN_TDC, /* FD */
 	IFLA_CAN_CTRLMODE_EXT,
 
 	/* add new constants above here */
@@ -145,7 +145,7 @@ enum {
 };
 
 /*
- * CAN FD Transmitter Delay Compensation (TDC)
+ * CAN FD/XL Transmitter Delay Compensation (TDC)
  *
  * Please refer to struct can_tdc_const and can_tdc in
  * include/linux/can/bittiming.h for further details.

-- 
2.49.1

[PATCH 04/21] can: netlink: refactor can_validate_bittiming()

[Vincent Mailhol]

Whenever can_validate_bittiming() is called, it is always preceded by
some boilerplate code which was copy pasted all over the place. Move
that repeated code directly inside can_validate_bittiming().

Finally, the mempcy() is not needed. Just use the pointer returned by
nla_data() as-is.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 drivers/net/can/dev/netlink.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index d9f6ab3efb9767409c318b714f19df8a30e51137..bc91df8d75ac41381fefea895d7e490a965d3f7b 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -36,13 +36,20 @@ static const struct nla_policy can_tdc_policy[IFLA_CAN_TDC_MAX + 1] = {
 	[IFLA_CAN_TDC_TDCF] = { .type = NLA_U32 },
 };
 
-static int can_validate_bittiming(const struct can_bittiming *bt,
-				  struct netlink_ext_ack *extack)
+static int can_validate_bittiming(struct nlattr *data[],
+				  struct netlink_ext_ack *extack,
+				  int ifla_can_bittiming)
 {
+	struct can_bittiming *bt;
+
+	if (!data[ifla_can_bittiming])
+		return 0;
+
+	bt = nla_data(data[ifla_can_bittiming]);
+
 	/* sample point is in one-tenth of a percent */
 	if (bt->sample_point >= 1000) {
 		NL_SET_ERR_MSG(extack, "sample point must be between 0 and 100%");
-
 		return -EINVAL;
 	}
 
@@ -105,14 +112,9 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 		}
 	}
 
-	if (data[IFLA_CAN_BITTIMING]) {
-		struct can_bittiming bt;
-
-		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
-		err = can_validate_bittiming(&bt, extack);
-		if (err)
-			return err;
-	}
+	err = can_validate_bittiming(data, extack, IFLA_CAN_BITTIMING);
+	if (err)
+		return err;
 
 	if (is_can_fd) {
 		if (!data[IFLA_CAN_BITTIMING] || !data[IFLA_CAN_DATA_BITTIMING])
@@ -124,14 +126,9 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 			return -EOPNOTSUPP;
 	}
 
-	if (data[IFLA_CAN_DATA_BITTIMING]) {
-		struct can_bittiming bt;
-
-		memcpy(&bt, nla_data(data[IFLA_CAN_DATA_BITTIMING]), sizeof(bt));
-		err = can_validate_bittiming(&bt, extack);
-		if (err)
-			return err;
-	}
+	err = can_validate_bittiming(data, extack, IFLA_CAN_DATA_BITTIMING);
+	if (err)
+		return err;
 
 	return 0;
 }

-- 
2.49.1

[PATCH 05/21] can: netlink: add can_validate_tdc()

[Vincent Mailhol]

Factorise the TDC validation out of can_validate() and move it in the
new can_validate_tdc() function. This is a preparation patch for the
introduction of CAN XL because this TDC validation will be reused
later on.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
Changelog:

RFC 1 -> RFC 2:

  - fix bug on tdc flags mutual exclusivity:
      'if (tdc_auto == tdc_manual)' -> 'if (tdc_auto && tdc_manual)'
---
 drivers/net/can/dev/netlink.c | 80 +++++++++++++++++++++++++------------------
 include/linux/can/bittiming.h |  4 +++
 2 files changed, 51 insertions(+), 33 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index bc91df8d75ac41381fefea895d7e490a965d3f7b..1367ebe5db8fd6fcac49d3a1f59d44b61847128f 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -56,6 +56,49 @@ static int can_validate_bittiming(struct nlattr *data[],
 	return 0;
 }
 
+static int can_validate_tdc(struct nlattr *data_tdc,
+			    struct netlink_ext_ack *extack, u32 tdc_flags)
+{
+	bool tdc_manual = tdc_flags & CAN_CTRLMODE_TDC_MANUAL_MASK;
+	bool tdc_auto = tdc_flags & CAN_CTRLMODE_TDC_AUTO_MASK;
+	int err;
+
+	/* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually exclusive */
+	if (tdc_auto && tdc_manual)
+		return -EOPNOTSUPP;
+
+	/* If one of the CAN_CTRLMODE_TDC_* flag is set then TDC
+	 * must be set and vice-versa
+	 */
+	if ((tdc_auto || tdc_manual) != !!data_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_tdc) {
+		struct nlattr *tb_tdc[IFLA_CAN_TDC_MAX + 1];
+
+		err = nla_parse_nested(tb_tdc, IFLA_CAN_TDC_MAX,
+				       data_tdc, can_tdc_policy, extack);
+		if (err)
+			return err;
+
+		if (tb_tdc[IFLA_CAN_TDC_TDCV]) {
+			if (tdc_auto)
+				return -EOPNOTSUPP;
+		} else {
+			if (tdc_manual)
+				return -EOPNOTSUPP;
+		}
+
+		if (!tb_tdc[IFLA_CAN_TDC_TDCO])
+			return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 			struct netlink_ext_ack *extack)
 {
@@ -74,42 +117,13 @@ 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_FD_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_FD_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];
-
-			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;
-		}
+		err = can_validate_tdc(data[IFLA_CAN_TDC], extack,
+				       cm->flags & CAN_CTRLMODE_FD_TDC_MASK);
+		if (err)
+			return err;
 	}
 
 	err = can_validate_bittiming(data, extack, IFLA_CAN_BITTIMING);
diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
index 4d5f7794194ab13641c7854c2d66625c4e942f6c..71f839c3f0325b2a496a4bc447044a4853541338 100644
--- a/include/linux/can/bittiming.h
+++ b/include/linux/can/bittiming.h
@@ -16,6 +16,10 @@
 
 #define CAN_CTRLMODE_FD_TDC_MASK				\
 	(CAN_CTRLMODE_TDC_AUTO | CAN_CTRLMODE_TDC_MANUAL)
+#define CAN_CTRLMODE_TDC_AUTO_MASK				\
+	(CAN_CTRLMODE_TDC_AUTO)
+#define CAN_CTRLMODE_TDC_MANUAL_MASK				\
+	(CAN_CTRLMODE_TDC_MANUAL)
 
 /*
  * struct can_tdc - CAN FD Transmission Delay Compensation parameters

-- 
2.49.1

[PATCH 06/21] can: netlink: add can_validate_databittiming()

[Vincent Mailhol]

Factorise the databittiming validation out of can_validate() and move
it in the new add can_validate_databittiming() function. This is a
preparation patch for the introduction of CAN XL as this databittiming
validation will be reused later on.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 drivers/net/can/dev/netlink.c | 60 ++++++++++++++++++++++++++++++-------------
 1 file changed, 42 insertions(+), 18 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 1367ebe5db8fd6fcac49d3a1f59d44b61847128f..f7b12057bc9c6c286aa0c4341d565a497254296d 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -99,10 +99,48 @@ static int can_validate_tdc(struct nlattr *data_tdc,
 	return 0;
 }
 
+static int can_validate_databittiming(struct nlattr *data[],
+				      struct netlink_ext_ack *extack,
+				      int ifla_can_data_bittiming, u32 flags)
+{
+	struct nlattr *data_tdc;
+	u32 tdc_flags;
+	bool is_on;
+	int err;
+
+	if (ifla_can_data_bittiming == IFLA_CAN_DATA_BITTIMING) {
+		data_tdc = data[IFLA_CAN_TDC];
+		tdc_flags = flags & CAN_CTRLMODE_FD_TDC_MASK;
+		is_on = flags & CAN_CTRLMODE_FD;
+	} else {
+		WARN_ON(1); /* Place holder for CAN XL */
+	}
+
+	if (is_on) {
+		if (!data[IFLA_CAN_BITTIMING] || !data[ifla_can_data_bittiming])
+			return -EOPNOTSUPP;
+	}
+
+	if (data[ifla_can_data_bittiming] || data_tdc) {
+		if (!is_on)
+			return -EOPNOTSUPP;
+	}
+
+	err = can_validate_bittiming(data, extack, ifla_can_data_bittiming);
+	if (err)
+		return err;
+
+	err = can_validate_tdc(data_tdc, extack, tdc_flags);
+	if (err)
+		return err;
+
+	return 0;
+}
+
 static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 			struct netlink_ext_ack *extack)
 {
-	bool is_can_fd = false;
+	u32 flags = 0;
 	int err;
 
 	/* Make sure that valid CAN FD configurations always consist of
@@ -118,29 +156,15 @@ 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]);
 
-		is_can_fd = cm->flags & cm->mask & CAN_CTRLMODE_FD;
-
-		err = can_validate_tdc(data[IFLA_CAN_TDC], extack,
-				       cm->flags & CAN_CTRLMODE_FD_TDC_MASK);
-		if (err)
-			return err;
+		flags = cm->flags & cm->mask;
 	}
 
 	err = can_validate_bittiming(data, extack, IFLA_CAN_BITTIMING);
 	if (err)
 		return err;
 
-	if (is_can_fd) {
-		if (!data[IFLA_CAN_BITTIMING] || !data[IFLA_CAN_DATA_BITTIMING])
-			return -EOPNOTSUPP;
-	}
-
-	if (data[IFLA_CAN_DATA_BITTIMING] || data[IFLA_CAN_TDC]) {
-		if (!is_can_fd)
-			return -EOPNOTSUPP;
-	}
-
-	err = can_validate_bittiming(data, extack, IFLA_CAN_DATA_BITTIMING);
+	err = can_validate_databittiming(data, extack,
+					 IFLA_CAN_DATA_BITTIMING, flags);
 	if (err)
 		return err;
 

-- 
2.49.1

[PATCH 07/21] can: netlink: remove comment in can_validate()

[Vincent Mailhol]

The comment in can_validate() is just paraphrasing the code. When
adding CAN XL, updating this comment would add some overhead work for
no clear benefit.

Now that the function has been refactored and split into smaller
pieces, let the code speak for itself. Remove the comment.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 drivers/net/can/dev/netlink.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index f7b12057bc9c6c286aa0c4341d565a497254296d..6ea629331d20483c5e70567eb1be226a3b09882c 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -143,13 +143,6 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 	u32 flags = 0;
 	int err;
 
-	/* Make sure that valid CAN FD configurations always consist of
-	 * - nominal/arbitration bittiming
-	 * - data bittiming
-	 * - control mode with CAN_CTRLMODE_FD set
-	 * - TDC parameters are coherent (details below)
-	 */
-
 	if (!data)
 		return 0;
 

-- 
2.49.1

[PATCH 08/21] can: netlink: refactor CAN_CTRLMODE_TDC_{AUTO,MANUAL} flag reset logic

[Vincent Mailhol]

CAN_CTRLMODE_TDC_AUTO and CAN_CTRLMODE_TDC_MANUAL are mutually
exclusive. This means that whenever the user switches from auto to
manual mode (or vice versa), the other flag which was set previously
needs to be cleared.

Currently, this is handled with a masking operation. It can be done in
a simpler manner by clearing any of the previous TDC flags before
copying netlink attributes. The code becomes easier to understand and
will make it easier to add the new upcoming CAN XL flags which will
have a similar reset logic as the current TDC flags.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 drivers/net/can/dev/netlink.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 6ea629331d20483c5e70567eb1be226a3b09882c..a7bcb2b0a1c06711601d0bb037c4b998259b5dfd 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -247,6 +247,10 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		if ((maskedflags & ctrlstatic) != ctrlstatic)
 			return -EOPNOTSUPP;
 
+		/* If a top dependency flag is provided, reset all its dependencies */
+		if (cm->mask & CAN_CTRLMODE_FD)
+			priv->ctrlmode &= !CAN_CTRLMODE_FD_TDC_MASK;
+
 		/* clear bits to be modified and copy the flag values */
 		priv->ctrlmode &= ~cm->mask;
 		priv->ctrlmode |= maskedflags;
@@ -263,11 +267,6 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		}
 
 		fd_tdc_flag_provided = cm->mask & CAN_CTRLMODE_FD_TDC_MASK;
-		/* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually
-		 * exclusive: make sure to turn the other one off
-		 */
-		if (fd_tdc_flag_provided)
-			priv->ctrlmode &= cm->flags | ~CAN_CTRLMODE_FD_TDC_MASK;
 	}
 
 	if (data[IFLA_CAN_BITTIMING]) {

-- 
2.49.1

[PATCH 09/21] can: netlink: remove useless check in can_tdc_changelink()

[Vincent Mailhol]

can_tdc_changelink() return -EOPNOTSUPP under this condition:

  !tdc_const || !can_fd_tdc_is_enabled(priv)

But this function is only called if the data[IFLA_CAN_TDC] parameters
are provided. At this point, can_validate_tdc() already checked that
either of the tdc auto or tdc manual control modes were provided, that
is to say, can_fd_tdc_is_enabled(priv) must be true.

Because the right hand operand of this condition is always true,
remove it.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 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 a7bcb2b0a1c06711601d0bb037c4b998259b5dfd..fcb23df08f7b76f341a298d0bd16ec62e0a98525 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -172,7 +172,7 @@ static int can_tdc_changelink(struct can_priv *priv, const struct nlattr *nla,
 	const struct can_tdc_const *tdc_const = priv->fd.tdc_const;
 	int err;
 
-	if (!tdc_const || !can_fd_tdc_is_enabled(priv))
+	if (!tdc_const)
 		return -EOPNOTSUPP;
 
 	err = nla_parse_nested(tb_tdc, IFLA_CAN_TDC_MAX, nla,

-- 
2.49.1

[PATCH 10/21] can: netlink: make can_tdc_changelink() FD agnostic

[Vincent Mailhol]

can_tdc_changelink() needs to access can_priv->fd making it
specific to CAN FD. Change the function parameter from struct can_priv
to struct data_bittiming_params. This way, the function becomes CAN FD
agnostic and can be reused later on for the CAN XL TDC.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 drivers/net/can/dev/netlink.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index fcb23df08f7b76f341a298d0bd16ec62e0a98525..1a3389e3880f1c9c57e2e45f4cacc272c260fb07 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -164,12 +164,13 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 	return 0;
 }
 
-static int can_tdc_changelink(struct can_priv *priv, const struct nlattr *nla,
+static int can_tdc_changelink(struct data_bittiming_params *dbt_params,
+			      const struct nlattr *nla,
 			      struct netlink_ext_ack *extack)
 {
 	struct nlattr *tb_tdc[IFLA_CAN_TDC_MAX + 1];
 	struct can_tdc tdc = { 0 };
-	const struct can_tdc_const *tdc_const = priv->fd.tdc_const;
+	const struct can_tdc_const *tdc_const = dbt_params->tdc_const;
 	int err;
 
 	if (!tdc_const)
@@ -207,7 +208,7 @@ static int can_tdc_changelink(struct can_priv *priv, const struct nlattr *nla,
 		tdc.tdcf = tdcf;
 	}
 
-	priv->fd.tdc = tdc;
+	dbt_params->tdc = tdc;
 
 	return 0;
 }
@@ -375,8 +376,8 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		memset(&priv->fd.tdc, 0, sizeof(priv->fd.tdc));
 		if (data[IFLA_CAN_TDC]) {
 			/* TDC parameters are provided: use them */
-			err = can_tdc_changelink(priv, data[IFLA_CAN_TDC],
-						 extack);
+			err = can_tdc_changelink(&priv->fd,
+						 data[IFLA_CAN_TDC], extack);
 			if (err) {
 				priv->ctrlmode &= ~CAN_CTRLMODE_FD_TDC_MASK;
 				return err;

-- 
2.49.1

[PATCH 11/21] can: netlink: add can_dtb_changelink()

[Vincent Mailhol]

Factorise the databittiming parsing out of can_changelink() and move
it in the new can_dtb_changelink() function. This is a preparation
patch for the introduction of CAN XL because the databittiming
changelink logic will be reused later on.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
Changelog:

RFC -> v1:

  - In the RFC, can_dbt_changelink() required 8 parameters. Instead,
    just pass a boolean and then get the FD/XL specific parameters in
    an "if (fd) {} else {}" conditional.
---
 drivers/net/can/dev/netlink.c | 152 ++++++++++++++++++++++++------------------
 1 file changed, 88 insertions(+), 64 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 1a3389e3880f1c9c57e2e45f4cacc272c260fb07..47c0b88ab2b3b1f6a2b006760124fe0ba4767e42 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -213,12 +213,95 @@ static int can_tdc_changelink(struct data_bittiming_params *dbt_params,
 	return 0;
 }
 
+static int can_dbt_changelink(struct net_device *dev, struct nlattr *data[],
+			      bool fd, struct netlink_ext_ack *extack)
+{
+	struct nlattr *data_bittiming, *data_tdc;
+	struct can_priv *priv = netdev_priv(dev);
+	struct data_bittiming_params *dbt_params;
+	struct can_bittiming dbt;
+	bool need_tdc_calc = false;
+	u32 tdc_mask;
+	int err;
+
+	if (fd) {
+		data_bittiming = data[IFLA_CAN_DATA_BITTIMING];
+		data_tdc = data[IFLA_CAN_TDC];
+		dbt_params = &priv->fd;
+		tdc_mask = CAN_CTRLMODE_FD_TDC_MASK;
+	} else {
+		WARN_ON(1); /* Place holder for CAN XL */
+	}
+
+	if (!data_bittiming)
+		return 0;
+
+	/* Do not allow changing bittiming while running */
+	if (dev->flags & IFF_UP)
+		return -EBUSY;
+
+	/* Calculate bittiming parameters based on data_bittiming_const
+	 * if set, otherwise pass bitrate directly via do_set_bitrate().
+	 * Bail out if neither is given.
+	 */
+	if (!dbt_params->data_bittiming_const && !dbt_params->do_set_data_bittiming &&
+	    !dbt_params->data_bitrate_const)
+		return -EOPNOTSUPP;
+
+	memcpy(&dbt, nla_data(data_bittiming), sizeof(dbt));
+	err = can_get_bittiming(dev, &dbt, dbt_params->data_bittiming_const,
+				dbt_params->data_bitrate_const,
+				dbt_params->data_bitrate_const_cnt, extack);
+	if (err)
+		return err;
+
+	if (priv->bitrate_max && dbt.bitrate > priv->bitrate_max) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "CAN data bitrate %u bps surpasses transceiver capabilities of %u bps",
+				   dbt.bitrate, priv->bitrate_max);
+		return -EINVAL;
+	}
+
+	memset(&dbt_params->tdc, 0, sizeof(dbt_params->tdc));
+	if (data[IFLA_CAN_CTRLMODE]) {
+		struct can_ctrlmode *cm = nla_data(data[IFLA_CAN_CTRLMODE]);
+
+		need_tdc_calc = !(cm->mask & tdc_mask);
+	}
+	if (data_tdc) {
+		/* TDC parameters are provided: use them */
+		err = can_tdc_changelink(dbt_params, data_tdc, extack);
+		if (err) {
+			priv->ctrlmode &= ~tdc_mask;
+			return err;
+		}
+	} else if (need_tdc_calc) {
+		/* Neither of TDC parameters nor TDC flags are provided:
+		 * do calculation
+		 */
+		can_calc_tdco(&dbt_params->tdc, dbt_params->tdc_const, &dbt,
+			      &priv->ctrlmode, priv->ctrlmode_supported);
+	} /* else: both CAN_CTRLMODE_TDC_{AUTO,MANUAL} are explicitly
+	   * turned off. TDC is disabled: do nothing
+	   */
+
+	memcpy(&dbt_params->data_bittiming, &dbt, sizeof(dbt));
+
+	if (dbt_params->do_set_data_bittiming) {
+		/* Finally, set the bit-timing registers */
+		err = dbt_params->do_set_data_bittiming(dev);
+		if (err)
+			return err;
+	}
+
+	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);
-	bool fd_tdc_flag_provided = false;
 	int err;
 
 	/* We need synchronization with dev->stop() */
@@ -266,8 +349,6 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 			priv->ctrlmode &= ~CAN_CTRLMODE_FD_TDC_MASK;
 			memset(&priv->fd.tdc, 0, sizeof(priv->fd.tdc));
 		}
-
-		fd_tdc_flag_provided = cm->mask & CAN_CTRLMODE_FD_TDC_MASK;
 	}
 
 	if (data[IFLA_CAN_BITTIMING]) {
@@ -340,67 +421,10 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 			return err;
 	}
 
-	if (data[IFLA_CAN_DATA_BITTIMING]) {
-		struct can_bittiming dbt;
-
-		/* Do not allow changing bittiming while running */
-		if (dev->flags & IFF_UP)
-			return -EBUSY;
-
-		/* Calculate bittiming parameters based on
-		 * data_bittiming_const if set, otherwise pass bitrate
-		 * directly via do_set_bitrate(). Bail out if neither
-		 * is given.
-		 */
-		if (!priv->fd.data_bittiming_const && !priv->fd.do_set_data_bittiming &&
-		    !priv->fd.data_bitrate_const)
-			return -EOPNOTSUPP;
-
-		memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
-		       sizeof(dbt));
-		err = can_get_bittiming(dev, &dbt,
-					priv->fd.data_bittiming_const,
-					priv->fd.data_bitrate_const,
-					priv->fd.data_bitrate_const_cnt,
-					extack);
-		if (err)
-			return err;
-
-		if (priv->bitrate_max && dbt.bitrate > priv->bitrate_max) {
-			NL_SET_ERR_MSG_FMT(extack,
-					   "CANFD data bitrate %u bps surpasses transceiver capabilities of %u bps",
-					   dbt.bitrate, priv->bitrate_max);
-			return -EINVAL;
-		}
-
-		memset(&priv->fd.tdc, 0, sizeof(priv->fd.tdc));
-		if (data[IFLA_CAN_TDC]) {
-			/* TDC parameters are provided: use them */
-			err = can_tdc_changelink(&priv->fd,
-						 data[IFLA_CAN_TDC], extack);
-			if (err) {
-				priv->ctrlmode &= ~CAN_CTRLMODE_FD_TDC_MASK;
-				return err;
-			}
-		} else if (!fd_tdc_flag_provided) {
-			/* Neither of TDC parameters nor TDC flags are
-			 * provided: do calculation
-			 */
-			can_calc_tdco(&priv->fd.tdc, priv->fd.tdc_const, &dbt,
-				      &priv->ctrlmode, priv->ctrlmode_supported);
-		} /* else: both CAN_CTRLMODE_TDC_{AUTO,MANUAL} are explicitly
-		   * turned off. TDC is disabled: do nothing
-		   */
-
-		memcpy(&priv->fd.data_bittiming, &dbt, sizeof(dbt));
-
-		if (priv->fd.do_set_data_bittiming) {
-			/* Finally, set the bit-timing registers */
-			err = priv->fd.do_set_data_bittiming(dev);
-			if (err)
-				return err;
-		}
-	}
+	/* CAN FD */
+	err = can_dbt_changelink(dev, data, true, extack);
+	if (err)
+		return err;
 
 	if (data[IFLA_CAN_TERMINATION]) {
 		const u16 termval = nla_get_u16(data[IFLA_CAN_TERMINATION]);

-- 
2.49.1

[PATCH 12/21] can: netlink: add can_ctrlmode_changelink()

[Vincent Mailhol]

Split the control mode change link logic into a new function:
can_ctrlmode_changelink(). The purpose is to increase code readability
by preventing can_changelink() from becoming too big.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 drivers/net/can/dev/netlink.c | 98 ++++++++++++++++++++++++-------------------
 1 file changed, 55 insertions(+), 43 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 47c0b88ab2b3b1f6a2b006760124fe0ba4767e42..2f6192fc439bc3a7528aea2ad17efd18fce91c2c 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -164,6 +164,60 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 	return 0;
 }
 
+static int can_ctrlmode_changelink(struct net_device *dev,
+				   struct nlattr *data[],
+				   struct netlink_ext_ack *extack)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	struct can_ctrlmode *cm;
+	u32 maskedflags;
+	u32 ctrlstatic;
+
+	if (!data[IFLA_CAN_CTRLMODE])
+		return 0;
+
+	/* Do not allow changing controller mode while running */
+	if (dev->flags & IFF_UP)
+		return -EBUSY;
+
+	cm = nla_data(data[IFLA_CAN_CTRLMODE]);
+	maskedflags = cm->flags & cm->mask;
+	ctrlstatic = can_get_static_ctrlmode(priv);
+
+	/* check whether provided bits are allowed to be passed */
+	if (maskedflags & ~(priv->ctrlmode_supported | ctrlstatic))
+		return -EOPNOTSUPP;
+
+	/* do not check for static fd-non-iso if 'fd' is disabled */
+	if (!(maskedflags & CAN_CTRLMODE_FD))
+		ctrlstatic &= ~CAN_CTRLMODE_FD_NON_ISO;
+
+	/* make sure static options are provided by configuration */
+	if ((maskedflags & ctrlstatic) != ctrlstatic)
+		return -EOPNOTSUPP;
+
+	/* If a top dependency flag is provided, reset all its dependencies */
+	if (cm->mask & CAN_CTRLMODE_FD)
+		priv->ctrlmode &= !CAN_CTRLMODE_FD_TDC_MASK;
+
+	/* clear bits to be modified and copy the flag values */
+	priv->ctrlmode &= ~cm->mask;
+	priv->ctrlmode |= maskedflags;
+
+	/* CAN_CTRLMODE_FD can only be set when driver supports FD */
+	if (priv->ctrlmode & CAN_CTRLMODE_FD) {
+		dev->mtu = CANFD_MTU;
+	} else {
+		dev->mtu = CAN_MTU;
+		memset(&priv->fd.data_bittiming, 0,
+		       sizeof(priv->fd.data_bittiming));
+		priv->ctrlmode &= ~CAN_CTRLMODE_FD_TDC_MASK;
+		memset(&priv->fd.tdc, 0, sizeof(priv->fd.tdc));
+	}
+
+	return 0;
+}
+
 static int can_tdc_changelink(struct data_bittiming_params *dbt_params,
 			      const struct nlattr *nla,
 			      struct netlink_ext_ack *extack)
@@ -307,49 +361,7 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 	/* We need synchronization with dev->stop() */
 	ASSERT_RTNL();
 
-	if (data[IFLA_CAN_CTRLMODE]) {
-		struct can_ctrlmode *cm;
-		u32 ctrlstatic;
-		u32 maskedflags;
-
-		/* Do not allow changing controller mode while running */
-		if (dev->flags & IFF_UP)
-			return -EBUSY;
-		cm = nla_data(data[IFLA_CAN_CTRLMODE]);
-		ctrlstatic = can_get_static_ctrlmode(priv);
-		maskedflags = cm->flags & cm->mask;
-
-		/* check whether provided bits are allowed to be passed */
-		if (maskedflags & ~(priv->ctrlmode_supported | ctrlstatic))
-			return -EOPNOTSUPP;
-
-		/* do not check for static fd-non-iso if 'fd' is disabled */
-		if (!(maskedflags & CAN_CTRLMODE_FD))
-			ctrlstatic &= ~CAN_CTRLMODE_FD_NON_ISO;
-
-		/* make sure static options are provided by configuration */
-		if ((maskedflags & ctrlstatic) != ctrlstatic)
-			return -EOPNOTSUPP;
-
-		/* If a top dependency flag is provided, reset all its dependencies */
-		if (cm->mask & CAN_CTRLMODE_FD)
-			priv->ctrlmode &= !CAN_CTRLMODE_FD_TDC_MASK;
-
-		/* clear bits to be modified and copy the flag values */
-		priv->ctrlmode &= ~cm->mask;
-		priv->ctrlmode |= maskedflags;
-
-		/* CAN_CTRLMODE_FD can only be set when driver supports FD */
-		if (priv->ctrlmode & CAN_CTRLMODE_FD) {
-			dev->mtu = CANFD_MTU;
-		} else {
-			dev->mtu = CAN_MTU;
-			memset(&priv->fd.data_bittiming, 0,
-			       sizeof(priv->fd.data_bittiming));
-			priv->ctrlmode &= ~CAN_CTRLMODE_FD_TDC_MASK;
-			memset(&priv->fd.tdc, 0, sizeof(priv->fd.tdc));
-		}
-	}
+	can_ctrlmode_changelink(dev, data, extack);
 
 	if (data[IFLA_CAN_BITTIMING]) {
 		struct can_bittiming bt;

-- 
2.49.1

[PATCH 13/21] can: netlink: make can_tdc_get_size() FD agnostic

[Vincent Mailhol]

can_tdc_get_size() depends on some variables which are specific to CAN
FD. Move these to the function parameters list so that, later on, this
function can be reused for the CAN XL TDC.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 drivers/net/can/dev/netlink.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 2f6192fc439bc3a7528aea2ad17efd18fce91c2c..fc0d9d4ebc01fa56965a1c0d481c07bd5fee392b 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -465,32 +465,32 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 	return 0;
 }
 
-static size_t can_tdc_get_size(const struct net_device *dev)
+static size_t can_tdc_get_size(struct data_bittiming_params *dbt_params,
+			       u32 tdc_flags)
 {
-	struct can_priv *priv = netdev_priv(dev);
+	bool tdc_manual = tdc_flags & CAN_CTRLMODE_TDC_MANUAL_MASK;
 	size_t size;
 
-	if (!priv->fd.tdc_const)
+	if (!dbt_params->tdc_const)
 		return 0;
 
 	size = nla_total_size(0);			/* nest IFLA_CAN_TDC */
-	if (priv->ctrlmode_supported & CAN_CTRLMODE_TDC_MANUAL) {
+	if (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->fd.tdc_const->tdcf_max) {
+	if (dbt_params->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_fd_tdc_is_enabled(priv)) {
-		if (priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL ||
-		    priv->fd.do_get_auto_tdcv)
+	if (tdc_flags) {
+		if (tdc_manual || dbt_params->do_get_auto_tdcv)
 			size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCV */
 		size += nla_total_size(sizeof(u32));		/* IFLA_CAN_TDCO */
-		if (priv->fd.tdc_const->tdcf_max)
+		if (dbt_params->tdc_const->tdcf_max)
 			size += nla_total_size(sizeof(u32));	/* IFLA_CAN_TDCF */
 	}
 
@@ -534,7 +534,8 @@ static size_t can_get_size(const struct net_device *dev)
 		size += nla_total_size(sizeof(*priv->fd.data_bitrate_const) *
 				       priv->fd.data_bitrate_const_cnt);
 	size += sizeof(priv->bitrate_max);			/* IFLA_CAN_BITRATE_MAX */
-	size += can_tdc_get_size(dev);				/* IFLA_CAN_TDC */
+	size += can_tdc_get_size(&priv->fd,			/* IFLA_CAN_TDC */
+				 priv->ctrlmode & CAN_CTRLMODE_FD_TDC_MASK);
 	size += can_ctrlmode_ext_get_size();			/* IFLA_CAN_CTRLMODE_EXT */
 
 	return size;

-- 
2.49.1

[PATCH 14/21] can: netlink: add can_data_bittiming_get_size()

[Vincent Mailhol]

Add the can_data_bittiming_get_size() function to factorise the logic
to retrieve the size of below data bittiming parameters:

  - data_bittiming
  - data_bittiming_const
  - data_bitrate_const
  - tdc parameters

This function will be reused later on for CAN XL.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 drivers/net/can/dev/netlink.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index fc0d9d4ebc01fa56965a1c0d481c07bd5fee392b..e1ccb65bb555919a99cd9d71fe5f99399539fde2 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -497,6 +497,23 @@ static size_t can_tdc_get_size(struct data_bittiming_params *dbt_params,
 	return size;
 }
 
+static size_t can_data_bittiming_get_size(struct data_bittiming_params *dbt_params,
+					  u32 tdc_flags)
+{
+	size_t size = 0;
+
+	if (dbt_params->data_bittiming.bitrate)		/* IFLA_CAN_DATA_BITTIMING */
+		size += nla_total_size(sizeof(dbt_params->data_bittiming));
+	if (dbt_params->data_bittiming_const)		/* IFLA_CAN_DATA_BITTIMING_CONST */
+		size += nla_total_size(sizeof(*dbt_params->data_bittiming_const));
+	if (dbt_params->data_bitrate_const)		/* IFLA_CAN_DATA_BITRATE_CONST */
+		size += nla_total_size(sizeof(*dbt_params->data_bitrate_const) *
+				       dbt_params->data_bitrate_const_cnt);
+	size += can_tdc_get_size(dbt_params, tdc_flags);/* IFLA_CAN_TDC */
+
+	return size;
+}
+
 static size_t can_ctrlmode_ext_get_size(void)
 {
 	return nla_total_size(0) +		/* nest IFLA_CAN_CTRLMODE_EXT */
@@ -518,10 +535,6 @@ static size_t can_get_size(const struct net_device *dev)
 	size += nla_total_size(sizeof(u32));			/* IFLA_CAN_RESTART_MS */
 	if (priv->do_get_berr_counter)				/* IFLA_CAN_BERR_COUNTER */
 		size += nla_total_size(sizeof(struct can_berr_counter));
-	if (priv->fd.data_bittiming.bitrate)			/* IFLA_CAN_DATA_BITTIMING */
-		size += nla_total_size(sizeof(struct can_bittiming));
-	if (priv->fd.data_bittiming_const)			/* IFLA_CAN_DATA_BITTIMING_CONST */
-		size += nla_total_size(sizeof(struct can_bittiming_const));
 	if (priv->termination_const) {
 		size += nla_total_size(sizeof(priv->termination));		/* IFLA_CAN_TERMINATION */
 		size += nla_total_size(sizeof(*priv->termination_const) *	/* IFLA_CAN_TERMINATION_CONST */
@@ -530,14 +543,12 @@ static size_t can_get_size(const struct net_device *dev)
 	if (priv->bitrate_const)				/* IFLA_CAN_BITRATE_CONST */
 		size += nla_total_size(sizeof(*priv->bitrate_const) *
 				       priv->bitrate_const_cnt);
-	if (priv->fd.data_bitrate_const)			/* IFLA_CAN_DATA_BITRATE_CONST */
-		size += nla_total_size(sizeof(*priv->fd.data_bitrate_const) *
-				       priv->fd.data_bitrate_const_cnt);
 	size += sizeof(priv->bitrate_max);			/* IFLA_CAN_BITRATE_MAX */
-	size += can_tdc_get_size(&priv->fd,			/* IFLA_CAN_TDC */
-				 priv->ctrlmode & CAN_CTRLMODE_FD_TDC_MASK);
 	size += can_ctrlmode_ext_get_size();			/* IFLA_CAN_CTRLMODE_EXT */
 
+	size += can_data_bittiming_get_size(&priv->fd,
+					    priv->ctrlmode & CAN_CTRLMODE_FD_TDC_MASK);
+
 	return size;
 }
 

-- 
2.49.1

[PATCH 15/21] can: netlink: add can_bittiming_fill_info()

[Vincent Mailhol]

Add can_bittiming_fill_info() to factorise the logic when filling the
bittiming information for Classical CAN and CAN FD. This function will
be reused later on for CAN XL.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 drivers/net/can/dev/netlink.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index e1ccb65bb555919a99cd9d71fe5f99399539fde2..c3004c92e55cef72b9d03f7742e05fb1404cb1a3 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -552,6 +552,14 @@ static size_t can_get_size(const struct net_device *dev)
 	return size;
 }
 
+static int can_bittiming_fill_info(struct sk_buff *skb, int ifla_can_bittiming,
+				   struct can_bittiming *bittiming)
+{
+	return bittiming->bitrate != CAN_BITRATE_UNSET &&
+		bittiming->bitrate != CAN_BITRATE_UNKNOWN &&
+		nla_put(skb, ifla_can_bittiming, sizeof(*bittiming), bittiming);
+}
+
 static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
 {
 	struct nlattr *nest;
@@ -634,10 +642,8 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 	if (priv->do_get_state)
 		priv->do_get_state(dev, &state);
 
-	if ((priv->bittiming.bitrate != CAN_BITRATE_UNSET &&
-	     priv->bittiming.bitrate != CAN_BITRATE_UNKNOWN &&
-	     nla_put(skb, IFLA_CAN_BITTIMING,
-		     sizeof(priv->bittiming), &priv->bittiming)) ||
+	if (can_bittiming_fill_info(skb, IFLA_CAN_BITTIMING,
+				    &priv->bittiming) ||
 
 	    (priv->bittiming_const &&
 	     nla_put(skb, IFLA_CAN_BITTIMING_CONST,
@@ -652,9 +658,8 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 	     !priv->do_get_berr_counter(dev, &bec) &&
 	     nla_put(skb, IFLA_CAN_BERR_COUNTER, sizeof(bec), &bec)) ||
 
-	    (priv->fd.data_bittiming.bitrate &&
-	     nla_put(skb, IFLA_CAN_DATA_BITTIMING,
-		     sizeof(priv->fd.data_bittiming), &priv->fd.data_bittiming)) ||
+	    can_bittiming_fill_info(skb, IFLA_CAN_DATA_BITTIMING,
+				    &priv->fd.data_bittiming) ||
 
 	    (priv->fd.data_bittiming_const &&
 	     nla_put(skb, IFLA_CAN_DATA_BITTIMING_CONST,

-- 
2.49.1

[PATCH 16/21] can: netlink: add can_bittiming_const_fill_info()

[Vincent Mailhol]

Add function can_bittiming_fill_info() to factorise the logic when
filling the bittiming constant information for Classical CAN and CAN
FD. This function will be reused later on for CAN XL.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 drivers/net/can/dev/netlink.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index c3004c92e55cef72b9d03f7742e05fb1404cb1a3..cf9bf8eb1ae8e5c6b7910d8c9935cad73b78ef40 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -560,6 +560,15 @@ static int can_bittiming_fill_info(struct sk_buff *skb, int ifla_can_bittiming,
 		nla_put(skb, ifla_can_bittiming, sizeof(*bittiming), bittiming);
 }
 
+static int can_bittiming_const_fill_info(struct sk_buff *skb,
+					 int ifla_can_bittiming_const,
+					 const struct can_bittiming_const *bittiming_const)
+{
+	return bittiming_const &&
+		nla_put(skb, ifla_can_bittiming_const,
+			sizeof(*bittiming_const), bittiming_const);
+}
+
 static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
 {
 	struct nlattr *nest;
@@ -645,9 +654,8 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 	if (can_bittiming_fill_info(skb, IFLA_CAN_BITTIMING,
 				    &priv->bittiming) ||
 
-	    (priv->bittiming_const &&
-	     nla_put(skb, IFLA_CAN_BITTIMING_CONST,
-		     sizeof(*priv->bittiming_const), priv->bittiming_const)) ||
+	    can_bittiming_const_fill_info(skb, IFLA_CAN_BITTIMING_CONST,
+					  priv->bittiming_const) ||
 
 	    nla_put(skb, IFLA_CAN_CLOCK, sizeof(priv->clock), &priv->clock) ||
 	    nla_put_u32(skb, IFLA_CAN_STATE, state) ||
@@ -661,10 +669,8 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 	    can_bittiming_fill_info(skb, IFLA_CAN_DATA_BITTIMING,
 				    &priv->fd.data_bittiming) ||
 
-	    (priv->fd.data_bittiming_const &&
-	     nla_put(skb, IFLA_CAN_DATA_BITTIMING_CONST,
-		     sizeof(*priv->fd.data_bittiming_const),
-		     priv->fd.data_bittiming_const)) ||
+	    can_bittiming_const_fill_info(skb, IFLA_CAN_DATA_BITTIMING_CONST,
+					  priv->fd.data_bittiming_const) ||
 
 	    (priv->termination_const &&
 	     (nla_put_u16(skb, IFLA_CAN_TERMINATION, priv->termination) ||

-- 
2.49.1

[PATCH 17/21] can: netlink: add can_bitrate_const_fill_info()

[Vincent Mailhol]

Add can_bitrate_const_fill_info() to factorise the logic when filling
the bitrate constant information for Classical CAN and CAN FD. This
function will be reused later on for CAN XL.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 drivers/net/can/dev/netlink.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index cf9bf8eb1ae8e5c6b7910d8c9935cad73b78ef40..092031693ce9f55f2c396fc7de8336932f6f5516 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -569,6 +569,15 @@ static int can_bittiming_const_fill_info(struct sk_buff *skb,
 			sizeof(*bittiming_const), bittiming_const);
 }
 
+static int can_bitrate_const_fill_info(struct sk_buff *skb,
+				       int ifla_can_bitrate_const,
+				       const u32 *bitrate_const, unsigned int cnt)
+{
+	return bitrate_const &&
+		nla_put(skb, ifla_can_bitrate_const,
+			sizeof(*bitrate_const) * cnt, bitrate_const);
+}
+
 static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
 {
 	struct nlattr *nest;
@@ -679,17 +688,13 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 		      priv->termination_const_cnt,
 		      priv->termination_const))) ||
 
-	    (priv->bitrate_const &&
-	     nla_put(skb, IFLA_CAN_BITRATE_CONST,
-		     sizeof(*priv->bitrate_const) *
-		     priv->bitrate_const_cnt,
-		     priv->bitrate_const)) ||
+	    can_bitrate_const_fill_info(skb, IFLA_CAN_BITRATE_CONST,
+					priv->bitrate_const,
+					priv->bitrate_const_cnt) ||
 
-	    (priv->fd.data_bitrate_const &&
-	     nla_put(skb, IFLA_CAN_DATA_BITRATE_CONST,
-		     sizeof(*priv->fd.data_bitrate_const) *
-		     priv->fd.data_bitrate_const_cnt,
-		     priv->fd.data_bitrate_const)) ||
+	    can_bitrate_const_fill_info(skb, IFLA_CAN_DATA_BITRATE_CONST,
+					priv->fd.data_bitrate_const,
+					priv->fd.data_bitrate_const_cnt) ||
 
 	    (nla_put(skb, IFLA_CAN_BITRATE_MAX,
 		     sizeof(priv->bitrate_max),

-- 
2.49.1

[PATCH 18/21] can: netlink: make can_tdc_fill_info() FD agnostic

[Vincent Mailhol]

can_tdc_fill_info() depends on some variables which are specific to CAN
FD. Move these to the function parameters list so that, later on, this
function can be reused for the CAN XL TDC.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
Changelog:

  RFC -> v1:

   - Just pass the IFLA index instead of passing each argument
     individually. Instead, derive these as local variables depending
     on whethe the IFLA index is IFLA_CAN_TDC or IFLA_CAN_XL_TDC.
---
 drivers/net/can/dev/netlink.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 092031693ce9f55f2c396fc7de8336932f6f5516..2784cacd7adf5e69f24c110e749a651b188f69f4 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -578,21 +578,34 @@ static int can_bitrate_const_fill_info(struct sk_buff *skb,
 			sizeof(*bitrate_const) * cnt, bitrate_const);
 }
 
-static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
+static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev,
+			     int ifla_can_tdc)
 {
-	struct nlattr *nest;
 	struct can_priv *priv = netdev_priv(dev);
-	struct can_tdc *tdc = &priv->fd.tdc;
-	const struct can_tdc_const *tdc_const = priv->fd.tdc_const;
+	struct data_bittiming_params *dbt_params;
+	const struct can_tdc_const *tdc_const;
+	struct can_tdc *tdc;
+	struct nlattr *nest;
+	bool tdc_is_enabled, tdc_manual;
+
+	if (ifla_can_tdc == IFLA_CAN_TDC) {
+		dbt_params = &priv->fd;
+		tdc_is_enabled = can_fd_tdc_is_enabled(priv);
+		tdc_manual = priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL;
+	} else {
+		WARN_ON(1); /* Place holder for CAN XL */
+	}
+	tdc_const = dbt_params->tdc_const;
+	tdc = &dbt_params->tdc;
 
 	if (!tdc_const)
 		return 0;
 
-	nest = nla_nest_start(skb, IFLA_CAN_TDC);
+	nest = nla_nest_start(skb, ifla_can_tdc);
 	if (!nest)
 		return -EMSGSIZE;
 
-	if (priv->ctrlmode_supported & CAN_CTRLMODE_TDC_MANUAL &&
+	if (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;
@@ -604,15 +617,15 @@ static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev)
 	     nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MAX, tdc_const->tdcf_max)))
 		goto err_cancel;
 
-	if (can_fd_tdc_is_enabled(priv)) {
+	if (tdc_is_enabled) {
 		u32 tdcv;
 		int err = -EINVAL;
 
-		if (priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL) {
+		if (tdc_manual) {
 			tdcv = tdc->tdcv;
 			err = 0;
-		} else if (priv->fd.do_get_auto_tdcv) {
-			err = priv->fd.do_get_auto_tdcv(dev, &tdcv);
+		} else if (dbt_params->do_get_auto_tdcv) {
+			err = dbt_params->do_get_auto_tdcv(dev, &tdcv);
 		}
 		if (!err && nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdcv))
 			goto err_cancel;
@@ -700,7 +713,7 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 		     sizeof(priv->bitrate_max),
 		     &priv->bitrate_max)) ||
 
-	    can_tdc_fill_info(skb, dev) ||
+	    can_tdc_fill_info(skb, dev, IFLA_CAN_TDC) ||
 
 	    can_ctrlmode_ext_fill_info(skb, priv)
 	    )

-- 
2.49.1

[PATCH 19/21] can: calc_bittiming: make can_calc_tdco() FD agnostic

[Vincent Mailhol]

can_calc_tdco() uses the CAN_CTRLMODE_FD_TDC_MASK and
CAN_CTRLMODE_TDC_AUTO macros making it specific to CAN FD. Add the tdc
mask to the function parameter list. The value of the tdc auto flag
can then be derived from that mask and stored in a local variable.
This way, the function becomes CAN FD agnostic and can be reused later
on for the CAN XL TDC.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
Changelog:

  RFC -> v1:

   - new patch. I overlooked this in the RFC and the CAN XL's TDC was
     broken because of that.
---
 drivers/net/can/dev/calc_bittiming.c | 10 ++++++----
 drivers/net/can/dev/netlink.c        |  2 +-
 include/linux/can/bittiming.h        |  4 ++--
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
index a94bd67c670c4801344e1fed6372d0182c46271f..394d6974f48151230510d7f43c80d75e1429dd37 100644
--- a/drivers/net/can/dev/calc_bittiming.c
+++ b/drivers/net/can/dev/calc_bittiming.c
@@ -173,13 +173,15 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 
 void can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const,
 		   const struct can_bittiming *dbt,
-		   u32 *ctrlmode, u32 ctrlmode_supported)
+		   u32 tdc_mask, u32 *ctrlmode, u32 ctrlmode_supported)
 
 {
-	if (!tdc_const || !(ctrlmode_supported & CAN_CTRLMODE_TDC_AUTO))
+	u32 tdc_auto = tdc_mask & CAN_CTRLMODE_TDC_AUTO_MASK;
+
+	if (!tdc_const || !(ctrlmode_supported & tdc_auto))
 		return;
 
-	*ctrlmode &= ~CAN_CTRLMODE_FD_TDC_MASK;
+	*ctrlmode &= ~tdc_mask;
 
 	/* As specified in ISO 11898-1 section 11.3.3 "Transmitter
 	 * delay compensation" (TDC) is only applicable if data BRP is
@@ -193,6 +195,6 @@ void can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const,
 		if (sample_point_in_tc < tdc_const->tdco_min)
 			return;
 		tdc->tdco = min(sample_point_in_tc, tdc_const->tdco_max);
-		*ctrlmode |= CAN_CTRLMODE_TDC_AUTO;
+		*ctrlmode |= tdc_auto;
 	}
 }
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 2784cacd7adf5e69f24c110e749a651b188f69f4..d1c54c7dd333d229f03fc653d61a21a48f8c8865 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -334,7 +334,7 @@ static int can_dbt_changelink(struct net_device *dev, struct nlattr *data[],
 		 * do calculation
 		 */
 		can_calc_tdco(&dbt_params->tdc, dbt_params->tdc_const, &dbt,
-			      &priv->ctrlmode, priv->ctrlmode_supported);
+			      tdc_mask, &priv->ctrlmode, priv->ctrlmode_supported);
 	} /* else: both CAN_CTRLMODE_TDC_{AUTO,MANUAL} are explicitly
 	   * turned off. TDC is disabled: do nothing
 	   */
diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
index 71f839c3f0325b2a496a4bc447044a4853541338..d30816dd93c7082c774ca4c01ee42465cd042ca0 100644
--- a/include/linux/can/bittiming.h
+++ b/include/linux/can/bittiming.h
@@ -135,7 +135,7 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 
 void can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const,
 		   const struct can_bittiming *dbt,
-		   u32 *ctrlmode, u32 ctrlmode_supported);
+		   u32 tdc_mask, u32 *ctrlmode, u32 ctrlmode_supported);
 #else /* !CONFIG_CAN_CALC_BITTIMING */
 static inline int
 can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
@@ -148,7 +148,7 @@ can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 static inline void
 can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const,
 	      const struct can_bittiming *dbt,
-	      u32 *ctrlmode, u32 ctrlmode_supported)
+	      u32 tdc_mask, u32 *ctrlmode, u32 ctrlmode_supported)
 {
 }
 #endif /* CONFIG_CAN_CALC_BITTIMING */

-- 
2.49.1

[PATCH 20/21] can: dev: add can_get_ctrlmode_str()

[Vincent Mailhol]

In an effort to give more human readable messages when errors occur
because of conflicting options, it can be useful to convert the CAN
control mode flags into text.

Add a function which converts the first set CAN control mode into a
human readable string. The reason to only convert the first one is to
simplify edge cases: imagine that there are several invalid control
modes, we would just return the first invalid one to the user, thus
not having to handle complex string concatenation. The user can then
solve the first problem, call the netlink interface again and see the
next issue.

People who wish to enumerate all the control modes can still do so by,
for example, using this new function in a for_each_set_bit() loop.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 drivers/net/can/dev/dev.c | 33 +++++++++++++++++++++++++++++++++
 include/linux/can/dev.h   |  2 ++
 2 files changed, 35 insertions(+)

diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index 3913971125de0ab16b4ad9f36712954141014ddf..d9368ca828fffbda45a19af78e4f8271acfba6c4 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -88,6 +88,39 @@ const char *can_get_state_str(const enum can_state state)
 }
 EXPORT_SYMBOL_GPL(can_get_state_str);
 
+const char *can_get_ctrlmode_str(u32 ctrlmode)
+{
+	switch (ctrlmode & ~(ctrlmode - 1)) {
+	case 0:
+		return "none";
+	case CAN_CTRLMODE_LOOPBACK:
+		return "loopback";
+	case CAN_CTRLMODE_LISTENONLY:
+		return "listen-only";
+	case CAN_CTRLMODE_3_SAMPLES:
+		return "triple-sampling";
+	case CAN_CTRLMODE_ONE_SHOT:
+		return "one-shot";
+	case CAN_CTRLMODE_BERR_REPORTING:
+		return "berr-reporting";
+	case CAN_CTRLMODE_FD:
+		return "fd";
+	case CAN_CTRLMODE_PRESUME_ACK:
+		return "presume-ack";
+	case CAN_CTRLMODE_FD_NON_ISO:
+		return "fd-non-iso";
+	case CAN_CTRLMODE_CC_LEN8_DLC:
+		return "cc-len8-dlc";
+	case CAN_CTRLMODE_TDC_AUTO:
+		return "fd-tdc-auto";
+	case CAN_CTRLMODE_TDC_MANUAL:
+		return "fd-tdc-manual";
+	default:
+		return "<unknown>";
+	}
+}
+EXPORT_SYMBOL_GPL(can_get_ctrlmode_str);
+
 static enum can_state can_state_err_to_state(u16 err)
 {
 	if (err < CAN_ERROR_WARNING_THRESHOLD)
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 55aaadaacf68f940fa1b71f7c438e68b84080292..27690a8bea44d334bf1ac8f779ae36189e0e1493 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -159,6 +159,8 @@ int can_restart_now(struct net_device *dev);
 void can_bus_off(struct net_device *dev);
 
 const char *can_get_state_str(const enum can_state state);
+const char *can_get_ctrlmode_str(u32 ctrlmode);
+
 void can_state_get_by_berr_counter(const struct net_device *dev,
 				   const struct can_berr_counter *bec,
 				   enum can_state *tx_state,

-- 
2.49.1

[PATCH 21/21] can: netlink: add userland error messages

[Vincent Mailhol]

Use NL_SET_ERR_MSG() and NL_SET_ERR_MSG_FMT() to return meaningful
error messages to the userland whenever a -EOPNOTSUPP error is
returned due to a failed validation of the CAN netlink arguments.

Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
---
 drivers/net/can/dev/netlink.c | 82 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 62 insertions(+), 20 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index d1c54c7dd333d229f03fc653d61a21a48f8c8865..f39f6310312aa404e025fb7ff11e483a3d9f9f20 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -63,15 +63,23 @@ static int can_validate_tdc(struct nlattr *data_tdc,
 	bool tdc_auto = tdc_flags & CAN_CTRLMODE_TDC_AUTO_MASK;
 	int err;
 
-	/* CAN_CTRLMODE_TDC_{AUTO,MANUAL} are mutually exclusive */
-	if (tdc_auto && tdc_manual)
+	if (tdc_auto && tdc_manual) {
+		NL_SET_ERR_MSG(extack,
+			       "TDC manual and auto modes are mutually exclusive");
 		return -EOPNOTSUPP;
+	}
 
 	/* If one of the CAN_CTRLMODE_TDC_* flag is set then TDC
 	 * must be set and vice-versa
 	 */
-	if ((tdc_auto || tdc_manual) != !!data_tdc)
+	if ((tdc_auto || tdc_manual) && !data_tdc) {
+		NL_SET_ERR_MSG(extack, "TDC parameters are missing");
 		return -EOPNOTSUPP;
+	}
+	if (!(tdc_auto || tdc_manual) && data_tdc) {
+		NL_SET_ERR_MSG(extack, "TDC mode (auto or manual) is missing");
+		return -EOPNOTSUPP;
+	}
 
 	/* If providing TDC parameters, at least TDCO is needed. TDCV
 	 * is needed if and only if CAN_CTRLMODE_TDC_MANUAL is set
@@ -85,15 +93,23 @@ static int can_validate_tdc(struct nlattr *data_tdc,
 			return err;
 
 		if (tb_tdc[IFLA_CAN_TDC_TDCV]) {
-			if (tdc_auto)
+			if (tdc_auto) {
+				NL_SET_ERR_MSG(extack,
+					       "TDCV is incompatible with TDC auto mode");
 				return -EOPNOTSUPP;
+			}
 		} else {
-			if (tdc_manual)
+			if (tdc_manual) {
+				NL_SET_ERR_MSG(extack,
+					       "TDC manual mode requires TDCV");
 				return -EOPNOTSUPP;
+			}
 		}
 
-		if (!tb_tdc[IFLA_CAN_TDC_TDCO])
+		if (!tb_tdc[IFLA_CAN_TDC_TDCO]) {
+			NL_SET_ERR_MSG(extack, "TDCO is missing");
 			return -EOPNOTSUPP;
+		}
 	}
 
 	return 0;
@@ -104,6 +120,7 @@ static int can_validate_databittiming(struct nlattr *data[],
 				      int ifla_can_data_bittiming, u32 flags)
 {
 	struct nlattr *data_tdc;
+	const char *type;
 	u32 tdc_flags;
 	bool is_on;
 	int err;
@@ -112,18 +129,31 @@ static int can_validate_databittiming(struct nlattr *data[],
 		data_tdc = data[IFLA_CAN_TDC];
 		tdc_flags = flags & CAN_CTRLMODE_FD_TDC_MASK;
 		is_on = flags & CAN_CTRLMODE_FD;
+		type = "FD";
 	} else {
 		WARN_ON(1); /* Place holder for CAN XL */
 	}
 
 	if (is_on) {
-		if (!data[IFLA_CAN_BITTIMING] || !data[ifla_can_data_bittiming])
+		if (!data[IFLA_CAN_BITTIMING] || !data[ifla_can_data_bittiming]) {
+			NL_SET_ERR_MSG_FMT(extack,
+					   "Provide both nominal and %s data bittiming",
+					   type);
 			return -EOPNOTSUPP;
-	}
-
-	if (data[ifla_can_data_bittiming] || data_tdc) {
-		if (!is_on)
+		}
+	} else {
+		if (data[ifla_can_data_bittiming]) {
+			NL_SET_ERR_MSG_FMT(extack,
+					   "%s data bittiming requires CAN %s",
+					   type, type);
 			return -EOPNOTSUPP;
+		}
+		if (data_tdc) {
+			NL_SET_ERR_MSG_FMT(extack,
+					   "%s TDC requires CAN %s",
+					   type, type);
+			return -EOPNOTSUPP;
+		}
 	}
 
 	err = can_validate_bittiming(data, extack, ifla_can_data_bittiming);
@@ -170,8 +200,7 @@ static int can_ctrlmode_changelink(struct net_device *dev,
 {
 	struct can_priv *priv = netdev_priv(dev);
 	struct can_ctrlmode *cm;
-	u32 maskedflags;
-	u32 ctrlstatic;
+	u32 ctrlstatic, maskedflags, notsupp, ctrlstatic_missing;
 
 	if (!data[IFLA_CAN_CTRLMODE])
 		return 0;
@@ -181,20 +210,28 @@ static int can_ctrlmode_changelink(struct net_device *dev,
 		return -EBUSY;
 
 	cm = nla_data(data[IFLA_CAN_CTRLMODE]);
-	maskedflags = cm->flags & cm->mask;
 	ctrlstatic = can_get_static_ctrlmode(priv);
+	maskedflags = cm->flags & cm->mask;
+	notsupp = maskedflags & ~(priv->ctrlmode_supported | ctrlstatic);
+	ctrlstatic_missing = (maskedflags & ctrlstatic) ^ ctrlstatic;
 
-	/* check whether provided bits are allowed to be passed */
-	if (maskedflags & ~(priv->ctrlmode_supported | ctrlstatic))
+	if (notsupp) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "requested control mode %s not supported",
+				   can_get_ctrlmode_str(notsupp));
 		return -EOPNOTSUPP;
+	}
 
 	/* do not check for static fd-non-iso if 'fd' is disabled */
 	if (!(maskedflags & CAN_CTRLMODE_FD))
 		ctrlstatic &= ~CAN_CTRLMODE_FD_NON_ISO;
 
-	/* make sure static options are provided by configuration */
-	if ((maskedflags & ctrlstatic) != ctrlstatic)
+	if (ctrlstatic_missing) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "missing required %s static control mode",
+				   can_get_ctrlmode_str(ctrlstatic_missing));
 		return -EOPNOTSUPP;
+	}
 
 	/* If a top dependency flag is provided, reset all its dependencies */
 	if (cm->mask & CAN_CTRLMODE_FD)
@@ -227,8 +264,10 @@ static int can_tdc_changelink(struct data_bittiming_params *dbt_params,
 	const struct can_tdc_const *tdc_const = dbt_params->tdc_const;
 	int err;
 
-	if (!tdc_const)
+	if (!tdc_const) {
+		NL_SET_ERR_MSG(extack, "The device does not support TDC");
 		return -EOPNOTSUPP;
+	}
 
 	err = nla_parse_nested(tb_tdc, IFLA_CAN_TDC_MAX, nla,
 			       can_tdc_policy, extack);
@@ -443,8 +482,11 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		const unsigned int num_term = priv->termination_const_cnt;
 		unsigned int i;
 
-		if (!priv->do_set_termination)
+		if (!priv->do_set_termination) {
+			NL_SET_ERR_MSG(extack,
+				       "Termination is not configurable on this device");
 			return -EOPNOTSUPP;
+		}
 
 		/* check whether given value is supported by the interface */
 		for (i = 0; i < num_term; i++) {

-- 
2.49.1

Re: [PATCH 00/21] can: netlink: preparation before introduction of CAN XL step 2/2

[Vincent Mailhol]

On 03/09/2025 à 17:49, Vincent Mailhol wrote:

(...)

> The follow up series which introduces CAN XL is nearly completed but
> will be sent only once this one is approved: one thing at a time, I do
> not want to overwhelm people (including myself).

If you want a preview of the full CAN XL, you can have a look at my work in
progress here:

https://git.kernel.org/pub/scm/linux/kernel/git/mailhol/linux.git/
log/?h=b4/canxl-netlink
https://git.kernel.org/pub/scm/linux/kernel/git/mailhol/iproute2-next.git/log/?h=canxl-netlink

The kernel part is nearly completed, but I am still playing some whack-a-mole to
find potential gaps in the configuration validation. I also need to rewrite or
fine tune the commit description.

The iproute2 part is still under development. It has the PWM interface but I
have not added all the control modes yet.

Regardless, the two links above are just an FYI. Beware that there will be some
random force pushes without any notice. You can play with it but it is
*not* opened for comments until the preparation series is approved.

Looking forward for your comments on this CAN XL preparation series, it took me
a fair amount of effort :)


Yours sincerely,
Vincent Mailhol

Re: [PATCH 00/21] can: netlink: preparation before introduction of CAN XL step 2/2

[Oliver Hartkopp]

On 03.09.25 11:26, Vincent Mailhol wrote:
> On 03/09/2025 à 17:49, Vincent Mailhol wrote:
> 
> (...)
> 
>> The follow up series which introduces CAN XL is nearly completed but
>> will be sent only once this one is approved: one thing at a time, I do
>> not want to overwhelm people (including myself).
> 
> If you want a preview of the full CAN XL, you can have a look at my work in
> progress here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/mailhol/linux.git/
> log/?h=b4/canxl-netlink
> https://git.kernel.org/pub/scm/linux/kernel/git/mailhol/iproute2-next.git/log/?h=canxl-netlink
> 
> The kernel part is nearly completed, but I am still playing some whack-a-mole to
> find potential gaps in the configuration validation. I also need to rewrite or
> fine tune the commit description.
> 
> The iproute2 part is still under development. It has the PWM interface but I
> have not added all the control modes yet.
> 

Thanks Vincent!

The repos are very helpful.

With "missing" control modes you refer to CAN_CTRLMODE_XL_ERR_SIGNAL, 
CAN_CTRLMODE_XL_RRS and CAN_CTRLMODE_RESTRICTED, right?

Best regards,
Oliver

> Regardless, the two links above are just an FYI. Beware that there will be some
> random force pushes without any notice. You can play with it but it is
> *not* opened for comments until the preparation series is approved.
> 
> Looking forward for your comments on this CAN XL preparation series, it took me
> a fair amount of effort :)
> 
> 
> Yours sincerely,
> Vincent Mailhol

Re: [PATCH 07/21] can: netlink: remove comment in can_validate()

[Oliver Hartkopp]

Hi Vincent,

On 03.09.25 10:50, Vincent Mailhol wrote:
> The comment in can_validate() is just paraphrasing the code. When
> adding CAN XL, updating this comment would add some overhead work for
> no clear benefit.

I generally see that the code introduced by yourself has nearly no comments.

E.g. if you look at the [PATCH 12/21] can: netlink: add 
can_ctrlmode_changelink() the comments introduced by myself document the 
different steps as we had problems with the complexity there and it was 
hard to review either.

I would like to motivate you to generally add more comments.

When people (like me) look into that code that they haven't written 
themselves and there is not even a hint of "what's the idea of what we 
are doing here" then the code is hard to follow and to review.

We definitely don't need a full blown documentation on top of each 
function. But I like this comment you want to remove here and I would 
like to have more of it, so that people get an impression what they will 
see in the following code.

Best regards,
Oliver

> 
> Now that the function has been refactored and split into smaller
> pieces, let the code speak for itself. Remove the comment.
> 
> Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
> ---
>   drivers/net/can/dev/netlink.c | 7 -------
>   1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
> index f7b12057bc9c6c286aa0c4341d565a497254296d..6ea629331d20483c5e70567eb1be226a3b09882c 100644
> --- a/drivers/net/can/dev/netlink.c
> +++ b/drivers/net/can/dev/netlink.c
> @@ -143,13 +143,6 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
>   	u32 flags = 0;
>   	int err;
>   
> -	/* Make sure that valid CAN FD configurations always consist of
> -	 * - nominal/arbitration bittiming
> -	 * - data bittiming
> -	 * - control mode with CAN_CTRLMODE_FD set
> -	 * - TDC parameters are coherent (details below)
> -	 */
> -
>   	if (!data)
>   		return 0;
>   
> 

Re: [PATCH 00/21] can: netlink: preparation before introduction of CAN XL step 2/2

[Vincent Mailhol]

On 04/09/2025 at 15:36, Oliver Hartkopp wrote:
> On 03.09.25 11:26, Vincent Mailhol wrote:
>> On 03/09/2025 à 17:49, Vincent Mailhol wrote:
>>
>> (...)
>>
>>> The follow up series which introduces CAN XL is nearly completed but
>>> will be sent only once this one is approved: one thing at a time, I do
>>> not want to overwhelm people (including myself).
>>
>> If you want a preview of the full CAN XL, you can have a look at my work in
>> progress here:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/mailhol/linux.git/
>> log/?h=b4/canxl-netlink
>> https://git.kernel.org/pub/scm/linux/kernel/git/mailhol/iproute2-next.git/
>> log/?h=canxl-netlink
>>
>> The kernel part is nearly completed, but I am still playing some whack-a-mole to
>> find potential gaps in the configuration validation. I also need to rewrite or
>> fine tune the commit description.
>>
>> The iproute2 part is still under development. It has the PWM interface but I
>> have not added all the control modes yet.
>>
> 
> Thanks Vincent!
> 
> The repos are very helpful.
> 
> With "missing" control modes you refer to CAN_CTRLMODE_XL_ERR_SIGNAL,
> CAN_CTRLMODE_XL_RRS and CAN_CTRLMODE_RESTRICTED, right?

Yes, I only added the CAN_CTRLMODE_XL_TMS so far in iproute2. The kernel has all
of the four flags (but because I did not finish testing, I highly suspect that
there are still some bugs somewhere).

Concerning the CAN_CTRLMODE_XL_RRS, I am not sure if that one is needed. I still
have it in my WIP series but I am recently considering to remove it. The reason
is that when reading ISO 11898-1 having RRS configurable looks mandatory to me.

In the logical Link control (LLC) this RRS bit is named FTYP (for Frame Type).
For example, CiA only mentions FTYP in their CAN XL knowledge page:

  https://www.can-cia.org/can-knowledge/can-xl

Contrarily to CAN FD's RRS which is indeed specified as being dominant and which
is just ignored in the LLC, the CAN XL FTYP/RRS is part of the LLC interface and
is meant to be configurable.

Nothing in the standard tells us that this should be a dominant bit. I think
your intention was to add CAN_CTRLMODE_XL_RRS as a quirk for the devices which
expose this flag. But as far as I can see, it seems that a device which does not
expose it is just not compliant.

If some day a device which can not set the FTYP/RRS flag appears in the wild,
then maybe we can add a flag which would specify that RRS is not configurable
(opposite logic as what you suggested). But as long as such a device do not
exist, it is better to add nothing.


Yours sincerely,
Vincent Mailhol

Re: [PATCH 07/21] can: netlink: remove comment in can_validate()

[Vincent Mailhol]

On 04/09/2025 at 15:51, Oliver Hartkopp wrote:
> Hi Vincent,
> 
> On 03.09.25 10:50, Vincent Mailhol wrote:
>> The comment in can_validate() is just paraphrasing the code. When
>> adding CAN XL, updating this comment would add some overhead work for
>> no clear benefit.
> 
> I generally see that the code introduced by yourself has nearly no comments.

I tend to disagree. While it is true that I added no C-style comment blocks, I
added a ton of error messages which I would argue are documentation.

For example, this code:

	/* If one of the CAN_CTRLMODE_TDC_* flag is set then TDC
	 * must be set and vice-versa
	 */
	if ((tdc_auto || tdc_manual) != !!data_tdc)
		return -EOPNOTSUPP;

was transformed into:

	/* If one of the CAN_CTRLMODE_TDC_* flag is set then TDC
	 * must be set and vice-versa
	 */
	if ((tdc_auto || tdc_manual) && !data_tdc) {
		NL_SET_ERR_MSG(extack, "TDC parameters are missing");
		return -EOPNOTSUPP;
	}
	if (!(tdc_auto || tdc_manual) && data_tdc) {
		NL_SET_ERR_MSG(extack, "TDC mode (auto or manual) is missing");
		return -EOPNOTSUPP;
	}

Which I think is a huge improvement on the documentation. And this has real
value because the user do not have to look at the source code anymore to
understand why an

  ip link set can ...

returned an error.

> E.g. if you look at the [PATCH 12/21] can: netlink: add
> can_ctrlmode_changelink() the comments introduced by myself document the
> different steps as we had problems with the complexity there and it was hard to
> review either.

Those comments are still here.

> I would like to motivate you to generally add more comments.
This is a refactoring series. I kept all existing comments except of one and
then added a more comments in the form of error message. I am not adding code,
just moving it around, so I do not really get why I should be adding even more
comments to the existing code.

> When people (like me) look into that code that they haven't written themselves
> and there is not even a hint of "what's the idea of what we are doing here" then
> the code is hard to follow and to review.

Is this an issue in my code refactoring or an issue with the existing code?

> We definitely don't need a full blown documentation on top of each function. But
> I like this comment you want to remove here and I would like to have more of it,
> so that people get an impression what they will see in the following code.


Yours sincerely,
Vincent Mailhol

Re: [PATCH 06/21] can: netlink: add can_validate_databittiming()

[kernel test robot]

Hi Vincent,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 2fd4161d0d2547650d9559d57fc67b4e0a26a9e3]

url:    https://github.com/intel-lab-lkp/linux/commits/Vincent-Mailhol/can-dev-move-struct-data_bittiming_params-to-linux-can-bittiming-h/20250903-170807
base:   2fd4161d0d2547650d9559d57fc67b4e0a26a9e3
patch link:    https://lore.kernel.org/r/20250903-canxl-netlink-prep-v1-6-904bd6037cd9%40kernel.org
patch subject: [PATCH 06/21] can: netlink: add can_validate_databittiming()
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20250905/202509050259.NjPdQyAD-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250905/202509050259.NjPdQyAD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509050259.NjPdQyAD-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/can/dev/netlink.c:111:6: warning: variable 'is_on' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
     111 |         if (ifla_can_data_bittiming == IFLA_CAN_DATA_BITTIMING) {
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/can/dev/netlink.c:119:6: note: uninitialized use occurs here
     119 |         if (is_on) {
         |             ^~~~~
   drivers/net/can/dev/netlink.c:111:2: note: remove the 'if' if its condition is always true
     111 |         if (ifla_can_data_bittiming == IFLA_CAN_DATA_BITTIMING) {
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     112 |                 data_tdc = data[IFLA_CAN_TDC];
     113 |                 tdc_flags = flags & CAN_CTRLMODE_FD_TDC_MASK;
     114 |                 is_on = flags & CAN_CTRLMODE_FD;
     115 |         } else {
         |           ~~~~~~
     116 |                 WARN_ON(1); /* Place holder for CAN XL */
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     117 |         }
         |         ~
   drivers/net/can/dev/netlink.c:108:12: note: initialize the variable 'is_on' to silence this warning
     108 |         bool is_on;
         |                   ^
         |                    = 0
   1 warning generated.


vim +111 drivers/net/can/dev/netlink.c

   101	
   102	static int can_validate_databittiming(struct nlattr *data[],
   103					      struct netlink_ext_ack *extack,
   104					      int ifla_can_data_bittiming, u32 flags)
   105	{
   106		struct nlattr *data_tdc;
   107		u32 tdc_flags;
   108		bool is_on;
   109		int err;
   110	
 > 111		if (ifla_can_data_bittiming == IFLA_CAN_DATA_BITTIMING) {
   112			data_tdc = data[IFLA_CAN_TDC];
   113			tdc_flags = flags & CAN_CTRLMODE_FD_TDC_MASK;
   114			is_on = flags & CAN_CTRLMODE_FD;
   115		} else {
   116			WARN_ON(1); /* Place holder for CAN XL */
   117		}
   118	
   119		if (is_on) {
   120			if (!data[IFLA_CAN_BITTIMING] || !data[ifla_can_data_bittiming])
   121				return -EOPNOTSUPP;
   122		}
   123	
   124		if (data[ifla_can_data_bittiming] || data_tdc) {
   125			if (!is_on)
   126				return -EOPNOTSUPP;
   127		}
   128	
   129		err = can_validate_bittiming(data, extack, ifla_can_data_bittiming);
   130		if (err)
   131			return err;
   132	
   133		err = can_validate_tdc(data_tdc, extack, tdc_flags);
   134		if (err)
   135			return err;
   136	
   137		return 0;
   138	}
   139	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 11/21] can: netlink: add can_dtb_changelink()

[kernel test robot]

Hi Vincent,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 2fd4161d0d2547650d9559d57fc67b4e0a26a9e3]

url:    https://github.com/intel-lab-lkp/linux/commits/Vincent-Mailhol/can-dev-move-struct-data_bittiming_params-to-linux-can-bittiming-h/20250903-170807
base:   2fd4161d0d2547650d9559d57fc67b4e0a26a9e3
patch link:    https://lore.kernel.org/r/20250903-canxl-netlink-prep-v1-11-904bd6037cd9%40kernel.org
patch subject: [PATCH 11/21] can: netlink: add can_dtb_changelink()
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20250905/202509050404.ZLQknagH-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250905/202509050404.ZLQknagH-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509050404.ZLQknagH-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/net/can/dev/netlink.c:111:6: warning: variable 'is_on' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
     111 |         if (ifla_can_data_bittiming == IFLA_CAN_DATA_BITTIMING) {
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/can/dev/netlink.c:119:6: note: uninitialized use occurs here
     119 |         if (is_on) {
         |             ^~~~~
   drivers/net/can/dev/netlink.c:111:2: note: remove the 'if' if its condition is always true
     111 |         if (ifla_can_data_bittiming == IFLA_CAN_DATA_BITTIMING) {
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     112 |                 data_tdc = data[IFLA_CAN_TDC];
     113 |                 tdc_flags = flags & CAN_CTRLMODE_FD_TDC_MASK;
     114 |                 is_on = flags & CAN_CTRLMODE_FD;
     115 |         } else {
         |           ~~~~~~
     116 |                 WARN_ON(1); /* Place holder for CAN XL */
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     117 |         }
         |         ~
   drivers/net/can/dev/netlink.c:108:12: note: initialize the variable 'is_on' to silence this warning
     108 |         bool is_on;
         |                   ^
         |                    = 0
>> drivers/net/can/dev/netlink.c:227:6: warning: variable 'data_bittiming' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
     227 |         if (fd) {
         |             ^~
   drivers/net/can/dev/netlink.c:236:7: note: uninitialized use occurs here
     236 |         if (!data_bittiming)
         |              ^~~~~~~~~~~~~~
   drivers/net/can/dev/netlink.c:227:2: note: remove the 'if' if its condition is always true
     227 |         if (fd) {
         |         ^~~~~~~
     228 |                 data_bittiming = data[IFLA_CAN_DATA_BITTIMING];
     229 |                 data_tdc = data[IFLA_CAN_TDC];
     230 |                 dbt_params = &priv->fd;
     231 |                 tdc_mask = CAN_CTRLMODE_FD_TDC_MASK;
     232 |         } else {
         |           ~~~~~~
     233 |                 WARN_ON(1); /* Place holder for CAN XL */
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     234 |         }
         |         ~
   drivers/net/can/dev/netlink.c:219:31: note: initialize the variable 'data_bittiming' to silence this warning
     219 |         struct nlattr *data_bittiming, *data_tdc;
         |                                      ^
         |                                       = NULL
   2 warnings generated.


vim +227 drivers/net/can/dev/netlink.c

   215	
   216	static int can_dbt_changelink(struct net_device *dev, struct nlattr *data[],
   217				      bool fd, struct netlink_ext_ack *extack)
   218	{
   219		struct nlattr *data_bittiming, *data_tdc;
   220		struct can_priv *priv = netdev_priv(dev);
   221		struct data_bittiming_params *dbt_params;
   222		struct can_bittiming dbt;
   223		bool need_tdc_calc = false;
   224		u32 tdc_mask;
   225		int err;
   226	
 > 227		if (fd) {
   228			data_bittiming = data[IFLA_CAN_DATA_BITTIMING];
   229			data_tdc = data[IFLA_CAN_TDC];
   230			dbt_params = &priv->fd;
   231			tdc_mask = CAN_CTRLMODE_FD_TDC_MASK;
   232		} else {
   233			WARN_ON(1); /* Place holder for CAN XL */
   234		}
   235	
   236		if (!data_bittiming)
   237			return 0;
   238	
   239		/* Do not allow changing bittiming while running */
   240		if (dev->flags & IFF_UP)
   241			return -EBUSY;
   242	
   243		/* Calculate bittiming parameters based on data_bittiming_const
   244		 * if set, otherwise pass bitrate directly via do_set_bitrate().
   245		 * Bail out if neither is given.
   246		 */
   247		if (!dbt_params->data_bittiming_const && !dbt_params->do_set_data_bittiming &&
   248		    !dbt_params->data_bitrate_const)
   249			return -EOPNOTSUPP;
   250	
   251		memcpy(&dbt, nla_data(data_bittiming), sizeof(dbt));
   252		err = can_get_bittiming(dev, &dbt, dbt_params->data_bittiming_const,
   253					dbt_params->data_bitrate_const,
   254					dbt_params->data_bitrate_const_cnt, extack);
   255		if (err)
   256			return err;
   257	
   258		if (priv->bitrate_max && dbt.bitrate > priv->bitrate_max) {
   259			NL_SET_ERR_MSG_FMT(extack,
   260					   "CAN data bitrate %u bps surpasses transceiver capabilities of %u bps",
   261					   dbt.bitrate, priv->bitrate_max);
   262			return -EINVAL;
   263		}
   264	
   265		memset(&dbt_params->tdc, 0, sizeof(dbt_params->tdc));
   266		if (data[IFLA_CAN_CTRLMODE]) {
   267			struct can_ctrlmode *cm = nla_data(data[IFLA_CAN_CTRLMODE]);
   268	
   269			need_tdc_calc = !(cm->mask & tdc_mask);
   270		}
   271		if (data_tdc) {
   272			/* TDC parameters are provided: use them */
   273			err = can_tdc_changelink(dbt_params, data_tdc, extack);
   274			if (err) {
   275				priv->ctrlmode &= ~tdc_mask;
   276				return err;
   277			}
   278		} else if (need_tdc_calc) {
   279			/* Neither of TDC parameters nor TDC flags are provided:
   280			 * do calculation
   281			 */
   282			can_calc_tdco(&dbt_params->tdc, dbt_params->tdc_const, &dbt,
   283				      &priv->ctrlmode, priv->ctrlmode_supported);
   284		} /* else: both CAN_CTRLMODE_TDC_{AUTO,MANUAL} are explicitly
   285		   * turned off. TDC is disabled: do nothing
   286		   */
   287	
   288		memcpy(&dbt_params->data_bittiming, &dbt, sizeof(dbt));
   289	
   290		if (dbt_params->do_set_data_bittiming) {
   291			/* Finally, set the bit-timing registers */
   292			err = dbt_params->do_set_data_bittiming(dev);
   293			if (err)
   294				return err;
   295		}
   296	
   297		return 0;
   298	}
   299	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 18/21] can: netlink: make can_tdc_fill_info() FD agnostic

[kernel test robot]

Hi Vincent,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 2fd4161d0d2547650d9559d57fc67b4e0a26a9e3]

url:    https://github.com/intel-lab-lkp/linux/commits/Vincent-Mailhol/can-dev-move-struct-data_bittiming_params-to-linux-can-bittiming-h/20250903-170807
base:   2fd4161d0d2547650d9559d57fc67b4e0a26a9e3
patch link:    https://lore.kernel.org/r/20250903-canxl-netlink-prep-v1-18-904bd6037cd9%40kernel.org
patch subject: [PATCH 18/21] can: netlink: make can_tdc_fill_info() FD agnostic
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20250905/202509050541.1FKRbqOi-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250905/202509050541.1FKRbqOi-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202509050541.1FKRbqOi-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/net/can/dev/netlink.c:111:6: warning: variable 'is_on' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
     111 |         if (ifla_can_data_bittiming == IFLA_CAN_DATA_BITTIMING) {
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/can/dev/netlink.c:119:6: note: uninitialized use occurs here
     119 |         if (is_on) {
         |             ^~~~~
   drivers/net/can/dev/netlink.c:111:2: note: remove the 'if' if its condition is always true
     111 |         if (ifla_can_data_bittiming == IFLA_CAN_DATA_BITTIMING) {
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     112 |                 data_tdc = data[IFLA_CAN_TDC];
     113 |                 tdc_flags = flags & CAN_CTRLMODE_FD_TDC_MASK;
     114 |                 is_on = flags & CAN_CTRLMODE_FD;
     115 |         } else {
         |           ~~~~~~
     116 |                 WARN_ON(1); /* Place holder for CAN XL */
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     117 |         }
         |         ~
   drivers/net/can/dev/netlink.c:108:12: note: initialize the variable 'is_on' to silence this warning
     108 |         bool is_on;
         |                   ^
         |                    = 0
   drivers/net/can/dev/netlink.c:281:6: warning: variable 'data_bittiming' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
     281 |         if (fd) {
         |             ^~
   drivers/net/can/dev/netlink.c:290:7: note: uninitialized use occurs here
     290 |         if (!data_bittiming)
         |              ^~~~~~~~~~~~~~
   drivers/net/can/dev/netlink.c:281:2: note: remove the 'if' if its condition is always true
     281 |         if (fd) {
         |         ^~~~~~~
     282 |                 data_bittiming = data[IFLA_CAN_DATA_BITTIMING];
     283 |                 data_tdc = data[IFLA_CAN_TDC];
     284 |                 dbt_params = &priv->fd;
     285 |                 tdc_mask = CAN_CTRLMODE_FD_TDC_MASK;
     286 |         } else {
         |           ~~~~~~
     287 |                 WARN_ON(1); /* Place holder for CAN XL */
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     288 |         }
         |         ~
   drivers/net/can/dev/netlink.c:273:31: note: initialize the variable 'data_bittiming' to silence this warning
     273 |         struct nlattr *data_bittiming, *data_tdc;
         |                                      ^
         |                                       = NULL
>> drivers/net/can/dev/netlink.c:591:6: warning: variable 'dbt_params' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
     591 |         if (ifla_can_tdc == IFLA_CAN_TDC) {
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/net/can/dev/netlink.c:598:14: note: uninitialized use occurs here
     598 |         tdc_const = dbt_params->tdc_const;
         |                     ^~~~~~~~~~
   drivers/net/can/dev/netlink.c:591:2: note: remove the 'if' if its condition is always true
     591 |         if (ifla_can_tdc == IFLA_CAN_TDC) {
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     592 |                 dbt_params = &priv->fd;
     593 |                 tdc_is_enabled = can_fd_tdc_is_enabled(priv);
     594 |                 tdc_manual = priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL;
     595 |         } else {
         |           ~~~~~~
     596 |                 WARN_ON(1); /* Place holder for CAN XL */
         |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     597 |         }
         |         ~
   drivers/net/can/dev/netlink.c:585:42: note: initialize the variable 'dbt_params' to silence this warning
     585 |         struct data_bittiming_params *dbt_params;
         |                                                 ^
         |                                                  = NULL
   3 warnings generated.


vim +591 drivers/net/can/dev/netlink.c

   580	
   581	static int can_tdc_fill_info(struct sk_buff *skb, const struct net_device *dev,
   582				     int ifla_can_tdc)
   583	{
   584		struct can_priv *priv = netdev_priv(dev);
   585		struct data_bittiming_params *dbt_params;
   586		const struct can_tdc_const *tdc_const;
   587		struct can_tdc *tdc;
   588		struct nlattr *nest;
   589		bool tdc_is_enabled, tdc_manual;
   590	
 > 591		if (ifla_can_tdc == IFLA_CAN_TDC) {
   592			dbt_params = &priv->fd;
   593			tdc_is_enabled = can_fd_tdc_is_enabled(priv);
   594			tdc_manual = priv->ctrlmode & CAN_CTRLMODE_TDC_MANUAL;
   595		} else {
   596			WARN_ON(1); /* Place holder for CAN XL */
   597		}
   598		tdc_const = dbt_params->tdc_const;
   599		tdc = &dbt_params->tdc;
   600	
   601		if (!tdc_const)
   602			return 0;
   603	
   604		nest = nla_nest_start(skb, ifla_can_tdc);
   605		if (!nest)
   606			return -EMSGSIZE;
   607	
   608		if (tdc_manual &&
   609		    (nla_put_u32(skb, IFLA_CAN_TDC_TDCV_MIN, tdc_const->tdcv_min) ||
   610		     nla_put_u32(skb, IFLA_CAN_TDC_TDCV_MAX, tdc_const->tdcv_max)))
   611			goto err_cancel;
   612		if (nla_put_u32(skb, IFLA_CAN_TDC_TDCO_MIN, tdc_const->tdco_min) ||
   613		    nla_put_u32(skb, IFLA_CAN_TDC_TDCO_MAX, tdc_const->tdco_max))
   614			goto err_cancel;
   615		if (tdc_const->tdcf_max &&
   616		    (nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MIN, tdc_const->tdcf_min) ||
   617		     nla_put_u32(skb, IFLA_CAN_TDC_TDCF_MAX, tdc_const->tdcf_max)))
   618			goto err_cancel;
   619	
   620		if (tdc_is_enabled) {
   621			u32 tdcv;
   622			int err = -EINVAL;
   623	
   624			if (tdc_manual) {
   625				tdcv = tdc->tdcv;
   626				err = 0;
   627			} else if (dbt_params->do_get_auto_tdcv) {
   628				err = dbt_params->do_get_auto_tdcv(dev, &tdcv);
   629			}
   630			if (!err && nla_put_u32(skb, IFLA_CAN_TDC_TDCV, tdcv))
   631				goto err_cancel;
   632			if (nla_put_u32(skb, IFLA_CAN_TDC_TDCO, tdc->tdco))
   633				goto err_cancel;
   634			if (tdc_const->tdcf_max &&
   635			    nla_put_u32(skb, IFLA_CAN_TDC_TDCF, tdc->tdcf))
   636				goto err_cancel;
   637		}
   638	
   639		nla_nest_end(skb, nest);
   640		return 0;
   641	
   642	err_cancel:
   643		nla_nest_cancel(skb, nest);
   644		return -EMSGSIZE;
   645	}
   646	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 07/21] can: netlink: remove comment in can_validate()

[Oliver Hartkopp]

On 04.09.25 11:48, Vincent Mailhol wrote:
> On 04/09/2025 at 15:51, Oliver Hartkopp wrote:
>> Hi Vincent,
>>
>> On 03.09.25 10:50, Vincent Mailhol wrote:
>>> The comment in can_validate() is just paraphrasing the code. When
>>> adding CAN XL, updating this comment would add some overhead work for
>>> no clear benefit.
>>
>> I generally see that the code introduced by yourself has nearly no comments.
> 
> I tend to disagree. While it is true that I added no C-style comment blocks, I
> added a ton of error messages which I would argue are documentation.
> 
> For example, this code:
> 
> 	/* If one of the CAN_CTRLMODE_TDC_* flag is set then TDC
> 	 * must be set and vice-versa
> 	 */
> 	if ((tdc_auto || tdc_manual) != !!data_tdc)
> 		return -EOPNOTSUPP;
> 
> was transformed into:
> 
> 	/* If one of the CAN_CTRLMODE_TDC_* flag is set then TDC
> 	 * must be set and vice-versa
> 	 */
> 	if ((tdc_auto || tdc_manual) && !data_tdc) {
> 		NL_SET_ERR_MSG(extack, "TDC parameters are missing");
> 		return -EOPNOTSUPP;
> 	}
> 	if (!(tdc_auto || tdc_manual) && data_tdc) {
> 		NL_SET_ERR_MSG(extack, "TDC mode (auto or manual) is missing");
> 		return -EOPNOTSUPP;
> 	}
> 
> Which I think is a huge improvement on the documentation. And this has real
> value because the user do not have to look at the source code anymore to
> understand why an
> 
>    ip link set can ...
> 
> returned an error.
> 
>> E.g. if you look at the [PATCH 12/21] can: netlink: add
>> can_ctrlmode_changelink() the comments introduced by myself document the
>> different steps as we had problems with the complexity there and it was hard to
>> review either.
> 
> Those comments are still here.
> 
>> I would like to motivate you to generally add more comments.
> This is a refactoring series. I kept all existing comments except of one and
> then added a more comments in the form of error message. I am not adding code,
> just moving it around, so I do not really get why I should be adding even more
> comments to the existing code.
> 
>> When people (like me) look into that code that they haven't written themselves
>> and there is not even a hint of "what's the idea of what we are doing here" then
>> the code is hard to follow and to review.
> 
> Is this an issue in my code refactoring or an issue with the existing code?
> 
>> We definitely don't need a full blown documentation on top of each function. But
>> I like this comment you want to remove here and I would like to have more of it,
>> so that people get an impression what they will see in the following code.
> 

No need to defend yourself with specific references or even feel 
personally attacked.

My overall feeling is that you spend an excellent effort in commit 
messages but this information is then omitted in code comments.

As I've already written "I would like to motivate you to generally add 
more comments.". And this can also happen when refactoring things where 
new functions are created which reduces the context to the original code 
section.

Best regards,
Oliver

Re: [PATCH 00/21] can: netlink: preparation before introduction of CAN XL step 2/2

[Oliver Hartkopp]

On 04.09.25 11:18, Vincent Mailhol wrote:

> Concerning the CAN_CTRLMODE_XL_RRS, I am not sure if that one is needed. I still
> have it in my WIP series but I am recently considering to remove it. The reason
> is that when reading ISO 11898-1 having RRS configurable looks mandatory to me.
> 
> In the logical Link control (LLC) this RRS bit is named FTYP (for Frame Type).
> For example, CiA only mentions FTYP in their CAN XL knowledge page:
> 
>    https://www.can-cia.org/can-knowledge/can-xl
> 
> Contrarily to CAN FD's RRS which is indeed specified as being dominant and which
> is just ignored in the LLC, the CAN XL FTYP/RRS is part of the LLC interface and
> is meant to be configurable.

I double checked my XCANB CAN XL controller spec and indeed the RRS bit 
is part of every RX/TX FIFO element and the figures see it as 
configurable element too.

> Nothing in the standard tells us that this should be a dominant bit. I think
> your intention was to add CAN_CTRLMODE_XL_RRS as a quirk for the devices which
> expose this flag. But as far as I can see, it seems that a device which does not
> expose it is just not compliant.

Let's see if we will find CAN XL IP cores where the engineers have a 
different view on this. I currently have a discussion on this RRS bit 
with the Vector support because the RRS bit is not visible in the 
CANalyser 19 GUI.

> If some day a device which can not set the FTYP/RRS flag appears in the wild,
> then maybe we can add a flag which would specify that RRS is not configurable
> (opposite logic as what you suggested). But as long as such a device do not
> exist, it is better to add nothing.

ACK. After this discussion I would also vote to omit my glorious patch 
which added the CAN_CTRLMODE_XL_RRS flag. Let's see if we find a CAN XL 
controller that does not support the variable RRS bit in reading and 
writing. And if it shows up we can add this flag to handle it (similar 
to the fd-non-iso feature).

Best regards,
Oliver

Re: [PATCH 06/21] can: netlink: add can_validate_databittiming()

[Vincent Mailhol]

On 05/09/2025 at 03:46, kernel test robot wrote:
> Hi Vincent,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on 2fd4161d0d2547650d9559d57fc67b4e0a26a9e3]

OK. I will silence it in v2, but I don't think this is anyhow problematic.

> url:    https://github.com/intel-lab-lkp/linux/commits/Vincent-Mailhol/can-dev-move-struct-data_bittiming_params-to-linux-can-bittiming-h/20250903-170807
> base:   2fd4161d0d2547650d9559d57fc67b4e0a26a9e3
> patch link:    https://lore.kernel.org/r/20250903-canxl-netlink-prep-v1-6-904bd6037cd9%40kernel.org
> patch subject: [PATCH 06/21] can: netlink: add can_validate_databittiming()
> config: x86_64-kexec (https://download.01.org/0day-ci/archive/20250905/202509050259.NjPdQyAD-lkp@intel.com/config)
> compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250905/202509050259.NjPdQyAD-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202509050259.NjPdQyAD-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
>>> drivers/net/can/dev/netlink.c:111:6: warning: variable 'is_on' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
>      111 |         if (ifla_can_data_bittiming == IFLA_CAN_DATA_BITTIMING) {
>          |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    drivers/net/can/dev/netlink.c:119:6: note: uninitialized use occurs here
>      119 |         if (is_on) {
>          |             ^~~~~
>    drivers/net/can/dev/netlink.c:111:2: note: remove the 'if' if its condition is always true

That's it. This if condition is always true at the moment because all the
callers specify ifla_can_data_bittiming as IFLA_CAN_DATA_BITTIMING. But I do not
want to remove the else branch because is serves to show to the reader what is
coming up next and help to understand the refactor logic.
>      111 |         if (ifla_can_data_bittiming == IFLA_CAN_DATA_BITTIMING) {
>          |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>      112 |                 data_tdc = data[IFLA_CAN_TDC];
>      113 |                 tdc_flags = flags & CAN_CTRLMODE_FD_TDC_MASK;
>      114 |                 is_on = flags & CAN_CTRLMODE_FD;
>      115 |         } else {
>          |           ~~~~~~
>      116 |                 WARN_ON(1); /* Place holder for CAN XL */
>          |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The WARN_ON(1) was to point out that the else branch should never be reached.

I will change this to:

	return -EOPNOTSUPP; /* Place holder for CAN XL */

in v2. This will serve the same purpose as the current WARN_ON(1) but will
suppress the warning.

I will do the exact same on patches 11/21 and 18/21 on which the kernel test
robot also sent a similar report.


Yours sincerely,
Vincent Mailhol

Re: [PATCH 00/21] can: netlink: preparation before introduction of CAN XL step 2/2

[Vincent Mailhol]

On 05/09/2025 at 20:11, Oliver Hartkopp wrote:
> On 04.09.25 11:18, Vincent Mailhol wrote:
> 
>> Concerning the CAN_CTRLMODE_XL_RRS, I am not sure if that one is needed. I still
>> have it in my WIP series but I am recently considering to remove it. The reason
>> is that when reading ISO 11898-1 having RRS configurable looks mandatory to me.
>>
>> In the logical Link control (LLC) this RRS bit is named FTYP (for Frame Type).
>> For example, CiA only mentions FTYP in their CAN XL knowledge page:
>>
>>    https://www.can-cia.org/can-knowledge/can-xl
>>
>> Contrarily to CAN FD's RRS which is indeed specified as being dominant and which
>> is just ignored in the LLC, the CAN XL FTYP/RRS is part of the LLC interface and
>> is meant to be configurable.
> 
> I double checked my XCANB CAN XL controller spec and indeed the RRS bit is part
> of every RX/TX FIFO element and the figures see it as configurable element too.
> 
>> Nothing in the standard tells us that this should be a dominant bit. I think
>> your intention was to add CAN_CTRLMODE_XL_RRS as a quirk for the devices which
>> expose this flag. But as far as I can see, it seems that a device which does not
>> expose it is just not compliant.
> 
> Let's see if we will find CAN XL IP cores where the engineers have a different
> view on this. I currently have a discussion on this RRS bit with the Vector
> support because the RRS bit is not visible in the CANalyser 19 GUI.
> 
>> If some day a device which can not set the FTYP/RRS flag appears in the wild,
>> then maybe we can add a flag which would specify that RRS is not configurable
>> (opposite logic as what you suggested). But as long as such a device do not
>> exist, it is better to add nothing.
> 
> ACK. After this discussion I would also vote to omit my glorious patch which
> added the CAN_CTRLMODE_XL_RRS flag. Let's see if we find a CAN XL controller
> that does not support the variable RRS bit in reading and writing. And if it
> shows up we can add this flag to handle it (similar to the fd-non-iso feature).

OK. Good that we reached out to the same conclusion :)

Because I already implemented all the logic, I will save the current RRS patch
somewhere in case we need to resurrect it some days.


Yours sincerely,
Vincent Mailhol

Re: [PATCH 07/21] can: netlink: remove comment in can_validate()

[Vincent Mailhol]

On 05/09/2025 at 19:55, Oliver Hartkopp wrote:

(..)

> No need to defend yourself with specific references or even feel personally
> attacked.

Thanks. I was not sure how to read your previous message.

> My overall feeling is that you spend an excellent effort in commit messages but
> this information is then omitted in code comments.
> 
> As I've already written "I would like to motivate you to generally add more
> comments.". And this can also happen when refactoring things where new functions
> are created which reduces the context to the original code section.

My current mind set is that I want to do more ironing on the upcoming XL
patches. Because I do the documentation last once everything is working well,
this is still on my TODO list.

And when this is done, there is also

  Documentation/networking/can.rst

which need an update. At the moment, I am rather happy by just keeping the
existing documentation in this refactor series and want to put the extra effort
on the new stuff. Thinking of the upcoming work and of my current bandwidth, I
am really not in the mood into injecting more time in the refactor.

That said, on a second thought, I finally decided to keep the comment which I
previously wanted to remove. I will just move it from can_validate() to
can_validate_databittiming() in patch 06/21 "can: netlink: add
can_validate_databittiming()".


Yours sincerely,
Vincent Mailhol