[PATCH 0/2] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS

[PATCH 0/2] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS

[Ira W. Snyder]

This series adds support for can_cmp_echo_skb(), and then uses it to fix
support for CAN_RAW_RECV_OWN_MSGS in the janz-ican3 driver.

After these patches are applied, the SocketCAN test tst-rcv-own-msgs works
correctly. However, if cangen is invoked in non-loopback mode, the driver
stops accepting packets after a few seconds.

The cangen command which causes the driver to stop accepting packets:
cangen -g 0 -e -i -L 8 -x can0

Due to the issues mentioned above, please DO NOT apply the 2nd patch in
this series!

Ira W. Snyder (2):
  can: add can_cmp_echo_skb() for echo skb comparison
  can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS

 drivers/net/can/dev.c        |   34 ++++++++++++++++++++++++++++++++++
 drivers/net/can/janz-ican3.c |   37 ++++++++++++++++++++++++++++++-------
 include/linux/can/dev.h      |    2 ++
 3 files changed, 66 insertions(+), 7 deletions(-)

-- 
1.7.3.4

[PATCH 1/2] can: add can_cmp_echo_skb() for echo skb comparison

[Ira W. Snyder]

This function allows drivers to compare an incoming skb against the echo
skb stack. On CAN hardware which has support for hardware loopback, this
allows drivers to support the CAN_RAW_RECV_OWN_MSGS flag correctly.

Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
 drivers/net/can/dev.c   |   34 ++++++++++++++++++++++++++++++++++
 include/linux/can/dev.h |    2 ++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index c5fe3a3..a6c5076 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -348,6 +348,40 @@ unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx)
 EXPORT_SYMBOL_GPL(can_get_echo_skb);
 
 /*
+ * Compare an skb with an existing echo skb
+ *
+ * This function will be used on devices which have a hardware loopback.
+ * On these devices, this function can be used to compare a received skb
+ * with the saved echo skbs so that the hardware echo skb can be dropped.
+ *
+ * Returns true if the skb's are identical, false otherwise.
+ */
+bool can_cmp_echo_skb(struct sk_buff *skb, struct net_device *dev,
+		      unsigned int idx)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	struct can_frame *cf = (struct can_frame *)skb->data;
+
+	BUG_ON(idx >= priv->echo_skb_max);
+
+	if (priv->echo_skb[idx]) {
+		struct sk_buff *echo_skb = priv->echo_skb[idx];
+		struct can_frame *echo_cf = (struct can_frame *)echo_skb->data;
+
+		if (cf->can_id != echo_cf->can_id)
+			return false;
+
+		if (cf->can_dlc != echo_cf->can_dlc)
+			return false;
+
+		return memcmp(cf->data, echo_cf->data, cf->can_dlc) == 0;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(can_cmp_echo_skb);
+
+/*
   * Remove the skb from the stack and free it.
   *
   * The function is typically called when TX failed.
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 5d2efe7..904a938 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -93,6 +93,8 @@ void can_bus_off(struct net_device *dev);
 void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 		      unsigned int idx);
 unsigned int can_get_echo_skb(struct net_device *dev, unsigned int idx);
+bool can_cmp_echo_skb(struct sk_buff *skb, struct net_device *dev,
+		      unsigned int idx);
 void can_free_echo_skb(struct net_device *dev, unsigned int idx);
 
 struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf);
-- 
1.7.3.4

[PATCH 2/2] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS

[Ira W. Snyder]

The Janz VMOD-ICAN3 firmware does not support any sort of TX-done
notification or interrupt. The driver previously used the hardware
loopback to attempt to work around this deficiency, but this caused all
sockets to receive all messages, even if CAN_RAW_RECV_OWN_MSGS is off.

Using the new function can_cmp_echo_skb(), we can drop the loopback
messages and return the original skbs. This fixes the issues with
CAN_RAW_RECV_OWN_MSGS, but leaves the driver vulnerable to a condition
where it will overwrite the echo skb stack.

NOT EVEN CLOSE TO Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---

WARNING: BROKEN CODE!

If a user uses non-loopback mode and sends packets very quickly, the driver
will stop accepting packets until the bus is re-started.

I've managed to trigger the BUG printk() in can_put_echo_skb(), since the
receive queue can fall behind the transmit queue, and I don't know how to
detect that situation.

In short, the patch "works", but I'm not happy with it. See my next posting
for a working patch that takes a different approach.

 drivers/net/can/janz-ican3.c |   37 ++++++++++++++++++++++++++++++-------
 1 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index 08c893c..183c88c 100644
--- a/drivers/net/can/janz-ican3.c
+++ b/drivers/net/can/janz-ican3.c
@@ -235,10 +235,10 @@ struct ican3_dev {
 
 	/* fast host interface */
 	unsigned int fastrx_start;
-	unsigned int fastrx_int;
 	unsigned int fastrx_num;
 	unsigned int fasttx_start;
 	unsigned int fasttx_num;
+	unsigned int fasttx_echo;
 
 	/* first free DPM page */
 	unsigned int free_page;
@@ -454,7 +454,6 @@ static void __devinit ican3_init_fast_host_interface(struct ican3_dev *mod)
 	/* save the start recv page */
 	mod->fastrx_start = mod->free_page;
 	mod->fastrx_num = 0;
-	mod->fastrx_int = 0;
 
 	/* build a single fast tohost queue descriptor */
 	memset(&desc, 0, sizeof(desc));
@@ -491,6 +490,7 @@ static void __devinit ican3_init_fast_host_interface(struct ican3_dev *mod)
 	/* save the start xmit page */
 	mod->fasttx_start = mod->free_page;
 	mod->fasttx_num = 0;
+	mod->fasttx_echo = 0;
 
 	/* build a single fast fromhost queue descriptor */
 	memset(&desc, 0, sizeof(desc));
@@ -820,6 +820,9 @@ static void ican3_to_can_frame(struct ican3_dev *mod,
 		if (desc->data[0] & ICAN3_EFF_RTR)
 			cf->can_id |= CAN_RTR_FLAG;
 
+		if (desc->data[1] & ICAN3_ECHO)
+			dev_err(mod->dev, "echo frame\n");
+
 		if (desc->data[0] & ICAN3_EFF) {
 			cf->can_id |= CAN_EFF_FLAG;
 			cf->can_id |= desc->data[2] << 21; /* 28-21 */
@@ -837,7 +840,8 @@ static void ican3_to_can_frame(struct ican3_dev *mod,
 
 static void can_frame_to_ican3(struct ican3_dev *mod,
 			       struct can_frame *cf,
-			       struct ican3_fast_desc *desc)
+			       struct ican3_fast_desc *desc,
+			       bool echo)
 {
 	/* clear out any stale data in the descriptor */
 	memset(desc->data, 0, sizeof(desc->data));
@@ -845,7 +849,9 @@ static void can_frame_to_ican3(struct ican3_dev *mod,
 	/* we always use the extended format, with the ECHO flag set */
 	desc->command = ICAN3_CAN_TYPE_EFF;
 	desc->data[0] |= cf->can_dlc;
-	desc->data[1] |= ICAN3_ECHO;
+
+	if (echo)
+		desc->data[1] |= ICAN3_ECHO;
 
 	if (cf->can_id & CAN_RTR_FLAG)
 		desc->data[0] |= ICAN3_EFF_RTR;
@@ -1126,6 +1132,7 @@ static int ican3_recv_skb(struct ican3_dev *mod)
 	struct can_frame *cf;
 	struct sk_buff *skb;
 	unsigned long flags;
+	int i;
 
 	spin_lock_irqsave(&mod->lock, flags);
 
@@ -1150,6 +1157,16 @@ static int ican3_recv_skb(struct ican3_dev *mod)
 	/* convert the ICAN3 frame into Linux CAN format */
 	ican3_to_can_frame(mod, &desc, cf);
 
+	/* check if this is an ECHO frame */
+	for (i = 0; i < ICAN3_TX_BUFFERS; i++) {
+		if (can_cmp_echo_skb(skb, ndev, i)) {
+			stats->rx_packets++;
+			stats->rx_bytes += can_get_echo_skb(ndev, i);
+			kfree_skb(skb);
+			goto err_noalloc;
+		}
+	}
+
 	/* receive the skb, update statistics */
 	netif_receive_skb(skb);
 	stats->rx_packets++;
@@ -1422,6 +1439,9 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 	void __iomem *desc_addr;
 	unsigned long flags;
 
+	if (can_dropped_invalid_skb(ndev, skb))
+		return NETDEV_TX_OK;
+
 	spin_lock_irqsave(&mod->lock, flags);
 
 	/* check that we can actually transmit */
@@ -1439,7 +1459,11 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 	memcpy_fromio(&desc, desc_addr, 1);
 
 	/* convert the Linux CAN frame into ICAN3 format */
-	can_frame_to_ican3(mod, cf, &desc);
+	can_frame_to_ican3(mod, cf, &desc, skb->pkt_type == PACKET_LOOPBACK);
+
+	/* add to the ECHO stack */
+	can_put_echo_skb(skb, ndev, mod->fasttx_echo);
+	mod->fasttx_echo = (mod->fasttx_echo + 1) & (ICAN3_TX_BUFFERS - 1);
 
 	/*
 	 * the programming manual says that you must set the IVALID bit, then
@@ -1462,7 +1486,6 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 	/* update statistics */
 	stats->tx_packets++;
 	stats->tx_bytes += cf->can_dlc;
-	kfree_skb(skb);
 
 	/*
 	 * This hardware doesn't have TX-done notifications, so we'll try and
@@ -1654,7 +1677,7 @@ static int __devinit ican3_probe(struct platform_device *pdev)
 	dev = &pdev->dev;
 
 	/* allocate the CAN device and private data */
-	ndev = alloc_candev(sizeof(*mod), 0);
+	ndev = alloc_candev(sizeof(*mod), ICAN3_TX_BUFFERS);
 	if (!ndev) {
 		dev_err(dev, "unable to allocate CANdev\n");
 		ret = -ENOMEM;
-- 
1.7.3.4

Re: [PATCH 2/2] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS

[Oliver Hartkopp]

On 05.07.2012 22:58, Ira W. Snyder wrote:

> The Janz VMOD-ICAN3 firmware does not support any sort of TX-done
> notification or interrupt. The driver previously used the hardware
> loopback to attempt to work around this deficiency, but this caused all
> sockets to receive all messages, even if CAN_RAW_RECV_OWN_MSGS is off.
> 
> Using the new function can_cmp_echo_skb(), we can drop the loopback
> messages and return the original skbs. This fixes the issues with
> CAN_RAW_RECV_OWN_MSGS, but leaves the driver vulnerable to a condition
> where it will overwrite the echo skb stack.
> 
> NOT EVEN CLOSE TO Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> ---
> 
> WARNING: BROKEN CODE!
> 
> If a user uses non-loopback mode and sends packets very quickly, the driver
> will stop accepting packets until the bus is re-started.
> 
> I've managed to trigger the BUG printk() in can_put_echo_skb(), since the
> receive queue can fall behind the transmit queue, and I don't know how to
> detect that situation.
> 
> In short, the patch "works", but I'm not happy with it. See my next posting
> for a working patch that takes a different approach.
> 


Hello Ira,

i tried to get behind your driver this weekend.

The first thing:

ICAN3_TX_BUFFERS is 512 !! (whow)

Usually we use the netdev tx queue to pick one or two CAN frames (when there's
a shadow tx register available) but not 512 ...

Please try one or two tx buffers to take advantage of Linux CAN frame
handling. This should also make the things easier in your napi function that
handles the interrupts and incoming DPM messages.

If you only need to check the received CAN frames against 1-2 echo_skbs this
should be easy and fast to do in the napi function.

ndev = alloc_candev(sizeof(*mod), ICAN3_TX_BUFFERS);

is nice with ICAN3_TX_BUFFERS = 1 or 2 ... (preferably 1 for the moment)

E.g. when you detect a CAN frame in the napi function, that is in the
echo_skbs and you consume/"get" these echo_skbs, you may restart the queue
with netif_start_queue(dev) at that point.

Of course this means that there must not be any implementation problem with
the echo mode in the Janz card: If you don't get the frames back the echo_skbs
are not cleared and the tx-queue is not re-started again.

If you get any problems with the Janz card according this topic we might fix
that by adding a timestamp to the echo_skbs to detect timeouts (e.g. to
automatically clear echo_skbs after 200ms/500ms).

Btw. you have an interesting problem to solve :-)

Best regards,
Oliver