[PATCH v7 0/7] Add CAN FD infrastructure for CAN driver

[PATCH v7 0/7] Add CAN FD infrastructure for CAN driver

[Oliver Hartkopp]

This is the patch series to extend the CAN driver infrastructure with CAN FD.

Changes v6 -> v7:

Reworked patch 4 (can: provide a separate bittiming_const ...) to make it
even smaller and clearer (see commit message).

Regards,
Oliver


*** BLURB HERE ***

Oliver Hartkopp (7):
  can: preserve skbuff protocol in can_put_echo_skb
  can: only send bitrate data via netlink when available
  can: move sanity check for bitrate and tq into can_get_bittiming
  can: provide a separate bittiming_const parameter to bittiming
    functions
  can: introduce the data bitrate configuration for CAN FD
  can: allow to change the device mtu for CAN FD capable devices
  can: add bittiming check at interface open for CAN FD

 drivers/net/can/dev.c            | 160 ++++++++++++++++++++++++++++++---------
 include/linux/can/dev.h          |   7 +-
 include/uapi/linux/can/netlink.h |   3 +
 3 files changed, 133 insertions(+), 37 deletions(-)

-- 
1.9.0

[PATCH v7 1/7] can: preserve skbuff protocol in can_put_echo_skb

[Oliver Hartkopp]

The skbuff protocol value was formerly fixed/sanitized to ETH_P_CAN in
can_put_echo_skb(). With CAN FD this value has to be preserved.
This patch changes the hard assignment of the protocol value to a check of
valid protocol values for CAN and CAN FD.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index c0563f1..e1a3741 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -317,7 +317,9 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 	BUG_ON(idx >= priv->echo_skb_max);
 
 	/* check flag whether this packet has to be looped back */
-	if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
+	if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK ||
+	    (skb->protocol != htons(ETH_P_CAN) &&
+	     skb->protocol != htons(ETH_P_CANFD))) {
 		kfree_skb(skb);
 		return;
 	}
@@ -329,7 +331,6 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 			return;
 
 		/* make settings for echo to reduce code in irq context */
-		skb->protocol = htons(ETH_P_CAN);
 		skb->pkt_type = PACKET_BROADCAST;
 		skb->ip_summed = CHECKSUM_UNNECESSARY;
 		skb->dev = dev;
-- 
1.9.0

[PATCH v7 2/7] can: only send bitrate data via netlink when available

[Oliver Hartkopp]

When setting the bitrate both can_calc_bittiming() and can_fixup_bittiming()
lead to the bitrate variable to be set, when a proper bit timing is available.
Only then the bitrate configuration is stored for the device, so checking for
priv->bittiming.bitrate is always sufficient.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index e1a3741..de04eac 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -606,7 +606,7 @@ int open_candev(struct net_device *dev)
 {
 	struct can_priv *priv = netdev_priv(dev);
 
-	if (!priv->bittiming.tq && !priv->bittiming.bitrate) {
+	if (!priv->bittiming.bitrate) {
 		netdev_err(dev, "bit-timing not yet defined\n");
 		return -EINVAL;
 	}
@@ -719,7 +719,8 @@ static size_t can_get_size(const struct net_device *dev)
 	struct can_priv *priv = netdev_priv(dev);
 	size_t size = 0;
 
-	size += nla_total_size(sizeof(struct can_bittiming));	/* IFLA_CAN_BITTIMING */
+	if (priv->bittiming.bitrate)				/* IFLA_CAN_BITTIMING */
+		size += nla_total_size(sizeof(struct can_bittiming));
 	if (priv->bittiming_const)				/* IFLA_CAN_BITTIMING_CONST */
 		size += nla_total_size(sizeof(struct can_bittiming_const));
 	size += nla_total_size(sizeof(struct can_clock));	/* IFLA_CAN_CLOCK */
@@ -741,8 +742,9 @@ 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 (nla_put(skb, IFLA_CAN_BITTIMING,
-		    sizeof(priv->bittiming), &priv->bittiming) ||
+	if ((priv->bittiming.bitrate &&
+	     nla_put(skb, IFLA_CAN_BITTIMING,
+		     sizeof(priv->bittiming), &priv->bittiming)) ||
 	    (priv->bittiming_const &&
 	     nla_put(skb, IFLA_CAN_BITTIMING_CONST,
 		     sizeof(*priv->bittiming_const), priv->bittiming_const)) ||
-- 
1.9.0

[PATCH v7 3/7] can: move sanity check for bitrate and tq into can_get_bittiming

[Oliver Hartkopp]

This patch moves a sanity check in order to have a second user for CAN FD.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index de04eac..89fc798 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -263,12 +263,19 @@ static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt)
 	if (priv->bittiming_const) {
 
 		/* Non-expert mode? Check if the bitrate has been pre-defined */
-		if (!bt->tq)
+		if (!bt->tq) {
 			/* Determine bit-timing parameters */
-			err = can_calc_bittiming(dev, bt);
-		else
+			if (bt->bitrate)
+				err = can_calc_bittiming(dev, bt);
+			else
+				return -EINVAL;
+		} else {
 			/* Check bit-timing params and calculate proper brp */
-			err = can_fixup_bittiming(dev, bt);
+			if (!bt->bitrate)
+				err = can_fixup_bittiming(dev, bt);
+			else
+				return -EINVAL;
+		}
 		if (err)
 			return err;
 	}
@@ -667,8 +674,6 @@ static int can_changelink(struct net_device *dev,
 		if (dev->flags & IFF_UP)
 			return -EBUSY;
 		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
-		if ((!bt.bitrate && !bt.tq) || (bt.bitrate && bt.tq))
-			return -EINVAL;
 		err = can_get_bittiming(dev, &bt);
 		if (err)
 			return err;
-- 
1.9.0

[PATCH v7 4/7] can: provide a separate bittiming_const parameter to bittiming functions

[Oliver Hartkopp]

As the bittiming calculation functions are to be used with different
bittiming_const structures for CAN and CAN FD the direct reference to
priv->bittiming_const inside these functions has to be removed.

Also simplify the return value generation in can_get_bittiming() as only
correct return values of can_[calc|fixup]_bittiming() lead to a return value
of zero. And moved the check for existing bittiming const to one place.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev.c | 59 ++++++++++++++++++++++-----------------------------
 1 file changed, 25 insertions(+), 34 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 89fc798..c8cd528 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -99,10 +99,10 @@ static int can_update_spt(const struct can_bittiming_const *btc,
 	return 1000 * (tseg + 1 - *tseg2) / (tseg + 1);
 }
 
-static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
+static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
+			      const struct can_bittiming_const *btc)
 {
 	struct can_priv *priv = netdev_priv(dev);
-	const struct can_bittiming_const *btc = priv->bittiming_const;
 	long rate, best_rate = 0;
 	long best_error = 1000000000, error = 0;
 	int best_tseg = 0, best_brp = 0, brp = 0;
@@ -110,9 +110,6 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
 	int spt_error = 1000, spt = 0, sampl_pt;
 	u64 v64;
 
-	if (!priv->bittiming_const)
-		return -ENOTSUPP;
-
 	/* Use CIA recommended sample points */
 	if (bt->sample_point) {
 		sampl_pt = bt->sample_point;
@@ -204,7 +201,8 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
 	return 0;
 }
 #else /* !CONFIG_CAN_CALC_BITTIMING */
-static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
+static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
+			      const struct can_bittiming_const *btc)
 {
 	netdev_err(dev, "bit-timing calculation not available\n");
 	return -EINVAL;
@@ -217,16 +215,13 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
  * prescaler value brp. You can find more information in the header
  * file linux/can/netlink.h.
  */
-static int can_fixup_bittiming(struct net_device *dev, struct can_bittiming *bt)
+static int can_fixup_bittiming(struct net_device *dev, struct can_bittiming *bt,
+			       const struct can_bittiming_const *btc)
 {
 	struct can_priv *priv = netdev_priv(dev);
-	const struct can_bittiming_const *btc = priv->bittiming_const;
 	int tseg1, alltseg;
 	u64 brp64;
 
-	if (!priv->bittiming_const)
-		return -ENOTSUPP;
-
 	tseg1 = bt->prop_seg + bt->phase_seg1;
 	if (!bt->sjw)
 		bt->sjw = 1;
@@ -254,33 +249,29 @@ static int can_fixup_bittiming(struct net_device *dev, struct can_bittiming *bt)
 	return 0;
 }
 
-static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt)
+static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt,
+			     const struct can_bittiming_const *btc)
 {
-	struct can_priv *priv = netdev_priv(dev);
-	int err;
+	int err = -EINVAL;
 
 	/* Check if the CAN device has bit-timing parameters */
-	if (priv->bittiming_const) {
-
-		/* Non-expert mode? Check if the bitrate has been pre-defined */
-		if (!bt->tq) {
-			/* Determine bit-timing parameters */
-			if (bt->bitrate)
-				err = can_calc_bittiming(dev, bt);
-			else
-				return -EINVAL;
-		} else {
-			/* Check bit-timing params and calculate proper brp */
-			if (!bt->bitrate)
-				err = can_fixup_bittiming(dev, bt);
-			else
-				return -EINVAL;
-		}
-		if (err)
-			return err;
+	if (!btc)
+		return -ENOTSUPP;
+
+	if (!bt->tq) {
+		/* non-expert mode: Determine bit-timing parameters */
+		if (bt->bitrate)
+			err = can_calc_bittiming(dev, bt, btc);
+	} else {
+		/*
+		 * expert mode: Check bit-timing parameters and
+		 * calculate proper brp and a human readable bitrate
+		 */
+		if (!bt->bitrate)
+			err = can_fixup_bittiming(dev, bt, btc);
 	}
 
-	return 0;
+	return err;
 }
 
 /*
@@ -674,7 +665,7 @@ static int can_changelink(struct net_device *dev,
 		if (dev->flags & IFF_UP)
 			return -EBUSY;
 		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
-		err = can_get_bittiming(dev, &bt);
+		err = can_get_bittiming(dev, &bt, priv->bittiming_const);
 		if (err)
 			return err;
 		memcpy(&priv->bittiming, &bt, sizeof(bt));
-- 
1.9.0

[PATCH v7 5/7] can: introduce the data bitrate configuration for CAN FD

[Oliver Hartkopp]

As CAN FD offers a second bitrate for the data section of the CAN frame the
infrastructure for storing and configuring this second bitrate is introduced.
Improved the readability of the if-statement by inserting some newlines.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev.c            | 45 +++++++++++++++++++++++++++++++++++++++-
 include/linux/can/dev.h          |  6 ++++--
 include/uapi/linux/can/netlink.h |  2 ++
 3 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index c8cd528..b2b0a9b 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -647,6 +647,10 @@ static const struct nla_policy can_policy[IFLA_CAN_MAX + 1] = {
 				= { .len = sizeof(struct can_bittiming_const) },
 	[IFLA_CAN_CLOCK]	= { .len = sizeof(struct can_clock) },
 	[IFLA_CAN_BERR_COUNTER]	= { .len = sizeof(struct can_berr_counter) },
+	[IFLA_CAN_DATA_BITTIMING]
+				= { .len = sizeof(struct can_bittiming) },
+	[IFLA_CAN_DATA_BITTIMING_CONST]
+				= { .len = sizeof(struct can_bittiming_const) },
 };
 
 static int can_changelink(struct net_device *dev,
@@ -678,6 +682,27 @@ static int can_changelink(struct net_device *dev,
 		}
 	}
 
+	if (data[IFLA_CAN_DATA_BITTIMING]) {
+		struct can_bittiming dbt;
+
+		/* Do not allow changing bittiming while running */
+		if (dev->flags & IFF_UP)
+			return -EBUSY;
+		memcpy(&dbt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
+		       sizeof(dbt));
+		err = can_get_bittiming(dev, &dbt, priv->data_bittiming_const);
+		if (err)
+			return err;
+		memcpy(&priv->data_bittiming, &dbt, sizeof(dbt));
+
+		if (priv->do_set_data_bittiming) {
+			/* Finally, set the bit-timing registers */
+			err = priv->do_set_data_bittiming(dev);
+			if (err)
+				return err;
+		}
+	}
+
 	if (data[IFLA_CAN_CTRLMODE]) {
 		struct can_ctrlmode *cm;
 
@@ -725,6 +750,10 @@ 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->data_bittiming.bitrate)			/* IFLA_CAN_DATA_BITTIMING */
+		size += nla_total_size(sizeof(struct can_bittiming));
+	if (priv->data_bittiming_const)				/* IFLA_CAN_DATA_BITTIMING_CONST */
+		size += nla_total_size(sizeof(struct can_bittiming_const));
 
 	return size;
 }
@@ -738,20 +767,34 @@ 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 &&
 	     nla_put(skb, IFLA_CAN_BITTIMING,
 		     sizeof(priv->bittiming), &priv->bittiming)) ||
+
 	    (priv->bittiming_const &&
 	     nla_put(skb, IFLA_CAN_BITTIMING_CONST,
 		     sizeof(*priv->bittiming_const), priv->bittiming_const)) ||
+
 	    nla_put(skb, IFLA_CAN_CLOCK, sizeof(cm), &priv->clock) ||
 	    nla_put_u32(skb, IFLA_CAN_STATE, state) ||
 	    nla_put(skb, IFLA_CAN_CTRLMODE, sizeof(cm), &cm) ||
 	    nla_put_u32(skb, IFLA_CAN_RESTART_MS, priv->restart_ms) ||
+
 	    (priv->do_get_berr_counter &&
 	     !priv->do_get_berr_counter(dev, &bec) &&
-	     nla_put(skb, IFLA_CAN_BERR_COUNTER, sizeof(bec), &bec)))
+	     nla_put(skb, IFLA_CAN_BERR_COUNTER, sizeof(bec), &bec)) ||
+
+	    (priv->data_bittiming.bitrate &&
+	     nla_put(skb, IFLA_CAN_DATA_BITTIMING,
+		     sizeof(priv->data_bittiming), &priv->data_bittiming)) ||
+
+	    (priv->data_bittiming_const &&
+	     nla_put(skb, IFLA_CAN_DATA_BITTIMING_CONST,
+		     sizeof(*priv->data_bittiming_const),
+		     priv->data_bittiming_const)))
 		return -EMSGSIZE;
+
 	return 0;
 }
 
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index dc5f902..8adaee9 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -33,8 +33,9 @@ enum can_mode {
 struct can_priv {
 	struct can_device_stats can_stats;
 
-	struct can_bittiming bittiming;
-	const struct can_bittiming_const *bittiming_const;
+	struct can_bittiming bittiming, data_bittiming;
+	const struct can_bittiming_const *bittiming_const,
+		*data_bittiming_const;
 	struct can_clock clock;
 
 	enum can_state state;
@@ -45,6 +46,7 @@ struct can_priv {
 	struct timer_list restart_timer;
 
 	int (*do_set_bittiming)(struct net_device *dev);
+	int (*do_set_data_bittiming)(struct net_device *dev);
 	int (*do_set_mode)(struct net_device *dev, enum can_mode mode);
 	int (*do_get_state)(const struct net_device *dev,
 			    enum can_state *state);
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index df944ed..b41933d 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -122,6 +122,8 @@ enum {
 	IFLA_CAN_RESTART_MS,
 	IFLA_CAN_RESTART,
 	IFLA_CAN_BERR_COUNTER,
+	IFLA_CAN_DATA_BITTIMING,
+	IFLA_CAN_DATA_BITTIMING_CONST,
 	__IFLA_CAN_MAX
 };
 
-- 
1.9.0

[PATCH v7 6/7] can: allow to change the device mtu for CAN FD capable devices

[Oliver Hartkopp]

The configuration for CAN FD depends on CAN_CTRLMODE_FD enabled in the driver
specific ctrlmode_supported capabilities.

The configuration can be done either with the 'fd { on | off }' option in the
'ip' tool from iproute2 or by setting the CAN netdevice MTU to CAN_MTU (16) or
to CANFD_MTU (72).

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev.c            | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/can/dev.h          |  1 +
 include/uapi/linux/can/netlink.h |  1 +
 3 files changed, 42 insertions(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index b2b0a9b..f2825b4 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -595,6 +595,40 @@ void free_candev(struct net_device *dev)
 EXPORT_SYMBOL_GPL(free_candev);
 
 /*
+ * changing MTU and control mode for CAN/CANFD devices
+ */
+int can_change_mtu(struct net_device *dev, int new_mtu)
+{
+	struct can_priv *priv = netdev_priv(dev);
+
+	/* Do not allow changing the MTU while running */
+	if (dev->flags & IFF_UP)
+		return -EBUSY;
+
+	/* allow change of MTU according to the CANFD ability of the device */
+	switch (new_mtu) {
+
+	case CAN_MTU:
+		priv->ctrlmode &= ~CAN_CTRLMODE_FD;
+		break;
+
+	case CANFD_MTU:
+		if (!(priv->ctrlmode_supported & CAN_CTRLMODE_FD))
+			return -EINVAL;
+
+		priv->ctrlmode |= CAN_CTRLMODE_FD;
+		break;
+
+	default:
+			return -EINVAL;
+	}
+
+	dev->mtu = new_mtu;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(can_change_mtu);
+
+/*
  * Common open function when the device gets opened.
  *
  * This function should be called in the open function of the device
@@ -714,6 +748,12 @@ static int can_changelink(struct net_device *dev,
 			return -EOPNOTSUPP;
 		priv->ctrlmode &= ~cm->mask;
 		priv->ctrlmode |= cm->flags;
+
+		/* 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;
 	}
 
 	if (data[IFLA_CAN_RESTART_MS]) {
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 8adaee9..3ce5e52 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -113,6 +113,7 @@ struct can_priv *safe_candev_priv(struct net_device *dev);
 
 int open_candev(struct net_device *dev);
 void close_candev(struct net_device *dev);
+int can_change_mtu(struct net_device *dev, int new_mtu);
 
 int register_candev(struct net_device *dev);
 void unregister_candev(struct net_device *dev);
diff --git a/include/uapi/linux/can/netlink.h b/include/uapi/linux/can/netlink.h
index b41933d..7e2e186 100644
--- a/include/uapi/linux/can/netlink.h
+++ b/include/uapi/linux/can/netlink.h
@@ -96,6 +96,7 @@ struct can_ctrlmode {
 #define CAN_CTRLMODE_3_SAMPLES		0x04	/* Triple sampling mode */
 #define CAN_CTRLMODE_ONE_SHOT		0x08	/* One-Shot mode */
 #define CAN_CTRLMODE_BERR_REPORTING	0x10	/* Bus-error reporting */
+#define CAN_CTRLMODE_FD			0x20	/* CAN FD mode */
 
 /*
  * CAN device statistics
-- 
1.9.0

[PATCH v7 7/7] can: add bittiming check at interface open for CAN FD

[Oliver Hartkopp]

Additionally to have the second (data) bitrate available the data bitrate
has to be greater or equal to the arbitration bitrate in CAN FD.

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
 drivers/net/can/dev.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index f2825b4..05b5dc2 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -643,6 +643,14 @@ int open_candev(struct net_device *dev)
 		return -EINVAL;
 	}
 
+	/* For CAN FD the data bitrate has to be >= the arbitration bitrate */
+	if ((priv->ctrlmode & CAN_CTRLMODE_FD) &&
+	    (!priv->data_bittiming.bitrate ||
+	     (priv->data_bittiming.bitrate < priv->bittiming.bitrate))) {
+		netdev_err(dev, "incorrect/missing data bit-timing\n");
+		return -EINVAL;
+	}
+
 	/* Switch carrier on if device was stopped while in bus-off state */
 	if (!netif_carrier_ok(dev))
 		netif_carrier_on(dev);
-- 
1.9.0

Re: [PATCH v7 4/7] can: provide a separate bittiming_const parameter to bittiming functions

[Marc Kleine-Budde]

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

On 02/28/2014 11:26 AM, Oliver Hartkopp wrote:
> As the bittiming calculation functions are to be used with different
> bittiming_const structures for CAN and CAN FD the direct reference to
> priv->bittiming_const inside these functions has to be removed.
> 
> Also simplify the return value generation in can_get_bittiming() as only
> correct return values of can_[calc|fixup]_bittiming() lead to a return value
> of zero. And moved the check for existing bittiming const to one place.
> 
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
>  drivers/net/can/dev.c | 59 ++++++++++++++++++++++-----------------------------
>  1 file changed, 25 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index 89fc798..c8cd528 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -99,10 +99,10 @@ static int can_update_spt(const struct can_bittiming_const *btc,
>  	return 1000 * (tseg + 1 - *tseg2) / (tseg + 1);
>  }
>  
> -static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
> +static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
> +			      const struct can_bittiming_const *btc)
>  {
>  	struct can_priv *priv = netdev_priv(dev);
> -	const struct can_bittiming_const *btc = priv->bittiming_const;
>  	long rate, best_rate = 0;
>  	long best_error = 1000000000, error = 0;
>  	int best_tseg = 0, best_brp = 0, brp = 0;
> @@ -110,9 +110,6 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
>  	int spt_error = 1000, spt = 0, sampl_pt;
>  	u64 v64;
>  
> -	if (!priv->bittiming_const)
> -		return -ENOTSUPP;
> -
>  	/* Use CIA recommended sample points */
>  	if (bt->sample_point) {
>  		sampl_pt = bt->sample_point;
> @@ -204,7 +201,8 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
>  	return 0;
>  }
>  #else /* !CONFIG_CAN_CALC_BITTIMING */
> -static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
> +static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt,
> +			      const struct can_bittiming_const *btc)
>  {
>  	netdev_err(dev, "bit-timing calculation not available\n");
>  	return -EINVAL;
> @@ -217,16 +215,13 @@ static int can_calc_bittiming(struct net_device *dev, struct can_bittiming *bt)
>   * prescaler value brp. You can find more information in the header
>   * file linux/can/netlink.h.
>   */
> -static int can_fixup_bittiming(struct net_device *dev, struct can_bittiming *bt)
> +static int can_fixup_bittiming(struct net_device *dev, struct can_bittiming *bt,
> +			       const struct can_bittiming_const *btc)
>  {
>  	struct can_priv *priv = netdev_priv(dev);
> -	const struct can_bittiming_const *btc = priv->bittiming_const;
>  	int tseg1, alltseg;
>  	u64 brp64;
>  
> -	if (!priv->bittiming_const)
> -		return -ENOTSUPP;
> -
>  	tseg1 = bt->prop_seg + bt->phase_seg1;
>  	if (!bt->sjw)
>  		bt->sjw = 1;
> @@ -254,33 +249,29 @@ static int can_fixup_bittiming(struct net_device *dev, struct can_bittiming *bt)
>  	return 0;
>  }
>  
> -static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt)
> +static int can_get_bittiming(struct net_device *dev, struct can_bittiming *bt,
> +			     const struct can_bittiming_const *btc)
>  {
> -	struct can_priv *priv = netdev_priv(dev);
> -	int err;
> +	int err = -EINVAL;

Keep err uninitialized and make an explicit else err = -EINVAL.... see
below:

>  
>  	/* Check if the CAN device has bit-timing parameters */
> -	if (priv->bittiming_const) {
> -
> -		/* Non-expert mode? Check if the bitrate has been pre-defined */
> -		if (!bt->tq) {
> -			/* Determine bit-timing parameters */
> -			if (bt->bitrate)
> -				err = can_calc_bittiming(dev, bt);
> -			else
> -				return -EINVAL;
> -		} else {
> -			/* Check bit-timing params and calculate proper brp */
> -			if (!bt->bitrate)
> -				err = can_fixup_bittiming(dev, bt);
> -			else
> -				return -EINVAL;
> -		}
> -		if (err)
> -			return err;
> +	if (!btc)
> +		return -ENOTSUPP;
> +
> +	if (!bt->tq) {
> +		/* non-expert mode: Determine bit-timing parameters */
> +		if (bt->bitrate)

Why not:
	if (!bt->tq && bt->bitrate) {

> +			err = can_calc_bittiming(dev, bt, btc);
> +	} else {

	} else if (bt->tq && !bt->bitrate) {

> +		/*
> +		 * expert mode: Check bit-timing parameters and
> +		 * calculate proper brp and a human readable bitrate
> +		 */
> +		if (!bt->bitrate)
> +			err = can_fixup_bittiming(dev, bt, btc);
>  	}
	} else {
		err = -EINVAL;
	}
>  
> -	return 0;
> +	return err;
>  }
>  
>  /*
> @@ -674,7 +665,7 @@ static int can_changelink(struct net_device *dev,
>  		if (dev->flags & IFF_UP)
>  			return -EBUSY;
>  		memcpy(&bt, nla_data(data[IFLA_CAN_BITTIMING]), sizeof(bt));
> -		err = can_get_bittiming(dev, &bt);
> +		err = can_get_bittiming(dev, &bt, priv->bittiming_const);
>  		if (err)
>  			return err;
>  		memcpy(&priv->bittiming, &bt, sizeof(bt));
> 


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v7 4/7] can: provide a separate bittiming_const parameter to bittiming functions

[Oliver Hartkopp]

On 28.02.2014 11:46, Marc Kleine-Budde wrote:
> On 02/28/2014 11:26 AM, Oliver Hartkopp wrote:


>>  {
>> -	struct can_priv *priv = netdev_priv(dev);
>> -	int err;
>> +	int err = -EINVAL;
> 
> Keep err uninitialized and make an explicit else err = -EINVAL.... see
> below:
> 


>> +	if (!bt->tq) {
>> +		/* non-expert mode: Determine bit-timing parameters */
>> +		if (bt->bitrate)
> 
> Why not:
> 	if (!bt->tq && bt->bitrate) {
> 
>> +			err = can_calc_bittiming(dev, bt, btc);
>> +	} else {
> 
> 	} else if (bt->tq && !bt->bitrate) {
> 
>> +		/*
>> +		 * expert mode: Check bit-timing parameters and
>> +		 * calculate proper brp and a human readable bitrate
>> +		 */
>> +		if (!bt->bitrate)
>> +			err = can_fixup_bittiming(dev, bt, btc);
>>  	}
> 	} else {
> 		err = -EINVAL;
> 	}

I don't have a strong preference on my implementation.

But IMHO your suggested changes only add additional code and additional
comparisons in the if statement.

I don't see a benefit in your suggestions :-(

Regards,
Oliver

Re: [PATCH v7 4/7] can: provide a separate bittiming_const parameter to bittiming functions

[Marc Kleine-Budde]

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

On 02/28/2014 01:31 PM, Oliver Hartkopp wrote:
> 
> 
> On 28.02.2014 11:46, Marc Kleine-Budde wrote:
>> On 02/28/2014 11:26 AM, Oliver Hartkopp wrote:
> 
> 
>>>  {
>>> -	struct can_priv *priv = netdev_priv(dev);
>>> -	int err;
>>> +	int err = -EINVAL;
>>
>> Keep err uninitialized and make an explicit else err = -EINVAL.... see
>> below:
>>
> 
> 
>>> +	if (!bt->tq) {
>>> +		/* non-expert mode: Determine bit-timing parameters */
>>> +		if (bt->bitrate)
>>
>> Why not:
>> 	if (!bt->tq && bt->bitrate) {
>>
>>> +			err = can_calc_bittiming(dev, bt, btc);
>>> +	} else {
>>
>> 	} else if (bt->tq && !bt->bitrate) {
>>
>>> +		/*
>>> +		 * expert mode: Check bit-timing parameters and
>>> +		 * calculate proper brp and a human readable bitrate
>>> +		 */
>>> +		if (!bt->bitrate)
>>> +			err = can_fixup_bittiming(dev, bt, btc);
>>>  	}
>> 	} else {
>> 		err = -EINVAL;
>> 	}
> 
> I don't have a strong preference on my implementation.
> 
> But IMHO your suggested changes only add additional code and additional
> comparisons in the if statement.
> 
> I don't see a benefit in your suggestions :-(

You have 3 seperate if()s and an implicit error value from the
initialisation. This is IMHO more straight forward:

	if (!bt->tq && bt->bitrate)
		err = can_calc_bittiming(dev, bt, btc);
	else if (bt->tq && !bt->bitrate)
		err = can_fixup_bittiming(dev, bt, btc);
	else
		err = -EINVAL;

Marc

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 242 bytes --]

Re: [PATCH v7 4/7] can: provide a separate bittiming_const parameter to bittiming functions

[Oliver Hartkopp]

On 28.02.2014 14:02, Marc Kleine-Budde wrote:
> On 02/28/2014 01:31 PM, Oliver Hartkopp wrote:
>>
>>
>> On 28.02.2014 11:46, Marc Kleine-Budde wrote:
>>> On 02/28/2014 11:26 AM, Oliver Hartkopp wrote:
>>
>>
>>>>  {
>>>> -	struct can_priv *priv = netdev_priv(dev);
>>>> -	int err;
>>>> +	int err = -EINVAL;
>>>
>>> Keep err uninitialized and make an explicit else err = -EINVAL.... see
>>> below:
>>>
>>
>>
>>>> +	if (!bt->tq) {
>>>> +		/* non-expert mode: Determine bit-timing parameters */
>>>> +		if (bt->bitrate)
>>>
>>> Why not:
>>> 	if (!bt->tq && bt->bitrate) {
>>>
>>>> +			err = can_calc_bittiming(dev, bt, btc);
>>>> +	} else {
>>>
>>> 	} else if (bt->tq && !bt->bitrate) {
>>>
>>>> +		/*
>>>> +		 * expert mode: Check bit-timing parameters and
>>>> +		 * calculate proper brp and a human readable bitrate
>>>> +		 */
>>>> +		if (!bt->bitrate)
>>>> +			err = can_fixup_bittiming(dev, bt, btc);
>>>>  	}
>>> 	} else {
>>> 		err = -EINVAL;
>>> 	}
>>
>> I don't have a strong preference on my implementation.
>>
>> But IMHO your suggested changes only add additional code and additional
>> comparisons in the if statement.
>>
>> I don't see a benefit in your suggestions :-(
> 
> You have 3 seperate if()s and an implicit error value from the
> initialisation. This is IMHO more straight forward:
> 
> 	if (!bt->tq && bt->bitrate)
> 		err = can_calc_bittiming(dev, bt, btc);
> 	else if (bt->tq && !bt->bitrate)
> 		err = can_fixup_bittiming(dev, bt, btc);
> 	else
> 		err = -EINVAL;
> 

Ok - looks good too.

Will change this in the v8 patch series.

Regards,
Oliver