[PATCH testing] mrf24j40: Move INIT_COMPLETION to before the packet is sent

[PATCH testing] mrf24j40: Move INIT_COMPLETION to before the packet is sent

[Alan Ott]

This avoids a race condition where the tx_complete interrupt could happen
before the completion is initialized.

---

David H,

Try this out and see if it changes anything for you. I put a bunch of
schedule_timeout()s in mrf24j40_tx() to basically guarantee that a reception
will happen during mrf24j40_tx(), and it seems to work. It would not work
before with said schedule_timeout() calls.

Alan.

---
 drivers/net/ieee802154/mrf24j40.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ieee802154/mrf24j40.c b/drivers/net/ieee802154/mrf24j40.c
index 556151d..0ea2a5a 100644
--- a/drivers/net/ieee802154/mrf24j40.c
+++ b/drivers/net/ieee802154/mrf24j40.c
@@ -345,6 +345,8 @@ static int mrf24j40_tx(struct ieee802154_dev *dev, struct sk_buff *skb)
 	if (ret)
 		goto err;
 
+	INIT_COMPLETION(devrec->tx_complete);
+
 	/* Set TXNTRIG bit of TXNCON to send packet */
 	ret = read_short_reg(devrec, REG_TXNCON, &val);
 	if (ret)
@@ -355,8 +357,6 @@ static int mrf24j40_tx(struct ieee802154_dev *dev, struct sk_buff *skb)
 		val |= 0x4;
 	write_short_reg(devrec, REG_TXNCON, val);
 
-	INIT_COMPLETION(devrec->tx_complete);
-
 	/* Wait for the device to send the TX complete interrupt. */
 	ret = wait_for_completion_interruptible_timeout(
 						&devrec->tx_complete,
-- 
1.7.11.2

[PATCH beta 1] 0/3] Fix race conditions in mrf24j40 interrupts

[Alan Ott]

David Hauweele noticed that the mrf24j40 would hang arbitrarily after some
period of heavy traffic.  Two race conditions were discovered, and the
driver was changed to use threaded interrupts, since the enable/disable of
interrupts in the driver has recently been a lighning rod whenever issues
arise related to interrupts (costing engineering time), and since threaded
interrupts are the right way to do it.

Alan Ott (3):
  mrf24j40: Move INIT_COMPLETION() to before packet transmission
  mrf24j40: Use threaded IRQ handler
  mrf24j40: Use level-triggered interrupts

 drivers/net/ieee802154/mrf24j40.c | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

-- 
1.7.11.2

[PATCH beta 1] 1/3] mrf24j40: Move INIT_COMPLETION() to before packet transmission

[Alan Ott]

This avoids a race condition where complete(tx_complete) could be called
before tx_complete is initialized.

Signed-off-by: Alan Ott <alan@signal11.us>
---
 drivers/net/ieee802154/mrf24j40.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ieee802154/mrf24j40.c b/drivers/net/ieee802154/mrf24j40.c
index 556151d..0ea2a5a 100644
--- a/drivers/net/ieee802154/mrf24j40.c
+++ b/drivers/net/ieee802154/mrf24j40.c
@@ -345,6 +345,8 @@ static int mrf24j40_tx(struct ieee802154_dev *dev, struct sk_buff *skb)
 	if (ret)
 		goto err;
 
+	INIT_COMPLETION(devrec->tx_complete);
+
 	/* Set TXNTRIG bit of TXNCON to send packet */
 	ret = read_short_reg(devrec, REG_TXNCON, &val);
 	if (ret)
@@ -355,8 +357,6 @@ static int mrf24j40_tx(struct ieee802154_dev *dev, struct sk_buff *skb)
 		val |= 0x4;
 	write_short_reg(devrec, REG_TXNCON, val);
 
-	INIT_COMPLETION(devrec->tx_complete);
-
 	/* Wait for the device to send the TX complete interrupt. */
 	ret = wait_for_completion_interruptible_timeout(
 						&devrec->tx_complete,
-- 
1.7.11.2

[PATCH beta 1] 2/3] mrf24j40: Use threaded IRQ handler

[Alan Ott]

Eliminate all the workqueue and interrupt enable/disable.

Signed-off-by: Alan Ott <alan@signal11.us>
---
 drivers/net/ieee802154/mrf24j40.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ieee802154/mrf24j40.c b/drivers/net/ieee802154/mrf24j40.c
index 0ea2a5a..a55ab8d 100644
--- a/drivers/net/ieee802154/mrf24j40.c
+++ b/drivers/net/ieee802154/mrf24j40.c
@@ -83,7 +83,6 @@ struct mrf24j40 {
 
 	struct mutex buffer_mutex; /* only used to protect buf */
 	struct completion tx_complete;
-	struct work_struct irqwork;
 	u8 *buf; /* 3 bytes. Used for SPI single-register transfers. */
 };
 
@@ -591,17 +590,6 @@ static struct ieee802154_ops mrf24j40_ops = {
 static irqreturn_t mrf24j40_isr(int irq, void *data)
 {
 	struct mrf24j40 *devrec = data;
-
-	disable_irq_nosync(irq);
-
-	schedule_work(&devrec->irqwork);
-
-	return IRQ_HANDLED;
-}
-
-static void mrf24j40_isrwork(struct work_struct *work)
-{
-	struct mrf24j40 *devrec = container_of(work, struct mrf24j40, irqwork);
 	u8 intstat;
 	int ret;
 
@@ -619,7 +607,7 @@ static void mrf24j40_isrwork(struct work_struct *work)
 		mrf24j40_handle_rx(devrec);
 
 out:
-	enable_irq(devrec->spi->irq);
+	return IRQ_HANDLED;
 }
 
 static int mrf24j40_probe(struct spi_device *spi)
@@ -649,7 +637,6 @@ static int mrf24j40_probe(struct spi_device *spi)
 
 	mutex_init(&devrec->buffer_mutex);
 	init_completion(&devrec->tx_complete);
-	INIT_WORK(&devrec->irqwork, mrf24j40_isrwork);
 	devrec->spi = spi;
 	dev_set_drvdata(&spi->dev, devrec);
 
@@ -695,11 +682,12 @@ static int mrf24j40_probe(struct spi_device *spi)
 	val &= ~0x3; /* Clear RX mode (normal) */
 	write_short_reg(devrec, REG_RXMCR, val);
 
-	ret = request_irq(spi->irq,
-			  mrf24j40_isr,
-			  IRQF_TRIGGER_FALLING,
-			  dev_name(&spi->dev),
-			  devrec);
+	ret = request_threaded_irq(spi->irq,
+				   NULL,
+				   mrf24j40_isr,
+				   IRQF_TRIGGER_FALLING|IRQF_ONESHOT,
+				   dev_name(&spi->dev),
+				   devrec);
 
 	if (ret) {
 		dev_err(printdev(devrec), "Unable to get IRQ");
@@ -728,7 +716,6 @@ static int mrf24j40_remove(struct spi_device *spi)
 	dev_dbg(printdev(devrec), "remove\n");
 
 	free_irq(spi->irq, devrec);
-	flush_work(&devrec->irqwork); /* TODO: Is this the right call? */
 	ieee802154_unregister_device(devrec->dev);
 	ieee802154_free_device(devrec->dev);
 	/* TODO: Will ieee802154_free_device() wait until ->xmit() is
-- 
1.7.11.2

[PATCH beta 1] 3/3] mrf24j40: Use level-triggered interrupts

[Alan Ott]

The mrf24j40 generates level interrupts. There are rare cases where it
appears that the interrupt line never gets de-asserted between interrupts,
causing interrupts to be lost, and causing a hung device from the driver's
perspective.  Switching the driver to interpret these interrupts as
level-triggered fixes this issue.

Signed-off-by: Alan Ott <alan@signal11.us>
---
 drivers/net/ieee802154/mrf24j40.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ieee802154/mrf24j40.c b/drivers/net/ieee802154/mrf24j40.c
index a55ab8d..d59dbff 100644
--- a/drivers/net/ieee802154/mrf24j40.c
+++ b/drivers/net/ieee802154/mrf24j40.c
@@ -685,7 +685,7 @@ static int mrf24j40_probe(struct spi_device *spi)
 	ret = request_threaded_irq(spi->irq,
 				   NULL,
 				   mrf24j40_isr,
-				   IRQF_TRIGGER_FALLING|IRQF_ONESHOT,
+				   IRQF_TRIGGER_LOW|IRQF_ONESHOT,
 				   dev_name(&spi->dev),
 				   devrec);
 
-- 
1.7.11.2

Re: [PATCH beta 1] 0/3] Fix race conditions in mrf24j40 interrupts

[Alan Ott]

On 05/21/2013 10:01 PM, Alan Ott wrote:
> David Hauweele noticed that the mrf24j40 would hang arbitrarily after some
> period of heavy traffic.  Two race conditions were discovered, and the
> driver was changed to use threaded interrupts, since the enable/disable of
> interrupts in the driver has recently been a lighning rod whenever issues
> arise related to interrupts (costing engineering time), and since threaded
> interrupts are the right way to do it.
>
> Alan Ott (3):
>   mrf24j40: Move INIT_COMPLETION() to before packet transmission
>   mrf24j40: Use threaded IRQ handler
>   mrf24j40: Use level-triggered interrupts
>
>  drivers/net/ieee802154/mrf24j40.c | 31 +++++++++----------------------
>  1 file changed, 9 insertions(+), 22 deletions(-)

I forgot to add, I ran ping -f both ways all afternoon (6.5 hours), and
it seems solid.

Re: [PATCH beta 1] 0/3] Fix race conditions in mrf24j40 interrupts

[David Hauweele]

Hello,

I cannot use level-triggered interrupts with GPIO on the RPi, so I
cannot test this specific patch.
However I agree with the idea of level-triggered interrupts, that
would fix all major problems related to missed interrupts.

Beside this I'm running a ping -f since more than two hours now and it
seems to work well.

David

2013/5/22 Alan Ott <alan@signal11.us>:
> On 05/21/2013 10:01 PM, Alan Ott wrote:
>> David Hauweele noticed that the mrf24j40 would hang arbitrarily after some
>> period of heavy traffic.  Two race conditions were discovered, and the
>> driver was changed to use threaded interrupts, since the enable/disable of
>> interrupts in the driver has recently been a lighning rod whenever issues
>> arise related to interrupts (costing engineering time), and since threaded
>> interrupts are the right way to do it.
>>
>> Alan Ott (3):
>>   mrf24j40: Move INIT_COMPLETION() to before packet transmission
>>   mrf24j40: Use threaded IRQ handler
>>   mrf24j40: Use level-triggered interrupts
>>
>>  drivers/net/ieee802154/mrf24j40.c | 31 +++++++++----------------------
>>  1 file changed, 9 insertions(+), 22 deletions(-)
>
> I forgot to add, I ran ping -f both ways all afternoon (6.5 hours), and
> it seems solid.
>

Re: [PATCH beta 1] 0/3] Fix race conditions in mrf24j40 interrupts

[Alan Ott]

On 5/22/13 4:32 PM, David Hauweele wrote:
> I cannot use level-triggered interrupts with GPIO on the RPi, so I
> cannot test this specific patch.

Is there another interrupt line you can tie into which does support 
level-trigger interrupts (INT0 or something)?

> However I agree with the idea of level-triggered interrupts, that
> would fix all major problems related to missed interrupts.
>
> Beside this I'm running a ping -f since more than two hours now and it
> seems to work well.
>

So that surprises me. I thought level-trigger interrupts were the thing 
that would fix this problem, and if you're not running with that patch, 
you just have the INIT_COMPLETION() fix (which you said didn't fix your 
issue) and the threaded interrupts patch, which I was fairly sure I had 
determined wasn't fixing any actual race-condition-related problems.

I'm glad, but surprised that you're no longer seeing issues.

Alan.

>
> 2013/5/22 Alan Ott <alan@signal11.us>:
>> On 05/21/2013 10:01 PM, Alan Ott wrote:
>>> David Hauweele noticed that the mrf24j40 would hang arbitrarily after some
>>> period of heavy traffic.  Two race conditions were discovered, and the
>>> driver was changed to use threaded interrupts, since the enable/disable of
>>> interrupts in the driver has recently been a lighning rod whenever issues
>>> arise related to interrupts (costing engineering time), and since threaded
>>> interrupts are the right way to do it.
>>>
>>> Alan Ott (3):
>>>    mrf24j40: Move INIT_COMPLETION() to before packet transmission
>>>    mrf24j40: Use threaded IRQ handler
>>>    mrf24j40: Use level-triggered interrupts
>>>
>>>   drivers/net/ieee802154/mrf24j40.c | 31 +++++++++----------------------
>>>   1 file changed, 9 insertions(+), 22 deletions(-)
>>
>> I forgot to add, I ran ping -f both ways all afternoon (6.5 hours), and
>> it seems solid.
>>

Re: [PATCH beta 1] 0/3] Fix race conditions in mrf24j40 interrupts

[David Hauweele]

2013/5/23 Alan Ott <alan@signal11.us>:
> On 5/22/13 4:32 PM, David Hauweele wrote:
>>
>> I cannot use level-triggered interrupts with GPIO on the RPi, so I
>> cannot test this specific patch.
>
>
> Is there another interrupt line you can tie into which does support
> level-trigger interrupts (INT0 or something)?

According to the datasheet it should be possible but the bcm2708 port
does not support it. I've been told that we shouldn't use level-triggered
interrupts in the first place.

>
>
>> However I agree with the idea of level-triggered interrupts, that
>> would fix all major problems related to missed interrupts.
>>
>> Beside this I'm running a ping -f since more than two hours now and it
>> seems to work well.
>>
>
> So that surprises me. I thought level-trigger interrupts were the thing that
> would fix this problem, and if you're not running with that patch, you just
> have the INIT_COMPLETION() fix (which you said didn't fix your issue) and
> the threaded interrupts patch, which I was fairly sure I had determined
> wasn't fixing any actual race-condition-related problems.

I should have been more clear about this. I've tested [PATCH 1/3]
which fixes the race condition with tx_complete. That is the
INIT_COMPLETION() fix. But it is still possible to miss an interrupt,
perhaps it just took longer this time. I ran the test again today and
it failed after 30 minutes.

I did not test [PATCH 2/3], that is the threaded IRQ. Instead I
removed interrupt enable/disable from the IRQ handler and the
workqueue. Without this the driver would fail within seconds of a ping
-f. Have you observed this too ? Perhaps this problem is specific to
the bcm2708 port.

David

>
> I'm glad, but surprised that you're no longer seeing issues.
>
> Alan.
>
>
>>
>> 2013/5/22 Alan Ott <alan@signal11.us>:
>>>
>>> On 05/21/2013 10:01 PM, Alan Ott wrote:
>>>>
>>>> David Hauweele noticed that the mrf24j40 would hang arbitrarily after
>>>> some
>>>> period of heavy traffic.  Two race conditions were discovered, and the
>>>> driver was changed to use threaded interrupts, since the enable/disable
>>>> of
>>>> interrupts in the driver has recently been a lighning rod whenever
>>>> issues
>>>> arise related to interrupts (costing engineering time), and since
>>>> threaded
>>>> interrupts are the right way to do it.
>>>>
>>>> Alan Ott (3):
>>>>    mrf24j40: Move INIT_COMPLETION() to before packet transmission
>>>>    mrf24j40: Use threaded IRQ handler
>>>>    mrf24j40: Use level-triggered interrupts
>>>>
>>>>   drivers/net/ieee802154/mrf24j40.c | 31 +++++++++----------------------
>>>>   1 file changed, 9 insertions(+), 22 deletions(-)
>>>
>>>
>>> I forgot to add, I ran ping -f both ways all afternoon (6.5 hours), and
>>> it seems solid.
>>>
>

Re: [PATCH beta 1] 0/3] Fix race conditions in mrf24j40 interrupts

[Alan Ott]

On 05/23/2013 01:54 PM, David Hauweele wrote:
> 2013/5/23 Alan Ott <alan@signal11.us>:
>> On 5/22/13 4:32 PM, David Hauweele wrote:
>>>
>>> I cannot use level-triggered interrupts with GPIO on the RPi, so I
>>> cannot test this specific patch.
>>
>>
>> Is there another interrupt line you can tie into which does support
>> level-trigger interrupts (INT0 or something)?
> 
> According to the datasheet it should be possible but the bcm2708 port
> does not support it. I've been told that we shouldn't use level-triggered
> interrupts in the first place.


Who is "we," and why shouldn't we use level-triggered interrupts? The
CPU needs to detect whatever type of interrupt the attached hardware
generates.


> 
>>
>>
>>> However I agree with the idea of level-triggered interrupts, that
>>> would fix all major problems related to missed interrupts.
>>>
>>> Beside this I'm running a ping -f since more than two hours now and it
>>> seems to work well.
>>>
>>
>> So that surprises me. I thought level-trigger interrupts were the thing that
>> would fix this problem, and if you're not running with that patch, you just
>> have the INIT_COMPLETION() fix (which you said didn't fix your issue) and
>> the threaded interrupts patch, which I was fairly sure I had determined
>> wasn't fixing any actual race-condition-related problems.
> 
> I should have been more clear about this. I've tested [PATCH 1/3]
> which fixes the race condition with tx_complete. That is the
> INIT_COMPLETION() fix. But it is still possible to miss an interrupt,
> perhaps it just took longer this time. I ran the test again today and
> it failed after 30 minutes.
> 
> I did not test [PATCH 2/3], that is the threaded IRQ. Instead I
> removed interrupt enable/disable from the IRQ handler and the
> workqueue. Without this the driver would fail within seconds of a ping
> -f.


Without what? What do you mean by "without this?" Without the
enable/disable, or without the change that removes the enable/disable?


> Have you observed this too ? Perhaps this problem is specific to
> the bcm2708 port.
> 

What I observe right now is that it seems to work great (ping -f for 6.5
hours) when using the three patches in this patch set on a BeagleBone.

> 
>>
>> I'm glad, but surprised that you're no longer seeing issues.
>>
>> Alan.
>>
>>
>>>
>>> 2013/5/22 Alan Ott <alan@signal11.us>:
>>>>
>>>> On 05/21/2013 10:01 PM, Alan Ott wrote:
>>>>>
>>>>> David Hauweele noticed that the mrf24j40 would hang arbitrarily after
>>>>> some
>>>>> period of heavy traffic.  Two race conditions were discovered, and the
>>>>> driver was changed to use threaded interrupts, since the enable/disable
>>>>> of
>>>>> interrupts in the driver has recently been a lighning rod whenever
>>>>> issues
>>>>> arise related to interrupts (costing engineering time), and since
>>>>> threaded
>>>>> interrupts are the right way to do it.
>>>>>
>>>>> Alan Ott (3):
>>>>>    mrf24j40: Move INIT_COMPLETION() to before packet transmission
>>>>>    mrf24j40: Use threaded IRQ handler
>>>>>    mrf24j40: Use level-triggered interrupts
>>>>>
>>>>>   drivers/net/ieee802154/mrf24j40.c | 31 +++++++++----------------------
>>>>>   1 file changed, 9 insertions(+), 22 deletions(-)
>>>>
>>>>
>>>> I forgot to add, I ran ping -f both ways all afternoon (6.5 hours), and
>>>> it seems solid.
>>>>
>>

[PATCH v1 0/3] Fix race conditions in mrf24j40 interrupts

[Alan Ott]

After testing with the betas of this patchset, it's been rebased and is
ready for inclusion.

David Hauweele noticed that the mrf24j40 would hang arbitrarily after some
period of heavy traffic.  Two race conditions were discovered, and the
driver was changed to use threaded interrupts, since the enable/disable of
interrupts in the driver has recently been a lighning rod whenever issues
arise related to interrupts (costing engineering time), and since threaded
interrupts are the right way to do it.

Alan Ott (3):
  mrf24j40: Move INIT_COMPLETION() to before packet transmission
  mrf24j40: Use threaded IRQ handler
  mrf24j40: Use level-triggered interrupts

 drivers/net/ieee802154/mrf24j40.c | 31 +++++++++----------------------
 1 file changed, 9 insertions(+), 22 deletions(-)

-- 
1.8.1.2

[PATCH v1 1/3] mrf24j40: Move INIT_COMPLETION() to before packet transmission

[Alan Ott]

This avoids a race condition where complete(tx_complete) could be called
before tx_complete is initialized.

Signed-off-by: Alan Ott <alan@signal11.us>
---
 drivers/net/ieee802154/mrf24j40.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ieee802154/mrf24j40.c b/drivers/net/ieee802154/mrf24j40.c
index 42e6dee..66bb4ce 100644
--- a/drivers/net/ieee802154/mrf24j40.c
+++ b/drivers/net/ieee802154/mrf24j40.c
@@ -344,6 +344,8 @@ static int mrf24j40_tx(struct ieee802154_dev *dev, struct sk_buff *skb)
 	if (ret)
 		goto err;
 
+	INIT_COMPLETION(devrec->tx_complete);
+
 	/* Set TXNTRIG bit of TXNCON to send packet */
 	ret = read_short_reg(devrec, REG_TXNCON, &val);
 	if (ret)
@@ -354,8 +356,6 @@ static int mrf24j40_tx(struct ieee802154_dev *dev, struct sk_buff *skb)
 		val |= 0x4;
 	write_short_reg(devrec, REG_TXNCON, val);
 
-	INIT_COMPLETION(devrec->tx_complete);
-
 	/* Wait for the device to send the TX complete interrupt. */
 	ret = wait_for_completion_interruptible_timeout(
 						&devrec->tx_complete,
-- 
1.8.1.2

[PATCH v1 2/3] mrf24j40: Use threaded IRQ handler

[Alan Ott]

Eliminate all the workqueue and interrupt enable/disable.

Signed-off-by: Alan Ott <alan@signal11.us>
---
 drivers/net/ieee802154/mrf24j40.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ieee802154/mrf24j40.c b/drivers/net/ieee802154/mrf24j40.c
index 66bb4ce..c1bc688 100644
--- a/drivers/net/ieee802154/mrf24j40.c
+++ b/drivers/net/ieee802154/mrf24j40.c
@@ -82,7 +82,6 @@ struct mrf24j40 {
 
 	struct mutex buffer_mutex; /* only used to protect buf */
 	struct completion tx_complete;
-	struct work_struct irqwork;
 	u8 *buf; /* 3 bytes. Used for SPI single-register transfers. */
 };
 
@@ -590,17 +589,6 @@ static struct ieee802154_ops mrf24j40_ops = {
 static irqreturn_t mrf24j40_isr(int irq, void *data)
 {
 	struct mrf24j40 *devrec = data;
-
-	disable_irq_nosync(irq);
-
-	schedule_work(&devrec->irqwork);
-
-	return IRQ_HANDLED;
-}
-
-static void mrf24j40_isrwork(struct work_struct *work)
-{
-	struct mrf24j40 *devrec = container_of(work, struct mrf24j40, irqwork);
 	u8 intstat;
 	int ret;
 
@@ -618,7 +606,7 @@ static void mrf24j40_isrwork(struct work_struct *work)
 		mrf24j40_handle_rx(devrec);
 
 out:
-	enable_irq(devrec->spi->irq);
+	return IRQ_HANDLED;
 }
 
 static int mrf24j40_probe(struct spi_device *spi)
@@ -642,7 +630,6 @@ static int mrf24j40_probe(struct spi_device *spi)
 
 	mutex_init(&devrec->buffer_mutex);
 	init_completion(&devrec->tx_complete);
-	INIT_WORK(&devrec->irqwork, mrf24j40_isrwork);
 	devrec->spi = spi;
 	spi_set_drvdata(spi, devrec);
 
@@ -688,11 +675,12 @@ static int mrf24j40_probe(struct spi_device *spi)
 	val &= ~0x3; /* Clear RX mode (normal) */
 	write_short_reg(devrec, REG_RXMCR, val);
 
-	ret = request_irq(spi->irq,
-			  mrf24j40_isr,
-			  IRQF_TRIGGER_FALLING,
-			  dev_name(&spi->dev),
-			  devrec);
+	ret = request_threaded_irq(spi->irq,
+				   NULL,
+				   mrf24j40_isr,
+				   IRQF_TRIGGER_FALLING|IRQF_ONESHOT,
+				   dev_name(&spi->dev),
+				   devrec);
 
 	if (ret) {
 		dev_err(printdev(devrec), "Unable to get IRQ");
@@ -721,7 +709,6 @@ static int mrf24j40_remove(struct spi_device *spi)
 	dev_dbg(printdev(devrec), "remove\n");
 
 	free_irq(spi->irq, devrec);
-	flush_work(&devrec->irqwork); /* TODO: Is this the right call? */
 	ieee802154_unregister_device(devrec->dev);
 	ieee802154_free_device(devrec->dev);
 	/* TODO: Will ieee802154_free_device() wait until ->xmit() is
-- 
1.8.1.2

[PATCH v1 3/3] mrf24j40: Use level-triggered interrupts

[Alan Ott]

The mrf24j40 generates level interrupts. There are rare cases where it
appears that the interrupt line never gets de-asserted between interrupts,
causing interrupts to be lost, and causing a hung device from the driver's
perspective.  Switching the driver to interpret these interrupts as
level-triggered fixes this issue.

Signed-off-by: Alan Ott <alan@signal11.us>
---
 drivers/net/ieee802154/mrf24j40.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ieee802154/mrf24j40.c b/drivers/net/ieee802154/mrf24j40.c
index c1bc688..0632d34 100644
--- a/drivers/net/ieee802154/mrf24j40.c
+++ b/drivers/net/ieee802154/mrf24j40.c
@@ -678,7 +678,7 @@ static int mrf24j40_probe(struct spi_device *spi)
 	ret = request_threaded_irq(spi->irq,
 				   NULL,
 				   mrf24j40_isr,
-				   IRQF_TRIGGER_FALLING|IRQF_ONESHOT,
+				   IRQF_TRIGGER_LOW|IRQF_ONESHOT,
 				   dev_name(&spi->dev),
 				   devrec);
 
-- 
1.8.1.2

Re: [PATCH v1 0/3] Fix race conditions in mrf24j40 interrupts

[David Miller]

From: Alan Ott <alan@signal11.us>
Date: Sat,  5 Oct 2013 23:52:21 -0400

> After testing with the betas of this patchset, it's been rebased and is
> ready for inclusion.
> 
> David Hauweele noticed that the mrf24j40 would hang arbitrarily after some
> period of heavy traffic.  Two race conditions were discovered, and the
> driver was changed to use threaded interrupts, since the enable/disable of
> interrupts in the driver has recently been a lighning rod whenever issues
> arise related to interrupts (costing engineering time), and since threaded
> interrupts are the right way to do it.

Series applied, thanks.