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
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 <[email protected]>
---
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
This avoids a race condition where complete(tx_complete) could be called
before tx_complete is initialized.
Signed-off-by: Alan Ott <[email protected]>
---
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
Eliminate all the workqueue and interrupt enable/disable.
Signed-off-by: Alan Ott <[email protected]>
---
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
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.
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 <[email protected]>:
> 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.
>
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 <[email protected]>:
>> 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.
>>
2013/5/23 Alan Ott <[email protected]>:
> 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 <[email protected]>:
>>>
>>> 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.
>>>
>
On 05/23/2013 01:54 PM, David Hauweele wrote:
> 2013/5/23 Alan Ott <[email protected]>:
>> 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 <[email protected]>:
>>>>
>>>> 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.
>>>>
>>
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
This avoids a race condition where complete(tx_complete) could be called
before tx_complete is initialized.
Signed-off-by: Alan Ott <[email protected]>
---
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
Eliminate all the workqueue and interrupt enable/disable.
Signed-off-by: Alan Ott <[email protected]>
---
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
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 <[email protected]>
---
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
From: Alan Ott <[email protected]>
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.