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

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

[Wolfgang Grandegger]

Hello,

as you might have already realized, Michael has reported problems with
the PCH_CAN driver. With his help we try to fix these issues and add
PCH PCI support to the C_CAN driver replacing the PCH_CAN driver sooner
than later. Here follows my current patch stack for the records. When
Michael's tests are successful, I'm going to post final patches.

For Michael I have prepared out-of-tree driver sources allowing to
easily build the drivers also for older 3.x kernel versions. More
tester are welcome.

As also pointed out by Casper, the problems are obvisouly due to races
with stop and wakeup of the netif tx queue, managing tx_next and
concurrent register accesses. For the moment I have fixed the races by
adding "spin[un]lock_irqsave/restore" but, thinking more about it,
"spinlock_bh" should be enough to protect against softirq context.

With the C_CAN driver I realized some other minor issues:

- The C/D_CAN type handling is common code and could go to
  alloc_c_can_dev() or register_c_can_dev().

Comments are welcome.

Wolfgang.

Wolfgang Grandegger (6):
  pch_can: add spinlock to protect tx objects
  c_can: add spinlock to protect tx and rx objects
  c_can: add optional reset callback
  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     |   24 ++++++++++++++++-
 drivers/net/can/c_can/c_can.h     |    2 ++
 drivers/net/can/c_can/c_can_pci.c |   54 ++++++++++++++++++++++++++++++++++---
 drivers/net/can/pch_can.c         |   11 ++++++++
 4 files changed, 87 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[RFC 1/6] pch_can: add spinlock 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 48b3d62..2d3f757 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 2/6] c_can: add spinlock to protect tx and rx objects

[Wolfgang Grandegger]

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

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index e5180df..4fab6b8 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>
@@ -531,10 +532,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 */
@@ -550,6 +554,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;
 }
 
@@ -734,6 +740,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);
@@ -755,6 +764,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);
 }
 
 /*
@@ -785,6 +796,9 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
 	unsigned int msg_obj, msg_ctrl_save;
 	struct c_can_priv *priv = netdev_priv(dev);
 	u32 val = c_can_read_reg32(priv, C_CAN_INTPND1_REG);
+	unsigned long flags;
+
+	spin_lock_irqsave(&priv->lock, flags);
 
 	for (msg_obj = C_CAN_MSG_OBJ_RX_FIRST;
 			msg_obj <= C_CAN_MSG_OBJ_RX_LAST && quota > 0;
@@ -801,7 +815,7 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
 					C_CAN_IFACE(MSGCTRL_REG, 0));
 
 			if (msg_ctrl_save & IF_MCONT_EOB)
-				return num_rx_pkts;
+				goto out;
 
 			if (msg_ctrl_save & IF_MCONT_MSGLST) {
 				c_can_handle_lost_msg_obj(dev, 0, msg_obj);
@@ -833,6 +847,9 @@ static int c_can_do_rx_poll(struct net_device *dev, int quota)
 		}
 	}
 
+out:
+	spin_unlock_irqrestore(&priv->lock, flags);
+
 	return num_rx_pkts;
 }
 
@@ -1156,6 +1173,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 e5ed41d..c683f0d 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -169,6 +169,7 @@ struct c_can_priv {
 	void *priv;		/* for board-specific data */
 	u16 irqstatus;
 	enum c_can_dev_id type;
+	spinlock_t lock;	/* to protect tx and rx message objects */
 };
 
 struct net_device *alloc_c_can_dev(void);
-- 
1.7.9.5

[RFC 3/6] c_can: add optional reset callback

[Wolfgang Grandegger]

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

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 4fab6b8..7dd771b 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -1123,6 +1123,9 @@ static int c_can_open(struct net_device *dev)
 		goto exit_irq_fail;
 	}
 
+	if (priv->reset)
+		priv->reset(priv);
+
 	napi_enable(&priv->napi);
 
 	/* start the c_can controller */
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index c683f0d..fd949a4 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -161,6 +161,7 @@ struct c_can_priv {
 	int last_status;
 	u16 (*read_reg) (struct c_can_priv *priv, enum reg index);
 	void (*write_reg) (struct c_can_priv *priv, enum reg index, u16 val);
+	void (*reset) (struct c_can_priv *priv);
 	void __iomem *base;
 	const u16 *regs;
 	unsigned long irq_flags; /* for request_irq() */
-- 
1.7.9.5

[RFC 4/6] 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 3d7830b..7b73f90 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 __devinit 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 5/6] 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 7b73f90..320e371 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 __devinit c_can_pci_probe(struct pci_dev *pdev,
 		goto out_disable_device;
 	}
 
-	pci_set_master(pdev);
-	pci_enable_msi(pdev);
+
+	if (!pci_enable_msi(pdev)) {
+		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 6/6] 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 |   44 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 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 320e371..9719656 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;
+	/* CAN reset callback */
+	void (*reset) (struct c_can_priv *priv);
 };
 
 /*
@@ -65,6 +71,27 @@ 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(struct c_can_priv *priv)
+{
+	u32 __iomem *addr = priv->base + PCH_PCI_SOFT_RESET;
+
+	/* write to sw reset register */
+	iowrite32(1, addr);
+	iowrite32(0, addr);
+}
+
 static int __devinit c_can_pci_probe(struct pci_dev *pdev,
 				     const struct pci_device_id *ent)
 {
@@ -86,7 +113,6 @@ static int __devinit c_can_pci_probe(struct pci_dev *pdev,
 		goto out_disable_device;
 	}
 
-
 	if (!pci_enable_msi(pdev)) {
 		dev_info(&pdev->dev, "MSI enabled\n");
 		pci_set_master(pdev);
@@ -147,11 +173,17 @@ static int __devinit 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->reset = c_can_pci_data->reset;
+
 	ret = register_c_can_dev(dev);
 	if (ret) {
 		dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
@@ -203,6 +235,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 */
+	.reset = 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 +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 = {
-- 
1.7.9.5

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

[Marc Kleine-Budde]

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

On 11/26/2012 09:23 AM, Wolfgang Grandegger wrote:
> Hello,
> 
> as you might have already realized, Michael has reported problems with
> the PCH_CAN driver. With his help we try to fix these issues and add
> PCH PCI support to the C_CAN driver replacing the PCH_CAN driver sooner
> than later. Here follows my current patch stack for the records. When
> Michael's tests are successful, I'm going to post final patches.
> 
> For Michael I have prepared out-of-tree driver sources allowing to
> easily build the drivers also for older 3.x kernel versions. More
> tester are welcome.
> 
> As also pointed out by Casper, the problems are obvisouly due to races
> with stop and wakeup of the netif tx queue, managing tx_next and
> concurrent register accesses. For the moment I have fixed the races by
> adding "spin[un]lock_irqsave/restore" but, thinking more about it,
> "spinlock_bh" should be enough to protect against softirq context.
> 
> With the C_CAN driver I realized some other minor issues:
> 
> - The C/D_CAN type handling is common code and could go to
>   alloc_c_can_dev() or register_c_can_dev().
> 
> Comments are welcome.

Please make the patches based on linux-can-next/for-davem

Marc
> 
> Wolfgang.
> 
> Wolfgang Grandegger (6):
>   pch_can: add spinlock to protect tx objects
>   c_can: add spinlock to protect tx and rx objects
>   c_can: add optional reset callback
>   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     |   24 ++++++++++++++++-
>  drivers/net/can/c_can/c_can.h     |    2 ++
>  drivers/net/can/c_can/c_can_pci.c |   54 ++++++++++++++++++++++++++++++++++---
>  drivers/net/can/pch_can.c         |   11 ++++++++
>  4 files changed, 87 insertions(+), 4 deletions(-)
> 


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

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

[Wolfgang Grandegger]

On 11/26/2012 09:47 AM, Marc Kleine-Budde wrote:
> On 11/26/2012 09:23 AM, Wolfgang Grandegger wrote:
>> Hello,
>>
>> as you might have already realized, Michael has reported problems with
>> the PCH_CAN driver. With his help we try to fix these issues and add
>> PCH PCI support to the C_CAN driver replacing the PCH_CAN driver sooner
>> than later. Here follows my current patch stack for the records. When
>> Michael's tests are successful, I'm going to post final patches.
>>
>> For Michael I have prepared out-of-tree driver sources allowing to
>> easily build the drivers also for older 3.x kernel versions. More
>> tester are welcome.
>>
>> As also pointed out by Casper, the problems are obvisouly due to races
>> with stop and wakeup of the netif tx queue, managing tx_next and
>> concurrent register accesses. For the moment I have fixed the races by
>> adding "spin[un]lock_irqsave/restore" but, thinking more about it,
>> "spinlock_bh" should be enough to protect against softirq context.
>>
>> With the C_CAN driver I realized some other minor issues:
>>
>> - The C/D_CAN type handling is common code and could go to
>>   alloc_c_can_dev() or register_c_can_dev().
>>
>> Comments are welcome.
> 
> Please make the patches based on linux-can-next/for-davem

OK, will do for the next round. For the moment these are just RFC patches.

Wolfgang.

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

[Marc Kleine-Budde]

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

On 11/26/2012 09:50 AM, Wolfgang Grandegger wrote:
> On 11/26/2012 09:47 AM, Marc Kleine-Budde wrote:
>> On 11/26/2012 09:23 AM, Wolfgang Grandegger wrote:
>>> Hello,
>>>
>>> as you might have already realized, Michael has reported problems with
>>> the PCH_CAN driver. With his help we try to fix these issues and add
>>> PCH PCI support to the C_CAN driver replacing the PCH_CAN driver sooner
>>> than later. Here follows my current patch stack for the records. When
>>> Michael's tests are successful, I'm going to post final patches.
>>>
>>> For Michael I have prepared out-of-tree driver sources allowing to
>>> easily build the drivers also for older 3.x kernel versions. More
>>> tester are welcome.
>>>
>>> As also pointed out by Casper, the problems are obvisouly due to races
>>> with stop and wakeup of the netif tx queue, managing tx_next and
>>> concurrent register accesses. For the moment I have fixed the races by
>>> adding "spin[un]lock_irqsave/restore" but, thinking more about it,
>>> "spinlock_bh" should be enough to protect against softirq context.
>>>
>>> With the C_CAN driver I realized some other minor issues:
>>>
>>> - The C/D_CAN type handling is common code and could go to
>>>   alloc_c_can_dev() or register_c_can_dev().
>>>
>>> Comments are welcome.
>>
>> Please make the patches based on linux-can-next/for-davem
> 
> OK, will do for the next round. For the moment these are just RFC patches.

AnilKumar Ch has added added a init callback, maybe you can make use of
it, too. And give it a more general name then.

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

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

[Wolfgang Grandegger]

On 11/26/2012 09:59 AM, Marc Kleine-Budde wrote:
> On 11/26/2012 09:50 AM, Wolfgang Grandegger wrote:
>> On 11/26/2012 09:47 AM, Marc Kleine-Budde wrote:
>>> On 11/26/2012 09:23 AM, Wolfgang Grandegger wrote:
>>>> Hello,
>>>>
>>>> as you might have already realized, Michael has reported problems with
>>>> the PCH_CAN driver. With his help we try to fix these issues and add
>>>> PCH PCI support to the C_CAN driver replacing the PCH_CAN driver sooner
>>>> than later. Here follows my current patch stack for the records. When
>>>> Michael's tests are successful, I'm going to post final patches.
>>>>
>>>> For Michael I have prepared out-of-tree driver sources allowing to
>>>> easily build the drivers also for older 3.x kernel versions. More
>>>> tester are welcome.
>>>>
>>>> As also pointed out by Casper, the problems are obvisouly due to races
>>>> with stop and wakeup of the netif tx queue, managing tx_next and
>>>> concurrent register accesses. For the moment I have fixed the races by
>>>> adding "spin[un]lock_irqsave/restore" but, thinking more about it,
>>>> "spinlock_bh" should be enough to protect against softirq context.
>>>>
>>>> With the C_CAN driver I realized some other minor issues:
>>>>
>>>> - The C/D_CAN type handling is common code and could go to
>>>>   alloc_c_can_dev() or register_c_can_dev().
>>>>
>>>> Comments are welcome.
>>>
>>> Please make the patches based on linux-can-next/for-davem
>>
>> OK, will do for the next round. For the moment these are just RFC patches.
> 
> AnilKumar Ch has added added a init callback, maybe you can make use of
> it, too. And give it a more general name then.

Ah, right, missed that. As it's called at open/close the same name would
make sense.

Wolfgang.

> 
> Marc
>