[PATCH bluetooth-next 0/4] at86rf230: trac debugfs support

[PATCH bluetooth-next 0/4] at86rf230: trac debugfs support

[Alexander Aring]

Hi,

this patch series based on:

 "[RFCv5 bluetooth-next 0/4] ieee802154: aret handling changes"

and can be used for testing the ARET/NO ARET mode with ARET_ON state only.
I add support for debugfs to check the trac status statistics.

In the previously patch series I said that the at86rf2xx transceivers
checks automatically if ack request is set or not in a 802.15.4 frame.

There exists two cases:

1. When the ackrequest bit is set and using STATE_ARET_ON the transceiver
   will wait for an ack frame after transmit. If ack is received then
   the transceiver logic is "SUCCESS" otherwise "NO_ACK".
2. When the ackrequest bit isn't set and using the STATE_ARET_ON the
   transceiver will not wait for an ack frame and the transceiver logic
   is "SUCCESS".

The transceiver logic is in this case the error code from transmit
algorithmn.

To the test (802.15.4 6LoWPAN):

1. Create some imagine 6LoWPAN node by using:

ip -6 neigh add fe80::abcd lladdr 02:01:02:03:04:05:06:07 dev lowpan0

This will create some neighbor discovery entry for some node which isn't
there in your network. We wan't just test some 802.15.4 unicast addressing.
This unicast addressing will not answer with an ACK frame, because there is
no node with this address.

2. Set ackrequest bit to 0 by using:

iwpan dev wpan0 set ackreq_default 0

3. Ping node:

ping6 fe80::abcd%lowpan0

4. Check trac status stats.

You will see that only "SUCCESS" will be increased, which is the behaviour
on no aret functionality.

5. Do it again but with ackrequest bit to 1 by using:

iwpan dev wpan0 set ackreq_default 1

6. Ping node again.

ping6 fe80::abcd%lowpan0

7. Check the trac status again.

Now, "SUCCESS" isn't increased (it could be for broadcast frames only). But
now you will see that "NO ACK" is increased which is the trac status that
no ack frames was received when using aret functionality.


Some additional notes:

I think the registration of debugfs failed when two at86rf230 will be probed,
because the debugfs "at86rf230" already exists. Is there some way to add a
number to it like the name "at86rf230%d"? Or we just leave it as it is, it's
just a debugging feature and should be disabled then when using two at86rf230
transceivers.

- Alex

changes since RFC becomes PATCH:
 - remove reserved trac value stats and print warning
 - print warning also on tx when trac status is not defined
 - print warning also on rx when trac status is not defined
 - add spi bus name to the debugfs entry of at86rf230
 - add patch for TRX_OFF for when tx_retries is above threshold
 - add patch for make a more detailed warning about edge triggered
   irq's.

Alexander Aring (4):
  at86rf230: change trac status check behaviour
  at86rf230: interrupt tx with force trx_off
  at86rf230: add debugfs support
  at86rf230: detailed edge triggered irq warning

 drivers/net/ieee802154/Kconfig     |   7 ++
 drivers/net/ieee802154/at86rf230.c | 195 ++++++++++++++++++++++++++++---------
 drivers/net/ieee802154/at86rf230.h |   8 ++
 3 files changed, 163 insertions(+), 47 deletions(-)

-- 
2.5.0

[PATCH bluetooth-next 1/4] at86rf230: change trac status check behaviour

[Alexander Aring]

When transmit is done, indicated by trx_end irq, we do first a force
state change to TX_ON and then checking the trac status, if the trac
status is unequal zero we do a state change to TRX_OFF.

This patch changes to the following behaviour, we first check on trac
status after trx_end occurs and then doing a normal change to TX_ON
without do the state change to TRX_OFF when trac status is unequal zero.

The reasons are that the datasheet doesn't described when the trac
status register is cleared, we should doing to evaluate the trac status
at first. The reason to remove the TRX_OFF change if the trac status is
unequal to zero and it was force is the following paragraph inside The
at86rf2xx datasheets:

"Using FORCE_PLL_ON to interrupt an TX_ARET transaction, it is
 recommended to check register bits [7:5] of register address 0x32 for
 value 0. If this value is different, TRX_CMD sequence FORCE_TRX_OFF shall
 be used immediately followed by TRX_CMD sequence PLL_ON. This performs a
 state transition to PLL_ON."

The meaning is here "to interrupt an TX_ARET transaction" in case of
trx_end interrupt the "TX_ARET transaction" is already done and we don't
interrupt the "TX_ARET transaction" by doing the change to TX_ON (PLL_ON)
here. Additional I changed the force change to normal TX_ON which seems to
work here.

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

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 6422caa..39485d0 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -667,28 +667,9 @@ at86rf230_tx_trac_check(void *context)
 {
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
-	const u8 *buf = ctx->buf;
-	const u8 trac = (buf[1] & 0xe0) >> 5;
 
-	/* If trac status is different than zero we need to do a state change
-	 * to STATE_FORCE_TRX_OFF then STATE_RX_AACK_ON to recover the
-	 * transceiver.
-	 */
-	if (trac)
-		at86rf230_async_state_change(lp, ctx, STATE_FORCE_TRX_OFF,
-					     at86rf230_tx_on, true);
-	else
-		at86rf230_tx_on(context);
-}
-
-static void
-at86rf230_tx_trac_status(void *context)
-{
-	struct at86rf230_state_change *ctx = context;
-	struct at86rf230_local *lp = ctx->lp;
-
-	at86rf230_async_read_reg(lp, RG_TRX_STATE, ctx,
-				 at86rf230_tx_trac_check, true);
+	at86rf230_async_state_change(lp, &lp->irq, STATE_TX_ON,
+				     at86rf230_tx_on, true);
 }
 
 static void
@@ -758,10 +739,8 @@ at86rf230_irq_trx_end(struct at86rf230_local *lp)
 {
 	if (lp->is_tx) {
 		lp->is_tx = 0;
-		at86rf230_async_state_change(lp, &lp->irq,
-					     STATE_FORCE_TX_ON,
-					     at86rf230_tx_trac_status,
-					     true);
+		at86rf230_async_read_reg(lp, RG_TRX_STATE, &lp->irq,
+					 at86rf230_tx_trac_check, true);
 	} else {
 		at86rf230_async_read_reg(lp, RG_TRX_STATE, &lp->irq,
 					 at86rf230_rx_trac_check, true);
-- 
2.5.0

[PATCH bluetooth-next 2/4] at86rf230: interrupt tx with force trx_off

[Alexander Aring]

To abort a TX_ARET_BUSY state it's recommended to switch into TRX_OFF
state by doing STATE_TRX_FORCE_OFF. This patch will do always a TRX_OFF
state change when the transceiver stucks in any state. From TRX_OFF we
can switch to the states which are also possible by TX_ON state.

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

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 39485d0..1e71a97 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -377,14 +377,6 @@ at86rf230_async_read_reg(struct at86rf230_local *lp, const u8 reg,
 	}
 }
 
-static inline u8 at86rf230_state_to_force(u8 state)
-{
-	if (state == STATE_TX_ON)
-		return STATE_FORCE_TX_ON;
-	else
-		return STATE_FORCE_TRX_OFF;
-}
-
 static void
 at86rf230_async_state_assert(void *context)
 {
@@ -426,7 +418,7 @@ at86rf230_async_state_assert(void *context)
 				u8 state = ctx->to_state;
 
 				if (lp->tx_retry >= AT86RF2XX_MAX_TX_RETRIES)
-					state = at86rf230_state_to_force(state);
+					state = STATE_FORCE_TRX_OFF;
 				lp->tx_retry++;
 
 				at86rf230_async_state_change(lp, ctx, state,
-- 
2.5.0

[PATCH bluetooth-next 3/4] at86rf230: add debugfs support

[Alexander Aring]

This patch introduce debugfs support for collect trac status stats. To
clear the stats ifdown the interface of at86rf230 and start the
interface again.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 drivers/net/ieee802154/Kconfig     |   7 ++
 drivers/net/ieee802154/at86rf230.c | 158 +++++++++++++++++++++++++++++++++----
 drivers/net/ieee802154/at86rf230.h |   8 ++
 3 files changed, 159 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ieee802154/Kconfig b/drivers/net/ieee802154/Kconfig
index 1dd5ab8..5a614b2 100644
--- a/drivers/net/ieee802154/Kconfig
+++ b/drivers/net/ieee802154/Kconfig
@@ -32,6 +32,13 @@ config IEEE802154_AT86RF230
 	  This driver can also be built as a module. To do so, say M here.
 	  the module will be called 'at86rf230'.
 
+config IEEE802154_AT86RF230_DEBUGFS
+	depends on IEEE802154_AT86RF230
+	bool "AT86RF230 debugfs interface"
+	depends on DEBUG_FS
+	---help---
+	  This option compiles debugfs code for the at86rf230 driver.
+
 config IEEE802154_MRF24J40
 	tristate "Microchip MRF24J40 transceiver driver"
 	depends on IEEE802154_DRIVERS && MAC802154
diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 1e71a97..19c16b9 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -31,6 +31,7 @@
 #include <linux/skbuff.h>
 #include <linux/of_gpio.h>
 #include <linux/ieee802154.h>
+#include <linux/debugfs.h>
 
 #include <net/mac802154.h>
 #include <net/cfg802154.h>
@@ -83,6 +84,15 @@ struct at86rf230_state_change {
 	bool irq_enable;
 };
 
+struct at86rf230_trac {
+	u64 success;
+	u64 success_data_pending;
+	u64 success_wait_for_ack;
+	u64 channel_access_failure;
+	u64 no_ack;
+	u64 invalid;
+};
+
 struct at86rf230_local {
 	struct spi_device *spi;
 
@@ -103,6 +113,8 @@ struct at86rf230_local {
 	u8 tx_retry;
 	struct sk_buff *tx_skb;
 	struct at86rf230_state_change tx;
+
+	struct at86rf230_trac trac;
 };
 
 #define AT86RF2XX_NUMREGS 0x3F
@@ -660,6 +672,31 @@ at86rf230_tx_trac_check(void *context)
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
 
+	if (IS_ENABLED(CONFIG_IEEE802154_AT86RF230_DEBUGFS)) {
+		u8 trac = TRAC_MASK(ctx->buf[1]);
+
+		switch (trac) {
+		case TRAC_SUCCESS:
+			lp->trac.success++;
+			break;
+		case TRAC_SUCCESS_DATA_PENDING:
+			lp->trac.success_data_pending++;
+			break;
+		case TRAC_CHANNEL_ACCESS_FAILURE:
+			lp->trac.channel_access_failure++;
+			break;
+		case TRAC_NO_ACK:
+			lp->trac.no_ack++;
+			break;
+		case TRAC_INVALID:
+			lp->trac.invalid++;
+			break;
+		default:
+			WARN_ONCE(1, "received tx trac status %d\n", trac);
+			break;
+		}
+	}
+
 	at86rf230_async_state_change(lp, &lp->irq, STATE_TX_ON,
 				     at86rf230_tx_on, true);
 }
@@ -696,13 +733,32 @@ at86rf230_rx_read_frame_complete(void *context)
 }
 
 static void
-at86rf230_rx_read_frame(void *context)
+at86rf230_rx_trac_check(void *context)
 {
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
 	u8 *buf = ctx->buf;
 	int rc;
 
+	if (IS_ENABLED(CONFIG_IEEE802154_AT86RF230_DEBUGFS)) {
+		u8 trac = TRAC_MASK(buf[1]);
+
+		switch (trac) {
+		case TRAC_SUCCESS:
+			lp->trac.success++;
+			break;
+		case TRAC_SUCCESS_WAIT_FOR_ACK:
+			lp->trac.success_wait_for_ack++;
+			break;
+		case TRAC_INVALID:
+			lp->trac.invalid++;
+			break;
+		default:
+			WARN_ONCE(1, "received rx trac status %d\n", trac);
+			break;
+		}
+	}
+
 	buf[0] = CMD_FB;
 	ctx->trx.len = AT86RF2XX_MAX_BUF;
 	ctx->msg.complete = at86rf230_rx_read_frame_complete;
@@ -715,18 +771,6 @@ at86rf230_rx_read_frame(void *context)
 }
 
 static void
-at86rf230_rx_trac_check(void *context)
-{
-	/* 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
-	 * maybe timing relevant. Datasheet doesn't say anything about this.
-	 * The programming guide say do it so.
-	 */
-
-	at86rf230_rx_read_frame(context);
-}
-
-static void
 at86rf230_irq_trx_end(struct at86rf230_local *lp)
 {
 	if (lp->is_tx) {
@@ -891,6 +935,10 @@ at86rf230_start(struct ieee802154_hw *hw)
 {
 	struct at86rf230_local *lp = hw->priv;
 
+	/* reset trac stats on start */
+	if (IS_ENABLED(CONFIG_IEEE802154_AT86RF230_DEBUGFS))
+		memset(&lp->trac, 0, sizeof(struct at86rf230_trac));
+
 	at86rf230_awake(lp);
 	enable_irq(lp->spi->irq);
 
@@ -1591,6 +1639,81 @@ at86rf230_setup_spi_messages(struct at86rf230_local *lp)
 	lp->tx.timer.function = at86rf230_async_state_timer;
 }
 
+#ifdef CONFIG_IEEE802154_AT86RF230_DEBUGFS
+static struct dentry *at86rf230_debugfs_root;
+
+static int at86rf230_stats_show(struct seq_file *file, void *offset)
+{
+	struct at86rf230_local *lp = file->private;
+	int ret;
+
+	ret = seq_printf(file, "SUCCESS:\t\t%8llu\n", lp->trac.success);
+	if (ret < 0)
+		return ret;
+
+	ret = seq_printf(file, "SUCCESS_DATA_PENDING:\t%8llu\n",
+			 lp->trac.success_data_pending);
+	if (ret < 0)
+		return ret;
+
+	ret = seq_printf(file, "SUCCESS_WAIT_FOR_ACK:\t%8llu\n",
+			 lp->trac.success_wait_for_ack);
+	if (ret < 0)
+		return ret;
+
+	ret = seq_printf(file, "CHANNEL_ACCESS_FAILURE:\t%8llu\n",
+			 lp->trac.channel_access_failure);
+	if (ret < 0)
+		return ret;
+
+	ret = seq_printf(file, "NO_ACK:\t\t\t%8llu\n", lp->trac.no_ack);
+	if (ret < 0)
+		return ret;
+
+	return seq_printf(file, "INVALID:\t\t%8llu\n", lp->trac.invalid);
+}
+
+static int at86rf230_stats_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, at86rf230_stats_show, inode->i_private);
+}
+
+static const struct file_operations at86rf230_stats_fops = {
+	.open		= at86rf230_stats_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+static int at86rf230_debugfs_init(struct at86rf230_local *lp)
+{
+	char debugfs_dir_name[DNAME_INLINE_LEN + 1] = "at86rf230-";
+	struct dentry *stats;
+
+	strncat(debugfs_dir_name, dev_name(&lp->spi->dev), DNAME_INLINE_LEN);
+
+	at86rf230_debugfs_root = debugfs_create_dir(debugfs_dir_name, NULL);
+	if (!at86rf230_debugfs_root)
+		return -ENOMEM;
+
+	stats = debugfs_create_file("trac_stats", S_IRUGO,
+				    at86rf230_debugfs_root, lp,
+				    &at86rf230_stats_fops);
+	if (!stats)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static void at86rf230_debugfs_remove(void)
+{
+	debugfs_remove_recursive(at86rf230_debugfs_root);
+}
+#else
+static int at86rf230_debugfs_init(struct at86rf230_local *lp) { return 0; }
+static void at86rf230_debugfs_remove(void) { }
+#endif
+
 static int at86rf230_probe(struct spi_device *spi)
 {
 	struct ieee802154_hw *hw;
@@ -1686,12 +1809,18 @@ static int at86rf230_probe(struct spi_device *spi)
 	/* going into sleep by default */
 	at86rf230_sleep(lp);
 
-	rc = ieee802154_register_hw(lp->hw);
+	rc = at86rf230_debugfs_init(lp);
 	if (rc)
 		goto free_dev;
 
+	rc = ieee802154_register_hw(lp->hw);
+	if (rc)
+		goto free_debugfs;
+
 	return rc;
 
+free_debugfs:
+	at86rf230_debugfs_remove();
 free_dev:
 	ieee802154_free_hw(lp->hw);
 
@@ -1706,6 +1835,7 @@ static int at86rf230_remove(struct spi_device *spi)
 	at86rf230_write_subreg(lp, SR_IRQ_MASK, 0);
 	ieee802154_unregister_hw(lp->hw);
 	ieee802154_free_hw(lp->hw);
+	at86rf230_debugfs_remove();
 	dev_dbg(&spi->dev, "unregistered at86rf230\n");
 
 	return 0;
diff --git a/drivers/net/ieee802154/at86rf230.h b/drivers/net/ieee802154/at86rf230.h
index 1e6d1cc..fd9c1f4 100644
--- a/drivers/net/ieee802154/at86rf230.h
+++ b/drivers/net/ieee802154/at86rf230.h
@@ -216,5 +216,13 @@
 #define STATE_TRANSITION_IN_PROGRESS 0x1F
 
 #define TRX_STATE_MASK		(0x1F)
+#define TRAC_MASK(x)		((x & 0xe0) >> 5)
+
+#define TRAC_SUCCESS			0
+#define TRAC_SUCCESS_DATA_PENDING	1
+#define TRAC_SUCCESS_WAIT_FOR_ACK	2
+#define TRAC_CHANNEL_ACCESS_FAILURE	3
+#define TRAC_NO_ACK			5
+#define TRAC_INVALID			7
 
 #endif /* !_AT86RF230_H */
-- 
2.5.0

[PATCH bluetooth-next 4/4] at86rf230: detailed edge triggered irq warning

[Alexander Aring]

This patch introduce a more detailed information why edge triggered
irq's are currently not recommended. It could be that rising/falling
edge detection can happen while the irq is disabled.

Suggested-by: Stefan Schmidt <stefan@osg.samsung.com>
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 19c16b9..b8b0628 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -1376,7 +1376,7 @@ static int at86rf230_hw_init(struct at86rf230_local *lp, u8 xtal_trim)
 	if (irq_type == IRQ_TYPE_EDGE_RISING ||
 	    irq_type == IRQ_TYPE_EDGE_FALLING)
 		dev_warn(&lp->spi->dev,
-			 "Using edge triggered irq's are not recommended!\n");
+			 "Using edge triggered irq's are not recommended, because it can cause races and result in a non-functional driver!\n");
 	if (irq_type == IRQ_TYPE_EDGE_FALLING ||
 	    irq_type == IRQ_TYPE_LEVEL_LOW)
 		irq_pol = IRQ_ACTIVE_LOW;
-- 
2.5.0

Re: [PATCH bluetooth-next 1/4] at86rf230: change trac status check behaviour

[Stefan Schmidt]

Hello.

On 27/08/15 21:49, Alexander Aring wrote:
> When transmit is done, indicated by trx_end irq, we do first a force
> state change to TX_ON and then checking the trac status, if the trac
> status is unequal zero we do a state change to TRX_OFF.
>
> This patch changes to the following behaviour, we first check on trac
> status after trx_end occurs and then doing a normal change to TX_ON
> without do the state change to TRX_OFF when trac status is unequal zero.
>
> The reasons are that the datasheet doesn't described when the trac
> status register is cleared, we should doing to evaluate the trac status
> at first. The reason to remove the TRX_OFF change if the trac status is
> unequal to zero and it was force is the following paragraph inside The
> at86rf2xx datasheets:
>
> "Using FORCE_PLL_ON to interrupt an TX_ARET transaction, it is
>   recommended to check register bits [7:5] of register address 0x32 for
>   value 0. If this value is different, TRX_CMD sequence FORCE_TRX_OFF shall
>   be used immediately followed by TRX_CMD sequence PLL_ON. This performs a
>   state transition to PLL_ON."
>
> The meaning is here "to interrupt an TX_ARET transaction" in case of
> trx_end interrupt the "TX_ARET transaction" is already done and we don't
> interrupt the "TX_ARET transaction" by doing the change to TX_ON (PLL_ON)
> here. Additional I changed the force change to normal TX_ON which seems to
> work here.
>
> Signed-off-by: Alexander Aring<alex.aring@gmail.com>
> ---
>   drivers/net/ieee802154/at86rf230.c | 29 ++++-------------------------
>   1 file changed, 4 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> index 6422caa..39485d0 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -667,28 +667,9 @@ at86rf230_tx_trac_check(void *context)
>   {
>   	struct at86rf230_state_change *ctx = context;
>   	struct at86rf230_local *lp = ctx->lp;
> -	const u8 *buf = ctx->buf;
> -	const u8 trac = (buf[1] & 0xe0) >> 5;
>   
> -	/* If trac status is different than zero we need to do a state change
> -	 * to STATE_FORCE_TRX_OFF then STATE_RX_AACK_ON to recover the
> -	 * transceiver.
> -	 */
> -	if (trac)
> -		at86rf230_async_state_change(lp, ctx, STATE_FORCE_TRX_OFF,
> -					     at86rf230_tx_on, true);
> -	else
> -		at86rf230_tx_on(context);
> -}
> -
> -static void
> -at86rf230_tx_trac_status(void *context)
> -{
> -	struct at86rf230_state_change *ctx = context;
> -	struct at86rf230_local *lp = ctx->lp;
> -
> -	at86rf230_async_read_reg(lp, RG_TRX_STATE, ctx,
> -				 at86rf230_tx_trac_check, true);
> +	at86rf230_async_state_change(lp, &lp->irq, STATE_TX_ON,
> +				     at86rf230_tx_on, true);
>   }
>   
>   static void
> @@ -758,10 +739,8 @@ at86rf230_irq_trx_end(struct at86rf230_local *lp)
>   {
>   	if (lp->is_tx) {
>   		lp->is_tx = 0;
> -		at86rf230_async_state_change(lp, &lp->irq,
> -					     STATE_FORCE_TX_ON,
> -					     at86rf230_tx_trac_status,
> -					     true);
> +		at86rf230_async_read_reg(lp, RG_TRX_STATE, &lp->irq,
> +					 at86rf230_tx_trac_check, true);
>   	} else {
>   		at86rf230_async_read_reg(lp, RG_TRX_STATE, &lp->irq,
>   					 at86rf230_rx_trac_check, true);

Reviewed-by: Stefan Schmidt <stefan@osg.samsung.com>

regards
Stefan Schmidt

Re: [PATCH bluetooth-next 2/4] at86rf230: interrupt tx with force trx_off

[Stefan Schmidt]

Hello.

On 27/08/15 21:49, Alexander Aring wrote:
> To abort a TX_ARET_BUSY state it's recommended to switch into TRX_OFF
> state by doing STATE_TRX_FORCE_OFF. This patch will do always a TRX_OFF
> state change when the transceiver stucks in any state. From TRX_OFF we
> can switch to the states which are also possible by TX_ON state.
>
> Signed-off-by: Alexander Aring<alex.aring@gmail.com>
> ---
>   drivers/net/ieee802154/at86rf230.c | 10 +---------
>   1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> index 39485d0..1e71a97 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -377,14 +377,6 @@ at86rf230_async_read_reg(struct at86rf230_local *lp, const u8 reg,
>   	}
>   }
>   
> -static inline u8 at86rf230_state_to_force(u8 state)
> -{
> -	if (state == STATE_TX_ON)
> -		return STATE_FORCE_TX_ON;
> -	else
> -		return STATE_FORCE_TRX_OFF;
> -}
> -
>   static void
>   at86rf230_async_state_assert(void *context)
>   {
> @@ -426,7 +418,7 @@ at86rf230_async_state_assert(void *context)
>   				u8 state = ctx->to_state;
>   
>   				if (lp->tx_retry >= AT86RF2XX_MAX_TX_RETRIES)
> -					state = at86rf230_state_to_force(state);
> +					state = STATE_FORCE_TRX_OFF;
>   				lp->tx_retry++;
>   
>   				at86rf230_async_state_change(lp, ctx, state,

Reviewed-by: Stefan Schmidt <stefan@osg.samsung.com>

regards
Stefan Schmidt

Re: [PATCH bluetooth-next 3/4] at86rf230: add debugfs support

[Stefan Schmidt]

On 27/08/15 21:49, Alexander Aring wrote:
> This patch introduce debugfs support for collect trac status stats. To
> clear the stats ifdown the interface of at86rf230 and start the
> interface again.
>
> Signed-off-by: Alexander Aring<alex.aring@gmail.com>
> ---
>   drivers/net/ieee802154/Kconfig     |   7 ++
>   drivers/net/ieee802154/at86rf230.c | 158 +++++++++++++++++++++++++++++++++----
>   drivers/net/ieee802154/at86rf230.h |   8 ++
>   3 files changed, 159 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/ieee802154/Kconfig b/drivers/net/ieee802154/Kconfig
> index 1dd5ab8..5a614b2 100644
> --- a/drivers/net/ieee802154/Kconfig
> +++ b/drivers/net/ieee802154/Kconfig
> @@ -32,6 +32,13 @@ config IEEE802154_AT86RF230
>   	  This driver can also be built as a module. To do so, say M here.
>   	  the module will be called 'at86rf230'.
>   
> +config IEEE802154_AT86RF230_DEBUGFS
> +	depends on IEEE802154_AT86RF230
> +	bool "AT86RF230 debugfs interface"
> +	depends on DEBUG_FS
> +	---help---
> +	  This option compiles debugfs code for the at86rf230 driver.
> +
>   config IEEE802154_MRF24J40
>   	tristate "Microchip MRF24J40 transceiver driver"
>   	depends on IEEE802154_DRIVERS && MAC802154
> diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> index 1e71a97..19c16b9 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -31,6 +31,7 @@
>   #include <linux/skbuff.h>
>   #include <linux/of_gpio.h>
>   #include <linux/ieee802154.h>
> +#include <linux/debugfs.h>
>   
>   #include <net/mac802154.h>
>   #include <net/cfg802154.h>
> @@ -83,6 +84,15 @@ struct at86rf230_state_change {
>   	bool irq_enable;
>   };
>   
> +struct at86rf230_trac {
> +	u64 success;
> +	u64 success_data_pending;
> +	u64 success_wait_for_ack;
> +	u64 channel_access_failure;
> +	u64 no_ack;
> +	u64 invalid;
> +};
> +
>   struct at86rf230_local {
>   	struct spi_device *spi;
>   
> @@ -103,6 +113,8 @@ struct at86rf230_local {
>   	u8 tx_retry;
>   	struct sk_buff *tx_skb;
>   	struct at86rf230_state_change tx;
> +
> +	struct at86rf230_trac trac;
>   };
>   
>   #define AT86RF2XX_NUMREGS 0x3F
> @@ -660,6 +672,31 @@ at86rf230_tx_trac_check(void *context)
>   	struct at86rf230_state_change *ctx = context;
>   	struct at86rf230_local *lp = ctx->lp;
>   
> +	if (IS_ENABLED(CONFIG_IEEE802154_AT86RF230_DEBUGFS)) {
> +		u8 trac = TRAC_MASK(ctx->buf[1]);
> +
> +		switch (trac) {
> +		case TRAC_SUCCESS:
> +			lp->trac.success++;
> +			break;
> +		case TRAC_SUCCESS_DATA_PENDING:
> +			lp->trac.success_data_pending++;
> +			break;
> +		case TRAC_CHANNEL_ACCESS_FAILURE:
> +			lp->trac.channel_access_failure++;
> +			break;
> +		case TRAC_NO_ACK:
> +			lp->trac.no_ack++;
> +			break;
> +		case TRAC_INVALID:
> +			lp->trac.invalid++;
> +			break;
> +		default:
> +			WARN_ONCE(1, "received tx trac status %d\n", trac);
> +			break;
> +		}
> +	}
> +
>   	at86rf230_async_state_change(lp, &lp->irq, STATE_TX_ON,
>   				     at86rf230_tx_on, true);
>   }
> @@ -696,13 +733,32 @@ at86rf230_rx_read_frame_complete(void *context)
>   }
>   
>   static void
> -at86rf230_rx_read_frame(void *context)
> +at86rf230_rx_trac_check(void *context)
>   {
>   	struct at86rf230_state_change *ctx = context;
>   	struct at86rf230_local *lp = ctx->lp;
>   	u8 *buf = ctx->buf;
>   	int rc;
>   
> +	if (IS_ENABLED(CONFIG_IEEE802154_AT86RF230_DEBUGFS)) {
> +		u8 trac = TRAC_MASK(buf[1]);
> +
> +		switch (trac) {
> +		case TRAC_SUCCESS:
> +			lp->trac.success++;
> +			break;
> +		case TRAC_SUCCESS_WAIT_FOR_ACK:
> +			lp->trac.success_wait_for_ack++;
> +			break;
> +		case TRAC_INVALID:
> +			lp->trac.invalid++;
> +			break;
> +		default:
> +			WARN_ONCE(1, "received rx trac status %d\n", trac);
> +			break;
> +		}
> +	}
> +
>   	buf[0] = CMD_FB;
>   	ctx->trx.len = AT86RF2XX_MAX_BUF;
>   	ctx->msg.complete = at86rf230_rx_read_frame_complete;
> @@ -715,18 +771,6 @@ at86rf230_rx_read_frame(void *context)
>   }
>   
>   static void
> -at86rf230_rx_trac_check(void *context)
> -{
> -	/* 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
> -	 * maybe timing relevant. Datasheet doesn't say anything about this.
> -	 * The programming guide say do it so.
> -	 */
> -
> -	at86rf230_rx_read_frame(context);
> -}
> -
> -static void
>   at86rf230_irq_trx_end(struct at86rf230_local *lp)
>   {
>   	if (lp->is_tx) {
> @@ -891,6 +935,10 @@ at86rf230_start(struct ieee802154_hw *hw)
>   {
>   	struct at86rf230_local *lp = hw->priv;
>   
> +	/* reset trac stats on start */
> +	if (IS_ENABLED(CONFIG_IEEE802154_AT86RF230_DEBUGFS))
> +		memset(&lp->trac, 0, sizeof(struct at86rf230_trac));
> +
>   	at86rf230_awake(lp);
>   	enable_irq(lp->spi->irq);
>   
> @@ -1591,6 +1639,81 @@ at86rf230_setup_spi_messages(struct at86rf230_local *lp)
>   	lp->tx.timer.function = at86rf230_async_state_timer;
>   }
>   
> +#ifdef CONFIG_IEEE802154_AT86RF230_DEBUGFS
> +static struct dentry *at86rf230_debugfs_root;
> +
> +static int at86rf230_stats_show(struct seq_file *file, void *offset)
> +{
> +	struct at86rf230_local *lp = file->private;
> +	int ret;
> +
> +	ret = seq_printf(file, "SUCCESS:\t\t%8llu\n", lp->trac.success);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = seq_printf(file, "SUCCESS_DATA_PENDING:\t%8llu\n",
> +			 lp->trac.success_data_pending);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = seq_printf(file, "SUCCESS_WAIT_FOR_ACK:\t%8llu\n",
> +			 lp->trac.success_wait_for_ack);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = seq_printf(file, "CHANNEL_ACCESS_FAILURE:\t%8llu\n",
> +			 lp->trac.channel_access_failure);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = seq_printf(file, "NO_ACK:\t\t\t%8llu\n", lp->trac.no_ack);
> +	if (ret < 0)
> +		return ret;
> +
> +	return seq_printf(file, "INVALID:\t\t%8llu\n", lp->trac.invalid);
> +}
> +
> +static int at86rf230_stats_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, at86rf230_stats_show, inode->i_private);
> +}
> +
> +static const struct file_operations at86rf230_stats_fops = {
> +	.open		= at86rf230_stats_open,
> +	.read		= seq_read,
> +	.llseek		= seq_lseek,
> +	.release	= single_release,
> +};
> +
> +static int at86rf230_debugfs_init(struct at86rf230_local *lp)
> +{
> +	char debugfs_dir_name[DNAME_INLINE_LEN + 1] = "at86rf230-";
> +	struct dentry *stats;
> +
> +	strncat(debugfs_dir_name, dev_name(&lp->spi->dev), DNAME_INLINE_LEN);
> +
> +	at86rf230_debugfs_root = debugfs_create_dir(debugfs_dir_name, NULL);
> +	if (!at86rf230_debugfs_root)
> +		return -ENOMEM;
> +
> +	stats = debugfs_create_file("trac_stats", S_IRUGO,
> +				    at86rf230_debugfs_root, lp,
> +				    &at86rf230_stats_fops);
> +	if (!stats)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +static void at86rf230_debugfs_remove(void)
> +{
> +	debugfs_remove_recursive(at86rf230_debugfs_root);
> +}
> +#else
> +static int at86rf230_debugfs_init(struct at86rf230_local *lp) { return 0; }
> +static void at86rf230_debugfs_remove(void) { }
> +#endif
> +
>   static int at86rf230_probe(struct spi_device *spi)
>   {
>   	struct ieee802154_hw *hw;
> @@ -1686,12 +1809,18 @@ static int at86rf230_probe(struct spi_device *spi)
>   	/* going into sleep by default */
>   	at86rf230_sleep(lp);
>   
> -	rc = ieee802154_register_hw(lp->hw);
> +	rc = at86rf230_debugfs_init(lp);
>   	if (rc)
>   		goto free_dev;
>   
> +	rc = ieee802154_register_hw(lp->hw);
> +	if (rc)
> +		goto free_debugfs;
> +
>   	return rc;
>   
> +free_debugfs:
> +	at86rf230_debugfs_remove();
>   free_dev:
>   	ieee802154_free_hw(lp->hw);
>   
> @@ -1706,6 +1835,7 @@ static int at86rf230_remove(struct spi_device *spi)
>   	at86rf230_write_subreg(lp, SR_IRQ_MASK, 0);
>   	ieee802154_unregister_hw(lp->hw);
>   	ieee802154_free_hw(lp->hw);
> +	at86rf230_debugfs_remove();
>   	dev_dbg(&spi->dev, "unregistered at86rf230\n");
>   
>   	return 0;
> diff --git a/drivers/net/ieee802154/at86rf230.h b/drivers/net/ieee802154/at86rf230.h
> index 1e6d1cc..fd9c1f4 100644
> --- a/drivers/net/ieee802154/at86rf230.h
> +++ b/drivers/net/ieee802154/at86rf230.h
> @@ -216,5 +216,13 @@
>   #define STATE_TRANSITION_IN_PROGRESS 0x1F
>   
>   #define TRX_STATE_MASK		(0x1F)
> +#define TRAC_MASK(x)		((x & 0xe0) >> 5)
> +
> +#define TRAC_SUCCESS			0
> +#define TRAC_SUCCESS_DATA_PENDING	1
> +#define TRAC_SUCCESS_WAIT_FOR_ACK	2
> +#define TRAC_CHANNEL_ACCESS_FAILURE	3
> +#define TRAC_NO_ACK			5
> +#define TRAC_INVALID			7
>   
>   #endif /* !_AT86RF230_H */


Thanks a lot for taking my review into account for v2!

Reviewed-by: Stefan Schmidt <stefan@osg.samsung.com>

regards
Stefan Schmidt

Re: [PATCH bluetooth-next 4/4] at86rf230: detailed edge triggered irq warning

[Stefan Schmidt]

Hello.

On 27/08/15 21:49, Alexander Aring wrote:
> This patch introduce a more detailed information why edge triggered
> irq's are currently not recommended. It could be that rising/falling
> edge detection can happen while the irq is disabled.
>
> Suggested-by: Stefan Schmidt<stefan@osg.samsung.com>
> 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 19c16b9..b8b0628 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -1376,7 +1376,7 @@ static int at86rf230_hw_init(struct at86rf230_local *lp, u8 xtal_trim)
>   	if (irq_type == IRQ_TYPE_EDGE_RISING ||
>   	    irq_type == IRQ_TYPE_EDGE_FALLING)
>   		dev_warn(&lp->spi->dev,
> -			 "Using edge triggered irq's are not recommended!\n");
> +			 "Using edge triggered irq's are not recommended, because it can cause races and result in a non-functional driver!\n");
>   	if (irq_type == IRQ_TYPE_EDGE_FALLING ||
>   	    irq_type == IRQ_TYPE_LEVEL_LOW)
>   		irq_pol = IRQ_ACTIVE_LOW;

Reviewed-by: Stefan Schmidt <stefan@osg.samsung.com>

regards
Stefan Schmidt

Re: [PATCH bluetooth-next 0/4] at86rf230: trac debugfs support

[Stefan Schmidt]

Hello.

On 27/08/15 21:49, Alexander Aring wrote:
> Hi,
>
> this patch series based on:
>
>   "[RFCv5 bluetooth-next 0/4] ieee802154: aret handling changes"
>
> and can be used for testing the ARET/NO ARET mode with ARET_ON state only.
> I add support for debugfs to check the trac status statistics.
>
> In the previously patch series I said that the at86rf2xx transceivers
> checks automatically if ack request is set or not in a 802.15.4 frame.
>
> There exists two cases:
>
> 1. When the ackrequest bit is set and using STATE_ARET_ON the transceiver
>     will wait for an ack frame after transmit. If ack is received then
>     the transceiver logic is "SUCCESS" otherwise "NO_ACK".
> 2. When the ackrequest bit isn't set and using the STATE_ARET_ON the
>     transceiver will not wait for an ack frame and the transceiver logic
>     is "SUCCESS".
>
> The transceiver logic is in this case the error code from transmit
> algorithmn.
>
> To the test (802.15.4 6LoWPAN):
>
> 1. Create some imagine 6LoWPAN node by using:
>
> ip -6 neigh add fe80::abcd lladdr 02:01:02:03:04:05:06:07 dev lowpan0
>
> This will create some neighbor discovery entry for some node which isn't
> there in your network. We wan't just test some 802.15.4 unicast addressing.
> This unicast addressing will not answer with an ACK frame, because there is
> no node with this address.
>
> 2. Set ackrequest bit to 0 by using:
>
> iwpan dev wpan0 set ackreq_default 0
>
> 3. Ping node:
>
> ping6 fe80::abcd%lowpan0
>
> 4. Check trac status stats.
>
> You will see that only "SUCCESS" will be increased, which is the behaviour
> on no aret functionality.
>
> 5. Do it again but with ackrequest bit to 1 by using:
>
> iwpan dev wpan0 set ackreq_default 1
>
> 6. Ping node again.
>
> ping6 fe80::abcd%lowpan0
>
> 7. Check the trac status again.
>
> Now, "SUCCESS" isn't increased (it could be for broadcast frames only). But
> now you will see that "NO ACK" is increased which is the trac status that
> no ack frames was received when using aret functionality.
>
>
> Some additional notes:
>
> I think the registration of debugfs failed when two at86rf230 will be probed,
> because the debugfs "at86rf230" already exists. Is there some way to add a
> number to it like the name "at86rf230%d"? Or we just leave it as it is, it's
> just a debugging feature and should be disabled then when using two at86rf230
> transceivers.

root@pi3:~# cat /sys/kernel/debug/at86rf230-spi32766.0/trac_stats
SUCCESS:                    3634
SUCCESS_DATA_PENDING:          0
SUCCESS_WAIT_FOR_ACK:       1259
CHANNEL_ACCESS_FAILURE:      147
NO_ACK:                      206
INVALID:                       0


Gave it a go here and testing worked fine so far. You can add my

Tested-by: Stefan Schmidt <stefan@osg.samsung.com>

for the whole series as well.

regards
Stefan Schmidt

Re: [PATCH bluetooth-next 0/4] at86rf230: trac debugfs support

[Marcel Holtmann]

Hi Alex,

> this patch series based on:
> 
> "[RFCv5 bluetooth-next 0/4] ieee802154: aret handling changes"
> 
> and can be used for testing the ARET/NO ARET mode with ARET_ON state only.
> I add support for debugfs to check the trac status statistics.
> 
> In the previously patch series I said that the at86rf2xx transceivers
> checks automatically if ack request is set or not in a 802.15.4 frame.
> 
> There exists two cases:
> 
> 1. When the ackrequest bit is set and using STATE_ARET_ON the transceiver
>   will wait for an ack frame after transmit. If ack is received then
>   the transceiver logic is "SUCCESS" otherwise "NO_ACK".
> 2. When the ackrequest bit isn't set and using the STATE_ARET_ON the
>   transceiver will not wait for an ack frame and the transceiver logic
>   is "SUCCESS".
> 
> The transceiver logic is in this case the error code from transmit
> algorithmn.
> 
> To the test (802.15.4 6LoWPAN):
> 
> 1. Create some imagine 6LoWPAN node by using:
> 
> ip -6 neigh add fe80::abcd lladdr 02:01:02:03:04:05:06:07 dev lowpan0
> 
> This will create some neighbor discovery entry for some node which isn't
> there in your network. We wan't just test some 802.15.4 unicast addressing.
> This unicast addressing will not answer with an ACK frame, because there is
> no node with this address.
> 
> 2. Set ackrequest bit to 0 by using:
> 
> iwpan dev wpan0 set ackreq_default 0
> 
> 3. Ping node:
> 
> ping6 fe80::abcd%lowpan0
> 
> 4. Check trac status stats.
> 
> You will see that only "SUCCESS" will be increased, which is the behaviour
> on no aret functionality.
> 
> 5. Do it again but with ackrequest bit to 1 by using:
> 
> iwpan dev wpan0 set ackreq_default 1
> 
> 6. Ping node again.
> 
> ping6 fe80::abcd%lowpan0
> 
> 7. Check the trac status again.
> 
> Now, "SUCCESS" isn't increased (it could be for broadcast frames only). But
> now you will see that "NO ACK" is increased which is the trac status that
> no ack frames was received when using aret functionality.
> 
> 
> Some additional notes:
> 
> I think the registration of debugfs failed when two at86rf230 will be probed,
> because the debugfs "at86rf230" already exists. Is there some way to add a
> number to it like the name "at86rf230%d"? Or we just leave it as it is, it's
> just a debugging feature and should be disabled then when using two at86rf230
> transceivers.
> 
> - Alex
> 
> changes since RFC becomes PATCH:
> - remove reserved trac value stats and print warning
> - print warning also on tx when trac status is not defined
> - print warning also on rx when trac status is not defined
> - add spi bus name to the debugfs entry of at86rf230
> - add patch for TRX_OFF for when tx_retries is above threshold
> - add patch for make a more detailed warning about edge triggered
>   irq's.
> 
> Alexander Aring (4):
>  at86rf230: change trac status check behaviour
>  at86rf230: interrupt tx with force trx_off
>  at86rf230: add debugfs support
>  at86rf230: detailed edge triggered irq warning
> 
> drivers/net/ieee802154/Kconfig     |   7 ++
> drivers/net/ieee802154/at86rf230.c | 195 ++++++++++++++++++++++++++++---------
> drivers/net/ieee802154/at86rf230.h |   8 ++
> 3 files changed, 163 insertions(+), 47 deletions(-)

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

Regards

Marcel