[RFC bluetooth-next 0/2] at86rf230: trac debugfs support (for testing aret changes patch-serie)

[RFC bluetooth-next 0/2] at86rf230: trac debugfs support (for testing aret changes patch-serie)

[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

Alexander Aring (2):
  at86rf230: change trac status check behaviour
  at86rf230: add debugfs support

 drivers/net/ieee802154/Kconfig     |   7 ++
 drivers/net/ieee802154/at86rf230.c | 191 ++++++++++++++++++++++++++++++-------
 drivers/net/ieee802154/at86rf230.h |   8 ++
 3 files changed, 169 insertions(+), 37 deletions(-)

-- 
2.5.0

[RFC bluetooth-next 1/2] 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

[RFC bluetooth-next 2/2] 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 | 166 +++++++++++++++++++++++++++++++++----
 drivers/net/ieee802154/at86rf230.h |   8 ++
 3 files changed, 167 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 39485d0..b4a3c11 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,16 @@ 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;
+	u64 reserved;
+};
+
 struct at86rf230_local {
 	struct spi_device *spi;
 
@@ -103,6 +114,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
@@ -668,6 +681,33 @@ 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:
+			lp->trac.reserved++;
+		case TRAC_SUCCESS_WAIT_FOR_ACK:
+			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);
 }
@@ -704,13 +744,36 @@ 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:
+			lp->trac.reserved++;
+		case TRAC_SUCCESS_DATA_PENDING:
+		case TRAC_CHANNEL_ACCESS_FAILURE:
+		case TRAC_NO_ACK:
+			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;
@@ -723,18 +786,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) {
@@ -899,6 +950,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);
 
@@ -1599,6 +1654,82 @@ 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;
+
+	ret = seq_printf(file, "INVALID:\t\t%8llu\n", lp->trac.invalid);
+	if (ret < 0)
+		return ret;
+
+	return seq_printf(file, "RESERVED:\t\t%8llu\n", lp->trac.reserved);
+}
+
+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)
+{
+	struct dentry *stats;
+
+	at86rf230_debugfs_root = debugfs_create_dir("at86rf230", 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;
@@ -1694,12 +1825,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);
 
@@ -1714,6 +1851,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

Re: [RFC bluetooth-next 2/2] at86rf230: add debugfs support

[Stefan Schmidt]

Hello.

On 06/08/15 17:21, 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 | 166 +++++++++++++++++++++++++++++++++----
>   drivers/net/ieee802154/at86rf230.h |   8 ++
>   3 files changed, 167 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 39485d0..b4a3c11 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,16 @@ 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;
> +	u64 reserved;

What are you using the reserved for here? I can see that the trac states 
4 and 6 are not assigned. Are you collecting them here? Why?
> +};
> +
>   struct at86rf230_local {
>   	struct spi_device *spi;
>   
> @@ -103,6 +114,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
> @@ -668,6 +681,33 @@ 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:
> +			lp->trac.reserved++;

Please put the default state at the end of the switch/case statement.
> +		case TRAC_SUCCESS_WAIT_FOR_ACK:
> +			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);
>   }
> @@ -704,13 +744,36 @@ 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:
> +			lp->trac.reserved++;

Default state please at the end.

> +		case TRAC_SUCCESS_DATA_PENDING:
> +		case TRAC_CHANNEL_ACCESS_FAILURE:
> +		case TRAC_NO_ACK:
> +			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;
> @@ -723,18 +786,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) {
> @@ -899,6 +950,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);
>   
> @@ -1599,6 +1654,82 @@ 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;
> +
> +	ret = seq_printf(file, "INVALID:\t\t%8llu\n", lp->trac.invalid);
> +	if (ret < 0)
> +		return ret;
> +
> +	return seq_printf(file, "RESERVED:\t\t%8llu\n", lp->trac.reserved);
> +}
> +
> +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)
> +{
> +	struct dentry *stats;
> +
> +	at86rf230_debugfs_root = debugfs_create_dir("at86rf230", NULL);

If we want to support systems with several transceivers we should switch 
the static at86rf230 above to something like at86rf23-spi-XXXX by using 
the assigned SPI device to identify it cleanly.
> +	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;
> @@ -1694,12 +1825,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);
>   
> @@ -1714,6 +1851,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
Where did you find those? I looked through my copies of the 
at86rf231/233 datasheets and could not find them. Page number would be 
helpful.

regards
Stefan Schmidt

Re: [RFC bluetooth-next 1/2] at86rf230: change trac status check behaviour

[Stefan Schmidt]

Hello.

On 06/08/15 17:21, 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."
I had a hard time finding a register description of 0x32 in my copies. 
Are they outdated or am I just blind? Any hints appreciated. :)

> 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);

I'm all for simplifying this as long as it keeps working. :)

regards
Stefan Schmidt

Re: [RFC bluetooth-next 0/2] at86rf230: trac debugfs support (for testing aret changes patch-serie)

[Stefan Schmidt]

Hello.

On 06/08/15 17:21, 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.

I looked over them and had some feedback. Testing is still pending.

>
> 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.
I commented on this in the actual patch. Maybe we could re-use the spi 
dev name here as this one would be unique. Like at86rf2xx-spi-XXX

regards
Stefan Schmidt

Re: [RFC bluetooth-next 1/2] at86rf230: change trac status check behaviour

[Alexander Aring]

On Thu, Aug 27, 2015 at 11:56:43AM +0200, Stefan Schmidt wrote:
> Hello.
> 
> On 06/08/15 17:21, 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."
> I had a hard time finding a register description of 0x32 in my copies. Are
> they outdated or am I just blind? Any hints appreciated. :)
> 

I think this is a mistake in the datasheet, they mean the "TRAC_STATUS"
here, which is the only value which fits in the range of [7:5] in
Register 0x02.

Nevertheless this handling is for interrupt the TX_ARET transaction,
then maybe this would be more fit if we do a that on the only once
FORCE_TX_ON change, this is at [0] when the maximum of "trying to change
into TX_ON is reached".

This case is very unlikely I would simple move always to FORCE_TRX_OFF
when interrupt any TX_*ON state. From TRX_OFF you can reach
RX_AACK_ON/TX_*ON states on all at86rf2xx(except 230 which we don't
support) transceivers

- Alex

[0] http://lxr.free-electrons.com/source/drivers/net/ieee802154/at86rf230.c#L558

Re: [RFC bluetooth-next 1/2] at86rf230: change trac status check behaviour

[Stefan Schmidt]

Hello.

On 27/08/15 12:13, Alexander Aring wrote:
> On Thu, Aug 27, 2015 at 11:56:43AM +0200, Stefan Schmidt wrote:
>> Hello.
>>
>> On 06/08/15 17:21, 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."
>> I had a hard time finding a register description of 0x32 in my copies. Are
>> they outdated or am I just blind? Any hints appreciated. :)
>>
> I think this is a mistake in the datasheet, they mean the "TRAC_STATUS"
> here, which is the only value which fits in the range of [7:5] in
> Register 0x02.

This makes more sense. Thanks.
Code 4 and 6 and marked as reserved which is what you are counting 
aggregated in the reserved counter.
Not sure if that really buys us anything. We don't know if we get them 
at all (something we will see over time with your patches), we don't 
know which one comes in (count be changed when counting them separately) 
but most important we don't know what they mean and what we should do. :)

I would say ignore them. The counter has no meaning for us.

regards
Stefan Schmidt

Re: [RFC bluetooth-next 1/2] at86rf230: change trac status check behaviour

[Alexander Aring]

On Thu, Aug 27, 2015 at 02:23:30PM +0200, Stefan Schmidt wrote:
> Hello.
> 
> On 27/08/15 12:13, Alexander Aring wrote:
> >On Thu, Aug 27, 2015 at 11:56:43AM +0200, Stefan Schmidt wrote:
> >>Hello.
> >>
> >>On 06/08/15 17:21, 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."
> >>I had a hard time finding a register description of 0x32 in my copies. Are
> >>they outdated or am I just blind? Any hints appreciated. :)
> >>
> >I think this is a mistake in the datasheet, they mean the "TRAC_STATUS"
> >here, which is the only value which fits in the range of [7:5] in
> >Register 0x02.
> 
> This makes more sense. Thanks.
> Code 4 and 6 and marked as reserved which is what you are counting
> aggregated in the reserved counter.
> Not sure if that really buys us anything. We don't know if we get them at
> all (something we will see over time with your patches), we don't know which
> one comes in (count be changed when counting them separately) but most
> important we don't know what they mean and what we should do. :)
> 
> I would say ignore them. The counter has no meaning for us.

ok. I will include this in the default branch which do a "dev_warn" then.

- Alex

Re: [RFC bluetooth-next 1/2] at86rf230: change trac status check behaviour

[Stefan Schmidt]

Hello.

On 27/08/15 14:31, Alexander Aring wrote:
> On Thu, Aug 27, 2015 at 02:23:30PM +0200, Stefan Schmidt wrote:
>> Hello.
>>
>> On 27/08/15 12:13, Alexander Aring wrote:
>>> On Thu, Aug 27, 2015 at 11:56:43AM +0200, Stefan Schmidt wrote:
>>>> Hello.
>>>>
>>>> On 06/08/15 17:21, 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."
>>>> I had a hard time finding a register description of 0x32 in my copies. Are
>>>> they outdated or am I just blind? Any hints appreciated. :)
>>>>
>>> I think this is a mistake in the datasheet, they mean the "TRAC_STATUS"
>>> here, which is the only value which fits in the range of [7:5] in
>>> Register 0x02.
>> This makes more sense. Thanks.
>> Code 4 and 6 and marked as reserved which is what you are counting
>> aggregated in the reserved counter.
>> Not sure if that really buys us anything. We don't know if we get them at
>> all (something we will see over time with your patches), we don't know which
>> one comes in (count be changed when counting them separately) but most
>> important we don't know what they mean and what we should do. :)
>>
>> I would say ignore them. The counter has no meaning for us.
> ok. I will include this in the default branch which do a "dev_warn" then.
>
> - Alex

Sounds good to me. Maybe think about rate limiting it.

regards
Stefan Schmidt