[PATCH bluetooth-next 0/9] at86rf230: various fixes and cleanups

[PATCH bluetooth-next 0/9] at86rf230: various fixes and cleanups

[Alexander Aring]

Hi,

this series fix a race condition when switching to RX_AACK_ON and irq is
enabled. Also contains correct handling of enable_irq if the irq was disabled
before. Additional we fix the handling of lifs/sifs timing in ARET mode.

- Alex

Alexander Aring (9):
  at86rf230: fix errno on tx timeout handling
  at86rf230: add missing error handling
  at86rf230: correct aret lifs and sifs handling
  at86rf230: correct at86rf2xx lifs timings
  at86rf230: squash unnecessary dereferencing
  at86rf230: add missing enable_irq
  at86rf230: fix race condition
  at86rf230: fix enable_irq handling on async spi
  at86rf230: remove unnecessary print of async error

 drivers/net/ieee802154/at86rf230.c | 180 +++++++++++++++++++------------------
 1 file changed, 92 insertions(+), 88 deletions(-)

-- 
2.1.2

[PATCH bluetooth-next 1/9] at86rf230: fix errno on tx timeout handling

[Alexander Aring]

The rc variable is zero if we get a timeout. Instead of pass the rc
variable to the async error handling function which try to recover the
phy, we use a static -ETIMEOUT errno.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index c9d2a75..6857038 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -995,7 +995,7 @@ at86rf230_xmit(struct ieee802154_dev *dev, struct sk_buff *skb)
 	rc = wait_for_completion_interruptible_timeout(&lp->tx_complete,
 						       msecs_to_jiffies(lp->data->t_tx_timeout));
 	if (!rc) {
-		at86rf230_async_error(lp, ctx, rc);
+		at86rf230_async_error(lp, ctx, -ETIMEDOUT);
 		return -ETIMEDOUT;
 	}
 
-- 
2.1.2

Re: [PATCH bluetooth-next 1/9] at86rf230: fix errno on tx timeout handling

[Varka Bhadram]

On 10/07/2014 02:08 PM, Alexander Aring wrote:
> The rc variable is zero if we get a timeout. Instead of pass the rc
> variable to the async error handling function which try to recover the
> phy, we use a static -ETIMEOUT errno.

s/ETIMEOUT/ETIMEDOUT

> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>   drivers/net/ieee802154/at86rf230.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> index c9d2a75..6857038 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -995,7 +995,7 @@ at86rf230_xmit(struct ieee802154_dev *dev, struct sk_buff *skb)
>   	rc = wait_for_completion_interruptible_timeout(&lp->tx_complete,
>   						       msecs_to_jiffies(lp->data->t_tx_timeout));
>   	if (!rc) {
> -		at86rf230_async_error(lp, ctx, rc);
> +		at86rf230_async_error(lp, ctx, -ETIMEDOUT);
>   		return -ETIMEDOUT;
>   	}
>   


-- 
Regards,
Varka Bhadram.

[PATCH bluetooth-next 2/9] at86rf230: add missing error handling

[Alexander Aring]

This patch adds an async error handling function if sync state change
runs into a timeout. The async error handling function tries to recover
the phy state machine into a valid state.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 6857038..44d2f1d 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -690,8 +690,10 @@ at86rf230_sync_state_change(struct at86rf230_local *lp, unsigned int state)
 
 	rc = wait_for_completion_timeout(&lp->state_complete,
 					 msecs_to_jiffies(100));
-	if (!rc)
+	if (!rc) {
+		at86rf230_async_error(lp, &lp->state, -ETIMEDOUT);
 		return -ETIMEDOUT;
+	}
 
 	return 0;
 }
-- 
2.1.2

[PATCH bluetooth-next 3/9] at86rf230: correct aret lifs and sifs handling

[Alexander Aring]

This patch adds lifs/sifs handling only if max_frame_retries is above
zero. The at86rf2xx datasheets says nothing about phy lifs/sifs
handling. I asked the atmel support and they said lifs/sifs is done
by phy when max_frame_retries is above zero.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 44d2f1d..2a25324 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -89,6 +89,7 @@ struct at86rf230_local {
 	struct at86rf230_state_change irq;
 
 	bool tx_aret;
+	s8 max_frame_retries;
 	bool is_tx;
 	/* spinlock for is_tx protection */
 	spinlock_t lock;
@@ -1001,6 +1002,9 @@ at86rf230_xmit(struct ieee802154_dev *dev, struct sk_buff *skb)
 		return -ETIMEDOUT;
 	}
 
+	if (lp->max_frame_retries > 0)
+		return 0;
+
 	/* Interfame spacing time, which is phy depend.
 	 * TODO
 	 * Move this handling in MAC 802.15.4 layer.
@@ -1230,6 +1234,7 @@ at86rf230_set_frame_retries(struct ieee802154_dev *dev, s8 retries)
 		return -EINVAL;
 
 	lp->tx_aret = retries >= 0;
+	lp->max_frame_retries = retries;
 
 	if (retries >= 0)
 		rc = at86rf230_write_subreg(lp, SR_MAX_FRAME_RETRIES, retries);
-- 
2.1.2

Re: [PATCH bluetooth-next 3/9] at86rf230: correct aret lifs and sifs handling

[Varka Bhadram]

Hi Alex,

On 10/07/2014 02:08 PM, Alexander Aring wrote:

> This patch adds lifs/sifs handling only if max_frame_retries is above
> zero. The at86rf2xx datasheets says nothing about phy lifs/sifs
> handling. I asked the atmel support and they said lifs/sifs is done
> by phy when max_frame_retries is above zero.
>
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>   drivers/net/ieee802154/at86rf230.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> index 44d2f1d..2a25324 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -89,6 +89,7 @@ struct at86rf230_local {
>   	struct at86rf230_state_change irq;
>   
>   	bool tx_aret;
> +	s8 max_frame_retries;

Why s8 here..? Is there any reason..

>   	bool is_tx;
>   	/* spinlock for is_tx protection */
>   	spinlock_t lock;
> @@ -1001,6 +1002,9 @@ at86rf230_xmit(struct ieee802154_dev *dev, struct sk_buff *skb)
>   		return -ETIMEDOUT;
>   	}
>   
> +	if (lp->max_frame_retries > 0)
> +		return 0;
> +
>   	/* Interfame spacing time, which is phy depend.
>   	 * TODO
>   	 * Move this handling in MAC 802.15.4 layer.
> @@ -1230,6 +1234,7 @@ at86rf230_set_frame_retries(struct ieee802154_dev *dev, s8 retries)
>   		return -EINVAL;
>   
>   	lp->tx_aret = retries >= 0;
> +	lp->max_frame_retries = retries;
>   
>   	if (retries >= 0)
>   		rc = at86rf230_write_subreg(lp, SR_MAX_FRAME_RETRIES, retries);

-- 
Regards,
Varka Bhadram.

Re: [PATCH bluetooth-next 3/9] at86rf230: correct aret lifs and sifs handling

[Alexander Aring]

On Tue, Oct 07, 2014 at 02:21:44PM +0530, Varka Bhadram wrote:
> Hi Alex,
> 
> On 10/07/2014 02:08 PM, Alexander Aring wrote:
> 
> >This patch adds lifs/sifs handling only if max_frame_retries is above
> >zero. The at86rf2xx datasheets says nothing about phy lifs/sifs
> >handling. I asked the atmel support and they said lifs/sifs is done
> >by phy when max_frame_retries is above zero.
> >
> >Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> >---
> >  drivers/net/ieee802154/at86rf230.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> >diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> >index 44d2f1d..2a25324 100644
> >--- a/drivers/net/ieee802154/at86rf230.c
> >+++ b/drivers/net/ieee802154/at86rf230.c
> >@@ -89,6 +89,7 @@ struct at86rf230_local {
> >  	struct at86rf230_state_change irq;
> >  	bool tx_aret;
> >+	s8 max_frame_retries;
> 
> Why s8 here..? Is there any reason..
> 

yep.

> >  	bool is_tx;
> >  	/* spinlock for is_tx protection */
> >  	spinlock_t lock;
> >@@ -1001,6 +1002,9 @@ at86rf230_xmit(struct ieee802154_dev *dev, struct sk_buff *skb)
> >  		return -ETIMEDOUT;
> >  	}
> >+	if (lp->max_frame_retries > 0)
> >+		return 0;
> >+
> >  	/* Interfame spacing time, which is phy depend.
> >  	 * TODO
> >  	 * Move this handling in MAC 802.15.4 layer.
> >@@ -1230,6 +1234,7 @@ at86rf230_set_frame_retries(struct ieee802154_dev *dev, s8 retries)
> >  		return -EINVAL;
> >  	lp->tx_aret = retries >= 0;
> >+	lp->max_frame_retries = retries;

this parameter retries is s8. The value "-1" means no ARET handling here.

If you do now u8 at max_frame_retries, then we get a overflow by setting
"-1" here and have a invalid max_frame_retries setting of 255.

Then checking via:

if (lp->max_frame_retries > 0)

doesn't work.

- Alex

Re: [PATCH bluetooth-next 3/9] at86rf230: correct aret lifs and sifs handling

[Varka Bhadram]

On 10/07/2014 02:26 PM, Alexander Aring wrote:
> On Tue, Oct 07, 2014 at 02:21:44PM +0530, Varka Bhadram wrote:
>> Hi Alex,
>>
>> On 10/07/2014 02:08 PM, Alexander Aring wrote:
>>
>>> This patch adds lifs/sifs handling only if max_frame_retries is above
>>> zero. The at86rf2xx datasheets says nothing about phy lifs/sifs
>>> handling. I asked the atmel support and they said lifs/sifs is done
>>> by phy when max_frame_retries is above zero.
>>>
>>> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
>>> ---
>>>   drivers/net/ieee802154/at86rf230.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
>>> index 44d2f1d..2a25324 100644
>>> --- a/drivers/net/ieee802154/at86rf230.c
>>> +++ b/drivers/net/ieee802154/at86rf230.c
>>> @@ -89,6 +89,7 @@ struct at86rf230_local {
>>>   	struct at86rf230_state_change irq;
>>>   	bool tx_aret;
>>> +	s8 max_frame_retries;
>> Why s8 here..? Is there any reason..
>>
> yep.
>
>>>   	bool is_tx;
>>>   	/* spinlock for is_tx protection */
>>>   	spinlock_t lock;
>>> @@ -1001,6 +1002,9 @@ at86rf230_xmit(struct ieee802154_dev *dev, struct sk_buff *skb)
>>>   		return -ETIMEDOUT;
>>>   	}
>>> +	if (lp->max_frame_retries > 0)
>>> +		return 0;
>>> +
>>>   	/* Interfame spacing time, which is phy depend.
>>>   	 * TODO
>>>   	 * Move this handling in MAC 802.15.4 layer.
>>> @@ -1230,6 +1234,7 @@ at86rf230_set_frame_retries(struct ieee802154_dev *dev, s8 retries)
>>>   		return -EINVAL;
>>>   	lp->tx_aret = retries >= 0;
>>> +	lp->max_frame_retries = retries;
> this parameter retries is s8. The value "-1" means no ARET handling here.
>
> If you do now u8 at max_frame_retries, then we get a overflow by setting
> "-1" here and have a invalid max_frame_retries setting of 255.
>
> Then checking via:
>
> if (lp->max_frame_retries > 0)
>
> doesn't work.
>
> - Alex

Thanks..

-- 
Regards,
Varka Bhadram.

[PATCH bluetooth-next 4/9] at86rf230: correct at86rf2xx lifs timings

[Alexander Aring]

Symbol rate is 16 us. Lifs is 40 symbols. 16 * 40 = 640 us.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 2a25324..b26135f 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -1267,7 +1267,7 @@ static struct at86rf2xx_chip_data at86rf233_data = {
 	.t_frame = 4096,
 	.t_p_ack = 545,
 	.t_sifs = 192,
-	.t_lifs = 480,
+	.t_lifs = 640,
 	.t_tx_timeout = 2000,
 	.rssi_base_val = -91,
 	.set_channel = at86rf23x_set_channel,
@@ -1283,7 +1283,7 @@ static struct at86rf2xx_chip_data at86rf231_data = {
 	.t_frame = 4096,
 	.t_p_ack = 545,
 	.t_sifs = 192,
-	.t_lifs = 480,
+	.t_lifs = 640,
 	.t_tx_timeout = 2000,
 	.rssi_base_val = -91,
 	.set_channel = at86rf23x_set_channel,
@@ -1299,7 +1299,7 @@ static struct at86rf2xx_chip_data at86rf212_data = {
 	.t_frame = 4096,
 	.t_p_ack = 545,
 	.t_sifs = 192,
-	.t_lifs = 480,
+	.t_lifs = 640,
 	.t_tx_timeout = 2000,
 	.rssi_base_val = -100,
 	.set_channel = at86rf212_set_channel,
-- 
2.1.2

[PATCH bluetooth-next 5/9] at86rf230: squash unnecessary dereferencing

[Alexander Aring]

This patch removes dereferencing irq number over spi struct. Instead we
doing it directly over isr paramater.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index b26135f..09503fb 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -904,7 +904,7 @@ static irqreturn_t at86rf230_isr(int irq, void *data)
 	u8 *buf = ctx->buf;
 	int rc;
 
-	disable_irq_nosync(lp->spi->irq);
+	disable_irq_nosync(irq);
 
 	buf[0] = (RG_IRQ_STATUS & CMD_REG_MASK) | CMD_REG;
 	ctx->trx.len = 2;
-- 
2.1.2

[PATCH bluetooth-next 6/9] at86rf230: add missing enable_irq

[Alexander Aring]

This patch adds a missing enable_irq when spi_async in isr failed.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 09503fb..8754f15 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -911,6 +911,7 @@ static irqreturn_t at86rf230_isr(int irq, void *data)
 	ctx->msg.complete = at86rf230_irq_status;
 	rc = spi_async(lp->spi, &ctx->msg);
 	if (rc) {
+		enable_irq(irq);
 		at86rf230_async_error(lp, ctx, rc);
 		return IRQ_NONE;
 	}
-- 
2.1.2

[PATCH bluetooth-next 7/9] at86rf230: fix race condition

[Alexander Aring]

When the driver waits for a tx completion currently the driver direct
enables the irq. When we switching to RX_AACK_ON some steps afterwards
the driver could receive a new frame and request resources which are
already in use, for example irq state change resource.

To be sure there are no new interrupts when we switching to RX_AACK_ON,
we enable the irq when state change to RX_AACK_ON was completed.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 8754f15..5dbec64 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -705,6 +705,7 @@ at86rf230_tx_complete(void *context)
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
 
+	enable_irq(lp->spi->irq);
 	complete(&lp->tx_complete);
 }
 
@@ -860,7 +861,6 @@ at86rf230_irq_trx_end(struct at86rf230_local *lp)
 	if (lp->is_tx) {
 		lp->is_tx = 0;
 		spin_unlock(&lp->lock);
-		enable_irq(lp->spi->irq);
 
 		if (lp->tx_aret)
 			return at86rf230_async_state_change(lp, &lp->irq,
-- 
2.1.2

[PATCH bluetooth-next 8/9] at86rf230: fix enable_irq handling on async spi

[Alexander Aring]

Sometimes the async state function is call in an context where the spi
irq is diabled. This patch fix the handling to enable the irq when
spi_async failed in the async state change calling chain. We do this by
a context parameter irq_enable and evaluate this parameter when
spi_async failed instead of returning spi_async errno.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 157 ++++++++++++++++++-------------------
 1 file changed, 77 insertions(+), 80 deletions(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 5dbec64..7a1a8e3 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -74,6 +74,8 @@ struct at86rf230_state_change {
 	void (*complete)(void *context);
 	u8 from_state;
 	u8 to_state;
+
+	bool irq_enable;
 };
 
 struct at86rf230_local {
@@ -292,10 +294,11 @@ struct at86rf230_local {
 
 #define AT86RF2XX_NUMREGS 0x3F
 
-static int
+static void
 at86rf230_async_state_change(struct at86rf230_local *lp,
 			     struct at86rf230_state_change *ctx,
-			     const u8 state, void (*complete)(void *context));
+			     const u8 state, void (*complete)(void *context),
+			     const bool irq_enable);
 
 static inline int
 __at86rf230_write(struct at86rf230_local *lp,
@@ -452,7 +455,7 @@ at86rf230_async_error_recover(void *context)
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
 
-	at86rf230_async_state_change(lp, ctx, STATE_RX_AACK_ON, NULL);
+	at86rf230_async_state_change(lp, ctx, STATE_RX_AACK_ON, NULL, false);
 }
 
 static void
@@ -462,21 +465,31 @@ at86rf230_async_error(struct at86rf230_local *lp,
 	dev_err(&lp->spi->dev, "spi_async error %d\n", rc);
 
 	at86rf230_async_state_change(lp, ctx, STATE_FORCE_TRX_OFF,
-				     at86rf230_async_error_recover);
+				     at86rf230_async_error_recover, false);
 }
 
 /* Generic function to get some register value in async mode */
-static int
+static void
 at86rf230_async_read_reg(struct at86rf230_local *lp, const u8 reg,
 			 struct at86rf230_state_change *ctx,
-			 void (*complete)(void *context))
+			 void (*complete)(void *context),
+			 const bool irq_enable)
 {
+	int rc;
+
 	u8 *tx_buf = ctx->buf;
 
 	tx_buf[0] = (reg & CMD_REG_MASK) | CMD_REG;
 	ctx->trx.len = 2;
 	ctx->msg.complete = complete;
-	return spi_async(lp->spi, &ctx->msg);
+	ctx->irq_enable = irq_enable;
+	rc = spi_async(lp->spi, &ctx->msg);
+	if (rc) {
+		if (irq_enable)
+			enable_irq(lp->spi->irq);
+
+		at86rf230_async_error(lp, ctx, rc);
+	}
 }
 
 static void
@@ -513,7 +526,8 @@ at86rf230_async_state_assert(void *context)
 			if (ctx->to_state == STATE_TX_ON) {
 				at86rf230_async_state_change(lp, ctx,
 							     STATE_FORCE_TX_ON,
-							     ctx->complete);
+							     ctx->complete,
+							     ctx->irq_enable);
 				return;
 			}
 		}
@@ -536,7 +550,6 @@ at86rf230_async_state_delay(void *context)
 	struct at86rf230_local *lp = ctx->lp;
 	struct at86rf2xx_chip_data *c = lp->data;
 	bool force = false;
-	int rc;
 
 	/* The force state changes are will show as normal states in the
 	 * state status subregister. We change the to_state to the
@@ -605,10 +618,9 @@ at86rf230_async_state_delay(void *context)
 	udelay(1);
 
 change:
-	rc = at86rf230_async_read_reg(lp, RG_TRX_STATUS, ctx,
-				      at86rf230_async_state_assert);
-	if (rc)
-		dev_err(&lp->spi->dev, "spi_async error %d\n", rc);
+	at86rf230_async_read_reg(lp, RG_TRX_STATUS, ctx,
+				 at86rf230_async_state_assert,
+				 ctx->irq_enable);
 }
 
 static void
@@ -623,10 +635,9 @@ at86rf230_async_state_change_start(void *context)
 	/* Check for "possible" STATE_TRANSITION_IN_PROGRESS */
 	if (trx_state == STATE_TRANSITION_IN_PROGRESS) {
 		udelay(1);
-		rc = at86rf230_async_read_reg(lp, RG_TRX_STATUS, ctx,
-					      at86rf230_async_state_change_start);
-		if (rc)
-			dev_err(&lp->spi->dev, "spi_async error %d\n", rc);
+		at86rf230_async_read_reg(lp, RG_TRX_STATUS, ctx,
+					 at86rf230_async_state_change_start,
+					 ctx->irq_enable);
 		return;
 	}
 
@@ -648,20 +659,28 @@ at86rf230_async_state_change_start(void *context)
 	ctx->trx.len = 2;
 	ctx->msg.complete = at86rf230_async_state_delay;
 	rc = spi_async(lp->spi, &ctx->msg);
-	if (rc)
+	if (rc) {
+		if (ctx->irq_enable)
+			enable_irq(lp->spi->irq);
+
+		at86rf230_async_error(lp, &lp->state, rc);
 		dev_err(&lp->spi->dev, "spi_async error %d\n", rc);
+	}
 }
 
-static int
+static void
 at86rf230_async_state_change(struct at86rf230_local *lp,
 			     struct at86rf230_state_change *ctx,
-			     const u8 state, void (*complete)(void *context))
+			     const u8 state, void (*complete)(void *context),
+			     const bool irq_enable)
 {
 	/* Initialization for the state change context */
 	ctx->to_state = state;
 	ctx->complete = complete;
-	return at86rf230_async_read_reg(lp, RG_TRX_STATUS, ctx,
-					at86rf230_async_state_change_start);
+	ctx->irq_enable = irq_enable;
+	at86rf230_async_read_reg(lp, RG_TRX_STATUS, ctx,
+				 at86rf230_async_state_change_start,
+				 irq_enable);
 }
 
 static void
@@ -682,12 +701,9 @@ at86rf230_sync_state_change(struct at86rf230_local *lp, unsigned int state)
 {
 	int rc;
 
-	rc = at86rf230_async_state_change(lp, &lp->state, state,
-					  at86rf230_sync_state_change_complete);
-	if (rc) {
-		at86rf230_async_error(lp, &lp->state, rc);
-		return rc;
-	}
+	at86rf230_async_state_change(lp, &lp->state, state,
+				     at86rf230_sync_state_change_complete,
+				     false);
 
 	rc = wait_for_completion_timeout(&lp->state_complete,
 					 msecs_to_jiffies(100));
@@ -714,12 +730,9 @@ at86rf230_tx_on(void *context)
 {
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
-	int rc;
 
-	rc = at86rf230_async_state_change(lp, &lp->irq, STATE_RX_AACK_ON,
-					  at86rf230_tx_complete);
-	if (rc)
-		at86rf230_async_error(lp, ctx, rc);
+	at86rf230_async_state_change(lp, &lp->irq, STATE_RX_AACK_ON,
+				     at86rf230_tx_complete, true);
 }
 
 static void
@@ -727,12 +740,9 @@ at86rf230_tx_trac_error(void *context)
 {
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
-	int rc;
 
-	rc = at86rf230_async_state_change(lp, ctx, STATE_TX_ON,
-					  at86rf230_tx_on);
-	if (rc)
-		at86rf230_async_error(lp, ctx, rc);
+	at86rf230_async_state_change(lp, ctx, STATE_TX_ON,
+				     at86rf230_tx_on, true);
 }
 
 static void
@@ -742,17 +752,14 @@ at86rf230_tx_trac_check(void *context)
 	struct at86rf230_local *lp = ctx->lp;
 	const u8 *buf = ctx->buf;
 	const u8 trac = (buf[1] & 0xe0) >> 5;
-	int rc;
 
 	/* If trac status is different than zero we need to do a state change
 	 * to STATE_FORCE_TRX_OFF then STATE_TX_ON to recover the transceiver
 	 * state to TX_ON.
 	 */
 	if (trac) {
-		rc = at86rf230_async_state_change(lp, ctx, STATE_FORCE_TRX_OFF,
-						  at86rf230_tx_trac_error);
-		if (rc)
-			at86rf230_async_error(lp, ctx, rc);
+		at86rf230_async_state_change(lp, ctx, STATE_FORCE_TRX_OFF,
+					     at86rf230_tx_trac_error, true);
 		return;
 	}
 
@@ -765,12 +772,9 @@ at86rf230_tx_trac_status(void *context)
 {
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
-	int rc;
 
-	rc = at86rf230_async_read_reg(lp, RG_TRX_STATE, ctx,
-				      at86rf230_tx_trac_check);
-	if (rc)
-		at86rf230_async_error(lp, ctx, rc);
+	at86rf230_async_read_reg(lp, RG_TRX_STATE, ctx,
+				 at86rf230_tx_trac_check, true);
 }
 
 static void
@@ -823,15 +827,21 @@ at86rf230_rx_read_frame_complete(void *context)
 	at86rf230_rx(lp, buf + 2, len);
 }
 
-static int
+static void
 at86rf230_rx_read_frame(struct at86rf230_local *lp)
 {
+	int rc;
+
 	u8 *buf = lp->irq.buf;
 
 	buf[0] = CMD_FB;
 	lp->irq.trx.len = AT86RF2XX_MAX_BUF;
 	lp->irq.msg.complete = at86rf230_rx_read_frame_complete;
-	return spi_async(lp->spi, &lp->irq.msg);
+	rc = spi_async(lp->spi, &lp->irq.msg);
+	if (rc) {
+		enable_irq(lp->spi->irq);
+		at86rf230_async_error(lp, &lp->irq, rc);
+	}
 }
 
 static void
@@ -839,7 +849,6 @@ at86rf230_rx_trac_check(void *context)
 {
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
-	int rc;
 
 	/* Possible check on trac status here. This could be useful to make
 	 * some stats why receive is failed. Not used at the moment, but it's
@@ -847,14 +856,10 @@ at86rf230_rx_trac_check(void *context)
 	 * The programming guide say do it so.
 	 */
 
-	rc = at86rf230_rx_read_frame(lp);
-	if (rc) {
-		enable_irq(lp->spi->irq);
-		at86rf230_async_error(lp, ctx, rc);
-	}
+	at86rf230_rx_read_frame(lp);
 }
 
-static int
+static void
 at86rf230_irq_trx_end(struct at86rf230_local *lp)
 {
 	spin_lock(&lp->lock);
@@ -863,17 +868,19 @@ at86rf230_irq_trx_end(struct at86rf230_local *lp)
 		spin_unlock(&lp->lock);
 
 		if (lp->tx_aret)
-			return at86rf230_async_state_change(lp, &lp->irq,
-							    STATE_FORCE_TX_ON,
-							    at86rf230_tx_trac_status);
+			at86rf230_async_state_change(lp, &lp->irq,
+						     STATE_FORCE_TX_ON,
+						     at86rf230_tx_trac_status,
+						     true);
 		else
-			return at86rf230_async_state_change(lp, &lp->irq,
-							    STATE_RX_AACK_ON,
-							    at86rf230_tx_complete);
+			at86rf230_async_state_change(lp, &lp->irq,
+						     STATE_RX_AACK_ON,
+						     at86rf230_tx_complete,
+						     true);
 	} else {
 		spin_unlock(&lp->lock);
-		return at86rf230_async_read_reg(lp, RG_TRX_STATE, &lp->irq,
-						at86rf230_rx_trac_check);
+		at86rf230_async_read_reg(lp, RG_TRX_STATE, &lp->irq,
+					 at86rf230_rx_trac_check, true);
 	}
 }
 
@@ -884,12 +891,9 @@ at86rf230_irq_status(void *context)
 	struct at86rf230_local *lp = ctx->lp;
 	const u8 *buf = lp->irq.buf;
 	const u8 irq = buf[1];
-	int rc;
 
 	if (irq & IRQ_TRX_END) {
-		rc = at86rf230_irq_trx_end(lp);
-		if (rc)
-			at86rf230_async_error(lp, ctx, rc);
+		at86rf230_irq_trx_end(lp);
 	} else {
 		enable_irq(lp->spi->irq);
 		dev_err(&lp->spi->dev, "not supported irq %02x received\n",
@@ -964,12 +968,9 @@ at86rf230_xmit_tx_on(void *context)
 {
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
-	int rc;
 
-	rc = at86rf230_async_state_change(lp, ctx, STATE_TX_ARET_ON,
-					  at86rf230_write_frame);
-	if (rc)
-		at86rf230_async_error(lp, ctx, rc);
+	at86rf230_async_state_change(lp, ctx, STATE_TX_ARET_ON,
+				     at86rf230_write_frame, false);
 }
 
 static int
@@ -990,12 +991,8 @@ at86rf230_xmit(struct ieee802154_dev *dev, struct sk_buff *skb)
 	if (lp->tx_aret)
 		tx_complete = at86rf230_xmit_tx_on;
 
-	rc = at86rf230_async_state_change(lp, ctx, STATE_TX_ON,
-					  tx_complete);
-	if (rc) {
-		at86rf230_async_error(lp, ctx, rc);
-		return rc;
-	}
+	at86rf230_async_state_change(lp, ctx, STATE_TX_ON, tx_complete, false);
+
 	rc = wait_for_completion_interruptible_timeout(&lp->tx_complete,
 						       msecs_to_jiffies(lp->data->t_tx_timeout));
 	if (!rc) {
-- 
2.1.2

[PATCH bluetooth-next 9/9] at86rf230: remove unnecessary print of async error

[Alexander Aring]

The async error function will already printout the errno over dev_err.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/at86rf230.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 7a1a8e3..6ebd665 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -664,7 +664,6 @@ at86rf230_async_state_change_start(void *context)
 			enable_irq(lp->spi->irq);
 
 		at86rf230_async_error(lp, &lp->state, rc);
-		dev_err(&lp->spi->dev, "spi_async error %d\n", rc);
 	}
 }
 
-- 
2.1.2

Re: [PATCH bluetooth-next 0/9] at86rf230: various fixes and cleanups

[Marcel Holtmann]

Hi Alex,

> this series fix a race condition when switching to RX_AACK_ON and irq is
> enabled. Also contains correct handling of enable_irq if the irq was disabled
> before. Additional we fix the handling of lifs/sifs timing in ARET mode.
> 
> - Alex
> 
> Alexander Aring (9):
>  at86rf230: fix errno on tx timeout handling
>  at86rf230: add missing error handling
>  at86rf230: correct aret lifs and sifs handling
>  at86rf230: correct at86rf2xx lifs timings
>  at86rf230: squash unnecessary dereferencing
>  at86rf230: add missing enable_irq
>  at86rf230: fix race condition
>  at86rf230: fix enable_irq handling on async spi
>  at86rf230: remove unnecessary print of async error
> 
> drivers/net/ieee802154/at86rf230.c | 180 +++++++++++++++++++------------------
> 1 file changed, 92 insertions(+), 88 deletions(-)

all 9 patches have been applied to bluetooth-next tree.

Regards

Marcel