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

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

[Alexander Aring]

Hi,

this patch series contains a RFC for changing the max_frame_retries behaviour.
The current behaviour is that we allow a range from -1 until 7 (802.15.4
standard describes a range from 0 until 7 only). Nevertheless for historical
reason we have the "-1" value, which describes no aret handling.

The point is when the ack request bit is set inside the mac header the
transceiver need to handle the ack somehow, this is mostly the aret mode
which is done by hardware. Instead to having this as mac setting which doesn't
allow a change during interface up, we simple do this handling per frame
indicated by the ack request bit.

Instead of max_frame_retries "-1" value which could be useful for making a
default ack request bit handling if no information is given if it should be
set or not, we have now the ack request default entry inside the mib.

This could be useful for the 6LoWPAN stack which have this as default
behaviour for data frames.

- Alex

changes since v5:
 - complete different handling for support no aret/aret mode in at86rf230.
   The reason is: I finally figured out that the TX_ARET state will self
   detect if the acknowledge bit is set or not. This is a little embarrassing
   because I thought always ARET means always to wait for the ack frame, but
   it isn't. But this answers another question because TX_ON mode will not
   do any CSMA-CA handling before, so then we could not have any CSMA-CA handling
   by sending frames without acknowledge request bit. Now we always do CSMA-CA
   handling and the ARET state do the rest.
   All at86rf230 users will expierence now by default CSMA handling, this will make
   some more (random more) delays per transmit. You can disable this handling by
   setting minBe = 0. E.g "iwpan dev wpan0 set backoff_exponents 0 3".
 - I also thought about to add a capability for if the transceiver supports no aret
   handling. But if the ackreq bit is set then nothing at functionality will be
   changed, the transceiver don't know then if sending was successful or not by
   waiting for the acknowledge frame. NO ARET able transceivers should only try
   to avoid the acknowledge request bit at 802.15.4 frame which is done by the
   ackreq default setting which is false by default. We cannot complete avoid
   the sending frames where the acknowledge request bit is not set.

changes since v4:
 - change language suggestions at patch 2/6
 - change "max_frame_retries" to "frame_retries" inside commit messages, when
   the kernel value "frame_retries" is meant.
 - add several reviewed-by tags to patches which was reviewed

changes since v3:
 - remove unnecessary brackets inside of ieee802154_is_ackreq
 - use ieee802154_get_fc_from_skb __get_unaligned_memmove16 instead memcpy

changes since v2:
 - add patch "mac802154: cfg: remove test and set checks" because Phoebe
   notice that in v1 and we did that at several other places. Makes only
   sense on a followed another layer call like driverops.
 - remove if branch in "at86rf230: use aret mode if ackreq is set while xmit"
   for check if aret handling is necessary.
 - add patch "at86rf230: remove max_frame_retries -1 check" because previous
   patch removed the "-1" max_frame_retries handling.

Alexander Aring (4):
  mac802154: cfg: remove test and set checks
  mac802154: change frame_retries behaviour
  at86rf230: use STATE_TX_ARET mode only
  ieee802154: add ack request default handling

 drivers/net/ieee802154/at86rf230.c | 52 ++++++++++----------------------------
 include/net/cfg802154.h            |  5 ++++
 include/net/nl802154.h             |  4 +++
 net/ieee802154/6lowpan/tx.c        |  2 +-
 net/ieee802154/nl802154.c          | 33 ++++++++++++++++++++++++
 net/ieee802154/rdev-ops.h          | 13 ++++++++++
 net/ieee802154/trace.h             | 19 ++++++++++++++
 net/mac802154/cfg.c                | 27 ++++++++------------
 net/mac802154/iface.c              |  3 +--
 net/mac802154/main.c               |  9 +++----
 10 files changed, 104 insertions(+), 63 deletions(-)

-- 
2.5.0

[RFCv5 bluetooth-next 1/4] mac802154: cfg: remove test and set checks

[Alexander Aring]

This patch removes several checks if a value is really changed. This
makes only sense if we have another layer call e.g. calling the
driver_ops which is done by callbacks like "set_channel".

For MAC settings which need to be set by phy registers (if the phy
supports that handling) this is set by doing an interface up currently
and are not direct driver_ops calls, so we remove the checks from these
configuration callbacks.

Reviewed-by: Stefan Schmidt <stefan@osg.samsung.com>
Suggested-by: Phoebe Buckheister <phoebe.buckheister@itwm.fraunhofer.de>
Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 net/mac802154/cfg.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
index f7ba51e..cecfcda 100644
--- a/net/mac802154/cfg.c
+++ b/net/mac802154/cfg.c
@@ -209,10 +209,6 @@ ieee802154_set_backoff_exponent(struct wpan_phy *wpan_phy,
 {
 	ASSERT_RTNL();
 
-	if (wpan_dev->min_be == min_be &&
-	    wpan_dev->max_be == max_be)
-		return 0;
-
 	wpan_dev->min_be = min_be;
 	wpan_dev->max_be = max_be;
 	return 0;
@@ -224,9 +220,6 @@ ieee802154_set_short_addr(struct wpan_phy *wpan_phy, struct wpan_dev *wpan_dev,
 {
 	ASSERT_RTNL();
 
-	if (wpan_dev->short_addr == short_addr)
-		return 0;
-
 	wpan_dev->short_addr = short_addr;
 	return 0;
 }
@@ -238,9 +231,6 @@ ieee802154_set_max_csma_backoffs(struct wpan_phy *wpan_phy,
 {
 	ASSERT_RTNL();
 
-	if (wpan_dev->csma_retries == max_csma_backoffs)
-		return 0;
-
 	wpan_dev->csma_retries = max_csma_backoffs;
 	return 0;
 }
@@ -252,9 +242,6 @@ ieee802154_set_max_frame_retries(struct wpan_phy *wpan_phy,
 {
 	ASSERT_RTNL();
 
-	if (wpan_dev->frame_retries == max_frame_retries)
-		return 0;
-
 	wpan_dev->frame_retries = max_frame_retries;
 	return 0;
 }
@@ -265,9 +252,6 @@ ieee802154_set_lbt_mode(struct wpan_phy *wpan_phy, struct wpan_dev *wpan_dev,
 {
 	ASSERT_RTNL();
 
-	if (wpan_dev->lbt == mode)
-		return 0;
-
 	wpan_dev->lbt = mode;
 	return 0;
 }
-- 
2.5.0

[RFCv5 bluetooth-next 2/4] mac802154: change frame_retries behaviour

[Alexander Aring]

This patch changes the default minimum value of frame_retries to 0 and
changes the frame_retries default value to 3 which is also 802.15.4
default.

We don't use the frame_retries "-1" value as indicator for no-aret mode
anymore, instead we checking on the ack request bit inside the 802.15.4
frame control field. This allows a acknowledge handling per frame. This
checking is done by transceiver or inside xmit callback of driver layer.

If a transceiver doesn't support ARET handling the transmit
functionality ignores ack frames then, which isn't well but should not
effect anything of current functionality.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 net/mac802154/iface.c | 3 +--
 net/mac802154/main.c  | 9 +++------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
index 416de90..8837c5a 100644
--- a/net/mac802154/iface.c
+++ b/net/mac802154/iface.c
@@ -483,8 +483,7 @@ ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata,
 	wpan_dev->min_be = 3;
 	wpan_dev->max_be = 5;
 	wpan_dev->csma_retries = 4;
-	/* for compatibility, actual default is 3 */
-	wpan_dev->frame_retries = -1;
+	wpan_dev->frame_retries = 3;
 
 	wpan_dev->pan_id = cpu_to_le16(IEEE802154_PANID_BROADCAST);
 	wpan_dev->short_addr = cpu_to_le16(IEEE802154_ADDR_BROADCAST);
diff --git a/net/mac802154/main.c b/net/mac802154/main.c
index 9e55431..e8cab5b 100644
--- a/net/mac802154/main.c
+++ b/net/mac802154/main.c
@@ -111,7 +111,7 @@ ieee802154_alloc_hw(size_t priv_data_len, const struct ieee802154_ops *ops)
 	phy->supported.max_minbe = 8;
 	phy->supported.min_maxbe = 3;
 	phy->supported.max_maxbe = 8;
-	phy->supported.min_frame_retries = -1;
+	phy->supported.min_frame_retries = 0;
 	phy->supported.max_frame_retries = 7;
 	phy->supported.max_csma_backoffs = 5;
 	phy->supported.lbt = NL802154_SUPPORTED_BOOL_FALSE;
@@ -177,11 +177,8 @@ int ieee802154_register_hw(struct ieee802154_hw *hw)
 	}
 
 	if (!(hw->flags & IEEE802154_HW_FRAME_RETRIES)) {
-		/* TODO should be 3, but our default value is -1 which means
-		 * no ARET handling.
-		 */
-		local->phy->supported.min_frame_retries = -1;
-		local->phy->supported.max_frame_retries = -1;
+		local->phy->supported.min_frame_retries = 3;
+		local->phy->supported.max_frame_retries = 3;
 	}
 
 	if (hw->flags & IEEE802154_HW_PROMISCUOUS)
-- 
2.5.0

[RFCv5 bluetooth-next 3/4] at86rf230: use STATE_TX_ARET mode only

[Alexander Aring]

This patch changes the state change behaviour of at86rf230 to always
TX_ARET mode. According the at86rf2xx datasheets TX_ARET mode doesn't
mean to be always waiting for ack frames after transmit. The transceiver
will automatically wait for ack frames or not if the acknowledge request
bit is set. See section "TX_ARET_ON – Transmit with Automatic Frame
Retransmission and CSMA-CA Retry".

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

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index d0d5bf6..6422caa 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -97,9 +97,7 @@ struct at86rf230_local {
 
 	struct at86rf230_state_change irq;
 
-	bool tx_aret;
 	unsigned long cal_timeout;
-	s8 max_frame_retries;
 	bool is_tx;
 	bool is_tx_from_off;
 	u8 tx_retry;
@@ -651,7 +649,7 @@ at86rf230_tx_complete(void *context)
 
 	enable_irq(ctx->irq);
 
-	ieee802154_xmit_complete(lp->hw, lp->tx_skb, !lp->tx_aret);
+	ieee802154_xmit_complete(lp->hw, lp->tx_skb, false);
 }
 
 static void
@@ -760,17 +758,10 @@ at86rf230_irq_trx_end(struct at86rf230_local *lp)
 {
 	if (lp->is_tx) {
 		lp->is_tx = 0;
-
-		if (lp->tx_aret)
-			at86rf230_async_state_change(lp, &lp->irq,
-						     STATE_FORCE_TX_ON,
-						     at86rf230_tx_trac_status,
-						     true);
-		else
-			at86rf230_async_state_change(lp, &lp->irq,
-						     STATE_RX_AACK_ON,
-						     at86rf230_tx_complete,
-						     true);
+		at86rf230_async_state_change(lp, &lp->irq,
+					     STATE_FORCE_TX_ON,
+					     at86rf230_tx_trac_status,
+					     true);
 	} else {
 		at86rf230_async_read_reg(lp, RG_TRX_STATE, &lp->irq,
 					 at86rf230_rx_trac_check, true);
@@ -876,24 +867,16 @@ at86rf230_xmit_start(void *context)
 	struct at86rf230_state_change *ctx = context;
 	struct at86rf230_local *lp = ctx->lp;
 
-	/* In ARET mode we need to go into STATE_TX_ARET_ON after we
-	 * are in STATE_TX_ON. The pfad differs here, so we change
-	 * the complete handler.
-	 */
-	if (lp->tx_aret) {
-		if (lp->is_tx_from_off) {
-			lp->is_tx_from_off = false;
-			at86rf230_async_state_change(lp, ctx, STATE_TX_ARET_ON,
-						     at86rf230_write_frame,
-						     false);
-		} else {
-			at86rf230_async_state_change(lp, ctx, STATE_TX_ON,
-						     at86rf230_xmit_tx_on,
-						     false);
-		}
+	/* check if we change from off state */
+	if (lp->is_tx_from_off) {
+		lp->is_tx_from_off = false;
+		at86rf230_async_state_change(lp, ctx, STATE_TX_ARET_ON,
+					     at86rf230_write_frame,
+					     false);
 	} else {
 		at86rf230_async_state_change(lp, ctx, STATE_TX_ON,
-					     at86rf230_write_frame, false);
+					     at86rf230_xmit_tx_on,
+					     false);
 	}
 }
 
@@ -1267,15 +1250,8 @@ static int
 at86rf230_set_frame_retries(struct ieee802154_hw *hw, s8 retries)
 {
 	struct at86rf230_local *lp = hw->priv;
-	int rc = 0;
-
-	lp->tx_aret = retries >= 0;
-	lp->max_frame_retries = retries;
 
-	if (retries >= 0)
-		rc = at86rf230_write_subreg(lp, SR_MAX_FRAME_RETRIES, retries);
-
-	return rc;
+	return at86rf230_write_subreg(lp, SR_MAX_FRAME_RETRIES, retries);
 }
 
 static int
-- 
2.5.0

[RFCv5 bluetooth-next 4/4] ieee802154: add ack request default handling

[Alexander Aring]

This patch introduce a new mib entry which isn't part of 802.15.4 but
useful as default behaviour to set the ack request bit or not if we
don't know if the ack request bit should set. This is currently used for
stacks like IEEE 802.15.4 6LoWPAN.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 include/net/cfg802154.h     |  5 +++++
 include/net/nl802154.h      |  4 ++++
 net/ieee802154/6lowpan/tx.c |  2 +-
 net/ieee802154/nl802154.c   | 33 +++++++++++++++++++++++++++++++++
 net/ieee802154/rdev-ops.h   | 13 +++++++++++++
 net/ieee802154/trace.h      | 19 +++++++++++++++++++
 net/mac802154/cfg.c         | 11 +++++++++++
 7 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
index 382f94b..21353f8 100644
--- a/include/net/cfg802154.h
+++ b/include/net/cfg802154.h
@@ -63,6 +63,8 @@ struct cfg802154_ops {
 					 s8 max_frame_retries);
 	int	(*set_lbt_mode)(struct wpan_phy *wpan_phy,
 				struct wpan_dev *wpan_dev, bool mode);
+	int	(*set_ackreq_default)(struct wpan_phy *wpan_phy,
+				      struct wpan_dev *wpan_dev, bool ackreq);
 };
 
 static inline bool
@@ -193,6 +195,9 @@ struct wpan_dev {
 	bool lbt;
 
 	bool promiscuous_mode;
+
+	/* fallback for acknowledgment bit setting */
+	bool ackreq;
 };
 
 #define to_phy(_dev)	container_of(_dev, struct wpan_phy, dev)
diff --git a/include/net/nl802154.h b/include/net/nl802154.h
index b0ab530..cf2713d 100644
--- a/include/net/nl802154.h
+++ b/include/net/nl802154.h
@@ -52,6 +52,8 @@ enum nl802154_commands {
 
 	NL802154_CMD_SET_LBT_MODE,
 
+	NL802154_CMD_SET_ACKREQ_DEFAULT,
+
 	/* add new commands above here */
 
 	/* used to define NL802154_CMD_MAX below */
@@ -104,6 +106,8 @@ enum nl802154_attrs {
 
 	NL802154_ATTR_SUPPORTED_COMMANDS,
 
+	NL802154_ATTR_ACKREQ_DEFAULT,
+
 	/* add attributes here, update the policy in nl802154.c */
 
 	__NL802154_ATTR_AFTER_LAST,
diff --git a/net/ieee802154/6lowpan/tx.c b/net/ieee802154/6lowpan/tx.c
index 2597abb..1bf4a30 100644
--- a/net/ieee802154/6lowpan/tx.c
+++ b/net/ieee802154/6lowpan/tx.c
@@ -224,7 +224,7 @@ static int lowpan_header(struct sk_buff *skb, struct net_device *dev)
 	} else {
 		da.mode = IEEE802154_ADDR_LONG;
 		da.extended_addr = ieee802154_devaddr_from_raw(daddr);
-		cb->ackreq = wpan_dev->frame_retries >= 0;
+		cb->ackreq = wpan_dev->ackreq;
 	}
 
 	return dev_hard_header(skb, lowpan_dev_info(dev)->real_dev,
diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
index 68f2401..1b00a14 100644
--- a/net/ieee802154/nl802154.c
+++ b/net/ieee802154/nl802154.c
@@ -230,6 +230,8 @@ static const struct nla_policy nl802154_policy[NL802154_ATTR_MAX+1] = {
 	[NL802154_ATTR_WPAN_PHY_CAPS] = { .type = NLA_NESTED },
 
 	[NL802154_ATTR_SUPPORTED_COMMANDS] = { .type = NLA_NESTED },
+
+	[NL802154_ATTR_ACKREQ_DEFAULT] = { .type = NLA_U8 },
 };
 
 /* message building helper */
@@ -458,6 +460,7 @@ static int nl802154_send_wpan_phy(struct cfg802154_registered_device *rdev,
 	CMD(set_max_csma_backoffs, SET_MAX_CSMA_BACKOFFS);
 	CMD(set_max_frame_retries, SET_MAX_FRAME_RETRIES);
 	CMD(set_lbt_mode, SET_LBT_MODE);
+	CMD(set_ackreq_default, SET_ACKREQ_DEFAULT);
 
 	if (rdev->wpan_phy.flags & WPAN_PHY_FLAG_TXPOWER)
 		CMD(set_tx_power, SET_TX_POWER);
@@ -656,6 +659,10 @@ nl802154_send_iface(struct sk_buff *msg, u32 portid, u32 seq, int flags,
 	if (nla_put_u8(msg, NL802154_ATTR_LBT_MODE, wpan_dev->lbt))
 		goto nla_put_failure;
 
+	/* ackreq default behaviour */
+	if (nla_put_u8(msg, NL802154_ATTR_ACKREQ_DEFAULT, wpan_dev->ackreq))
+		goto nla_put_failure;
+
 	genlmsg_end(msg, hdr);
 	return 0;
 
@@ -1042,6 +1049,24 @@ static int nl802154_set_lbt_mode(struct sk_buff *skb, struct genl_info *info)
 	return rdev_set_lbt_mode(rdev, wpan_dev, mode);
 }
 
+static int
+nl802154_set_ackreq_default(struct sk_buff *skb, struct genl_info *info)
+{
+	struct cfg802154_registered_device *rdev = info->user_ptr[0];
+	struct net_device *dev = info->user_ptr[1];
+	struct wpan_dev *wpan_dev = dev->ieee802154_ptr;
+	bool ackreq;
+
+	if (netif_running(dev))
+		return -EBUSY;
+
+	if (!info->attrs[NL802154_ATTR_ACKREQ_DEFAULT])
+		return -EINVAL;
+
+	ackreq = !!nla_get_u8(info->attrs[NL802154_ATTR_ACKREQ_DEFAULT]);
+	return rdev_set_ackreq_default(rdev, wpan_dev, ackreq);
+}
+
 #define NL802154_FLAG_NEED_WPAN_PHY	0x01
 #define NL802154_FLAG_NEED_NETDEV	0x02
 #define NL802154_FLAG_NEED_RTNL		0x04
@@ -1248,6 +1273,14 @@ static const struct genl_ops nl802154_ops[] = {
 		.internal_flags = NL802154_FLAG_NEED_NETDEV |
 				  NL802154_FLAG_NEED_RTNL,
 	},
+	{
+		.cmd = NL802154_CMD_SET_ACKREQ_DEFAULT,
+		.doit = nl802154_set_ackreq_default,
+		.policy = nl802154_policy,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL802154_FLAG_NEED_NETDEV |
+				  NL802154_FLAG_NEED_RTNL,
+	},
 };
 
 /* initialisation/exit functions */
diff --git a/net/ieee802154/rdev-ops.h b/net/ieee802154/rdev-ops.h
index 8d5960a..03b3575 100644
--- a/net/ieee802154/rdev-ops.h
+++ b/net/ieee802154/rdev-ops.h
@@ -195,4 +195,17 @@ rdev_set_lbt_mode(struct cfg802154_registered_device *rdev,
 	return ret;
 }
 
+static inline int
+rdev_set_ackreq_default(struct cfg802154_registered_device *rdev,
+			struct wpan_dev *wpan_dev, bool ackreq)
+{
+	int ret;
+
+	trace_802154_rdev_set_ackreq_default(&rdev->wpan_phy, wpan_dev,
+					     ackreq);
+	ret = rdev->ops->set_ackreq_default(&rdev->wpan_phy, wpan_dev, ackreq);
+	trace_802154_rdev_return_int(&rdev->wpan_phy, ret);
+	return ret;
+}
+
 #endif /* __CFG802154_RDEV_OPS */
diff --git a/net/ieee802154/trace.h b/net/ieee802154/trace.h
index 4399b7f..9a471e4 100644
--- a/net/ieee802154/trace.h
+++ b/net/ieee802154/trace.h
@@ -275,6 +275,25 @@ TRACE_EVENT(802154_rdev_set_lbt_mode,
 		WPAN_DEV_PR_ARG, BOOL_TO_STR(__entry->mode))
 );
 
+TRACE_EVENT(802154_rdev_set_ackreq_default,
+	TP_PROTO(struct wpan_phy *wpan_phy, struct wpan_dev *wpan_dev,
+		 bool ackreq),
+	TP_ARGS(wpan_phy, wpan_dev, ackreq),
+	TP_STRUCT__entry(
+		WPAN_PHY_ENTRY
+		WPAN_DEV_ENTRY
+		__field(bool, ackreq)
+	),
+	TP_fast_assign(
+		WPAN_PHY_ASSIGN;
+		WPAN_DEV_ASSIGN;
+		__entry->ackreq = ackreq;
+	),
+	TP_printk(WPAN_PHY_PR_FMT ", " WPAN_DEV_PR_FMT
+		", ackreq default: %s", WPAN_PHY_PR_ARG,
+		WPAN_DEV_PR_ARG, BOOL_TO_STR(__entry->ackreq))
+);
+
 TRACE_EVENT(802154_rdev_return_int,
 	TP_PROTO(struct wpan_phy *wpan_phy, int ret),
 	TP_ARGS(wpan_phy, ret),
diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
index cecfcda..c865ebb 100644
--- a/net/mac802154/cfg.c
+++ b/net/mac802154/cfg.c
@@ -256,6 +256,16 @@ ieee802154_set_lbt_mode(struct wpan_phy *wpan_phy, struct wpan_dev *wpan_dev,
 	return 0;
 }
 
+static int
+ieee802154_set_ackreq_default(struct wpan_phy *wpan_phy,
+			      struct wpan_dev *wpan_dev, bool ackreq)
+{
+	ASSERT_RTNL();
+
+	wpan_dev->ackreq = ackreq;
+	return 0;
+}
+
 const struct cfg802154_ops mac802154_config_ops = {
 	.add_virtual_intf_deprecated = ieee802154_add_iface_deprecated,
 	.del_virtual_intf_deprecated = ieee802154_del_iface_deprecated,
@@ -273,4 +283,5 @@ const struct cfg802154_ops mac802154_config_ops = {
 	.set_max_csma_backoffs = ieee802154_set_max_csma_backoffs,
 	.set_max_frame_retries = ieee802154_set_max_frame_retries,
 	.set_lbt_mode = ieee802154_set_lbt_mode,
+	.set_ackreq_default = ieee802154_set_ackreq_default,
 };
-- 
2.5.0

Re: [RFCv5 bluetooth-next 2/4] mac802154: change frame_retries behaviour

[Stefan Schmidt]

Hello.

On 06/08/15 09:28, Alexander Aring wrote:
> This patch changes the default minimum value of frame_retries to 0 and
> changes the frame_retries default value to 3 which is also 802.15.4
> default.
>
> We don't use the frame_retries "-1" value as indicator for no-aret mode
> anymore, instead we checking on the ack request bit inside the 802.15.4
> frame control field. This allows a acknowledge handling per frame. This
> checking is done by transceiver or inside xmit callback of driver layer.
>
> If a transceiver doesn't support ARET handling the transmit
> functionality ignores ack frames then, which isn't well but should not
> effect anything of current functionality.
>
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>   net/mac802154/iface.c | 3 +--
>   net/mac802154/main.c  | 9 +++------
>   2 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
> index 416de90..8837c5a 100644
> --- a/net/mac802154/iface.c
> +++ b/net/mac802154/iface.c
> @@ -483,8 +483,7 @@ ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata,
>   	wpan_dev->min_be = 3;
>   	wpan_dev->max_be = 5;
>   	wpan_dev->csma_retries = 4;
> -	/* for compatibility, actual default is 3 */
> -	wpan_dev->frame_retries = -1;
> +	wpan_dev->frame_retries = 3;
>   
>   	wpan_dev->pan_id = cpu_to_le16(IEEE802154_PANID_BROADCAST);
>   	wpan_dev->short_addr = cpu_to_le16(IEEE802154_ADDR_BROADCAST);
> diff --git a/net/mac802154/main.c b/net/mac802154/main.c
> index 9e55431..e8cab5b 100644
> --- a/net/mac802154/main.c
> +++ b/net/mac802154/main.c
> @@ -111,7 +111,7 @@ ieee802154_alloc_hw(size_t priv_data_len, const struct ieee802154_ops *ops)
>   	phy->supported.max_minbe = 8;
>   	phy->supported.min_maxbe = 3;
>   	phy->supported.max_maxbe = 8;
> -	phy->supported.min_frame_retries = -1;
> +	phy->supported.min_frame_retries = 0;
>   	phy->supported.max_frame_retries = 7;
>   	phy->supported.max_csma_backoffs = 5;
>   	phy->supported.lbt = NL802154_SUPPORTED_BOOL_FALSE;
> @@ -177,11 +177,8 @@ int ieee802154_register_hw(struct ieee802154_hw *hw)
>   	}
>   
>   	if (!(hw->flags & IEEE802154_HW_FRAME_RETRIES)) {
> -		/* TODO should be 3, but our default value is -1 which means
> -		 * no ARET handling.
> -		 */
> -		local->phy->supported.min_frame_retries = -1;
> -		local->phy->supported.max_frame_retries = -1;
> +		local->phy->supported.min_frame_retries = 3;
> +		local->phy->supported.max_frame_retries = 3;
>   	}
>   
>   	if (hw->flags & IEEE802154_HW_PROMISCUOUS)

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

regards
Stefan Schmidt

Re: [RFCv5 bluetooth-next 3/4] at86rf230: use STATE_TX_ARET mode only

[Stefan Schmidt]

Hello.

On 06/08/15 09:28, Alexander Aring wrote:
> This patch changes the state change behaviour of at86rf230 to always
> TX_ARET mode. According the at86rf2xx datasheets TX_ARET mode doesn't
> mean to be always waiting for ack frames after transmit. The transceiver
> will automatically wait for ack frames or not if the acknowledge request
> bit is set. See section "TX_ARET_ON – Transmit with Automatic Frame
> Retransmission and CSMA-CA Retry".
>
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>   drivers/net/ieee802154/at86rf230.c | 52 ++++++++++----------------------------
>   1 file changed, 14 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> index d0d5bf6..6422caa 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -97,9 +97,7 @@ struct at86rf230_local {
>   
>   	struct at86rf230_state_change irq;
>   
> -	bool tx_aret;
>   	unsigned long cal_timeout;
> -	s8 max_frame_retries;
>   	bool is_tx;
>   	bool is_tx_from_off;
>   	u8 tx_retry;
> @@ -651,7 +649,7 @@ at86rf230_tx_complete(void *context)
>   
>   	enable_irq(ctx->irq);
>   
> -	ieee802154_xmit_complete(lp->hw, lp->tx_skb, !lp->tx_aret);
> +	ieee802154_xmit_complete(lp->hw, lp->tx_skb, false);
>   }
>   
>   static void
> @@ -760,17 +758,10 @@ at86rf230_irq_trx_end(struct at86rf230_local *lp)
>   {
>   	if (lp->is_tx) {
>   		lp->is_tx = 0;
> -
> -		if (lp->tx_aret)
> -			at86rf230_async_state_change(lp, &lp->irq,
> -						     STATE_FORCE_TX_ON,
> -						     at86rf230_tx_trac_status,
> -						     true);
> -		else
> -			at86rf230_async_state_change(lp, &lp->irq,
> -						     STATE_RX_AACK_ON,
> -						     at86rf230_tx_complete,
> -						     true);
> +		at86rf230_async_state_change(lp, &lp->irq,
> +					     STATE_FORCE_TX_ON,
> +					     at86rf230_tx_trac_status,
> +					     true);
>   	} else {
>   		at86rf230_async_read_reg(lp, RG_TRX_STATE, &lp->irq,
>   					 at86rf230_rx_trac_check, true);
> @@ -876,24 +867,16 @@ at86rf230_xmit_start(void *context)
>   	struct at86rf230_state_change *ctx = context;
>   	struct at86rf230_local *lp = ctx->lp;
>   
> -	/* In ARET mode we need to go into STATE_TX_ARET_ON after we
> -	 * are in STATE_TX_ON. The pfad differs here, so we change
> -	 * the complete handler.
> -	 */
> -	if (lp->tx_aret) {
> -		if (lp->is_tx_from_off) {
> -			lp->is_tx_from_off = false;
> -			at86rf230_async_state_change(lp, ctx, STATE_TX_ARET_ON,
> -						     at86rf230_write_frame,
> -						     false);
> -		} else {
> -			at86rf230_async_state_change(lp, ctx, STATE_TX_ON,
> -						     at86rf230_xmit_tx_on,
> -						     false);
> -		}
> +	/* check if we change from off state */
> +	if (lp->is_tx_from_off) {
> +		lp->is_tx_from_off = false;
> +		at86rf230_async_state_change(lp, ctx, STATE_TX_ARET_ON,
> +					     at86rf230_write_frame,
> +					     false);
>   	} else {
>   		at86rf230_async_state_change(lp, ctx, STATE_TX_ON,
> -					     at86rf230_write_frame, false);
> +					     at86rf230_xmit_tx_on,
> +					     false);
>   	}
>   }
>   
> @@ -1267,15 +1250,8 @@ static int
>   at86rf230_set_frame_retries(struct ieee802154_hw *hw, s8 retries)
>   {
>   	struct at86rf230_local *lp = hw->priv;
> -	int rc = 0;
> -
> -	lp->tx_aret = retries >= 0;
> -	lp->max_frame_retries = retries;
>   
> -	if (retries >= 0)
> -		rc = at86rf230_write_subreg(lp, SR_MAX_FRAME_RETRIES, retries);
> -
> -	return rc;
> +	return at86rf230_write_subreg(lp, SR_MAX_FRAME_RETRIES, retries);
>   }
>   
>   static int

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

regards
Stefan Schmidt

Re: [RFCv5 bluetooth-next 4/4] ieee802154: add ack request default handling

[Stefan Schmidt]

Hello.

On 06/08/15 09:28, Alexander Aring wrote:
> This patch introduce a new mib entry which isn't part of 802.15.4 but
> useful as default behaviour to set the ack request bit or not if we
> don't know if the ack request bit should set. This is currently used for
> stacks like IEEE 802.15.4 6LoWPAN.
>
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>   include/net/cfg802154.h     |  5 +++++
>   include/net/nl802154.h      |  4 ++++
>   net/ieee802154/6lowpan/tx.c |  2 +-
>   net/ieee802154/nl802154.c   | 33 +++++++++++++++++++++++++++++++++
>   net/ieee802154/rdev-ops.h   | 13 +++++++++++++
>   net/ieee802154/trace.h      | 19 +++++++++++++++++++
>   net/mac802154/cfg.c         | 11 +++++++++++
>   7 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> index 382f94b..21353f8 100644
> --- a/include/net/cfg802154.h
> +++ b/include/net/cfg802154.h
> @@ -63,6 +63,8 @@ struct cfg802154_ops {
>   					 s8 max_frame_retries);
>   	int	(*set_lbt_mode)(struct wpan_phy *wpan_phy,
>   				struct wpan_dev *wpan_dev, bool mode);
> +	int	(*set_ackreq_default)(struct wpan_phy *wpan_phy,
> +				      struct wpan_dev *wpan_dev, bool ackreq);
>   };
>   
>   static inline bool
> @@ -193,6 +195,9 @@ struct wpan_dev {
>   	bool lbt;
>   
>   	bool promiscuous_mode;
> +
> +	/* fallback for acknowledgment bit setting */
> +	bool ackreq;
>   };
>   
>   #define to_phy(_dev)	container_of(_dev, struct wpan_phy, dev)
> diff --git a/include/net/nl802154.h b/include/net/nl802154.h
> index b0ab530..cf2713d 100644
> --- a/include/net/nl802154.h
> +++ b/include/net/nl802154.h
> @@ -52,6 +52,8 @@ enum nl802154_commands {
>   
>   	NL802154_CMD_SET_LBT_MODE,
>   
> +	NL802154_CMD_SET_ACKREQ_DEFAULT,
> +
>   	/* add new commands above here */
>   
>   	/* used to define NL802154_CMD_MAX below */
> @@ -104,6 +106,8 @@ enum nl802154_attrs {
>   
>   	NL802154_ATTR_SUPPORTED_COMMANDS,
>   
> +	NL802154_ATTR_ACKREQ_DEFAULT,
> +
>   	/* add attributes here, update the policy in nl802154.c */
>   
>   	__NL802154_ATTR_AFTER_LAST,
> diff --git a/net/ieee802154/6lowpan/tx.c b/net/ieee802154/6lowpan/tx.c
> index 2597abb..1bf4a30 100644
> --- a/net/ieee802154/6lowpan/tx.c
> +++ b/net/ieee802154/6lowpan/tx.c
> @@ -224,7 +224,7 @@ static int lowpan_header(struct sk_buff *skb, struct net_device *dev)
>   	} else {
>   		da.mode = IEEE802154_ADDR_LONG;
>   		da.extended_addr = ieee802154_devaddr_from_raw(daddr);
> -		cb->ackreq = wpan_dev->frame_retries >= 0;
> +		cb->ackreq = wpan_dev->ackreq;
>   	}
>   
>   	return dev_hard_header(skb, lowpan_dev_info(dev)->real_dev,
> diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
> index 68f2401..1b00a14 100644
> --- a/net/ieee802154/nl802154.c
> +++ b/net/ieee802154/nl802154.c
> @@ -230,6 +230,8 @@ static const struct nla_policy nl802154_policy[NL802154_ATTR_MAX+1] = {
>   	[NL802154_ATTR_WPAN_PHY_CAPS] = { .type = NLA_NESTED },
>   
>   	[NL802154_ATTR_SUPPORTED_COMMANDS] = { .type = NLA_NESTED },
> +
> +	[NL802154_ATTR_ACKREQ_DEFAULT] = { .type = NLA_U8 },
>   };
>   
>   /* message building helper */
> @@ -458,6 +460,7 @@ static int nl802154_send_wpan_phy(struct cfg802154_registered_device *rdev,
>   	CMD(set_max_csma_backoffs, SET_MAX_CSMA_BACKOFFS);
>   	CMD(set_max_frame_retries, SET_MAX_FRAME_RETRIES);
>   	CMD(set_lbt_mode, SET_LBT_MODE);
> +	CMD(set_ackreq_default, SET_ACKREQ_DEFAULT);
>   
>   	if (rdev->wpan_phy.flags & WPAN_PHY_FLAG_TXPOWER)
>   		CMD(set_tx_power, SET_TX_POWER);
> @@ -656,6 +659,10 @@ nl802154_send_iface(struct sk_buff *msg, u32 portid, u32 seq, int flags,
>   	if (nla_put_u8(msg, NL802154_ATTR_LBT_MODE, wpan_dev->lbt))
>   		goto nla_put_failure;
>   
> +	/* ackreq default behaviour */
> +	if (nla_put_u8(msg, NL802154_ATTR_ACKREQ_DEFAULT, wpan_dev->ackreq))
> +		goto nla_put_failure;
> +
>   	genlmsg_end(msg, hdr);
>   	return 0;
>   
> @@ -1042,6 +1049,24 @@ static int nl802154_set_lbt_mode(struct sk_buff *skb, struct genl_info *info)
>   	return rdev_set_lbt_mode(rdev, wpan_dev, mode);
>   }
>   
> +static int
> +nl802154_set_ackreq_default(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct cfg802154_registered_device *rdev = info->user_ptr[0];
> +	struct net_device *dev = info->user_ptr[1];
> +	struct wpan_dev *wpan_dev = dev->ieee802154_ptr;
> +	bool ackreq;
> +
> +	if (netif_running(dev))
> +		return -EBUSY;

I was not able to change ackreq_default on a running wpan0 interface

root@raspberrypi:~/wpan-tools-0.4# iwpan wpan0 set ackreq_default 1
command failed: Device or resource busy (-16)

Which makes it impossible to change it per frame as you stated in the 
cover letter. I guess this should go.


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

regards
Stefan Schmidt

Re: [RFCv5 bluetooth-next 4/4] ieee802154: add ack request default handling

[Alexander Aring]

On Thu, Aug 06, 2015 at 07:37:18PM +0200, Stefan Schmidt wrote:
> Hello.
> 
> On 06/08/15 09:28, Alexander Aring wrote:
> >This patch introduce a new mib entry which isn't part of 802.15.4 but
> >useful as default behaviour to set the ack request bit or not if we
> >don't know if the ack request bit should set. This is currently used for
> >stacks like IEEE 802.15.4 6LoWPAN.
> >
> >Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> >---
> >  include/net/cfg802154.h     |  5 +++++
> >  include/net/nl802154.h      |  4 ++++
> >  net/ieee802154/6lowpan/tx.c |  2 +-
> >  net/ieee802154/nl802154.c   | 33 +++++++++++++++++++++++++++++++++
> >  net/ieee802154/rdev-ops.h   | 13 +++++++++++++
> >  net/ieee802154/trace.h      | 19 +++++++++++++++++++
> >  net/mac802154/cfg.c         | 11 +++++++++++
> >  7 files changed, 86 insertions(+), 1 deletion(-)
> >
> >diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> >index 382f94b..21353f8 100644
> >--- a/include/net/cfg802154.h
> >+++ b/include/net/cfg802154.h
> >@@ -63,6 +63,8 @@ struct cfg802154_ops {
> >  					 s8 max_frame_retries);
> >  	int	(*set_lbt_mode)(struct wpan_phy *wpan_phy,
> >  				struct wpan_dev *wpan_dev, bool mode);
> >+	int	(*set_ackreq_default)(struct wpan_phy *wpan_phy,
> >+				      struct wpan_dev *wpan_dev, bool ackreq);
> >  };
> >  static inline bool
> >@@ -193,6 +195,9 @@ struct wpan_dev {
> >  	bool lbt;
> >  	bool promiscuous_mode;
> >+
> >+	/* fallback for acknowledgment bit setting */
> >+	bool ackreq;
> >  };
> >  #define to_phy(_dev)	container_of(_dev, struct wpan_phy, dev)
> >diff --git a/include/net/nl802154.h b/include/net/nl802154.h
> >index b0ab530..cf2713d 100644
> >--- a/include/net/nl802154.h
> >+++ b/include/net/nl802154.h
> >@@ -52,6 +52,8 @@ enum nl802154_commands {
> >  	NL802154_CMD_SET_LBT_MODE,
> >+	NL802154_CMD_SET_ACKREQ_DEFAULT,
> >+
> >  	/* add new commands above here */
> >  	/* used to define NL802154_CMD_MAX below */
> >@@ -104,6 +106,8 @@ enum nl802154_attrs {
> >  	NL802154_ATTR_SUPPORTED_COMMANDS,
> >+	NL802154_ATTR_ACKREQ_DEFAULT,
> >+
> >  	/* add attributes here, update the policy in nl802154.c */
> >  	__NL802154_ATTR_AFTER_LAST,
> >diff --git a/net/ieee802154/6lowpan/tx.c b/net/ieee802154/6lowpan/tx.c
> >index 2597abb..1bf4a30 100644
> >--- a/net/ieee802154/6lowpan/tx.c
> >+++ b/net/ieee802154/6lowpan/tx.c
> >@@ -224,7 +224,7 @@ static int lowpan_header(struct sk_buff *skb, struct net_device *dev)
> >  	} else {
> >  		da.mode = IEEE802154_ADDR_LONG;
> >  		da.extended_addr = ieee802154_devaddr_from_raw(daddr);
> >-		cb->ackreq = wpan_dev->frame_retries >= 0;
> >+		cb->ackreq = wpan_dev->ackreq;
> >  	}
> >  	return dev_hard_header(skb, lowpan_dev_info(dev)->real_dev,
> >diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
> >index 68f2401..1b00a14 100644
> >--- a/net/ieee802154/nl802154.c
> >+++ b/net/ieee802154/nl802154.c
> >@@ -230,6 +230,8 @@ static const struct nla_policy nl802154_policy[NL802154_ATTR_MAX+1] = {
> >  	[NL802154_ATTR_WPAN_PHY_CAPS] = { .type = NLA_NESTED },
> >  	[NL802154_ATTR_SUPPORTED_COMMANDS] = { .type = NLA_NESTED },
> >+
> >+	[NL802154_ATTR_ACKREQ_DEFAULT] = { .type = NLA_U8 },
> >  };
> >  /* message building helper */
> >@@ -458,6 +460,7 @@ static int nl802154_send_wpan_phy(struct cfg802154_registered_device *rdev,
> >  	CMD(set_max_csma_backoffs, SET_MAX_CSMA_BACKOFFS);
> >  	CMD(set_max_frame_retries, SET_MAX_FRAME_RETRIES);
> >  	CMD(set_lbt_mode, SET_LBT_MODE);
> >+	CMD(set_ackreq_default, SET_ACKREQ_DEFAULT);
> >  	if (rdev->wpan_phy.flags & WPAN_PHY_FLAG_TXPOWER)
> >  		CMD(set_tx_power, SET_TX_POWER);
> >@@ -656,6 +659,10 @@ nl802154_send_iface(struct sk_buff *msg, u32 portid, u32 seq, int flags,
> >  	if (nla_put_u8(msg, NL802154_ATTR_LBT_MODE, wpan_dev->lbt))
> >  		goto nla_put_failure;
> >+	/* ackreq default behaviour */
> >+	if (nla_put_u8(msg, NL802154_ATTR_ACKREQ_DEFAULT, wpan_dev->ackreq))
> >+		goto nla_put_failure;
> >+
> >  	genlmsg_end(msg, hdr);
> >  	return 0;
> >@@ -1042,6 +1049,24 @@ static int nl802154_set_lbt_mode(struct sk_buff *skb, struct genl_info *info)
> >  	return rdev_set_lbt_mode(rdev, wpan_dev, mode);
> >  }
> >+static int
> >+nl802154_set_ackreq_default(struct sk_buff *skb, struct genl_info *info)
> >+{
> >+	struct cfg802154_registered_device *rdev = info->user_ptr[0];
> >+	struct net_device *dev = info->user_ptr[1];
> >+	struct wpan_dev *wpan_dev = dev->ieee802154_ptr;
> >+	bool ackreq;
> >+
> >+	if (netif_running(dev))
> >+		return -EBUSY;
> 
> I was not able to change ackreq_default on a running wpan0 interface
> 

yes.

> root@raspberrypi:~/wpan-tools-0.4# iwpan wpan0 set ackreq_default 1
> command failed: Device or resource busy (-16)
> 
> Which makes it impossible to change it per frame as you stated in the cover
> letter. I guess this should go.
> 

yea, ehm I think everybody (including myself) is confused now.

The behaviour before was only indicated by max_frame_retries "-1" or
max_frame_retries above 0. Which means:

 - NO ARET (transmit doesn't wait for ack) == (max_frame_retries = -1)
 - ARET (transmit wait for ack, the ARET mechanism was used by using the
         max_frame_retries >= 0 parameter, which is can only be done by
	 transceiver)

The max_frame_retries parameter is only changed when the interface was
down, this was meant by "it was not per frame handling", which ends in
some awkward behaviour because you can set the ack request bit in cases
where the user can define it (e.g. af_802154). For 6LoWPAN ackreq was
set now by (max_frame_retries >= 0) condition.

What we did now is to remove the (max_frame_retries = -1) value and let
the max_frame_retries configureable even it's ARET or NO ARET mode.
(Note this value is only a parameter for the ARET mode algorithm, so it
 isn't used on NO ARET mode).

All transceiver drivers should now indicate by ack request bit if it
should going into ARET mode or NO ARET mode and this handling is per frame.
It hasn't nothing to do with the ackreq_default entry (Maybe I should be
more clear in the cover-letter).


What I always thought before is that the at86rf2xx transceivers and
going into TX_ARET_ON will indicate (ARET mode only), so it always waits
for an ack frame after transmit. -> Since RFCv5 I know this isn't true
you can check the this behaviour with my debugfs patches. The
transceiver will check on ackreq bit on his own and then wait for ack or
not.

This means the at86rf2xx transceivers can be run in TX_ARET_ON also in
TX_ON by setting ackreq bit always to false (doesn't wait for ack) and
min_be = 0 (disable CSMA handling). What we can't turn off is the CCA
handling (maybe you can disable it by choosing the lowest threshold for
energy above threshold "or"), but then it's the same mechanism like TX_ON.

Don't know now why we always turn into TX_ON mode by when NO ARET mode
when the transceiver decide on his own if ack request bit is set or not.
Maybe somebody also didn't saw that on the first sight at the datasheet
and never looked back how TX_ARET_ON is working. (The name is also
really confused).


Now to your question:

We can change it that we allow to this value during ifup.
The question is here if we want a "clean" change to this value.

While changing this parameter, there could be frames in netdev queues
which are still pending. This will only affects then new frames which
comming into this queue.

The simplest way would again to do this handling when ifdown. I know I
do this if (!netif_running(dev)) too many and maybe it also doesn't
matter a "clean" locking to changing this parameter because we doesn't
care then if this bit is set for 4-7 frames when we set it. If you want
I can change it.

- Alex

Re: [RFCv5 bluetooth-next 4/4] ieee802154: add ack request default handling

[Stefan Schmidt]

Hello.

On 07/08/15 08:43, Alexander Aring wrote:
> On Thu, Aug 06, 2015 at 07:37:18PM +0200, Stefan Schmidt wrote:
>> Hello.
>>
>> On 06/08/15 09:28, Alexander Aring wrote:
>>> This patch introduce a new mib entry which isn't part of 802.15.4 but
>>> useful as default behaviour to set the ack request bit or not if we
>>> don't know if the ack request bit should set. This is currently used for
>>> stacks like IEEE 802.15.4 6LoWPAN.
>>>
>>> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
>>> ---
>>>   include/net/cfg802154.h     |  5 +++++
>>>   include/net/nl802154.h      |  4 ++++
>>>   net/ieee802154/6lowpan/tx.c |  2 +-
>>>   net/ieee802154/nl802154.c   | 33 +++++++++++++++++++++++++++++++++
>>>   net/ieee802154/rdev-ops.h   | 13 +++++++++++++
>>>   net/ieee802154/trace.h      | 19 +++++++++++++++++++
>>>   net/mac802154/cfg.c         | 11 +++++++++++
>>>   7 files changed, 86 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
>>> index 382f94b..21353f8 100644
>>> --- a/include/net/cfg802154.h
>>> +++ b/include/net/cfg802154.h
>>> @@ -63,6 +63,8 @@ struct cfg802154_ops {
>>>   					 s8 max_frame_retries);
>>>   	int	(*set_lbt_mode)(struct wpan_phy *wpan_phy,
>>>   				struct wpan_dev *wpan_dev, bool mode);
>>> +	int	(*set_ackreq_default)(struct wpan_phy *wpan_phy,
>>> +				      struct wpan_dev *wpan_dev, bool ackreq);
>>>   };
>>>   static inline bool
>>> @@ -193,6 +195,9 @@ struct wpan_dev {
>>>   	bool lbt;
>>>   	bool promiscuous_mode;
>>> +
>>> +	/* fallback for acknowledgment bit setting */
>>> +	bool ackreq;
>>>   };
>>>   #define to_phy(_dev)	container_of(_dev, struct wpan_phy, dev)
>>> diff --git a/include/net/nl802154.h b/include/net/nl802154.h
>>> index b0ab530..cf2713d 100644
>>> --- a/include/net/nl802154.h
>>> +++ b/include/net/nl802154.h
>>> @@ -52,6 +52,8 @@ enum nl802154_commands {
>>>   	NL802154_CMD_SET_LBT_MODE,
>>> +	NL802154_CMD_SET_ACKREQ_DEFAULT,
>>> +
>>>   	/* add new commands above here */
>>>   	/* used to define NL802154_CMD_MAX below */
>>> @@ -104,6 +106,8 @@ enum nl802154_attrs {
>>>   	NL802154_ATTR_SUPPORTED_COMMANDS,
>>> +	NL802154_ATTR_ACKREQ_DEFAULT,
>>> +
>>>   	/* add attributes here, update the policy in nl802154.c */
>>>   	__NL802154_ATTR_AFTER_LAST,
>>> diff --git a/net/ieee802154/6lowpan/tx.c b/net/ieee802154/6lowpan/tx.c
>>> index 2597abb..1bf4a30 100644
>>> --- a/net/ieee802154/6lowpan/tx.c
>>> +++ b/net/ieee802154/6lowpan/tx.c
>>> @@ -224,7 +224,7 @@ static int lowpan_header(struct sk_buff *skb, struct net_device *dev)
>>>   	} else {
>>>   		da.mode = IEEE802154_ADDR_LONG;
>>>   		da.extended_addr = ieee802154_devaddr_from_raw(daddr);
>>> -		cb->ackreq = wpan_dev->frame_retries >= 0;
>>> +		cb->ackreq = wpan_dev->ackreq;
>>>   	}
>>>   	return dev_hard_header(skb, lowpan_dev_info(dev)->real_dev,
>>> diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
>>> index 68f2401..1b00a14 100644
>>> --- a/net/ieee802154/nl802154.c
>>> +++ b/net/ieee802154/nl802154.c
>>> @@ -230,6 +230,8 @@ static const struct nla_policy nl802154_policy[NL802154_ATTR_MAX+1] = {
>>>   	[NL802154_ATTR_WPAN_PHY_CAPS] = { .type = NLA_NESTED },
>>>   	[NL802154_ATTR_SUPPORTED_COMMANDS] = { .type = NLA_NESTED },
>>> +
>>> +	[NL802154_ATTR_ACKREQ_DEFAULT] = { .type = NLA_U8 },
>>>   };
>>>   /* message building helper */
>>> @@ -458,6 +460,7 @@ static int nl802154_send_wpan_phy(struct cfg802154_registered_device *rdev,
>>>   	CMD(set_max_csma_backoffs, SET_MAX_CSMA_BACKOFFS);
>>>   	CMD(set_max_frame_retries, SET_MAX_FRAME_RETRIES);
>>>   	CMD(set_lbt_mode, SET_LBT_MODE);
>>> +	CMD(set_ackreq_default, SET_ACKREQ_DEFAULT);
>>>   	if (rdev->wpan_phy.flags & WPAN_PHY_FLAG_TXPOWER)
>>>   		CMD(set_tx_power, SET_TX_POWER);
>>> @@ -656,6 +659,10 @@ nl802154_send_iface(struct sk_buff *msg, u32 portid, u32 seq, int flags,
>>>   	if (nla_put_u8(msg, NL802154_ATTR_LBT_MODE, wpan_dev->lbt))
>>>   		goto nla_put_failure;
>>> +	/* ackreq default behaviour */
>>> +	if (nla_put_u8(msg, NL802154_ATTR_ACKREQ_DEFAULT, wpan_dev->ackreq))
>>> +		goto nla_put_failure;
>>> +
>>>   	genlmsg_end(msg, hdr);
>>>   	return 0;
>>> @@ -1042,6 +1049,24 @@ static int nl802154_set_lbt_mode(struct sk_buff *skb, struct genl_info *info)
>>>   	return rdev_set_lbt_mode(rdev, wpan_dev, mode);
>>>   }
>>> +static int
>>> +nl802154_set_ackreq_default(struct sk_buff *skb, struct genl_info *info)
>>> +{
>>> +	struct cfg802154_registered_device *rdev = info->user_ptr[0];
>>> +	struct net_device *dev = info->user_ptr[1];
>>> +	struct wpan_dev *wpan_dev = dev->ieee802154_ptr;
>>> +	bool ackreq;
>>> +
>>> +	if (netif_running(dev))
>>> +		return -EBUSY;
>> I was not able to change ackreq_default on a running wpan0 interface
>>
> yes.
>
>> root@raspberrypi:~/wpan-tools-0.4# iwpan wpan0 set ackreq_default 1
>> command failed: Device or resource busy (-16)
>>
>> Which makes it impossible to change it per frame as you stated in the cover
>> letter. I guess this should go.
>>
> yea, ehm I think everybody (including myself) is confused now.
>
> The behaviour before was only indicated by max_frame_retries "-1" or
> max_frame_retries above 0. Which means:
>
>   - NO ARET (transmit doesn't wait for ack) == (max_frame_retries = -1)
>   - ARET (transmit wait for ack, the ARET mechanism was used by using the
>           max_frame_retries >= 0 parameter, which is can only be done by
> 	 transceiver)
>
> The max_frame_retries parameter is only changed when the interface was
> down, this was meant by "it was not per frame handling", which ends in
> some awkward behaviour because you can set the ack request bit in cases
> where the user can define it (e.g. af_802154). For 6LoWPAN ackreq was
> set now by (max_frame_retries >= 0) condition.
>
> What we did now is to remove the (max_frame_retries = -1) value and let
> the max_frame_retries configureable even it's ARET or NO ARET mode.
> (Note this value is only a parameter for the ARET mode algorithm, so it
>   isn't used on NO ARET mode).
>
> All transceiver drivers should now indicate by ack request bit if it
> should going into ARET mode or NO ARET mode and this handling is per frame.
> It hasn't nothing to do with the ackreq_default entry (Maybe I should be
> more clear in the cover-letter).
>
>
> What I always thought before is that the at86rf2xx transceivers and
> going into TX_ARET_ON will indicate (ARET mode only), so it always waits
> for an ack frame after transmit. -> Since RFCv5 I know this isn't true
> you can check the this behaviour with my debugfs patches. The
> transceiver will check on ackreq bit on his own and then wait for ack or
> not.
>
> This means the at86rf2xx transceivers can be run in TX_ARET_ON also in
> TX_ON by setting ackreq bit always to false (doesn't wait for ack) and
> min_be = 0 (disable CSMA handling). What we can't turn off is the CCA
> handling (maybe you can disable it by choosing the lowest threshold for
> energy above threshold "or"), but then it's the same mechanism like TX_ON.
>
> Don't know now why we always turn into TX_ON mode by when NO ARET mode
> when the transceiver decide on his own if ack request bit is set or not.
> Maybe somebody also didn't saw that on the first sight at the datasheet
> and never looked back how TX_ARET_ON is working. (The name is also
> really confused).
>
>
> Now to your question:
>
> We can change it that we allow to this value during ifup.
> The question is here if we want a "clean" change to this value.
>
> While changing this parameter, there could be frames in netdev queues
> which are still pending. This will only affects then new frames which
> comming into this queue.
>
> The simplest way would again to do this handling when ifdown. I know I
> do this if (!netif_running(dev)) too many and maybe it also doesn't
> matter a "clean" locking to changing this parameter because we doesn't
> care then if this bit is set for 4-7 frames when we set it. If you want
> I can change it.

No, keep it as you have it right now. Changing this only while the 
device is down is ok for now. We might want to re-visit all the places 
where we have been careful at some later point but for no this is fine.

You should have a reviewied-by tag for all patches now from my side 
(kernel and wpan-tools). I'm fine with this series getting merged.

regards
Stefan Schmidt