[RFC v3 0/7] pch_can/c_can: fix races and add PCH support to c_can

[RFC v3 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Wolfgang Grandegger]

Hello,

here is v3 of my patches for the C_CAN drivers.

Alexander reported strange results with write-readback of C_CAN register
showing up with concurrent I2C access. They might not be related to CAN
but to other problems on that board. At least Michael did not realize
them yet. Apart from that the c_can driver seems to work fine.
Anyway, we now know that the handling of reveive objects is not water-
proof. To fix out-of-order receipten we should implement an improved
algorithm as suggested by Marc here:

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

I will not have time to look into that befoer Xmas. Volunteers are
welcome.

What's also missing is the suspend/resume handling. Then the pch_can
driver is obsolete and can be removed.

Wolfgang.

Changes since v2:

- re-use spin_[un]lock_[irqsave|irqrestore] to protect the tx objects
  of the pch_can driver. spin_[un]lock_bh was not sufficient.
- adapted for the most recent version of the net-next tree

Changes since v1:

- use init callback after renaming it from initram
- use different sets of interface registers for rx and tx (like PCH_CAN)
- use spin_[un]lock_bh to protect the tx objects

Wolfgang Grandegger (7):
  pch_can: add spinlocks to protect tx objects
  c_can: rename callback "initram" to "init" to more general usage
  c_can: use different sets of interface registers for rx and tx
  c_can: add spinlock to protect tx objects
  c_can_pci: introduce board specific PCI bar
  c_can_pci: enable PCI bus master only for MSI
  c_can_pci: add support for PCH CAN on Intel EG20T PCH

 drivers/net/can/c_can/c_can.c          |  102 ++++++++++++++++++++------------
 drivers/net/can/c_can/c_can.h          |    3 +-
 drivers/net/can/c_can/c_can_pci.c      |   57 +++++++++++++++++-
 drivers/net/can/c_can/c_can_platform.c |    2 +-
 drivers/net/can/pch_can.c              |   11 ++++
 5 files changed, 131 insertions(+), 44 deletions(-)

-- 
1.7.9.5

[RFC v3 1/7] pch_can: add spinlocks to protect tx objects

[Wolfgang Grandegger]

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/pch_can.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/can/pch_can.c b/drivers/net/can/pch_can.c
index 7d17485..af61961 100644
--- a/drivers/net/can/pch_can.c
+++ b/drivers/net/can/pch_can.c
@@ -182,6 +182,7 @@ struct pch_can_priv {
 	struct napi_struct napi;
 	int tx_obj;	/* Point next Tx Obj index */
 	int use_msi;
+	spinlock_t lock;	/* to protect tx objects */
 };
 
 static const struct can_bittiming_const pch_can_bittiming_const = {
@@ -722,8 +723,11 @@ static void pch_can_tx_complete(struct net_device *ndev, u32 int_stat)
 {
 	struct pch_can_priv *priv = netdev_priv(ndev);
 	struct net_device_stats *stats = &(priv->ndev->stats);
+	unsigned long flags;
 	u32 dlc;
 
+	spin_lock_irqsave(&priv->lock, flags);
+
 	can_get_echo_skb(ndev, int_stat - PCH_RX_OBJ_END - 1);
 	iowrite32(PCH_CMASK_RX_TX_GET | PCH_CMASK_CLRINTPND,
 		  &priv->regs->ifregs[1].cmask);
@@ -734,6 +738,8 @@ static void pch_can_tx_complete(struct net_device *ndev, u32 int_stat)
 	stats->tx_packets++;
 	if (int_stat == PCH_TX_OBJ_END)
 		netif_wake_queue(ndev);
+
+	spin_unlock_irqrestore(&priv->lock, flags);
 }
 
 static int pch_can_poll(struct napi_struct *napi, int quota)
@@ -894,6 +900,7 @@ static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev)
 {
 	struct pch_can_priv *priv = netdev_priv(ndev);
 	struct can_frame *cf = (struct can_frame *)skb->data;
+	unsigned long flags;
 	int tx_obj_no;
 	int i;
 	u32 id2;
@@ -901,6 +908,8 @@ static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev)
 	if (can_dropped_invalid_skb(ndev, skb))
 		return NETDEV_TX_OK;
 
+	spin_lock_irqsave(&priv->lock, flags);
+
 	tx_obj_no = priv->tx_obj;
 	if (priv->tx_obj == PCH_TX_OBJ_END) {
 		if (ioread32(&priv->regs->treq2) & PCH_TREQ2_TX_MASK)
@@ -911,6 +920,8 @@ static netdev_tx_t pch_xmit(struct sk_buff *skb, struct net_device *ndev)
 		priv->tx_obj++;
 	}
 
+	spin_unlock_irqrestore(&priv->lock, flags);
+
 	/* Setting the CMASK register. */
 	pch_can_bit_set(&priv->regs->ifregs[1].cmask, PCH_CMASK_ALL);
 
-- 
1.7.9.5

Re: [RFC v3 1/7] pch_can: add spinlocks to protect tx objects

[Michael Pellegrini]

Wolfgang Grandegger <wg <at> grandegger.com> writes:

> 
> Signed-off-by: Wolfgang Grandegger <wg <at> grandegger.com>
> ---
>  drivers/net/can/pch_can.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/net/can/pch_can.c b/drivers/net/can/pch_can.c
> index 7d17485..af61961 100644
> --- a/drivers/net/can/pch_can.c
> +++ b/drivers/net/can/pch_can.c
>  <at>  <at>  -182,6 +182,7  <at>  <at>  struct pch_can_priv {
>  	struct napi_struct napi;
>  	int tx_obj;	/* Point next Tx Obj index */
>  	int use_msi;
> +	spinlock_t lock;	/* to protect tx objects */
>  };
> 
>  static const struct can_bittiming_const pch_can_bittiming_const = {
>  <at>  <at>  -722,8 +723,11  <at>  <at>  static void 
pch_can_tx_complete(struct net_device *ndev, u32 int_stat)
>  {
>  	struct pch_can_priv *priv = netdev_priv(ndev);
>  	struct net_device_stats *stats = &(priv->ndev->stats);
> +	unsigned long flags;
>  	u32 dlc;
> 
> +	spin_lock_irqsave(&priv->lock, flags);
> +
>  	can_get_echo_skb(ndev, int_stat - PCH_RX_OBJ_END - 1);
>  	iowrite32(PCH_CMASK_RX_TX_GET | PCH_CMASK_CLRINTPND,
>  		  &priv->regs->ifregs[1].cmask);
>  <at>  <at>  -734,6 +738,8  <at>  <at>  static void 
pch_can_tx_complete(struct net_device *ndev, u32 int_stat)
>  	stats->tx_packets++;
>  	if (int_stat == PCH_TX_OBJ_END)
>  		netif_wake_queue(ndev);
> +
> +	spin_unlock_irqrestore(&priv->lock, flags);
>  }
> 
>  static int pch_can_poll(struct napi_struct *napi, int quota)
>  <at>  <at>  -894,6 +900,7  <at>  <at>  static netdev_tx_t pch_xmit(struct 
sk_buff *skb, struct net_device *ndev)
>  {
>  	struct pch_can_priv *priv = netdev_priv(ndev);
>  	struct can_frame *cf = (struct can_frame *)skb->data;
> +	unsigned long flags;
>  	int tx_obj_no;
>  	int i;
>  	u32 id2;
>  <at>  <at>  -901,6 +908,8  <at>  <at>  static netdev_tx_t pch_xmit(struct 
sk_buff *skb, struct net_device *ndev)
>  	if (can_dropped_invalid_skb(ndev, skb))
>  		return NETDEV_TX_OK;
> 
> +	spin_lock_irqsave(&priv->lock, flags);
> +
>  	tx_obj_no = priv->tx_obj;
>  	if (priv->tx_obj == PCH_TX_OBJ_END) {
>  		if (ioread32(&priv->regs->treq2) & PCH_TREQ2_TX_MASK)
>  <at>  <at>  -911,6 +920,8  <at>  <at>  static netdev_tx_t pch_xmit(struct 
sk_buff *skb, struct net_device *ndev)
>  		priv->tx_obj++;
>  	}
> 
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
>  	/* Setting the CMASK register. */
>  	pch_can_bit_set(&priv->regs->ifregs[1].cmask, PCH_CMASK_ALL);
> 

I believe I have found a bug in this changelist.  When applying these 
changes to the standard pch_can driver which ships with Ubuntu 12.04 (kernel 
version 3.2.0-24.39), the race condition is still present and data 
transmission ceases after a short period of time.

I applied the following modification and the data transmission problem went 
away:

diff -c c-can-pci-v7/pch_can.c c-can-pci-v7-mrp/pch_can.c
*** c-can-pci-v7/pch_can.c	2012-11-25 05:09:13.000000000 -0500
--- c-can-pci-v7-mrp/pch_can.c	2012-11-26 11:29:11.350012074 -0500
***************
*** 921,928 ****
  		priv->tx_obj++;
  	}
  
- 	spin_unlock_irqrestore(&priv->lock, flags);
- 
  	/* Setting the CMASK register. */
  	pch_can_bit_set(&priv->regs->ifregs[1].cmask, PCH_CMASK_ALL);
  
--- 921,926 ----
***************
*** 957,962 ****
--- 955,962 ----
  
  	pch_can_rw_msg_obj(&priv->regs->ifregs[1].creq, tx_obj_no);
  
+ 	spin_unlock_irqrestore(&priv->lock, flags);
+ 
  	return NETDEV_TX_OK;
  }

This modification was suggested by Wolfgang during testing many months back 
(I found it in my archive of test code); I think it got lost during the 
shuffle between versions.

- Mike

Re: [RFC v3 1/7] pch_can: add spinlocks to protect tx objects

[Wolfgang Grandegger]

Hi Mike,

On 06/20/2013 04:23 PM, Mike Pellegrini wrote:
> I believe I have found a bug in this changelist.  When applying these
> changes to the standard pch_can driver which ships with Ubuntu 12.04
> (kernel version 3.2.0-24.39), the race condition is still present and data
> transmission ceases after a short period of time.
> 
> I applied the following modification and the data transmission problem went
> away:
> 
> diff -c c-can-pci-v7/pch_can.c c-can-pci-v7-mrp/pch_can.c
> *** c-can-pci-v7/pch_can.c 2012-11-25 05:09:13.000000000 -0500
> --- c-can-pci-v7-mrp/pch_can.c 2012-11-26 11:29:11.350012074 -0500
> ***************
> *** 921,928 ****
>   priv->tx_obj++;
>   }
> 
> - spin_unlock_irqrestore(&priv->lock, flags);
> -
>   /* Setting the CMASK register. */
>   pch_can_bit_set(&priv->regs->ifregs[1].cmask, PCH_CMASK_ALL);
> 
> --- 921,926 ----
> ***************
> *** 957,962 ****
> --- 955,962 ----
> 
>   pch_can_rw_msg_obj(&priv->regs->ifregs[1].creq, tx_obj_no);
> 
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
>   return NETDEV_TX_OK;
>   }
> 
> This modification was suggested by Wolfgang during testing many months back
> (I found it in my archive of test code); I think it got lost during the
> shuffle between versions.

Yes, I know. These patches did not go mainline yet because we know that
this driver does have other issues. Unfortunately the same is true for
the C_CAN based driver for the PCH I posted some time ago. That's a bad
situation but so far nobody with a hardware at hand made real progress
on the remaining issues. Maybe I should push my current patches
mainline, especially to introduce the C_CAN based driver.. and to make
the pch_can driver finally deprecated (or even remove it).

Wolfgang.


> 
> - Mike
> 
> 
> On Tue, Dec 11, 2012 at 4:18 PM, Wolfgang Grandegger <wg@grandegger.com>wrote:
> 
>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
>> ---
>>  drivers/net/can/pch_can.c |   11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/net/can/pch_can.c b/drivers/net/can/pch_can.c
>> index 7d17485..af61961 100644
>> --- a/drivers/net/can/pch_can.c
>> +++ b/drivers/net/can/pch_can.c
>> @@ -182,6 +182,7 @@ struct pch_can_priv {
>>         struct napi_struct napi;
>>         int tx_obj;     /* Point next Tx Obj index */
>>         int use_msi;
>> +       spinlock_t lock;        /* to protect tx objects */
>>  };
>>
>>  static const struct can_bittiming_const pch_can_bittiming_const = {
>> @@ -722,8 +723,11 @@ static void pch_can_tx_complete(struct net_device
>> *ndev, u32 int_stat)
>>  {
>>         struct pch_can_priv *priv = netdev_priv(ndev);
>>         struct net_device_stats *stats = &(priv->ndev->stats);
>> +       unsigned long flags;
>>         u32 dlc;
>>
>> +       spin_lock_irqsave(&priv->lock, flags);
>> +
>>         can_get_echo_skb(ndev, int_stat - PCH_RX_OBJ_END - 1);
>>         iowrite32(PCH_CMASK_RX_TX_GET | PCH_CMASK_CLRINTPND,
>>                   &priv->regs->ifregs[1].cmask);
>> @@ -734,6 +738,8 @@ static void pch_can_tx_complete(struct net_device
>> *ndev, u32 int_stat)
>>         stats->tx_packets++;
>>         if (int_stat == PCH_TX_OBJ_END)
>>                 netif_wake_queue(ndev);
>> +
>> +       spin_unlock_irqrestore(&priv->lock, flags);
>>  }
>>
>>  static int pch_can_poll(struct napi_struct *napi, int quota)
>> @@ -894,6 +900,7 @@ static netdev_tx_t pch_xmit(struct sk_buff *skb,
>> struct net_device *ndev)
>>  {
>>         struct pch_can_priv *priv = netdev_priv(ndev);
>>         struct can_frame *cf = (struct can_frame *)skb->data;
>> +       unsigned long flags;
>>         int tx_obj_no;
>>         int i;
>>         u32 id2;
>> @@ -901,6 +908,8 @@ static netdev_tx_t pch_xmit(struct sk_buff *skb,
>> struct net_device *ndev)
>>         if (can_dropped_invalid_skb(ndev, skb))
>>                 return NETDEV_TX_OK;
>>
>> +       spin_lock_irqsave(&priv->lock, flags);
>> +
>>         tx_obj_no = priv->tx_obj;
>>         if (priv->tx_obj == PCH_TX_OBJ_END) {
>>                 if (ioread32(&priv->regs->treq2) & PCH_TREQ2_TX_MASK)
>> @@ -911,6 +920,8 @@ static netdev_tx_t pch_xmit(struct sk_buff *skb,
>> struct net_device *ndev)
>>                 priv->tx_obj++;
>>         }
>>
>> +       spin_unlock_irqrestore(&priv->lock, flags);
>> +
>>         /* Setting the CMASK register. */
>>         pch_can_bit_set(&priv->regs->ifregs[1].cmask, PCH_CMASK_ALL);
>>
>> --
>> 1.7.9.5
>>
>>
> 

[RFC v3 2/7] c_can: rename callback "initram" to "init" to more general usage

[Wolfgang Grandegger]

The callback "init" is called with the argument "true" when the
device is opened and with "false" when it is closed allowing for
device specific initialization and cleanup, e.g. raminit or reset.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/c_can/c_can.c          |   16 ++++++++--------
 drivers/net/can/c_can/c_can.h          |    2 +-
 drivers/net/can/c_can/c_can_platform.c |    2 +-
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 5233b8f..3ae356f 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -233,10 +233,10 @@ static inline void c_can_pm_runtime_put_sync(const struct c_can_priv *priv)
 		pm_runtime_put_sync(priv->device);
 }
 
-static inline void c_can_reset_ram(const struct c_can_priv *priv, bool enable)
+static inline void c_can_init(const struct c_can_priv *priv, bool enable)
 {
-	if (priv->raminit)
-		priv->raminit(priv, enable);
+	if (priv->init)
+		priv->init(priv, enable);
 }
 
 static inline int get_tx_next_msg_obj(const struct c_can_priv *priv)
@@ -1096,7 +1096,7 @@ static int c_can_open(struct net_device *dev)
 	struct c_can_priv *priv = netdev_priv(dev);
 
 	c_can_pm_runtime_get_sync(priv);
-	c_can_reset_ram(priv, true);
+	c_can_init(priv, true);
 
 	/* open the can device */
 	err = open_candev(dev);
@@ -1125,7 +1125,7 @@ static int c_can_open(struct net_device *dev)
 exit_irq_fail:
 	close_candev(dev);
 exit_open_fail:
-	c_can_reset_ram(priv, false);
+	c_can_init(priv, false);
 	c_can_pm_runtime_put_sync(priv);
 	return err;
 }
@@ -1140,7 +1140,7 @@ static int c_can_close(struct net_device *dev)
 	free_irq(dev->irq, dev);
 	close_candev(dev);
 
-	c_can_reset_ram(priv, false);
+	c_can_init(priv, false);
 	c_can_pm_runtime_put_sync(priv);
 
 	return 0;
@@ -1198,7 +1198,7 @@ int c_can_power_down(struct net_device *dev)
 
 	c_can_stop(dev);
 
-	c_can_reset_ram(priv, false);
+	c_can_init(priv, false);
 	c_can_pm_runtime_put_sync(priv);
 
 	return 0;
@@ -1217,7 +1217,7 @@ int c_can_power_up(struct net_device *dev)
 	WARN_ON(priv->type != BOSCH_D_CAN);
 
 	c_can_pm_runtime_get_sync(priv);
-	c_can_reset_ram(priv, true);
+	c_can_init(priv, true);
 
 	/* Clear PDR and INIT bits */
 	val = priv->read_reg(priv, C_CAN_CTRL_EX_REG);
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index d2e1c21..4baf3f6 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -171,7 +171,7 @@ struct c_can_priv {
 	enum c_can_dev_id type;
 	u32 __iomem *raminit_ctrlreg;
 	unsigned int instance;
-	void (*raminit) (const struct c_can_priv *priv, bool enable);
+	void (*init) (const struct c_can_priv *priv, bool enable);
 };
 
 struct net_device *alloc_c_can_dev(void);
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index d63b919..d9551d1 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -205,7 +205,7 @@ static int c_can_plat_probe(struct platform_device *pdev)
 		if (!priv->raminit_ctrlreg || priv->instance < 0)
 			dev_info(&pdev->dev, "control memory is not used for raminit\n");
 		else
-			priv->raminit = c_can_hw_raminit;
+			priv->init = c_can_hw_raminit;
 		break;
 	default:
 		ret = -EINVAL;
-- 
1.7.9.5

[RFC v3 3/7] c_can: use different sets of interface registers for rx and tx

[Wolfgang Grandegger]

To avoid conflicts between CPU access to the message RAM we now
use the first set of interface registers for RX and the second
for TX.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/c_can/c_can.c |   73 ++++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 31 deletions(-)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 3ae356f..b0738c9 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -158,6 +158,12 @@
 #define C_CAN_NEXT_MSG_OBJ_MASK	(C_CAN_MSG_OBJ_TX_NUM - 1)
 #define RECEIVE_OBJECT_BITS	0x0000ffff
 
+/* interface register sets used for rx and tx */
+enum c_can_iface {
+	C_CAN_IFACE_RX = 0,
+	C_CAN_IFACE_TX = 1,
+};
+
 /* status interrupt */
 #define STATUS_INTERRUPT	0x8000
 
@@ -272,7 +278,8 @@ static void c_can_enable_all_interrupts(struct c_can_priv *priv,
 	priv->write_reg(priv, C_CAN_CTRL_REG, cntrl_save);
 }
 
-static inline int c_can_msg_obj_is_busy(struct c_can_priv *priv, int iface)
+static inline int c_can_msg_obj_is_busy(struct c_can_priv *priv,
+					enum c_can_iface iface)
 {
 	int count = MIN_TIMEOUT_VALUE;
 
@@ -290,7 +297,7 @@ static inline int c_can_msg_obj_is_busy(struct c_can_priv *priv, int iface)
 }
 
 static inline void c_can_object_get(struct net_device *dev,
-					int iface, int objno, int mask)
+				enum c_can_iface iface, int objno, int mask)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 
@@ -310,7 +317,7 @@ static inline void c_can_object_get(struct net_device *dev,
 }
 
 static inline void c_can_object_put(struct net_device *dev,
-					int iface, int objno, int mask)
+				enum c_can_iface iface, int objno, int mask)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 
@@ -330,7 +337,7 @@ static inline void c_can_object_put(struct net_device *dev,
 }
 
 static void c_can_write_msg_object(struct net_device *dev,
-			int iface, struct can_frame *frame, int objno)
+		enum c_can_iface iface, struct can_frame *frame, int objno)
 {
 	int i;
 	u16 flags = 0;
@@ -366,8 +373,7 @@ static void c_can_write_msg_object(struct net_device *dev,
 }
 
 static inline void c_can_mark_rx_msg_obj(struct net_device *dev,
-						int iface, int ctrl_mask,
-						int obj)
+				enum c_can_iface iface, int ctrl_mask, int obj)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 
@@ -378,8 +384,7 @@ static inline void c_can_mark_rx_msg_obj(struct net_device *dev,
 }
 
 static inline void c_can_activate_all_lower_rx_msg_obj(struct net_device *dev,
-						int iface,
-						int ctrl_mask)
+					enum c_can_iface iface, int ctrl_mask)
 {
 	int i;
 	struct c_can_priv *priv = netdev_priv(dev);
@@ -393,8 +398,7 @@ static inline void c_can_activate_all_lower_rx_msg_obj(struct net_device *dev,
 }
 
 static inline void c_can_activate_rx_msg_obj(struct net_device *dev,
-						int iface, int ctrl_mask,
-						int obj)
+				enum c_can_iface iface, int ctrl_mask, int obj)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 
@@ -405,7 +409,7 @@ static inline void c_can_activate_rx_msg_obj(struct net_device *dev,
 }
 
 static void c_can_handle_lost_msg_obj(struct net_device *dev,
-					int iface, int objno)
+					enum c_can_iface iface, int objno)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 	struct net_device_stats *stats = &dev->stats;
@@ -434,7 +438,8 @@ static void c_can_handle_lost_msg_obj(struct net_device *dev,
 	netif_receive_skb(skb);
 }
 
-static int c_can_read_msg_object(struct net_device *dev, int iface, int ctrl)
+static int c_can_read_msg_object(struct net_device *dev,
+					enum c_can_iface iface, int ctrl)
 {
 	u16 flags, data;
 	int i;
@@ -480,7 +485,8 @@ static int c_can_read_msg_object(struct net_device *dev, int iface, int ctrl)
 	return 0;
 }
 
-static void c_can_setup_receive_object(struct net_device *dev, int iface,
+static void c_can_setup_receive_object(struct net_device *dev,
+					enum c_can_iface iface,
 					int objno, unsigned int mask,
 					unsigned int id, unsigned int mcont)
 {
@@ -503,7 +509,8 @@ static void c_can_setup_receive_object(struct net_device *dev, int iface,
 			c_can_read_reg32(priv, C_CAN_MSGVAL1_REG));
 }
 
-static void c_can_inval_msg_object(struct net_device *dev, int iface, int objno)
+static void c_can_inval_msg_object(struct net_device *dev,
+					enum c_can_iface iface, int objno)
 {
 	struct c_can_priv *priv = netdev_priv(dev);
 
@@ -544,7 +551,7 @@ static netdev_tx_t c_can_start_xmit(struct sk_buff *skb,
 	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, C_CAN_IFACE_TX, frame, msg_obj_no);
 	can_put_echo_skb(skb, dev, msg_obj_no - C_CAN_MSG_OBJ_TX_FIRST);
 
 	/*
@@ -606,16 +613,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, C_CAN_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, C_CAN_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, C_CAN_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, C_CAN_IFACE_RX, C_CAN_MSG_OBJ_RX_LAST,
+			0, 0, IF_MCONT_EOB | IF_MCONT_RXIE | IF_MCONT_UMASK);
 }
 
 /*
@@ -748,10 +757,10 @@ 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))
-					& IF_MCONT_DLC_MASK;
+				C_CAN_IFACE(MSGCTRL_REG, C_CAN_IFACE_TX))
+				& IF_MCONT_DLC_MASK;
 			stats->tx_packets++;
-			c_can_inval_msg_object(dev, 0, msg_obj_no);
+			c_can_inval_msg_object(dev, C_CAN_IFACE_TX, msg_obj_no);
 		} else {
 			break;
 		}
@@ -801,16 +810,17 @@ 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 &
-					~IF_COMM_TXRQST);
+			c_can_object_get(dev, C_CAN_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, C_CAN_IFACE_RX));
 
 			if (msg_ctrl_save & IF_MCONT_EOB)
 				return num_rx_pkts;
 
 			if (msg_ctrl_save & IF_MCONT_MSGLST) {
-				c_can_handle_lost_msg_obj(dev, 0, msg_obj);
+				c_can_handle_lost_msg_obj(dev, C_CAN_IFACE_RX,
+								msg_obj);
 				num_rx_pkts++;
 				quota--;
 				continue;
@@ -820,19 +830,20 @@ 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, C_CAN_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, C_CAN_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, C_CAN_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);
+						C_CAN_IFACE_RX, msg_ctrl_save);
 
 			num_rx_pkts++;
 			quota--;
-- 
1.7.9.5

[RFC v3 4/7] c_can: add spinlock to protect tx objects

[Wolfgang Grandegger]

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/c_can/c_can.c |   13 +++++++++++++
 drivers/net/can/c_can/c_can.h |    1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index b0738c9..30646d6 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>
@@ -544,10 +545,13 @@ 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 */
@@ -563,6 +567,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;
 }
 
@@ -749,6 +755,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);
@@ -770,6 +779,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);
 }
 
 /*
@@ -1177,6 +1188,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 4baf3f6..3487d5e 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 (*init) (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);
-- 
1.7.9.5

[RFC v3 5/7] c_can_pci: introduce board specific PCI bar

[Wolfgang Grandegger]

Also fix minor issue with pci_iomap specifying size 0 for
mapping the full range.

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/c_can/c_can_pci.c |    5 ++++-
 1 file changed, 4 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 b374be7..3a2ac45 100644
--- a/drivers/net/can/c_can/c_can_pci.c
+++ b/drivers/net/can/c_can/c_can_pci.c
@@ -31,6 +31,8 @@ struct c_can_pci_data {
 	enum c_can_pci_reg_align reg_align;
 	/* Set the frequency */
 	unsigned int freq;
+	/* PCI bar number */
+	int bar;
 };
 
 /*
@@ -87,7 +89,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, 0);
 	if (!addr) {
 		dev_err(&pdev->dev,
 			"device has no PCI memory resources, "
@@ -195,6 +197,7 @@ 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,
 };
 
 #define C_CAN_ID(_vend, _dev, _driverdata) {		\
-- 
1.7.9.5

[RFC v3 6/7] c_can_pci: enable PCI bus master only for MSI

[Wolfgang Grandegger]

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/c_can/c_can_pci.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
index 3a2ac45..98562c2 100644
--- a/drivers/net/can/c_can/c_can_pci.c
+++ b/drivers/net/can/c_can/c_can_pci.c
@@ -86,8 +86,11 @@ static int c_can_pci_probe(struct pci_dev *pdev,
 		goto out_disable_device;
 	}
 
-	pci_set_master(pdev);
-	pci_enable_msi(pdev);
+	ret = pci_enable_msi(pdev);
+	if (!ret) {
+		dev_info(&pdev->dev, "MSI enabled\n");
+		pci_set_master(pdev);
+	}
 
 	addr = pci_iomap(pdev, c_can_pci_data->bar, 0);
 	if (!addr) {
-- 
1.7.9.5

[RFC v3 7/7] c_can_pci: add support for PCH CAN on Intel EG20T PCH

[Wolfgang Grandegger]

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 drivers/net/can/c_can/c_can_pci.c |   45 +++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/net/can/c_can/c_can_pci.c b/drivers/net/can/c_can/c_can_pci.c
index 98562c2..c078a05 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 {
@@ -33,6 +37,8 @@ struct c_can_pci_data {
 	unsigned int freq;
 	/* PCI bar number */
 	int bar;
+	/* Callback for reset */
+	void (*init) (const struct c_can_priv *priv, bool enable);
 };
 
 /*
@@ -65,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)
 {
@@ -147,11 +176,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->init = c_can_pci_data->init;
+
 	ret = register_c_can_dev(dev);
 	if (ret) {
 		dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
@@ -203,6 +238,14 @@ static struct c_can_pci_data c_can_sta2x11= {
 	.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) {		\
 	PCI_DEVICE(_vend, _dev),			\
 	.driver_data = (unsigned long)&_driverdata,	\
@@ -210,6 +253,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.7.9.5

Re: [RFC v3 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Christian Bendele]

Wolfgang Grandegger <wg <at> grandegger.com> writes:

> 
> Hello,
> 
> here is v3 of my patches for the C_CAN drivers.

What are those patches against? They don't apply against current mainstream
kernel (3.7)... 


> Anyway, we now know that the handling of reveive objects is not water-
> proof. To fix out-of-order receipten we should implement an improved
> algorithm as suggested by Marc here:
> 
>   http://marc.info/?l=linux-can&m=135487418225533&w=2

I'd like to take a look at this, but unfortunately that link consistently times
out for me.


> I will not have time to look into that befoer Xmas. Volunteers are
> welcome.

Pfffffft.... Personally I wouldn't be opposed to investing some time in this,
but I don't think I could get this cleared through legal in any useful time 
frame. 
Definitely not before next year. If there's still a lot of work left in January
I'll give it a try.

Christian

Re: [RFC v3 0/7] pch_can/c_can: fix races and add PCH support to c_can

[Wolfgang Grandegger]

On 12/12/2012 01:51 PM, Christian Bendele wrote:
> Wolfgang Grandegger <wg <at> grandegger.com> writes:
> 
>>
>> Hello,
>>
>> here is v3 of my patches for the C_CAN drivers.
> 
> What are those patches against? They don't apply against current mainstream
> kernel (3.7)... 

They are against David Miller's "net-next" tree, which you can get as
shown below:

git clone --reference=<another-recent-linux-tree> \
	git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next

>> Anyway, we now know that the handling of reveive objects is not water-
>> proof. To fix out-of-order receipten we should implement an improved
>> algorithm as suggested by Marc here:
>>
>>   http://marc.info/?l=linux-can&m=135487418225533&w=2

> 
> I'd like to take a look at this, but unfortunately that link consistently times
> out for me.

Try http://news.gmane.org/gmane.linux.can instead. Search for:

7 Dec 10:55	Marc Kleine-Budde	
***********	[RFC v2 0/7] pch_can/c_can: fix races and add PCH support to c_can

>> I will not have time to look into that befoer Xmas. Volunteers are
>> welcome.
> 
> Pfffffft.... Personally I wouldn't be opposed to investing some time in this,
> but I don't think I could get this cleared through legal in any useful time 
> frame. 
> Definitely not before next year. If there's still a lot of work left in January
> I'll give it a try.

Hope I will find the time before beginning of next year. Anyway, would
be nice ifyou could give the c_can_pci driver a try to see if it works
better than the pch_can.

Thanks,

Wolfgang,