[PATCH 0/6] can: janz-ican3: multiple fixes

[PATCH 0/6] can: janz-ican3: multiple fixes

[Ira W. Snyder]

From: "Ira W. Snyder" <iws@ovro.caltech.edu>

This patch series started out as a way to fix support for
CAN_RAW_RECV_OWN_MSGS, and has grown quite a bit since then.

The new addition is a workaround for a quirk in the firmware. If you have
enabled infinite bus error messages to be generated, but your host CPU
cannot keep up, the firmware will stop responding to control messages until
it is hard-reset. This can be worked around by setting a small bus error
quota, and re-enabling bus errors every time you receive a message.

With this change, leaving the controller in "one-shot off" mode, it will
successfully send the packet as soon as the bus is reconnected. This is the
expected behavior, and it works well. The "one-shot on" mode works as well,
without retransmissions.

I moved the TX-error vs. ECHO skb handling from patch #6 (add support for
one shot mode) into patch #4 (fix support for CAN_RAW_RECV_OWN_MSGS). This
puts all of the ECHO skb changes into a single patch.

Many thanks to those who have provided feedback for this patch series. It
is appreciated.

Changes since last version:
- several unrelated changes broken into separate small patches
- ECHO vs. TX-error handling moved into patch #4, where it belongs
- avoid firmware lockup caused by patch #4's use of bus error messages
- support for single shot mode is now very simple
- various nitpicks suggested by reviewers

Ira W. Snyder (6):
  can: janz-ican3: remove dead code
  can: janz-ican3: drop invalid skbs
  can: janz-ican3: fix error and byte counters
  can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS
  can: janz-ican3: avoid firmware lockup caused by infinite bus error quota
  can: janz-ican3: add support for one shot mode

 drivers/net/can/janz-ican3.c |  228 ++++++++++++++++++++++++++++++++----------
 1 files changed, 177 insertions(+), 51 deletions(-)

-- 
1.7.8.6

[PATCH 1/6] can: janz-ican3: remove dead code

[Ira W. Snyder]

From: "Ira W. Snyder" <iws@ovro.caltech.edu>

The code which used this variable was removed during review, before the
driver was added to mainline Linux. It is now dead code, and can be
removed.

Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
 drivers/net/can/janz-ican3.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index 08c893c..020055e 100644
--- a/drivers/net/can/janz-ican3.c
+++ b/drivers/net/can/janz-ican3.c
@@ -235,7 +235,6 @@ 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;
@@ -454,7 +453,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));
-- 
1.7.8.6

[PATCH 2/6] can: janz-ican3: drop invalid skbs

[Ira W. Snyder]

From: "Ira W. Snyder" <iws@ovro.caltech.edu>

The commit which added the janz-ican3 driver and commit
3ccd4c61 "can: Unify droping of invalid tx skbs and netdev stats" were
committed into mainline Linux during the same merge window.

Therefore, the addition of this code to the janz-ican3 driver was
forgotten. This patch adds the expected code.

Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
 drivers/net/can/janz-ican3.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index 020055e..ea1919e 100644
--- a/drivers/net/can/janz-ican3.c
+++ b/drivers/net/can/janz-ican3.c
@@ -1420,6 +1420,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 */
-- 
1.7.8.6

[PATCH 3/6] can: janz-ican3: fix error and byte counters

[Ira W. Snyder]

From: "Ira W. Snyder" <iws@ovro.caltech.edu>

The error and byte counter statistics were being incremented
incorrectly. For example, a TX error would be counted both in tx_errors
and rx_errors.

This corrects the problem so that tx_errors and rx_errors are only
incremented for errors caused by packets sent to the bus. Error packets
generated by the driver are not counted.

The byte counters are only increased for packets which are actually
transmitted or received from the bus. Error packets generated by the
driver are not counted.

Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
 drivers/net/can/janz-ican3.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index ea1919e..d9d4ed1 100644
--- a/drivers/net/can/janz-ican3.c
+++ b/drivers/net/can/janz-ican3.c
@@ -907,8 +907,7 @@ static void ican3_handle_msglost(struct ican3_dev *mod, struct ican3_msg *msg)
 	if (skb) {
 		cf->can_id |= CAN_ERR_CRTL;
 		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
-		stats->rx_errors++;
-		stats->rx_bytes += cf->can_dlc;
+		stats->rx_over_errors++;
 		netif_rx(skb);
 	}
 }
@@ -956,7 +955,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 		cf->can_id |= CAN_ERR_CRTL;
 		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
 		stats->rx_over_errors++;
-		stats->rx_errors++;
 	}
 
 	/* error warning + passive interrupt */
@@ -982,7 +980,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 
 		dev_dbg(mod->dev, "bus error interrupt\n");
 		mod->can.can_stats.bus_error++;
-		stats->rx_errors++;
 		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
 
 		switch (ecc & ECC_MASK) {
@@ -1001,8 +998,12 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 			break;
 		}
 
-		if ((ecc & ECC_DIR) == 0)
+		if (!(ecc & ECC_DIR)) {
 			cf->data[2] |= CAN_ERR_PROT_TX;
+			stats->tx_errors++;
+		} else {
+			stats->rx_errors++;
+		}
 
 		cf->data[6] = txerr;
 		cf->data[7] = rxerr;
@@ -1028,8 +1029,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 	}
 
 	mod->can.state = state;
-	stats->rx_errors++;
-	stats->rx_bytes += cf->can_dlc;
 	netif_rx(skb);
 	return 0;
 }
-- 
1.7.8.6

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

[Ira W. Snyder]

From: "Ira W. Snyder" <iws@ovro.caltech.edu>

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 ican3_cmp_echo_skb(), we can drop the loopback
messages and return the original skbs. This fixes the issues with
CAN_RAW_RECV_OWN_MSGS.

A private skb queue is used to store the echo skbs. This avoids the need
for any index management.

Due to a lack of TX-error interrupts, bus errors are permanently
enabled, and are used as a TX-error notification. This is used to drop
an echo skb when transmission fails. Bus error packets are not generated
if the user has not enabled bus error reporting.

Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
 drivers/net/can/janz-ican3.c |  199 ++++++++++++++++++++++++++++++++----------
 1 files changed, 154 insertions(+), 45 deletions(-)

diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index d9d4ed1..1304712 100644
--- a/drivers/net/can/janz-ican3.c
+++ b/drivers/net/can/janz-ican3.c
@@ -220,6 +220,9 @@ struct ican3_dev {
 	/* old and new style host interface */
 	unsigned int iftype;
 
+	/* queue for echo packets */
+	struct sk_buff_head echoq;
+
 	/*
 	 * Any function which changes the current DPM page must hold this
 	 * lock while it is performing data accesses. This ensures that the
@@ -924,7 +927,7 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 	struct net_device *dev = mod->ndev;
 	struct net_device_stats *stats = &dev->stats;
 	enum can_state state = mod->can.state;
-	u8 status, isrc, rxerr, txerr;
+	u8 isrc, ecc, status, rxerr, txerr;
 	struct can_frame *cf;
 	struct sk_buff *skb;
 
@@ -940,15 +943,42 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 		return -EINVAL;
 	}
 
-	skb = alloc_can_err_skb(dev, &cf);
-	if (skb == NULL)
-		return -ENOMEM;
-
 	isrc = msg->data[0];
+	ecc = msg->data[2];
 	status = msg->data[3];
 	rxerr = msg->data[4];
 	txerr = msg->data[5];
 
+	/*
+	 * This hardware lacks any support other than bus error messages to
+	 * determine if packet transmission has failed.
+	 *
+	 * When TX errors happen, one echo skb needs to be dropped from the
+	 * front of the queue.
+	 *
+	 * A small bit of code is duplicated here and below, to avoid error
+	 * skb allocation when it will just be freed immediately.
+	 */
+	if (isrc == CEVTIND_BEI) {
+		dev_dbg(mod->dev, "bus error interrupt, part 1\n");
+
+		/* TX error */
+		if (!(ecc & ECC_DIR)) {
+			kfree_skb(skb_dequeue(&mod->echoq));
+			stats->tx_errors++;
+		} else {
+			stats->rx_errors++;
+		}
+
+		/* bus error reporting is off, return immediately */
+		if (!(mod->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
+			return 0;
+	}
+
+	skb = alloc_can_err_skb(dev, &cf);
+	if (skb == NULL)
+		return -ENOMEM;
+
 	/* data overrun interrupt */
 	if (isrc == CEVTIND_DOI || isrc == CEVTIND_LOST) {
 		dev_dbg(mod->dev, "data overrun interrupt\n");
@@ -976,9 +1006,8 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 
 	/* bus error interrupt */
 	if (isrc == CEVTIND_BEI) {
-		u8 ecc = msg->data[2];
+		dev_dbg(mod->dev, "bus error interrupt, part 2\n");
 
-		dev_dbg(mod->dev, "bus error interrupt\n");
 		mod->can.can_stats.bus_error++;
 		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
 
@@ -998,12 +1027,8 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 			break;
 		}
 
-		if (!(ecc & ECC_DIR)) {
+		if (!(ecc & ECC_DIR))
 			cf->data[2] |= CAN_ERR_PROT_TX;
-			stats->tx_errors++;
-		} else {
-			stats->rx_errors++;
-		}
 
 		cf->data[6] = txerr;
 		cf->data[7] = rxerr;
@@ -1088,6 +1113,85 @@ static void ican3_handle_message(struct ican3_dev *mod, struct ican3_msg *msg)
 }
 
 /*
+ * The ican3 needs to store all echo skbs, and therefore cannot
+ * use the generic infrastructure for this.
+ */
+static void ican3_put_echo_skb(struct ican3_dev *mod, struct sk_buff *skb)
+{
+	struct sock *srcsk = skb->sk;
+
+	if (atomic_read(&skb->users) != 1) {
+		struct sk_buff *old_skb = skb;
+
+		skb = skb_clone(old_skb, GFP_ATOMIC);
+		kfree_skb(old_skb);
+		if (!skb)
+			return;
+	} else {
+		skb_orphan(skb);
+	}
+
+	skb->sk = srcsk;
+
+	/* save this skb for tx interrupt echo handling */
+	skb_queue_tail(&mod->echoq, skb);
+}
+
+static unsigned int ican3_get_echo_skb(struct ican3_dev *mod)
+{
+	struct sk_buff *skb = skb_dequeue(&mod->echoq);
+	struct can_frame *cf;
+	u8 dlc;
+
+	if (!skb)
+		return 0;
+
+	cf = (struct can_frame *)skb->data;
+	dlc = cf->can_dlc;
+
+	/* check flag whether this packet has to be looped back */
+	if (skb->pkt_type != PACKET_LOOPBACK) {
+		kfree_skb(skb);
+		return dlc;
+	}
+
+	skb->protocol = htons(ETH_P_CAN);
+	skb->pkt_type = PACKET_BROADCAST;
+	skb->ip_summed = CHECKSUM_UNNECESSARY;
+	skb->dev = mod->ndev;
+	netif_receive_skb(skb);
+	return dlc;
+}
+
+/*
+ * 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.
+ */
+static bool ican3_echo_skb_matches(struct ican3_dev *mod, struct sk_buff *skb)
+{
+	struct can_frame *cf = (struct can_frame *)skb->data;
+	struct sk_buff *echo_skb = skb_peek(&mod->echoq);
+	struct can_frame *echo_cf;
+
+	if (!echo_skb)
+		return false;
+
+	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;
+}
+
+/*
  * Check that there is room in the TX ring to transmit another skb
  *
  * LOCKING: must hold mod->lock
@@ -1097,6 +1201,10 @@ static bool ican3_txok(struct ican3_dev *mod)
 	struct ican3_fast_desc __iomem *desc;
 	u8 control;
 
+	/* check that we have echo queue space */
+	if (skb_queue_len(&mod->echoq) >= ICAN3_TX_BUFFERS)
+		return false;
+
 	/* copy the control bits of the descriptor */
 	ican3_set_page(mod, mod->fasttx_start + (mod->fasttx_num / 16));
 	desc = mod->dpm + ((mod->fasttx_num % 16) * sizeof(*desc));
@@ -1147,10 +1255,27 @@ static int ican3_recv_skb(struct ican3_dev *mod)
 	/* convert the ICAN3 frame into Linux CAN format */
 	ican3_to_can_frame(mod, &desc, cf);
 
-	/* receive the skb, update statistics */
-	netif_receive_skb(skb);
+	/*
+	 * If this is an ECHO frame received from the hardware loopback
+	 * feature, use the skb saved in the ECHO stack instead. This allows
+	 * the Linux CAN core to support CAN_RAW_RECV_OWN_MSGS correctly.
+	 *
+	 * Since this is a confirmation of a successfully transmitted packet
+	 * sent from this host, update the transmit statistics.
+	 *
+	 * Also, the netdevice queue needs to be allowed to send packets again.
+	 */
+	if (ican3_echo_skb_matches(mod, skb)) {
+		stats->tx_packets++;
+		stats->tx_bytes += ican3_get_echo_skb(mod);
+		kfree_skb(skb);
+		goto err_noalloc;
+	}
+
+	/* update statistics, receive the skb */
 	stats->rx_packets++;
 	stats->rx_bytes += cf->can_dlc;
+	netif_receive_skb(skb);
 
 err_noalloc:
 	/* toggle the valid bit and return the descriptor to the ring */
@@ -1173,13 +1298,13 @@ err_noalloc:
 static int ican3_napi(struct napi_struct *napi, int budget)
 {
 	struct ican3_dev *mod = container_of(napi, struct ican3_dev, napi);
-	struct ican3_msg msg;
 	unsigned long flags;
 	int received = 0;
 	int ret;
 
 	/* process all communication messages */
 	while (true) {
+		struct ican3_msg msg;
 		ret = ican3_recv_msg(mod, &msg);
 		if (ret)
 			break;
@@ -1351,7 +1476,6 @@ static int __devinit ican3_startup_module(struct ican3_dev *mod)
 static int ican3_open(struct net_device *ndev)
 {
 	struct ican3_dev *mod = netdev_priv(ndev);
-	u8 quota;
 	int ret;
 
 	/* open the CAN layer */
@@ -1361,19 +1485,6 @@ static int ican3_open(struct net_device *ndev)
 		return ret;
 	}
 
-	/* set the bus error generation state appropriately */
-	if (mod->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
-		quota = ICAN3_BUSERR_QUOTA_MAX;
-	else
-		quota = 0;
-
-	ret = ican3_set_buserror(mod, quota);
-	if (ret) {
-		dev_err(mod->dev, "unable to set bus-error\n");
-		close_candev(ndev);
-		return ret;
-	}
-
 	/* bring the bus online */
 	ret = ican3_set_bus_state(mod, true);
 	if (ret) {
@@ -1405,6 +1516,9 @@ static int ican3_stop(struct net_device *ndev)
 		return ret;
 	}
 
+	/* drop all outstanding echo skbs */
+	skb_queue_purge(&mod->echoq);
+
 	/* close the CAN layer */
 	close_candev(ndev);
 	return 0;
@@ -1413,7 +1527,6 @@ static int ican3_stop(struct net_device *ndev)
 static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 {
 	struct ican3_dev *mod = netdev_priv(ndev);
-	struct net_device_stats *stats = &ndev->stats;
 	struct can_frame *cf = (struct can_frame *)skb->data;
 	struct ican3_fast_desc desc;
 	void __iomem *desc_addr;
@@ -1426,8 +1539,7 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 
 	/* check that we can actually transmit */
 	if (!ican3_txok(mod)) {
-		dev_err(mod->dev, "no free descriptors, stopping queue\n");
-		netif_stop_queue(ndev);
+		dev_err(mod->dev, "BUG: no free descriptors\n");
 		spin_unlock_irqrestore(&mod->lock, flags);
 		return NETDEV_TX_BUSY;
 	}
@@ -1442,6 +1554,14 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 	can_frame_to_ican3(mod, cf, &desc);
 
 	/*
+	 * This hardware doesn't have TX-done notifications, so we'll try and
+	 * emulate it the best we can using ECHO skbs. Add the skb to the ECHO
+	 * stack. Upon packet reception, check if the ECHO skb and received
+	 * skb match, and use that to wake the queue.
+	 */
+	ican3_put_echo_skb(mod, skb);
+
+	/*
 	 * the programming manual says that you must set the IVALID bit, then
 	 * interrupt, then set the valid bit. Quite weird, but it seems to be
 	 * required for this to work
@@ -1459,19 +1579,7 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 	mod->fasttx_num = (desc.control & DESC_WRAP) ? 0
 						     : (mod->fasttx_num + 1);
 
-	/* 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
-	 * emulate it the best we can using ECHO skbs. Get the next TX
-	 * descriptor, and see if we have room to send. If not, stop the queue.
-	 * It will be woken when the ECHO skb for the current packet is recv'd.
-	 */
-
-	/* copy the control bits of the descriptor */
+	/* if there is no free descriptor space, stop the transmit queue */
 	if (!ican3_txok(mod))
 		netif_stop_queue(ndev);
 
@@ -1667,6 +1775,7 @@ static int __devinit ican3_probe(struct platform_device *pdev)
 	mod->dev = &pdev->dev;
 	mod->num = pdata->modno;
 	netif_napi_add(ndev, &mod->napi, ican3_napi, ICAN3_RX_BUFFERS);
+	skb_queue_head_init(&mod->echoq);
 	spin_lock_init(&mod->lock);
 	init_completion(&mod->termination_comp);
 	init_completion(&mod->buserror_comp);
-- 
1.7.8.6

[PATCH 5/6] can: janz-ican3: avoid firmware lockup caused by infinite bus error quota

[Ira W. Snyder]

From: "Ira W. Snyder" <iws@ovro.caltech.edu>

If the bus error quota is set to infinite and the host CPU cannot keep
up, the Janz VMOD-ICAN3 firmware will stop responding to control
messages until the controller is reset.

The firmware will automatically stop sending bus error messages when the
quota is reached, and will only resume sending bus error messages when
the quota is re-set to a positive value.

This limitation is worked around by setting the bus error quota to one
message, and then re-setting the quota to one message every time a bus
error message is received. By doing this, the firmware never stops
responding to control messages. The CAN bus can be reset without a
hard-reset of the controller card.

Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
 drivers/net/can/janz-ican3.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index 1304712..df3ba07 100644
--- a/drivers/net/can/janz-ican3.c
+++ b/drivers/net/can/janz-ican3.c
@@ -960,6 +960,7 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 	 * skb allocation when it will just be freed immediately.
 	 */
 	if (isrc == CEVTIND_BEI) {
+		int ret;
 		dev_dbg(mod->dev, "bus error interrupt, part 1\n");
 
 		/* TX error */
@@ -970,6 +971,16 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 			stats->rx_errors++;
 		}
 
+		/*
+		 * The controller automatically disables bus-error interrupts
+		 * and therefore we must re-enable them.
+		 */
+		ret = ican3_set_buserror(mod, 1);
+		if (ret) {
+			dev_err(mod->dev, "unable to re-enable bus-error\n");
+			return ret;
+		}
+
 		/* bus error reporting is off, return immediately */
 		if (!(mod->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
 			return 0;
@@ -1447,7 +1458,7 @@ static int __devinit ican3_startup_module(struct ican3_dev *mod)
 	}
 
 	/* default to "bus errors enabled" */
-	ret = ican3_set_buserror(mod, ICAN3_BUSERR_QUOTA_MAX);
+	ret = ican3_set_buserror(mod, 1);
 	if (ret) {
 		dev_err(mod->dev, "unable to set bus-error\n");
 		return ret;
-- 
1.7.8.6

[PATCH 6/6] can: janz-ican3: add support for one shot mode

[Ira W. Snyder]

From: "Ira W. Snyder" <iws@ovro.caltech.edu>

The Janz VMOD-ICAN3 hardware has support for one shot packet
transmission. This means that a packet will be attempted to be sent
once, with no automatic retries.

The SocketCAN core has a controller-wide setting for this mode:
CAN_CTRLMODE_ONE_SHOT. The Janz VMOD-ICAN3 hardware supports this flag
on a per-packet level, but the SocketCAN core does not.

Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
---
 drivers/net/can/janz-ican3.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index df3ba07..c271582 100644
--- a/drivers/net/can/janz-ican3.c
+++ b/drivers/net/can/janz-ican3.c
@@ -116,6 +116,7 @@
 #define ICAN3_BUSERR_QUOTA_MAX	255
 
 /* Janz ICAN3 CAN Frame Conversion */
+#define ICAN3_SNGL	0x02
 #define ICAN3_ECHO	0x10
 #define ICAN3_EFF_RTR	0x40
 #define ICAN3_SFF_RTR	0x10
@@ -848,6 +849,10 @@ static void can_frame_to_ican3(struct ican3_dev *mod,
 	desc->data[0] |= cf->can_dlc;
 	desc->data[1] |= ICAN3_ECHO;
 
+	/* support single transmission (no retries) mode */
+	if (mod->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
+		desc->data[1] |= ICAN3_SNGL;
+
 	if (cf->can_id & CAN_RTR_FLAG)
 		desc->data[0] |= ICAN3_EFF_RTR;
 
@@ -1807,7 +1812,8 @@ static int __devinit ican3_probe(struct platform_device *pdev)
 	mod->can.do_set_mode = ican3_set_mode;
 	mod->can.do_get_berr_counter = ican3_get_berr_counter;
 	mod->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES
-				    | CAN_CTRLMODE_BERR_REPORTING;
+				    | CAN_CTRLMODE_BERR_REPORTING
+				    | CAN_CTRLMODE_ONE_SHOT;
 
 	/* find our IRQ number */
 	mod->irq = platform_get_irq(pdev, 0);
-- 
1.7.8.6

Re: [PATCH 1/6] can: janz-ican3: remove dead code

[Marc Kleine-Budde]

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

On 07/19/2012 12:33 AM, Ira W. Snyder wrote:
> From: "Ira W. Snyder" <iws@ovro.caltech.edu>
> 
> The code which used this variable was removed during review, before the
> driver was added to mainline Linux. It is now dead code, and can be
> removed.
> 
> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>

Looks good.

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: 262 bytes --]

Re: [PATCH 2/6] can: janz-ican3: drop invalid skbs

[Marc Kleine-Budde]

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

On 07/19/2012 12:33 AM, Ira W. Snyder wrote:
> From: "Ira W. Snyder" <iws@ovro.caltech.edu>
> 
> The commit which added the janz-ican3 driver and commit
> 3ccd4c61 "can: Unify droping of invalid tx skbs and netdev stats" were
> committed into mainline Linux during the same merge window.
> 
> Therefore, the addition of this code to the janz-ican3 driver was
> forgotten. This patch adds the expected code.
> 
> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>

looks good.

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: 262 bytes --]

Re: [PATCH 3/6] can: janz-ican3: fix error and byte counters

[Marc Kleine-Budde]

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

On 07/19/2012 12:33 AM, Ira W. Snyder wrote:
> From: "Ira W. Snyder" <iws@ovro.caltech.edu>
> 
> The error and byte counter statistics were being incremented
> incorrectly. For example, a TX error would be counted both in tx_errors
> and rx_errors.
> 
> This corrects the problem so that tx_errors and rx_errors are only
> incremented for errors caused by packets sent to the bus. Error packets
> generated by the driver are not counted.
> 
> The byte counters are only increased for packets which are actually
> transmitted or received from the bus. Error packets generated by the
> driver are not counted.
> 
> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>

Looking at the patch (and not the complete driver) I have some
questions. See inline.

> ---
>  drivers/net/can/janz-ican3.c |   13 ++++++-------
>  1 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> index ea1919e..d9d4ed1 100644
> --- a/drivers/net/can/janz-ican3.c
> +++ b/drivers/net/can/janz-ican3.c
> @@ -907,8 +907,7 @@ static void ican3_handle_msglost(struct ican3_dev *mod, struct ican3_msg *msg)
>  	if (skb) {
>  		cf->can_id |= CAN_ERR_CRTL;
>  		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> -		stats->rx_errors++;
                ^^^^^^^^^^^^^^^^^^^
> -		stats->rx_bytes += cf->can_dlc;
> +		stats->rx_over_errors++;

I've checked the sja1000 and the mscan driver, on an rx overflow error
they increment both rx_errors and rx_over_errors.

>  		netif_rx(skb);
>  	}
>  }
> @@ -956,7 +955,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>  		cf->can_id |= CAN_ERR_CRTL;
>  		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
>  		stats->rx_over_errors++;
> -		stats->rx_errors++;

dito

>  	}
>  
>  	/* error warning + passive interrupt */
> @@ -982,7 +980,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>  
>  		dev_dbg(mod->dev, "bus error interrupt\n");
>  		mod->can.can_stats.bus_error++;
> -		stats->rx_errors++;

this rx_errors probably moved to the one below ...
>  		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>  
>  		switch (ecc & ECC_MASK) {
> @@ -1001,8 +998,12 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>  			break;
>  		}
>  
> -		if ((ecc & ECC_DIR) == 0)
> +		if (!(ecc & ECC_DIR)) {
>  			cf->data[2] |= CAN_ERR_PROT_TX;
> +			stats->tx_errors++;
> +		} else {
> +			stats->rx_errors++;

...here. right?

> +		}
>  
>  		cf->data[6] = txerr;
>  		cf->data[7] = rxerr;
> @@ -1028,8 +1029,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>  	}
>  
>  	mod->can.state = state;
> -	stats->rx_errors++;
> -	stats->rx_bytes += cf->can_dlc;
>  	netif_rx(skb);
>  	return 0;
>  }

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: 262 bytes --]

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

[Marc Kleine-Budde]

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

On 07/19/2012 12:33 AM, Ira W. Snyder wrote:
> From: "Ira W. Snyder" <iws@ovro.caltech.edu>
> 
> 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 ican3_cmp_echo_skb(), we can drop the loopback
> messages and return the original skbs. This fixes the issues with
> CAN_RAW_RECV_OWN_MSGS.
> 
> A private skb queue is used to store the echo skbs. This avoids the need
> for any index management.
> 
> Due to a lack of TX-error interrupts, bus errors are permanently
> enabled, and are used as a TX-error notification. This is used to drop
> an echo skb when transmission fails. Bus error packets are not generated
> if the user has not enabled bus error reporting.
> 
> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>

Nitpicks inline, otherwise looks good.

Marc

> ---
>  drivers/net/can/janz-ican3.c |  199 ++++++++++++++++++++++++++++++++----------
>  1 files changed, 154 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> index d9d4ed1..1304712 100644
> --- a/drivers/net/can/janz-ican3.c
> +++ b/drivers/net/can/janz-ican3.c
> @@ -220,6 +220,9 @@ struct ican3_dev {
>  	/* old and new style host interface */
>  	unsigned int iftype;
>  
> +	/* queue for echo packets */
> +	struct sk_buff_head echoq;
> +
>  	/*
>  	 * Any function which changes the current DPM page must hold this
>  	 * lock while it is performing data accesses. This ensures that the
> @@ -924,7 +927,7 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>  	struct net_device *dev = mod->ndev;
>  	struct net_device_stats *stats = &dev->stats;
>  	enum can_state state = mod->can.state;
> -	u8 status, isrc, rxerr, txerr;
> +	u8 isrc, ecc, status, rxerr, txerr;
>  	struct can_frame *cf;
>  	struct sk_buff *skb;
>  
> @@ -940,15 +943,42 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>  		return -EINVAL;
>  	}
>  
> -	skb = alloc_can_err_skb(dev, &cf);
> -	if (skb == NULL)
> -		return -ENOMEM;
> -
>  	isrc = msg->data[0];
> +	ecc = msg->data[2];
>  	status = msg->data[3];
>  	rxerr = msg->data[4];
>  	txerr = msg->data[5];
>  
> +	/*
> +	 * This hardware lacks any support other than bus error messages to
> +	 * determine if packet transmission has failed.
> +	 *
> +	 * When TX errors happen, one echo skb needs to be dropped from the
> +	 * front of the queue.
> +	 *
> +	 * A small bit of code is duplicated here and below, to avoid error
> +	 * skb allocation when it will just be freed immediately.
> +	 */
> +	if (isrc == CEVTIND_BEI) {
> +		dev_dbg(mod->dev, "bus error interrupt, part 1\n");

nitpick: I would drop the ", part 1".

> +
> +		/* TX error */
> +		if (!(ecc & ECC_DIR)) {
> +			kfree_skb(skb_dequeue(&mod->echoq));
> +			stats->tx_errors++;
> +		} else {
> +			stats->rx_errors++;
> +		}
> +
> +		/* bus error reporting is off, return immediately */
> +		if (!(mod->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
> +			return 0;
> +	}
> +
> +	skb = alloc_can_err_skb(dev, &cf);
> +	if (skb == NULL)
> +		return -ENOMEM;
> +
>  	/* data overrun interrupt */
>  	if (isrc == CEVTIND_DOI || isrc == CEVTIND_LOST) {
>  		dev_dbg(mod->dev, "data overrun interrupt\n");
> @@ -976,9 +1006,8 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>  
>  	/* bus error interrupt */
>  	if (isrc == CEVTIND_BEI) {
> -		u8 ecc = msg->data[2];
> +		dev_dbg(mod->dev, "bus error interrupt, part 2\n");
>  
> -		dev_dbg(mod->dev, "bus error interrupt\n");

I would remove the debug message here completely.

>  		mod->can.can_stats.bus_error++;
>  		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>  
> @@ -998,12 +1027,8 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>  			break;
>  		}
>  
> -		if (!(ecc & ECC_DIR)) {
> +		if (!(ecc & ECC_DIR))
>  			cf->data[2] |= CAN_ERR_PROT_TX;
> -			stats->tx_errors++;
> -		} else {
> -			stats->rx_errors++;
> -		}
>  
>  		cf->data[6] = txerr;
>  		cf->data[7] = rxerr;
> @@ -1088,6 +1113,85 @@ static void ican3_handle_message(struct ican3_dev *mod, struct ican3_msg *msg)
>  }
>  
>  /*
> + * The ican3 needs to store all echo skbs, and therefore cannot
> + * use the generic infrastructure for this.
> + */
> +static void ican3_put_echo_skb(struct ican3_dev *mod, struct sk_buff *skb)
> +{
> +	struct sock *srcsk = skb->sk;
> +
> +	if (atomic_read(&skb->users) != 1) {
> +		struct sk_buff *old_skb = skb;
> +
> +		skb = skb_clone(old_skb, GFP_ATOMIC);
> +		kfree_skb(old_skb);
> +		if (!skb)
> +			return;
> +	} else {
> +		skb_orphan(skb);
> +	}
> +
> +	skb->sk = srcsk;
> +
> +	/* save this skb for tx interrupt echo handling */
> +	skb_queue_tail(&mod->echoq, skb);
> +}
> +
> +static unsigned int ican3_get_echo_skb(struct ican3_dev *mod)
> +{
> +	struct sk_buff *skb = skb_dequeue(&mod->echoq);
> +	struct can_frame *cf;
> +	u8 dlc;
> +
> +	if (!skb)
> +		return 0;

Have you tested if this happens? It should now. Maybe add a netdev_err
or a pr_err_once here?

> +
> +	cf = (struct can_frame *)skb->data;
> +	dlc = cf->can_dlc;
> +
> +	/* check flag whether this packet has to be looped back */
> +	if (skb->pkt_type != PACKET_LOOPBACK) {
> +		kfree_skb(skb);
> +		return dlc;
> +	}
> +
> +	skb->protocol = htons(ETH_P_CAN);
> +	skb->pkt_type = PACKET_BROADCAST;
> +	skb->ip_summed = CHECKSUM_UNNECESSARY;
> +	skb->dev = mod->ndev;
> +	netif_receive_skb(skb);
> +	return dlc;
> +}
> +
> +/*
> + * 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.
> + */
> +static bool ican3_echo_skb_matches(struct ican3_dev *mod, struct sk_buff *skb)
> +{
> +	struct can_frame *cf = (struct can_frame *)skb->data;
> +	struct sk_buff *echo_skb = skb_peek(&mod->echoq);
> +	struct can_frame *echo_cf;
> +
> +	if (!echo_skb)
> +		return false;
> +
> +	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;
> +}
> +
> +/*
>   * Check that there is room in the TX ring to transmit another skb
>   *
>   * LOCKING: must hold mod->lock
> @@ -1097,6 +1201,10 @@ static bool ican3_txok(struct ican3_dev *mod)
>  	struct ican3_fast_desc __iomem *desc;
>  	u8 control;
>  
> +	/* check that we have echo queue space */
> +	if (skb_queue_len(&mod->echoq) >= ICAN3_TX_BUFFERS)
> +		return false;
> +
>  	/* copy the control bits of the descriptor */
>  	ican3_set_page(mod, mod->fasttx_start + (mod->fasttx_num / 16));
>  	desc = mod->dpm + ((mod->fasttx_num % 16) * sizeof(*desc));
> @@ -1147,10 +1255,27 @@ static int ican3_recv_skb(struct ican3_dev *mod)
>  	/* convert the ICAN3 frame into Linux CAN format */
>  	ican3_to_can_frame(mod, &desc, cf);
>  
> -	/* receive the skb, update statistics */
> -	netif_receive_skb(skb);
> +	/*
> +	 * If this is an ECHO frame received from the hardware loopback
> +	 * feature, use the skb saved in the ECHO stack instead. This allows
> +	 * the Linux CAN core to support CAN_RAW_RECV_OWN_MSGS correctly.
> +	 *
> +	 * Since this is a confirmation of a successfully transmitted packet
> +	 * sent from this host, update the transmit statistics.
> +	 *
> +	 * Also, the netdevice queue needs to be allowed to send packets again.
> +	 */
> +	if (ican3_echo_skb_matches(mod, skb)) {
> +		stats->tx_packets++;
> +		stats->tx_bytes += ican3_get_echo_skb(mod);
> +		kfree_skb(skb);
> +		goto err_noalloc;
> +	}
> +
> +	/* update statistics, receive the skb */
>  	stats->rx_packets++;
>  	stats->rx_bytes += cf->can_dlc;
> +	netif_receive_skb(skb);
>  
>  err_noalloc:
>  	/* toggle the valid bit and return the descriptor to the ring */
> @@ -1173,13 +1298,13 @@ err_noalloc:
>  static int ican3_napi(struct napi_struct *napi, int budget)
>  {
>  	struct ican3_dev *mod = container_of(napi, struct ican3_dev, napi);
> -	struct ican3_msg msg;
>  	unsigned long flags;
>  	int received = 0;
>  	int ret;
>  
>  	/* process all communication messages */
>  	while (true) {
> +		struct ican3_msg msg;
>  		ret = ican3_recv_msg(mod, &msg);
>  		if (ret)
>  			break;
> @@ -1351,7 +1476,6 @@ static int __devinit ican3_startup_module(struct ican3_dev *mod)
>  static int ican3_open(struct net_device *ndev)
>  {
>  	struct ican3_dev *mod = netdev_priv(ndev);
> -	u8 quota;
>  	int ret;
>  
>  	/* open the CAN layer */
> @@ -1361,19 +1485,6 @@ static int ican3_open(struct net_device *ndev)
>  		return ret;
>  	}
>  
> -	/* set the bus error generation state appropriately */
> -	if (mod->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
> -		quota = ICAN3_BUSERR_QUOTA_MAX;
> -	else
> -		quota = 0;
> -
> -	ret = ican3_set_buserror(mod, quota);
> -	if (ret) {
> -		dev_err(mod->dev, "unable to set bus-error\n");
> -		close_candev(ndev);
> -		return ret;
> -	}
> -
>  	/* bring the bus online */
>  	ret = ican3_set_bus_state(mod, true);
>  	if (ret) {
> @@ -1405,6 +1516,9 @@ static int ican3_stop(struct net_device *ndev)
>  		return ret;
>  	}
>  
> +	/* drop all outstanding echo skbs */
> +	skb_queue_purge(&mod->echoq);
> +
>  	/* close the CAN layer */
>  	close_candev(ndev);
>  	return 0;
> @@ -1413,7 +1527,6 @@ static int ican3_stop(struct net_device *ndev)
>  static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
>  {
>  	struct ican3_dev *mod = netdev_priv(ndev);
> -	struct net_device_stats *stats = &ndev->stats;
>  	struct can_frame *cf = (struct can_frame *)skb->data;
>  	struct ican3_fast_desc desc;
>  	void __iomem *desc_addr;
> @@ -1426,8 +1539,7 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
>  
>  	/* check that we can actually transmit */
>  	if (!ican3_txok(mod)) {
> -		dev_err(mod->dev, "no free descriptors, stopping queue\n");
> -		netif_stop_queue(ndev);
> +		dev_err(mod->dev, "BUG: no free descriptors\n");
>  		spin_unlock_irqrestore(&mod->lock, flags);
>  		return NETDEV_TX_BUSY;
>  	}
> @@ -1442,6 +1554,14 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	can_frame_to_ican3(mod, cf, &desc);
>  
>  	/*
> +	 * This hardware doesn't have TX-done notifications, so we'll try and
> +	 * emulate it the best we can using ECHO skbs. Add the skb to the ECHO
> +	 * stack. Upon packet reception, check if the ECHO skb and received
> +	 * skb match, and use that to wake the queue.
> +	 */
> +	ican3_put_echo_skb(mod, skb);
> +
> +	/*
>  	 * the programming manual says that you must set the IVALID bit, then
>  	 * interrupt, then set the valid bit. Quite weird, but it seems to be
>  	 * required for this to work
> @@ -1459,19 +1579,7 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	mod->fasttx_num = (desc.control & DESC_WRAP) ? 0
>  						     : (mod->fasttx_num + 1);
>  
> -	/* 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
> -	 * emulate it the best we can using ECHO skbs. Get the next TX
> -	 * descriptor, and see if we have room to send. If not, stop the queue.
> -	 * It will be woken when the ECHO skb for the current packet is recv'd.
> -	 */
> -
> -	/* copy the control bits of the descriptor */
> +	/* if there is no free descriptor space, stop the transmit queue */
>  	if (!ican3_txok(mod))
>  		netif_stop_queue(ndev);
>  
> @@ -1667,6 +1775,7 @@ static int __devinit ican3_probe(struct platform_device *pdev)
>  	mod->dev = &pdev->dev;
>  	mod->num = pdata->modno;
>  	netif_napi_add(ndev, &mod->napi, ican3_napi, ICAN3_RX_BUFFERS);
> +	skb_queue_head_init(&mod->echoq);
>  	spin_lock_init(&mod->lock);
>  	init_completion(&mod->termination_comp);
>  	init_completion(&mod->buserror_comp);


-- 
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: 262 bytes --]

Re: [PATCH 5/6] can: janz-ican3: avoid firmware lockup caused by infinite bus error quota

[Marc Kleine-Budde]

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

On 07/19/2012 12:33 AM, Ira W. Snyder wrote:
> From: "Ira W. Snyder" <iws@ovro.caltech.edu>
> 
> If the bus error quota is set to infinite and the host CPU cannot keep
> up, the Janz VMOD-ICAN3 firmware will stop responding to control
> messages until the controller is reset.
> 
> The firmware will automatically stop sending bus error messages when the
> quota is reached, and will only resume sending bus error messages when
> the quota is re-set to a positive value.
> 
> This limitation is worked around by setting the bus error quota to one
> message, and then re-setting the quota to one message every time a bus
> error message is received. By doing this, the firmware never stops
> responding to control messages. The CAN bus can be reset without a
> hard-reset of the controller card.
> 
> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>

looks good.

Marc
> ---
>  drivers/net/can/janz-ican3.c |   13 ++++++++++++-
>  1 files changed, 12 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> index 1304712..df3ba07 100644
> --- a/drivers/net/can/janz-ican3.c
> +++ b/drivers/net/can/janz-ican3.c
> @@ -960,6 +960,7 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>  	 * skb allocation when it will just be freed immediately.
>  	 */
>  	if (isrc == CEVTIND_BEI) {
> +		int ret;
>  		dev_dbg(mod->dev, "bus error interrupt, part 1\n");
>  
>  		/* TX error */
> @@ -970,6 +971,16 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>  			stats->rx_errors++;
>  		}
>  
> +		/*
> +		 * The controller automatically disables bus-error interrupts
> +		 * and therefore we must re-enable them.
> +		 */
> +		ret = ican3_set_buserror(mod, 1);
> +		if (ret) {
> +			dev_err(mod->dev, "unable to re-enable bus-error\n");
> +			return ret;
> +		}
> +
>  		/* bus error reporting is off, return immediately */
>  		if (!(mod->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
>  			return 0;
> @@ -1447,7 +1458,7 @@ static int __devinit ican3_startup_module(struct ican3_dev *mod)
>  	}
>  
>  	/* default to "bus errors enabled" */
> -	ret = ican3_set_buserror(mod, ICAN3_BUSERR_QUOTA_MAX);
> +	ret = ican3_set_buserror(mod, 1);
>  	if (ret) {
>  		dev_err(mod->dev, "unable to set bus-error\n");
>  		return ret;


-- 
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: 262 bytes --]

Re: [PATCH 6/6] can: janz-ican3: add support for one shot mode

[Marc Kleine-Budde]

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

On 07/19/2012 12:33 AM, Ira W. Snyder wrote:
> From: "Ira W. Snyder" <iws@ovro.caltech.edu>
> 
> The Janz VMOD-ICAN3 hardware has support for one shot packet
> transmission. This means that a packet will be attempted to be sent
> once, with no automatic retries.
> 
> The SocketCAN core has a controller-wide setting for this mode:
> CAN_CTRLMODE_ONE_SHOT. The Janz VMOD-ICAN3 hardware supports this flag
> on a per-packet level, but the SocketCAN core does not.
> 
> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>

looks good

Marc

> ---
>  drivers/net/can/janz-ican3.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> index df3ba07..c271582 100644
> --- a/drivers/net/can/janz-ican3.c
> +++ b/drivers/net/can/janz-ican3.c
> @@ -116,6 +116,7 @@
>  #define ICAN3_BUSERR_QUOTA_MAX	255
>  
>  /* Janz ICAN3 CAN Frame Conversion */
> +#define ICAN3_SNGL	0x02
>  #define ICAN3_ECHO	0x10
>  #define ICAN3_EFF_RTR	0x40
>  #define ICAN3_SFF_RTR	0x10
> @@ -848,6 +849,10 @@ static void can_frame_to_ican3(struct ican3_dev *mod,
>  	desc->data[0] |= cf->can_dlc;
>  	desc->data[1] |= ICAN3_ECHO;
>  
> +	/* support single transmission (no retries) mode */
> +	if (mod->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT)
> +		desc->data[1] |= ICAN3_SNGL;
> +
>  	if (cf->can_id & CAN_RTR_FLAG)
>  		desc->data[0] |= ICAN3_EFF_RTR;
>  
> @@ -1807,7 +1812,8 @@ static int __devinit ican3_probe(struct platform_device *pdev)
>  	mod->can.do_set_mode = ican3_set_mode;
>  	mod->can.do_get_berr_counter = ican3_get_berr_counter;
>  	mod->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES
> -				    | CAN_CTRLMODE_BERR_REPORTING;
> +				    | CAN_CTRLMODE_BERR_REPORTING
> +				    | CAN_CTRLMODE_ONE_SHOT;
>  
>  	/* find our IRQ number */
>  	mod->irq = platform_get_irq(pdev, 0);


-- 
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: 262 bytes --]

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

[Ira W. Snyder]

On Thu, Jul 19, 2012 at 10:33:47AM +0200, Marc Kleine-Budde wrote:
> On 07/19/2012 12:33 AM, Ira W. Snyder wrote:
> > From: "Ira W. Snyder" <iws@ovro.caltech.edu>
> > 
> > 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 ican3_cmp_echo_skb(), we can drop the loopback
> > messages and return the original skbs. This fixes the issues with
> > CAN_RAW_RECV_OWN_MSGS.
> > 
> > A private skb queue is used to store the echo skbs. This avoids the need
> > for any index management.
> > 
> > Due to a lack of TX-error interrupts, bus errors are permanently
> > enabled, and are used as a TX-error notification. This is used to drop
> > an echo skb when transmission fails. Bus error packets are not generated
> > if the user has not enabled bus error reporting.
> > 
> > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> 
> Nitpicks inline, otherwise looks good.
> 
> Marc
> 

Thanks for the review. Another version of this patch with the nitpicks
fixed will be posted in reply.

> > ---
> >  drivers/net/can/janz-ican3.c |  199 ++++++++++++++++++++++++++++++++----------
> >  1 files changed, 154 insertions(+), 45 deletions(-)
> > 
> > diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> > index d9d4ed1..1304712 100644
> > --- a/drivers/net/can/janz-ican3.c
> > +++ b/drivers/net/can/janz-ican3.c
> > @@ -220,6 +220,9 @@ struct ican3_dev {
> >  	/* old and new style host interface */
> >  	unsigned int iftype;
> >  
> > +	/* queue for echo packets */
> > +	struct sk_buff_head echoq;
> > +
> >  	/*
> >  	 * Any function which changes the current DPM page must hold this
> >  	 * lock while it is performing data accesses. This ensures that the
> > @@ -924,7 +927,7 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
> >  	struct net_device *dev = mod->ndev;
> >  	struct net_device_stats *stats = &dev->stats;
> >  	enum can_state state = mod->can.state;
> > -	u8 status, isrc, rxerr, txerr;
> > +	u8 isrc, ecc, status, rxerr, txerr;
> >  	struct can_frame *cf;
> >  	struct sk_buff *skb;
> >  
> > @@ -940,15 +943,42 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
> >  		return -EINVAL;
> >  	}
> >  
> > -	skb = alloc_can_err_skb(dev, &cf);
> > -	if (skb == NULL)
> > -		return -ENOMEM;
> > -
> >  	isrc = msg->data[0];
> > +	ecc = msg->data[2];
> >  	status = msg->data[3];
> >  	rxerr = msg->data[4];
> >  	txerr = msg->data[5];
> >  
> > +	/*
> > +	 * This hardware lacks any support other than bus error messages to
> > +	 * determine if packet transmission has failed.
> > +	 *
> > +	 * When TX errors happen, one echo skb needs to be dropped from the
> > +	 * front of the queue.
> > +	 *
> > +	 * A small bit of code is duplicated here and below, to avoid error
> > +	 * skb allocation when it will just be freed immediately.
> > +	 */
> > +	if (isrc == CEVTIND_BEI) {
> > +		dev_dbg(mod->dev, "bus error interrupt, part 1\n");
> 
> nitpick: I would drop the ", part 1".
> 

Agreed.

> > +
> > +		/* TX error */
> > +		if (!(ecc & ECC_DIR)) {
> > +			kfree_skb(skb_dequeue(&mod->echoq));
> > +			stats->tx_errors++;
> > +		} else {
> > +			stats->rx_errors++;
> > +		}
> > +
> > +		/* bus error reporting is off, return immediately */
> > +		if (!(mod->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
> > +			return 0;
> > +	}
> > +
> > +	skb = alloc_can_err_skb(dev, &cf);
> > +	if (skb == NULL)
> > +		return -ENOMEM;
> > +
> >  	/* data overrun interrupt */
> >  	if (isrc == CEVTIND_DOI || isrc == CEVTIND_LOST) {
> >  		dev_dbg(mod->dev, "data overrun interrupt\n");
> > @@ -976,9 +1006,8 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
> >  
> >  	/* bus error interrupt */
> >  	if (isrc == CEVTIND_BEI) {
> > -		u8 ecc = msg->data[2];
> > +		dev_dbg(mod->dev, "bus error interrupt, part 2\n");
> >  
> > -		dev_dbg(mod->dev, "bus error interrupt\n");
> 
> I would remove the debug message here completely.
> 

Agreed.

> >  		mod->can.can_stats.bus_error++;
> >  		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> >  
> > @@ -998,12 +1027,8 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
> >  			break;
> >  		}
> >  
> > -		if (!(ecc & ECC_DIR)) {
> > +		if (!(ecc & ECC_DIR))
> >  			cf->data[2] |= CAN_ERR_PROT_TX;
> > -			stats->tx_errors++;
> > -		} else {
> > -			stats->rx_errors++;
> > -		}
> >  
> >  		cf->data[6] = txerr;
> >  		cf->data[7] = rxerr;
> > @@ -1088,6 +1113,85 @@ static void ican3_handle_message(struct ican3_dev *mod, struct ican3_msg *msg)
> >  }
> >  
> >  /*
> > + * The ican3 needs to store all echo skbs, and therefore cannot
> > + * use the generic infrastructure for this.
> > + */
> > +static void ican3_put_echo_skb(struct ican3_dev *mod, struct sk_buff *skb)
> > +{
> > +	struct sock *srcsk = skb->sk;
> > +
> > +	if (atomic_read(&skb->users) != 1) {
> > +		struct sk_buff *old_skb = skb;
> > +
> > +		skb = skb_clone(old_skb, GFP_ATOMIC);
> > +		kfree_skb(old_skb);
> > +		if (!skb)
> > +			return;
> > +	} else {
> > +		skb_orphan(skb);
> > +	}
> > +
> > +	skb->sk = srcsk;
> > +
> > +	/* save this skb for tx interrupt echo handling */
> > +	skb_queue_tail(&mod->echoq, skb);
> > +}
> > +
> > +static unsigned int ican3_get_echo_skb(struct ican3_dev *mod)
> > +{
> > +	struct sk_buff *skb = skb_dequeue(&mod->echoq);
> > +	struct can_frame *cf;
> > +	u8 dlc;
> > +
> > +	if (!skb)
> > +		return 0;
> 
> Have you tested if this happens? It should now. Maybe add a netdev_err
> or a pr_err_once here?
> 

It doesn't happen. I've added a netdev_err().

> > +
> > +	cf = (struct can_frame *)skb->data;
> > +	dlc = cf->can_dlc;
> > +
> > +	/* check flag whether this packet has to be looped back */
> > +	if (skb->pkt_type != PACKET_LOOPBACK) {
> > +		kfree_skb(skb);
> > +		return dlc;
> > +	}
> > +
> > +	skb->protocol = htons(ETH_P_CAN);
> > +	skb->pkt_type = PACKET_BROADCAST;
> > +	skb->ip_summed = CHECKSUM_UNNECESSARY;
> > +	skb->dev = mod->ndev;
> > +	netif_receive_skb(skb);
> > +	return dlc;
> > +}
> > +
> > +/*
> > + * 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.
> > + */
> > +static bool ican3_echo_skb_matches(struct ican3_dev *mod, struct sk_buff *skb)
> > +{
> > +	struct can_frame *cf = (struct can_frame *)skb->data;
> > +	struct sk_buff *echo_skb = skb_peek(&mod->echoq);
> > +	struct can_frame *echo_cf;
> > +
> > +	if (!echo_skb)
> > +		return false;
> > +
> > +	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;
> > +}
> > +
> > +/*
> >   * Check that there is room in the TX ring to transmit another skb
> >   *
> >   * LOCKING: must hold mod->lock
> > @@ -1097,6 +1201,10 @@ static bool ican3_txok(struct ican3_dev *mod)
> >  	struct ican3_fast_desc __iomem *desc;
> >  	u8 control;
> >  
> > +	/* check that we have echo queue space */
> > +	if (skb_queue_len(&mod->echoq) >= ICAN3_TX_BUFFERS)
> > +		return false;
> > +
> >  	/* copy the control bits of the descriptor */
> >  	ican3_set_page(mod, mod->fasttx_start + (mod->fasttx_num / 16));
> >  	desc = mod->dpm + ((mod->fasttx_num % 16) * sizeof(*desc));
> > @@ -1147,10 +1255,27 @@ static int ican3_recv_skb(struct ican3_dev *mod)
> >  	/* convert the ICAN3 frame into Linux CAN format */
> >  	ican3_to_can_frame(mod, &desc, cf);
> >  
> > -	/* receive the skb, update statistics */
> > -	netif_receive_skb(skb);
> > +	/*
> > +	 * If this is an ECHO frame received from the hardware loopback
> > +	 * feature, use the skb saved in the ECHO stack instead. This allows
> > +	 * the Linux CAN core to support CAN_RAW_RECV_OWN_MSGS correctly.
> > +	 *
> > +	 * Since this is a confirmation of a successfully transmitted packet
> > +	 * sent from this host, update the transmit statistics.
> > +	 *
> > +	 * Also, the netdevice queue needs to be allowed to send packets again.
> > +	 */
> > +	if (ican3_echo_skb_matches(mod, skb)) {
> > +		stats->tx_packets++;
> > +		stats->tx_bytes += ican3_get_echo_skb(mod);
> > +		kfree_skb(skb);
> > +		goto err_noalloc;
> > +	}
> > +
> > +	/* update statistics, receive the skb */
> >  	stats->rx_packets++;
> >  	stats->rx_bytes += cf->can_dlc;
> > +	netif_receive_skb(skb);
> >  
> >  err_noalloc:
> >  	/* toggle the valid bit and return the descriptor to the ring */
> > @@ -1173,13 +1298,13 @@ err_noalloc:
> >  static int ican3_napi(struct napi_struct *napi, int budget)
> >  {
> >  	struct ican3_dev *mod = container_of(napi, struct ican3_dev, napi);
> > -	struct ican3_msg msg;
> >  	unsigned long flags;
> >  	int received = 0;
> >  	int ret;
> >  
> >  	/* process all communication messages */
> >  	while (true) {
> > +		struct ican3_msg msg;
> >  		ret = ican3_recv_msg(mod, &msg);
> >  		if (ret)
> >  			break;
> > @@ -1351,7 +1476,6 @@ static int __devinit ican3_startup_module(struct ican3_dev *mod)
> >  static int ican3_open(struct net_device *ndev)
> >  {
> >  	struct ican3_dev *mod = netdev_priv(ndev);
> > -	u8 quota;
> >  	int ret;
> >  
> >  	/* open the CAN layer */
> > @@ -1361,19 +1485,6 @@ static int ican3_open(struct net_device *ndev)
> >  		return ret;
> >  	}
> >  
> > -	/* set the bus error generation state appropriately */
> > -	if (mod->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
> > -		quota = ICAN3_BUSERR_QUOTA_MAX;
> > -	else
> > -		quota = 0;
> > -
> > -	ret = ican3_set_buserror(mod, quota);
> > -	if (ret) {
> > -		dev_err(mod->dev, "unable to set bus-error\n");
> > -		close_candev(ndev);
> > -		return ret;
> > -	}
> > -
> >  	/* bring the bus online */
> >  	ret = ican3_set_bus_state(mod, true);
> >  	if (ret) {
> > @@ -1405,6 +1516,9 @@ static int ican3_stop(struct net_device *ndev)
> >  		return ret;
> >  	}
> >  
> > +	/* drop all outstanding echo skbs */
> > +	skb_queue_purge(&mod->echoq);
> > +
> >  	/* close the CAN layer */
> >  	close_candev(ndev);
> >  	return 0;
> > @@ -1413,7 +1527,6 @@ static int ican3_stop(struct net_device *ndev)
> >  static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
> >  {
> >  	struct ican3_dev *mod = netdev_priv(ndev);
> > -	struct net_device_stats *stats = &ndev->stats;
> >  	struct can_frame *cf = (struct can_frame *)skb->data;
> >  	struct ican3_fast_desc desc;
> >  	void __iomem *desc_addr;
> > @@ -1426,8 +1539,7 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
> >  
> >  	/* check that we can actually transmit */
> >  	if (!ican3_txok(mod)) {
> > -		dev_err(mod->dev, "no free descriptors, stopping queue\n");
> > -		netif_stop_queue(ndev);
> > +		dev_err(mod->dev, "BUG: no free descriptors\n");
> >  		spin_unlock_irqrestore(&mod->lock, flags);
> >  		return NETDEV_TX_BUSY;
> >  	}
> > @@ -1442,6 +1554,14 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
> >  	can_frame_to_ican3(mod, cf, &desc);
> >  
> >  	/*
> > +	 * This hardware doesn't have TX-done notifications, so we'll try and
> > +	 * emulate it the best we can using ECHO skbs. Add the skb to the ECHO
> > +	 * stack. Upon packet reception, check if the ECHO skb and received
> > +	 * skb match, and use that to wake the queue.
> > +	 */
> > +	ican3_put_echo_skb(mod, skb);
> > +
> > +	/*
> >  	 * the programming manual says that you must set the IVALID bit, then
> >  	 * interrupt, then set the valid bit. Quite weird, but it seems to be
> >  	 * required for this to work
> > @@ -1459,19 +1579,7 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
> >  	mod->fasttx_num = (desc.control & DESC_WRAP) ? 0
> >  						     : (mod->fasttx_num + 1);
> >  
> > -	/* 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
> > -	 * emulate it the best we can using ECHO skbs. Get the next TX
> > -	 * descriptor, and see if we have room to send. If not, stop the queue.
> > -	 * It will be woken when the ECHO skb for the current packet is recv'd.
> > -	 */
> > -
> > -	/* copy the control bits of the descriptor */
> > +	/* if there is no free descriptor space, stop the transmit queue */
> >  	if (!ican3_txok(mod))
> >  		netif_stop_queue(ndev);
> >  
> > @@ -1667,6 +1775,7 @@ static int __devinit ican3_probe(struct platform_device *pdev)
> >  	mod->dev = &pdev->dev;
> >  	mod->num = pdata->modno;
> >  	netif_napi_add(ndev, &mod->napi, ican3_napi, ICAN3_RX_BUFFERS);
> > +	skb_queue_head_init(&mod->echoq);
> >  	spin_lock_init(&mod->lock);
> >  	init_completion(&mod->termination_comp);
> >  	init_completion(&mod->buserror_comp);
> 
> 
> -- 
> 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   |
> 

Re: [PATCH 3/6] can: janz-ican3: fix error and byte counters

[Ira W. Snyder]

On Thu, Jul 19, 2012 at 10:11:38AM +0200, Marc Kleine-Budde wrote:
> On 07/19/2012 12:33 AM, Ira W. Snyder wrote:
> > From: "Ira W. Snyder" <iws@ovro.caltech.edu>
> > 
> > The error and byte counter statistics were being incremented
> > incorrectly. For example, a TX error would be counted both in tx_errors
> > and rx_errors.
> > 
> > This corrects the problem so that tx_errors and rx_errors are only
> > incremented for errors caused by packets sent to the bus. Error packets
> > generated by the driver are not counted.
> > 
> > The byte counters are only increased for packets which are actually
> > transmitted or received from the bus. Error packets generated by the
> > driver are not counted.
> > 
> > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> 
> Looking at the patch (and not the complete driver) I have some
> questions. See inline.
> 

Thanks for the review. A new version of the patch which has addressed
these comments will be posted in reply.

Ira

> > ---
> >  drivers/net/can/janz-ican3.c |   13 ++++++-------
> >  1 files changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> > index ea1919e..d9d4ed1 100644
> > --- a/drivers/net/can/janz-ican3.c
> > +++ b/drivers/net/can/janz-ican3.c
> > @@ -907,8 +907,7 @@ static void ican3_handle_msglost(struct ican3_dev *mod, struct ican3_msg *msg)
> >  	if (skb) {
> >  		cf->can_id |= CAN_ERR_CRTL;
> >  		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> > -		stats->rx_errors++;
>                 ^^^^^^^^^^^^^^^^^^^
> > -		stats->rx_bytes += cf->can_dlc;
> > +		stats->rx_over_errors++;
> 
> I've checked the sja1000 and the mscan driver, on an rx overflow error
> they increment both rx_errors and rx_over_errors.
> 

You're right, that was my mistake. I added the rx_errors++ back.

> >  		netif_rx(skb);
> >  	}
> >  }
> > @@ -956,7 +955,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
> >  		cf->can_id |= CAN_ERR_CRTL;
> >  		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> >  		stats->rx_over_errors++;
> > -		stats->rx_errors++;
> 
> dito
> 

ditto :)

> >  	}
> >  
> >  	/* error warning + passive interrupt */
> > @@ -982,7 +980,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
> >  
> >  		dev_dbg(mod->dev, "bus error interrupt\n");
> >  		mod->can.can_stats.bus_error++;
> > -		stats->rx_errors++;
> 
> this rx_errors probably moved to the one below ...
> >  		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> >  
> >  		switch (ecc & ECC_MASK) {
> > @@ -1001,8 +998,12 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
> >  			break;
> >  		}
> >  
> > -		if ((ecc & ECC_DIR) == 0)
> > +		if (!(ecc & ECC_DIR)) {
> >  			cf->data[2] |= CAN_ERR_PROT_TX;
> > +			stats->tx_errors++;
> > +		} else {
> > +			stats->rx_errors++;
> 
> ...here. right?
> 

Yep, it moved.

> > +		}
> >  
> >  		cf->data[6] = txerr;
> >  		cf->data[7] = rxerr;
> > @@ -1028,8 +1029,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
> >  	}
> >  
> >  	mod->can.state = state;
> > -	stats->rx_errors++;
> > -	stats->rx_bytes += cf->can_dlc;
> >  	netif_rx(skb);
> >  	return 0;
> >  }
> 
> 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   |
> 

[PATCH 3/6] can: janz-ican3: fix error and byte counters

[Ira W. Snyder]

From: "Ira W. Snyder" <iws@ovro.caltech.edu>

The error and byte counter statistics were being incremented
incorrectly. For example, a TX error would be counted both in tx_errors
and rx_errors.

This corrects the problem so that tx_errors and rx_errors are only
incremented for errors caused by packets sent to the bus. Error packets
generated by the driver are not counted.

The byte counters are only increased for packets which are actually
transmitted or received from the bus. Error packets generated by the
driver are not counted.

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

Changes for this version:
- for RX overflow errors, both rx_over_errors and rx_errors are incremented

 drivers/net/can/janz-ican3.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index ea1919e..14d5abc 100644
--- a/drivers/net/can/janz-ican3.c
+++ b/drivers/net/can/janz-ican3.c
@@ -907,8 +907,8 @@ static void ican3_handle_msglost(struct ican3_dev *mod, struct ican3_msg *msg)
 	if (skb) {
 		cf->can_id |= CAN_ERR_CRTL;
 		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
+		stats->rx_over_errors++;
 		stats->rx_errors++;
-		stats->rx_bytes += cf->can_dlc;
 		netif_rx(skb);
 	}
 }
@@ -982,7 +982,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 
 		dev_dbg(mod->dev, "bus error interrupt\n");
 		mod->can.can_stats.bus_error++;
-		stats->rx_errors++;
 		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
 
 		switch (ecc & ECC_MASK) {
@@ -1001,8 +1000,12 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 			break;
 		}
 
-		if ((ecc & ECC_DIR) == 0)
+		if (!(ecc & ECC_DIR)) {
 			cf->data[2] |= CAN_ERR_PROT_TX;
+			stats->tx_errors++;
+		} else {
+			stats->rx_errors++;
+		}
 
 		cf->data[6] = txerr;
 		cf->data[7] = rxerr;
@@ -1028,8 +1031,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 	}
 
 	mod->can.state = state;
-	stats->rx_errors++;
-	stats->rx_bytes += cf->can_dlc;
 	netif_rx(skb);
 	return 0;
 }
-- 
1.7.8.6

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

[Ira W. Snyder]

From: "Ira W. Snyder" <iws@ovro.caltech.edu>

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 ican3_cmp_echo_skb(), we can drop the loopback
messages and return the original skbs. This fixes the issues with
CAN_RAW_RECV_OWN_MSGS.

A private skb queue is used to store the echo skbs. This avoids the need
for any index management.

Due to a lack of TX-error interrupts, bus errors are permanently
enabled, and are used as a TX-error notification. This is used to drop
an echo skb when transmission fails. Bus error packets are not generated
if the user has not enabled bus error reporting.

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

For this version, the following was changed:
- removed the unnecessary second "bus error interrupt" debug message
- added error printk to ican3_get_echo_skb() condition which should never happen

 drivers/net/can/janz-ican3.c |  203 ++++++++++++++++++++++++++++++++----------
 1 files changed, 157 insertions(+), 46 deletions(-)

diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index 14d5abc..41a880c 100644
--- a/drivers/net/can/janz-ican3.c
+++ b/drivers/net/can/janz-ican3.c
@@ -220,6 +220,9 @@ struct ican3_dev {
 	/* old and new style host interface */
 	unsigned int iftype;
 
+	/* queue for echo packets */
+	struct sk_buff_head echoq;
+
 	/*
 	 * Any function which changes the current DPM page must hold this
 	 * lock while it is performing data accesses. This ensures that the
@@ -925,7 +928,7 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 	struct net_device *dev = mod->ndev;
 	struct net_device_stats *stats = &dev->stats;
 	enum can_state state = mod->can.state;
-	u8 status, isrc, rxerr, txerr;
+	u8 isrc, ecc, status, rxerr, txerr;
 	struct can_frame *cf;
 	struct sk_buff *skb;
 
@@ -941,15 +944,43 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 		return -EINVAL;
 	}
 
-	skb = alloc_can_err_skb(dev, &cf);
-	if (skb == NULL)
-		return -ENOMEM;
-
 	isrc = msg->data[0];
+	ecc = msg->data[2];
 	status = msg->data[3];
 	rxerr = msg->data[4];
 	txerr = msg->data[5];
 
+	/*
+	 * This hardware lacks any support other than bus error messages to
+	 * determine if packet transmission has failed.
+	 *
+	 * When TX errors happen, one echo skb needs to be dropped from the
+	 * front of the queue.
+	 *
+	 * A small bit of code is duplicated here and below, to avoid error
+	 * skb allocation when it will just be freed immediately.
+	 */
+	if (isrc == CEVTIND_BEI) {
+		int ret;
+		dev_dbg(mod->dev, "bus error interrupt\n");
+
+		/* TX error */
+		if (!(ecc & ECC_DIR)) {
+			kfree_skb(skb_dequeue(&mod->echoq));
+			stats->tx_errors++;
+		} else {
+			stats->rx_errors++;
+		}
+
+		/* bus error reporting is off, return immediately */
+		if (!(mod->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING))
+			return 0;
+	}
+
+	skb = alloc_can_err_skb(dev, &cf);
+	if (skb == NULL)
+		return -ENOMEM;
+
 	/* data overrun interrupt */
 	if (isrc == CEVTIND_DOI || isrc == CEVTIND_LOST) {
 		dev_dbg(mod->dev, "data overrun interrupt\n");
@@ -978,9 +1009,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 
 	/* bus error interrupt */
 	if (isrc == CEVTIND_BEI) {
-		u8 ecc = msg->data[2];
-
-		dev_dbg(mod->dev, "bus error interrupt\n");
 		mod->can.can_stats.bus_error++;
 		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
 
@@ -1000,12 +1028,8 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 			break;
 		}
 
-		if (!(ecc & ECC_DIR)) {
+		if (!(ecc & ECC_DIR))
 			cf->data[2] |= CAN_ERR_PROT_TX;
-			stats->tx_errors++;
-		} else {
-			stats->rx_errors++;
-		}
 
 		cf->data[6] = txerr;
 		cf->data[7] = rxerr;
@@ -1090,6 +1114,88 @@ static void ican3_handle_message(struct ican3_dev *mod, struct ican3_msg *msg)
 }
 
 /*
+ * The ican3 needs to store all echo skbs, and therefore cannot
+ * use the generic infrastructure for this.
+ */
+static void ican3_put_echo_skb(struct ican3_dev *mod, struct sk_buff *skb)
+{
+	struct sock *srcsk = skb->sk;
+
+	if (atomic_read(&skb->users) != 1) {
+		struct sk_buff *old_skb = skb;
+
+		skb = skb_clone(old_skb, GFP_ATOMIC);
+		kfree_skb(old_skb);
+		if (!skb)
+			return;
+	} else {
+		skb_orphan(skb);
+	}
+
+	skb->sk = srcsk;
+
+	/* save this skb for tx interrupt echo handling */
+	skb_queue_tail(&mod->echoq, skb);
+}
+
+static unsigned int ican3_get_echo_skb(struct ican3_dev *mod)
+{
+	struct sk_buff *skb = skb_dequeue(&mod->echoq);
+	struct can_frame *cf;
+	u8 dlc;
+
+	/* this should never trigger unless there is a driver bug */
+	if (!skb) {
+		netdev_err(mod->ndev, "BUG: echo skb not occupied\n");
+		return 0;
+	}
+
+	cf = (struct can_frame *)skb->data;
+	dlc = cf->can_dlc;
+
+	/* check flag whether this packet has to be looped back */
+	if (skb->pkt_type != PACKET_LOOPBACK) {
+		kfree_skb(skb);
+		return dlc;
+	}
+
+	skb->protocol = htons(ETH_P_CAN);
+	skb->pkt_type = PACKET_BROADCAST;
+	skb->ip_summed = CHECKSUM_UNNECESSARY;
+	skb->dev = mod->ndev;
+	netif_receive_skb(skb);
+	return dlc;
+}
+
+/*
+ * 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.
+ */
+static bool ican3_echo_skb_matches(struct ican3_dev *mod, struct sk_buff *skb)
+{
+	struct can_frame *cf = (struct can_frame *)skb->data;
+	struct sk_buff *echo_skb = skb_peek(&mod->echoq);
+	struct can_frame *echo_cf;
+
+	if (!echo_skb)
+		return false;
+
+	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;
+}
+
+/*
  * Check that there is room in the TX ring to transmit another skb
  *
  * LOCKING: must hold mod->lock
@@ -1099,6 +1205,10 @@ static bool ican3_txok(struct ican3_dev *mod)
 	struct ican3_fast_desc __iomem *desc;
 	u8 control;
 
+	/* check that we have echo queue space */
+	if (skb_queue_len(&mod->echoq) >= ICAN3_TX_BUFFERS)
+		return false;
+
 	/* copy the control bits of the descriptor */
 	ican3_set_page(mod, mod->fasttx_start + (mod->fasttx_num / 16));
 	desc = mod->dpm + ((mod->fasttx_num % 16) * sizeof(*desc));
@@ -1149,10 +1259,27 @@ static int ican3_recv_skb(struct ican3_dev *mod)
 	/* convert the ICAN3 frame into Linux CAN format */
 	ican3_to_can_frame(mod, &desc, cf);
 
-	/* receive the skb, update statistics */
-	netif_receive_skb(skb);
+	/*
+	 * If this is an ECHO frame received from the hardware loopback
+	 * feature, use the skb saved in the ECHO stack instead. This allows
+	 * the Linux CAN core to support CAN_RAW_RECV_OWN_MSGS correctly.
+	 *
+	 * Since this is a confirmation of a successfully transmitted packet
+	 * sent from this host, update the transmit statistics.
+	 *
+	 * Also, the netdevice queue needs to be allowed to send packets again.
+	 */
+	if (ican3_echo_skb_matches(mod, skb)) {
+		stats->tx_packets++;
+		stats->tx_bytes += ican3_get_echo_skb(mod);
+		kfree_skb(skb);
+		goto err_noalloc;
+	}
+
+	/* update statistics, receive the skb */
 	stats->rx_packets++;
 	stats->rx_bytes += cf->can_dlc;
+	netif_receive_skb(skb);
 
 err_noalloc:
 	/* toggle the valid bit and return the descriptor to the ring */
@@ -1175,13 +1302,13 @@ err_noalloc:
 static int ican3_napi(struct napi_struct *napi, int budget)
 {
 	struct ican3_dev *mod = container_of(napi, struct ican3_dev, napi);
-	struct ican3_msg msg;
 	unsigned long flags;
 	int received = 0;
 	int ret;
 
 	/* process all communication messages */
 	while (true) {
+		struct ican3_msg msg;
 		ret = ican3_recv_msg(mod, &msg);
 		if (ret)
 			break;
@@ -1353,7 +1480,6 @@ static int __devinit ican3_startup_module(struct ican3_dev *mod)
 static int ican3_open(struct net_device *ndev)
 {
 	struct ican3_dev *mod = netdev_priv(ndev);
-	u8 quota;
 	int ret;
 
 	/* open the CAN layer */
@@ -1363,19 +1489,6 @@ static int ican3_open(struct net_device *ndev)
 		return ret;
 	}
 
-	/* set the bus error generation state appropriately */
-	if (mod->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)
-		quota = ICAN3_BUSERR_QUOTA_MAX;
-	else
-		quota = 0;
-
-	ret = ican3_set_buserror(mod, quota);
-	if (ret) {
-		dev_err(mod->dev, "unable to set bus-error\n");
-		close_candev(ndev);
-		return ret;
-	}
-
 	/* bring the bus online */
 	ret = ican3_set_bus_state(mod, true);
 	if (ret) {
@@ -1407,6 +1520,9 @@ static int ican3_stop(struct net_device *ndev)
 		return ret;
 	}
 
+	/* drop all outstanding echo skbs */
+	skb_queue_purge(&mod->echoq);
+
 	/* close the CAN layer */
 	close_candev(ndev);
 	return 0;
@@ -1415,7 +1531,6 @@ static int ican3_stop(struct net_device *ndev)
 static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 {
 	struct ican3_dev *mod = netdev_priv(ndev);
-	struct net_device_stats *stats = &ndev->stats;
 	struct can_frame *cf = (struct can_frame *)skb->data;
 	struct ican3_fast_desc desc;
 	void __iomem *desc_addr;
@@ -1428,8 +1543,7 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 
 	/* check that we can actually transmit */
 	if (!ican3_txok(mod)) {
-		dev_err(mod->dev, "no free descriptors, stopping queue\n");
-		netif_stop_queue(ndev);
+		dev_err(mod->dev, "BUG: no free descriptors\n");
 		spin_unlock_irqrestore(&mod->lock, flags);
 		return NETDEV_TX_BUSY;
 	}
@@ -1444,6 +1558,14 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 	can_frame_to_ican3(mod, cf, &desc);
 
 	/*
+	 * This hardware doesn't have TX-done notifications, so we'll try and
+	 * emulate it the best we can using ECHO skbs. Add the skb to the ECHO
+	 * stack. Upon packet reception, check if the ECHO skb and received
+	 * skb match, and use that to wake the queue.
+	 */
+	ican3_put_echo_skb(mod, skb);
+
+	/*
 	 * the programming manual says that you must set the IVALID bit, then
 	 * interrupt, then set the valid bit. Quite weird, but it seems to be
 	 * required for this to work
@@ -1461,19 +1583,7 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 	mod->fasttx_num = (desc.control & DESC_WRAP) ? 0
 						     : (mod->fasttx_num + 1);
 
-	/* 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
-	 * emulate it the best we can using ECHO skbs. Get the next TX
-	 * descriptor, and see if we have room to send. If not, stop the queue.
-	 * It will be woken when the ECHO skb for the current packet is recv'd.
-	 */
-
-	/* copy the control bits of the descriptor */
+	/* if there is no free descriptor space, stop the transmit queue */
 	if (!ican3_txok(mod))
 		netif_stop_queue(ndev);
 
@@ -1669,6 +1779,7 @@ static int __devinit ican3_probe(struct platform_device *pdev)
 	mod->dev = &pdev->dev;
 	mod->num = pdata->modno;
 	netif_napi_add(ndev, &mod->napi, ican3_napi, ICAN3_RX_BUFFERS);
+	skb_queue_head_init(&mod->echoq);
 	spin_lock_init(&mod->lock);
 	init_completion(&mod->termination_comp);
 	init_completion(&mod->buserror_comp);
-- 
1.7.8.6

Re: [PATCH 3/6] can: janz-ican3: fix error and byte counters

[Marc Kleine-Budde]

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

On 07/19/2012 05:54 PM, Ira W. Snyder wrote:
> From: "Ira W. Snyder" <iws@ovro.caltech.edu>
> 
> The error and byte counter statistics were being incremented
> incorrectly. For example, a TX error would be counted both in tx_errors
> and rx_errors.
> 
> This corrects the problem so that tx_errors and rx_errors are only
> incremented for errors caused by packets sent to the bus. Error packets
> generated by the driver are not counted.
> 
> The byte counters are only increased for packets which are actually
> transmitted or received from the bus. Error packets generated by the
> driver are not counted.
> 
> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> ---
> 
> Changes for this version:
> - for RX overflow errors, both rx_over_errors and rx_errors are incremented
> 
>  drivers/net/can/janz-ican3.c |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> index ea1919e..14d5abc 100644
> --- a/drivers/net/can/janz-ican3.c
> +++ b/drivers/net/can/janz-ican3.c
> @@ -907,8 +907,8 @@ static void ican3_handle_msglost(struct ican3_dev *mod, struct ican3_msg *msg)
>  	if (skb) {
>  		cf->can_id |= CAN_ERR_CRTL;
>  		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> +		stats->rx_over_errors++;
>  		stats->rx_errors++;
> -		stats->rx_bytes += cf->can_dlc;
>  		netif_rx(skb);
>  	}
>  }
> @@ -982,7 +982,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>  
>  		dev_dbg(mod->dev, "bus error interrupt\n");
>  		mod->can.can_stats.bus_error++;
> -		stats->rx_errors++;
                ^^^^^^^^^^^^^^^^^^

What about this one? Accidentally removed?

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: 262 bytes --]

Re: [PATCH 3/6] can: janz-ican3: fix error and byte counters

[Ira W. Snyder]

On Thu, Jul 19, 2012 at 09:32:23PM +0200, Marc Kleine-Budde wrote:
> On 07/19/2012 05:54 PM, Ira W. Snyder wrote:
> > From: "Ira W. Snyder" <iws@ovro.caltech.edu>
> > 
> > The error and byte counter statistics were being incremented
> > incorrectly. For example, a TX error would be counted both in tx_errors
> > and rx_errors.
> > 
> > This corrects the problem so that tx_errors and rx_errors are only
> > incremented for errors caused by packets sent to the bus. Error packets
> > generated by the driver are not counted.
> > 
> > The byte counters are only increased for packets which are actually
> > transmitted or received from the bus. Error packets generated by the
> > driver are not counted.
> > 
> > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> > ---
> > 
> > Changes for this version:
> > - for RX overflow errors, both rx_over_errors and rx_errors are incremented
> > 
> >  drivers/net/can/janz-ican3.c |   11 ++++++-----
> >  1 files changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> > index ea1919e..14d5abc 100644
> > --- a/drivers/net/can/janz-ican3.c
> > +++ b/drivers/net/can/janz-ican3.c
> > @@ -907,8 +907,8 @@ static void ican3_handle_msglost(struct ican3_dev *mod, struct ican3_msg *msg)
> >  	if (skb) {
> >  		cf->can_id |= CAN_ERR_CRTL;
> >  		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> > +		stats->rx_over_errors++;
> >  		stats->rx_errors++;
> > -		stats->rx_bytes += cf->can_dlc;
> >  		netif_rx(skb);
> >  	}
> >  }
> > @@ -982,7 +982,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
> >  
> >  		dev_dbg(mod->dev, "bus error interrupt\n");
> >  		mod->can.can_stats.bus_error++;
> > -		stats->rx_errors++;
>                 ^^^^^^^^^^^^^^^^^^
> 
> What about this one? Accidentally removed?
> 

No, it is correct. You snipped the extra parts of the patch that were
relevant. I've re-added them, along with my explanation.


> @@ -1001,8 +1000,12 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>  			break;
>  		}
>  
> -		if ((ecc & ECC_DIR) == 0)
> +		if (!(ecc & ECC_DIR)) {
>  			cf->data[2] |= CAN_ERR_PROT_TX;
> +			stats->tx_errors++;
> +		} else {
> +			stats->rx_errors++;
> +		}
>  

The error counter you mention above moved down here. We are now
distinguishing between RX bus errors and TX bus errors.

>  		cf->data[6] = txerr;
>  		cf->data[7] = rxerr;
> @@ -1028,8 +1031,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>  	}
>  
>  	mod->can.state = state;
> -	stats->rx_errors++;
> -	stats->rx_bytes += cf->can_dlc;
>  	netif_rx(skb);
>  	return 0;
>  }

And this was just bogus. Before this fix, I was incrementing rx_errors
twice when a bus error interrupt was received.

Ira

> 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   |
>