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

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

[Ira W. Snyder]

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

Per reviewer feedback, I have moved the janz-ican3 driver-specific
modifications to the can_put_echo_skb() and can_get_echo_skb() into the
janz-ican3 driver itself.

The first patch brings support up to the level of the previously posted
patch series.

The second patch modifies the driver-specific versions of
can_put_echo_skb() and can_get_echo_skb() to use an SKB queue. This
removes the burden of index management, and allows increased TX buffer
queue length.

The third patch adds support for CAN_CTRLMODE_ONE_SHOT. My coworker has
requested this, and it seems like a good enhancement.

Ira W. Snyder (3):
  can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS
  can: janz-ican3: increase tx buffer size
  can: janz-ican3: add support for one shot mode

 drivers/net/can/janz-ican3.c |  164 +++++++++++++++++++++++++++++++++++-------
 1 files changed, 137 insertions(+), 27 deletions(-)

-- 
1.7.8.6

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

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

This is a simple merge of the code from the previously posted patch
series into the driver itself. This avoids changing the code for other
drivers which don't have the deficiencies of this hardware.

 drivers/net/can/janz-ican3.c |  169 +++++++++++++++++++++++++++++++++--------
 1 files changed, 136 insertions(+), 33 deletions(-)

diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index 08c893c..0f48136 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));
@@ -1091,6 +1089,107 @@ 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 sk_buff *skb, struct net_device *dev,
+			       unsigned int idx)
+{
+	struct can_priv *priv = netdev_priv(dev);
+
+	BUG_ON(idx >= priv->echo_skb_max);
+
+	if (!priv->echo_skb[idx]) {
+		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 */
+		priv->echo_skb[idx] = skb;
+	} else {
+		/* locking problem with netif_stop_queue() ?? */
+		netdev_err(dev, "%s: BUG! echo_skb is occupied!\n", __func__);
+		kfree_skb(skb);
+	}
+}
+
+static unsigned int ican3_get_echo_skb(struct net_device *dev, unsigned int idx)
+{
+	struct can_priv *priv = netdev_priv(dev);
+	struct sk_buff *skb = priv->echo_skb[idx];
+	struct can_frame *cf;
+	u8 dlc;
+
+	BUG_ON(idx >= priv->echo_skb_max);
+
+	if (!skb)
+		return 0;
+
+	priv->echo_skb[idx] = NULL;
+	cf = (struct can_frame *)skb->data;
+
+	/* check flag whether this packet has to be looped back */
+	if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
+		kfree_skb(skb);
+		return 0;
+	}
+
+	/* make settings for echo to reduce code in irq context */
+	skb->protocol = htons(ETH_P_CAN);
+	skb->pkt_type = PACKET_BROADCAST;
+	skb->ip_summed = CHECKSUM_UNNECESSARY;
+	skb->dev = dev;
+
+	dlc = cf->can_dlc;
+	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_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;
+}
+
+/*
  * Check that there is room in the TX ring to transmit another skb
  *
  * LOCKING: must hold mod->lock
@@ -1150,10 +1249,28 @@ 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_cmp_echo_skb(skb, ndev, 0)) {
+		stats->tx_packets++;
+		stats->tx_bytes += ican3_get_echo_skb(ndev, 0);
+		kfree_skb(skb);
+		netif_wake_queue(ndev);
+		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 */
@@ -1177,7 +1294,6 @@ 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;
 
@@ -1204,14 +1320,6 @@ static int ican3_napi(struct napi_struct *napi, int budget)
 	if (received < budget)
 		napi_complete(napi);
 
-	spin_lock_irqsave(&mod->lock, flags);
-
-	/* Wake up the transmit queue if necessary */
-	if (netif_queue_stopped(mod->ndev) && ican3_txok(mod))
-		netif_wake_queue(mod->ndev);
-
-	spin_unlock_irqrestore(&mod->lock, flags);
-
 	/* re-enable interrupt generation */
 	iowrite8(1 << mod->num, &mod->ctrl->int_enable);
 	return received;
@@ -1416,18 +1524,19 @@ 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;
 	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 */
 	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 +1551,16 @@ 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 and stop transmitting packets. Upon packet reception, check
+	 * if the ECHO skb and received skb match, and use that to wake the
+	 * queue.
+	 */
+	netif_stop_queue(ndev);
+	ican3_put_echo_skb(skb, ndev, 0);
+
+	/*
 	 * 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,22 +1578,6 @@ 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 (!ican3_txok(mod))
-		netif_stop_queue(ndev);
-
 	spin_unlock_irqrestore(&mod->lock, flags);
 	return NETDEV_TX_OK;
 }
@@ -1654,7 +1757,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), 1);
 	if (!ndev) {
 		dev_err(dev, "unable to allocate CANdev\n");
 		ret = -ENOMEM;
-- 
1.7.8.6

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

[Wolfgang Grandegger]

Hi Ira,

On 07/12/2012 06:15 PM, 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.
> 
> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>

I'm fine with the device-specific handling of the special echo skb
management. I have a few more comments to address... then you can add
my: Acked-by: Wolfgang Grandegger <wg@grandegger.com>


> ---
> 
> This is a simple merge of the code from the previously posted patch
> series into the driver itself. This avoids changing the code for other
> drivers which don't have the deficiencies of this hardware.
> 
>  drivers/net/can/janz-ican3.c |  169 +++++++++++++++++++++++++++++++++--------
>  1 files changed, 136 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> index 08c893c..0f48136 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;

Is this change related?

>  
>  	/* build a single fast tohost queue descriptor */
>  	memset(&desc, 0, sizeof(desc));
> @@ -1091,6 +1089,107 @@ 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 sk_buff *skb, struct net_device *dev,
> +			       unsigned int idx)
> +{
> +	struct can_priv *priv = netdev_priv(dev);
> +
> +	BUG_ON(idx >= priv->echo_skb_max);
> +
> +	if (!priv->echo_skb[idx]) {
> +		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 */
> +		priv->echo_skb[idx] = skb;
> +	} else {
> +		/* locking problem with netif_stop_queue() ?? */
> +		netdev_err(dev, "%s: BUG! echo_skb is occupied!\n", __func__);
> +		kfree_skb(skb);
> +	}
> +}
> +
> +static unsigned int ican3_get_echo_skb(struct net_device *dev, unsigned int idx)
> +{
> +	struct can_priv *priv = netdev_priv(dev);
> +	struct sk_buff *skb = priv->echo_skb[idx];
> +	struct can_frame *cf;
> +	u8 dlc;
> +
> +	BUG_ON(idx >= priv->echo_skb_max);
> +
> +	if (!skb)
> +		return 0;
> +
> +	priv->echo_skb[idx] = NULL;
> +	cf = (struct can_frame *)skb->data;
> +
> +	/* check flag whether this packet has to be looped back */
> +	if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
> +		kfree_skb(skb);
> +		return 0;
> +	}
> +	/* make settings for echo to reduce code in irq context */

Is this comment still appropriate?

> +	skb->protocol = htons(ETH_P_CAN);
> +	skb->pkt_type = PACKET_BROADCAST;
> +	skb->ip_summed = CHECKSUM_UNNECESSARY;
> +	skb->dev = dev;
> +
> +	dlc = cf->can_dlc;
> +	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_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;
> +}
> +
> +/*
>   * Check that there is room in the TX ring to transmit another skb
>   *
>   * LOCKING: must hold mod->lock
> @@ -1150,10 +1249,28 @@ 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_cmp_echo_skb(skb, ndev, 0)) {

I would prefer a more intuitive name, e.g. ican3_echo_skb_does_match().

> +		stats->tx_packets++;
> +		stats->tx_bytes += ican3_get_echo_skb(ndev, 0);
> +		kfree_skb(skb);
> +		netif_wake_queue(ndev);
> +		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 */
> @@ -1177,7 +1294,6 @@ 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;
>  
> @@ -1204,14 +1320,6 @@ static int ican3_napi(struct napi_struct *napi, int budget)
>  	if (received < budget)
>  		napi_complete(napi);
>  
> -	spin_lock_irqsave(&mod->lock, flags);
> -
> -	/* Wake up the transmit queue if necessary */
> -	if (netif_queue_stopped(mod->ndev) && ican3_txok(mod))
> -		netif_wake_queue(mod->ndev);
> -
> -	spin_unlock_irqrestore(&mod->lock, flags);
> -
>  	/* re-enable interrupt generation */
>  	iowrite8(1 << mod->num, &mod->ctrl->int_enable);
>  	return received;
> @@ -1416,18 +1524,19 @@ 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;
>  	unsigned long flags;
>  
> +	if (can_dropped_invalid_skb(ndev, skb))
> +		return NETDEV_TX_OK;
> +

Is this change related? At least you should add a comment in the commit
message.

>  	spin_lock_irqsave(&mod->lock, flags);
>  
>  	/* 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 +1551,16 @@ 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 and stop transmitting packets. Upon packet reception, check
> +	 * if the ECHO skb and received skb match, and use that to wake the
> +	 * queue.
> +	 */
> +	netif_stop_queue(ndev);
> +	ican3_put_echo_skb(skb, ndev, 0);
> +
> +	/*
>  	 * 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,22 +1578,6 @@ 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 (!ican3_txok(mod))
> -		netif_stop_queue(ndev);
> -
>  	spin_unlock_irqrestore(&mod->lock, flags);
>  	return NETDEV_TX_OK;
>  }
> @@ -1654,7 +1757,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), 1);

What is that change good for?

>  	if (!ndev) {
>  		dev_err(dev, "unable to allocate CANdev\n");
>  		ret = -ENOMEM;
> 

Wolfgang.

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

[Ira W. Snyder]

On Mon, Jul 16, 2012 at 09:28:45AM +0200, Wolfgang Grandegger wrote:
> Hi Ira,
> 
> On 07/12/2012 06:15 PM, 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.
> > 
> > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> 
> I'm fine with the device-specific handling of the special echo skb
> management. I have a few more comments to address... then you can add
> my: Acked-by: Wolfgang Grandegger <wg@grandegger.com>
> 
> 
> > ---
> > 
> > This is a simple merge of the code from the previously posted patch
> > series into the driver itself. This avoids changing the code for other
> > drivers which don't have the deficiencies of this hardware.
> > 
> >  drivers/net/can/janz-ican3.c |  169 +++++++++++++++++++++++++++++++++--------
> >  1 files changed, 136 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> > index 08c893c..0f48136 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;
> 
> Is this change related?
> 

No. It is unused code.

Do you want a separate patch to remove these two lines, separate from
the other patches?

> >  
> >  	/* build a single fast tohost queue descriptor */
> >  	memset(&desc, 0, sizeof(desc));
> > @@ -1091,6 +1089,107 @@ 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 sk_buff *skb, struct net_device *dev,
> > +			       unsigned int idx)
> > +{
> > +	struct can_priv *priv = netdev_priv(dev);
> > +
> > +	BUG_ON(idx >= priv->echo_skb_max);
> > +
> > +	if (!priv->echo_skb[idx]) {
> > +		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 */
> > +		priv->echo_skb[idx] = skb;
> > +	} else {
> > +		/* locking problem with netif_stop_queue() ?? */
> > +		netdev_err(dev, "%s: BUG! echo_skb is occupied!\n", __func__);
> > +		kfree_skb(skb);
> > +	}
> > +}
> > +
> > +static unsigned int ican3_get_echo_skb(struct net_device *dev, unsigned int idx)
> > +{
> > +	struct can_priv *priv = netdev_priv(dev);
> > +	struct sk_buff *skb = priv->echo_skb[idx];
> > +	struct can_frame *cf;
> > +	u8 dlc;
> > +
> > +	BUG_ON(idx >= priv->echo_skb_max);
> > +
> > +	if (!skb)
> > +		return 0;
> > +
> > +	priv->echo_skb[idx] = NULL;
> > +	cf = (struct can_frame *)skb->data;
> > +
> > +	/* check flag whether this packet has to be looped back */
> > +	if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
> > +		kfree_skb(skb);
> > +		return 0;
> > +	}
> > +	/* make settings for echo to reduce code in irq context */
> 
> Is this comment still appropriate?
> 

I don't know. It is copied verbatim from drivers/net/can/dev.c's
can_put_echo_skb() function. You wrote that function. :)

I have no problem removing the comment, if you'd like.

> > +	skb->protocol = htons(ETH_P_CAN);
> > +	skb->pkt_type = PACKET_BROADCAST;
> > +	skb->ip_summed = CHECKSUM_UNNECESSARY;
> > +	skb->dev = dev;
> > +
> > +	dlc = cf->can_dlc;
> > +	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_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;
> > +}
> > +
> > +/*
> >   * Check that there is room in the TX ring to transmit another skb
> >   *
> >   * LOCKING: must hold mod->lock
> > @@ -1150,10 +1249,28 @@ 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_cmp_echo_skb(skb, ndev, 0)) {
> 
> I would prefer a more intuitive name, e.g. ican3_echo_skb_does_match().
> 

Ok.

> > +		stats->tx_packets++;
> > +		stats->tx_bytes += ican3_get_echo_skb(ndev, 0);
> > +		kfree_skb(skb);
> > +		netif_wake_queue(ndev);
> > +		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 */
> > @@ -1177,7 +1294,6 @@ 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;
> >  
> > @@ -1204,14 +1320,6 @@ static int ican3_napi(struct napi_struct *napi, int budget)
> >  	if (received < budget)
> >  		napi_complete(napi);
> >  
> > -	spin_lock_irqsave(&mod->lock, flags);
> > -
> > -	/* Wake up the transmit queue if necessary */
> > -	if (netif_queue_stopped(mod->ndev) && ican3_txok(mod))
> > -		netif_wake_queue(mod->ndev);
> > -
> > -	spin_unlock_irqrestore(&mod->lock, flags);
> > -
> >  	/* re-enable interrupt generation */
> >  	iowrite8(1 << mod->num, &mod->ctrl->int_enable);
> >  	return received;
> > @@ -1416,18 +1524,19 @@ 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;
> >  	unsigned long flags;
> >  
> > +	if (can_dropped_invalid_skb(ndev, skb))
> > +		return NETDEV_TX_OK;
> > +
> 
> Is this change related? At least you should add a comment in the commit
> message.
> 

No, it is not related. I'll break it into a separate patch.

In commit 3ccd4c6167d "can: Unify droping of invalid tx skbs and netdev
stats" this code was added to all other drivers.

It looks like the patch adding the janz-ican3 driver and the above
mentioned patch went in to mainline in the same merge cycle, and
therefore this change was never made in the janz-ican3 driver.

> >  	spin_lock_irqsave(&mod->lock, flags);
> >  
> >  	/* 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 +1551,16 @@ 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 and stop transmitting packets. Upon packet reception, check
> > +	 * if the ECHO skb and received skb match, and use that to wake the
> > +	 * queue.
> > +	 */
> > +	netif_stop_queue(ndev);
> > +	ican3_put_echo_skb(skb, ndev, 0);
> > +
> > +	/*
> >  	 * 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,22 +1578,6 @@ 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 (!ican3_txok(mod))
> > -		netif_stop_queue(ndev);
> > -
> >  	spin_unlock_irqrestore(&mod->lock, flags);
> >  	return NETDEV_TX_OK;
> >  }
> > @@ -1654,7 +1757,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), 1);
> 
> What is that change good for?
> 

This change allocates one echo skb (in priv->echo_skb[]). It is useless
when the patch is merged into the next patch in the series, which uses
an skb queue instead.

It is probably easier to review the squashed patch, which is posted as a
reply to the next patch in the series.

> >  	if (!ndev) {
> >  		dev_err(dev, "unable to allocate CANdev\n");
> >  		ret = -ENOMEM;
> > 
> 
> Wolfgang.

Thanks for the review,
Ira

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

[Wolfgang Grandegger]

On 07/16/2012 06:00 PM, Ira W. Snyder wrote:
> On Mon, Jul 16, 2012 at 09:28:45AM +0200, Wolfgang Grandegger wrote:
>> Hi Ira,
>>
>> On 07/12/2012 06:15 PM, 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.
>>>
>>> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
>>
>> I'm fine with the device-specific handling of the special echo skb
>> management. I have a few more comments to address... then you can add
>> my: Acked-by: Wolfgang Grandegger <wg@grandegger.com>
>>
>>
>>> ---
>>>
>>> This is a simple merge of the code from the previously posted patch
>>> series into the driver itself. This avoids changing the code for other
>>> drivers which don't have the deficiencies of this hardware.
>>>
>>>  drivers/net/can/janz-ican3.c |  169 +++++++++++++++++++++++++++++++++--------
>>>  1 files changed, 136 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
>>> index 08c893c..0f48136 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;
>>
>> Is this change related?
>>
> 
> No. It is unused code.
> 
> Do you want a separate patch to remove these two lines, separate from
> the other patches?

For me it's fine if you just add a short note to the commit message.
(Upstream-)Maintainers usually don't like to change unrelated things
silently. But a separate patch fixing the unrelated things would be
perfect, though.

> 
>>>  
>>>  	/* build a single fast tohost queue descriptor */
>>>  	memset(&desc, 0, sizeof(desc));
>>> @@ -1091,6 +1089,107 @@ 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 sk_buff *skb, struct net_device *dev,
>>> +			       unsigned int idx)
>>> +{
>>> +	struct can_priv *priv = netdev_priv(dev);
>>> +
>>> +	BUG_ON(idx >= priv->echo_skb_max);
>>> +
>>> +	if (!priv->echo_skb[idx]) {
>>> +		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 */
>>> +		priv->echo_skb[idx] = skb;
>>> +	} else {
>>> +		/* locking problem with netif_stop_queue() ?? */
>>> +		netdev_err(dev, "%s: BUG! echo_skb is occupied!\n", __func__);
>>> +		kfree_skb(skb);
>>> +	}
>>> +}
>>> +
>>> +static unsigned int ican3_get_echo_skb(struct net_device *dev, unsigned int idx)
>>> +{
>>> +	struct can_priv *priv = netdev_priv(dev);
>>> +	struct sk_buff *skb = priv->echo_skb[idx];
>>> +	struct can_frame *cf;
>>> +	u8 dlc;
>>> +
>>> +	BUG_ON(idx >= priv->echo_skb_max);
>>> +
>>> +	if (!skb)
>>> +		return 0;
>>> +
>>> +	priv->echo_skb[idx] = NULL;
>>> +	cf = (struct can_frame *)skb->data;
>>> +
>>> +	/* check flag whether this packet has to be looped back */
>>> +	if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
>>> +		kfree_skb(skb);
>>> +		return 0;
>>> +	}
>>> +	/* make settings for echo to reduce code in irq context */
>>
>> Is this comment still appropriate?
>>
> 
> I don't know. It is copied verbatim from drivers/net/can/dev.c's
> can_put_echo_skb() function. You wrote that function. :)
> 
> I have no problem removing the comment, if you'd like.

It should be removed because it's not true any more due to the move from
the put to the get function.

>>> +	skb->protocol = htons(ETH_P_CAN);
>>> +	skb->pkt_type = PACKET_BROADCAST;
>>> +	skb->ip_summed = CHECKSUM_UNNECESSARY;
>>> +	skb->dev = dev;
>>> +
>>> +	dlc = cf->can_dlc;
>>> +	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_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;
>>> +}
>>> +
>>> +/*
>>>   * Check that there is room in the TX ring to transmit another skb
>>>   *
>>>   * LOCKING: must hold mod->lock
>>> @@ -1150,10 +1249,28 @@ 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_cmp_echo_skb(skb, ndev, 0)) {
>>
>> I would prefer a more intuitive name, e.g. ican3_echo_skb_does_match().
>>
> 
> Ok.
> 
>>> +		stats->tx_packets++;
>>> +		stats->tx_bytes += ican3_get_echo_skb(ndev, 0);
>>> +		kfree_skb(skb);
>>> +		netif_wake_queue(ndev);
>>> +		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 */
>>> @@ -1177,7 +1294,6 @@ 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;
>>>  
>>> @@ -1204,14 +1320,6 @@ static int ican3_napi(struct napi_struct *napi, int budget)
>>>  	if (received < budget)
>>>  		napi_complete(napi);
>>>  
>>> -	spin_lock_irqsave(&mod->lock, flags);
>>> -
>>> -	/* Wake up the transmit queue if necessary */
>>> -	if (netif_queue_stopped(mod->ndev) && ican3_txok(mod))
>>> -		netif_wake_queue(mod->ndev);
>>> -
>>> -	spin_unlock_irqrestore(&mod->lock, flags);
>>> -
>>>  	/* re-enable interrupt generation */
>>>  	iowrite8(1 << mod->num, &mod->ctrl->int_enable);
>>>  	return received;
>>> @@ -1416,18 +1524,19 @@ 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;
>>>  	unsigned long flags;
>>>  
>>> +	if (can_dropped_invalid_skb(ndev, skb))
>>> +		return NETDEV_TX_OK;
>>> +
>>
>> Is this change related? At least you should add a comment in the commit
>> message.
>>
> 
> No, it is not related. I'll break it into a separate patch.

See my comment above.

> In commit 3ccd4c6167d "can: Unify droping of invalid tx skbs and netdev
> stats" this code was added to all other drivers.
> 
> It looks like the patch adding the janz-ican3 driver and the above
> mentioned patch went in to mainline in the same merge cycle, and
> therefore this change was never made in the janz-ican3 driver.
> 
>>>  	spin_lock_irqsave(&mod->lock, flags);
>>>  
>>>  	/* 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 +1551,16 @@ 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 and stop transmitting packets. Upon packet reception, check
>>> +	 * if the ECHO skb and received skb match, and use that to wake the
>>> +	 * queue.
>>> +	 */
>>> +	netif_stop_queue(ndev);
>>> +	ican3_put_echo_skb(skb, ndev, 0);
>>> +
>>> +	/*
>>>  	 * 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,22 +1578,6 @@ 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 (!ican3_txok(mod))
>>> -		netif_stop_queue(ndev);
>>> -
>>>  	spin_unlock_irqrestore(&mod->lock, flags);
>>>  	return NETDEV_TX_OK;
>>>  }
>>> @@ -1654,7 +1757,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), 1);
>>
>> What is that change good for?
>>
> 
> This change allocates one echo skb (in priv->echo_skb[]). It is useless
> when the patch is merged into the next patch in the series, which uses
> an skb queue instead.

OK.

Wolfgang.

[PATCH 2/3] can: janz-ican3: increase tx buffer size

[Ira W. Snyder]

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

Using an skb queue allows the driver to increase the buffer size without
any difficult index management.

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

The index management in the echo skb array is tricky, and so I used an SKB
queue instead. It tracks the length of the queue, and makes it easy to stop
the transmit queue when it becomes full.

If you think this is an OK idea, please merge this into the previous patch.
I kept them split just in case you don't like it.

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

diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index 0f48136..5fff829 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
@@ -1092,54 +1095,37 @@ 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 sk_buff *skb, struct net_device *dev,
-			       unsigned int idx)
+static void ican3_put_echo_skb(struct ican3_dev *mod, struct sk_buff *skb)
 {
-	struct can_priv *priv = netdev_priv(dev);
-
-	BUG_ON(idx >= priv->echo_skb_max);
-
-	if (!priv->echo_skb[idx]) {
-		struct sock *srcsk = skb->sk;
+	struct sock *srcsk = skb->sk;
 
-		if (atomic_read(&skb->users) != 1) {
-			struct sk_buff *old_skb = skb;
+	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 = skb_clone(old_skb, GFP_ATOMIC);
+		kfree_skb(old_skb);
+		if (!skb)
+			return;
+	} else
+		skb_orphan(skb);
 
-		skb->sk = srcsk;
+	skb->sk = srcsk;
 
-		/* save this skb for tx interrupt echo handling */
-		priv->echo_skb[idx] = skb;
-	} else {
-		/* locking problem with netif_stop_queue() ?? */
-		netdev_err(dev, "%s: BUG! echo_skb is occupied!\n", __func__);
-		kfree_skb(skb);
-	}
+	/* save this skb for tx interrupt echo handling */
+	skb_queue_tail(&mod->echoq, skb);
 }
 
-static unsigned int ican3_get_echo_skb(struct net_device *dev, unsigned int idx)
+static unsigned int ican3_get_echo_skb(struct ican3_dev *mod)
 {
-	struct can_priv *priv = netdev_priv(dev);
-	struct sk_buff *skb = priv->echo_skb[idx];
+	struct sk_buff *skb = skb_dequeue(&mod->echoq);
 	struct can_frame *cf;
 	u8 dlc;
 
-	BUG_ON(idx >= priv->echo_skb_max);
-
 	if (!skb)
 		return 0;
 
-	priv->echo_skb[idx] = NULL;
-	cf = (struct can_frame *)skb->data;
-
 	/* check flag whether this packet has to be looped back */
-	if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
+	if (!(mod->ndev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
 		kfree_skb(skb);
 		return 0;
 	}
@@ -1148,8 +1134,9 @@ static unsigned int ican3_get_echo_skb(struct net_device *dev, unsigned int idx)
 	skb->protocol = htons(ETH_P_CAN);
 	skb->pkt_type = PACKET_BROADCAST;
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
-	skb->dev = dev;
+	skb->dev = mod->ndev;
 
+	cf = (struct can_frame *)skb->data;
 	dlc = cf->can_dlc;
 	netif_receive_skb(skb);
 
@@ -1165,16 +1152,12 @@ static unsigned int ican3_get_echo_skb(struct net_device *dev, unsigned int idx)
  *
  * Returns true if the skb's are identical, false otherwise.
  */
-static bool ican3_cmp_echo_skb(struct sk_buff *skb, struct net_device *dev,
-			       unsigned int idx)
+static bool ican3_cmp_echo_skb(struct ican3_dev *mod, struct sk_buff *skb)
 {
-	struct can_priv *priv = netdev_priv(dev);
 	struct can_frame *cf = (struct can_frame *)skb->data;
+	struct sk_buff *echo_skb = skb_peek(&mod->echoq);
 
-	BUG_ON(idx >= priv->echo_skb_max);
-
-	if (priv->echo_skb[idx]) {
-		struct sk_buff *echo_skb = priv->echo_skb[idx];
+	if (echo_skb) {
 		struct can_frame *echo_cf = (struct can_frame *)echo_skb->data;
 
 		if (cf->can_id != echo_cf->can_id)
@@ -1199,6 +1182,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));
@@ -1259,11 +1246,10 @@ static int ican3_recv_skb(struct ican3_dev *mod)
 	 *
 	 * Also, the netdevice queue needs to be allowed to send packets again.
 	 */
-	if (ican3_cmp_echo_skb(skb, ndev, 0)) {
+	if (ican3_cmp_echo_skb(mod, skb)) {
 		stats->tx_packets++;
-		stats->tx_bytes += ican3_get_echo_skb(ndev, 0);
+		stats->tx_bytes += ican3_get_echo_skb(mod);
 		kfree_skb(skb);
-		netif_wake_queue(ndev);
 		goto err_noalloc;
 	}
 
@@ -1293,12 +1279,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;
@@ -1315,6 +1302,14 @@ static int ican3_napi(struct napi_struct *napi, int budget)
 		received++;
 	}
 
+	spin_lock_irqsave(&mod->lock, flags);
+
+	/* Wake up the transmit queue if necessary */
+	if (netif_queue_stopped(mod->ndev) && ican3_txok(mod))
+		netif_wake_queue(mod->ndev);
+
+	spin_unlock_irqrestore(&mod->lock, flags);
+
 	/* We have processed all packets that the adapter had, but it
 	 * was less than our budget, stop polling */
 	if (received < budget)
@@ -1516,6 +1511,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;
@@ -1553,12 +1551,10 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 	/*
 	 * 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 and stop transmitting packets. Upon packet reception, check
-	 * if the ECHO skb and received skb match, and use that to wake the
-	 * queue.
+	 * stack. Upon packet reception, check if the ECHO skb and received
+	 * skb match, and use that to wake the queue.
 	 */
-	netif_stop_queue(ndev);
-	ican3_put_echo_skb(skb, ndev, 0);
+	ican3_put_echo_skb(mod, skb);
 
 	/*
 	 * the programming manual says that you must set the IVALID bit, then
@@ -1578,6 +1574,10 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 	mod->fasttx_num = (desc.control & DESC_WRAP) ? 0
 						     : (mod->fasttx_num + 1);
 
+	/* if there is no free descriptor space, stop the transmit queue */
+	if (!ican3_txok(mod))
+		netif_stop_queue(ndev);
+
 	spin_unlock_irqrestore(&mod->lock, flags);
 	return NETDEV_TX_OK;
 }
@@ -1757,7 +1757,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), 1);
+	ndev = alloc_candev(sizeof(*mod), 0);
 	if (!ndev) {
 		dev_err(dev, "unable to allocate CANdev\n");
 		ret = -ENOMEM;
@@ -1770,6 +1770,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 2/3] can: janz-ican3: increase tx buffer size

[Marc Kleine-Budde]

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

On 07/12/2012 06:15 PM, Ira W. Snyder wrote:
> From: "Ira W. Snyder" <iws@ovro.caltech.edu>
> 
> Using an skb queue allows the driver to increase the buffer size without
> any difficult index management.

Looks interesting, can you squash into the first patch for easier review?

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

[PATCH] 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.

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

This is a squashed together version of the first two patches in the series.

 drivers/net/can/janz-ican3.c |  156 +++++++++++++++++++++++++++++++++++-------
 1 files changed, 130 insertions(+), 26 deletions(-)

diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index 08c893c..5fff829 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
@@ -235,7 +238,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 +456,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));
@@ -1091,6 +1092,87 @@ 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;
+
+	/* check flag whether this packet has to be looped back */
+	if (!(mod->ndev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
+		kfree_skb(skb);
+		return 0;
+	}
+
+	/* make settings for echo to reduce code in irq context */
+	skb->protocol = htons(ETH_P_CAN);
+	skb->pkt_type = PACKET_BROADCAST;
+	skb->ip_summed = CHECKSUM_UNNECESSARY;
+	skb->dev = mod->ndev;
+
+	cf = (struct can_frame *)skb->data;
+	dlc = cf->can_dlc;
+	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_cmp_echo_skb(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);
+
+	if (echo_skb) {
+		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;
+}
+
+/*
  * Check that there is room in the TX ring to transmit another skb
  *
  * LOCKING: must hold mod->lock
@@ -1100,6 +1182,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));
@@ -1150,10 +1236,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_cmp_echo_skb(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 */
@@ -1176,13 +1279,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;
@@ -1199,11 +1302,6 @@ static int ican3_napi(struct napi_struct *napi, int budget)
 		received++;
 	}
 
-	/* We have processed all packets that the adapter had, but it
-	 * was less than our budget, stop polling */
-	if (received < budget)
-		napi_complete(napi);
-
 	spin_lock_irqsave(&mod->lock, flags);
 
 	/* Wake up the transmit queue if necessary */
@@ -1212,6 +1310,11 @@ static int ican3_napi(struct napi_struct *napi, int budget)
 
 	spin_unlock_irqrestore(&mod->lock, flags);
 
+	/* We have processed all packets that the adapter had, but it
+	 * was less than our budget, stop polling */
+	if (received < budget)
+		napi_complete(napi);
+
 	/* re-enable interrupt generation */
 	iowrite8(1 << mod->num, &mod->ctrl->int_enable);
 	return received;
@@ -1408,6 +1511,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;
@@ -1416,18 +1522,19 @@ 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;
 	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 */
 	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 +1549,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 +1574,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 +1770,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] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS

[Marc Kleine-Budde]

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

On 07/13/2012 05:20 PM, 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.
> 
> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> ---
> 
> This is a squashed together version of the first two patches in the series.
> 
>  drivers/net/can/janz-ican3.c |  156 +++++++++++++++++++++++++++++++++++-------
>  1 files changed, 130 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> index 08c893c..5fff829 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
> @@ -235,7 +238,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 +456,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));
> @@ -1091,6 +1092,87 @@ 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);

Please use { } on else, too

> +
> +	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;
> +
> +	/* check flag whether this packet has to be looped back */
> +	if (!(mod->ndev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^

Your flags have a IFF_ECHO, don't they?

> +		kfree_skb(skb);
> +		return 0;

please return here the dlc here, too.

> +	}
> +
> +	/* make settings for echo to reduce code in irq context */
> +	skb->protocol = htons(ETH_P_CAN);
> +	skb->pkt_type = PACKET_BROADCAST;
> +	skb->ip_summed = CHECKSUM_UNNECESSARY;
> +	skb->dev = mod->ndev;
> +
> +	cf = (struct can_frame *)skb->data;
> +	dlc = cf->can_dlc;
> +	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_cmp_echo_skb(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);
> +
> +	if (echo_skb) {
> +		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;
> +	}

Please restructure like this (more common coding style):

	if (!echo_skb)
		return false

	...
> +
> +	return false;
> +}
> +
> +/*
>   * Check that there is room in the TX ring to transmit another skb
>   *
>   * LOCKING: must hold mod->lock
> @@ -1100,6 +1182,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));
> @@ -1150,10 +1236,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_cmp_echo_skb(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 */
> @@ -1176,13 +1279,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;
> @@ -1199,11 +1302,6 @@ static int ican3_napi(struct napi_struct *napi, int budget)
>  		received++;
>  	}
>  
> -	/* We have processed all packets that the adapter had, but it
> -	 * was less than our budget, stop polling */
> -	if (received < budget)
> -		napi_complete(napi);
> -
>  	spin_lock_irqsave(&mod->lock, flags);
>  
>  	/* Wake up the transmit queue if necessary */
> @@ -1212,6 +1310,11 @@ static int ican3_napi(struct napi_struct *napi, int budget)
>  
>  	spin_unlock_irqrestore(&mod->lock, flags);
>  
> +	/* We have processed all packets that the adapter had, but it
> +	 * was less than our budget, stop polling */

Nitpick: preferred multi line commenting style is:

/*
 * comments...
 * ..more
 */
> +	if (received < budget)
> +		napi_complete(napi);
> +

Why are you moving the napi_complete?

>  	/* re-enable interrupt generation */
>  	iowrite8(1 << mod->num, &mod->ctrl->int_enable);
>  	return received;
> @@ -1408,6 +1511,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;
> @@ -1416,18 +1522,19 @@ 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;
>  	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 */
>  	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 +1549,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 +1574,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 +1770,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);

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] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS

[Ira W. Snyder]

On Wed, Jul 18, 2012 at 01:09:57AM +0200, Marc Kleine-Budde wrote:
> On 07/13/2012 05:20 PM, 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.
> > 
> > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> > ---
> > 
> > This is a squashed together version of the first two patches in the series.
> > 
> >  drivers/net/can/janz-ican3.c |  156 +++++++++++++++++++++++++++++++++++-------
> >  1 files changed, 130 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> > index 08c893c..5fff829 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
> > @@ -235,7 +238,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 +456,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));
> > @@ -1091,6 +1092,87 @@ 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);
> 
> Please use { } on else, too
> 

Ok. FWIW, this is present in drivers/net/can/dev.c can_put_echo_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;
> > +
> > +	/* check flag whether this packet has to be looped back */
> > +	if (!(mod->ndev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
>               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Your flags have a IFF_ECHO, don't they?
> 

I removed this check.

> > +		kfree_skb(skb);
> > +		return 0;
> 
> please return here the dlc here, too.
> 

Ok.

> > +	}
> > +
> > +	/* make settings for echo to reduce code in irq context */
> > +	skb->protocol = htons(ETH_P_CAN);
> > +	skb->pkt_type = PACKET_BROADCAST;
> > +	skb->ip_summed = CHECKSUM_UNNECESSARY;
> > +	skb->dev = mod->ndev;
> > +
> > +	cf = (struct can_frame *)skb->data;
> > +	dlc = cf->can_dlc;
> > +	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_cmp_echo_skb(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);
> > +
> > +	if (echo_skb) {
> > +		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;
> > +	}
> 
> Please restructure like this (more common coding style):
> 
> 	if (!echo_skb)
> 		return false
> 
> 	...

Ok.

> > +
> > +	return false;
> > +}
> > +
> > +/*
> >   * Check that there is room in the TX ring to transmit another skb
> >   *
> >   * LOCKING: must hold mod->lock
> > @@ -1100,6 +1182,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));
> > @@ -1150,10 +1236,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_cmp_echo_skb(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 */
> > @@ -1176,13 +1279,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;
> > @@ -1199,11 +1302,6 @@ static int ican3_napi(struct napi_struct *napi, int budget)
> >  		received++;
> >  	}
> >  
> > -	/* We have processed all packets that the adapter had, but it
> > -	 * was less than our budget, stop polling */
> > -	if (received < budget)
> > -		napi_complete(napi);
> > -
> >  	spin_lock_irqsave(&mod->lock, flags);
> >  
> >  	/* Wake up the transmit queue if necessary */
> > @@ -1212,6 +1310,11 @@ static int ican3_napi(struct napi_struct *napi, int budget)
> >  
> >  	spin_unlock_irqrestore(&mod->lock, flags);
> >  
> > +	/* We have processed all packets that the adapter had, but it
> > +	 * was less than our budget, stop polling */
> 
> Nitpick: preferred multi line commenting style is:
> 
> /*
>  * comments...
>  * ..more
>  */

Whoops. Fixed.

> > +	if (received < budget)
> > +		napi_complete(napi);
> > +
> 
> Why are you moving the napi_complete?
> 

Should the TX queue be restarted before or after calling
napi_complete()? Does it matter? I can leave the napi_complete() where
it was if it doesn't matter.

I'll post up a new series with these improvements incorporated after I
have an answer to the above question.

Thanks for the review,
Ira

> >  	/* re-enable interrupt generation */
> >  	iowrite8(1 << mod->num, &mod->ctrl->int_enable);
> >  	return received;
> > @@ -1408,6 +1511,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;
> > @@ -1416,18 +1522,19 @@ 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;
> >  	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 */
> >  	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 +1549,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 +1574,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 +1770,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);
> 
> 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   |
> 

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

[Marc Kleine-Budde]

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

On 07/18/2012 07:47 PM, Ira W. Snyder wrote:
> On Wed, Jul 18, 2012 at 01:09:57AM +0200, Marc Kleine-Budde wrote:
>> On 07/13/2012 05:20 PM, 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.
>>>
>>> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
>>> ---
>>>
>>> This is a squashed together version of the first two patches in the series.
>>>
>>>  drivers/net/can/janz-ican3.c |  156 +++++++++++++++++++++++++++++++++++-------
>>>  1 files changed, 130 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
>>> index 08c893c..5fff829 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
>>> @@ -235,7 +238,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 +456,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));
>>> @@ -1091,6 +1092,87 @@ 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);
>>
>> Please use { } on else, too
>>
> 
> Ok. FWIW, this is present in drivers/net/can/dev.c can_put_echo_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;
>>> +
>>> +	/* check flag whether this packet has to be looped back */
>>> +	if (!(mod->ndev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
>>               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> Your flags have a IFF_ECHO, don't they?
>>
> 
> I removed this check.
> 
>>> +		kfree_skb(skb);
>>> +		return 0;
>>
>> please return here the dlc here, too.
>>
> 
> Ok.
> 
>>> +	}
>>> +
>>> +	/* make settings for echo to reduce code in irq context */
>>> +	skb->protocol = htons(ETH_P_CAN);
>>> +	skb->pkt_type = PACKET_BROADCAST;
>>> +	skb->ip_summed = CHECKSUM_UNNECESSARY;
>>> +	skb->dev = mod->ndev;
>>> +
>>> +	cf = (struct can_frame *)skb->data;
>>> +	dlc = cf->can_dlc;
>>> +	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_cmp_echo_skb(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);
>>> +
>>> +	if (echo_skb) {
>>> +		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;
>>> +	}
>>
>> Please restructure like this (more common coding style):
>>
>> 	if (!echo_skb)
>> 		return false
>>
>> 	...
> 
> Ok.
> 
>>> +
>>> +	return false;
>>> +}
>>> +
>>> +/*
>>>   * Check that there is room in the TX ring to transmit another skb
>>>   *
>>>   * LOCKING: must hold mod->lock
>>> @@ -1100,6 +1182,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));
>>> @@ -1150,10 +1236,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_cmp_echo_skb(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 */
>>> @@ -1176,13 +1279,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;
>>> @@ -1199,11 +1302,6 @@ static int ican3_napi(struct napi_struct *napi, int budget)
>>>  		received++;
>>>  	}
>>>  
>>> -	/* We have processed all packets that the adapter had, but it
>>> -	 * was less than our budget, stop polling */
>>> -	if (received < budget)
>>> -		napi_complete(napi);
>>> -
>>>  	spin_lock_irqsave(&mod->lock, flags);
>>>  
>>>  	/* Wake up the transmit queue if necessary */
>>> @@ -1212,6 +1310,11 @@ static int ican3_napi(struct napi_struct *napi, int budget)
>>>  
>>>  	spin_unlock_irqrestore(&mod->lock, flags);
>>>  
>>> +	/* We have processed all packets that the adapter had, but it
>>> +	 * was less than our budget, stop polling */
>>
>> Nitpick: preferred multi line commenting style is:
>>
>> /*
>>  * comments...
>>  * ..more
>>  */
> 
> Whoops. Fixed.
> 
>>> +	if (received < budget)
>>> +		napi_complete(napi);
>>> +
>>
>> Why are you moving the napi_complete?
>>
> 
> Should the TX queue be restarted before or after calling
> napi_complete()? Does it matter? I can leave the napi_complete() where
> it was if it doesn't matter.

It does matter.

In a scenario where you call tx queue start before napi_complete,
imagine the following:
- call tx_queue_start, but not napi_complete, yet
- network layer calls xmit, CAN frame gets transmitted
- irq because you received a frame
- call napi schedule from irq handler
- then you call napi_complete from napi handler

This results in napi is not scheduled.

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

[PATCH 3/3] 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 supports this mode using CAN_CTRLMODE_ONE_SHOT.

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

This is trivial, and the driver might as well support this option.

 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 5fff829..d42f6f6 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;
 
@@ -1791,7 +1796,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 3/3] can: janz-ican3: add support for one shot mode

[Marc Kleine-Budde]

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

On 07/12/2012 06:15 PM, 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 supports this mode using CAN_CTRLMODE_ONE_SHOT.

What happens to the queued to-be-echoed CAN frames if the device fails
to send a CAN frame, due to one shot mode?

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/3] can: janz-ican3: add support for one shot mode

[Ira W. Snyder]

On Fri, Jul 13, 2012 at 10:59:18AM +0200, Marc Kleine-Budde wrote:
> On 07/12/2012 06:15 PM, 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 supports this mode using CAN_CTRLMODE_ONE_SHOT.
> 
> What happens to the queued to-be-echoed CAN frames if the device fails
> to send a CAN frame, due to one shot mode?
> 

I borrowed the hardware and tested this. I generated bus problems by
unplugging the cable from the card.

With "berr-reporting off" I get no indication from the firmware that
there is any problems until ERROR-WARNING (96 errors) or ERROR-PASSIVE
(128 errors) state is reached.

With "berr-reporting on", I get a bus-error message for every message I
try to send. It looks like this:

  can0  20000088  [8] 00 00 80 19 00 00 08 00   ERRORFRAME
        protocol-violation{{error-on-tx}{acknowledge-slot}}
        bus-error
        error-counter-tx-rx{{8}{0}}

The TX error count increases by 8 for each packet. My CAN book seems to
say this is the correct behavior when the bus is unconnected.

I fixed the echo skb problem (see my patch in reply to this) by always
enabling bus errors, and using those to know when packet transmission
failed. In testing, it works well. Please take that patch instead of
this one.

Thanks for the review.

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

[PATCH v2 3/3] 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 supports this mode using CAN_CTRLMODE_ONE_SHOT.

Every time a packet transmission failure happens, one packet needs to be
dropped from the front of the ECHO queue. This hardware lacks support
for TX-error interrupts, and so bus error indications are used as a
workaround. Every time a TX bus error is received, we know a packet
failed to transmit. If bus error reporting is off, do not forward the
bus error packets to userspace.

The rx_bytes and tx_bytes counters were audited to make sure they only
reflect actual data bytes received from the bus. They do not include
error packets.

The rx_errors and tx_errors counters were audited to make sure they are
correct in all situations. Previously, the rx_errors counts were
sometimes too high.

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

This patch should superceed the previous patch posted with this title.

 drivers/net/can/janz-ican3.c |   56 +++++++++++++++++++++++------------------
 1 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index 5fff829..fccfc3a 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;
 
@@ -911,7 +916,6 @@ static void ican3_handle_msglost(struct ican3_dev *mod, struct ican3_msg *msg)
 		cf->can_id |= CAN_ERR_CRTL;
 		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
 		stats->rx_errors++;
-		stats->rx_bytes += cf->can_dlc;
 		netif_rx(skb);
 	}
 }
@@ -928,7 +932,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;
 
@@ -949,6 +953,7 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 		return -ENOMEM;
 
 	isrc = msg->data[0];
+	ecc = msg->data[2];
 	status = msg->data[3];
 	rxerr = msg->data[4];
 	txerr = msg->data[5];
@@ -959,7 +964,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 */
@@ -981,11 +985,28 @@ 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");
+
+		/*
+		 * This hardware lacks any support other than bus error
+		 * messages to determine if the packet transmission failed.
+		 *
+		 * This is a TX error, so we free an echo skb from the front
+		 * of the queue.
+		 */
+		if ((ecc & ECC_DIR) == 0) {
+			cf->data[2] |= CAN_ERR_PROT_TX;
+			kfree_skb(skb_dequeue(&mod->echoq));
+			stats->tx_errors++;
+		}
+
+		/* bus error reporting is off, drop the error skb */
+		if (!(mod->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) {
+			kfree_skb(skb);
+			return 0;
+		}
+
 		mod->can.can_stats.bus_error++;
-		stats->rx_errors++;
 		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
 
 		switch (ecc & ECC_MASK) {
@@ -1004,9 +1025,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
 			break;
 		}
 
-		if ((ecc & ECC_DIR) == 0)
-			cf->data[2] |= CAN_ERR_PROT_TX;
-
 		cf->data[6] = txerr;
 		cf->data[7] = rxerr;
 	}
@@ -1031,8 +1049,8 @@ 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;
+	if (!(cf->data[2] & CAN_ERR_PROT_TX))
+		stats->rx_errors++;
 	netif_rx(skb);
 	return 0;
 }
@@ -1467,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) {
@@ -1791,7 +1796,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 v2 3/3] can: janz-ican3: add support for one shot mode

[Marc Kleine-Budde]

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

On 07/14/2012 01:07 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 supports this mode using CAN_CTRLMODE_ONE_SHOT.
> 
> Every time a packet transmission failure happens, one packet needs to be
> dropped from the front of the ECHO queue. This hardware lacks support
> for TX-error interrupts, and so bus error indications are used as a
> workaround. Every time a TX bus error is received, we know a packet
> failed to transmit. If bus error reporting is off, do not forward the
> bus error packets to userspace.

Can you please test in normal mode (i.e. non-oneshot-mode) on a proper
terminated bus without a second CAN station on the bus. Send a single
CAN message. How does the controller behave? What happens if you then
add a second member to the bus? Is the original CAN message finally
received?

With the scheme "drop frame from echo queue on tx error" in the worst
case you loose some echo messages, but at least the queue will not overflow.

> 
> The rx_bytes and tx_bytes counters were audited to make sure they only
> reflect actual data bytes received from the bus. They do not include
> error packets.
> 
> The rx_errors and tx_errors counters were audited to make sure they are
> correct in all situations. Previously, the rx_errors counts were
> sometimes too high.

Maybe split the _byte and _error fixes into a seperate patch (both fixes
can be in the same patch, though).
> 
> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> ---
> 
> This patch should superceed the previous patch posted with this title.
> 
>  drivers/net/can/janz-ican3.c |   56 +++++++++++++++++++++++------------------
>  1 files changed, 31 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> index 5fff829..fccfc3a 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;
> +

Is this a per-CAN-message flag, not controller wide setting?
(just curious)

>  	if (cf->can_id & CAN_RTR_FLAG)
>  		desc->data[0] |= ICAN3_EFF_RTR;
>  
> @@ -911,7 +916,6 @@ static void ican3_handle_msglost(struct ican3_dev *mod, struct ican3_msg *msg)
>  		cf->can_id |= CAN_ERR_CRTL;
>  		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
>  		stats->rx_errors++;
> -		stats->rx_bytes += cf->can_dlc;
>  		netif_rx(skb);
>  	}
>  }
> @@ -928,7 +932,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;
>  
> @@ -949,6 +953,7 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>  		return -ENOMEM;
>  
>  	isrc = msg->data[0];
> +	ecc = msg->data[2];
>  	status = msg->data[3];
>  	rxerr = msg->data[4];
>  	txerr = msg->data[5];
> @@ -959,7 +964,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 */
> @@ -981,11 +985,28 @@ 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");
> +
> +		/*
> +		 * This hardware lacks any support other than bus error
> +		 * messages to determine if the packet transmission failed.
> +		 *
> +		 * This is a TX error, so we free an echo skb from the front
> +		 * of the queue.
> +		 */
> +		if ((ecc & ECC_DIR) == 0) {

maybe:
    if (!(ecc & ECC_DIR)) {
then it's consistent with the if (!...)

> +			cf->data[2] |= CAN_ERR_PROT_TX;
> +			kfree_skb(skb_dequeue(&mod->echoq));
> +			stats->tx_errors++;
> +		}
> +
> +		/* bus error reporting is off, drop the error skb */
> +		if (!(mod->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) {
> +			kfree_skb(skb);
> +			return 0;
> +		}

Can you move the "if there is a tx-bus error, free echo queue" to the
front and return if bus error reporting is disabled? I don't like the
idea to alloc a skb and then free it, if it's not needed.

> +
>  		mod->can.can_stats.bus_error++;
> -		stats->rx_errors++;
>  		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>  
>  		switch (ecc & ECC_MASK) {
> @@ -1004,9 +1025,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>  			break;
>  		}
>  
> -		if ((ecc & ECC_DIR) == 0)
> -			cf->data[2] |= CAN_ERR_PROT_TX;
> -
>  		cf->data[6] = txerr;
>  		cf->data[7] = rxerr;
>  	}
> @@ -1031,8 +1049,8 @@ 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;
> +	if (!(cf->data[2] & CAN_ERR_PROT_TX))
> +		stats->rx_errors++;
>  	netif_rx(skb);
>  	return 0;
>  }
> @@ -1467,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;
> -	}

What happened to these two functions? Not needed anymore?

> -
>  	/* bring the bus online */
>  	ret = ican3_set_bus_state(mod, true);
>  	if (ret) {
> @@ -1791,7 +1796,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);

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 v2 3/3] can: janz-ican3: add support for one shot mode

[Ira W. Snyder]

On Wed, Jul 18, 2012 at 12:51:03AM +0200, Marc Kleine-Budde wrote:
> On 07/14/2012 01:07 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 supports this mode using CAN_CTRLMODE_ONE_SHOT.
> > 
> > Every time a packet transmission failure happens, one packet needs to be
> > dropped from the front of the ECHO queue. This hardware lacks support
> > for TX-error interrupts, and so bus error indications are used as a
> > workaround. Every time a TX bus error is received, we know a packet
> > failed to transmit. If bus error reporting is off, do not forward the
> > bus error packets to userspace.
> 
> Can you please test in normal mode (i.e. non-oneshot-mode) on a proper
> terminated bus without a second CAN station on the bus. Send a single
> CAN message. How does the controller behave? What happens if you then
> add a second member to the bus? Is the original CAN message finally
> received?
> 

I configured the bus like so:
ip link set can0 up type can bitrate 1000000 one-shot off berr-reporting off

And get this output:
21: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT qlen 10
    link/can
    can state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0
    bitrate 1000000 sample-point 0.750
    tq 125 prop-seg 2 phase-seg1 3 phase-seg2 2 sjw 1
    janz-ican3: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
    clock 8000000
    re-started bus-errors arbit-lost error-warn error-pass bus-off
    0          0          0          0          0          0
    RX: bytes  packets  errors  dropped overrun mcast
    0          0        0       0       0       0
    TX: bytes  packets  errors  dropped carrier collsns
    0          0        0       0       0       0

Then I ran:
cangen -n 1 -e -L 8 can0

The candump tool shows the following:
candump -e any,0:0,#FFFFFFFF
  can0  20000004  [8] 00 08 00 00 00 00 60 00   ERRORFRAME
        controller-problem{tx-error-warning}
        error-counter-tx-rx{{96}{0}}
  can0  20000004  [8] 00 20 00 00 00 00 80 00   ERRORFRAME
        controller-problem{tx-error-passive}
        error-counter-tx-rx{{128}{0}}

And the ip tool shows:
21: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT qlen 10
    link/can 
    can state ERROR-PASSIVE restart-ms 0 
    bitrate 1000000 sample-point 0.750 
    tq 125 prop-seg 2 phase-seg1 3 phase-seg2 2 sjw 1
    janz-ican3: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
    clock 8000000
    re-started bus-errors arbit-lost error-warn error-pass bus-off
    0          0          0          1          1          0         
    RX: bytes  packets  errors  dropped overrun mcast   
    0          0        2       0       0       0      
    TX: bytes  packets  errors  dropped carrier collsns 
    0          0        1822    0       0       0      


After I plug in another CAN station, the bus remains in exactly the same
state. I DO NOT receive the frame I sent, on either the sending station,
nor on the newly connected station. The bus remains in ERROR-PASSIVE
state.

If I send packets with my second CAN station, it starts getting bus
errors. The first CAN station acts as if it has dropped off the bus
completely, so there is no second device on the bus.

The firmware seems to go into some bad state where it will not respond
to control messages at all. It doesn't ever respond to control messages
again until I reset the card (by rmmod + insmod the driver).

> With the scheme "drop frame from echo queue on tx error" in the worst
> case you loose some echo messages, but at least the queue will not overflow.
> 
> > 
> > The rx_bytes and tx_bytes counters were audited to make sure they only
> > reflect actual data bytes received from the bus. They do not include
> > error packets.
> > 
> > The rx_errors and tx_errors counters were audited to make sure they are
> > correct in all situations. Previously, the rx_errors counts were
> > sometimes too high.
> 
> Maybe split the _byte and _error fixes into a seperate patch (both fixes
> can be in the same patch, though).

Ok.

> > 
> > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> > ---
> > 
> > This patch should superceed the previous patch posted with this title.
> > 
> >  drivers/net/can/janz-ican3.c |   56 +++++++++++++++++++++++------------------
> >  1 files changed, 31 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> > index 5fff829..fccfc3a 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;
> > +
> 
> Is this a per-CAN-message flag, not controller wide setting?
> (just curious)
> 

Yes, this setting is per-message. The ICAN3_ECHO bit is also
per-message.

> >  	if (cf->can_id & CAN_RTR_FLAG)
> >  		desc->data[0] |= ICAN3_EFF_RTR;
> >  
> > @@ -911,7 +916,6 @@ static void ican3_handle_msglost(struct ican3_dev *mod, struct ican3_msg *msg)
> >  		cf->can_id |= CAN_ERR_CRTL;
> >  		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> >  		stats->rx_errors++;
> > -		stats->rx_bytes += cf->can_dlc;
> >  		netif_rx(skb);
> >  	}
> >  }
> > @@ -928,7 +932,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;
> >  
> > @@ -949,6 +953,7 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
> >  		return -ENOMEM;
> >  
> >  	isrc = msg->data[0];
> > +	ecc = msg->data[2];
> >  	status = msg->data[3];
> >  	rxerr = msg->data[4];
> >  	txerr = msg->data[5];
> > @@ -959,7 +964,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 */
> > @@ -981,11 +985,28 @@ 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");
> > +
> > +		/*
> > +		 * This hardware lacks any support other than bus error
> > +		 * messages to determine if the packet transmission failed.
> > +		 *
> > +		 * This is a TX error, so we free an echo skb from the front
> > +		 * of the queue.
> > +		 */
> > +		if ((ecc & ECC_DIR) == 0) {
> 
> maybe:
>     if (!(ecc & ECC_DIR)) {
> then it's consistent with the if (!...)
> 

Ok.

> > +			cf->data[2] |= CAN_ERR_PROT_TX;
> > +			kfree_skb(skb_dequeue(&mod->echoq));
> > +			stats->tx_errors++;
> > +		}
> > +
> > +		/* bus error reporting is off, drop the error skb */
> > +		if (!(mod->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) {
> > +			kfree_skb(skb);
> > +			return 0;
> > +		}
> 
> Can you move the "if there is a tx-bus error, free echo queue" to the
> front and return if bus error reporting is disabled? I don't like the
> idea to alloc a skb and then free it, if it's not needed.
> 

Sure. That sounds good to me.

I will have to duplicate the "ecc & ECC_DIR" check twice in the code:
1) determine if this is a TX bus error to drop the echo packet
2) after error skb allocation, for "cf->data[2] |= CAN_ERR_PROT_TX"

> > +
> >  		mod->can.can_stats.bus_error++;
> > -		stats->rx_errors++;
> >  		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> >  
> >  		switch (ecc & ECC_MASK) {
> > @@ -1004,9 +1025,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
> >  			break;
> >  		}
> >  
> > -		if ((ecc & ECC_DIR) == 0)
> > -			cf->data[2] |= CAN_ERR_PROT_TX;
> > -
> >  		cf->data[6] = txerr;
> >  		cf->data[7] = rxerr;
> >  	}
> > @@ -1031,8 +1049,8 @@ 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;
> > +	if (!(cf->data[2] & CAN_ERR_PROT_TX))
> > +		stats->rx_errors++;
> >  	netif_rx(skb);
> >  	return 0;
> >  }
> > @@ -1467,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;
> > -	}
> 
> What happened to these two functions? Not needed anymore?
> 

If you look through the initialization ican3_startup_module(), the
ican3_set_buserror() function is still used.

The code above *disables* the hardware from generating bus errors if
CAN_CTRLMODE_BERR_REPORTING is not set. I am now using bus errors to
determine if a TX error happened. They must always be enabled, so this
code was removed.

Thanks for the review,
Ira

> > -
> >  	/* bring the bus online */
> >  	ret = ican3_set_bus_state(mod, true);
> >  	if (ret) {
> > @@ -1791,7 +1796,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);
> 
> 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   |
> 

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

[Ira W. Snyder]

On Wed, Jul 18, 2012 at 11:20:48AM -0700, Ira W. Snyder wrote:
> On Wed, Jul 18, 2012 at 12:51:03AM +0200, Marc Kleine-Budde wrote:
> > On 07/14/2012 01:07 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 supports this mode using CAN_CTRLMODE_ONE_SHOT.
> > > 
> > > Every time a packet transmission failure happens, one packet needs to be
> > > dropped from the front of the ECHO queue. This hardware lacks support
> > > for TX-error interrupts, and so bus error indications are used as a
> > > workaround. Every time a TX bus error is received, we know a packet
> > > failed to transmit. If bus error reporting is off, do not forward the
> > > bus error packets to userspace.
> > 
> > Can you please test in normal mode (i.e. non-oneshot-mode) on a proper
> > terminated bus without a second CAN station on the bus. Send a single
> > CAN message. How does the controller behave? What happens if you then
> > add a second member to the bus? Is the original CAN message finally
> > received?
> > 
> 
> I configured the bus like so:
> ip link set can0 up type can bitrate 1000000 one-shot off berr-reporting off
> 
> And get this output:
> 21: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT qlen 10
>     link/can
>     can state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0
>     bitrate 1000000 sample-point 0.750
>     tq 125 prop-seg 2 phase-seg1 3 phase-seg2 2 sjw 1
>     janz-ican3: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
>     clock 8000000
>     re-started bus-errors arbit-lost error-warn error-pass bus-off
>     0          0          0          0          0          0
>     RX: bytes  packets  errors  dropped overrun mcast
>     0          0        0       0       0       0
>     TX: bytes  packets  errors  dropped carrier collsns
>     0          0        0       0       0       0
> 
> Then I ran:
> cangen -n 1 -e -L 8 can0
> 
> The candump tool shows the following:
> candump -e any,0:0,#FFFFFFFF
>   can0  20000004  [8] 00 08 00 00 00 00 60 00   ERRORFRAME
>         controller-problem{tx-error-warning}
>         error-counter-tx-rx{{96}{0}}
>   can0  20000004  [8] 00 20 00 00 00 00 80 00   ERRORFRAME
>         controller-problem{tx-error-passive}
>         error-counter-tx-rx{{128}{0}}
> 
> And the ip tool shows:
> 21: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT qlen 10
>     link/can 
>     can state ERROR-PASSIVE restart-ms 0 
>     bitrate 1000000 sample-point 0.750 
>     tq 125 prop-seg 2 phase-seg1 3 phase-seg2 2 sjw 1
>     janz-ican3: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
>     clock 8000000
>     re-started bus-errors arbit-lost error-warn error-pass bus-off
>     0          0          0          1          1          0         
>     RX: bytes  packets  errors  dropped overrun mcast   
>     0          0        2       0       0       0      
>     TX: bytes  packets  errors  dropped carrier collsns 
>     0          0        1822    0       0       0      
> 
> 
> After I plug in another CAN station, the bus remains in exactly the same
> state. I DO NOT receive the frame I sent, on either the sending station,
> nor on the newly connected station. The bus remains in ERROR-PASSIVE
> state.
> 
> If I send packets with my second CAN station, it starts getting bus
> errors. The first CAN station acts as if it has dropped off the bus
> completely, so there is no second device on the bus.
> 
> The firmware seems to go into some bad state where it will not respond
> to control messages at all. It doesn't ever respond to control messages
> again until I reset the card (by rmmod + insmod the driver).
> 

I investigated this case further. There is some vague wording in the
manual which states that if you permanently enable bus errors (infinite
quota) and your host CPU cannot keep up, it will disable the errors.

What the manual doesn't say is that the errors seem to be disabled
permanently until you perform a hard reset of the controller.

I found a workaround for this, by setting a quota of 1 bus error. Every
time the controller generates a bus error, I re-enable bus errors with a
quota of 1.

This seems to work well. Rather than going into a bad state, the
firmware keeps responding to control messages. The transmit error
counter keeps increasing until the both my CAN stations are plugged into
the bus. At this point, the packet is transmitted successfully, and both
CAN stations see the message.

The output from candump now looks like this:

# I used cangen to send a single packet from can0 here

  can0  20000004  [8] 00 08 00 00 00 00 60 00   ERRORFRAME
        controller-problem{tx-error-warning}
        error-counter-tx-rx{{96}{0}}
  can0  20000004  [8] 00 20 00 00 00 00 80 00   ERRORFRAME
        controller-problem{tx-error-passive}
        error-counter-tx-rx{{128}{0}}

# I connected both can stations (can0 and can1) onto the bus here

  can0  1B916562  [8] 68 83 C3 5C 78 A9 D4 3C
  can1  1B916562  [8] 68 83 C3 5C 78 A9 D4 3C

# The packet I sent was finally received by both stations. can0 got the
# echo packet, and can1 got the packet as received over the bus.

I'll post the patch series now with the fixes suggested.

Thanks for the suggestion to test this.
Ira

> > With the scheme "drop frame from echo queue on tx error" in the worst
> > case you loose some echo messages, but at least the queue will not overflow.
> > 
> > > 
> > > The rx_bytes and tx_bytes counters were audited to make sure they only
> > > reflect actual data bytes received from the bus. They do not include
> > > error packets.
> > > 
> > > The rx_errors and tx_errors counters were audited to make sure they are
> > > correct in all situations. Previously, the rx_errors counts were
> > > sometimes too high.
> > 
> > Maybe split the _byte and _error fixes into a seperate patch (both fixes
> > can be in the same patch, though).
> 
> Ok.
> 
> > > 
> > > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> > > ---
> > > 
> > > This patch should superceed the previous patch posted with this title.
> > > 
> > >  drivers/net/can/janz-ican3.c |   56 +++++++++++++++++++++++------------------
> > >  1 files changed, 31 insertions(+), 25 deletions(-)
> > > 
> > > diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> > > index 5fff829..fccfc3a 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;
> > > +
> > 
> > Is this a per-CAN-message flag, not controller wide setting?
> > (just curious)
> > 
> 
> Yes, this setting is per-message. The ICAN3_ECHO bit is also
> per-message.
> 
> > >  	if (cf->can_id & CAN_RTR_FLAG)
> > >  		desc->data[0] |= ICAN3_EFF_RTR;
> > >  
> > > @@ -911,7 +916,6 @@ static void ican3_handle_msglost(struct ican3_dev *mod, struct ican3_msg *msg)
> > >  		cf->can_id |= CAN_ERR_CRTL;
> > >  		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
> > >  		stats->rx_errors++;
> > > -		stats->rx_bytes += cf->can_dlc;
> > >  		netif_rx(skb);
> > >  	}
> > >  }
> > > @@ -928,7 +932,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;
> > >  
> > > @@ -949,6 +953,7 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
> > >  		return -ENOMEM;
> > >  
> > >  	isrc = msg->data[0];
> > > +	ecc = msg->data[2];
> > >  	status = msg->data[3];
> > >  	rxerr = msg->data[4];
> > >  	txerr = msg->data[5];
> > > @@ -959,7 +964,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 */
> > > @@ -981,11 +985,28 @@ 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");
> > > +
> > > +		/*
> > > +		 * This hardware lacks any support other than bus error
> > > +		 * messages to determine if the packet transmission failed.
> > > +		 *
> > > +		 * This is a TX error, so we free an echo skb from the front
> > > +		 * of the queue.
> > > +		 */
> > > +		if ((ecc & ECC_DIR) == 0) {
> > 
> > maybe:
> >     if (!(ecc & ECC_DIR)) {
> > then it's consistent with the if (!...)
> > 
> 
> Ok.
> 
> > > +			cf->data[2] |= CAN_ERR_PROT_TX;
> > > +			kfree_skb(skb_dequeue(&mod->echoq));
> > > +			stats->tx_errors++;
> > > +		}
> > > +
> > > +		/* bus error reporting is off, drop the error skb */
> > > +		if (!(mod->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) {
> > > +			kfree_skb(skb);
> > > +			return 0;
> > > +		}
> > 
> > Can you move the "if there is a tx-bus error, free echo queue" to the
> > front and return if bus error reporting is disabled? I don't like the
> > idea to alloc a skb and then free it, if it's not needed.
> > 
> 
> Sure. That sounds good to me.
> 
> I will have to duplicate the "ecc & ECC_DIR" check twice in the code:
> 1) determine if this is a TX bus error to drop the echo packet
> 2) after error skb allocation, for "cf->data[2] |= CAN_ERR_PROT_TX"
> 
> > > +
> > >  		mod->can.can_stats.bus_error++;
> > > -		stats->rx_errors++;
> > >  		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> > >  
> > >  		switch (ecc & ECC_MASK) {
> > > @@ -1004,9 +1025,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
> > >  			break;
> > >  		}
> > >  
> > > -		if ((ecc & ECC_DIR) == 0)
> > > -			cf->data[2] |= CAN_ERR_PROT_TX;
> > > -
> > >  		cf->data[6] = txerr;
> > >  		cf->data[7] = rxerr;
> > >  	}
> > > @@ -1031,8 +1049,8 @@ 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;
> > > +	if (!(cf->data[2] & CAN_ERR_PROT_TX))
> > > +		stats->rx_errors++;
> > >  	netif_rx(skb);
> > >  	return 0;
> > >  }
> > > @@ -1467,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;
> > > -	}
> > 
> > What happened to these two functions? Not needed anymore?
> > 
> 
> If you look through the initialization ican3_startup_module(), the
> ican3_set_buserror() function is still used.
> 
> The code above *disables* the hardware from generating bus errors if
> CAN_CTRLMODE_BERR_REPORTING is not set. I am now using bus errors to
> determine if a TX error happened. They must always be enabled, so this
> code was removed.
> 
> Thanks for the review,
> Ira
> 
> > > -
> > >  	/* bring the bus online */
> > >  	ret = ican3_set_bus_state(mod, true);
> > >  	if (ret) {
> > > @@ -1791,7 +1796,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);
> > 
> > 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   |
> > 
> 
> 

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

[Marc Kleine-Budde]

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

On 07/18/2012 08:20 PM, Ira W. Snyder wrote:
[...]

>> Can you please test in normal mode (i.e. non-oneshot-mode) on a proper
>> terminated bus without a second CAN station on the bus. Send a single
>> CAN message. How does the controller behave? What happens if you then
>> add a second member to the bus? Is the original CAN message finally
>> received?
>>
> 
> I configured the bus like so:
> ip link set can0 up type can bitrate 1000000 one-shot off berr-reporting off
> 
> And get this output:
> 21: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT qlen 10
>     link/can
>     can state ERROR-ACTIVE (berr-counter tx 0 rx 0) restart-ms 0
>     bitrate 1000000 sample-point 0.750
>     tq 125 prop-seg 2 phase-seg1 3 phase-seg2 2 sjw 1
>     janz-ican3: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
>     clock 8000000
>     re-started bus-errors arbit-lost error-warn error-pass bus-off
>     0          0          0          0          0          0
>     RX: bytes  packets  errors  dropped overrun mcast
>     0          0        0       0       0       0
>     TX: bytes  packets  errors  dropped carrier collsns
>     0          0        0       0       0       0
> 
> Then I ran:
> cangen -n 1 -e -L 8 can0
> 
> The candump tool shows the following:
> candump -e any,0:0,#FFFFFFFF
>   can0  20000004  [8] 00 08 00 00 00 00 60 00   ERRORFRAME
>         controller-problem{tx-error-warning}
>         error-counter-tx-rx{{96}{0}}
>   can0  20000004  [8] 00 20 00 00 00 00 80 00   ERRORFRAME
>         controller-problem{tx-error-passive}
>         error-counter-tx-rx{{128}{0}}
> 
> And the ip tool shows:
> 21: can0: <NOARP,UP,LOWER_UP,ECHO> mtu 16 qdisc pfifo_fast state UNKNOWN mode DEFAULT qlen 10
>     link/can 
>     can state ERROR-PASSIVE restart-ms 0 
>     bitrate 1000000 sample-point 0.750 
>     tq 125 prop-seg 2 phase-seg1 3 phase-seg2 2 sjw 1
>     janz-ican3: tseg1 1..16 tseg2 1..8 sjw 1..4 brp 1..64 brp-inc 1
>     clock 8000000
>     re-started bus-errors arbit-lost error-warn error-pass bus-off
>     0          0          0          1          1          0         
>     RX: bytes  packets  errors  dropped overrun mcast   
>     0          0        2       0       0       0      
>     TX: bytes  packets  errors  dropped carrier collsns 
>     0          0        1822    0       0       0      
> 
> 
> After I plug in another CAN station, the bus remains in exactly the same
> state. I DO NOT receive the frame I sent, on either the sending station,
> nor on the newly connected station. The bus remains in ERROR-PASSIVE
> state.
> 
> If I send packets with my second CAN station, it starts getting bus
> errors. The first CAN station acts as if it has dropped off the bus
> completely, so there is no second device on the bus.
> 
> The firmware seems to go into some bad state where it will not respond
> to control messages at all. It doesn't ever respond to control messages
> again until I reset the card (by rmmod + insmod the driver).

Uhh, error handling is always complicated and controllers behave in a
strange way :)

[...]

>>> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
>>> index 5fff829..fccfc3a 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;
>>> +
>>
>> Is this a per-CAN-message flag, not controller wide setting?
>> (just curious)
>>
> 
> Yes, this setting is per-message. The ICAN3_ECHO bit is also
> per-message.
> 
>>>  	if (cf->can_id & CAN_RTR_FLAG)
>>>  		desc->data[0] |= ICAN3_EFF_RTR;
>>>  
>>> @@ -911,7 +916,6 @@ static void ican3_handle_msglost(struct ican3_dev *mod, struct ican3_msg *msg)
>>>  		cf->can_id |= CAN_ERR_CRTL;
>>>  		cf->data[1] = CAN_ERR_CRTL_RX_OVERFLOW;
>>>  		stats->rx_errors++;
>>> -		stats->rx_bytes += cf->can_dlc;
>>>  		netif_rx(skb);
>>>  	}
>>>  }
>>> @@ -928,7 +932,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;
>>>  
>>> @@ -949,6 +953,7 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>>>  		return -ENOMEM;
>>>  
>>>  	isrc = msg->data[0];
>>> +	ecc = msg->data[2];
>>>  	status = msg->data[3];
>>>  	rxerr = msg->data[4];
>>>  	txerr = msg->data[5];
>>> @@ -959,7 +964,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 */
>>> @@ -981,11 +985,28 @@ 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");
>>> +
>>> +		/*
>>> +		 * This hardware lacks any support other than bus error
>>> +		 * messages to determine if the packet transmission failed.
>>> +		 *
>>> +		 * This is a TX error, so we free an echo skb from the front
>>> +		 * of the queue.
>>> +		 */
>>> +		if ((ecc & ECC_DIR) == 0) {
>>
>> maybe:
>>     if (!(ecc & ECC_DIR)) {
>> then it's consistent with the if (!...)
>>
> 
> Ok.
> 
>>> +			cf->data[2] |= CAN_ERR_PROT_TX;
>>> +			kfree_skb(skb_dequeue(&mod->echoq));
>>> +			stats->tx_errors++;
>>> +		}
>>> +
>>> +		/* bus error reporting is off, drop the error skb */
>>> +		if (!(mod->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING)) {
>>> +			kfree_skb(skb);
>>> +			return 0;
>>> +		}
>>
>> Can you move the "if there is a tx-bus error, free echo queue" to the
>> front and return if bus error reporting is disabled? I don't like the
>> idea to alloc a skb and then free it, if it's not needed.
>>
> 
> Sure. That sounds good to me.
> 
> I will have to duplicate the "ecc & ECC_DIR" check twice in the code:
> 1) determine if this is a TX bus error to drop the echo packet
> 2) after error skb allocation, for "cf->data[2] |= CAN_ERR_PROT_TX"

Having a "ecc & ECC_DIR" is probably much, much more cheaper than to
allocate an skb and free it. The compiler has the chance to optimize the
code if you have two "ecc & ECC_DIR", but it cannot optimize skb
alloc+free :)

> 
>>> +
>>>  		mod->can.can_stats.bus_error++;
>>> -		stats->rx_errors++;
>>>  		cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>>>  
>>>  		switch (ecc & ECC_MASK) {
>>> @@ -1004,9 +1025,6 @@ static int ican3_handle_cevtind(struct ican3_dev *mod, struct ican3_msg *msg)
>>>  			break;
>>>  		}
>>>  
>>> -		if ((ecc & ECC_DIR) == 0)
>>> -			cf->data[2] |= CAN_ERR_PROT_TX;
>>> -
>>>  		cf->data[6] = txerr;
>>>  		cf->data[7] = rxerr;
>>>  	}
>>> @@ -1031,8 +1049,8 @@ 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;
>>> +	if (!(cf->data[2] & CAN_ERR_PROT_TX))
>>> +		stats->rx_errors++;
>>>  	netif_rx(skb);
>>>  	return 0;
>>>  }
>>> @@ -1467,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;
>>> -	}
>>
>> What happened to these two functions? Not needed anymore?
>>
> 
> If you look through the initialization ican3_startup_module(), the
> ican3_set_buserror() function is still used.
> 
> The code above *disables* the hardware from generating bus errors if
> CAN_CTRLMODE_BERR_REPORTING is not set. I am now using bus errors to
> determine if a TX error happened. They must always be enabled, so this
> code was removed.

Thanks for the explanation.

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