[RFC v3] CAN FD support

[RFC v3] CAN FD support

[Oliver Hartkopp]

Hi all,

changes since v2:

https://gitorious.org/linux-can/hartkopps-linux-can-next/commit/ef9c4914da29702f25f0f2342ab1f6d714aa118b/diffs/9d51e1e6e40be9a36210a9ac7a9f8d6067ba94a5

    can: bump version date
    candev: beautify dlc to length conversion tables
    af_can: make dev_add_pack() and dev_remove_pack() calls symetric
    can: add unlikely() to sanity checks where appropriate
    vcan: change error path in vcan_change_mtu()

Discussion about v2: http://marc.info/?l=linux-can&m=133676039322297&w=2

I generated this diff by 'git diff fb7944b' on the repository

https://gitorious.org/~hartkopp/linux-can/hartkopps-linux-can-next

Regards,
Oliver

 drivers/net/can/dev.c    |   35 ++++++++++++++
 drivers/net/can/vcan.c   |   27 ++++++++---
 include/linux/can.h      |   51 ++++++++++++++++++++-
 include/linux/can/core.h |    4 -
 include/linux/can/dev.h  |   33 ++++++++++----
 include/linux/can/raw.h  |    3 -
 include/linux/if_ether.h |    3 -
 net/can/af_can.c         |  110 +++++++++++++++++++++++++++++++++++------------
 net/can/raw.c            |   41 ++++++++++++++++-
 9 files changed, 257 insertions(+), 50 deletions(-)

---

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index f03d7a4..e1769b5 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -33,6 +33,39 @@ MODULE_DESCRIPTION(MOD_DESC);
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Wolfgang Grandegger <wg@grandegger.com>");
 
+/* CAN DLC to real data length conversion helpers */
+
+static const u8 dlc2len[] = {0, 1, 2, 3, 4, 5, 6, 7,
+			     8, 12, 16, 20, 24, 32, 48, 64};
+
+/* get data length from can_dlc with sanitized can_dlc */
+u8 can_dlc2len(u8 can_dlc)
+{
+	return dlc2len[can_dlc & 0x0F];
+}
+EXPORT_SYMBOL_GPL(can_dlc2len);
+
+static const u8 len2dlc[] = {0, 1, 2, 3, 4, 5, 6, 7, 8,		/* 0 - 8 */
+			     9, 9, 9, 9,			/* 9 - 12 */
+			     10, 10, 10, 10,			/* 13 - 16 */
+			     11, 11, 11, 11,			/* 17 - 20 */
+			     12, 12, 12, 12,			/* 21 - 24 */
+			     13, 13, 13, 13, 13, 13, 13, 13,	/* 25 - 32 */
+			     14, 14, 14, 14, 14, 14, 14, 14,	/* 33 - 40 */
+			     14, 14, 14, 14, 14, 14, 14, 14,	/* 41 - 48 */
+			     15, 15, 15, 15, 15, 15, 15, 15,	/* 49 - 56 */
+			     15, 15, 15, 15, 15, 15, 15, 15};	/* 57 - 64 */
+
+/* map the sanitized data length to an appropriate data length code */ 
+u8 can_len2dlc(u8 len)
+{
+	if (unlikely(len > 64))
+		return 0xF;
+
+	return len2dlc[len];
+}
+EXPORT_SYMBOL_GPL(can_len2dlc);
+
 #ifdef CONFIG_CAN_CALC_BITTIMING
 #define CAN_CALC_MAX_ERROR 50 /* in one-tenth of a percent */
 
@@ -454,7 +487,7 @@ EXPORT_SYMBOL_GPL(can_bus_off);
 static void can_setup(struct net_device *dev)
 {
 	dev->type = ARPHRD_CAN;
-	dev->mtu = sizeof(struct can_frame);
+	dev->mtu = CAN_MTU;
 	dev->hard_header_len = 0;
 	dev->addr_len = 0;
 	dev->tx_queue_len = 10;
diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c
index ea2d942..4f93c0b 100644
--- a/drivers/net/can/vcan.c
+++ b/drivers/net/can/vcan.c
@@ -70,13 +70,12 @@ MODULE_PARM_DESC(echo, "Echo sent frames (for testing). Default: 0 (Off)");
 
 static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
 {
-	struct can_frame *cf = (struct can_frame *)skb->data;
+	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
 	struct net_device_stats *stats = &dev->stats;
 
 	stats->rx_packets++;
-	stats->rx_bytes += cf->can_dlc;
+	stats->rx_bytes += cfd->len;
 
-	skb->protocol  = htons(ETH_P_CAN);
 	skb->pkt_type  = PACKET_BROADCAST;
 	skb->dev       = dev;
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
@@ -86,7 +85,7 @@ static void vcan_rx(struct sk_buff *skb, struct net_device *dev)
 
 static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
 {
-	struct can_frame *cf = (struct can_frame *)skb->data;
+	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
 	struct net_device_stats *stats = &dev->stats;
 	int loop;
 
@@ -94,7 +93,7 @@ static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
 		return NETDEV_TX_OK;
 
 	stats->tx_packets++;
-	stats->tx_bytes += cf->can_dlc;
+	stats->tx_bytes += cfd->len;
 
 	/* set flag whether this packet has to be looped back */
 	loop = skb->pkt_type == PACKET_LOOPBACK;
@@ -108,7 +107,7 @@ static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
 			 * CAN core already did the echo for us
 			 */
 			stats->rx_packets++;
-			stats->rx_bytes += cf->can_dlc;
+			stats->rx_bytes += cfd->len;
 		}
 		kfree_skb(skb);
 		return NETDEV_TX_OK;
@@ -133,14 +132,28 @@ static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)
 	return NETDEV_TX_OK;
 }
 
+static int vcan_change_mtu(struct net_device *dev, int new_mtu)
+{
+	/* Do not allow changing the MTU while running */
+	if (dev->flags & IFF_UP)
+		return -EBUSY;
+
+	if (new_mtu != CAN_MTU && new_mtu != CANFD_MTU)
+		return -EINVAL;
+
+	dev->mtu = new_mtu;
+	return 0;
+}
+
 static const struct net_device_ops vcan_netdev_ops = {
 	.ndo_start_xmit = vcan_tx,
+	.ndo_change_mtu = vcan_change_mtu,
 };
 
 static void vcan_setup(struct net_device *dev)
 {
 	dev->type		= ARPHRD_CAN;
-	dev->mtu		= sizeof(struct can_frame);
+	dev->mtu		= CAN_MTU;
 	dev->hard_header_len	= 0;
 	dev->addr_len		= 0;
 	dev->tx_queue_len	= 0;
diff --git a/include/linux/can.h b/include/linux/can.h
index 9a19bcb..c8504ac 100644
--- a/include/linux/can.h
+++ b/include/linux/can.h
@@ -46,18 +46,65 @@ typedef __u32 canid_t;
  */
 typedef __u32 can_err_mask_t;
 
+#define CAN_MAX_DLC 8
+#define CAN_MAX_DLEN 8
+
+#define CANFD_MAX_DLC 15
+#define CANFD_MAX_DLEN 64
+
 /**
  * struct can_frame - basic CAN frame structure
  * @can_id:  the CAN ID of the frame and CAN_*_FLAG flags, see above.
- * @can_dlc: the data length field of the CAN frame
+ * @can_dlc: frame payload length in byte (0 .. 8) aka data length code.
+ *           The DLC field from ISO 11898-1 Chapter 8.4.2.3 has a 1:1
+ *           mapping of the 'data length code' to the payload length.
  * @data:    the CAN frame payload.
  */
 struct can_frame {
 	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
-	__u8    can_dlc; /* data length code: 0 .. 8 */
+	__u8    can_dlc; /* frame payload length in byte (0 .. 8) */
 	__u8    data[8] __attribute__((aligned(8)));
 };
 
+/*
+ * defined bits for canfd_frame.flags
+ *
+ * As the default for CAN FD should be to support the high data rate in the
+ * payload section of the frame (HDR) and to support up to 64 byte in the
+ * data section (EDL) the bits are only set in the non-default case.
+ * Btw. as long as there's no real implementation for CAN FD network driver
+ * these bits are only preliminary.
+ *
+ * RX: NOHDR/NOEDL - info about received CAN FD frame
+ *     ESI         - bit from originating CAN controller
+ * TX: NOHDR/NOEDL - control per-frame settings if supported by CAN controller
+ *     ESI         - bit is set by local CAN controller
+ */
+#define CANFD_NOHDR 0x01 /* frame without high data rate */
+#define CANFD_NOEDL 0x02 /* frame without extended data length */
+#define CANFD_ESI   0x04 /* error state indicator */
+
+/**
+ * struct canfd_frame - CAN flexible data rate frame structure
+ * @can_id: the CAN ID of the frame and CAN_*_FLAG flags, see above.
+ * @len:    frame payload length in byte (0 .. 64)
+ * @flags:  additional flags for CAN FD
+ * @__res0: reserved / padding
+ * @__res1: reserved / padding
+ * @data:   the CAN FD frame payload (up to 64 byte).
+ */
+struct canfd_frame {
+	canid_t can_id;  /* 32 bit CAN_ID + EFF/RTR/ERR flags */
+	__u8    len;     /* frame payload length in byte (0 .. 64) */
+	__u8    flags;   /* additional flags for CAN FD */
+	__u8    __res0;  /* reserved / padding */
+	__u8    __res1;  /* reserved / padding */
+	__u8    data[64] __attribute__((aligned(8)));
+};
+
+#define CAN_MTU		(sizeof(struct can_frame))
+#define CANFD_MTU	(sizeof(struct canfd_frame))
+
 /* particular protocols of the protocol family PF_CAN */
 #define CAN_RAW		1 /* RAW sockets */
 #define CAN_BCM		2 /* Broadcast Manager */
diff --git a/include/linux/can/core.h b/include/linux/can/core.h
index 0ccc1cd..8e41c96 100644
--- a/include/linux/can/core.h
+++ b/include/linux/can/core.h
@@ -17,10 +17,10 @@
 #include <linux/skbuff.h>
 #include <linux/netdevice.h>
 
-#define CAN_VERSION "20090105"
+#define CAN_VERSION "20120511"
 
 /* increment this number each time you change some user-space interface */
-#define CAN_ABI_VERSION "8"
+#define CAN_ABI_VERSION "9"
 
 #define CAN_VERSION_STRING "rev " CAN_VERSION " abi " CAN_ABI_VERSION
 
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 5d2efe7..a243279 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -61,23 +61,40 @@ struct can_priv {
  * To be used in the CAN netdriver receive path to ensure conformance with
  * ISO 11898-1 Chapter 8.4.2.3 (DLC field)
  */
-#define get_can_dlc(i)	(min_t(__u8, (i), 8))
+#define get_can_dlc(i)		(min_t(__u8, (i), CAN_MAX_DLC))
+#define get_canfd_dlc(i)	(min_t(__u8, (i), CANFD_MAX_DLC))
 
 /* Drop a given socketbuffer if it does not contain a valid CAN frame. */
 static inline int can_dropped_invalid_skb(struct net_device *dev,
 					  struct sk_buff *skb)
 {
-	const struct can_frame *cf = (struct can_frame *)skb->data;
-
-	if (unlikely(skb->len != sizeof(*cf) || cf->can_dlc > 8)) {
-		kfree_skb(skb);
-		dev->stats.tx_dropped++;
-		return 1;
-	}
+	const struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+
+	if (skb->protocol == htons(ETH_P_CAN)) {
+		if (unlikely(skb->len != sizeof(struct can_frame) ||
+			     cfd->len > CAN_MAX_DLEN))
+			goto inval_skb;
+	} else if (skb->protocol == htons(ETH_P_CANFD)) {
+		if (unlikely(skb->len != sizeof(struct canfd_frame) ||
+			    cfd->len > CANFD_MAX_DLEN))
+			goto inval_skb;
+	} else
+		goto inval_skb;
 
 	return 0;
+
+inval_skb:
+	kfree_skb(skb);
+	dev->stats.tx_dropped++;
+	return 1;
 }
 
+/* get data length from can_dlc with sanitized can_dlc */
+u8 can_dlc2len(u8 can_dlc);
+
+/* map the sanitized data length to an appropriate data length code */ 
+u8 can_len2dlc(u8 len);
+
 struct net_device *alloc_candev(int sizeof_priv, unsigned int echo_skb_max);
 void free_candev(struct net_device *dev);
 
diff --git a/include/linux/can/raw.h b/include/linux/can/raw.h
index 781f3a3..5448c0f 100644
--- a/include/linux/can/raw.h
+++ b/include/linux/can/raw.h
@@ -23,7 +23,8 @@ enum {
 	CAN_RAW_FILTER = 1,	/* set 0 .. n can_filter(s)          */
 	CAN_RAW_ERR_FILTER,	/* set filter for error frames       */
 	CAN_RAW_LOOPBACK,	/* local loopback (default:on)       */
-	CAN_RAW_RECV_OWN_MSGS	/* receive my own msgs (default:off) */
+	CAN_RAW_RECV_OWN_MSGS,	/* receive my own msgs (default:off) */
+	CAN_RAW_FD_FRAMES,	/* use struct canfd_frame (default:off) */
 };
 
 #endif
diff --git a/include/linux/if_ether.h b/include/linux/if_ether.h
index 56d907a..260138b 100644
--- a/include/linux/if_ether.h
+++ b/include/linux/if_ether.h
@@ -105,7 +105,8 @@
 #define ETH_P_WAN_PPP   0x0007          /* Dummy type for WAN PPP frames*/
 #define ETH_P_PPP_MP    0x0008          /* Dummy type for PPP MP frames */
 #define ETH_P_LOCALTALK 0x0009		/* Localtalk pseudo type 	*/
-#define ETH_P_CAN	0x000C		/* Controller Area Network      */
+#define ETH_P_CAN	0x000C		/* Controller Area Network (CAN)*/
+#define ETH_P_CANFD	0x000D		/* CAN FD 64 byte payload frames*/
 #define ETH_P_PPPTALK	0x0010		/* Dummy type for Atalk over PPP*/
 #define ETH_P_TR_802_2	0x0011		/* 802.2 frames 		*/
 #define ETH_P_MOBITEX	0x0015		/* Mobitex (kaz@cafe.net)	*/
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 0ce2ad0..2059ed0 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -41,6 +41,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/stddef.h>
 #include <linux/init.h>
 #include <linux/kmod.h>
 #include <linux/slab.h>
@@ -220,30 +221,42 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
  *  -ENOBUFS on full driver queue (see net_xmit_errno())
  *  -ENOMEM when local loopback failed at calling skb_clone()
  *  -EPERM when trying to send on a non-CAN interface
+ *  -EMSGSIZE CAN frame size is bigger than CAN interface MTU
  *  -EINVAL when the skb->data does not contain a valid CAN frame
  */
 int can_send(struct sk_buff *skb, int loop)
 {
 	struct sk_buff *newskb = NULL;
-	struct can_frame *cf = (struct can_frame *)skb->data;
-	int err;
+	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+	int err = -EINVAL;
+
+	if (skb->len == CAN_MTU) {
+		skb->protocol = htons(ETH_P_CAN);
+		if (unlikely(cfd->len > CAN_MAX_DLEN))
+			goto inval_skb;
+	} else if (skb->len == CANFD_MTU) {
+		skb->protocol = htons(ETH_P_CANFD);
+		if (unlikely(cfd->len > CANFD_MAX_DLEN))
+			goto inval_skb;
+	} else
+		goto inval_skb;
 
-	if (skb->len != sizeof(struct can_frame) || cf->can_dlc > 8) {
-		kfree_skb(skb);
-		return -EINVAL;
+	/* make sure the CAN frame can pass the selected CAN netdevice */
+	if (unlikely(skb->len > skb->dev->mtu)) {
+		err = -EMSGSIZE;
+		goto inval_skb;
 	}
 
-	if (skb->dev->type != ARPHRD_CAN) {
-		kfree_skb(skb);
-		return -EPERM;
+	if (unlikely(skb->dev->type != ARPHRD_CAN)) {
+		err = -EPERM;
+		goto inval_skb;
 	}
 
-	if (!(skb->dev->flags & IFF_UP)) {
-		kfree_skb(skb);
-		return -ENETDOWN;
+	if (unlikely(!(skb->dev->flags & IFF_UP))) {
+		err = -ENETDOWN;
+		goto inval_skb;
 	}
 
-	skb->protocol = htons(ETH_P_CAN);
 	skb_reset_network_header(skb);
 	skb_reset_transport_header(skb);
 
@@ -300,6 +313,10 @@ int can_send(struct sk_buff *skb, int loop)
 	can_stats.tx_frames_delta++;
 
 	return 0;
+
+inval_skb:
+	kfree_skb(skb);
+	return err;
 }
 EXPORT_SYMBOL(can_send);
 
@@ -632,24 +649,11 @@ static int can_rcv_filter(struct dev_rcv_lists *d, struct sk_buff *skb)
 	return matches;
 }
 
-static int can_rcv(struct sk_buff *skb, struct net_device *dev,
-		   struct packet_type *pt, struct net_device *orig_dev)
+static void can_receive(struct sk_buff *skb, struct net_device *dev)
 {
 	struct dev_rcv_lists *d;
-	struct can_frame *cf = (struct can_frame *)skb->data;
 	int matches;
 
-	if (!net_eq(dev_net(dev), &init_net))
-		goto drop;
-
-	if (WARN_ONCE(dev->type != ARPHRD_CAN ||
-		      skb->len != sizeof(struct can_frame) ||
-		      cf->can_dlc > 8,
-		      "PF_CAN: dropped non conform skbuf: "
-		      "dev type %d, len %d, can_dlc %d\n",
-		      dev->type, skb->len, cf->can_dlc))
-		goto drop;
-
 	/* update statistics */
 	can_stats.rx_frames++;
 	can_stats.rx_frames_delta++;
@@ -673,7 +677,49 @@ static int can_rcv(struct sk_buff *skb, struct net_device *dev,
 		can_stats.matches++;
 		can_stats.matches_delta++;
 	}
+}
+
+static int can_rcv(struct sk_buff *skb, struct net_device *dev,
+		   struct packet_type *pt, struct net_device *orig_dev)
+{
+	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+
+	if (unlikely(!net_eq(dev_net(dev), &init_net)))
+		goto drop;
 
+	if (WARN_ONCE(dev->type != ARPHRD_CAN ||
+		      skb->len != CAN_MTU ||
+		      cfd->len > CAN_MAX_DLEN,
+		      "PF_CAN: dropped non conform CAN skbuf: "
+		      "dev type %d, len %d, datalen %d\n",
+		      dev->type, skb->len, cfd->len))
+		goto drop;
+
+	can_receive(skb, dev);
+	return NET_RX_SUCCESS;
+
+drop:
+	kfree_skb(skb);
+	return NET_RX_DROP;
+}
+
+static int canfd_rcv(struct sk_buff *skb, struct net_device *dev,
+		   struct packet_type *pt, struct net_device *orig_dev)
+{
+	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+
+	if (unlikely(!net_eq(dev_net(dev), &init_net)))
+		goto drop;
+
+	if (WARN_ONCE(dev->type != ARPHRD_CAN ||
+		      skb->len != CANFD_MTU ||
+		      cfd->len > CANFD_MAX_DLEN,
+		      "PF_CAN: dropped non conform CAN FD skbuf: "
+		      "dev type %d, len %d, datalen %d\n",
+		      dev->type, skb->len, cfd->len))
+		goto drop;
+
+	can_receive(skb, dev);
 	return NET_RX_SUCCESS;
 
 drop:
@@ -811,6 +857,12 @@ static struct packet_type can_packet __read_mostly = {
 	.func = can_rcv,
 };
 
+static struct packet_type canfd_packet __read_mostly = {
+	.type = cpu_to_be16(ETH_P_CANFD),
+	.dev  = NULL,
+	.func = canfd_rcv,
+};
+
 static const struct net_proto_family can_family_ops = {
 	.family = PF_CAN,
 	.create = can_create,
@@ -824,6 +876,10 @@ static struct notifier_block can_netdev_notifier __read_mostly = {
 
 static __init int can_init(void)
 {
+	/* check for correct padding that can_dlc owns always the same position */
+	BUILD_BUG_ON(offsetof(struct can_frame, can_dlc) !=
+		     offsetof(struct canfd_frame, len));
+
 	printk(banner);
 
 	memset(&can_rx_alldev_list, 0, sizeof(can_rx_alldev_list));
@@ -846,6 +902,7 @@ static __init int can_init(void)
 	sock_register(&can_family_ops);
 	register_netdevice_notifier(&can_netdev_notifier);
 	dev_add_pack(&can_packet);
+	dev_add_pack(&canfd_packet);
 
 	return 0;
 }
@@ -860,6 +917,7 @@ static __exit void can_exit(void)
 	can_remove_proc();
 
 	/* protocol unregister */
+	dev_remove_pack(&canfd_packet);
 	dev_remove_pack(&can_packet);
 	unregister_netdevice_notifier(&can_netdev_notifier);
 	sock_unregister(PF_CAN);
diff --git a/net/can/raw.c b/net/can/raw.c
index cde1b4a..2473e6e 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -82,6 +82,7 @@ struct raw_sock {
 	struct notifier_block notifier;
 	int loopback;
 	int recv_own_msgs;
+	int fd_frames;
 	int count;                 /* number of active filters */
 	struct can_filter dfilter; /* default/single filter */
 	struct can_filter *filter; /* pointer to filter(s) */
@@ -119,6 +120,15 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
 	if (!ro->recv_own_msgs && oskb->sk == sk)
 		return;
 
+	/* only pass correct frames to the socket */
+	if (ro->fd_frames) {
+		if (unlikely(oskb->len != CANFD_MTU))
+			return;
+	} else {
+		if (unlikely(oskb->len != CAN_MTU))
+			return;
+	}
+
 	/* clone the given skb to be able to enqueue it into the rcv queue */
 	skb = skb_clone(oskb, GFP_ATOMIC);
 	if (!skb)
@@ -291,6 +301,7 @@ static int raw_init(struct sock *sk)
 	/* set default loopback behaviour */
 	ro->loopback         = 1;
 	ro->recv_own_msgs    = 0;
+	ro->fd_frames        = 0;
 
 	/* set notifier */
 	ro->notifier.notifier_call = raw_notifier;
@@ -569,6 +580,15 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
 
 		break;
 
+	case CAN_RAW_FD_FRAMES:
+		if (optlen != sizeof(ro->fd_frames))
+			return -EINVAL;
+
+		if (copy_from_user(&ro->fd_frames, optval, optlen))
+			return -EFAULT;
+
+		break;
+
 	default:
 		return -ENOPROTOOPT;
 	}
@@ -627,6 +647,12 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
 		val = &ro->recv_own_msgs;
 		break;
 
+	case CAN_RAW_FD_FRAMES:
+		if (len > sizeof(int))
+			len = sizeof(int);
+		val = &ro->fd_frames;
+		break;
+
 	default:
 		return -ENOPROTOOPT;
 	}
@@ -662,13 +688,24 @@ static int raw_sendmsg(struct kiocb *iocb, struct socket *sock,
 	} else
 		ifindex = ro->ifindex;
 
-	if (size != sizeof(struct can_frame))
-		return -EINVAL;
+	if (ro->fd_frames) {
+		if (unlikely(size != CANFD_MTU))
+			return -EINVAL;
+	} else {
+		if (unlikely(size != CAN_MTU))
+			return -EINVAL;
+	}
 
 	dev = dev_get_by_index(&init_net, ifindex);
 	if (!dev)
 		return -ENXIO;
 
+	/* make sure the created CAN frame can pass the CAN netdevice */
+	if (unlikely(size > dev->mtu)) {
+		err = -EMSGSIZE;
+		goto put_dev;
+	}
+
 	skb = sock_alloc_send_skb(sk, size, msg->msg_flags & MSG_DONTWAIT,
 				  &err);
 	if (!skb)

Re: [RFC v3] CAN FD support

[Kurt Van Dijck]

Oliver,

You're running fast now :-)

I like the inversion of HDR & EDL bits. It make CAN & CANFD
even more compatible.

I still don't see the contribution of ETH_P_CANFD.
I think it introduces a lot of (almost) idenctical code,
especially compared to its benefits (cfr. 'helga').

Another issue I'm still struggling with: can_raw is differentiating
between CAN & CANFD with CAN_RAW_FD_FRAMES sockopt.

IMO, at most 1 property/method shall exist to differentiate CAN from CANFD.
(Preferrably, no method should have been necessary).

I see that 2 struct are in place now, with sizes CAN_MTU & CANFD_MTU.
This alone is IMO enough to differentiate.
Next to this property, an _extra_ sockopt is introduced.
This sockopt actually implements a filter, at can_raw level, *and*
is a second way of telling you want to use CANFD.

I understood that mixing CAN & CANFD on 1 bus is possible on CANFD chip level,
and we do want to support those features by using 2 structs
that in fact determine the CAN type. If an application want to mix
CAN & CANFD, it is currently not possible without opening another
socket, thereby loosing the proper sequence of events.

IMHO, the CAN_RAW_FD_FRAMES is not justifyable towards userspace
applications.

I put my view in the code down below, besides the elimination of the 'fd_frames'
member in struct raw_sock, and the corresponding CAN_RAW_FD_FRAMES sockopt.

IMO, the old behaviour (that is written in stone) is still preserved for
99.999%, _and_ no difficult sockopts are needed.
Difficult means in this case: it became not straightforward to predict
which frames got received.

Kurt

On Fri, May 11, 2012 at 08:27:16PM +0200, Oliver Hartkopp wrote:
> 
> I generated this diff by 'git diff fb7944b' on the repository
> 
> https://gitorious.org/~hartkopp/linux-can/hartkopps-linux-can-next
> 
> Regards,
> Oliver
> 
>  drivers/net/can/dev.c    |   35 ++++++++++++++
>  drivers/net/can/vcan.c   |   27 ++++++++---
>  include/linux/can.h      |   51 ++++++++++++++++++++-
>  include/linux/can/core.h |    4 -
>  include/linux/can/dev.h  |   33 ++++++++++----
>  include/linux/can/raw.h  |    3 -
>  include/linux/if_ether.h |    3 -
>  net/can/af_can.c         |  110 +++++++++++++++++++++++++++++++++++------------
>  net/can/raw.c            |   41 ++++++++++++++++-
>  9 files changed, 257 insertions(+), 50 deletions(-)
> 
> ---
[...]
> diff --git a/include/linux/can/raw.h b/include/linux/can/raw.h
> index 781f3a3..5448c0f 100644
> --- a/include/linux/can/raw.h
> +++ b/include/linux/can/raw.h
> @@ -23,7 +23,8 @@ enum {
>  	CAN_RAW_FILTER = 1,	/* set 0 .. n can_filter(s)          */
>  	CAN_RAW_ERR_FILTER,	/* set filter for error frames       */
>  	CAN_RAW_LOOPBACK,	/* local loopback (default:on)       */
> -	CAN_RAW_RECV_OWN_MSGS	/* receive my own msgs (default:off) */
> +	CAN_RAW_RECV_OWN_MSGS,	/* receive my own msgs (default:off) */
> +	CAN_RAW_FD_FRAMES,	/* use struct canfd_frame (default:off) */
>  };
>  
>  #endif
> diff --git a/net/can/raw.c b/net/can/raw.c
> index cde1b4a..2473e6e 100644
> --- a/net/can/raw.c
> +++ b/net/can/raw.c
> @@ -82,6 +82,7 @@ struct raw_sock {
>  	struct notifier_block notifier;
>  	int loopback;
>  	int recv_own_msgs;
> +	int fd_frames;
>  	int count;                 /* number of active filters */
>  	struct can_filter dfilter; /* default/single filter */
>  	struct can_filter *filter; /* pointer to filter(s) */
> @@ -119,6 +120,15 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
>  	if (!ro->recv_own_msgs && oskb->sk == sk)
>  		return;
>  
> +	/* only pass correct frames to the socket */
I'm not sure to introduce such check here, but it could be as simple as:

	if (unlikely((oskb->len != CAN_MTU)||(oskb->len != CANFD_MTU)))
		return;

> +	if (ro->fd_frames) {
> +		if (unlikely(oskb->len != CANFD_MTU))
> +			return;
> +	} else {
> +		if (unlikely(oskb->len != CAN_MTU))
> +			return;
> +	}
> +
>  	/* clone the given skb to be able to enqueue it into the rcv queue */
>  	skb = skb_clone(oskb, GFP_ATOMIC);
>  	if (!skb)
> @@ -291,6 +301,7 @@ static int raw_init(struct sock *sk)
>  	/* set default loopback behaviour */
>  	ro->loopback         = 1;
>  	ro->recv_own_msgs    = 0;
> +	ro->fd_frames        = 0;
>  
>  	/* set notifier */
>  	ro->notifier.notifier_call = raw_notifier;
> @@ -569,6 +580,15 @@ static int raw_setsockopt(struct socket *sock, int level, int optname,
>  
>  		break;
>  
> +	case CAN_RAW_FD_FRAMES:
> +		if (optlen != sizeof(ro->fd_frames))
> +			return -EINVAL;
> +
> +		if (copy_from_user(&ro->fd_frames, optval, optlen))
> +			return -EFAULT;
> +
> +		break;
> +
>  	default:
>  		return -ENOPROTOOPT;
>  	}
> @@ -627,6 +647,12 @@ static int raw_getsockopt(struct socket *sock, int level, int optname,
>  		val = &ro->recv_own_msgs;
>  		break;
>  
> +	case CAN_RAW_FD_FRAMES:
> +		if (len > sizeof(int))
> +			len = sizeof(int);
> +		val = &ro->fd_frames;
> +		break;
> +
>  	default:
>  		return -ENOPROTOOPT;
>  	}
> @@ -662,13 +688,24 @@ static int raw_sendmsg(struct kiocb *iocb, struct socket *sock,
>  	} else
>  		ifindex = ro->ifindex;
>  
	if (unlikely((size != CAN_MTU)&&(size != CANFD_MTU)))
		return -EINVAL;
> -	if (size != sizeof(struct can_frame))
> -		return -EINVAL;
> +	if (ro->fd_frames) {
> +		if (unlikely(size != CANFD_MTU))
> +			return -EINVAL;
> +	} else {
> +		if (unlikely(size != CAN_MTU))
> +			return -EINVAL;
> +	}
>  
>  	dev = dev_get_by_index(&init_net, ifindex);
>  	if (!dev)
>  		return -ENXIO;
>  
> +	/* make sure the created CAN frame can pass the CAN netdevice */
> +	if (unlikely(size > dev->mtu)) {
> +		err = -EMSGSIZE;
> +		goto put_dev;
> +	}
> +
>  	skb = sock_alloc_send_skb(sk, size, msg->msg_flags & MSG_DONTWAIT,
>  				  &err);
>  	if (!skb)
> 
> 
> --
> 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: [RFC v3] CAN FD support

[Oliver Hartkopp]

Hello Kurt,

thanks for your feedback.

Indeed i'm not totally happy with the current hard switch too! Let's see if we
can develop something less intrusive but still clean in design.

The question is how to cope with this scenario:

A legacy application wants to communicate via can0 not knowing anything about
CAN FD. This legacy app can only read and write struct can_frames and does not
know about different data rates in the data field.

1. For this scenario we need to define (globally!?) if legacy apps generated
(short) CAN frames use the slow or high data rate in the data field when
transmitted via that specific CAN FD interface.

2. in any case legacy applications must not get CAN frames with a DLC > 8

3. legacy applications assume struct can_frames on read and write. There
should no MSG_TRUNC been set in msg flags when receiving a can_frame.

OTOH we will have new applications that are aware of CAN FD. These new
applications may also be able to deal with two different sizes of structs for
read and write operations (can_frame and canfd_frame) - if we specify it so.

Therefore we would need at least some flag if the application is CAN FD aware
or not. Btw. i'll think about a better simultaneous access of legacy and new
apps to the CAN interfaces.

Some more comments inline:

On 14.05.2012 11:36, Kurt Van Dijck wrote:

> I like the inversion of HDR & EDL bits. It make CAN & CANFD
> even more compatible.
> 
> I still don't see the contribution of ETH_P_CANFD.


I'll think about it again.

> Another issue I'm still struggling with: can_raw is differentiating
> between CAN & CANFD with CAN_RAW_FD_FRAMES sockopt.


As written above it is needed to indicate a CAN FD aware app.

I just wonder if CAN FD apps should deal with both MTUs at a time.

> IMO, at most 1 property/method shall exist to differentiate CAN from CANFD.
> (Preferrably, no method should have been necessary).


The protocol CAN_RAW stands for struct can_frame. This is fixed.

If you want to exchange canfd_frames you can add a new sockopt or you
introduce a new protocol (e.g. CAN_RAWFD). The latter has no additional
property/method - besides the different protocol number ... %-)

Would you prefer that solution?

> I see that 2 struct are in place now, with sizes CAN_MTU & CANFD_MTU.
> This alone is IMO enough to differentiate.


See above. Yes, but you need to protect legacy apps of getting FD frames.

> Next to this property, an _extra_ sockopt is introduced.
> This sockopt actually implements a filter, at can_raw level, *and*
> is a second way of telling you want to use CANFD.


The exclusive-OR switch can probably be alleviated.

> 
> I understood that mixing CAN & CANFD on 1 bus is possible on CANFD chip level,
> and we do want to support those features by using 2 structs
> that in fact determine the CAN type. If an application want to mix
> CAN & CANFD, it is currently not possible without opening another
> socket, thereby loosing the proper sequence of events.


Beware of providing a needless flexibility here.
Once the CAN driver is switched to the CAN FD mode you can send and receive
all types of frames. You can send normal CAN frames inside struct canfd_frame
too! So when you open a socket - and you are a CAN FD aware app - you switch
to CAN_RAW_FD_FRAMES and you are free to send/recv whatever you like.

NB: You can only switch the CAN interface MTU when the interface is down.

> 
> IMHO, the CAN_RAW_FD_FRAMES is not justifyable towards userspace
> applications.
> 
> I put my view in the code down below, besides the elimination of the 'fd_frames'
> member in struct raw_sock, and the corresponding CAN_RAW_FD_FRAMES sockopt.
> 
> IMO, the old behaviour (that is written in stone) is still preserved for
> 99.999%, _and_ no difficult sockopts are needed.
> Difficult means in this case: it became not straightforward to predict
> which frames got received.


Indeed :-)

And that's not acceptable for legacy apps.

Thanks,
Oliver

Re: [RFC v3] CAN FD support

[Kurt Van Dijck]

Hello Oliver,

On Mon, May 14, 2012 at 09:50:43PM +0200, Oliver Hartkopp wrote:
> Hello Kurt,
> 
> thanks for your feedback.
> 
> Indeed i'm not totally happy with the current hard switch too! Let's see if we
> can develop something less intrusive but still clean in design.
> 
> The question is how to cope with this scenario:
> 
> A legacy application wants to communicate via can0 not knowing anything about
> CAN FD. This legacy app can only read and write struct can_frames and does not
> know about different data rates in the data field.
> 
> 1. For this scenario we need to define (globally!?) if legacy apps generated
> (short) CAN frames use the slow or high data rate in the data field when
> transmitted via that specific CAN FD interface.
IMHO, slow datarate matches the most closely to regular CAN2.0B.
> 
> 2. in any case legacy applications must not get CAN frames with a DLC > 8
Or cut the DLC to 8, and drop the remainder of the message? This changes
the actual can(fd)_frame. That is acceptable since you're running legacy apps
on CANFD bus.

An alternative could be to drop CANFD frames in can_recv() itself.
This may sound inefficient, but remind you're running legacy apps on CANFD busses
_WITH_ CANFD frames on it.

I'd rather choose a suboptimal compatibility mode than adding code to explicitely
tweak the subsystem.
> 
> 3. legacy applications assume struct can_frames on read and write. There
> should no MSG_TRUNC been set in msg flags when receiving a can_frame.
see 2. MSG_TRUNC would be set only with sizes != CAN_MTU & != CANFD_MTU.

This level of compatibility involves modifying the actual received CAN frame
when CANFD frames are received on a socket using CAN_MTU. I accept this
compatibility for sake of preserving the API that DLC <= 8 for such frames.

> 
> OTOH we will have new applications that are aware of CAN FD. These new
> applications may also be able to deal with two different sizes of structs for
> read and write operations (can_frame and canfd_frame) - if we specify it so.
they (ideally) always do:
	ret = recv(cansock, &canfd_frame, CANFD_MTU);
and recv() returns CANFD_MTU _or_ CAN_MTU.
That sounds acceptible.
> 
> Therefore we would need at least some flag if the application is CAN FD aware
> or not. Btw. i'll think about a better simultaneous access of legacy and new
> apps to the CAN interfaces.

Well, I think I just solved this in 2.
> 
> Some more comments inline:

For the remainder, a few topics remain, so I cut a bit:

> If you want to exchange canfd_frames you can add a new sockopt or you
> introduce a new protocol (e.g. CAN_RAWFD). The latter has no additional
> property/method - besides the different protocol number ... %-)
> 
> Would you prefer that solution?

No, IMHO that's even worse :-)
* more code would be duplicated
* no interleaved CAN & CANFD frames on 1 socket.

> > I understood that mixing CAN & CANFD on 1 bus is possible on CANFD chip level,
> > and we do want to support those features by using 2 structs
> > that in fact determine the CAN type. If an application want to mix
> > CAN & CANFD, it is currently not possible without opening another
> > socket, thereby loosing the proper sequence of events.
> 
> 
> Beware of providing a needless flexibility here.
> Once the CAN driver is switched to the CAN FD mode you can send and receive
> all types of frames. You can send normal CAN frames inside struct canfd_frame
> too! So when you open a socket - and you are a CAN FD aware app - you switch
> to CAN_RAW_FD_FRAMES and you are free to send/recv whatever you like.
Now this is interesting. I really tought that a struct can_frame and a
struct canfd_frame with similar content still produce a (slight) different
bitstream on the wire, i.e. a struct can_frame would produce a CAN2.0b bitstream
that is different than the CANFD bitstream.

I did think that this difference in bitstream/protocol would map
on a different struct.
Where am I wrong here?

Even if I'm not convinced, I now better understand your exclusive-or ...
This definitely needs clarification.

> 
> NB: You can only switch the CAN interface MTU when the interface is down.
very good!
No discussion there.
> 
> > 
> > IMHO, the CAN_RAW_FD_FRAMES is not justifyable towards userspace
> > applications.
> > 
> > I put my view in the code down below, besides the elimination of the 'fd_frames'
> > member in struct raw_sock, and the corresponding CAN_RAW_FD_FRAMES sockopt.
> > 
> > IMO, the old behaviour (that is written in stone) is still preserved for
> > 99.999%, _and_ no difficult sockopts are needed.
> > Difficult means in this case: it became not straightforward to predict
> > which frames got received.
> 
> 
> Indeed :-)
> 
> And that's not acceptable for legacy apps.

Even that I avoided this 0.001% difference above in (2.), I'll still
make a nasty comparison here.

If an ABI was 100% the same, I argue that if a legacy app does
	int value = 1;
	setsockopt(cansock, 5, &value, sizeof(value));
Then you also are changing the behaviour of the legacy application now.
No problem, let's create CANFD_RAW. But even that is not 100% compatible.

Ok, this is unlikely to be a problem, but 'unlikely' > 0%.
You see my point? A legacy app doing
	recv(cansock, &can_frame, <CANFD_MTU hardcoded>);
may break legacy apps, but this is considered unlikely, and
such crap should not prevent development of good stuff.

Just think about it.

Anyway, I still think I solved this problem in (2.) by modifying
the received content.

Kind regards,
Kurt

Re: [RFC v3] CAN FD support

[Oliver Hartkopp]

On 15.05.2012 10:34, Kurt Van Dijck wrote:

> On Mon, May 14, 2012 at 09:50:43PM +0200, Oliver Hartkopp wrote:

>> A legacy application wants to communicate via can0 not knowing anything about
>> CAN FD. This legacy app can only read and write struct can_frames and does not
>> know about different data rates in the data field.
>>
>> 1. For this scenario we need to define (globally!?) if legacy apps generated
>> (short) CAN frames use the slow or high data rate in the data field when
>> transmitted via that specific CAN FD interface.
> IMHO, slow datarate matches the most closely to regular CAN2.0B.


You can not rely on this.

The most probable migration scenario is to increase the payload data rate
without changing any application.

>>
>> 2. in any case legacy applications must not get CAN frames with a DLC > 8
> Or cut the DLC to 8, and drop the remainder of the message? This changes
> the actual can(fd)_frame. That is acceptable since you're running legacy apps
> on CANFD bus.
> 
> An alternative could be to drop CANFD frames in can_recv() itself.
> This may sound inefficient, but remind you're running legacy apps on CANFD busses
> _WITH_ CANFD frames on it.


I have a patch for that later in this mail.

> 
> I'd rather choose a suboptimal compatibility mode than adding code to explicitely
> tweak the subsystem.


CAN_RAW_FD_FRAMES *is* this compatibility switch :-)

>>
>> 3. legacy applications assume struct can_frames on read and write. There
>> should no MSG_TRUNC been set in msg flags when receiving a can_frame.
> see 2. MSG_TRUNC would be set only with sizes != CAN_MTU & != CANFD_MTU.
> 
> This level of compatibility involves modifying the actual received CAN frame
> when CANFD frames are received on a socket using CAN_MTU. I accept this
> compatibility for sake of preserving the API that DLC <= 8 for such frames.


Yes. I did exactly this in my patch this morning.

If you get a CANFD frame to be passed to a legacy socket

1. check id DLC <= 8 (in raw_rcv())
2. cut the length from CANFD_MTU to CAN_MTU (in raw_recvmsg())

> 
>>
>> OTOH we will have new applications that are aware of CAN FD. These new
>> applications may also be able to deal with two different sizes of structs for
>> read and write operations (can_frame and canfd_frame) - if we specify it so.
> they (ideally) always do:
> 	ret = recv(cansock, &canfd_frame, CANFD_MTU);
> and recv() returns CANFD_MTU _or_ CAN_MTU.
> That sounds acceptible.


Yes. IMO we can specify that the new CANFD aware applications have to deal
with both MTUs. But we can not demand this from legacy apps.


>> Beware of providing a needless flexibility here.
>> Once the CAN driver is switched to the CAN FD mode you can send and receive
>> all types of frames. You can send normal CAN frames inside struct canfd_frame
>> too! So when you open a socket - and you are a CAN FD aware app - you switch
>> to CAN_RAW_FD_FRAMES and you are free to send/recv whatever you like.
> Now this is interesting. I really tought that a struct can_frame and a
> struct canfd_frame with similar content still produce a (slight) different
> bitstream on the wire, i.e. a struct can_frame would produce a CAN2.0b bitstream
> that is different than the CANFD bitstream.
> 
> I did think that this difference in bitstream/protocol would map
> on a different struct.
> Where am I wrong here?


Yes. You get a different bitstream.

When you send a canfd_frame you can set the EDL bit and specify a length of
8 bytes which is different to a legacy can_frame without the EDL bit.

> 
> Even if I'm not convinced, I now better understand your exclusive-or ...
> This definitely needs clarification.


Yes. Especially we need to specify how the CAN controller should send
can_frames passed to its CAN driver. Should it been send with or without
EDL bit, and should it been send with high data rate or not.

This is what i wanted to suggest with the global CANFD driver settings.

(..)

> 
> Even that I avoided this 0.001% difference above in (2.), I'll still
> make a nasty comparison here.
> 
> If an ABI was 100% the same, I argue that if a legacy app does
> 	int value = 1;
> 	setsockopt(cansock, 5, &value, sizeof(value));
> Then you also are changing the behaviour of the legacy application now.


This is indeed nasty.
A legacy application would never have executed using an undefined sockopt.
So this has nothing to do with backwards compatibility. It's simply broken.

> No problem, let's create CANFD_RAW. But even that is not 100% compatible.
> 
> Ok, this is unlikely to be a problem, but 'unlikely' > 0%.
> You see my point? A legacy app doing
> 	recv(cansock, &can_frame, <CANFD_MTU hardcoded>);
> may break legacy apps, but this is considered unlikely, and
> such crap should not prevent development of good stuff.
> 
> Just think about it.


Done :-)

Legacy crap that was technically possible to do just has to be supported.

Here's my patch:

Changes to the previous version:

CANFD aware apps set CAN_RAW_FD_FRAMES to enable longer MTUs (not new).

1. CANFD aware apps may get CAN_MTU and CANFD_MTU frames on rx
2. CANFD aware apps may send CAN_MTU and CANFD_MTU frames on tx
3. legacy apps can receive CAN_MTU and CANFD_MTU frames if the DLC <= 8
   (the possible CANFD_MTU is cut down to CAN_MTU to preserve the ABI)

Effects to the CAN netdev driver:

Legacy operation (CAN_MTU)
- send and receive struct can_frames

CANFD operation (CANFD_MTU and CAN_MTU)
- you can send CANFD_MTU and CAN_MTU frames
  (a default setting (EDL, HDR) for CAN_MTU frames needs to be specified)
- the driver generates always canfd_frames on reception


diff --git a/net/can/raw.c b/net/can/raw.c
index f298707..a20ee8e 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -120,12 +120,11 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
 	if (!ro->recv_own_msgs && oskb->sk == sk)
 		return;
 
-	/* only pass correct frames to the socket */
-	if (ro->fd_frames) {
-		if (unlikely(oskb->len != CANFD_MTU))
-			return;
-	} else {
-		if (unlikely(oskb->len != CAN_MTU))
+	/* do not pass frames with DLC > 8 to a legacy socket */
+	if (!ro->fd_frames) {
+		struct canfd_frame *cfd = (struct canfd_frame *)oskb->data;
+
+		if (unlikely(cfd->len > CAN_MAX_DLEN))
 			return;
 	}
 
@@ -696,7 +695,7 @@ static int raw_sendmsg(struct kiocb *iocb, struct socket *sock,
 		ifindex = ro->ifindex;
 
 	if (ro->fd_frames) {
-		if (unlikely(size != CANFD_MTU))
+		if (unlikely(size != CANFD_MTU && size != CAN_MTU))
 			return -EINVAL;
 	} else {
 		if (unlikely(size != CAN_MTU))
@@ -752,7 +751,9 @@ static int raw_recvmsg(struct kiocb *iocb, struct socket *sock,
 		       struct msghdr *msg, size_t size, int flags)
 {
 	struct sock *sk = sock->sk;
+	struct raw_sock *ro = raw_sk(sk);
 	struct sk_buff *skb;
+	int rxmtu;
 	int err = 0;
 	int noblock;
 
@@ -763,10 +764,20 @@ static int raw_recvmsg(struct kiocb *iocb, struct socket *sock,
 	if (!skb)
 		return err;
 
-	if (size < skb->len)
+	/*
+	 * when serving a legacy socket the DLC <= 8 is already checked inside
+	 * raw_rcv(). Now check if we need to pass a canfd_frame to a legacy
+	 * socket and cut the possible CANFD_MTU/CAN_MTU length to CAN_MTU
+	 */
+	if (!ro->fd_frames)
+		rxmtu = CAN_MTU;
+	else
+		rxmtu = skb->len;
+
+	if (size < rxmtu)
 		msg->msg_flags |= MSG_TRUNC;
 	else
-		size = skb->len;
+		size = rxmtu;
 
 	err = memcpy_toiovec(msg->msg_iov, skb->data, size);
 	if (err < 0) {

Re: [RFC v3] CAN FD support

[Kurt Van Dijck]

On Tue, May 15, 2012 at 11:19:33AM +0200, Oliver Hartkopp wrote:
> On 15.05.2012 10:34, Kurt Van Dijck wrote:
> 
> > On Mon, May 14, 2012 at 09:50:43PM +0200, Oliver Hartkopp wrote:
> 
> >> A legacy application wants to communicate via can0 not knowing anything about
> >> CAN FD. This legacy app can only read and write struct can_frames and does not
> >> know about different data rates in the data field.
> >>
> >> 1. For this scenario we need to define (globally!?) if legacy apps generated
> >> (short) CAN frames use the slow or high data rate in the data field when
> >> transmitted via that specific CAN FD interface.
> > IMHO, slow datarate matches the most closely to regular CAN2.0B.
> 
> 
> You can not rely on this.
> 
> The most probable migration scenario is to increase the payload data rate
> without changing any application.
This view makes also sense. This is not a breaking issue for me :-)
> 
> >>
> >> 2. in any case legacy applications must not get CAN frames with a DLC > 8
> > Or cut the DLC to 8, and drop the remainder of the message? This changes
> > the actual can(fd)_frame. That is acceptable since you're running legacy apps
> > on CANFD bus.
> > 
> > An alternative could be to drop CANFD frames in can_recv() itself.
> > This may sound inefficient, but remind you're running legacy apps on CANFD busses
> > _WITH_ CANFD frames on it.
> 
> 
> I have a patch for that later in this mail.
waaw. You're running _with_ HDR bit on :-) ?
> 
> > 
> > I'd rather choose a suboptimal compatibility mode than adding code to explicitely
> > tweak the subsystem.
> 
> 
> CAN_RAW_FD_FRAMES *is* this compatibility switch :-)
see below. I see improvement, but I meant something different.
> 
> >>
> >> 3. legacy applications assume struct can_frames on read and write. There
> >> should no MSG_TRUNC been set in msg flags when receiving a can_frame.
> > see 2. MSG_TRUNC would be set only with sizes != CAN_MTU & != CANFD_MTU.
> > 
> > This level of compatibility involves modifying the actual received CAN frame
> > when CANFD frames are received on a socket using CAN_MTU. I accept this
> > compatibility for sake of preserving the API that DLC <= 8 for such frames.
> 
> 
> Yes. I did exactly this in my patch this morning.
Yes. I like it.
> 
> If you get a CANFD frame to be passed to a legacy socket
> 
> 1. check id DLC <= 8 (in raw_rcv())
> 2. cut the length from CANFD_MTU to CAN_MTU (in raw_recvmsg())
> 
> > 
> >>

[...]
> 
> Yes. Especially we need to specify how the CAN controller should send
> can_frames passed to its CAN driver. Should it been send with or without
> EDL bit, and should it been send with high data rate or not.
> 
> This is what i wanted to suggest with the global CANFD driver settings.

netlink-wise, I would have considered this a device setting.

[...]
> > 
> > Ok, this is unlikely to be a problem, but 'unlikely' > 0%.
> > You see my point? A legacy app doing
> > 	recv(cansock, &can_frame, <CANFD_MTU hardcoded>);
> > may break legacy apps, but this is considered unlikely, and
> > such crap should not prevent development of good stuff.
> > 
> > Just think about it.
> 
> 
> Done :-)
> 
> Legacy crap that was technically possible to do just has to be supported.

I still don't get the real difference, I probably never will :-(
> 
> Here's my patch:

> 
> Changes to the previous version:
> 
> CANFD aware apps set CAN_RAW_FD_FRAMES to enable longer MTUs (not new).
> 
> 1. CANFD aware apps may get CAN_MTU and CANFD_MTU frames on rx
> 2. CANFD aware apps may send CAN_MTU and CANFD_MTU frames on tx
> 3. legacy apps can receive CAN_MTU and CANFD_MTU frames if the DLC <= 8
>    (the possible CANFD_MTU is cut down to CAN_MTU to preserve the ABI)
> 
> Effects to the CAN netdev driver:
> 
> Legacy operation (CAN_MTU)
> - send and receive struct can_frames
> 
> CANFD operation (CANFD_MTU and CAN_MTU)
> - you can send CANFD_MTU and CAN_MTU frames
>   (a default setting (EDL, HDR) for CAN_MTU frames needs to be specified)
> - the driver generates always canfd_frames on reception
Interesting.
How does a driver makes the difference between received
CAN2.0B frames & CANFD frames?
I think it makes sense, equally to the send() path, to allow the driver
to generate can_frame _or_ canfd_frame on reception, depending on the
bitstream (if possible).

A CANFD application will then be able to distinguish between them based
on the return value of recv_msg (CAN_MTU or CANFD_MTU, even with payload <= 8).

Did I mention I very much like this additional patch?
Well done.

Regards,
Kurt

Re: [RFC v3] CAN FD support

[Oliver Hartkopp]

On 15.05.2012 12:03, Kurt Van Dijck wrote:

> On Tue, May 15, 2012 at 11:19:33AM +0200, Oliver Hartkopp wrote:

>>>> 2. in any case legacy applications must not get CAN frames with a DLC > 8
>>> Or cut the DLC to 8, and drop the remainder of the message? This changes
>>> the actual can(fd)_frame. That is acceptable since you're running legacy apps
>>> on CANFD bus.
>>>
>>> An alternative could be to drop CANFD frames in can_recv() itself.
>>> This may sound inefficient, but remind you're running legacy apps on CANFD busses
>>> _WITH_ CANFD frames on it.
>>
>>
>> I have a patch for that later in this mail.
> waaw. You're running _with_ HDR bit on :-) ?


The effect is when you use legacy can_frames the canfd_frame.flags is not set
explicitly. Therefore you need a (per device?) setting how to deal with the
short CAN_MTU frames. Assuming HDR bit settings in CAN_MTU sized frames is wrong.

(..)

>> Yes. Especially we need to specify how the CAN controller should send
>> can_frames passed to its CAN driver. Should it been send with or without
>> EDL bit, and should it been send with high data rate or not.
>>
>> This is what i wanted to suggest with the global CANFD driver settings.
> 
> netlink-wise, I would have considered this a device setting.
> 


Yep! It's a job for drivers/net/can/dev.c :-)


>> Here's my patch:
> 
>>
>> Changes to the previous version:
>>
>> CANFD aware apps set CAN_RAW_FD_FRAMES to enable longer MTUs (not new).
>>
>> 1. CANFD aware apps may get CAN_MTU and CANFD_MTU frames on rx
>> 2. CANFD aware apps may send CAN_MTU and CANFD_MTU frames on tx
>> 3. legacy apps can receive CAN_MTU and CANFD_MTU frames if the DLC <= 8
>>    (the possible CANFD_MTU is cut down to CAN_MTU to preserve the ABI)
>>
>> Effects to the CAN netdev driver:
>>
>> Legacy operation (CAN_MTU)
>> - send and receive struct can_frames
>>
>> CANFD operation (CANFD_MTU and CAN_MTU)
>> - you can send CANFD_MTU and CAN_MTU frames
>>   (a default setting (EDL, HDR) for CAN_MTU frames needs to be specified)
>> - the driver generates always canfd_frames on reception
> Interesting.
> How does a driver makes the difference between received
> CAN2.0B frames & CANFD frames?


As you always generate canfd_frames in this operating mode you can express
this information within canfd_frame.flags.

> I think it makes sense, equally to the send() path, to allow the driver
> to generate can_frame _or_ canfd_frame on reception, depending on the
> bitstream (if possible).


As written above this can be done with a singe canfd_frame.

A legacy app would get the can_frame as-is when DLC <= 8
A CANFD frame can evaluate the bits if they are at least interesting for the
app. For a detailed 'candump' output this could be interesting ;-)

> 
> A CANFD application will then be able to distinguish between them based
> on the return value of recv_msg (CAN_MTU or CANFD_MTU, even with payload <= 8).


There's no reason to provide a CAN_MTU frame to a CANFD application once the
driver is switched to CANFD-mode.

The CANFD app only gets the old-style CAN_MTU frames when the CAN interface is
not in CANFD mode.

> 
> Did I mention I very much like this additional patch?
> Well done.


Thanks to you! I was not really happy with the hard exclusive-or switch and
your remarks made me thinking about the simultaneous access with new and
legacy apps again. I think cutting the canfd_frames down to can_frames when
the content is generally usable for the legacy app makes the migration pretty
handy :-)

Will send a v4 patch for the ongoing discussion.

Regards,
Oliver

Re: [RFC v3] CAN FD support

[Kurt Van Dijck]

On Tue, May 15, 2012 at 03:01:28PM +0200, Oliver Hartkopp wrote:
> On 15.05.2012 12:03, Kurt Van Dijck wrote:
> 
> > On Tue, May 15, 2012 at 11:19:33AM +0200, Oliver Hartkopp wrote:
> 
> >>>> 2. in any case legacy applications must not get CAN frames with a DLC > 8
> >>> Or cut the DLC to 8, and drop the remainder of the message? This changes
> >>> the actual can(fd)_frame. That is acceptable since you're running legacy apps
> >>> on CANFD bus.
> >>>
[...]
> >>
> >> I have a patch for that later in this mail.
> > waaw. You're running _with_ HDR bit on :-) ?
> 
I meant you have a "High data rate" yourself. It was meant as some form
of humor to indicate my supprise of you generating a new patch very quick ...

[...]
> >> 1. CANFD aware apps may get CAN_MTU and CANFD_MTU frames on rx
> >> 2. CANFD aware apps may send CAN_MTU and CANFD_MTU frames on tx
> >> 3. legacy apps can receive CAN_MTU and CANFD_MTU frames if the DLC <= 8
> >>    (the possible CANFD_MTU is cut down to CAN_MTU to preserve the ABI)
> >>
> >> Effects to the CAN netdev driver:
> >>
> >> Legacy operation (CAN_MTU)
> >> - send and receive struct can_frames
> >>
> >> CANFD operation (CANFD_MTU and CAN_MTU)
> >> - you can send CANFD_MTU and CAN_MTU frames
> >>   (a default setting (EDL, HDR) for CAN_MTU frames needs to be specified)
> >> - the driver generates always canfd_frames on reception
> > Interesting.
> > How does a driver makes the difference between received
> > CAN2.0B frames & CANFD frames?
> 
> 
> As you always generate canfd_frames in this operating mode you can express
> this information within canfd_frame.flags.
> 
> > I think it makes sense, equally to the send() path, to allow the driver
> > to generate can_frame _or_ canfd_frame on reception, depending on the
> > bitstream (if possible).
> 
> 
> As written above this can be done with a singe canfd_frame.
> 
> A legacy app would get the can_frame as-is when DLC <= 8
> A CANFD frame can evaluate the bits if they are at least interesting for the
> app. For a detailed 'candump' output this could be interesting ;-)
> 
> > 
> > A CANFD application will then be able to distinguish between them based
> > on the return value of recv_msg (CAN_MTU or CANFD_MTU, even with payload <= 8).
> 
> 
> There's no reason to provide a CAN_MTU frame to a CANFD application once the
> driver is switched to CANFD-mode.
I think there is a reason. If a CAN2.0B frame was on the wire, generating
a can_frame rather than a canfd_frame better reflects what was on the wire.

I still think a can_frame & canfd_frame with same data differ on the wire.

If a CANFD chip (in a specific mode) is unable to distinguish this, then
that's a driver issue. But I propose not to put such sideeffect into the
generic parts.
> 
> The CANFD app only gets the old-style CAN_MTU frames when the CAN interface is
> not in CANFD mode.
> 
> > 
> > Did I mention I very much like this additional patch?
> > Well done.
> 
> 
> Thanks to you! I was not really happy with the hard exclusive-or switch and
> your remarks made me thinking about the simultaneous access with new and
> legacy apps again.
You're not tired of me then :-)
> I think cutting the canfd_frames down to can_frames when
> the content is generally usable for the legacy app makes the migration pretty
> handy :-)
My first suggestion was to also cut longer CANFD frames down to 8 bytes
(thereby loosing data!).
Remember it's still a compatibility mode. I think that would be acceptable.
> 
> Will send a v4 patch for the ongoing discussion.
I didn't wait on them ..., but now I will
> 
> Regards,
> Oliver
Kind regards,
Kurt

Re: [RFC v3] CAN FD support

[Oliver Hartkopp]

On 15.05.2012 15:37, Kurt Van Dijck wrote:

> On Tue, May 15, 2012 at 03:01:28PM +0200, Oliver Hartkopp wrote:

>>>>
>>>> I have a patch for that later in this mail.
>>> waaw. You're running _with_ HDR bit on :-) ?
>>
> I meant you have a "High data rate" yourself. It was meant as some form
> of humor to indicate my supprise of you generating a new patch very quick ...


Ah - ok :-)

Indeed i'm on vacancy this week and my wife pimps her garden so i need to
surveillance the dog in the house  8-)


>> There's no reason to provide a CAN_MTU frame to a CANFD application once the
>> driver is switched to CANFD-mode.
> I think there is a reason. If a CAN2.0B frame was on the wire, generating
> a can_frame rather than a canfd_frame better reflects what was on the wire.
> 
> I still think a can_frame & canfd_frame with same data differ on the wire.


Yes. But you can express this difference with the CANFD_NOEDL bit in
canfd_frame.flags:

CANFD_NOEDL == 1 => standard CAN 2.0B frame
CANFD_NOEDL == 0 => CAN FD frame (whatever DLC 0-F is used)

The distinction of this bit is only relevant to CANFD aware apps anyway.

>> Thanks to you! I was not really happy with the hard exclusive-or switch and
>> your remarks made me thinking about the simultaneous access with new and
>> legacy apps again.
> You're not tired of me then :-)


No. You are sometimes pretty brutal with your suggestions which makes me wake
up from sleep. ;-)

>> I think cutting the canfd_frames down to can_frames when
>> the content is generally usable for the legacy app makes the migration pretty
>> handy :-)
> My first suggestion was to also cut longer CANFD frames down to 8 bytes
> (thereby loosing data!).
> Remember it's still a compatibility mode. I think that would be acceptable.


At the point where CAN FD frames with a DLC > 8 become relevant for
applications a new CANFD aware application *should* come into service.

Silently cutting payload data is a bad idea.

Regards,
Oliver

Re: [RFC v3] CAN FD support

[Kurt Van Dijck]

> 
> >> There's no reason to provide a CAN_MTU frame to a CANFD application once the
> >> driver is switched to CANFD-mode.
> > I think there is a reason. If a CAN2.0B frame was on the wire, generating
> > a can_frame rather than a canfd_frame better reflects what was on the wire.
> > 
> > I still think a can_frame & canfd_frame with same data differ on the wire.
> 
> 
> Yes. But you can express this difference with the CANFD_NOEDL bit in
> canfd_frame.flags:
> 
> CANFD_NOEDL == 1 => standard CAN 2.0B frame
> CANFD_NOEDL == 0 => CAN FD frame (whatever DLC 0-F is used)
I see now.
Some more pieces fall into place.
I still haven't memorised the complete docs.
> 

> 
> >> I think cutting the canfd_frames down to can_frames when
> >> the content is generally usable for the legacy app makes the migration pretty
> >> handy :-)
> > My first suggestion was to also cut longer CANFD frames down to 8 bytes
> > (thereby loosing data!).
> > Remember it's still a compatibility mode. I think that would be acceptable.
> 
> 
> At the point where CAN FD frames with a DLC > 8 become relevant for
> applications a new CANFD aware application *should* come into service.
> 
> Silently cutting payload data is a bad idea.

I'm mostly not in favor of modifying/cutting the payload,
but I consider it here.
Since you introduce a sockopt to switch to CANFD mode, I foresee following
scenario:

People write a brand new CANFD aware application, at least, they apply canfd_frame.
They send frames, up to the 64 bytes, but do not see anything coming, because ....
they forgot to do setsockopt(...).

If they only would see their frames cut to 8 bytes, one can notice
there might be some compatibility mode in place.
If they don't see anything, yeah, the problem could be anywhere.

So a question comes in: I do this & that, and others say:
is your CAN bus ok, is this, is that?
until finally, as that is the way it goes, the code comes along,
and someone says: of course, ...

So I think it's worth considering ... due to the socket option.
Especially since I think the impact is minimal.

Kurt