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

This is another different approach to fixing the Janz ICAN3 support for
CAN_RAW_RECV_OWN_MSGS.

The can_put_echo_skb() function is changed to always keep all packets
until can_get_echo_skb() is called. Previously, it would drop packets if
they were not needed to be looped back. This makes it possible for the
new function can_cmp_echo_skb() to work with hardware-assisted loopback
support on the Janz ICAN3, which does not have TX-complete interrupts of
any kind.

Since we are now storing packets even in the non-loopback case, there is
some extra memory overhead, to store the extra packets between the calls
to can_put_echo_skb() and can_get_echo_skb().

After this patch series is applied, the SocketCAN tst-rcv-own-msgs test
passes.

Performance is rougly 15% less than using the previously posted patch:
[PATCH ALTERNATE VERSION 1/1] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS

This performance drop is due to the extra overhead of receiving the echo
packets from the card itself. This involves one extra interrupt for each
packet sent, and the associated overhead of running ican3_napi() for
each packet sent.

Ira W. Snyder (3):
  can: make the echo stack keep packet information longer
  can: add can_cmp_echo_skb() for echo skb comparison
  can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS

 drivers/net/can/dev.c        |   75 ++++++++++++++++++++++++++++++-----------
 drivers/net/can/janz-ican3.c |   56 +++++++++++++++++--------------
 include/linux/can/dev.h      |    2 +
 3 files changed, 87 insertions(+), 46 deletions(-)

-- 
1.7.8.6

[PATCH 1/3] can: make the echo stack keep packet information longer

[Ira W. Snyder]

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

In order for the can_cmp_echo_skb() function to be able to detect
packets received via the hardware loopback feature, all packets must be
saved. This means we cannot drop non-loopback packets until reception
time.
---
 drivers/net/can/dev.c |   41 +++++++++++++++++++++--------------------
 1 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
index c5fe3a3..98e6c50 100644
--- a/drivers/net/can/dev.c
+++ b/drivers/net/can/dev.c
@@ -282,12 +282,6 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 
 	BUG_ON(idx >= priv->echo_skb_max);
 
-	/* check flag whether this packet has to be looped back */
-	if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
-		kfree_skb(skb);
-		return;
-	}
-
 	if (!priv->echo_skb[idx]) {
 		struct sock *srcsk = skb->sk;
 
@@ -303,12 +297,6 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
 
 		skb->sk = srcsk;
 
-		/* 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;
-
 		/* save this skb for tx interrupt echo handling */
 		priv->echo_skb[idx] = skb;
 	} else {
@@ -329,21 +317,34 @@ EXPORT_SYMBOL_GPL(can_put_echo_skb);
 unsigned int can_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 (priv->echo_skb[idx]) {
-		struct sk_buff *skb = priv->echo_skb[idx];
-		struct can_frame *cf = (struct can_frame *)skb->data;
-		u8 dlc = cf->can_dlc;
+	if (!skb)
+		return 0;
 
-		netif_rx(priv->echo_skb[idx]);
-		priv->echo_skb[idx] = NULL;
+	priv->echo_skb[idx] = NULL;
+	cf = (struct can_frame *)skb->data;
 
-		return dlc;
+	/* 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;
 	}
 
-	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_rx(skb);
+
+	return dlc;
 }
 EXPORT_SYMBOL_GPL(can_get_echo_skb);
 
-- 
1.7.8.6

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

[Ira W. Snyder]

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

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

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

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

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

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

diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
index 08c893c..a49af40 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));
@@ -1150,6 +1148,21 @@ static int ican3_recv_skb(struct ican3_dev *mod)
 	/* convert the ICAN3 frame into Linux CAN format */
 	ican3_to_can_frame(mod, &desc, cf);
 
+	/*
+	 * 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.
+	 *
+	 * Also, the netdevice queue needs to be allowed to send packets again.
+	 */
+	if (can_cmp_echo_skb(skb, ndev, 0)) {
+		stats->rx_packets++;
+		stats->rx_bytes += can_get_echo_skb(ndev, 0);
+		kfree_skb(skb);
+		netif_wake_queue(ndev);
+		goto err_noalloc;
+	}
+
 	/* receive the skb, update statistics */
 	netif_receive_skb(skb);
 	stats->rx_packets++;
@@ -1177,7 +1190,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 +1216,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;
@@ -1422,12 +1426,14 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 	void __iomem *desc_addr;
 	unsigned long flags;
 
+	if (can_dropped_invalid_skb(ndev, skb))
+		return NETDEV_TX_OK;
+
 	spin_lock_irqsave(&mod->lock, flags);
 
 	/* check that we can actually transmit */
 	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 +1448,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);
+	can_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
@@ -1462,18 +1478,6 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
 	/* update statistics */
 	stats->tx_packets++;
 	stats->tx_bytes += cf->can_dlc;
-	kfree_skb(skb);
-
-	/*
-	 * This hardware doesn't have TX-done notifications, so we'll try and
-	 * 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 +1658,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 3/3] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS

[Oliver Hartkopp]

On 09.07.2012 21:56, 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 can_cmp_echo_skb(), we can drop the loopback
> messages and return the original skbs. This fixes the issues with
> CAN_RAW_RECV_OWN_MSGS.
> 
> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> ---
>  drivers/net/can/janz-ican3.c |   56 ++++++++++++++++++++++-------------------
>  1 files changed, 30 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> index 08c893c..a49af40 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));
> @@ -1150,6 +1148,21 @@ static int ican3_recv_skb(struct ican3_dev *mod)
>  	/* convert the ICAN3 frame into Linux CAN format */
>  	ican3_to_can_frame(mod, &desc, cf);
>  
> +	/*
> +	 * 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.
> +	 *
> +	 * Also, the netdevice queue needs to be allowed to send packets again.
> +	 */
> +	if (can_cmp_echo_skb(skb, ndev, 0)) {
> +		stats->rx_packets++;
> +		stats->rx_bytes += can_get_echo_skb(ndev, 0);
> +		kfree_skb(skb);
> +		netif_wake_queue(ndev);
> +		goto err_noalloc;
> +	}
> +
>  	/* receive the skb, update statistics */
>  	netif_receive_skb(skb);
>  	stats->rx_packets++;
> @@ -1177,7 +1190,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 +1216,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;
> @@ -1422,12 +1426,14 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	void __iomem *desc_addr;
>  	unsigned long flags;
>  
> +	if (can_dropped_invalid_skb(ndev, skb))
> +		return NETDEV_TX_OK;
> +
>  	spin_lock_irqsave(&mod->lock, flags);
>  
>  	/* check that we can actually transmit */
>  	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 +1448,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);
> +	can_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
> @@ -1462,18 +1478,6 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	/* update statistics */
>  	stats->tx_packets++;
>  	stats->tx_bytes += cf->can_dlc;


I think these statistic updates double the traffic as you do them in
ican3_recv_skb() again.

> -	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 +1658,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;

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

[Oliver Hartkopp]

Hello Ira,

looks good to me in general ...

Let's wait for other comments though.

Does it work without problems?

On 09.07.2012 21:56, Ira W. Snyder wrote:

> From: "Ira W. Snyder" <iws@ovro.caltech.edu>
> 
> This is another different approach to fixing the Janz ICAN3 support for
> CAN_RAW_RECV_OWN_MSGS.
> 
> The can_put_echo_skb() function is changed to always keep all packets
> until can_get_echo_skb() is called. Previously, it would drop packets if
> they were not needed to be looped back. This makes it possible for the
> new function can_cmp_echo_skb() to work with hardware-assisted loopback
> support on the Janz ICAN3, which does not have TX-complete interrupts of
> any kind.
> 
> Since we are now storing packets even in the non-loopback case, there is
> some extra memory overhead, to store the extra packets between the calls
> to can_put_echo_skb() and can_get_echo_skb().
> 
> After this patch series is applied, the SocketCAN tst-rcv-own-msgs test
> passes.
> 
> Performance is rougly 15% less than using the previously posted patch:
> [PATCH ALTERNATE VERSION 1/1] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS
> 
> This performance drop is due to the extra overhead of receiving the echo
> packets from the card itself. This involves one extra interrupt for each
> packet sent, and the associated overhead of running ican3_napi() for
> each packet sent.
> 


If it works without problems you might try to use 2 or 3 echo_skbs for testing
to ensure the Janz card gets lined with outgoing traffic without waiting for
each single sent frame.

I assume this would make the implementation slightly complexer but may solve
the performance drop on the wire.

> Ira W. Snyder (3):
>   can: make the echo stack keep packet information longer
>   can: add can_cmp_echo_skb() for echo skb comparison
>   can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS
> 
>  drivers/net/can/dev.c        |   75 ++++++++++++++++++++++++++++++-----------
>  drivers/net/can/janz-ican3.c |   56 +++++++++++++++++--------------
>  include/linux/can/dev.h      |    2 +
>  3 files changed, 87 insertions(+), 46 deletions(-)
> 

Re: [PATCH 1/3] can: make the echo stack keep packet information longer

[Oliver Hartkopp]

On 09.07.2012 21:56, Ira W. Snyder wrote:

> From: "Ira W. Snyder" <iws@ovro.caltech.edu>
> 
> In order for the can_cmp_echo_skb() function to be able to detect
> packets received via the hardware loopback feature, all packets must be
> saved. This means we cannot drop non-loopback packets until reception
> time.
> ---
>  drivers/net/can/dev.c |   41 +++++++++++++++++++++--------------------
>  1 files changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c
> index c5fe3a3..98e6c50 100644
> --- a/drivers/net/can/dev.c
> +++ b/drivers/net/can/dev.c
> @@ -282,12 +282,6 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>  
>  	BUG_ON(idx >= priv->echo_skb_max);
>  
> -	/* check flag whether this packet has to be looped back */
> -	if (!(dev->flags & IFF_ECHO) || skb->pkt_type != PACKET_LOOPBACK) {
> -		kfree_skb(skb);
> -		return;
> -	}
> -
>  	if (!priv->echo_skb[idx]) {
>  		struct sock *srcsk = skb->sk;
>  
> @@ -303,12 +297,6 @@ void can_put_echo_skb(struct sk_buff *skb, struct net_device *dev,
>  
>  		skb->sk = srcsk;
>  
> -		/* 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;
> -
>  		/* save this skb for tx interrupt echo handling */
>  		priv->echo_skb[idx] = skb;
>  	} else {
> @@ -329,21 +317,34 @@ EXPORT_SYMBOL_GPL(can_put_echo_skb);
>  unsigned int can_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 (priv->echo_skb[idx]) {
> -		struct sk_buff *skb = priv->echo_skb[idx];
> -		struct can_frame *cf = (struct can_frame *)skb->data;
> -		u8 dlc = cf->can_dlc;
> +	if (!skb)
> +		return 0;
>  
> -		netif_rx(priv->echo_skb[idx]);
> -		priv->echo_skb[idx] = NULL;
> +	priv->echo_skb[idx] = NULL;
> +	cf = (struct can_frame *)skb->data;
>  
> -		return dlc;
> +	/* 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;
>  	}
>  
> -	return 0;
> +	/* make settings for echo to reduce code in irq context */


This comment became pointless :-)

> +	skb->protocol = htons(ETH_P_CAN);
> +	skb->pkt_type = PACKET_BROADCAST;
> +	skb->ip_summed = CHECKSUM_UNNECESSARY;
> +	skb->dev = dev;


Hm - it's proably worth to check, if we really need to set all these values or if

	skb->pkt_type = PACKET_BROADCAST;

would just be enough.

Will do so - but this is not relevant for your current patch.

> +
> +	dlc = cf->can_dlc;
> +	netif_rx(skb);
> +
> +	return dlc;
>  }
>  EXPORT_SYMBOL_GPL(can_get_echo_skb);
>  


Regards,
Oliver

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

[Ira W. Snyder]

On Mon, Jul 09, 2012 at 10:57:28PM +0200, Oliver Hartkopp wrote:
> On 09.07.2012 21:56, 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 can_cmp_echo_skb(), we can drop the loopback
> > messages and return the original skbs. This fixes the issues with
> > CAN_RAW_RECV_OWN_MSGS.
> > 
> > Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
> > ---
> >  drivers/net/can/janz-ican3.c |   56 ++++++++++++++++++++++-------------------
> >  1 files changed, 30 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
> > index 08c893c..a49af40 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));
> > @@ -1150,6 +1148,21 @@ static int ican3_recv_skb(struct ican3_dev *mod)
> >  	/* convert the ICAN3 frame into Linux CAN format */
> >  	ican3_to_can_frame(mod, &desc, cf);
> >  
> > +	/*
> > +	 * 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.
> > +	 *
> > +	 * Also, the netdevice queue needs to be allowed to send packets again.
> > +	 */
> > +	if (can_cmp_echo_skb(skb, ndev, 0)) {
> > +		stats->rx_packets++;
> > +		stats->rx_bytes += can_get_echo_skb(ndev, 0);
> > +		kfree_skb(skb);
> > +		netif_wake_queue(ndev);
> > +		goto err_noalloc;
> > +	}
> > +
> >  	/* receive the skb, update statistics */
> >  	netif_receive_skb(skb);
> >  	stats->rx_packets++;
> > @@ -1177,7 +1190,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 +1216,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;
> > @@ -1422,12 +1426,14 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
> >  	void __iomem *desc_addr;
> >  	unsigned long flags;
> >  
> > +	if (can_dropped_invalid_skb(ndev, skb))
> > +		return NETDEV_TX_OK;
> > +
> >  	spin_lock_irqsave(&mod->lock, flags);
> >  
> >  	/* check that we can actually transmit */
> >  	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 +1448,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);
> > +	can_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
> > @@ -1462,18 +1478,6 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
> >  	/* update statistics */
> >  	stats->tx_packets++;
> >  	stats->tx_bytes += cf->can_dlc;
> 
> 
> I think these statistic updates double the traffic as you do them in
> ican3_recv_skb() again.
> 

How exactly are the RX and TX packet counters supposed to work?

There are several cases to consider. For each case, should the rx, tx,
none, or both sets of counters updated?

1) When ican3_xmit() is called
2) When ican3_recv_skb() is called with an ECHO packet (sent from the
local host, and looped back by the hardware)
3) When ican3_recv_skb() is called with a packet from an external host
(sent from a different device on the CAN bus)

The current code does the following:
1) tx counters only
2) rx counters only (we *did* receive a packet from hardware, after all)
3) rx counters only

Do you want the following:
1) tx counters only
2) none
3) rx counters only

I can also come up with good reasons for the following:
1) none (the packet has only been queued, not sucessfully transmitted)
2) tx counters only (we *did* successfully transmit a packet)
3) rx counters only

Please clarify.

Thanks,
Ira

> > -	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 +1658,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;
> 
> 

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

[Ira W. Snyder]

On Mon, Jul 09, 2012 at 11:05:33PM +0200, Oliver Hartkopp wrote:
> Hello Ira,
> 
> looks good to me in general ...
> 
> Let's wait for other comments though.
> 
> Does it work without problems?
> 

There are no problems that I could find. Hooking two cards together and
running cangen on both simultaneously at the maximum packet generation
rate worked fine. It worked in both loopback (no option specified) and
non-loopback (-x) mode.

The previous patch which used can_cmp_echo_skb() did not handle
non-loopback mode correctly.

This version seems to receive more ENOBUFS errors back to cangen than
the "ALTERNATE VERSION" which removed the netif_stop_queue() and
netif_wake_queue() functions. I don't know if that is a problem or not.

Overall, it looks good to me.

Thanks for the comments on the other patches,
Ira

> On 09.07.2012 21:56, Ira W. Snyder wrote:
> 
> > From: "Ira W. Snyder" <iws@ovro.caltech.edu>
> > 
> > This is another different approach to fixing the Janz ICAN3 support for
> > CAN_RAW_RECV_OWN_MSGS.
> > 
> > The can_put_echo_skb() function is changed to always keep all packets
> > until can_get_echo_skb() is called. Previously, it would drop packets if
> > they were not needed to be looped back. This makes it possible for the
> > new function can_cmp_echo_skb() to work with hardware-assisted loopback
> > support on the Janz ICAN3, which does not have TX-complete interrupts of
> > any kind.
> > 
> > Since we are now storing packets even in the non-loopback case, there is
> > some extra memory overhead, to store the extra packets between the calls
> > to can_put_echo_skb() and can_get_echo_skb().
> > 
> > After this patch series is applied, the SocketCAN tst-rcv-own-msgs test
> > passes.
> > 
> > Performance is rougly 15% less than using the previously posted patch:
> > [PATCH ALTERNATE VERSION 1/1] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS
> > 
> > This performance drop is due to the extra overhead of receiving the echo
> > packets from the card itself. This involves one extra interrupt for each
> > packet sent, and the associated overhead of running ican3_napi() for
> > each packet sent.
> > 
> 
> 
> If it works without problems you might try to use 2 or 3 echo_skbs for testing
> to ensure the Janz card gets lined with outgoing traffic without waiting for
> each single sent frame.
> 
> I assume this would make the implementation slightly complexer but may solve
> the performance drop on the wire.
> 
> > Ira W. Snyder (3):
> >   can: make the echo stack keep packet information longer
> >   can: add can_cmp_echo_skb() for echo skb comparison
> >   can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS
> > 
> >  drivers/net/can/dev.c        |   75 ++++++++++++++++++++++++++++++-----------
> >  drivers/net/can/janz-ican3.c |   56 +++++++++++++++++--------------
> >  include/linux/can/dev.h      |    2 +
> >  3 files changed, 87 insertions(+), 46 deletions(-)
> > 
> 
> 

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

[Wolfgang Grandegger]

Hi Ira,

On 07/09/2012 09:56 PM, Ira W. Snyder wrote:
> From: "Ira W. Snyder" <iws@ovro.caltech.edu>
> 
> This is another different approach to fixing the Janz ICAN3 support for
> CAN_RAW_RECV_OWN_MSGS.
> 
> The can_put_echo_skb() function is changed to always keep all packets
> until can_get_echo_skb() is called. Previously, it would drop packets if
> they were not needed to be looped back. This makes it possible for the
> new function can_cmp_echo_skb() to work with hardware-assisted loopback
> support on the Janz ICAN3, which does not have TX-complete interrupts of
> any kind.
> 
> Since we are now storing packets even in the non-loopback case, there is
> some extra memory overhead, to store the extra packets between the calls
> to can_put_echo_skb() and can_get_echo_skb().

Well, I don't like such device specific quirk going into the common
interface.

> After this patch series is applied, the SocketCAN tst-rcv-own-msgs test
> passes.
> 
> Performance is rougly 15% less than using the previously posted patch:
> [PATCH ALTERNATE VERSION 1/1] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS

Oops, that's a lot and a clear contra.

> This performance drop is due to the extra overhead of receiving the echo
> packets from the card itself. This involves one extra interrupt for each
> packet sent, and the associated overhead of running ican3_napi() for
> each packet sent.

Do we really want that. I agree that CAN_RAW_RECV_OWN_MSGS should work
as expected but we should not try hard to partially fix the timing
issue. Why do we not just use the default protocol callback (flags &=
!IFF_ECHO) if the hardware cannot do it better.

Wolfgang.

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

[Wolfgang Grandegger]

On 07/09/2012 11:16 PM, Ira W. Snyder wrote:
> On Mon, Jul 09, 2012 at 10:57:28PM +0200, Oliver Hartkopp wrote:
>> On 09.07.2012 21:56, 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 can_cmp_echo_skb(), we can drop the loopback
>>> messages and return the original skbs. This fixes the issues with
>>> CAN_RAW_RECV_OWN_MSGS.
>>>
>>> Signed-off-by: Ira W. Snyder <iws@ovro.caltech.edu>
>>> ---
>>>  drivers/net/can/janz-ican3.c |   56 ++++++++++++++++++++++-------------------
>>>  1 files changed, 30 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/net/can/janz-ican3.c b/drivers/net/can/janz-ican3.c
>>> index 08c893c..a49af40 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));
>>> @@ -1150,6 +1148,21 @@ static int ican3_recv_skb(struct ican3_dev *mod)
>>>  	/* convert the ICAN3 frame into Linux CAN format */
>>>  	ican3_to_can_frame(mod, &desc, cf);
>>>  
>>> +	/*
>>> +	 * 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.
>>> +	 *
>>> +	 * Also, the netdevice queue needs to be allowed to send packets again.
>>> +	 */
>>> +	if (can_cmp_echo_skb(skb, ndev, 0)) {
>>> +		stats->rx_packets++;
>>> +		stats->rx_bytes += can_get_echo_skb(ndev, 0);
>>> +		kfree_skb(skb);
>>> +		netif_wake_queue(ndev);
>>> +		goto err_noalloc;
>>> +	}
>>> +
>>>  	/* receive the skb, update statistics */
>>>  	netif_receive_skb(skb);
>>>  	stats->rx_packets++;
>>> @@ -1177,7 +1190,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 +1216,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;
>>> @@ -1422,12 +1426,14 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
>>>  	void __iomem *desc_addr;
>>>  	unsigned long flags;
>>>  
>>> +	if (can_dropped_invalid_skb(ndev, skb))
>>> +		return NETDEV_TX_OK;
>>> +
>>>  	spin_lock_irqsave(&mod->lock, flags);
>>>  
>>>  	/* check that we can actually transmit */
>>>  	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 +1448,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);
>>> +	can_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
>>> @@ -1462,18 +1478,6 @@ static int ican3_xmit(struct sk_buff *skb, struct net_device *ndev)
>>>  	/* update statistics */
>>>  	stats->tx_packets++;
>>>  	stats->tx_bytes += cf->can_dlc;
>>
>>
>> I think these statistic updates double the traffic as you do them in
>> ican3_recv_skb() again.
>>
> 
> How exactly are the RX and TX packet counters supposed to work?

The RX and TX packet and byte counters should be set once when a CAN
message has been *successfully* received or transmitted. For TX this is
usually done when the TX done interrupt is handled. Looped backed
packets (via echo skb) are not counted.

> There are several cases to consider. For each case, should the rx, tx,
> none, or both sets of counters updated?
> 
> 1) When ican3_xmit() is called
> 2) When ican3_recv_skb() is called with an ECHO packet (sent from the
> local host, and looped back by the hardware)
> 3) When ican3_recv_skb() is called with a packet from an external host
> (sent from a different device on the CAN bus)

Only messages from and to the CAN bus should be counted.

> The current code does the following:
> 1) tx counters only
> 2) rx counters only (we *did* receive a packet from hardware, after all)
> 3) rx counters only
> 
> Do you want the following:
> 1) tx counters only
> 2) none
> 3) rx counters only
> 
> I can also come up with good reasons for the following:
> 1) none (the packet has only been queued, not sucessfully transmitted)
> 2) tx counters only (we *did* successfully transmit a packet)
> 3) rx counters only

That above handling shpuld be fine. We do *not* count looped back
messages and the counters should be incremented on *success* (which
might not always be possible). For example the EMS USB does also not
provide a TX done notification:

http://lxr.linux.no/#linux+v3.4.4/drivers/net/can/usb/ems_usb.c#L497

Wolfgang.

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

[Wolfgang Grandegger]

On 07/10/2012 08:09 AM, Wolfgang Grandegger wrote:
> Hi Ira,
> 
> On 07/09/2012 09:56 PM, Ira W. Snyder wrote:
>> From: "Ira W. Snyder" <iws@ovro.caltech.edu>
>>
>> This is another different approach to fixing the Janz ICAN3 support for
>> CAN_RAW_RECV_OWN_MSGS.
>>
>> The can_put_echo_skb() function is changed to always keep all packets
>> until can_get_echo_skb() is called. Previously, it would drop packets if
>> they were not needed to be looped back. This makes it possible for the
>> new function can_cmp_echo_skb() to work with hardware-assisted loopback
>> support on the Janz ICAN3, which does not have TX-complete interrupts of
>> any kind.
>>
>> Since we are now storing packets even in the non-loopback case, there is
>> some extra memory overhead, to store the extra packets between the calls
>> to can_put_echo_skb() and can_get_echo_skb().
> 
> Well, I don't like such device specific quirk going into the common
> interface.

Be aware, all other drivers will suffer from that change!

>> After this patch series is applied, the SocketCAN tst-rcv-own-msgs test
>> passes.
>>
>> Performance is rougly 15% less than using the previously posted patch:
>> [PATCH ALTERNATE VERSION 1/1] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS

Are the 15% only relevant if CAN_RAW_RECV_OWN_MSGS is off?

> Oops, that's a lot and a clear contra.
> 
>> This performance drop is due to the extra overhead of receiving the echo
>> packets from the card itself. This involves one extra interrupt for each
>> packet sent, and the associated overhead of running ican3_napi() for
>> each packet sent.
> 
> Do we really want that. I agree that CAN_RAW_RECV_OWN_MSGS should work
> as expected but we should not try hard to partially fix the timing
> issue. Why do we not just use the default protocol callback (flags &=
> !IFF_ECHO) if the hardware cannot do it better.

Well, getting a looped back message even if it did not go out to the bus
is not nice either.

What about a device specific solution where you do the dropping of
looped back messages only if CAN_RAW_RECV_OWN_MSGS is off.

Wolfgang,

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

[Ira W. Snyder]

On Tue, Jul 10, 2012 at 08:59:09AM +0200, Wolfgang Grandegger wrote:
> On 07/10/2012 08:09 AM, Wolfgang Grandegger wrote:
> > Hi Ira,
> > 
> > On 07/09/2012 09:56 PM, Ira W. Snyder wrote:
> >> From: "Ira W. Snyder" <iws@ovro.caltech.edu>
> >>
> >> This is another different approach to fixing the Janz ICAN3 support for
> >> CAN_RAW_RECV_OWN_MSGS.
> >>
> >> The can_put_echo_skb() function is changed to always keep all packets
> >> until can_get_echo_skb() is called. Previously, it would drop packets if
> >> they were not needed to be looped back. This makes it possible for the
> >> new function can_cmp_echo_skb() to work with hardware-assisted loopback
> >> support on the Janz ICAN3, which does not have TX-complete interrupts of
> >> any kind.
> >>
> >> Since we are now storing packets even in the non-loopback case, there is
> >> some extra memory overhead, to store the extra packets between the calls
> >> to can_put_echo_skb() and can_get_echo_skb().
> > 
> > Well, I don't like such device specific quirk going into the common
> > interface.
> 
> Be aware, all other drivers will suffer from that change!
> 

I understand that. If you prefer, I can copy can_put_echo_skb() and
can_get_echo_skb() with my modifications into the janz-ican3 driver
itself, and leave the generic ones alone.

> >> After this patch series is applied, the SocketCAN tst-rcv-own-msgs test
> >> passes.
> >>
> >> Performance is rougly 15% less than using the previously posted patch:
> >> [PATCH ALTERNATE VERSION 1/1] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS
> 
> Are the 15% only relevant if CAN_RAW_RECV_OWN_MSGS is off?
> 

The performance difference happens in all cases. The ican3 hardware
doesn't have a TX-done interrupt, but the programming guide does promise
that the hardware loopback will not return the echo packet until it has
been successfully transmitted onto the bus. It does appear to work
correctly.

I talked with my coworker who is more knowledgable about the CAN bus. We
decided we like this patch as written, since we will be able to get more
accurate timing information by sending a packet and timing how long it
takes to receive it back using CAN_RAW_RECV_OWN_MSGS.

> > Oops, that's a lot and a clear contra.
> > 
> >> This performance drop is due to the extra overhead of receiving the echo
> >> packets from the card itself. This involves one extra interrupt for each
> >> packet sent, and the associated overhead of running ican3_napi() for
> >> each packet sent.
> > 
> > Do we really want that. I agree that CAN_RAW_RECV_OWN_MSGS should work
> > as expected but we should not try hard to partially fix the timing
> > issue. Why do we not just use the default protocol callback (flags &=
> > !IFF_ECHO) if the hardware cannot do it better.
> 
> Well, getting a looped back message even if it did not go out to the bus
> is not nice either.
> 

Right. As noted above, we have decided that we like this patch, since it
only loops back packets that have actually made it onto the bus.

> What about a device specific solution where you do the dropping of
> looped back messages only if CAN_RAW_RECV_OWN_MSGS is off.
> 

Isn't this option per-socket, rather than per-device?

I don't have any idea how I'd implement this suggestion.

Thanks,
Ira

> Wolfgang,

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

[Oliver Hartkopp]

Hello Wolfgang, hello Ira,

On 10.07.2012 17:22, Ira W. Snyder wrote:

> On Tue, Jul 10, 2012 at 08:59:09AM +0200, Wolfgang Grandegger wrote:

>>>> Since we are now storing packets even in the non-loopback case, there is
>>>> some extra memory overhead, to store the extra packets between the calls
>>>> to can_put_echo_skb() and can_get_echo_skb().
>>>
>>> Well, I don't like such device specific quirk going into the common
>>> interface.
>>
>> Be aware, all other drivers will suffer from that change!
>>
> 
> I understand that. If you prefer, I can copy can_put_echo_skb() and
> can_get_echo_skb() with my modifications into the janz-ican3 driver
> itself, and leave the generic ones alone.
> 


i also thought about probable impacts on other drivers and to me the idea to
include the changes in the ican3 driver is ok.

The general infrastucture of echo_skb[] can be re-used - but the access to
these skbs can be done ican3 specific. E.g. the BUG_ON() statements may become
obsolete then.

>>>> After this patch series is applied, the SocketCAN tst-rcv-own-msgs test
>>>> passes.
>>>>
>>>> Performance is rougly 15% less than using the previously posted patch:
>>>> [PATCH ALTERNATE VERSION 1/1] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS
>>
>> Are the 15% only relevant if CAN_RAW_RECV_OWN_MSGS is off?
>>
> 
> The performance difference happens in all cases. The ican3 hardware
> doesn't have a TX-done interrupt, but the programming guide does promise
> that the hardware loopback will not return the echo packet until it has
> been successfully transmitted onto the bus. It does appear to work
> correctly.
> 
> I talked with my coworker who is more knowledgable about the CAN bus. We
> decided we like this patch as written, since we will be able to get more
> accurate timing information by sending a packet and timing how long it
> takes to receive it back using CAN_RAW_RECV_OWN_MSGS.
> 


Good choice :-)

From the practical point you get into problems, when your CAN driver doesn't
support IFF_ECHO (e.g. the slcan driver or the legacy PEAK drivers) as people
call you on the phone and talk about traffic seen by candump but no traffic on
the bus %-(

I would suggest to have about 2-3 echo_skbs in flight. This allows to push the
tx side of the ican3 to it's max. throughput.

On the receiver side you just need to compare up to 3 frames then and when
>= 2 echo_skbs become free (NULL) you restart the tx queue.

Should be feasible with low effort ...


>> What about a device specific solution where you do the dropping of
>> looped back messages only if CAN_RAW_RECV_OWN_MSGS is off.
>>
> 
> Isn't this option per-socket, rather than per-device?


yes.

> 
> I don't have any idea how I'd implement this suggestion.
> 
> Thanks,
> Ira


Regards,
Oliver

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

[Wolfgang Grandegger]

On 07/10/2012 05:22 PM, Ira W. Snyder wrote:
> On Tue, Jul 10, 2012 at 08:59:09AM +0200, Wolfgang Grandegger wrote:
>> On 07/10/2012 08:09 AM, Wolfgang Grandegger wrote:
>>> Hi Ira,
>>>
>>> On 07/09/2012 09:56 PM, Ira W. Snyder wrote:
>>>> From: "Ira W. Snyder" <iws@ovro.caltech.edu>
>>>>
>>>> This is another different approach to fixing the Janz ICAN3 support for
>>>> CAN_RAW_RECV_OWN_MSGS.
>>>>
>>>> The can_put_echo_skb() function is changed to always keep all packets
>>>> until can_get_echo_skb() is called. Previously, it would drop packets if
>>>> they were not needed to be looped back. This makes it possible for the
>>>> new function can_cmp_echo_skb() to work with hardware-assisted loopback
>>>> support on the Janz ICAN3, which does not have TX-complete interrupts of
>>>> any kind.
>>>>
>>>> Since we are now storing packets even in the non-loopback case, there is
>>>> some extra memory overhead, to store the extra packets between the calls
>>>> to can_put_echo_skb() and can_get_echo_skb().
>>>
>>> Well, I don't like such device specific quirk going into the common
>>> interface.
>>
>> Be aware, all other drivers will suffer from that change!
>>
> 
> I understand that. If you prefer, I can copy can_put_echo_skb() and
> can_get_echo_skb() with my modifications into the janz-ican3 driver
> itself, and leave the generic ones alone.

Definitely, this is special handling for that device only. Till now we
have no other device needing a similar quirk.

>>>> After this patch series is applied, the SocketCAN tst-rcv-own-msgs test
>>>> passes.
>>>>
>>>> Performance is rougly 15% less than using the previously posted patch:
>>>> [PATCH ALTERNATE VERSION 1/1] can: janz-ican3: fix support for CAN_RAW_RECV_OWN_MSGS
>>
>> Are the 15% only relevant if CAN_RAW_RECV_OWN_MSGS is off?
>>
> 
> The performance difference happens in all cases. The ican3 hardware
> doesn't have a TX-done interrupt, but the programming guide does promise
> that the hardware loopback will not return the echo packet until it has
> been successfully transmitted onto the bus. It does appear to work
> correctly.
> 
> I talked with my coworker who is more knowledgable about the CAN bus. We
> decided we like this patch as written, since we will be able to get more
> accurate timing information by sending a packet and timing how long it
> takes to receive it back using CAN_RAW_RECV_OWN_MSGS.
> 
>>> Oops, that's a lot and a clear contra.
>>>
>>>> This performance drop is due to the extra overhead of receiving the echo
>>>> packets from the card itself. This involves one extra interrupt for each
>>>> packet sent, and the associated overhead of running ican3_napi() for
>>>> each packet sent.
>>>
>>> Do we really want that. I agree that CAN_RAW_RECV_OWN_MSGS should work
>>> as expected but we should not try hard to partially fix the timing
>>> issue. Why do we not just use the default protocol callback (flags &=
>>> !IFF_ECHO) if the hardware cannot do it better.
>>
>> Well, getting a looped back message even if it did not go out to the bus
>> is not nice either.
>>
> 
> Right. As noted above, we have decided that we like this patch, since it
> only loops back packets that have actually made it onto the bus.
> 
>> What about a device specific solution where you do the dropping of
>> looped back messages only if CAN_RAW_RECV_OWN_MSGS is off.
>>
> 
> Isn't this option per-socket, rather than per-device?
> 
> I don't have any idea how I'd implement this suggestion.

Hm, the xmit function does know if the packet should be looped back. If
not, the skb will be freed:

  http://lxr.linux.no/#linux+v3.4.4/drivers/net/can/dev.c#L285

Can't that be used to decide if the packet should be stored, restored
and removed if it matches a looped back message?

Wolfgang.