[PATCH v3 1/5] can: preserve skbuff protocol in can_put_echo_skb

[PATCH v3 1/5] 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.rc3

[PATCH v3 2/5] can: netlink send bittiming data only when a bitrate is available

[Oliver Hartkopp]

This is also a preparation for the CAN FD specific bittiming configurations
which are not existing in non-FD drivers.

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

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index e1a3741..9f99255 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -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,15 +742,20 @@ 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)) ||
+
 	    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)))
-- 
1.9.0.rc3

[PATCH v3 3/5] 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.

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

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 9f99255..3a19333 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,7 +110,7 @@ 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)
+	if (!btc)
 		return -ENOTSUPP;
 
 	/* Use CIA recommended sample points */
@@ -204,7 +204,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,14 +218,14 @@ 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)
+	if (!btc)
 		return -ENOTSUPP;
 
 	tseg1 = bt->prop_seg + bt->phase_seg1;
@@ -254,21 +255,21 @@ 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;
 
 	/* Check if the CAN device has bit-timing parameters */
-	if (priv->bittiming_const) {
+	if (btc) {
 
 		/* Non-expert mode? Check if the bitrate has been pre-defined */
 		if (!bt->tq)
 			/* Determine bit-timing parameters */
-			err = can_calc_bittiming(dev, bt);
+			err = can_calc_bittiming(dev, bt, btc);
 		else
 			/* Check bit-timing params and calculate proper brp */
-			err = can_fixup_bittiming(dev, bt);
+			err = can_fixup_bittiming(dev, bt, btc);
 		if (err)
 			return err;
 	}
@@ -669,7 +670,7 @@ static int can_changelink(struct net_device *dev,
 		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);
+		err = can_get_bittiming(dev, &bt, priv->bittiming_const);
 		if (err)
 			return err;
 		memcpy(&priv->bittiming, &bt, sizeof(bt));
-- 
1.9.0.rc3

[PATCH v3 4/5] can: introduce the data bitrate configuration for CAN FD

[Oliver Hartkopp]

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

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index 3a19333..a2b9bce 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -650,6 +650,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,
@@ -683,6 +687,29 @@ static int can_changelink(struct net_device *dev,
 		}
 	}
 
+	if (data[IFLA_CAN_DATA_BITTIMING]) {
+		struct can_bittiming bt;
+
+		/* Do not allow changing bittiming while running */
+		if (dev->flags & IFF_UP)
+			return -EBUSY;
+		memcpy(&bt, nla_data(data[IFLA_CAN_DATA_BITTIMING]),
+		       sizeof(bt));
+		if ((!bt.bitrate && !bt.tq) || (bt.bitrate && bt.tq))
+			return -EINVAL;
+		err = can_get_bittiming(dev, &bt, priv->data_bittiming_const);
+		if (err)
+			return err;
+		memcpy(&priv->data_bittiming, &bt, sizeof(bt));
+
+		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;
 
@@ -730,6 +757,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_DATA_CAN_BITTIMING */
+		size += nla_total_size(sizeof(struct can_bittiming));
+	if (priv->data_bittiming_const)				/* IFLA_DATA_CAN_BITTIMING_CONST */
+		size += nla_total_size(sizeof(struct can_bittiming_const));
 
 	return size;
 }
@@ -759,8 +790,17 @@ static int can_fill_info(struct sk_buff *skb, const struct net_device *dev)
 
 	    (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.rc3

[PATCH v3 5/5] 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 a2b9bce..397efa4 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -598,6 +598,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
@@ -721,6 +755,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.rc3

Re: [PATCH v3 5/5] can: allow to change the device mtu for CAN FD capable devices

[Marc Kleine-Budde]

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

Hello Oliver,

please add a cover letter (--compose option to git send-email) when
sending a new version of the series, briefly listing your changes (one
line per change).

Please add a patch description to patch 4, as in the kernel it's
considered bad style to have patches without a description.

On 02/15/2014 05:32 PM, Oliver Hartkopp wrote:
> 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).

Please don't add two possibilities to activate canfd on a CAN network
card. There should only be one.

> 
> 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 a2b9bce..397efa4 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -598,6 +598,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);

Who will be the user of this function? The individual network drivers?

> +
> +/*
>   * Common open function when the device gets opened.
>   *
>   * This function should be called in the open function of the device
> @@ -721,6 +755,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
> 

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 v3 5/5] can: allow to change the device mtu for CAN FD capable devices

[Marc Kleine-Budde]

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

On 02/15/2014 05:59 PM, Marc Kleine-Budde wrote:
> Hello Oliver,
> 
> please add a cover letter (--compose option to git send-email) when
> sending a new version of the series, briefly listing your changes (one
> line per change).
               ^^^ ... is enough.

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 v3 5/5] can: allow to change the device mtu for CAN FD capable devices

[Oliver Hartkopp]

On 15.02.2014 17:59, Marc Kleine-Budde wrote:

> please add a cover letter (--compose option to git send-email) when
> sending a new version of the series, briefly listing your changes (one
> line per change).

Will try :-)


> Please add a patch description to patch 4, as in the kernel it's
> considered bad style to have patches without a description.

Will do.


> On 02/15/2014 05:32 PM, Oliver Hartkopp wrote:
>> 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).
> 
> Please don't add two possibilities to activate canfd on a CAN network
> card. There should only be one.

Yes it /should/ be like this.
I thought about it some time but I didn't find a better solution.

The problem is that dev->mtu is the switch for the skb->len whether CAN frames
or CAN FD frames are generated or received.

The MTU can be modified by 'ip' (independently from CAN specific settings).

OTOH the only way the CAN devices provide their capabilities is the
ctrlmode_supported bitfield - and it does not make sense to provide
CAN_CTRLMODE_FD in ctrlmode_supported and do not provide the configuration for it.

If you would skip the CAN_CTRLMODE_FD bit in ctrlmode_supported there's no way
to identify a CAN FD capable device. You could only try to set the MTU to
CANFD_MTU and see, if it fails. Not a good solution either.


>> +EXPORT_SYMBOL_GPL(can_change_mtu);
> 
> Who will be the user of this function? The individual network drivers?
> 

Yes. It manages the MTU and CAN_CTRLMODE_FD consistency and only allows the
CANFD_MTU when the driver supports it.

Any other things to look at - or is the rest ok so far?

Regards,
Oliver

Re: [PATCH v3 5/5] can: allow to change the device mtu for CAN FD capable devices

[Marc Kleine-Budde]

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

On 02/15/2014 06:29 PM, Oliver Hartkopp wrote:
>> On 02/15/2014 05:32 PM, Oliver Hartkopp wrote:
>>> 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).
>>
>> Please don't add two possibilities to activate canfd on a CAN network
>> card. There should only be one.
> 
> Yes it /should/ be like this.
> I thought about it some time but I didn't find a better solution.
> 
> The problem is that dev->mtu is the switch for the skb->len whether CAN frames
> or CAN FD frames are generated or received.
> 
> The MTU can be modified by 'ip' (independently from CAN specific settings).

It _can_ be modified (directly by the userspace) _if_ the change mtu
callback is implemented. Maybe we don't need the callback?

> OTOH the only way the CAN devices provide their capabilities is the
> ctrlmode_supported bitfield - and it does not make sense to provide
> CAN_CTRLMODE_FD in ctrlmode_supported and do not provide the configuration for it.
> 
> If you would skip the CAN_CTRLMODE_FD bit in ctrlmode_supported there's no way
> to identify a CAN FD capable device. You could only try to set the MTU to
> CANFD_MTU and see, if it fails. Not a good solution either.
> 
> 
>>> +EXPORT_SYMBOL_GPL(can_change_mtu);
>>
>> Who will be the user of this function? The individual network drivers?
>>
> 
> Yes. It manages the MTU and CAN_CTRLMODE_FD consistency and only allows the
> CANFD_MTU when the driver supports it.

And it has to be assigned by a FD capable driver to the net_device_ops,
right?

> Any other things to look at - or is the rest ok so far?

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 v3 5/5] can: allow to change the device mtu for CAN FD capable devices

[Oliver Hartkopp]

On 02/18/2014 12:09 PM, Marc Kleine-Budde wrote:
> On 02/15/2014 06:29 PM, Oliver Hartkopp wrote:
>>> On 02/15/2014 05:32 PM, Oliver Hartkopp wrote:
>>>> 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).
>>>
>>> Please don't add two possibilities to activate canfd on a CAN network
>>> card. There should only be one.
>>
>> Yes it /should/ be like this.
>> I thought about it some time but I didn't find a better solution.
>>
>> The problem is that dev->mtu is the switch for the skb->len whether CAN frames
>> or CAN FD frames are generated or received.
>>
>> The MTU can be modified by 'ip' (independently from CAN specific settings).
>
> It _can_ be modified (directly by the userspace) _if_ the change mtu
> callback is implemented. Maybe we don't need the callback?

We need the callback to check if the setting is allowed.
E.g. to set the MTU to 72 is only allowed for CAN FD capable devices.

Or do you think about *only* switching the MTU via

ip link set can0 type can fd on
ip link set can0 type can fd off

??

This could be an idea too.

Currently the vcan can only be switched via mtu setting to identify CAN 
or CAN FD. But this could be a single and only exception to do it like 
this - as the vcan does not use the CAN driver infrastructure.

>
>> OTOH the only way the CAN devices provide their capabilities is the
>> ctrlmode_supported bitfield - and it does not make sense to provide
>> CAN_CTRLMODE_FD in ctrlmode_supported and do not provide the configuration for it.
>>
>> If you would skip the CAN_CTRLMODE_FD bit in ctrlmode_supported there's no way
>> to identify a CAN FD capable device. You could only try to set the MTU to
>> CANFD_MTU and see, if it fails. Not a good solution either.
>>
>>
>>>> +EXPORT_SYMBOL_GPL(can_change_mtu);
>>>
>>> Who will be the user of this function? The individual network drivers?
>>>
>>
>> Yes. It manages the MTU and CAN_CTRLMODE_FD consistency and only allows the
>> CANFD_MTU when the driver supports it.
>
> And it has to be assigned by a FD capable driver to the net_device_ops,
> right?
>

Yes.

Regards,
Oliver

Re: [PATCH v3 5/5] can: allow to change the device mtu for CAN FD capable devices

[Oliver Hartkopp]

On 18.02.2014 14:08, Oliver Hartkopp wrote:
>
>
> On 02/18/2014 12:09 PM, Marc Kleine-Budde wrote:
>> On 02/15/2014 06:29 PM, Oliver Hartkopp wrote:
>>>> On 02/15/2014 05:32 PM, Oliver Hartkopp wrote:
>>>>> 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).
>>>>
>>>> Please don't add two possibilities to activate canfd on a CAN network
>>>> card. There should only be one.
>>>
>>> Yes it /should/ be like this.
>>> I thought about it some time but I didn't find a better solution.
>>>
>>> The problem is that dev->mtu is the switch for the skb->len whether CAN frames
>>> or CAN FD frames are generated or received.
>>>
>>> The MTU can be modified by 'ip' (independently from CAN specific settings).
>>
>> It _can_ be modified (directly by the userspace) _if_ the change mtu
>> callback is implemented. Maybe we don't need the callback?

See:
http://lxr.free-electrons.com/source/net/core/dev.c#L5324

When we do not define our own callback function the mtu is set to any value 
the user specifies. This is definitely not a good idea %-)

The question is if we define our own can_change_mtu() callback to control the 
mtu access - but only allow the FD switching via ip tool.

Or if we allow to modify the FD switch via 'ip' and mtu setting which was the 
original patch idea?!?

>
> We need the callback to check if the setting is allowed.
> E.g. to set the MTU to 72 is only allowed for CAN FD capable devices.
>
> Or do you think about *only* switching the MTU via
>
> ip link set can0 type can fd on
> ip link set can0 type can fd off
>
> ??
>
> This could be an idea too.
>
> Currently the vcan can only be switched via mtu setting to identify CAN or CAN
> FD. But this could be a single and only exception to do it like this - as the
> vcan does not use the CAN driver infrastructure.
>
>>
>>> OTOH the only way the CAN devices provide their capabilities is the
>>> ctrlmode_supported bitfield - and it does not make sense to provide
>>> CAN_CTRLMODE_FD in ctrlmode_supported and do not provide the configuration
>>> for it.
>>>
>>> If you would skip the CAN_CTRLMODE_FD bit in ctrlmode_supported there's no way
>>> to identify a CAN FD capable device. You could only try to set the MTU to
>>> CANFD_MTU and see, if it fails. Not a good solution either.
>>>
>>>
>>>>> +EXPORT_SYMBOL_GPL(can_change_mtu);
>>>>
>>>> Who will be the user of this function? The individual network drivers?
>>>>
>>>
>>> Yes. It manages the MTU and CAN_CTRLMODE_FD consistency and only allows the
>>> CANFD_MTU when the driver supports it.
>>
>> And it has to be assigned by a FD capable driver to the net_device_ops,
>> right?
>>
>
> Yes.
>
> Regards,
> Oliver
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/5] can: introduce the data bitrate configuration for CAN FD

[Stephane Grosjean]

Hi Oliver,

Only some nitpicking from my side, for the moment ...

Le 15/02/2014 17:32, Oliver Hartkopp a écrit :
>   
> @@ -730,6 +757,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_DATA_CAN_BITTIMING */
> +		size += nla_total_size(sizeof(struct can_bittiming));
> +	if (priv->data_bittiming_const)				/* IFLA_DATA_CAN_BITTIMING_CONST */
> +		size += nla_total_size(sizeof(struct can_bittiming_const));
>   
>   	return size;
>   }

IFLA_DATA_CAN_xxx should be changed into IFLA_CAN_DATA_xxx, in both 
above eol comments.

Regards,

Stéphane

--
PEAK-System Technik GmbH, Otto-Roehm-Strasse 69, D-64293 Darmstadt 
Geschaeftsleitung: A.Gach/U.Wilhelm,St.Nr.:007/241/13586 FA Darmstadt 
HRB-9183 Darmstadt, Ust.IdNr.:DE 202220078, WEE-Reg.-Nr.: DE39305391 
Tel.+49 (0)6151-817320 / Fax:+49 (0)6151-817329, info@peak-system.com