[PATCH v2 00/17] can: bittiming: cleanups and rework SJW handling

[PATCH v2 00/17] can: bittiming: cleanups and rework SJW handling

[Marc Kleine-Budde]

Hello,

several people noticed that on modern CAN controllers with wide bit
timing registers the default SJW of 1 can result in unstable or no
synchronization to the CAN network. See Patch 14/17 for details.

During review of v1 Vincent pointed out that the original code and the
series doesn't always check user provided bit timing parameters,
sometimes silently limits them and the return error values are not
consistent.

This series first cleans up some code in bittiming.c, replacing
open-coded variants by macros or functions (Patches 1, 2).

Patch 3 adds the missing assignment of the effective TQ if the
interface is configured with low level timing parameters.

Patch 4 is another code cleanup.

Patches 5, 6 check the bit timing parameter during interface
registration.

Patch 7 adds a validation of the sample point.

The patches 8-13 convert the error messages from netdev_err() to
NL_SET_ERR_MSG_FMT, factor out the SJW handling from
can_fixup_bittiming(), add checking and error messages for the
individual limits and harmonize the error return values.

Patch 14 changes the default SJW value from 1 to min(Phase Seg1, Phase
Seg2 / 2).

Patch 15 switches can_calc_bittiming() to use the new SJW handling.

Patch 16 converts can_calc_bittiming() to NL_SET_ERR_MSG_FMT().

And patch 16 adds a NL_SET_ERR_MSG_FMT() error message to
can_validate_bitrate().

regards,
Marc

[PATCH v2 01/17] can: bittiming(): replace open coded variants of can_bit_time()

[Marc Kleine-Budde]

Commit 1c47fa6b31c2 ("can: dev: add a helper function to calculate the
duration of one bit") added the helper function can_bit_time().

Replace open coded variants of can_bit_time() by the helper function.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/bittiming.c      | 7 +++----
 drivers/net/can/dev/calc_bittiming.c | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index 7ae80763c960..32af609eee50 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -15,7 +15,7 @@ static int can_fixup_bittiming(const struct net_device *dev, struct can_bittimin
 			       const struct can_bittiming_const *btc)
 {
 	const struct can_priv *priv = netdev_priv(dev);
-	unsigned int tseg1, alltseg;
+	unsigned int tseg1;
 	u64 brp64;
 
 	tseg1 = bt->prop_seg + bt->phase_seg1;
@@ -38,9 +38,8 @@ static int can_fixup_bittiming(const struct net_device *dev, struct can_bittimin
 	if (bt->brp < btc->brp_min || bt->brp > btc->brp_max)
 		return -EINVAL;
 
-	alltseg = bt->prop_seg + bt->phase_seg1 + bt->phase_seg2 + 1;
-	bt->bitrate = priv->clock.freq / (bt->brp * alltseg);
-	bt->sample_point = ((tseg1 + 1) * 1000) / alltseg;
+	bt->bitrate = priv->clock.freq / (bt->brp * can_bit_time(bt));
+	bt->sample_point = ((tseg1 + 1) * 1000) / can_bit_time(bt);
 
 	return 0;
 }
diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
index d3caa040614d..28dbb6cbfd5d 100644
--- a/drivers/net/can/dev/calc_bittiming.c
+++ b/drivers/net/can/dev/calc_bittiming.c
@@ -170,7 +170,7 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 
 	/* real bitrate */
 	bt->bitrate = priv->clock.freq /
-		(bt->brp * (CAN_SYNC_SEG + tseg1 + tseg2));
+		(bt->brp * can_bit_time(bt));
 
 	return 0;
 }

base-commit: 609aa68d60965f70485655def733d533f99b341b
-- 
2.39.1

[PATCH v2 02/17] can: bittiming: can_fixup_bittiming(): use CAN_SYNC_SEG instead of 1

[Marc Kleine-Budde]

Commit 1c47fa6b31c2 ("can: dev: add a helper function to calculate the
duration of one bit") made the constant CAN_SYNC_SEG available in a
header file.

The magic number 1 in can_fixup_bittiming() represents the width of
the sync segment, replace it by CAN_SYNC_SEG to make the code more
readable.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/bittiming.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index 32af609eee50..5e111dbbe090 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -39,7 +39,7 @@ static int can_fixup_bittiming(const struct net_device *dev, struct can_bittimin
 		return -EINVAL;
 
 	bt->bitrate = priv->clock.freq / (bt->brp * can_bit_time(bt));
-	bt->sample_point = ((tseg1 + 1) * 1000) / can_bit_time(bt);
+	bt->sample_point = ((CAN_SYNC_SEG + tseg1) * 1000) / can_bit_time(bt);
 
 	return 0;
 }
-- 
2.39.1

[PATCH v2 03/17] can: bittiming: can_fixup_bittiming(): set effective tq

[Marc Kleine-Budde]

The can_fixup_bittiming() function is used to validate the
user-supplied low-level bit timing parameters and calculate the
bitrate prescaler (brp) from the requested time quanta (tq) and the
CAN clock of the controller.

can_fixup_bittiming() selects the best matching integer bit rate
prescaler, which may result in a different time quantum than the value
specified by the user.

Calculate the resulting time quantum and assign it so that the user
sees the effective time quantum.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/bittiming.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index 5e111dbbe090..e4917c2f34d3 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -40,6 +40,8 @@ static int can_fixup_bittiming(const struct net_device *dev, struct can_bittimin
 
 	bt->bitrate = priv->clock.freq / (bt->brp * can_bit_time(bt));
 	bt->sample_point = ((CAN_SYNC_SEG + tseg1) * 1000) / can_bit_time(bt);
+	bt->tq = DIV_U64_ROUND_CLOSEST(mul_u32_u32(bt->brp, NSEC_PER_SEC),
+				       priv->clock.freq);
 
 	return 0;
 }
-- 
2.39.1

[PATCH v2 04/17] can: bittiming: can_get_bittiming(): use direct return and remove unneeded else

[Marc Kleine-Budde]

Clean up the code flow a bit, don't assign err variable but directly
return. Remove the unneeded else, too.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/bittiming.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index e4917c2f34d3..263e46a1f648 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -67,22 +67,18 @@ int can_get_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 		      const u32 *bitrate_const,
 		      const unsigned int bitrate_const_cnt)
 {
-	int err;
-
 	/* Depending on the given can_bittiming parameter structure the CAN
 	 * timing parameters are calculated based on the provided bitrate OR
 	 * alternatively the CAN timing parameters (tq, prop_seg, etc.) are
 	 * provided directly which are then checked and fixed up.
 	 */
 	if (!bt->tq && bt->bitrate && btc)
-		err = can_calc_bittiming(dev, bt, btc);
-	else if (bt->tq && !bt->bitrate && btc)
-		err = can_fixup_bittiming(dev, bt, btc);
-	else if (!bt->tq && bt->bitrate && bitrate_const)
-		err = can_validate_bitrate(dev, bt, bitrate_const,
-					   bitrate_const_cnt);
-	else
-		err = -EINVAL;
+		return can_calc_bittiming(dev, bt, btc);
+	if (bt->tq && !bt->bitrate && btc)
+		return can_fixup_bittiming(dev, bt, btc);
+	if (!bt->tq && bt->bitrate && bitrate_const)
+		return can_validate_bitrate(dev, bt, bitrate_const,
+					    bitrate_const_cnt);
 
-	return err;
+	return -EINVAL;
 }
-- 
2.39.1

[PATCH v2 05/17] can: dev: register_candev(): ensure that bittiming const are valid

[Marc Kleine-Budde]

Implement the function can_bittiming_const_valid() to check the
validity of the specified bit timing constant. Call this function from
register_candev() to check the bit timing constants during the
registration of the CAN interface.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/dev.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index c1956b1e9faf..3b51055be40e 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -498,6 +498,18 @@ static int can_get_termination(struct net_device *ndev)
 	return 0;
 }
 
+static bool
+can_bittiming_const_valid(const struct can_bittiming_const *btc)
+{
+	if (!btc)
+		return true;
+
+	if (!btc->sjw_max)
+		return false;
+
+	return true;
+}
+
 /* Register the CAN network device */
 int register_candev(struct net_device *dev)
 {
@@ -518,6 +530,10 @@ int register_candev(struct net_device *dev)
 	if (!priv->data_bitrate_const != !priv->data_bitrate_const_cnt)
 		return -EINVAL;
 
+	if (!can_bittiming_const_valid(priv->bittiming_const) ||
+	    !can_bittiming_const_valid(priv->data_bittiming_const))
+		return -EINVAL;
+
 	if (!priv->termination_const) {
 		err = can_get_termination(dev);
 		if (err)
-- 
2.39.1

[PATCH v2 06/17] can: dev: register_candev(): bail out if both fixed bit rates and bit timing constants are provided

[Marc Kleine-Budde]

The CAN driver framework supports either fixed bit rates or bit timing
constants. Bail out during driver registration if both are given.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/dev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index 3b51055be40e..7f9334a8af50 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -530,6 +530,11 @@ int register_candev(struct net_device *dev)
 	if (!priv->data_bitrate_const != !priv->data_bitrate_const_cnt)
 		return -EINVAL;
 
+	/* We only support either fixed bit rates or bit timing const. */
+	if ((priv->bitrate_const || priv->data_bitrate_const) &&
+	    (priv->bittiming_const || priv->data_bittiming_const))
+		return -EINVAL;
+
 	if (!can_bittiming_const_valid(priv->bittiming_const) ||
 	    !can_bittiming_const_valid(priv->data_bittiming_const))
 		return -EINVAL;
-- 
2.39.1

[PATCH v2 07/17] can: netlink: can_validate(): validate sample point for CAN and CAN-FD

[Marc Kleine-Budde]

The sample point is a value in tenths of a percent. Meaningful values
are between 0 and 1000. Invalid values are rejected and an error
message is returned to user space via netlink.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/netlink.c | 33 ++++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 8efa22d9f214..02f5c00c521f 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -36,10 +36,24 @@ 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)
+{
+	/* 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;
+	}
+
+	return 0;
+}
+
 static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 			struct netlink_ext_ack *extack)
 {
 	bool is_can_fd = false;
+	int err;
 
 	/* Make sure that valid CAN FD configurations always consist of
 	 * - nominal/arbitration bittiming
@@ -51,6 +65,15 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 	if (!data)
 		return 0;
 
+	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;
+	}
+
 	if (data[IFLA_CAN_CTRLMODE]) {
 		struct can_ctrlmode *cm = nla_data(data[IFLA_CAN_CTRLMODE]);
 		u32 tdc_flags = cm->flags & CAN_CTRLMODE_TDC_MASK;
@@ -71,7 +94,6 @@ static int can_validate(struct nlattr *tb[], struct nlattr *data[],
 		 */
 		if (data[IFLA_CAN_TDC]) {
 			struct nlattr *tb_tdc[IFLA_CAN_TDC_MAX + 1];
-			int err;
 
 			err = nla_parse_nested(tb_tdc, IFLA_CAN_TDC_MAX,
 					       data[IFLA_CAN_TDC],
@@ -102,6 +124,15 @@ 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;
+	}
+
 	return 0;
 }
 
-- 
2.39.1

[PATCH v2 08/17] can: netlink: can_changelink(): convert from netdev_err() to NL_SET_ERR_MSG_FMT()

[Marc Kleine-Budde]

Since commit 51c352bdbcd2 ("netlink: add support for formatted extack
messages") formatted extack messages are supported to inform the user
space or warnings/errors during netlink calls.

Replace the netdev_err() by NL_SET_ERR_MSG_FMT() to better inform the
user about the problem. While there, use %u to print unsigned values
and improve error message a bit.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/netlink.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index 02f5c00c521f..a03b45a020b9 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -220,8 +220,9 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 			return err;
 
 		if (priv->bitrate_max && bt.bitrate > priv->bitrate_max) {
-			netdev_err(dev, "arbitration bitrate surpasses transceiver capabilities of %d bps\n",
-				   priv->bitrate_max);
+			NL_SET_ERR_MSG_FMT(extack,
+					   "arbitration bitrate %u bps surpasses transceiver capabilities of %u bps",
+					   bt.bitrate, priv->bitrate_max);
 			return -EINVAL;
 		}
 
@@ -324,8 +325,9 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 			return err;
 
 		if (priv->bitrate_max && dbt.bitrate > priv->bitrate_max) {
-			netdev_err(dev, "canfd data bitrate surpasses transceiver capabilities of %d bps\n",
-				   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;
 		}
 
-- 
2.39.1

[PATCH v2 09/17] can: bittiming: can_changelink() pass extack down callstack

[Marc Kleine-Budde]

This is a preparation patch.

In order to pass warning/error messages during netlink calls back to
user space, pass the extack struct down the callstack of
can_changelink(), the actual error messages will be added in the
following ptaches.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/bittiming.c      | 15 +++++++++------
 drivers/net/can/dev/calc_bittiming.c |  2 +-
 drivers/net/can/dev/netlink.c        |  6 ++++--
 include/linux/can/bittiming.h        |  5 +++--
 4 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index 263e46a1f648..0b0b8c767c5b 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -12,7 +12,8 @@
  * file linux/can/netlink.h.
  */
 static int can_fixup_bittiming(const struct net_device *dev, struct can_bittiming *bt,
-			       const struct can_bittiming_const *btc)
+			       const struct can_bittiming_const *btc,
+			       struct netlink_ext_ack *extack)
 {
 	const struct can_priv *priv = netdev_priv(dev);
 	unsigned int tseg1;
@@ -50,7 +51,8 @@ static int can_fixup_bittiming(const struct net_device *dev, struct can_bittimin
 static int
 can_validate_bitrate(const struct net_device *dev, const struct can_bittiming *bt,
 		     const u32 *bitrate_const,
-		     const unsigned int bitrate_const_cnt)
+		     const unsigned int bitrate_const_cnt,
+		     struct netlink_ext_ack *extack)
 {
 	unsigned int i;
 
@@ -65,7 +67,8 @@ can_validate_bitrate(const struct net_device *dev, const struct can_bittiming *b
 int can_get_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 		      const struct can_bittiming_const *btc,
 		      const u32 *bitrate_const,
-		      const unsigned int bitrate_const_cnt)
+		      const unsigned int bitrate_const_cnt,
+		      struct netlink_ext_ack *extack)
 {
 	/* Depending on the given can_bittiming parameter structure the CAN
 	 * timing parameters are calculated based on the provided bitrate OR
@@ -73,12 +76,12 @@ int can_get_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 	 * provided directly which are then checked and fixed up.
 	 */
 	if (!bt->tq && bt->bitrate && btc)
-		return can_calc_bittiming(dev, bt, btc);
+		return can_calc_bittiming(dev, bt, btc, extack);
 	if (bt->tq && !bt->bitrate && btc)
-		return can_fixup_bittiming(dev, bt, btc);
+		return can_fixup_bittiming(dev, bt, btc, extack);
 	if (!bt->tq && bt->bitrate && bitrate_const)
 		return can_validate_bitrate(dev, bt, bitrate_const,
-					    bitrate_const_cnt);
+					    bitrate_const_cnt, extack);
 
 	return -EINVAL;
 }
diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
index 28dbb6cbfd5d..46d28f377186 100644
--- a/drivers/net/can/dev/calc_bittiming.c
+++ b/drivers/net/can/dev/calc_bittiming.c
@@ -63,7 +63,7 @@ can_update_sample_point(const struct can_bittiming_const *btc,
 }
 
 int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
-		       const struct can_bittiming_const *btc)
+		       const struct can_bittiming_const *btc, struct netlink_ext_ack *extack)
 {
 	struct can_priv *priv = netdev_priv(dev);
 	unsigned int bitrate;			/* current bitrate */
diff --git a/drivers/net/can/dev/netlink.c b/drivers/net/can/dev/netlink.c
index a03b45a020b9..036d85ef07f5 100644
--- a/drivers/net/can/dev/netlink.c
+++ b/drivers/net/can/dev/netlink.c
@@ -215,7 +215,8 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		err = can_get_bittiming(dev, &bt,
 					priv->bittiming_const,
 					priv->bitrate_const,
-					priv->bitrate_const_cnt);
+					priv->bitrate_const_cnt,
+					extack);
 		if (err)
 			return err;
 
@@ -320,7 +321,8 @@ static int can_changelink(struct net_device *dev, struct nlattr *tb[],
 		err = can_get_bittiming(dev, &dbt,
 					priv->data_bittiming_const,
 					priv->data_bitrate_const,
-					priv->data_bitrate_const_cnt);
+					priv->data_bitrate_const_cnt,
+					extack);
 		if (err)
 			return err;
 
diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
index ef0a77173e3c..53d693ae5397 100644
--- a/include/linux/can/bittiming.h
+++ b/include/linux/can/bittiming.h
@@ -116,7 +116,7 @@ struct can_tdc_const {
 
 #ifdef CONFIG_CAN_CALC_BITTIMING
 int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
-		       const struct can_bittiming_const *btc);
+		       const struct can_bittiming_const *btc, struct netlink_ext_ack *extack);
 
 void can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const,
 		   const struct can_bittiming *dbt,
@@ -141,7 +141,8 @@ can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const,
 int can_get_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 		      const struct can_bittiming_const *btc,
 		      const u32 *bitrate_const,
-		      const unsigned int bitrate_const_cnt);
+		      const unsigned int bitrate_const_cnt,
+		      struct netlink_ext_ack *extack);
 
 /*
  * can_bit_time() - Duration of one bit
-- 
2.39.1

[PATCH v2 10/17] can: bittiming: factor out can_sjw_set_default() and can_sjw_check()

[Marc Kleine-Budde]

Factor out the functionality of assigning a SJW default value into
can_sjw_set_default() and the checking the SJW limits into
can_sjw_check().

This functions will be improved and called from a different function
in the following patches.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/bittiming.c | 30 ++++++++++++++++++++++++++----
 include/linux/can/bittiming.h   |  5 +++++
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index 0b0b8c767c5b..101de1b3bf30 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -6,6 +6,24 @@
 
 #include <linux/can/dev.h>
 
+void can_sjw_set_default(struct can_bittiming *bt)
+{
+	if (bt->sjw)
+		return;
+
+	/* If user space provides no sjw, use 1 as default */
+	bt->sjw = 1;
+}
+
+int can_sjw_check(const struct net_device *dev, const struct can_bittiming *bt,
+		  const struct can_bittiming_const *btc, struct netlink_ext_ack *extack)
+{
+	if (bt->sjw > btc->sjw_max)
+		return -ERANGE;
+
+	return 0;
+}
+
 /* Checks the validity of the specified bit-timing parameters prop_seg,
  * phase_seg1, phase_seg2 and sjw and tries to determine the bitrate
  * prescaler value brp. You can find more information in the header
@@ -18,12 +36,16 @@ static int can_fixup_bittiming(const struct net_device *dev, struct can_bittimin
 	const struct can_priv *priv = netdev_priv(dev);
 	unsigned int tseg1;
 	u64 brp64;
+	int err;
+
+	can_sjw_set_default(bt);
+
+	err = can_sjw_check(dev, bt, btc, extack);
+	if (err)
+		return err;
 
 	tseg1 = bt->prop_seg + bt->phase_seg1;
-	if (!bt->sjw)
-		bt->sjw = 1;
-	if (bt->sjw > btc->sjw_max ||
-	    tseg1 < btc->tseg1_min || tseg1 > btc->tseg1_max ||
+	if (tseg1 < btc->tseg1_min || tseg1 > btc->tseg1_max ||
 	    bt->phase_seg2 < btc->tseg2_min || bt->phase_seg2 > btc->tseg2_max)
 		return -ERANGE;
 
diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
index 53d693ae5397..6cb2ae308e3f 100644
--- a/include/linux/can/bittiming.h
+++ b/include/linux/can/bittiming.h
@@ -138,6 +138,11 @@ can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const,
 }
 #endif /* CONFIG_CAN_CALC_BITTIMING */
 
+void can_sjw_set_default(struct can_bittiming *bt);
+
+int can_sjw_check(const struct net_device *dev, const struct can_bittiming *bt,
+		  const struct can_bittiming_const *btc, struct netlink_ext_ack *extack);
+
 int can_get_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 		      const struct can_bittiming_const *btc,
 		      const u32 *bitrate_const,
-- 
2.39.1

[PATCH v2 11/17] can: bittiming: can_fixup_bittiming(): report error via netlink and harmonize error value

[Marc Kleine-Budde]

Check each bit timing parameter first individually against their
limits and report a meaningful error message via netlink to the user
space.

In case of an error, return -EINVAL instead of -ERANGE, this
corresponds better to the actual meaning of the error value.

Suggested-by: Vincent Mailhol <vincent.mailhol@gmail.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/bittiming.c | 38 +++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index 101de1b3bf30..727dcd52cc2c 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -33,22 +33,38 @@ static int can_fixup_bittiming(const struct net_device *dev, struct can_bittimin
 			       const struct can_bittiming_const *btc,
 			       struct netlink_ext_ack *extack)
 {
+	const unsigned int tseg1 = bt->prop_seg + bt->phase_seg1;
 	const struct can_priv *priv = netdev_priv(dev);
-	unsigned int tseg1;
 	u64 brp64;
 	int err;
 
+	if (tseg1 < btc->tseg1_min) {
+		NL_SET_ERR_MSG_FMT(extack, "prop-seg + phase-seg1: %u less than tseg1-min: %u",
+				   tseg1, btc->tseg1_min);
+		return -EINVAL;
+	}
+	if (tseg1 > btc->tseg1_max) {
+		NL_SET_ERR_MSG_FMT(extack, "prop-seg + phase-seg1: %u greater than tseg1-max: %u",
+				   tseg1, btc->tseg1_max);
+		return -EINVAL;
+	}
+	if (bt->phase_seg2 < btc->tseg2_min) {
+		NL_SET_ERR_MSG_FMT(extack, "phase-seg2: %u less than tseg2-min: %u",
+				   bt->phase_seg2, btc->tseg2_min);
+		return -EINVAL;
+	}
+	if (bt->phase_seg2 > btc->tseg2_max) {
+		NL_SET_ERR_MSG_FMT(extack, "phase-seg2: %u greater than tseg2-max: %u",
+				   bt->phase_seg2, btc->tseg2_max);
+		return -EINVAL;
+	}
+
 	can_sjw_set_default(bt);
 
 	err = can_sjw_check(dev, bt, btc, extack);
 	if (err)
 		return err;
 
-	tseg1 = bt->prop_seg + bt->phase_seg1;
-	if (tseg1 < btc->tseg1_min || tseg1 > btc->tseg1_max ||
-	    bt->phase_seg2 < btc->tseg2_min || bt->phase_seg2 > btc->tseg2_max)
-		return -ERANGE;
-
 	brp64 = (u64)priv->clock.freq * (u64)bt->tq;
 	if (btc->brp_inc > 1)
 		do_div(brp64, btc->brp_inc);
@@ -58,8 +74,16 @@ static int can_fixup_bittiming(const struct net_device *dev, struct can_bittimin
 		brp64 *= btc->brp_inc;
 	bt->brp = (u32)brp64;
 
-	if (bt->brp < btc->brp_min || bt->brp > btc->brp_max)
+	if (bt->brp < btc->brp_min) {
+		NL_SET_ERR_MSG_FMT(extack, "resulting brp: %u less than brp-min: %u",
+				   bt->brp, btc->brp_min);
 		return -EINVAL;
+	}
+	if (bt->brp > btc->brp_max) {
+		NL_SET_ERR_MSG_FMT(extack, "resulting brp: %u greater than brp-max: %u",
+				   bt->brp, btc->brp_max);
+		return -EINVAL;
+	}
 
 	bt->bitrate = priv->clock.freq / (bt->brp * can_bit_time(bt));
 	bt->sample_point = ((CAN_SYNC_SEG + tseg1) * 1000) / can_bit_time(bt);
-- 
2.39.1

[PATCH v2 12/17] can: bittiming: can_sjw_check(): report error via netlink and harmonize error value

[Marc Kleine-Budde]

If the user space has supplied an invalid SJW value (greater than the
maximum SJW value), report -EINVAL instead of -ERANGE, this better
matches the actual meaning of the error value.

Additionally report an error message via netlink to the user space.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/bittiming.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index 727dcd52cc2c..0a2a9b12565f 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -18,8 +18,11 @@ void can_sjw_set_default(struct can_bittiming *bt)
 int can_sjw_check(const struct net_device *dev, const struct can_bittiming *bt,
 		  const struct can_bittiming_const *btc, struct netlink_ext_ack *extack)
 {
-	if (bt->sjw > btc->sjw_max)
-		return -ERANGE;
+	if (bt->sjw > btc->sjw_max) {
+		NL_SET_ERR_MSG_FMT(extack, "sjw: %u greater than max sjw: %u",
+				   bt->sjw, btc->sjw_max);
+		return -EINVAL;
+	}
 
 	return 0;
 }
-- 
2.39.1

[PATCH v2 13/17] can: bittiming: can_sjw_check(): check that SJW is not longer than either Phase Buffer Segment

[Marc Kleine-Budde]

According to "The Configuration of the CAN Bit Timing" [1] the SJW
"may not be longer than either Phase Buffer Segment".

Check SJW against length of both Phase buffers. In case the SJW is
greater, report an error via netlink to user space and bail out.

[1] http://web.archive.org/http://www.oertel-halle.de/files/cia99paper.pdf

Suggested-by: Vincent Mailhol <vincent.mailhol@gmail.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/bittiming.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index 0a2a9b12565f..68287b79afe8 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -24,6 +24,20 @@ int can_sjw_check(const struct net_device *dev, const struct can_bittiming *bt,
 		return -EINVAL;
 	}
 
+	if (bt->sjw > bt->phase_seg1) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "sjw: %u greater than phase-seg1: %u",
+				   bt->sjw, bt->phase_seg1);
+		return -EINVAL;
+	}
+
+	if (bt->sjw > bt->phase_seg2) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "sjw: %u greater than phase-seg2: %u",
+				   bt->sjw, bt->phase_seg2);
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
-- 
2.39.1

[PATCH v2 14/17] can: bittiming: can_sjw_set_default(): use Phase Seg2 / 2 as default for SJW

[Marc Kleine-Budde]

"The (Re-)Synchronization Jump Width (SJW) defines how far a
 resynchronization may move the Sample Point inside the limits defined
 by the Phase Buffer Segments to compensate for edge phase errors." [1]

In other words, this means that the SJW parameter controls the CAN
controller's tolerance to frequency errors compared to other CAN
controllers.

If the user space doesn't provide a SJW parameter, the
kernel chooses a default value of 1. This has proven to be a good
default value for CAN controllers, but no longer for modern
controllers.

In the past there were CAN controllers like the sja1000 with a rather
limited range of bit timing parameters. For the standard bit rates
this results in the following bit timing parameters:

| Bit timing parameters for sja1000 with 8.000000 MHz ref clock
|                     _----+--------------=> tseg1: 1 …   16
|                    /    /     _---------=> tseg2: 1 …    8
|                   |    |     /    _-----=> sjw:   1 …    4
|                   |    |    |    /    _-=> brp:   1 …   64 (inc: 1)
|                   |    |    |   |    /
|  nominal          |    |    |   |   |     real  Bitrt    nom   real   SampP
|  Bitrate TQ[ns] PrS PhS1 PhS2 SJW BRP  Bitrate  Error  SampP  SampP   Error  BTR0 BTR1
|  1000000    125   2    3    2   1   1  1000000   0.0%  75.0%  75.0%   0.0%   0x00 0x14
|   800000    125   3    4    2   1   1   800000   0.0%  80.0%  80.0%   0.0%   0x00 0x16
|   666666    125   4    4    3   1   1   666666   0.0%  80.0%  75.0%   6.2%   0x00 0x27
|   500000    125   6    7    2   1   1   500000   0.0%  87.5%  87.5%   0.0%   0x00 0x1c
|   250000    250   6    7    2   1   2   250000   0.0%  87.5%  87.5%   0.0%   0x01 0x1c
|   125000    500   6    7    2   1   4   125000   0.0%  87.5%  87.5%   0.0%   0x03 0x1c
|   100000    625   6    7    2   1   5   100000   0.0%  87.5%  87.5%   0.0%   0x04 0x1c
|    83333    750   6    7    2   1   6    83333   0.0%  87.5%  87.5%   0.0%   0x05 0x1c
|    50000   1250   6    7    2   1  10    50000   0.0%  87.5%  87.5%   0.0%   0x09 0x1c
|    33333   1875   6    7    2   1  15    33333   0.0%  87.5%  87.5%   0.0%   0x0e 0x1c
|    20000   3125   6    7    2   1  25    20000   0.0%  87.5%  87.5%   0.0%   0x18 0x1c
|    10000   6250   6    7    2   1  50    10000   0.0%  87.5%  87.5%   0.0%   0x31 0x1c

The attentive reader will notice that the SJW is 1 in most cases,
while the Seg2 phase is 2. Both values are given in TQ units, which in
turn is a duration in nanoseconds.

For example the 500 kbit/s configuration:

|  nominal                                  real  Bitrt    nom   real   SampP
|  Bitrate TQ[ns] PrS PhS1 PhS2 SJW BRP  Bitrate  Error  SampP  SampP   Error  BTR0 BTR1
|   500000    125   6    7    2   1   1   500000   0.0%  87.5%  87.5%   0.0%   0x00 0x1c

the TQ is 125ns, the Phase Seg2 is "2" (== 250ns), the SJW is "1" (==
125 ns).

Looking at a more modern CAN controller like a mcp2518fd, it has wider
bit timing registers.

| Bit timing parameters for mcp251xfd with 40.000000 MHz ref clock
|                     _----+--------------=> tseg1: 2 …  256
|                    /    /     _---------=> tseg2: 1 …  128
|                   |    |     /    _-----=> sjw:   1 …  128
|                   |    |    |    /    _-=> brp:   1 …  256 (inc: 1)
|                   |    |    |   |    /
|  nominal          |    |    |   |   |     real  Bitrt    nom   real   SampP
|  Bitrate TQ[ns] PrS PhS1 PhS2 SJW BRP  Bitrate  Error  SampP  SampP   Error      NBTCFG
|   500000     25  34   35   10   1   1   500000   0.0%  87.5%  87.5%   0.0%   0x00440900

The TQ is 25ns, the Phase Seg 2 is "10" (== 250ns), the SJW is "1" (==
25ns).

Since the kernel chooses a default SJW of 1 regardless of the TQ, this
leads to a much smaller SJW and thus much smaller tolerances to
frequency errors.

To maintain the same oscillator tolerances on controllers with wide
bit timing registers, select a default SJW value of Phase Seg2 / 2
unless Phase Seg 1 is less. This results in the following bit timing
parameters:

| Bit timing parameters for mcp251xfd with 40.000000 MHz ref clock
|                     _----+--------------=> tseg1: 2 …  256
|                    /    /     _---------=> tseg2: 1 …  128
|                   |    |     /    _-----=> sjw:   1 …  128
|                   |    |    |    /    _-=> brp:   1 …  256 (inc: 1)
|                   |    |    |   |    /
|  nominal          |    |    |   |   |     real  Bitrt    nom   real   SampP
|  Bitrate TQ[ns] PrS PhS1 PhS2 SJW BRP  Bitrate  Error  SampP  SampP   Error      NBTCFG
|   500000     25  34   35   10   5   1   500000   0.0%  87.5%  87.5%   0.0%   0x00440904

The TQ is 25ns, the Phase Seg 2 is "10" (== 250ns), the SJW is "5" (==
125ns). Which is the same as on the sja1000 controller.

[1] http://web.archive.org/http://www.oertel-halle.de/files/cia99paper.pdf

Cc: Mark Bath <mark@baggywrinkle.co.uk>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/bittiming.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index 68287b79afe8..55714e08ca3a 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -11,8 +11,8 @@ void can_sjw_set_default(struct can_bittiming *bt)
 	if (bt->sjw)
 		return;
 
-	/* If user space provides no sjw, use 1 as default */
-	bt->sjw = 1;
+	/* If user space provides no sjw, use sane default of phase_seg2 / 2 */
+	bt->sjw = max(1U, min(bt->phase_seg1, bt->phase_seg2 / 2));
 }
 
 int can_sjw_check(const struct net_device *dev, const struct can_bittiming *bt,
-- 
2.39.1

[PATCH v2 15/17] can: bittiming: can_calc_bittiming(): clean up SJW handling

[Marc Kleine-Budde]

In the current code, if the user configures a bitrate, a default SJW
value of 1 is used. If the user configures both a bitrate and a SJW
value, can_calc_bittiming() silently limits the SJW value to SJW max
and TSEG2.

We came to the conclusion that if the user provided an invalid SJW
value, it's best to bail out and inform the user [1].

[1] https://lore.kernel.org/all/CAMZ6RqKqhmTgUZiwe5uqUjBDnhhC2iOjZ791+Y845btJYwVDKg@mail.gmail.com

Further the ISO 11898-1:2015 standard mandates that "SJW shall be less
than or equal to the minimum of these two items: Phase_Seg1 and
Phase_Seg2." [2] The current code is missing that check.

[2] https://lore.kernel.org/all/BL3PR11MB64844E3FC13C55433CDD0B3DFB449@BL3PR11MB6484.namprd11.prod.outlook.com

The previous patches introduced
1) can_sjw_set_default() - sets a default value for SJW if unset
2) can_sjw_check() - implements a SJW check against SJW max, Phase
   Seg1 and Phase Seg2. In the error case this function reports the error
   to user space via netlink.

Replace both the open-coded SJW default setting and the open-coded and
insufficient checks of SJW with the helper functions
can_sjw_set_default() and can_sjw_check().

Link: https://lore.kernel.org/all/CAMZ6RqKqhmTgUZiwe5uqUjBDnhhC2iOjZ791+Y845btJYwVDKg@mail.gmail.com
Link: https://lore.kernel.org/all/BL3PR11MB64844E3FC13C55433CDD0B3DFB449@BL3PR11MB6484.namprd11.prod.outlook.com
Suggested-by: Thomas Kopp <Thomas.Kopp@microchip.com>
Suggested-by: Vincent Mailhol <vincent.mailhol@gmail.com>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/calc_bittiming.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
index 46d28f377186..4c9e9c0ceff3 100644
--- a/drivers/net/can/dev/calc_bittiming.c
+++ b/drivers/net/can/dev/calc_bittiming.c
@@ -76,6 +76,7 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 	unsigned int best_brp = 0;		/* current best value for brp */
 	unsigned int brp, tsegall, tseg, tseg1 = 0, tseg2 = 0;
 	u64 v64;
+	int err;
 
 	/* Use CiA recommended sample points */
 	if (bt->sample_point) {
@@ -154,17 +155,11 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 	bt->phase_seg1 = tseg1 - bt->prop_seg;
 	bt->phase_seg2 = tseg2;
 
-	/* check for sjw user settings */
-	if (!bt->sjw || !btc->sjw_max) {
-		bt->sjw = 1;
-	} else {
-		/* bt->sjw is at least 1 -> sanitize upper bound to sjw_max */
-		if (bt->sjw > btc->sjw_max)
-			bt->sjw = btc->sjw_max;
-		/* bt->sjw must not be higher than tseg2 */
-		if (tseg2 < bt->sjw)
-			bt->sjw = tseg2;
-	}
+	can_sjw_set_default(bt);
+
+	err = can_sjw_check(dev, bt, btc, extack);
+	if (err)
+		return err;
 
 	bt->brp = best_brp;
 
-- 
2.39.1

[PATCH v2 16/17] can: bittiming: can_calc_bittiming(): convert from netdev_err() to NL_SET_ERR_MSG_FMT()

[Marc Kleine-Budde]

Replace the netdev_err() by NL_SET_ERR_MSG_FMT() to better inform the
user about the problem. While there, use %u to print unsigned values
and improve error message a bit.

In case of an error, return -EINVAL instead of -EDOM, this corresponds
better to the actual meaning of the error value.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/calc_bittiming.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/dev/calc_bittiming.c b/drivers/net/can/dev/calc_bittiming.c
index 4c9e9c0ceff3..3809c148fb88 100644
--- a/drivers/net/can/dev/calc_bittiming.c
+++ b/drivers/net/can/dev/calc_bittiming.c
@@ -134,13 +134,14 @@ int can_calc_bittiming(const struct net_device *dev, struct can_bittiming *bt,
 		do_div(v64, bt->bitrate);
 		bitrate_error = (u32)v64;
 		if (bitrate_error > CAN_CALC_MAX_ERROR) {
-			netdev_err(dev,
-				   "bitrate error %d.%d%% too high\n",
-				   bitrate_error / 10, bitrate_error % 10);
-			return -EDOM;
+			NL_SET_ERR_MSG_FMT(extack,
+					   "bitrate error: %u.%u%% too high",
+					   bitrate_error / 10, bitrate_error % 10);
+			return -EINVAL;
 		}
-		netdev_warn(dev, "bitrate error %d.%d%%\n",
-			    bitrate_error / 10, bitrate_error % 10);
+		NL_SET_ERR_MSG_FMT(extack,
+				   "bitrate error: %u.%u%%",
+				   bitrate_error / 10, bitrate_error % 10);
 	}
 
 	/* real sample point */
-- 
2.39.1

[PATCH v2 17/17] can: bittiming: can_validate_bitrate(): report error via netlink

[Marc Kleine-Budde]

Report an error to user space via netlink if the requested bit rate is
not supported by the device.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/dev/bittiming.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
index 55714e08ca3a..0b93900b1dfa 100644
--- a/drivers/net/can/dev/bittiming.c
+++ b/drivers/net/can/dev/bittiming.c
@@ -124,6 +124,9 @@ can_validate_bitrate(const struct net_device *dev, const struct can_bittiming *b
 			return 0;
 	}
 
+	NL_SET_ERR_MSG_FMT(extack, "bitrate %u bps not supported",
+			   bt->brp);
+
 	return -EINVAL;
 }
 
-- 
2.39.1

Re: [PATCH v2 10/17] can: bittiming: factor out can_sjw_set_default() and can_sjw_check()

[Vincent Mailhol]

On Thu. 2 Feb. 2023 at 20:09, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> Factor out the functionality of assigning a SJW default value into
> can_sjw_set_default() and the checking the SJW limits into
> can_sjw_check().
>
> This functions will be improved and called from a different function
> in the following patches.
>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/can/dev/bittiming.c | 30 ++++++++++++++++++++++++++----
>  include/linux/can/bittiming.h   |  5 +++++
>  2 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
> index 0b0b8c767c5b..101de1b3bf30 100644
> --- a/drivers/net/can/dev/bittiming.c
> +++ b/drivers/net/can/dev/bittiming.c
> @@ -6,6 +6,24 @@
>
>  #include <linux/can/dev.h>
>
> +void can_sjw_set_default(struct can_bittiming *bt)
> +{
> +       if (bt->sjw)
> +               return;
> +
> +       /* If user space provides no sjw, use 1 as default */
> +       bt->sjw = 1;
> +}
> +
> +int can_sjw_check(const struct net_device *dev, const struct can_bittiming *bt,
> +                 const struct can_bittiming_const *btc, struct netlink_ext_ack *extack)
> +{
> +       if (bt->sjw > btc->sjw_max)
> +               return -ERANGE;

You return -ERANGE here but then replace it by -EINVAL in patch #12.
Better to directly return -EINVAL here.

> +       return 0;
> +}
> +
>  /* Checks the validity of the specified bit-timing parameters prop_seg,
>   * phase_seg1, phase_seg2 and sjw and tries to determine the bitrate
>   * prescaler value brp. You can find more information in the header
> @@ -18,12 +36,16 @@ static int can_fixup_bittiming(const struct net_device *dev, struct can_bittimin
>         const struct can_priv *priv = netdev_priv(dev);
>         unsigned int tseg1;
>         u64 brp64;
> +       int err;
> +
> +       can_sjw_set_default(bt);
> +
> +       err = can_sjw_check(dev, bt, btc, extack);
> +       if (err)
> +               return err;
>
>         tseg1 = bt->prop_seg + bt->phase_seg1;
> -       if (!bt->sjw)
> -               bt->sjw = 1;
> -       if (bt->sjw > btc->sjw_max ||
> -           tseg1 < btc->tseg1_min || tseg1 > btc->tseg1_max ||
> +       if (tseg1 < btc->tseg1_min || tseg1 > btc->tseg1_max ||
>             bt->phase_seg2 < btc->tseg2_min || bt->phase_seg2 > btc->tseg2_max)
>                 return -ERANGE;
>
> diff --git a/include/linux/can/bittiming.h b/include/linux/can/bittiming.h
> index 53d693ae5397..6cb2ae308e3f 100644
> --- a/include/linux/can/bittiming.h
> +++ b/include/linux/can/bittiming.h
> @@ -138,6 +138,11 @@ can_calc_tdco(struct can_tdc *tdc, const struct can_tdc_const *tdc_const,
>  }
>  #endif /* CONFIG_CAN_CALC_BITTIMING */
>
> +void can_sjw_set_default(struct can_bittiming *bt);
> +
> +int can_sjw_check(const struct net_device *dev, const struct can_bittiming *bt,
> +                 const struct can_bittiming_const *btc, struct netlink_ext_ack *extack);
> +
>  int can_get_bittiming(const struct net_device *dev, struct can_bittiming *bt,
>                       const struct can_bittiming_const *btc,
>                       const u32 *bitrate_const,

Re: [PATCH v2 10/17] can: bittiming: factor out can_sjw_set_default() and can_sjw_check()

[Marc Kleine-Budde]

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

On 02.02.2023 20:51:04, Vincent Mailhol wrote:
> On Thu. 2 Feb. 2023 at 20:09, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > Factor out the functionality of assigning a SJW default value into
> > can_sjw_set_default() and the checking the SJW limits into
> > can_sjw_check().
> >
> > This functions will be improved and called from a different function
> > in the following patches.
> >
> > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > ---
> >  drivers/net/can/dev/bittiming.c | 30 ++++++++++++++++++++++++++----
> >  include/linux/can/bittiming.h   |  5 +++++
> >  2 files changed, 31 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
> > index 0b0b8c767c5b..101de1b3bf30 100644
> > --- a/drivers/net/can/dev/bittiming.c
> > +++ b/drivers/net/can/dev/bittiming.c
> > @@ -6,6 +6,24 @@
> >
> >  #include <linux/can/dev.h>
> >
> > +void can_sjw_set_default(struct can_bittiming *bt)
> > +{
> > +       if (bt->sjw)
> > +               return;
> > +
> > +       /* If user space provides no sjw, use 1 as default */
> > +       bt->sjw = 1;
> > +}
> > +
> > +int can_sjw_check(const struct net_device *dev, const struct can_bittiming *bt,
> > +                 const struct can_bittiming_const *btc, struct netlink_ext_ack *extack)
> > +{
> > +       if (bt->sjw > btc->sjw_max)
> > +               return -ERANGE;
> 
> You return -ERANGE here but then replace it by -EINVAL in patch #12.

ACK.

> Better to directly return -EINVAL here.

This patch only factors out the functionality, but doesn't change it on
purpose.

Marc

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

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

Re: [PATCH v2 14/17] can: bittiming: can_sjw_set_default(): use Phase Seg2 / 2 as default for SJW

[Vincent Mailhol]

On Thu. 2 Feb 2023 at 20:09, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> "The (Re-)Synchronization Jump Width (SJW) defines how far a
>  resynchronization may move the Sample Point inside the limits defined
>  by the Phase Buffer Segments to compensate for edge phase errors." [1]
>
> In other words, this means that the SJW parameter controls the CAN
> controller's tolerance to frequency errors compared to other CAN
> controllers.
>
> If the user space doesn't provide a SJW parameter, the
> kernel chooses a default value of 1. This has proven to be a good
> default value for CAN controllers, but no longer for modern
                    ^^^^^^^^^^^^^^^
> controllers.

Are you missing a word here? You oppose CAN controllers to modern ones.

I think the point is Classical CAN only controllers vs. CAN-FD
controllers. A CAN-FD controller is able to sample at bitrates up to 5
or 8 Mbits and have maximum bitimming values five or eight times the
ones of a Classical CAN only controller (which is only capable of
sampling 1 Mbits).

I propose this instead:

  This has proven to be a good default value for Classical CAN
  controllers, but no longer for modern CAN-FD ones.

> In the past there were CAN controllers like the sja1000 with a rather
> limited range of bit timing parameters. For the standard bit rates
> this results in the following bit timing parameters:
>
> | Bit timing parameters for sja1000 with 8.000000 MHz ref clock
> |                     _----+--------------=> tseg1: 1 …   16
> |                    /    /     _---------=> tseg2: 1 …    8
> |                   |    |     /    _-----=> sjw:   1 …    4
> |                   |    |    |    /    _-=> brp:   1 …   64 (inc: 1)
> |                   |    |    |   |    /
> |  nominal          |    |    |   |   |     real  Bitrt    nom   real   SampP
> |  Bitrate TQ[ns] PrS PhS1 PhS2 SJW BRP  Bitrate  Error  SampP  SampP   Error  BTR0 BTR1
> |  1000000    125   2    3    2   1   1  1000000   0.0%  75.0%  75.0%   0.0%   0x00 0x14
> |   800000    125   3    4    2   1   1   800000   0.0%  80.0%  80.0%   0.0%   0x00 0x16
> |   666666    125   4    4    3   1   1   666666   0.0%  80.0%  75.0%   6.2%   0x00 0x27
> |   500000    125   6    7    2   1   1   500000   0.0%  87.5%  87.5%   0.0%   0x00 0x1c
> |   250000    250   6    7    2   1   2   250000   0.0%  87.5%  87.5%   0.0%   0x01 0x1c
> |   125000    500   6    7    2   1   4   125000   0.0%  87.5%  87.5%   0.0%   0x03 0x1c
> |   100000    625   6    7    2   1   5   100000   0.0%  87.5%  87.5%   0.0%   0x04 0x1c
> |    83333    750   6    7    2   1   6    83333   0.0%  87.5%  87.5%   0.0%   0x05 0x1c
> |    50000   1250   6    7    2   1  10    50000   0.0%  87.5%  87.5%   0.0%   0x09 0x1c
> |    33333   1875   6    7    2   1  15    33333   0.0%  87.5%  87.5%   0.0%   0x0e 0x1c
> |    20000   3125   6    7    2   1  25    20000   0.0%  87.5%  87.5%   0.0%   0x18 0x1c
> |    10000   6250   6    7    2   1  50    10000   0.0%  87.5%  87.5%   0.0%   0x31 0x1c
>
> The attentive reader will notice that the SJW is 1 in most cases,
> while the Seg2 phase is 2. Both values are given in TQ units, which in
> turn is a duration in nanoseconds.
>
> For example the 500 kbit/s configuration:
>
> |  nominal                                  real  Bitrt    nom   real   SampP
> |  Bitrate TQ[ns] PrS PhS1 PhS2 SJW BRP  Bitrate  Error  SampP  SampP   Error  BTR0 BTR1
> |   500000    125   6    7    2   1   1   500000   0.0%  87.5%  87.5%   0.0%   0x00 0x1c
>
> the TQ is 125ns, the Phase Seg2 is "2" (== 250ns), the SJW is "1" (==
> 125 ns).
>
> Looking at a more modern CAN controller like a mcp2518fd, it has wider
> bit timing registers.
>
> | Bit timing parameters for mcp251xfd with 40.000000 MHz ref clock
> |                     _----+--------------=> tseg1: 2 …  256
> |                    /    /     _---------=> tseg2: 1 …  128
> |                   |    |     /    _-----=> sjw:   1 …  128
> |                   |    |    |    /    _-=> brp:   1 …  256 (inc: 1)
> |                   |    |    |   |    /
> |  nominal          |    |    |   |   |     real  Bitrt    nom   real   SampP
> |  Bitrate TQ[ns] PrS PhS1 PhS2 SJW BRP  Bitrate  Error  SampP  SampP   Error      NBTCFG
> |   500000     25  34   35   10   1   1   500000   0.0%  87.5%  87.5%   0.0%   0x00440900
>
> The TQ is 25ns, the Phase Seg 2 is "10" (== 250ns), the SJW is "1" (==
> 25ns).
>
> Since the kernel chooses a default SJW of 1 regardless of the TQ, this
> leads to a much smaller SJW and thus much smaller tolerances to
> frequency errors.
>
> To maintain the same oscillator tolerances on controllers with wide
> bit timing registers, select a default SJW value of Phase Seg2 / 2
> unless Phase Seg 1 is less. This results in the following bit timing
> parameters:
>
> | Bit timing parameters for mcp251xfd with 40.000000 MHz ref clock
> |                     _----+--------------=> tseg1: 2 …  256
> |                    /    /     _---------=> tseg2: 1 …  128
> |                   |    |     /    _-----=> sjw:   1 …  128
> |                   |    |    |    /    _-=> brp:   1 …  256 (inc: 1)
> |                   |    |    |   |    /
> |  nominal          |    |    |   |   |     real  Bitrt    nom   real   SampP
> |  Bitrate TQ[ns] PrS PhS1 PhS2 SJW BRP  Bitrate  Error  SampP  SampP   Error      NBTCFG
> |   500000     25  34   35   10   5   1   500000   0.0%  87.5%  87.5%   0.0%   0x00440904
>
> The TQ is 25ns, the Phase Seg 2 is "10" (== 250ns), the SJW is "5" (==
> 125ns). Which is the same as on the sja1000 controller.
>
> [1] http://web.archive.org/http://www.oertel-halle.de/files/cia99paper.pdf
>
> Cc: Mark Bath <mark@baggywrinkle.co.uk>
> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> ---
>  drivers/net/can/dev/bittiming.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
> index 68287b79afe8..55714e08ca3a 100644
> --- a/drivers/net/can/dev/bittiming.c
> +++ b/drivers/net/can/dev/bittiming.c
> @@ -11,8 +11,8 @@ void can_sjw_set_default(struct can_bittiming *bt)
>         if (bt->sjw)
>                 return;
>
> -       /* If user space provides no sjw, use 1 as default */
> -       bt->sjw = 1;
> +       /* If user space provides no sjw, use sane default of phase_seg2 / 2 */
> +       bt->sjw = max(1U, min(bt->phase_seg1, bt->phase_seg2 / 2));
>  }
>
>  int can_sjw_check(const struct net_device *dev, const struct can_bittiming *bt,

Re: [PATCH v2 10/17] can: bittiming: factor out can_sjw_set_default() and can_sjw_check()

[Vincent Mailhol]

On Thu. 2 Feb 2023 at 20:53, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 02.02.2023 20:51:04, Vincent Mailhol wrote:
> > On Thu. 2 Feb. 2023 at 20:09, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > Factor out the functionality of assigning a SJW default value into
> > > can_sjw_set_default() and the checking the SJW limits into
> > > can_sjw_check().
> > >
> > > This functions will be improved and called from a different function
> > > in the following patches.
> > >
> > > Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > > ---
> > >  drivers/net/can/dev/bittiming.c | 30 ++++++++++++++++++++++++++----
> > >  include/linux/can/bittiming.h   |  5 +++++
> > >  2 files changed, 31 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/can/dev/bittiming.c b/drivers/net/can/dev/bittiming.c
> > > index 0b0b8c767c5b..101de1b3bf30 100644
> > > --- a/drivers/net/can/dev/bittiming.c
> > > +++ b/drivers/net/can/dev/bittiming.c
> > > @@ -6,6 +6,24 @@
> > >
> > >  #include <linux/can/dev.h>
> > >
> > > +void can_sjw_set_default(struct can_bittiming *bt)
> > > +{
> > > +       if (bt->sjw)
> > > +               return;
> > > +
> > > +       /* If user space provides no sjw, use 1 as default */
> > > +       bt->sjw = 1;
> > > +}
> > > +
> > > +int can_sjw_check(const struct net_device *dev, const struct can_bittiming *bt,
> > > +                 const struct can_bittiming_const *btc, struct netlink_ext_ack *extack)
> > > +{
> > > +       if (bt->sjw > btc->sjw_max)
> > > +               return -ERANGE;
> >
> > You return -ERANGE here but then replace it by -EINVAL in patch #12.
>
> ACK.
>
> > Better to directly return -EINVAL here.
>
> This patch only factors out the functionality, but doesn't change it on
> purpose.

ACK.

I thought it was a leftover. I would not have done it that way, but I
do not want to over argue on this nitpick. So OK for me.

Re: [PATCH v2 00/17] can: bittiming: cleanups and rework SJW handling

[Vincent Mailhol]

I finished a first quick review and sent my nitpicks. Really great
work, thank you!

On Thu. 2 Feb 2023 at 20:08, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> Hello,
>
> several people noticed that on modern CAN controllers with wide bit
> timing registers the default SJW of 1 can result in unstable or no
> synchronization to the CAN network. See Patch 14/17 for details.

If this series addresses an actual issue, should it be backported to
stable branches?

> During review of v1 Vincent pointed out that the original code and the
> series doesn't always check user provided bit timing parameters,
> sometimes silently limits them and the return error values are not
> consistent.
>
> This series first cleans up some code in bittiming.c, replacing
> open-coded variants by macros or functions (Patches 1, 2).
>
> Patch 3 adds the missing assignment of the effective TQ if the
> interface is configured with low level timing parameters.
>
> Patch 4 is another code cleanup.
>
> Patches 5, 6 check the bit timing parameter during interface
> registration.
>
> Patch 7 adds a validation of the sample point.
>
> The patches 8-13 convert the error messages from netdev_err() to
> NL_SET_ERR_MSG_FMT, factor out the SJW handling from
> can_fixup_bittiming(), add checking and error messages for the
> individual limits and harmonize the error return values.
>
> Patch 14 changes the default SJW value from 1 to min(Phase Seg1, Phase
> Seg2 / 2).
>
> Patch 15 switches can_calc_bittiming() to use the new SJW handling.
>
> Patch 16 converts can_calc_bittiming() to NL_SET_ERR_MSG_FMT().
>
> And patch 16 adds a NL_SET_ERR_MSG_FMT() error message to
> can_validate_bitrate().

Re: [PATCH v2 14/17] can: bittiming: can_sjw_set_default(): use Phase Seg2 / 2 as default for SJW

[Marc Kleine-Budde]

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

On 02.02.2023 20:57:42, Vincent Mailhol wrote:
> On Thu. 2 Feb 2023 at 20:09, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > "The (Re-)Synchronization Jump Width (SJW) defines how far a
> >  resynchronization may move the Sample Point inside the limits defined
> >  by the Phase Buffer Segments to compensate for edge phase errors." [1]
> >
> > In other words, this means that the SJW parameter controls the CAN
> > controller's tolerance to frequency errors compared to other CAN
> > controllers.
> >
> > If the user space doesn't provide a SJW parameter, the
> > kernel chooses a default value of 1. This has proven to be a good
> > default value for CAN controllers, but no longer for modern
>                     ^^^^^^^^^^^^^^^
> > controllers.
> 
> Are you missing a word here? You oppose CAN controllers to modern
> ones.
> 
> I think the point is Classical CAN only controllers vs. CAN-FD
> controllers. A CAN-FD controller is able to sample at bitrates up to 5
> or 8 Mbits and have maximum bitimming values five or eight times the
> ones of a Classical CAN only controller (which is only capable of
> sampling 1 Mbits).
> 
> I propose this instead:
> 
>   This has proven to be a good default value for Classical CAN
>   controllers, but no longer for modern CAN-FD ones.

The difference that matters here is not that the controllers support
CAN-FD, but that they have a much greater max tseg{1,2} compared to the
sja1000. But that's only the case on CAN-FD controller.

Will change the description as you proposed!

Thanks,
Marc

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

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

Re: [PATCH v2 00/17] can: bittiming: cleanups and rework SJW handling

[Marc Kleine-Budde]

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

On 02.02.2023 21:05:23, Vincent Mailhol wrote:
> I finished a first quick review and sent my nitpicks. Really great
> work, thank you!
> 
> On Thu. 2 Feb 2023 at 20:08, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > Hello,
> >
> > several people noticed that on modern CAN controllers with wide bit
> > timing registers the default SJW of 1 can result in unstable or no
> > synchronization to the CAN network. See Patch 14/17 for details.
> 
> If this series addresses an actual issue, should it be backported to
> stable branches?

Hmmm, the changes are quite big so I want to have it tested a bit more
before it hits mainline. If there's interest in/need for back porting we
can think about it.

regards,
Marc

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

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

Re: [PATCH v2 14/17] can: bittiming: can_sjw_set_default(): use Phase Seg2 / 2 as default for SJW

[Marc Kleine-Budde]

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

On 02.02.2023 13:07:38, Marc Kleine-Budde wrote:
> On 02.02.2023 20:57:42, Vincent Mailhol wrote:
> > On Thu. 2 Feb 2023 at 20:09, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > "The (Re-)Synchronization Jump Width (SJW) defines how far a
> > >  resynchronization may move the Sample Point inside the limits defined
> > >  by the Phase Buffer Segments to compensate for edge phase errors." [1]
> > >
> > > In other words, this means that the SJW parameter controls the CAN
> > > controller's tolerance to frequency errors compared to other CAN
> > > controllers.
> > >
> > > If the user space doesn't provide a SJW parameter, the
> > > kernel chooses a default value of 1. This has proven to be a good
> > > default value for CAN controllers, but no longer for modern
> >                     ^^^^^^^^^^^^^^^
> > > controllers.
> > 
> > Are you missing a word here? You oppose CAN controllers to modern
> > ones.
> > 
> > I think the point is Classical CAN only controllers vs. CAN-FD
> > controllers. A CAN-FD controller is able to sample at bitrates up to 5
> > or 8 Mbits and have maximum bitimming values five or eight times the
> > ones of a Classical CAN only controller (which is only capable of
> > sampling 1 Mbits).
> > 
> > I propose this instead:
> > 
> >   This has proven to be a good default value for Classical CAN
> >   controllers, but no longer for modern CAN-FD ones.
> 
> The difference that matters here is not that the controllers support
> CAN-FD, but that they have a much greater max tseg{1,2} compared to the
> sja1000. But that's only the case on CAN-FD controller.
> 
> Will change the description as you proposed!

There are no other changes on this series, I've fixed the patch
description as suggested. I'll take this series without resending it.

Thanks again for the review!

regards,
Marc

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

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