c_can: wrong frame order reception

c_can: wrong frame order reception

[Alexander Stein]

Hello,

I tried using the c_can[_pci] driver for the PCH (aka EG20T) thus trying a 
different driver than pch_can.
Using 2 modules doing some CAN frame bursts ( I guess for a short time the bus 
load is near 100%) on the bus I noticed out of order reception. Each of the 
sent frames has a counter, increasing with each frame and a distinctive CAN-
ID. I attached those 2 patches I'm currently on.
1st: Support for eg20t within c_can
2nd: My debug output
Not attached as patch: I disabled MSI. I suppose there is a problem with that. 
But I can't check that when reading CAN frames is broken.

Here is some interesting output:
[ 3716.163618] c_can_pci 0000:02:0c.3 can0: 0x252 20 95
[ 3716.163721] c_can_pci 0000:02:0c.3 can0: 0x252 20 96
[ 3716.163943] c_can_pci 0000:02:0c.3 can0: 0x252 20 97
[ 3716.164067] c_can_pci 0000:02:0c.3 can0: 0x252 20 98
[ 3716.164164] c_can_pci 0000:02:0c.3 can0: free lower

[ 3716.164395] c_can_pci 0000:02:0c.3 can0: 0x252 20 9a
[ 3716.164476] c_can_pci 0000:02:0c.3 can0: 0x252 20 99

[ 3716.164671] c_can_pci 0000:02:0c.3 can0: 0x252 20 9b
[ 3716.164769] c_can_pci 0000:02:0c.3 can0: 0x252 20 9c
[ 3716.165041] c_can_pci 0000:02:0c.3 can0: 0x252 20 9d
[ 3716.165090] c_can_pci 0000:02:0c.3 can0: free lower

I separated the switched ones.
My observations:
Frame with counter "20 98" is in message box C_CAN_MSG_RX_LOW_LAST thus 
freeing all lower boxes ("free lower").
c_can_do_rx_poll will then check the next box C_CAN_MSG_RX_LOW_LAST + 1 which 
is (still) empty. All following message boxes are empty.
The next time the first message box is checked, which is 20 9a. All following 
boxes are empty until C_CAN_MSG_RX_LOW_LAST + 1 which is now has the frame 
with counter 20 99.
Starting with 20 9b all goes the normal way.

I guess that while te lower boxes are freed the new frame with counter 9a is 
about to be inserted in box C_CAN_MSG_RX_LOW_LAST + 1. But when this box is 
checked afterwards the corresponding bit is yet set in C_CAN_INTPND1_REG thus 
ignoring this box now.

Anybody has an idea how to fix that?

I'm currently running on a patched v3.10.32-rt31-rebase kernel. RT-preempt is 
active but I doubt this has any influence here.

Best regards
Alexander

Re: c_can: wrong frame order reception

[Alexander Stein]

On Wednesday 05 March 2014 15:58:09, Alexander Stein wrote:
> [...]

Forgot the patches...

commit 166f18484b0b21eed04204926e82ec244942517e
Author: Alexander Stein <alexander.stein@systec-electronic.com>
Date:   Tue Feb 25 16:52:27 2014 +0100

    c_can: Add support for eg20t (pch_can)
    
    Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index e59c42b..e761d06 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -34,6 +34,7 @@
 #include <linux/if_ether.h>
 #include <linux/list.h>
 #include <linux/io.h>
+#include <linux/spinlock.h>
 #include <linux/pm_runtime.h>
 
 #include <linux/can.h>
@@ -159,6 +160,10 @@
 #define C_CAN_NEXT_MSG_OBJ_MASK	(C_CAN_MSG_OBJ_TX_NUM - 1)
 #define RECEIVE_OBJECT_BITS	0x0000ffff
 
+/* message interface used for rx and tx */
+#define IFACE_RX		0
+#define IFACE_TX		1
+
 /* status interrupt */
 #define STATUS_INTERRUPT	0x8000
 
@@ -544,14 +549,17 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
 	u32 msg_obj_no;
 	struct c_can_priv *priv = netdev_priv(dev);
 	struct can_frame *frame = (struct can_frame *)skb->data;
+	unsigned long flags;
 
 	if (can_dropped_invalid_skb(dev, skb))
 		return NETDEV_TX_OK;
 
+	spin_lock_irqsave(&priv->lock, flags);
+
 	msg_obj_no = get_tx_next_msg_obj(priv);
 
 	/* prepare message object for transmission */
-	c_can_write_msg_object(dev, 0, frame, msg_obj_no);
+	c_can_write_msg_object(dev, IFACE_TX, frame, msg_obj_no);
 	can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
 
 	/*
@@ -563,6 +571,8 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
 			(priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) == 0)
 		netif_stop_queue(dev);
 
+	spin_unlock_irqrestore(&priv->lock, flags);
+
 	return NETDEV_TX_OK;
 }
 
@@ -613,16 +623,18 @@ static void c_can_configure_msg_objects(struct net_device *dev)
 	int i;
 
 	/* first invalidate all message objects */
-	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_NO_OF_OBJECTS; i++)
-		c_can_inval_msg_object(dev, 0, i);
+	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_MSG_OBJ_RX_LAST; i++)
+		c_can_inval_msg_object(dev, IFACE_RX, i);
+	for (i = C_CAN_MSG_OBJ_TX_FIRST; i <= C_CAN_MSG_OBJ_TX_LAST; i++)
+		c_can_inval_msg_object(dev, IFACE_TX, i);
 
 	/* setup receive message objects */
 	for (i = C_CAN_MSG_OBJ_RX_FIRST; i < C_CAN_MSG_OBJ_RX_LAST; i++)
-		c_can_setup_receive_object(dev, 0, i, 0, 0,
+		c_can_setup_receive_object(dev, IFACE_RX, i, 0, 0,
 			(IF_MCONT_RXIE | IF_MCONT_UMASK) & ~IF_MCONT_EOB);
 
-	c_can_setup_receive_object(dev, 0, C_CAN_MSG_OBJ_RX_LAST, 0, 0,
-			IF_MCONT_EOB | IF_MCONT_RXIE | IF_MCONT_UMASK);
+	c_can_setup_receive_object(dev, IFACE_RX, C_CAN_MSG_OBJ_RX_LAST,
+			0, 0, IF_MCONT_EOB | IF_MCONT_RXIE | IF_MCONT_UMASK);
 }
 
 /*
@@ -756,6 +768,9 @@ static void c_can_do_tx(struct net_device *dev)
 	u32 msg_obj_no;
 	struct c_can_priv *priv = netdev_priv(dev);
 	struct net_device_stats *stats = &dev->stats;
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->lock, flags);
 
 	for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
 		msg_obj_no = get_tx_echo_msg_obj(priv);
@@ -764,11 +779,11 @@ static void c_can_do_tx(struct net_device *dev)
 			can_get_echo_skb(dev,
 					msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
 			stats->tx_bytes += priv->read_reg(priv,
-					C_CAN_IFACE(MSGCTRL_REG, 0))
+					C_CAN_IFACE(MSGCTRL_REG, IFACE_TX))
 					& IF_MCONT_DLC_MASK;
 			stats->tx_packets++;
 			can_led_event(dev, CAN_LED_EVENT_TX);
-			c_can_inval_msg_object(dev, 0, msg_obj_no);
+			c_can_inval_msg_object(dev, IFACE_TX, msg_obj_no);
 		} else {
 			break;
 		}
@@ -778,6 +793,8 @@ static void c_can_do_tx(struct net_device *dev)
 	if (((priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) != 0) ||
 			((priv->tx_echo & C_CAN_NEXT_MSG_OBJ_MASK) == 0))
 		netif_wake_queue(dev);
+
+	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
 /*
@@ -818,13 +835,14 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
 		 * message object n, we need to handle the same properly.
 		 */
 		if (val & (1 << (msg_obj - 1))) {
-			c_can_object_get(dev, 0, msg_obj, IF_COMM_ALL &
+			c_can_object_get(dev, IFACE_RX, msg_obj, IF_COMM_ALL &
 					~IF_COMM_TXRQST);
 			msg_ctrl_save = priv->read_reg(priv,
-					C_CAN_IFACE(MSGCTRL_REG, 0));
+					C_CAN_IFACE(MSGCTRL_REG, IFACE_RX));
 
 			if (msg_ctrl_save & IF_MCONT_MSGLST) {
-				c_can_handle_lost_msg_obj(dev, 0, msg_obj);
+				c_can_handle_lost_msg_obj(dev, IFACE_RX,
+								msg_obj);
 				num_rx_pkts++;
 				quota--;
 				continue;
@@ -837,19 +855,19 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
 				continue;
 
 			/* read the data from the message object */
-			c_can_read_msg_object(dev, 0, msg_ctrl_save);
+			c_can_read_msg_object(dev, IFACE_RX, msg_ctrl_save);
 
 			if (msg_obj < C_CAN_MSG_RX_LOW_LAST)
-				c_can_mark_rx_msg_obj(dev, 0,
+				c_can_mark_rx_msg_obj(dev, IFACE_RX,
 						msg_ctrl_save, msg_obj);
 			else if (msg_obj > C_CAN_MSG_RX_LOW_LAST)
 				/* activate this msg obj */
-				c_can_activate_rx_msg_obj(dev, 0,
+				c_can_activate_rx_msg_obj(dev, IFACE_RX,
 						msg_ctrl_save, msg_obj);
 			else if (msg_obj == C_CAN_MSG_RX_LOW_LAST)
 				/* activate all lower message objects */
 				c_can_activate_all_lower_rx_msg_obj(dev,
-						0, msg_ctrl_save);
+						IFACE_RX, msg_ctrl_save);
 
 			num_rx_pkts++;
 			quota--;
@@ -1187,6 +1205,8 @@ struct net_device *alloc_c_can_dev(void)
 					CAN_CTRLMODE_LISTENONLY |
 					CAN_CTRLMODE_BERR_REPORTING;
 
+	spin_lock_init(&priv->lock);
+
 	return dev;
 }
 EXPORT_SYMBOL_GPL(alloc_c_can_dev);
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index d2e1c21..6e7715e 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -172,6 +172,7 @@ struct c_can_priv {
 	u32 __iomem *raminit_ctrlreg;
 	unsigned int instance;
 	void (*raminit) (const struct c_can_priv *priv, bool enable);
+	spinlock_t lock;	/* to protect tx and rx message objects */
 };
 
 struct net_device *alloc_c_can_dev(void);
diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
index b374be7..bc31d7f 100644
--- a/drivers/net/can/c_can/c_can_pci.c
+++ b/drivers/net/can/c_can/c_can_pci.c
@@ -19,9 +19,13 @@
 
 #include "c_can.h"
 
+#define PCI_DEVICE_ID_PCH_CAN	0x8818
+#define PCH_PCI_SOFT_RESET	0x01fc
+
 enum c_can_pci_reg_align {
 	C_CAN_REG_ALIGN_16,
 	C_CAN_REG_ALIGN_32,
+	C_CAN_REG_32,
 };
 
 struct c_can_pci_data {
@@ -31,6 +35,10 @@ struct c_can_pci_data {
 	enum c_can_pci_reg_align reg_align;
 	/* Set the frequency */
 	unsigned int freq;
+	/* PCI bar number */
+	int bar;
+	/* Callback for reset */
+	void (*init) (const struct c_can_priv *priv, bool enable);
 };
 
 /*
@@ -63,6 +71,29 @@ static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv,
 	writew(val, priv->base + 2 * priv->regs[index]);
 }
 
+static u16 c_can_pci_read_reg_32bit(struct c_can_priv *priv,
+				    enum reg index)
+{
+	return (u16)ioread32(priv->base + 2 * priv->regs[index]);
+}
+
+static void c_can_pci_write_reg_32bit(struct c_can_priv *priv,
+				      enum reg index, u16 val)
+{
+	iowrite32((u32)val, priv->base + 2 * priv->regs[index]);
+}
+
+static void c_can_pci_reset_pch(const struct c_can_priv *priv, bool enable)
+{
+	if (enable) {
+		u32 __iomem *addr = priv->base + PCH_PCI_SOFT_RESET;
+
+		/* write to sw reset register */
+		iowrite32(1, addr);
+		iowrite32(0, addr);
+	}
+}
+
 static int c_can_pci_probe(struct pci_dev *pdev,
 			   const struct pci_device_id *ent)
 {
@@ -87,7 +118,7 @@ static int c_can_pci_probe(struct pci_dev *pdev,
 	pci_set_master(pdev);
 	pci_enable_msi(pdev);
 
-	addr = pci_iomap(pdev, 0, pci_resource_len(pdev, 0));
+	addr = pci_iomap(pdev, c_can_pci_data->bar, pci_resource_len(pdev, 0));
 	if (!addr) {
 		dev_err(&pdev->dev,
 			"device has no PCI memory resources, "
@@ -142,11 +173,17 @@ static int c_can_pci_probe(struct pci_dev *pdev,
 		priv->read_reg = c_can_pci_read_reg_aligned_to_16bit;
 		priv->write_reg = c_can_pci_write_reg_aligned_to_16bit;
 		break;
+	case C_CAN_REG_32:
+		priv->read_reg = c_can_pci_read_reg_32bit;
+		priv->write_reg = c_can_pci_write_reg_32bit;
+		break;
 	default:
 		ret = -EINVAL;
 		goto out_free_c_can;
 	}
 
+	priv->raminit = c_can_pci_data->init;
+
 	ret = register_c_can_dev(dev);
 	if (ret) {
 		dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
@@ -195,6 +232,15 @@ static struct c_can_pci_data c_can_sta2x11= {
 	.type = BOSCH_C_CAN,
 	.reg_align = C_CAN_REG_ALIGN_32,
 	.freq = 52000000, /* 52 Mhz */
+	.bar = 0,
+};
+
+static struct c_can_pci_data c_can_pch = {
+	.type = BOSCH_C_CAN,
+	.reg_align = C_CAN_REG_32,
+	.freq = 50000000, /* 50 MHz */
+	.init = c_can_pci_reset_pch,
+	.bar = 1,
 };
 
 #define C_CAN_ID(_vend, _dev, _driverdata) {		\
@@ -204,6 +250,8 @@ static struct c_can_pci_data c_can_sta2x11= {
 static DEFINE_PCI_DEVICE_TABLE(c_can_pci_tbl) = {
 	C_CAN_ID(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_CAN,
 		 c_can_sta2x11),
+	C_CAN_ID(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_PCH_CAN,
+		 c_can_pch),
 	{},
 };
 static struct pci_driver c_can_pci_driver = {


diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index e761d06..5d1cab7 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -477,6 +477,7 @@ static int c_can_read_msg_object(struct net_device *dev, int iface, int ctrl)
 			frame->data[i + 1] = data >> 8;
 		}
 	}
+	netdev_info(dev, "0x%03x %02x %02x\n", frame->can_id, frame->data[2], frame->data[3]);
 
 	netif_receive_skb(skb);
 
@@ -864,10 +865,13 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
 				/* activate this msg obj */
 				c_can_activate_rx_msg_obj(dev, IFACE_RX,
 						msg_ctrl_save, msg_obj);
+			else if (msg_obj == C_CAN_MSG_RX_LOW_LAST) {
 			else if (msg_obj == C_CAN_MSG_RX_LOW_LAST)
 				/* activate all lower message objects */
 				c_can_activate_all_lower_rx_msg_obj(dev,
 						IFACE_RX, msg_ctrl_save);
+				netdev_info(dev, "free lower\n");
+			}
 
 			num_rx_pkts++;
 			quota--;

Re: c_can: wrong frame order reception

[Oliver Hartkopp]

Hello Alexander,

don't know if you monitored the patch set which was posted by Thomas Gleixner:

There was one patch "c_can: Make it SMP safe"

	http://marc.info/?l=linux-can&m=139516364829052&w=2

which addressed some issues, you obviously fixed with your below patch too.

Your patch below additionally implements the PCH_CAN support for C_CAN.

Can you please check, if the patchset from Thomas which is available here

	tag 'linux-can-fixes-for-3.15-20140401'
	in https://gitorious.org/linux-can/linux-can

fixes the frame order reception in your setup too - and if so, sent a rebased
patch for the PCH_CAN support?

Unfortunately I don't have a eg20t here :-]

Many thanks,
Oliver


On 05.03.2014 16:13, Alexander Stein wrote:
> On Wednesday 05 March 2014 15:58:09, Alexander Stein wrote:
>> [...]
> 
> Forgot the patches...
> 
> commit 166f18484b0b21eed04204926e82ec244942517e
> Author: Alexander Stein <alexander.stein@systec-electronic.com>
> Date:   Tue Feb 25 16:52:27 2014 +0100
> 
>     c_can: Add support for eg20t (pch_can)
>     
>     Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index e59c42b..e761d06 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -34,6 +34,7 @@
>  #include <linux/if_ether.h>
>  #include <linux/list.h>
>  #include <linux/io.h>
> +#include <linux/spinlock.h>
>  #include <linux/pm_runtime.h>
>  
>  #include <linux/can.h>
> @@ -159,6 +160,10 @@
>  #define C_CAN_NEXT_MSG_OBJ_MASK	(C_CAN_MSG_OBJ_TX_NUM - 1)
>  #define RECEIVE_OBJECT_BITS	0x0000ffff
>  
> +/* message interface used for rx and tx */
> +#define IFACE_RX		0
> +#define IFACE_TX		1
> +
>  /* status interrupt */
>  #define STATUS_INTERRUPT	0x8000
>  
> @@ -544,14 +549,17 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
>  	u32 msg_obj_no;
>  	struct c_can_priv *priv = netdev_priv(dev);
>  	struct can_frame *frame = (struct can_frame *)skb->data;
> +	unsigned long flags;
>  
>  	if (can_dropped_invalid_skb(dev, skb))
>  		return NETDEV_TX_OK;
>  
> +	spin_lock_irqsave(&priv->lock, flags);
> +
>  	msg_obj_no = get_tx_next_msg_obj(priv);
>  
>  	/* prepare message object for transmission */
> -	c_can_write_msg_object(dev, 0, frame, msg_obj_no);
> +	c_can_write_msg_object(dev, IFACE_TX, frame, msg_obj_no);
>  	can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
>  
>  	/*
> @@ -563,6 +571,8 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
>  			(priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) == 0)
>  		netif_stop_queue(dev);
>  
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
>  	return NETDEV_TX_OK;
>  }
>  
> @@ -613,16 +623,18 @@ static void c_can_configure_msg_objects(struct net_device *dev)
>  	int i;
>  
>  	/* first invalidate all message objects */
> -	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_NO_OF_OBJECTS; i++)
> -		c_can_inval_msg_object(dev, 0, i);
> +	for (i = C_CAN_MSG_OBJ_RX_FIRST; i <= C_CAN_MSG_OBJ_RX_LAST; i++)
> +		c_can_inval_msg_object(dev, IFACE_RX, i);
> +	for (i = C_CAN_MSG_OBJ_TX_FIRST; i <= C_CAN_MSG_OBJ_TX_LAST; i++)
> +		c_can_inval_msg_object(dev, IFACE_TX, i);
>  
>  	/* setup receive message objects */
>  	for (i = C_CAN_MSG_OBJ_RX_FIRST; i < C_CAN_MSG_OBJ_RX_LAST; i++)
> -		c_can_setup_receive_object(dev, 0, i, 0, 0,
> +		c_can_setup_receive_object(dev, IFACE_RX, i, 0, 0,
>  			(IF_MCONT_RXIE | IF_MCONT_UMASK) & ~IF_MCONT_EOB);
>  
> -	c_can_setup_receive_object(dev, 0, C_CAN_MSG_OBJ_RX_LAST, 0, 0,
> -			IF_MCONT_EOB | IF_MCONT_RXIE | IF_MCONT_UMASK);
> +	c_can_setup_receive_object(dev, IFACE_RX, C_CAN_MSG_OBJ_RX_LAST,
> +			0, 0, IF_MCONT_EOB | IF_MCONT_RXIE | IF_MCONT_UMASK);
>  }
>  
>  /*
> @@ -756,6 +768,9 @@ static void c_can_do_tx(struct net_device *dev)
>  	u32 msg_obj_no;
>  	struct c_can_priv *priv = netdev_priv(dev);
>  	struct net_device_stats *stats = &dev->stats;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&priv->lock, flags);
>  
>  	for (/* nix */; (priv->tx_next - priv->tx_echo) > 0; priv->tx_echo++) {
>  		msg_obj_no = get_tx_echo_msg_obj(priv);
> @@ -764,11 +779,11 @@ static void c_can_do_tx(struct net_device *dev)
>  			can_get_echo_skb(dev,
>  					msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
>  			stats->tx_bytes += priv->read_reg(priv,
> -					C_CAN_IFACE(MSGCTRL_REG, 0))
> +					C_CAN_IFACE(MSGCTRL_REG, IFACE_TX))
>  					& IF_MCONT_DLC_MASK;
>  			stats->tx_packets++;
>  			can_led_event(dev, CAN_LED_EVENT_TX);
> -			c_can_inval_msg_object(dev, 0, msg_obj_no);
> +			c_can_inval_msg_object(dev, IFACE_TX, msg_obj_no);
>  		} else {
>  			break;
>  		}
> @@ -778,6 +793,8 @@ static void c_can_do_tx(struct net_device *dev)
>  	if (((priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) != 0) ||
>  			((priv->tx_echo & C_CAN_NEXT_MSG_OBJ_MASK) == 0))
>  		netif_wake_queue(dev);
> +
> +	spin_unlock_irqrestore(&priv->lock, flags);
>  }
>  
>  /*
> @@ -818,13 +835,14 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
>  		 * message object n, we need to handle the same properly.
>  		 */
>  		if (val & (1 << (msg_obj - 1))) {
> -			c_can_object_get(dev, 0, msg_obj, IF_COMM_ALL &
> +			c_can_object_get(dev, IFACE_RX, msg_obj, IF_COMM_ALL &
>  					~IF_COMM_TXRQST);
>  			msg_ctrl_save = priv->read_reg(priv,
> -					C_CAN_IFACE(MSGCTRL_REG, 0));
> +					C_CAN_IFACE(MSGCTRL_REG, IFACE_RX));
>  
>  			if (msg_ctrl_save & IF_MCONT_MSGLST) {
> -				c_can_handle_lost_msg_obj(dev, 0, msg_obj);
> +				c_can_handle_lost_msg_obj(dev, IFACE_RX,
> +								msg_obj);
>  				num_rx_pkts++;
>  				quota--;
>  				continue;
> @@ -837,19 +855,19 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
>  				continue;
>  
>  			/* read the data from the message object */
> -			c_can_read_msg_object(dev, 0, msg_ctrl_save);
> +			c_can_read_msg_object(dev, IFACE_RX, msg_ctrl_save);
>  
>  			if (msg_obj < C_CAN_MSG_RX_LOW_LAST)
> -				c_can_mark_rx_msg_obj(dev, 0,
> +				c_can_mark_rx_msg_obj(dev, IFACE_RX,
>  						msg_ctrl_save, msg_obj);
>  			else if (msg_obj > C_CAN_MSG_RX_LOW_LAST)
>  				/* activate this msg obj */
> -				c_can_activate_rx_msg_obj(dev, 0,
> +				c_can_activate_rx_msg_obj(dev, IFACE_RX,
>  						msg_ctrl_save, msg_obj);
>  			else if (msg_obj == C_CAN_MSG_RX_LOW_LAST)
>  				/* activate all lower message objects */
>  				c_can_activate_all_lower_rx_msg_obj(dev,
> -						0, msg_ctrl_save);
> +						IFACE_RX, msg_ctrl_save);
>  
>  			num_rx_pkts++;
>  			quota--;
> @@ -1187,6 +1205,8 @@ struct net_device *alloc_c_can_dev(void)
>  					CAN_CTRLMODE_LISTENONLY |
>  					CAN_CTRLMODE_BERR_REPORTING;
>  
> +	spin_lock_init(&priv->lock);
> +
>  	return dev;
>  }
>  EXPORT_SYMBOL_GPL(alloc_c_can_dev);
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index d2e1c21..6e7715e 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -172,6 +172,7 @@ struct c_can_priv {
>  	u32 __iomem *raminit_ctrlreg;
>  	unsigned int instance;
>  	void (*raminit) (const struct c_can_priv *priv, bool enable);
> +	spinlock_t lock;	/* to protect tx and rx message objects */
>  };
>  
>  struct net_device *alloc_c_can_dev(void);
> diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
> index b374be7..bc31d7f 100644
> --- a/drivers/net/can/c_can/c_can_pci.c
> +++ b/drivers/net/can/c_can/c_can_pci.c
> @@ -19,9 +19,13 @@
>  
>  #include "c_can.h"
>  
> +#define PCI_DEVICE_ID_PCH_CAN	0x8818
> +#define PCH_PCI_SOFT_RESET	0x01fc
> +
>  enum c_can_pci_reg_align {
>  	C_CAN_REG_ALIGN_16,
>  	C_CAN_REG_ALIGN_32,
> +	C_CAN_REG_32,
>  };
>  
>  struct c_can_pci_data {
> @@ -31,6 +35,10 @@ struct c_can_pci_data {
>  	enum c_can_pci_reg_align reg_align;
>  	/* Set the frequency */
>  	unsigned int freq;
> +	/* PCI bar number */
> +	int bar;
> +	/* Callback for reset */
> +	void (*init) (const struct c_can_priv *priv, bool enable);
>  };
>  
>  /*
> @@ -63,6 +71,29 @@ static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv,
>  	writew(val, priv->base + 2 * priv->regs[index]);
>  }
>  
> +static u16 c_can_pci_read_reg_32bit(struct c_can_priv *priv,
> +				    enum reg index)
> +{
> +	return (u16)ioread32(priv->base + 2 * priv->regs[index]);
> +}
> +
> +static void c_can_pci_write_reg_32bit(struct c_can_priv *priv,
> +				      enum reg index, u16 val)
> +{
> +	iowrite32((u32)val, priv->base + 2 * priv->regs[index]);
> +}
> +
> +static void c_can_pci_reset_pch(const struct c_can_priv *priv, bool enable)
> +{
> +	if (enable) {
> +		u32 __iomem *addr = priv->base + PCH_PCI_SOFT_RESET;
> +
> +		/* write to sw reset register */
> +		iowrite32(1, addr);
> +		iowrite32(0, addr);
> +	}
> +}
> +
>  static int c_can_pci_probe(struct pci_dev *pdev,
>  			   const struct pci_device_id *ent)
>  {
> @@ -87,7 +118,7 @@ static int c_can_pci_probe(struct pci_dev *pdev,
>  	pci_set_master(pdev);
>  	pci_enable_msi(pdev);
>  
> -	addr = pci_iomap(pdev, 0, pci_resource_len(pdev, 0));
> +	addr = pci_iomap(pdev, c_can_pci_data->bar, pci_resource_len(pdev, 0));
>  	if (!addr) {
>  		dev_err(&pdev->dev,
>  			"device has no PCI memory resources, "
> @@ -142,11 +173,17 @@ static int c_can_pci_probe(struct pci_dev *pdev,
>  		priv->read_reg = c_can_pci_read_reg_aligned_to_16bit;
>  		priv->write_reg = c_can_pci_write_reg_aligned_to_16bit;
>  		break;
> +	case C_CAN_REG_32:
> +		priv->read_reg = c_can_pci_read_reg_32bit;
> +		priv->write_reg = c_can_pci_write_reg_32bit;
> +		break;
>  	default:
>  		ret = -EINVAL;
>  		goto out_free_c_can;
>  	}
>  
> +	priv->raminit = c_can_pci_data->init;
> +
>  	ret = register_c_can_dev(dev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
> @@ -195,6 +232,15 @@ static struct c_can_pci_data c_can_sta2x11= {
>  	.type = BOSCH_C_CAN,
>  	.reg_align = C_CAN_REG_ALIGN_32,
>  	.freq = 52000000, /* 52 Mhz */
> +	.bar = 0,
> +};
> +
> +static struct c_can_pci_data c_can_pch = {
> +	.type = BOSCH_C_CAN,
> +	.reg_align = C_CAN_REG_32,
> +	.freq = 50000000, /* 50 MHz */
> +	.init = c_can_pci_reset_pch,
> +	.bar = 1,
>  };
>  
>  #define C_CAN_ID(_vend, _dev, _driverdata) {		\
> @@ -204,6 +250,8 @@ static struct c_can_pci_data c_can_sta2x11= {
>  static DEFINE_PCI_DEVICE_TABLE(c_can_pci_tbl) = {
>  	C_CAN_ID(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_CAN,
>  		 c_can_sta2x11),
> +	C_CAN_ID(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_PCH_CAN,
> +		 c_can_pch),
>  	{},
>  };
>  static struct pci_driver c_can_pci_driver = {
> 
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index e761d06..5d1cab7 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -477,6 +477,7 @@ static int c_can_read_msg_object(struct net_device *dev, int iface, int ctrl)
>  			frame->data[i + 1] = data >> 8;
>  		}
>  	}
> +	netdev_info(dev, "0x%03x %02x %02x\n", frame->can_id, frame->data[2], frame->data[3]);
>  
>  	netif_receive_skb(skb);
>  
> @@ -864,10 +865,13 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
>  				/* activate this msg obj */
>  				c_can_activate_rx_msg_obj(dev, IFACE_RX,
>  						msg_ctrl_save, msg_obj);
> +			else if (msg_obj == C_CAN_MSG_RX_LOW_LAST) {
>  			else if (msg_obj == C_CAN_MSG_RX_LOW_LAST)
>  				/* activate all lower message objects */
>  				c_can_activate_all_lower_rx_msg_obj(dev,
>  						IFACE_RX, msg_ctrl_save);
> +				netdev_info(dev, "free lower\n");
> +			}
>  
>  			num_rx_pkts++;
>  			quota--;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: c_can: wrong frame order reception

[Alexander Stein]

Hello Oliver,

On Tuesday 01 April 2014 20:33:30, Oliver Hartkopp wrote:
> don't know if you monitored the patch set which was posted by Thomas 
Gleixner:
> 
> There was one patch "c_can: Make it SMP safe"
> 
> 	http://marc.info/?l=linux-can&m=139516364829052&w=2
> 
> which addressed some issues, you obviously fixed with your below patch too.
> 
> Your patch below additionally implements the PCH_CAN support for C_CAN.
> 
> Can you please check, if the patchset from Thomas which is available here
> 
> 	tag 'linux-can-fixes-for-3.15-20140401'
> 	in https://gitorious.org/linux-can/linux-can
> 
> fixes the frame order reception in your setup too - and if so, sent a 
rebased
> patch for the PCH_CAN support?

Yep, I've seen that. I've already picked those patches and rebased eg20t 
support on v3.14. Testing of those is already enqueued, but it got delayed due 
to some other problems.

Best regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
Am Windrad 2
08468 Heinsdorfergrund
Tel.: 03765 38600-1156
Fax: 03765 38600-4100
Email: alexander.stein@systec-electronic.com
Website: www.systec-electronic.com
 
Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Chemnitz, HRB 28082

Re: c_can: wrong frame order reception

[Alexander Stein]

Hello Oliver,

On Tuesday 01 April 2014 20:33:30, Oliver Hartkopp wrote:
> don't know if you monitored the patch set which was posted by Thomas Gleixner:
> 
> There was one patch "c_can: Make it SMP safe"
> 
> 	http://marc.info/?l=linux-can&m=139516364829052&w=2
> 
> which addressed some issues, you obviously fixed with your below patch too.
> 
> Your patch below additionally implements the PCH_CAN support for C_CAN.
> 
> Can you please check, if the patchset from Thomas which is available here
> 
> 	tag 'linux-can-fixes-for-3.15-20140401'
> 	in https://gitorious.org/linux-can/linux-can
> 
> fixes the frame order reception in your setup too - and if so, sent a rebased
> patch for the PCH_CAN support?
> 
> Unfortunately I don't have a eg20t here :-]

I tried my test based on linux-can-fixes-for-3.15-20140401 + a few patches for my board and the inlined one.
I noticed that I got no message swaps any more, the order is correct. BUT: I notice message losts, I think noticeable more than before. Interestingly it is only one message each time the counters don't match.

Best regards,
Alexander

From b96317dcb5dafee8ea3eb75e376ba7cb9d69837a Mon Sep 17 00:00:00 2001
From: Alexander Stein <alexander.stein@systec-electronic.com>
Date: Tue, 1 Apr 2014 09:02:46 +0200
Subject: [PATCH] c_can: Add support for eg20t (pch_can)

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/net/can/c_can/c_can_pci.c | 50 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
index bce0be5..ac2f1bb 100644
--- a/drivers/net/can/c_can/c_can_pci.c
+++ b/drivers/net/can/c_can/c_can_pci.c
@@ -19,9 +19,13 @@
 
 #include "c_can.h"
 
+#define PCI_DEVICE_ID_PCH_CAN	0x8818
+#define PCH_PCI_SOFT_RESET	0x01fc
+
 enum c_can_pci_reg_align {
 	C_CAN_REG_ALIGN_16,
 	C_CAN_REG_ALIGN_32,
+	C_CAN_REG_32,
 };
 
 struct c_can_pci_data {
@@ -31,6 +35,10 @@ struct c_can_pci_data {
 	enum c_can_pci_reg_align reg_align;
 	/* Set the frequency */
 	unsigned int freq;
+	/* PCI bar number */
+	int bar;
+	/* Callback for reset */
+	void (*init) (const struct c_can_priv *priv, bool enable);
 };
 
 /*
@@ -63,6 +71,29 @@ static void c_can_pci_write_reg_aligned_to_32bit(struct c_can_priv *priv,
 	writew(val, priv->base + 2 * priv->regs[index]);
 }
 
+static u16 c_can_pci_read_reg_32bit(struct c_can_priv *priv,
+				    enum reg index)
+{
+	return (u16)ioread32(priv->base + 2 * priv->regs[index]);
+}
+
+static void c_can_pci_write_reg_32bit(struct c_can_priv *priv,
+				      enum reg index, u16 val)
+{
+	iowrite32((u32)val, priv->base + 2 * priv->regs[index]);
+}
+
+static void c_can_pci_reset_pch(const struct c_can_priv *priv, bool enable)
+{
+	if (enable) {
+		u32 __iomem *addr = priv->base + PCH_PCI_SOFT_RESET;
+
+		/* write to sw reset register */
+		iowrite32(1, addr);
+		iowrite32(0, addr);
+	}
+}
+
 static int c_can_pci_probe(struct pci_dev *pdev,
 			   const struct pci_device_id *ent)
 {
@@ -87,7 +118,7 @@ static int c_can_pci_probe(struct pci_dev *pdev,
 	pci_set_master(pdev);
 	pci_enable_msi(pdev);
 
-	addr = pci_iomap(pdev, 0, pci_resource_len(pdev, 0));
+	addr = pci_iomap(pdev, c_can_pci_data->bar, pci_resource_len(pdev, 0));
 	if (!addr) {
 		dev_err(&pdev->dev,
 			"device has no PCI memory resources, "
@@ -142,11 +173,17 @@ static int c_can_pci_probe(struct pci_dev *pdev,
 		priv->read_reg = c_can_pci_read_reg_aligned_to_16bit;
 		priv->write_reg = c_can_pci_write_reg_aligned_to_16bit;
 		break;
+	case C_CAN_REG_32:
+		priv->read_reg = c_can_pci_read_reg_32bit;
+		priv->write_reg = c_can_pci_write_reg_32bit;
+		break;
 	default:
 		ret = -EINVAL;
 		goto out_free_c_can;
 	}
 
+	priv->raminit = c_can_pci_data->init;
+
 	ret = register_c_can_dev(dev);
 	if (ret) {
 		dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
@@ -193,6 +230,15 @@ static struct c_can_pci_data c_can_sta2x11= {
 	.type = BOSCH_C_CAN,
 	.reg_align = C_CAN_REG_ALIGN_32,
 	.freq = 52000000, /* 52 Mhz */
+	.bar = 0,
+};
+
+static struct c_can_pci_data c_can_pch = {
+	.type = BOSCH_C_CAN,
+	.reg_align = C_CAN_REG_32,
+	.freq = 50000000, /* 50 MHz */
+	.init = c_can_pci_reset_pch,
+	.bar = 1,
 };
 
 #define C_CAN_ID(_vend, _dev, _driverdata) {		\
@@ -202,6 +248,8 @@ static struct c_can_pci_data c_can_sta2x11= {
 static DEFINE_PCI_DEVICE_TABLE(c_can_pci_tbl) = {
 	C_CAN_ID(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_CAN,
 		 c_can_sta2x11),
+	C_CAN_ID(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_PCH_CAN,
+		 c_can_pch),
 	{},
 };
 static struct pci_driver c_can_pci_driver = {
-- 
1.8.3.2

Re: c_can: wrong frame order reception

[Wolfgang Grandegger]

Hi Alexander,

On Thu, 03 Apr 2014 15:41:38 +0200, Alexander Stein
<alexander.stein@systec-electronic.com> wrote:
> Hello Oliver,
> 
> On Tuesday 01 April 2014 20:33:30, Oliver Hartkopp wrote:
>> don't know if you monitored the patch set which was posted by Thomas
>> Gleixner:
>> 
>> There was one patch "c_can: Make it SMP safe"
>> 
>> 	http://marc.info/?l=linux-can&m=139516364829052&w=2
>> 
>> which addressed some issues, you obviously fixed with your below patch
>> too.
>> 
>> Your patch below additionally implements the PCH_CAN support for C_CAN.
>> 
>> Can you please check, if the patchset from Thomas which is available
here
>> 
>> 	tag 'linux-can-fixes-for-3.15-20140401'
>> 	in https://gitorious.org/linux-can/linux-can
>> 
>> fixes the frame order reception in your setup too - and if so, sent a
>> rebased
>> patch for the PCH_CAN support?
>> 
>> Unfortunately I don't have a eg20t here :-]

Me too!

> I tried my test based on linux-can-fixes-for-3.15-20140401 + a few
patches
> for my board and the inlined one.
> I noticed that I got no message swaps any more, the order is correct.
BUT:
> I notice message losts, I think noticeable more than before.
Interestingly
> it is only one message each time the counters don't match.
> 
> Best regards,
> Alexander
> 
>>From b96317dcb5dafee8ea3eb75e376ba7cb9d69837a Mon Sep 17 00:00:00 2001
> From: Alexander Stein <alexander.stein@systec-electronic.com>
> Date: Tue, 1 Apr 2014 09:02:46 +0200
> Subject: [PATCH] c_can: Add support for eg20t (pch_can)
> 
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
>  drivers/net/can/c_can/c_can_pci.c | 50
>  ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/c_can/c_can_pci.c
> b/drivers/net/can/c_can/c_can_pci.c
> index bce0be5..ac2f1bb 100644
> --- a/drivers/net/can/c_can/c_can_pci.c
> +++ b/drivers/net/can/c_can/c_can_pci.c
> @@ -19,9 +19,13 @@
>  
>  #include "c_can.h"
>  
> +#define PCI_DEVICE_ID_PCH_CAN	0x8818
> +#define PCH_PCI_SOFT_RESET	0x01fc
> +
>  enum c_can_pci_reg_align {
>  	C_CAN_REG_ALIGN_16,
>  	C_CAN_REG_ALIGN_32,
> +	C_CAN_REG_32,
>  };
>  
>  struct c_can_pci_data {
> @@ -31,6 +35,10 @@ struct c_can_pci_data {
>  	enum c_can_pci_reg_align reg_align;
>  	/* Set the frequency */
>  	unsigned int freq;
> +	/* PCI bar number */
> +	int bar;
> +	/* Callback for reset */
> +	void (*init) (const struct c_can_priv *priv, bool enable);
>  };
>  
>  /*
> @@ -63,6 +71,29 @@ static void
c_can_pci_write_reg_aligned_to_32bit(struct
> c_can_priv *priv,
>  	writew(val, priv->base + 2 * priv->regs[index]);
>  }
>  
> +static u16 c_can_pci_read_reg_32bit(struct c_can_priv *priv,
> +				    enum reg index)
> +{
> +	return (u16)ioread32(priv->base + 2 * priv->regs[index]);
> +}
> +
> +static void c_can_pci_write_reg_32bit(struct c_can_priv *priv,
> +				      enum reg index, u16 val)
> +{
> +	iowrite32((u32)val, priv->base + 2 * priv->regs[index]);
> +}
> +
> +static void c_can_pci_reset_pch(const struct c_can_priv *priv, bool
> enable)
> +{
> +	if (enable) {
> +		u32 __iomem *addr = priv->base + PCH_PCI_SOFT_RESET;
> +
> +		/* write to sw reset register */
> +		iowrite32(1, addr);
> +		iowrite32(0, addr);
> +	}
> +}
> +
>  static int c_can_pci_probe(struct pci_dev *pdev,
>  			   const struct pci_device_id *ent)
>  {
> @@ -87,7 +118,7 @@ static int c_can_pci_probe(struct pci_dev *pdev,
>  	pci_set_master(pdev);
>  	pci_enable_msi(pdev);
>  
> -	addr = pci_iomap(pdev, 0, pci_resource_len(pdev, 0));
> +	addr = pci_iomap(pdev, c_can_pci_data->bar, pci_resource_len(pdev,
0));
>  	if (!addr) {
>  		dev_err(&pdev->dev,
>  			"device has no PCI memory resources, "
> @@ -142,11 +173,17 @@ static int c_can_pci_probe(struct pci_dev *pdev,
>  		priv->read_reg = c_can_pci_read_reg_aligned_to_16bit;
>  		priv->write_reg = c_can_pci_write_reg_aligned_to_16bit;
>  		break;
> +	case C_CAN_REG_32:
> +		priv->read_reg = c_can_pci_read_reg_32bit;
> +		priv->write_reg = c_can_pci_write_reg_32bit;
> +		break;
>  	default:
>  		ret = -EINVAL;
>  		goto out_free_c_can;
>  	}
>  
> +	priv->raminit = c_can_pci_data->init;
> +
>  	ret = register_c_can_dev(dev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
> @@ -193,6 +230,15 @@ static struct c_can_pci_data c_can_sta2x11= {
>  	.type = BOSCH_C_CAN,
>  	.reg_align = C_CAN_REG_ALIGN_32,
>  	.freq = 52000000, /* 52 Mhz */
> +	.bar = 0,
> +};
> +
> +static struct c_can_pci_data c_can_pch = {
> +	.type = BOSCH_C_CAN,
> +	.reg_align = C_CAN_REG_32,
> +	.freq = 50000000, /* 50 MHz */
> +	.init = c_can_pci_reset_pch,
> +	.bar = 1,
>  };
>  
>  #define C_CAN_ID(_vend, _dev, _driverdata) {		\
> @@ -202,6 +248,8 @@ static struct c_can_pci_data c_can_sta2x11= {
>  static DEFINE_PCI_DEVICE_TABLE(c_can_pci_tbl) = {
>  	C_CAN_ID(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_CAN,
>  		 c_can_sta2x11),
> +	C_CAN_ID(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_PCH_CAN,
> +		 c_can_pch),
>  	{},
>  };
>  static struct pci_driver c_can_pci_driver = {

Ah, nice, that's the patch I digged out yesterday evening and which I was
going to post here. I understand that this patch is basically working,
right? Could you please re-send it in a separate email. There are others
on this list having trouble with the "pch_can" and it would be nice to
switch to c_can as soon as possible... and fix the remaining problems
there.

Wolfgang.

Re: c_can: wrong frame order reception

[wouter van herpen]

Hi all,

I am currently working on the Intel atom platform with EG20T.

Using the c_can driver from the main line 4.1.10 kernel sources, I still see
frames received in a swapped order.

It looks like all the patches mentioned in the list above are already
applied in this main line c_can driver.

Is the frame order swapping still a known issue? Are there any additional
patches regarding this issue that I'm not aware off?

Re: c_can: wrong frame order reception

[Marc Kleine-Budde]

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

On 10/21/2015 11:19 AM, wouter van herpen wrote:
> I am currently working on the Intel atom platform with EG20T.
> 
> Using the c_can driver from the main line 4.1.10 kernel sources, I still see
> frames received in a swapped order.
> 
> It looks like all the patches mentioned in the list above are already
> applied in this main line c_can driver.
> 
> Is the frame order swapping still a known issue? Are there any additional
> patches regarding this issue that I'm not aware off?

Are you on an SMP system?

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

Re: c_can: wrong frame order reception

[wouter van herpen]

Marc Kleine-Budde <mkl <at> pengutronix.de> writes:

> Are you on an SMP system?
> 
> Marc

Hi Marc,
Yes, it is indeed an SMP system.

Re: c_can: wrong frame order reception

[Marc Kleine-Budde]

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

On 10/21/2015 11:19 AM, wouter van herpen wrote:
> I am currently working on the Intel atom platform with EG20T.
> 
> Using the c_can driver from the main line 4.1.10 kernel sources, I still see
> frames received in a swapped order.
> 
> It looks like all the patches mentioned in the list above are already
> applied in this main line c_can driver.
> 
> Is the frame order swapping still a known issue? Are there any additional
> patches regarding this issue that I'm not aware off?

Maybe Oliver can help. What's needed on a v4.1.10 SMP system to fix the
RX order?

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

Re: c_can: wrong frame order reception

[Oliver Hartkopp]

Hi Wouter,

On 21.10.2015 16:33, Marc Kleine-Budde wrote:
> On 10/21/2015 11:19 AM, wouter van herpen wrote:
>> I am currently working on the Intel atom platform with EG20T.
>>
>> Using the c_can driver from the main line 4.1.10 kernel sources, I still see
>> frames received in a swapped order.
>>
>> It looks like all the patches mentioned in the list above are already
>> applied in this main line c_can driver.
>>
>> Is the frame order swapping still a known issue? Are there any additional
>> patches regarding this issue that I'm not aware off?
>
> Maybe Oliver can help. What's needed on a v4.1.10 SMP system to fix the
> RX order?
>

you can try to use the SMP-affinity to pin the innterrupt to a specific CPU:

	https://www.kernel.org/doc/Documentation/IRQ-affinity.txt

But we're currently also checking the usability of receive packet steering 
(RPS) on SMP systems. Please check

	http://marc.info/?l=linux-can&m=144251788500555&w=2

whether just using

	echo f > /sys/class/net/can0/queues/rx-0/rps_cpus

helps in you case or whether you need to set the skb hash which is suggested 
there too.

Best regards,
Oliver

Re: c_can: wrong frame order reception

[wouter van herpen]

> you can try to use the SMP-affinity to pin the innterrupt to a specific CPU:
> 
> 	https://www.kernel.org/doc/Documentation/IRQ-affinity.txt
> 
> But we're currently also checking the usability of receive packet steering 
> (RPS) on SMP systems. Please check
> 
> 	http://marc.info/?l=linux-can&m=144251788500555&w=2
> 
> whether just using
> 
> 	echo f > /sys/class/net/can0/queues/rx-0/rps_cpus
> 
> helps in you case or whether you need to set the skb hash which is suggested 
> there too.
> 
> 

Hi Oliver,

I have tried SMP affinity, in combination with adjusting
/sys/class/net/can0/queues/rx-0/rps_cpus. These changes did not solve the
out of order reception.
Also adding skb_set_hash(skb, dev->ifindex, PKT_HASH_TYPE_L2); did not make
a difference.

Do you have any other suggestions?

I have also done some testing with the pch_can driver. I noticed there the
out of order receptions are less frequent. Using the pch_can driver, OOO
reception only occurs if we stress the CPU over e.g. UART.
Using c_can driver, the OOO replies occur more often.

Re: c_can: wrong frame order reception

[Oliver Hartkopp]

Hi Wouter,



On 10/22/2015 11:50 AM, wouter van herpen wrote:
>> you can try to use the SMP-affinity to pin the innterrupt to a specific CPU:
>>
>> 	https://www.kernel.org/doc/Documentation/IRQ-affinity.txt
>>
>> But we're currently also checking the usability of receive packet steering 
>> (RPS) on SMP systems. Please check
>>
>> 	http://marc.info/?l=linux-can&m=144251788500555&w=2
>>
>> whether just using
>>
>> 	echo f > /sys/class/net/can0/queues/rx-0/rps_cpus
>>
>> helps in you case or whether you need to set the skb hash which is suggested 
>> there too.
>>
>>
> 
> Hi Oliver,
> 
> I have tried SMP affinity, in combination with adjusting
> /sys/class/net/can0/queues/rx-0/rps_cpus. These changes did not solve the
> out of order reception.
> Also adding skb_set_hash(skb, dev->ifindex, PKT_HASH_TYPE_L2); did not make
> a difference.

That's pretty bad!

> 
> Do you have any other suggestions?

No. Indeed setting an interrupt with SMP affinity to a single CPU is already
the 'strongest' tool I know.

Did you also try SMP affinity without changing the RPS settings?

> 
> I have also done some testing with the pch_can driver. I noticed there the
> out of order receptions are less frequent. Using the pch_can driver, OOO
> reception only occurs if we stress the CPU over e.g. UART.
> Using c_can driver, the OOO replies occur more often.

@Marc: Both drivers (c_can/pch_can) use NAPI. Would it make sense to convert
e.g. the c_can driver to non-NAPI for a test?

Regards,
Oliver