[PATCH 0/2] changed irq handling and some cleanup for at86rf230

[PATCH 0/2] changed irq handling and some cleanup for at86rf230

[Sascha Herrmann]

The first patch contains some cleanup for the at86rf230 driver. The
second patch changes the irq handling of the driver to avoid a
lockup caused by the irq handling of the driver.

Sascha Herrmann (2):
  at86rf230: remove unnecessary / dead code
  at86rf230: change irq handling to prevent lockups with edge type irq

 drivers/net/ieee802154/at86rf230.c |   31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

-- 
1.7.10.4


------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html

[PATCH 1/2] at86rf230: remove unnecessary / dead code

[Sascha Herrmann]

In at86rf230_probe() lp was first set to dev->priv and a few lines later
dev->priv was set to lp again, without changing lp in between. The call
to ieee802154_unregister_device() before err_irq: was unreachable.

Signed-off-by: Sascha Herrmann <sascha@ps.nvbi.de>
---
 drivers/net/ieee802154/at86rf230.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 6e88eab..fc315dd 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -838,7 +838,6 @@ static int at86rf230_probe(struct spi_device *spi)
 
 	lp->spi = spi;
 
-	dev->priv = lp;
 	dev->parent = &spi->dev;
 	dev->extra_tx_headroom = 0;
 	/* We do support only 2.4 Ghz */
@@ -940,7 +939,6 @@ static int at86rf230_probe(struct spi_device *spi)
 
 	return rc;
 
-	ieee802154_unregister_device(lp->dev);
 err_irq:
 	free_irq(spi->irq, lp);
 	flush_work(&lp->irqwork);
-- 
1.7.10.4

Re: [PATCH 1/2] at86rf230: remove unnecessary / dead code

[David Miller]

From: Sascha Herrmann <sascha@ps.nvbi.de>
Date: Thu,  4 Apr 2013 23:02:00 +0200

> In at86rf230_probe() lp was first set to dev->priv and a few lines later
> dev->priv was set to lp again, without changing lp in between. The call
> to ieee802154_unregister_device() before err_irq: was unreachable.
> 
> Signed-off-by: Sascha Herrmann <sascha@ps.nvbi.de>

Applied.

[PATCH 2/2] at86rf230: change irq handling to prevent lockups with edge type irq

[Sascha Herrmann]

Hard code the interrupt type of the at86rf230 to rising edge type and
remove the calls to disable_irq_nosync() and enable_irq() from the
isr and the irqwork function. The at86rf230 resets the irq line only
after the irq status register is read. Disabling the irq can lock the
driver in situations where a irq is set by the radio while the driver
is still reading the frame buffer. Additional the irq filter register
is set to filter out all unused interrupts and the irq status register
is read in the probe function to clear the irq line.

Signed-off-by: Sascha Herrmann <sascha@ps.nvbi.de>
---
 drivers/net/ieee802154/at86rf230.c |   29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index fc315dd..eb5359f 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -51,7 +51,7 @@ struct at86rf230_local {
 	struct ieee802154_dev *dev;
 
 	spinlock_t lock;
-	bool irq_disabled;
+	bool irq_busy;
 	bool is_tx;
 };
 
@@ -544,7 +544,7 @@ at86rf230_xmit(struct ieee802154_dev *dev, struct sk_buff *skb)
 	unsigned long flags;
 
 	spin_lock(&lp->lock);
-	if  (lp->irq_disabled) {
+	if  (lp->irq_busy) {
 		spin_unlock(&lp->lock);
 		return -EBUSY;
 	}
@@ -705,20 +705,16 @@ static void at86rf230_irqwork(struct work_struct *work)
 	}
 
 	spin_lock_irqsave(&lp->lock, flags);
-	lp->irq_disabled = 0;
+	lp->irq_busy = 0;
 	spin_unlock_irqrestore(&lp->lock, flags);
-
-	enable_irq(lp->spi->irq);
 }
 
 static irqreturn_t at86rf230_isr(int irq, void *data)
 {
 	struct at86rf230_local *lp = data;
 
-	disable_irq_nosync(irq);
-
 	spin_lock(&lp->lock);
-	lp->irq_disabled = 1;
+	lp->irq_busy = 1;
 	spin_unlock(&lp->lock);
 
 	schedule_work(&lp->irqwork);
@@ -748,12 +744,7 @@ static int at86rf230_hw_init(struct at86rf230_local *lp)
 		dev_info(&lp->spi->dev, "Status: %02x\n", status);
 	}
 
-	rc = at86rf230_write_subreg(lp, SR_IRQ_MASK, 0xff); /* IRQ_TRX_UR |
-							     * IRQ_CCA_ED |
-							     * IRQ_TRX_END |
-							     * IRQ_PLL_UNL |
-							     * IRQ_PLL_LOCK
-							     */
+	rc = at86rf230_write_subreg(lp, SR_IRQ_MASK, IRQ_TRX_END);
 	if (rc)
 		return rc;
 
@@ -819,7 +810,7 @@ static int at86rf230_probe(struct spi_device *spi)
 {
 	struct ieee802154_dev *dev;
 	struct at86rf230_local *lp;
-	u8 man_id_0, man_id_1;
+	u8 man_id_0, man_id_1, status;
 	int rc;
 	const char *chip;
 	int supported = 0;
@@ -928,11 +919,17 @@ static int at86rf230_probe(struct spi_device *spi)
 	if (rc)
 		goto err_gpio_dir;
 
-	rc = request_irq(spi->irq, at86rf230_isr, IRQF_SHARED,
+	rc = request_irq(spi->irq, at86rf230_isr,
+			 IRQF_SHARED | IRQF_TRIGGER_RISING,
 			 dev_name(&spi->dev), lp);
 	if (rc)
 		goto err_gpio_dir;
 
+	/* Read irq status register to reset irq line */
+	rc = at86rf230_read_subreg(lp, RG_IRQ_STATUS, 0xff, 0, &status);
+	if (rc)
+		goto err_irq;
+
 	rc = ieee802154_register_device(lp->dev);
 	if (rc)
 		goto err_irq;
-- 
1.7.10.4

Re: [PATCH 2/2] at86rf230: change irq handling to prevent lockups with edge type irq

[Werner Almesberger]

Sascha Herrmann wrote:
> -	rc = request_irq(spi->irq, at86rf230_isr, IRQF_SHARED,
> +	rc = request_irq(spi->irq, at86rf230_isr,
> +			 IRQF_SHARED | IRQF_TRIGGER_RISING,

To clarify the purpose of your patch, I think it's only needed
for platforms that don't support level-triggered interrupts.
I.e., if you used IRQF_TRIGGER_HIGH (and happened to have a
platform that supports it, which you said on IRC you don't),
the existing code should work fine. Correct ?

I wonder if it wouldn't be better to make the code work with
both edge and level interrupts instead of having to choose.

E.g., this sort of loop in at86rf230_irqwork should work with
either:

	while (1) {
		irq = read_and_clear_interrupt();
		...
		if (!test_and_clear_bit(0, &lp->irq_disabled))
			break;
		enable_irq();
	}
	lp->irq_busy = 0; /* allow _xmit */

Disadvantage: you get extra IRQ_STATUS reads, which has a
slight penalty, given that all this is on SPI.

To achieve perfection, at86rf230_probe could try all four
possible trigger modes, pick one the platform supports, and
set TRX_CTRL_1.IRQ_POLARITY accordingly.

In any case, I gave your patch very light testing on ATBEN and
it performed as good as the original code.

- Werner

------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire 
the most talented Cisco Certified professionals. Visit the 
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html

Re: [Linux-zigbee-devel] [PATCH 2/2] at86rf230: change irq handling to prevent lockups with edge type irq

[Werner Almesberger]

I wrote:
> To achieve perfection, at86rf230_probe could try all four
> possible trigger modes, pick one the platform supports, and
> set TRX_CTRL_1.IRQ_POLARITY accordingly.

Thinking of it, probing by trying request_irq has an unpleasant ring
to it. Perhaps a better way would be to leave this decision to the
platform code and do one of these:

1) pass irqflags and the polarity in the platform data, or

2) pass irqflags and extract the polarity from the irqflags, or

3) set up the trigger mode outside the driver and pass only the
   polarity,

where 1) with (irqflags & IRQF_TRIGGER_MASK) == 0 includes
case 3).

- Werner

Re: [Linux-zigbee-devel] [PATCH 2/2] at86rf230: change irq handling to prevent lockups with edge type irq

[Sascha Herrmann]

>I wrote:
>> To achieve perfection, at86rf230_probe could try all four
>> possible trigger modes, pick one the platform supports, and
>> set TRX_CTRL_1.IRQ_POLARITY accordingly.
>
>Thinking of it, probing by trying request_irq has an unpleasant ring
>to it. Perhaps a better way would be to leave this decision to the
>platform code and do one of these:
>
>1) pass irqflags and the polarity in the platform data, or
>
>2) pass irqflags and extract the polarity from the irqflags, or
>
>3) set up the trigger mode outside the driver and pass only the
>   polarity,
>
>where 1) with (irqflags & IRQF_TRIGGER_MASK) == 0 includes
>case 3).

The change with this patch is mostly about the trigger type of the interrupt. The trigger level isn't configurable in the rf230, as far as I understand the datasheet. I posted a version of this patch with the option to configure the interrupt type (edge or level) for discussion on the linux-zigbee list some time ago. The result tended toward hardcoding the interrupt type in the driver [1]. My assumption is, that every platform would support edge type interrupts.

If it is preferred to have the interrupt type configurable I now would prefer to implement seperate isr and irqwork functions for each irq type. I fear the sollution to read the interrupt status register in a loop (as suggested in your earlier message) would leave chances for race conditions or spurious interrupts, depending on wheter interrupts are enabled before or after reading the status register. 

Hard coding the interrupt type would have the pro of keeping the driver complexity low. Surely for this option, the assumption that (at least nearly) every platform supports edge type interrupts should hold.

[1] http://www.mail-archive.com/linux-zigbee-devel@lists.sourceforge.net/msg01385.html

Thanks,
Sascha

Re: [Linux-zigbee-devel] [PATCH 2/2] at86rf230: change irq handling to prevent lockups with edge type irq

[Werner Almesberger]

Sascha Herrmann wrote:
> The trigger level isn't configurable in the rf230,

Right, I should have mentioned that the polarity can only be set on
the AT86RF231. The 230 has to make do with rising edge or high level.

> [...] discussion on the linux-zigbee list [...]

I read it now. I think this chip is a bit unusual in that it has a
very generic interface and for many things there is more than one way
to do them (e.g., interrupts, reset, TX_START), which results in
different hardware configurations, and differences in driver
behaviour.

> I fear the sollution to read the interrupt status register in a loop
> (as suggested in your earlier message) would leave chances for race
> conditions or spurious interrupts, depending on wheter interrupts are
> enabled before or after reading the status register. 

I don't think the analysis is worse than for any other solution.
There are also tools and methods that can help if it becomes too
much of a headache.

If you don't like the loop, a double read without loop would work in
this case as well:

	irq = read_and_clear_interrupt();
	enable_irq();
	irq |= read_and_clear_interrupt();
	...

By the way, once we switch to early RX_ON, I think we'll have the
problem that two TRX_END interrupts may be generated without any
host action between them, in which case the first will be
interpreted as the end of sending and the second will be ignored,
leaving a received frame in the buffer, which in turn leaves
dynamic buffer protection on and thus prevents any further
reception.

Not sure yet how to solve this. I also don't know how bad our
latencies are, but I fear that they can at times be substantial.
Already a single register access with spi-gpio takes some 80 us
(measured on an otherwise idle Ben, 336 MHz MIPS).

> Surely for this option, the assumption that (at least nearly) every
> platform supports edge type interrupts should hold.

I think you're on relatively safe ground with the assumption that
most relevant platforms per se can do it. But if the interrupt line
happens to be shared with some other device, then level trigger is
quite popular.

- Werner

Re: [PATCH 2/2] at86rf230: change irq handling to prevent lockups with edge type irq

[Sascha Herrmann]

On 06.04.2013 16:20, Werner Almesberger wrote:

> Sascha Herrmann wrote:
>> The trigger level isn't configurable in the rf230,
> 
> Right, I should have mentioned that the polarity can only be set on
> the AT86RF231. The 230 has to make do with rising edge or high level.


Oh, I didn't realized, that this is possible at all with the rf231...

 
>> I fear the sollution to read the interrupt status register in a loop
>> (as suggested in your earlier message) would leave chances for race
>> conditions or spurious interrupts, depending on wheter interrupts are
>> enabled before or after reading the status register. 
> 
> I don't think the analysis is worse than for any other solution.
> There are also tools and methods that can help if it becomes too
> much of a headache.
> 
> If you don't like the loop, a double read without loop would work in
> this case as well:
> 
> 	irq = read_and_clear_interrupt();
> 	enable_irq();
> 	irq |= read_and_clear_interrupt();
> 	...


Maybe one way to eliminate the extra latency of the second register read
would be to split the interrupt handling function into a generic part
and two different functions to handle the different types of interrupts:

	static void at86rf230_irqwork_level(void) {
		__at86rf230_irqwork();

		spin_lock_irqsave(&lp->lock, flags);
		lp->irq_busy = 0;
		enable_irq()
		spin_unlock_irqrestore(&lp->lock, flags);
	}

For edge type configuration the call to enable_irq() must be removed.
The same would be required for the isr function. The probe function
could than decide, which isr function should be registered for the
interrupt.

> By the way, once we switch to early RX_ON, I think we'll have the
> problem that two TRX_END interrupts may be generated without any
> host action between them, in which case the first will be
> interpreted as the end of sending and the second will be ignored,
> leaving a received frame in the buffer, which in turn leaves
> dynamic buffer protection on and thus prevents any further
> reception.


I think this is right, we should have an eye on this when working on the
early RX_ON...

> Not sure yet how to solve this. I also don't know how bad our
> latencies are, but I fear that they can at times be substantial.
> Already a single register access with spi-gpio takes some 80 us
> (measured on an otherwise idle Ben, 336 MHz MIPS).
> 
>> Surely for this option, the assumption that (at least nearly) every
>> platform supports edge type interrupts should hold.
> 
> I think you're on relatively safe ground with the assumption that
> most relevant platforms per se can do it. But if the interrupt line
> happens to be shared with some other device, then level trigger is
> quite popular.


If you think the solution above would be ok, I could try to send a
version which allows the configuration of trigger type and level.

Thanks,
Sascha

-- 
Hi! I'm a .signature virus! Copy me into your ~/.signature to help me
spread!

Re: [PATCH 2/2] at86rf230: change irq handling to prevent lockups with edge type irq

[Werner Almesberger]

Sascha Herrmann wrote:
> Maybe one way to eliminate the extra latency of the second register read
> would be to split the interrupt handling function into a generic part
> and two different functions to handle the different types of interrupts:

Yes, if you want to optimize the number of register accesses and
work queue invocations, splitting the paths that touch interrupts
seems to be the most straightforward approach.

> If you think the solution above would be ok, I could try to send a
> version which allows the configuration of trigger type and level.

Sounds good to me. Pity the irq_get_irq_type() you mentioned
doesn't exist. That would have made things a bit nicer.

- Werner