[RFCv3 bluetooth-next 0/6] ieee802154: aret handling changes

[RFCv3 bluetooth-next 0/6] 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 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 (6):
  mac802154: cfg: remove test and set checks
  ieee802154: add helpers for frame control checks
  at86rf230: use aret mode if ackreq is set while xmit
  mac802154: change max_frame_retries behaviour
  at86rf230: remove max_frame_retries -1 check
  ieee802154: add ack request default handling

 drivers/net/ieee802154/at86rf230.c | 14 +++++---------
 include/linux/ieee802154.h         | 29 +++++++++++++++++++++++++++++
 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 +++------
 11 files changed, 124 insertions(+), 34 deletions(-)

-- 
2.5.0

[RFCv3 bluetooth-next 1/6] 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.

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

[RFCv3 bluetooth-next 2/6] ieee802154: add helpers for frame control checks

[Alexander Aring]

This patch introduce two static inline functions. The first to get the
frame control field from an sk_buff. The second is for checking on the
acknowledgment request bit on the frame control field. Later we can
introduce more functions to check on the frame control fields.

These will deprecate the current behaviour which requires a
host-byteorder conversion and manually bit handling.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 include/linux/ieee802154.h | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/include/linux/ieee802154.h b/include/linux/ieee802154.h
index 1dc1f4e..4f26c01 100644
--- a/include/linux/ieee802154.h
+++ b/include/linux/ieee802154.h
@@ -25,6 +25,8 @@
 
 #include <linux/types.h>
 #include <linux/random.h>
+#include <linux/skbuff.h>
+#include <linux/unaligned/memmove.h>
 #include <asm/byteorder.h>
 
 #define IEEE802154_MTU			127
@@ -205,6 +207,33 @@ enum {
 	IEEE802154_SCAN_IN_PROGRESS = 0xfc,
 };
 
+/* frame control handling */
+#define IEEE802154_FCTL_ACKREQ	0x0020
+
+/**
+ * ieee802154_is_ackreq - check if acknowledgment request bit is set
+ * @fc: frame control bytes in little-endian byteorder
+ */
+static inline bool ieee802154_is_ackreq(__le16 fc)
+{
+	return fc & cpu_to_le16(IEEE802154_FCTL_ACKREQ);
+}
+
+/**
+ * ieee802154_get_fc_from_skb - get the frame control field from an skb
+ * @skb: skb where the frame control field will be get from
+ */
+static inline __le16 ieee802154_get_fc_from_skb(const struct sk_buff *skb)
+{
+	/* return some invalid fc on failure */
+	if (unlikely(skb->mac_len < 2)) {
+		WARN_ON(1);
+		return cpu_to_le16(0);
+	}
+
+	return (__force __le16)__get_unaligned_memmove16(skb_mac_header(skb));
+}
+
 /**
  * ieee802154_is_valid_psdu_len - check if psdu len is valid
  * available lengths:
-- 
2.5.0

[RFCv3 bluetooth-next 3/6] at86rf230: use aret mode if ackreq is set while xmit

[Alexander Aring]

This patch does a different aret mode handling. We will drop the
max_frame_retries "-1" value and check on the ack request bit while
xmit. If it's set we doing transmit in aret mode. If it's not set we
doing a transmit in normal tx mode.

This allows a mixed aret and non aret mode while interface is up and
doesn't depends on the max_frame_retries value inside the mib.

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

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index f7bd9f3..8b9557f 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -99,7 +99,6 @@ struct at86rf230_local {
 
 	bool tx_aret;
 	unsigned long cal_timeout;
-	s8 max_frame_retries;
 	bool is_tx;
 	bool is_tx_from_off;
 	u8 tx_retry;
@@ -900,10 +899,14 @@ at86rf230_xmit(struct ieee802154_hw *hw, struct sk_buff *skb)
 {
 	struct at86rf230_local *lp = hw->priv;
 	struct at86rf230_state_change *ctx = &lp->tx;
+	__le16 fc;
 
 	lp->tx_skb = skb;
 	lp->tx_retry = 0;
 
+	fc = ieee802154_get_fc_from_skb(skb);
+	lp->tx_aret = ieee802154_is_ackreq(fc);
+
 	/* After 5 minutes in PLL and the same frequency we run again the
 	 * calibration loops which is recommended by at86rf2xx datasheets.
 	 *
@@ -1267,9 +1270,6 @@ 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);
 
-- 
2.5.0

[RFCv3 bluetooth-next 4/6] mac802154: change max_frame_retries behaviour

[Alexander Aring]

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

We don't use max_frame_retries "-1" as indicator for no-aret mode
anymore, instead we checking on the ack request bit inside the 802.15.4
frame control field.

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 91f1208..6e1764b 100644
--- a/net/mac802154/main.c
+++ b/net/mac802154/main.c
@@ -109,7 +109,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;
@@ -175,11 +175,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

[RFCv3 bluetooth-next 5/6] at86rf230: remove max_frame_retries -1 check

[Alexander Aring]

Since we remove the max_frame_retries "-1" option from mac802154 stack
we don't need to check on such option as parameter.

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

diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
index 8b9557f..0bb52b2 100644
--- a/drivers/net/ieee802154/at86rf230.c
+++ b/drivers/net/ieee802154/at86rf230.c
@@ -1268,12 +1268,8 @@ static int
 at86rf230_set_frame_retries(struct ieee802154_hw *hw, s8 retries)
 {
 	struct at86rf230_local *lp = hw->priv;
-	int rc = 0;
 
-	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

[RFCv3 bluetooth-next 6/6] 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: [RFCv3 bluetooth-next 0/6] ieee802154: aret handling changes

[Stefan Schmidt]

Hello.

On 30/07/15 10:55, Alexander Aring wrote:
> 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.

I think it is sound to handle it per frame to give us the opportunity to 
change this setting without taking the interface down. I wonder about 
the frame retry counter which needs to be written in some transceiver 
register to be used by the hardware directly. That one can only be done 
when the interface is down. Whcih means we can decide if we want ARET 
handling per frame but not how often the retry happens. Do I miss something?

> 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.

You have the wpan-tools bits for the new netlink command pushed 
somewhere to test out with this patchset?

I'm going to review the implementation now and run some tests with this 
code enabled.

regards
Stefan Schmidt

Re: [RFCv3 bluetooth-next 1/6] mac802154: cfg: remove test and set checks

[Stefan Schmidt]

Hello.

On 30/07/15 10:55, Alexander Aring wrote:
> 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.
>
> Suggested-by: Phoebe Buckheister<phoebe.buckheister@itwm.fraunhofer.de>
> Signed-off-by: Alexander Aring<alex.aring@gmail.com>
> ---

I always found these checks comforting so see. But given that we don't 
really need them and are only double checking here when we already did I 
think its fine to remove them.

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

regards
Stefan Schmidt

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

Re: [RFCv3 bluetooth-next 2/6] ieee802154: add helpers for frame control checks

[Stefan Schmidt]

Hello.

On 30/07/15 10:55, Alexander Aring wrote:
> This patch introduce two static inline functions. The first to get the
> frame control field from an sk_buff. The second is for checking on the
> acknowledgment request bit on the frame control field. Later we can
> introduce more functions to check on the frame control fields.
>
> These will deprecate the current behaviour which requires a
> host-byteorder conversion and manually bit handling.
>
> Signed-off-by: Alexander Aring<alex.aring@gmail.com>
> ---

Some language suggestions inside.
>   include/linux/ieee802154.h | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
>
> diff --git a/include/linux/ieee802154.h b/include/linux/ieee802154.h
> index 1dc1f4e..4f26c01 100644
> --- a/include/linux/ieee802154.h
> +++ b/include/linux/ieee802154.h
> @@ -25,6 +25,8 @@
>   
>   #include <linux/types.h>
>   #include <linux/random.h>
> +#include <linux/skbuff.h>
> +#include <linux/unaligned/memmove.h>
>   #include <asm/byteorder.h>
>   
>   #define IEEE802154_MTU			127
> @@ -205,6 +207,33 @@ enum {
>   	IEEE802154_SCAN_IN_PROGRESS = 0xfc,
>   };
>   
> +/* frame control handling */
> +#define IEEE802154_FCTL_ACKREQ	0x0020
> +
> +/**
> + * ieee802154_is_ackreq - check if acknowledgment request bit is set
> + * @fc: frame control bytes in little-endian byteorder
> + */
> +static inline bool ieee802154_is_ackreq(__le16 fc)
> +{
> +	return fc & cpu_to_le16(IEEE802154_FCTL_ACKREQ);
> +}
> +
> +/**
> + * ieee802154_get_fc_from_skb - get the frame control field from an skb

... from a skb
> + * @skb: skb where the frame control field will be get from
Maybe:

skb which contains the frame control field

> + */
> +static inline __le16 ieee802154_get_fc_from_skb(const struct sk_buff *skb)
> +{
> +	/* return some invalid fc on failure */
Maybe:

return on invalid fc

> +	if (unlikely(skb->mac_len < 2)) {
> +		WARN_ON(1);
> +		return cpu_to_le16(0);
> +	}
> +
> +	return (__force __le16)__get_unaligned_memmove16(skb_mac_header(skb));

Just to make sure we don't run into problems like we did with the 
6lowpan stack. __get_unaligned_memmove16 is not pulling the fc bytes out 
of the skb, right? The skb stays as it is.

> +}
> +
>   /**
>    * ieee802154_is_valid_psdu_len - check if psdu len is valid
>    * available lengths:

Given the above is true you have my:

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

regards
Stefan Schmidt

Re: [RFCv3 bluetooth-next 3/6] at86rf230: use aret mode if ackreq is set while xmit

[Stefan Schmidt]

Hello.

On 30/07/15 10:55, Alexander Aring wrote:
> This patch does a different aret mode handling. We will drop the
> max_frame_retries "-1" value and check on the ack request bit while
> xmit. If it's set we doing transmit in aret mode. If it's not set we
> doing a transmit in normal tx mode.
>
> This allows a mixed aret and non aret mode while interface is up and
> doesn't depends on the max_frame_retries value inside the mib.
>
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>   drivers/net/ieee802154/at86rf230.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> index f7bd9f3..8b9557f 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -99,7 +99,6 @@ struct at86rf230_local {
>   
>   	bool tx_aret;
>   	unsigned long cal_timeout;
> -	s8 max_frame_retries;
>   	bool is_tx;
>   	bool is_tx_from_off;
>   	u8 tx_retry;
> @@ -900,10 +899,14 @@ at86rf230_xmit(struct ieee802154_hw *hw, struct sk_buff *skb)
>   {
>   	struct at86rf230_local *lp = hw->priv;
>   	struct at86rf230_state_change *ctx = &lp->tx;
> +	__le16 fc;
>   
>   	lp->tx_skb = skb;
>   	lp->tx_retry = 0;
>   
> +	fc = ieee802154_get_fc_from_skb(skb);
> +	lp->tx_aret = ieee802154_is_ackreq(fc);
> +
>   	/* After 5 minutes in PLL and the same frequency we run again the
>   	 * calibration loops which is recommended by at86rf2xx datasheets.
>   	 *
> @@ -1267,9 +1270,6 @@ 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);
>   

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

regards
Stefan Schmidt

Re: [RFCv3 bluetooth-next 4/6] mac802154: change max_frame_retries behaviour

[Stefan Schmidt]

Hello.

On 30/07/15 10:55, Alexander Aring wrote:
> This patch changes the default min value of max_frame_retries to 0 and
> changes the max_frame_retries default value to 3 which is also 802.15.4
> default.
You mean min_frame_retries to 0 here?


> We don't use max_frame_retries "-1" as indicator for no-aret mode
> anymore, instead we checking on the ack request bit inside the 802.15.4
> frame control field.
>
> 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 91f1208..6e1764b 100644
> --- a/net/mac802154/main.c
> +++ b/net/mac802154/main.c
> @@ -109,7 +109,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;

Maybe also set it to 3 here if we want to have this as default and 
letting it to userspace to set it to another value if needed?

>   	phy->supported.max_csma_backoffs = 5;
>   	phy->supported.lbt = NL802154_SUPPORTED_BOOL_FALSE;
> @@ -175,11 +175,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;

This confuses me. During alloc_hw we set min to 0 which makes sense for 
me but afterwards we set both, min and max, to 3? Should it not only be 
max to three here? Am I missing something?

regards
Stefan Schmidt

Re: [RFCv3 bluetooth-next 5/6] at86rf230: remove max_frame_retries -1 check

[Stefan Schmidt]

Hello.

On 30/07/15 10:55, Alexander Aring wrote:
> Since we remove the max_frame_retries "-1" option from mac802154 stack
> we don't need to check on such option as parameter.
>
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>   drivers/net/ieee802154/at86rf230.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/net/ieee802154/at86rf230.c b/drivers/net/ieee802154/at86rf230.c
> index 8b9557f..0bb52b2 100644
> --- a/drivers/net/ieee802154/at86rf230.c
> +++ b/drivers/net/ieee802154/at86rf230.c
> @@ -1268,12 +1268,8 @@ static int
>   at86rf230_set_frame_retries(struct ieee802154_hw *hw, s8 retries)
>   {
>   	struct at86rf230_local *lp = hw->priv;
> -	int rc = 0;
>   
> -	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: [RFCv3 bluetooth-next 6/6] ieee802154: add ack request default handling

[Stefan Schmidt]

Hello.

On 30/07/15 10:55, 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;
> +
> +	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))
> +);
> +

Hmm, having all this boilerplate code for trace events its a bit 
cumbersome imho. But it seems that is how it is done. :/

>   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,
>   };

Looking over the code I found now problem. For really testing it I would 
need the userspace counterpart in wpan-tools. You have that published 
somewhere?

regards
Stefan Schmidt

Re: [RFCv3 bluetooth-next 2/6] ieee802154: add helpers for frame control checks

[Alexander Aring]

On Tue, Aug 04, 2015 at 06:29:13PM +0200, Stefan Schmidt wrote:
> Hello.
> 
> On 30/07/15 10:55, Alexander Aring wrote:
> >This patch introduce two static inline functions. The first to get the
> >frame control field from an sk_buff. The second is for checking on the
> >acknowledgment request bit on the frame control field. Later we can
> >introduce more functions to check on the frame control fields.
> >
> >These will deprecate the current behaviour which requires a
> >host-byteorder conversion and manually bit handling.
> >
> >Signed-off-by: Alexander Aring<alex.aring@gmail.com>
> >---
> 
> Some language suggestions inside.

ok.
> >  include/linux/ieee802154.h | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >
> >diff --git a/include/linux/ieee802154.h b/include/linux/ieee802154.h
> >index 1dc1f4e..4f26c01 100644
> >--- a/include/linux/ieee802154.h
> >+++ b/include/linux/ieee802154.h
> >@@ -25,6 +25,8 @@
> >  #include <linux/types.h>
> >  #include <linux/random.h>
> >+#include <linux/skbuff.h>
> >+#include <linux/unaligned/memmove.h>
> >  #include <asm/byteorder.h>
> >  #define IEEE802154_MTU			127
> >@@ -205,6 +207,33 @@ enum {
> >  	IEEE802154_SCAN_IN_PROGRESS = 0xfc,
> >  };
> >+/* frame control handling */
> >+#define IEEE802154_FCTL_ACKREQ	0x0020
> >+
> >+/**
> >+ * ieee802154_is_ackreq - check if acknowledgment request bit is set
> >+ * @fc: frame control bytes in little-endian byteorder
> >+ */
> >+static inline bool ieee802154_is_ackreq(__le16 fc)
> >+{
> >+	return fc & cpu_to_le16(IEEE802154_FCTL_ACKREQ);
> >+}
> >+
> >+/**
> >+ * ieee802154_get_fc_from_skb - get the frame control field from an skb
> 
> ... from a skb

ok.
> >+ * @skb: skb where the frame control field will be get from
> Maybe:
> 
> skb which contains the frame control field
> 

ok.
> >+ */
> >+static inline __le16 ieee802154_get_fc_from_skb(const struct sk_buff *skb)
> >+{
> >+	/* return some invalid fc on failure */
> Maybe:
> 
> return on invalid fc
> 

ok.
> >+	if (unlikely(skb->mac_len < 2)) {
> >+		WARN_ON(1);
> >+		return cpu_to_le16(0);
> >+	}
> >+
> >+	return (__force __le16)__get_unaligned_memmove16(skb_mac_header(skb));
> 
> Just to make sure we don't run into problems like we did with the 6lowpan
> stack. __get_unaligned_memmove16 is not pulling the fc bytes out of the skb,
> right? The skb stays as it is.
> 

right it doesn't manipulate the skb. For the "problems like we did with
the 6lowpan" you need to decide which problems, I see several:

 - running skb_pull (which removes) buffer and we don't have the room to
   pull out the bytes of skb, example: skb->len = 3, skb_pull size is 4
   which ends in a BUG(), we need to check it with skb_may_pull before.

 - running skb_push and we don't have the headroom for that. Like
   headroom space is 3 but we running push for size 4. the function
   skb_cow will reallocte headroom if needed.

These two problems are mostly out of 6lowpan code (I think).

The problem which I mentioned at iphc is more different. This is read
out the data but we doesn't check if we getting a buffer out of read
access. Example:

tmp a[3], b[4];

memcpy(b, a, ARRAY_SIZE(b));

And the source pointer will read something from the stack. But this is
handled here by checking "unlikely(skb->mac_len < 2)".

> >+}
> >+
> >  /**
> >   * ieee802154_is_valid_psdu_len - check if psdu len is valid
> >   * available lengths:
> 
> Given the above is true you have my:
> 

ok.

- Alex

Re: [RFCv3 bluetooth-next 4/6] mac802154: change max_frame_retries behaviour

[Alexander Aring]

On Tue, Aug 04, 2015 at 06:40:46PM +0200, Stefan Schmidt wrote:
> Hello.
> 
> On 30/07/15 10:55, Alexander Aring wrote:
> >This patch changes the default min value of max_frame_retries to 0 and
> >changes the max_frame_retries default value to 3 which is also 802.15.4
> >default.
> You mean min_frame_retries to 0 here?

No, you mixed "frame_retries" ranges with "frame_retries" value.

> 
> 
> >We don't use max_frame_retries "-1" as indicator for no-aret mode
> >anymore, instead we checking on the ack request bit inside the 802.15.4
> >frame control field.
> >
> >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;

Here is meant the frame_retries value, 802.15.4-2011 describe this value
as 3 for default.

> >  	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 91f1208..6e1764b 100644
> >--- a/net/mac802154/main.c
> >+++ b/net/mac802154/main.c
> >@@ -109,7 +109,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;
> 
> Maybe also set it to 3 here if we want to have this as default and letting
> it to userspace to set it to another value if needed?
> 

Here is the frame_retries range by 802.15.4 which is "0..7". So now
there comes more internal mechanism, this range is set ONLY when you the
driver supports IEEE802154_HW_FRAME_RETRIES. If a transceiver supports
IEEE802154_HW_FRAME_RETRIES but not the default of range "0..7" then the
driver can overwrite this range.

> >  	phy->supported.max_csma_backoffs = 5;
> >  	phy->supported.lbt = NL802154_SUPPORTED_BOOL_FALSE;
> >@@ -175,11 +175,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;
> 
> This confuses me. During alloc_hw we set min to 0 which makes sense for me
> but afterwards we set both, min and max, to 3? Should it not only be max to
> three here? Am I missing something?
> 

Here is also meant the range. But in this case the transceiver doesn't
support IEEE802154_HW_FRAME_RETRIES. Then the stack assumes the 802.15.4
default value and the range is "3..3".

We use this range then for NL802154 if the range is valid or not. Anyway
with this patch series the transceiver (_maybe_ driver) need to check
the ackreq bit now and if set then we turning in some transmit mode where we
waiting for an ack frame. When the ackreq bit isn't set then we need to
turn into _some_ normal tx mode.

Sadly is that the "ARET" mode and "TX ONLY" mode differs on some
transceivers if they do then CSMA handling or not. :( For me it makes
completely sense to turn into ARET mode or not with checking the ackreq
bit, because we cannot always turn into ARET mode. ARET mode means
usually "waiting for ack frame after transmit" and this _SHOULD_
transceivers do when ackreq is set. If no ack frames comes back ->
transceiver logic is "transmit failed".

- Alex

Re: [RFCv3 bluetooth-next 4/6] mac802154: change max_frame_retries behaviour

[Alexander Aring]

On Tue, Aug 04, 2015 at 08:00:09PM +0200, Alexander Aring wrote:
> On Tue, Aug 04, 2015 at 06:40:46PM +0200, Stefan Schmidt wrote:
> > Hello.
> > 
> > On 30/07/15 10:55, Alexander Aring wrote:
> > >This patch changes the default min value of max_frame_retries to 0 and
> > >changes the max_frame_retries default value to 3 which is also 802.15.4
> > >default.
> > You mean min_frame_retries to 0 here?
> 
> No, you mixed "frame_retries" ranges with "frame_retries" value.
> 

I need to admit, the mib value in 802.15.4 is named "max_frame_retries".
But it is sounds bad to name the variables for the range to:

min_max_frame_retries and max_max_frame_retries

so for the range we drop the "max" here.

At min_be and max_be, we need to do that, because we have a "min":

min_minbe, max_minbe, min_maxbe, max_maxbe.

But for "max_frame_retries" there exists no "min_frame_retries" inside
the 802.15.4 mib. We have this at maxbe, minbe because it's a random
generated number for some range, "max_frame_retries" is a static value.

- Alex

Re: [RFCv3 bluetooth-next 2/6] ieee802154: add helpers for frame control checks

[Stefan Schmidt]

Hello.

On 04/08/15 19:44, Alexander Aring wrote:
> On Tue, Aug 04, 2015 at 06:29:13PM +0200, Stefan Schmidt wrote:
>> Hello.
>>
>> On 30/07/15 10:55, Alexander Aring wrote:
>>> This patch introduce two static inline functions. The first to get the
>>> frame control field from an sk_buff. The second is for checking on the
>>> acknowledgment request bit on the frame control field. Later we can
>>> introduce more functions to check on the frame control fields.
>>>
>>> These will deprecate the current behaviour which requires a
>>> host-byteorder conversion and manually bit handling.
>>>
>>> Signed-off-by: Alexander Aring<alex.aring@gmail.com>
>>> ---
>> Some language suggestions inside.
> ok.
>>>   include/linux/ieee802154.h | 29 +++++++++++++++++++++++++++++
>>>   1 file changed, 29 insertions(+)
>>>
>>> diff --git a/include/linux/ieee802154.h b/include/linux/ieee802154.h
>>> index 1dc1f4e..4f26c01 100644
>>> --- a/include/linux/ieee802154.h
>>> +++ b/include/linux/ieee802154.h
>>> @@ -25,6 +25,8 @@
>>>   #include <linux/types.h>
>>>   #include <linux/random.h>
>>> +#include <linux/skbuff.h>
>>> +#include <linux/unaligned/memmove.h>
>>>   #include <asm/byteorder.h>
>>>   #define IEEE802154_MTU			127
>>> @@ -205,6 +207,33 @@ enum {
>>>   	IEEE802154_SCAN_IN_PROGRESS = 0xfc,
>>>   };
>>> +/* frame control handling */
>>> +#define IEEE802154_FCTL_ACKREQ	0x0020
>>> +
>>> +/**
>>> + * ieee802154_is_ackreq - check if acknowledgment request bit is set
>>> + * @fc: frame control bytes in little-endian byteorder
>>> + */
>>> +static inline bool ieee802154_is_ackreq(__le16 fc)
>>> +{
>>> +	return fc & cpu_to_le16(IEEE802154_FCTL_ACKREQ);
>>> +}
>>> +
>>> +/**
>>> + * ieee802154_get_fc_from_skb - get the frame control field from an skb
>> ... from a skb
> ok.
>>> + * @skb: skb where the frame control field will be get from
>> Maybe:
>>
>> skb which contains the frame control field
>>
> ok.
>>> + */
>>> +static inline __le16 ieee802154_get_fc_from_skb(const struct sk_buff *skb)
>>> +{
>>> +	/* return some invalid fc on failure */
>> Maybe:
>>
>> return on invalid fc
>>
> ok.
>>> +	if (unlikely(skb->mac_len < 2)) {
>>> +		WARN_ON(1);
>>> +		return cpu_to_le16(0);
>>> +	}
>>> +
>>> +	return (__force __le16)__get_unaligned_memmove16(skb_mac_header(skb));
>> Just to make sure we don't run into problems like we did with the 6lowpan
>> stack. __get_unaligned_memmove16 is not pulling the fc bytes out of the skb,
>> right? The skb stays as it is.
>>
> right it doesn't manipulate the skb. For the "problems like we did with
> the 6lowpan" you need to decide which problems, I see several:
>
>   - running skb_pull (which removes) buffer and we don't have the room to
>     pull out the bytes of skb, example: skb->len = 3, skb_pull size is 4
>     which ends in a BUG(), we need to check it with skb_may_pull before.

Sorry, I should have been a bit more verbose here to make it clearer. I 
meant the above.

regards
Stefan Schmidt

Re: [RFCv3 bluetooth-next 0/6] ieee802154: aret handling changes

[Alexander Aring]

On Tue, Aug 04, 2015 at 06:28:56PM +0200, Stefan Schmidt wrote:
> Hello.
> 
> On 30/07/15 10:55, Alexander Aring wrote:
> >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.
> 
> I think it is sound to handle it per frame to give us the opportunity to
> change this setting without taking the interface down. I wonder about the
> frame retry counter which needs to be written in some transceiver register
> to be used by the hardware directly. That one can only be done when the
> interface is down. Whcih means we can decide if we want ARET handling per
> frame but not how often the retry happens. Do I miss something?
> 

yes, turning into ARET mode (which use the max_frame_retries setting) or
NOT ARET mode (I always call this normal TX mode, which don't use the
max_frame_retries value) is now per frame. It depends on ackreq bit is
set or not, because this bit occurs - when the frame is transmitted the
receiving node will send a ack frame back then.

The number "how often the retry is inside ARET mode, if no ack was received"
can be set by interface down only. I already thought about to remove the
check "netif_running" (we probably can do that later). The point where I
am feeling bad is, this is some mac operation which is done by hardware.

We have with mac802154 normally the SoftMAC implementation for doing MAC
layer on linux side, but for ARET handling we can't because these timing
restrictions. In linux when a phy have a running interface means that
the mac802154 is operational and the phy do receive/transmit handling.

Now I can say what happens when e.g. at86rf2xx transceiver is in
TX_ARET_BUSY mode (means something is currently transmitted in ARET mode).
While in this state the transceiver changes his mac value
"max_frame_retries" to some other value. What happens then? I don't
know, the datasheet doesn't say anything how this is protected while
transmit handling. Maybe it isn't protected.

So we have the two arguments for allow/deny transceiver mac settings
while interface up:

Argument to allow:
 - The transceiver will handle the change while transmit "somehow".

Argument to deny:
 - We simple don't allow change mac settings which are handled by
   transceiver while mac is "operational" -> interface is up.

Currently we have the option where we deny to change these mib values
while mac is operational. I also don't think that there exists much use
case for doing a "complex" locking mechanism to allow the change while
interface is up. And this will huge complex with multiple interface
support and such things. Easiest way -> setup mib values which are
handled by transceiver on interface up and don't allow changes during
mac operational.

That's why I do different handling for mib values and pib values.

For phy settings (pib) CCA/channel/page we allow to change these values
during interface up. These values should not having much todo with the
mac layer. Probably we can argument with the same argument what happens
while TX_ARET_BUSY and different channel setting? I also don't know what
happens then. :-) But the point is that the linux side doesn't do any
phy layer things, for MAC we have mac802154 and the transceiver mac
operations (ARET/CSMA/etc...).

> >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.
> 
> You have the wpan-tools bits for the new netlink command pushed somewhere to
> test out with this patchset?
> 

Yes, I have it. Will send them later. Also I want to include for "next
nl802154 feature" the CONFIG_NL802154_EXPERIMENTAL, but I don't think we
should introduce it for this patch series. Because we change the
behaviour of max_frame_retries -1 to the ackreq bit. We will introduce
them later then (maybe for llsec).

Should we name it ackreq_default or ackreq_fallback? I don't think
either that this value need to handled by cfg802154_ops, because setting
this configuration is just a variable in wpan_dev. It's bad that I don't
have a HardMAC driver so I don't know right now to dealing with this value.
I think it's fine to set this value in nl802154, it doesn't need to
reach the mac802154 layer. For "real mib" values this looks different
the underlaying layer need to know the mib values then. Anyway we can
also remove it later again if we don't see a use case for that.

- Alex

Re: [RFCv3 bluetooth-next 2/6] ieee802154: add helpers for frame control checks

[Alexander Aring]

On Tue, Aug 04, 2015 at 08:35:33PM +0200, Stefan Schmidt wrote:
...
> >right it doesn't manipulate the skb. For the "problems like we did with
> >the 6lowpan" you need to decide which problems, I see several:
> >
> >  - running skb_pull (which removes) buffer and we don't have the room to
> >    pull out the bytes of skb, example: skb->len = 3, skb_pull size is 4
> >    which ends in a BUG(), we need to check it with skb_may_pull before.
> 
> Sorry, I should have been a bit more verbose here to make it clearer. I
> meant the above.
> 

This doesn't happend here. It's just dereferencing frame control field
which should be always at beginning of skb_mac_header. We need the
unaligned access, because we cannot assume that the address is at an
aligned address.

And the frame control field is in little endian byte order, we simple
use the byte order which we also transmit (everywhere).

- Alex

Re: [RFCv3 bluetooth-next 4/6] mac802154: change max_frame_retries behaviour

[Stefan Schmidt]

Hello.

On 04/08/15 20:00, Alexander Aring wrote:
> On Tue, Aug 04, 2015 at 06:40:46PM +0200, Stefan Schmidt wrote:
>> Hello.
>>
>> On 30/07/15 10:55, Alexander Aring wrote:
>>> This patch changes the default min value of max_frame_retries to 0 and
>>> changes the max_frame_retries default value to 3 which is also 802.15.4
>>> default.
>> You mean min_frame_retries to 0 here?
> No, you mixed "frame_retries" ranges with "frame_retries" value.

So one is meant as a range, got it.

>>
>>> We don't use max_frame_retries "-1" as indicator for no-aret mode
>>> anymore, instead we checking on the ack request bit inside the 802.15.4
>>> frame control field.
>>>
>>> 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;
> Here is meant the frame_retries value, 802.15.4-2011 describe this value
> as 3 for default.
>
>>>   	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 91f1208..6e1764b 100644
>>> --- a/net/mac802154/main.c
>>> +++ b/net/mac802154/main.c
>>> @@ -109,7 +109,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;
>> Maybe also set it to 3 here if we want to have this as default and letting
>> it to userspace to set it to another value if needed?
>>
> Here is the frame_retries range by 802.15.4 which is "0..7". So now
> there comes more internal mechanism, this range is set ONLY when you the
> driver supports IEEE802154_HW_FRAME_RETRIES. If a transceiver supports
> IEEE802154_HW_FRAME_RETRIES but not the default of range "0..7" then the
> driver can overwrite this range.

Allowed range, fine.

>>>   	phy->supported.max_csma_backoffs = 5;
>>>   	phy->supported.lbt = NL802154_SUPPORTED_BOOL_FALSE;
>>> @@ -175,11 +175,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;
>> This confuses me. During alloc_hw we set min to 0 which makes sense for me
>> but afterwards we set both, min and max, to 3? Should it not only be max to
>> three here? Am I missing something?
>>
> Here is also meant the range. But in this case the transceiver doesn't
> support IEEE802154_HW_FRAME_RETRIES. Then the stack assumes the 802.15.4
> default value and the range is "3..3".

Hmm, would it not make sense to assume that a transceiver which does not 
support IEEE802154_HW_FRAME_RETRIES does not support ARET at all?

For me it sounds more as if we would want to set the range to 0..0 here 
because the transceiver does not support IEEE802154_HW_FRAME_RETRIES 
andthus no ARET.

regards
Stefan Schmidt

Re: [RFCv3 bluetooth-next 2/6] ieee802154: add helpers for frame control checks

[Stefan Schmidt]

Hello.

On 04/08/15 20:47, Alexander Aring wrote:
> On Tue, Aug 04, 2015 at 08:35:33PM +0200, Stefan Schmidt wrote:
> ...
>>> right it doesn't manipulate the skb. For the "problems like we did with
>>> the 6lowpan" you need to decide which problems, I see several:
>>>
>>>   - running skb_pull (which removes) buffer and we don't have the room to
>>>     pull out the bytes of skb, example: skb->len = 3, skb_pull size is 4
>>>     which ends in a BUG(), we need to check it with skb_may_pull before.
>> Sorry, I should have been a bit more verbose here to make it clearer. I
>> meant the above.
>>
> This doesn't happend here. It's just dereferencing frame control field
> which should be always at beginning of skb_mac_header. We need the
> unaligned access, because we cannot assume that the address is at an
> aligned address.
>
> And the frame control field is in little endian byte order, we simple
> use the byte order which we also transmit (everywhere).

Thanks for explaining it again.

regards
Stefan Schmidt

Re: [RFCv3 bluetooth-next 0/6] ieee802154: aret handling changes

[Stefan Schmidt]

Hello.

On 04/08/15 20:42, Alexander Aring wrote:
> On Tue, Aug 04, 2015 at 06:28:56PM +0200, Stefan Schmidt wrote:
>> Hello.
>>
>> On 30/07/15 10:55, Alexander Aring wrote:
>>> 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.
>> I think it is sound to handle it per frame to give us the opportunity to
>> change this setting without taking the interface down. I wonder about the
>> frame retry counter which needs to be written in some transceiver register
>> to be used by the hardware directly. That one can only be done when the
>> interface is down. Whcih means we can decide if we want ARET handling per
>> frame but not how often the retry happens. Do I miss something?
>>
> yes, turning into ARET mode (which use the max_frame_retries setting) or
> NOT ARET mode (I always call this normal TX mode, which don't use the
> max_frame_retries value) is now per frame. It depends on ackreq bit is
> set or not, because this bit occurs - when the frame is transmitted the
> receiving node will send a ack frame back then.
>
> The number "how often the retry is inside ARET mode, if no ack was received"
> can be set by interface down only. I already thought about to remove the
> check "netif_running" (we probably can do that later). The point where I
> am feeling bad is, this is some mac operation which is done by hardware.
>
> We have with mac802154 normally the SoftMAC implementation for doing MAC
> layer on linux side, but for ARET handling we can't because these timing
> restrictions. In linux when a phy have a running interface means that
> the mac802154 is operational and the phy do receive/transmit handling.
>
> Now I can say what happens when e.g. at86rf2xx transceiver is in
> TX_ARET_BUSY mode (means something is currently transmitted in ARET mode).
> While in this state the transceiver changes his mac value
> "max_frame_retries" to some other value. What happens then? I don't
> know, the datasheet doesn't say anything how this is protected while
> transmit handling. Maybe it isn't protected.
>
> So we have the two arguments for allow/deny transceiver mac settings
> while interface up:
>
> Argument to allow:
>   - The transceiver will handle the change while transmit "somehow".
>
> Argument to deny:
>   - We simple don't allow change mac settings which are handled by
>     transceiver while mac is "operational" -> interface is up.
>
> Currently we have the option where we deny to change these mib values
> while mac is operational.

We should keep it as is for now. Only changing mac setting while the 
interface is down.
I was just curious.

It could be that we need to re-think it later for some settings (like 
the retry counter) but there is no need now and we would to more testing 
to see how the transceiver are behaving with this.

>   I also don't think that there exists much use
> case for doing a "complex" locking mechanism to allow the change while
> interface is up. And this will huge complex with multiple interface
> support and such things. Easiest way -> setup mib values which are
> handled by transceiver on interface up and don't allow changes during
> mac operational.
>
> That's why I do different handling for mib values and pib values.
>
> For phy settings (pib) CCA/channel/page we allow to change these values
> during interface up. These values should not having much todo with the
> mac layer. Probably we can argument with the same argument what happens
> while TX_ARET_BUSY and different channel setting? I also don't know what
> happens then. :-) But the point is that the linux side doesn't do any
> phy layer things, for MAC we have mac802154 and the transceiver mac
> operations (ARET/CSMA/etc...).
>
>>> 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.
>> You have the wpan-tools bits for the new netlink command pushed somewhere to
>> test out with this patchset?
>>
> Yes, I have it. Will send them later. Also I want to include for "next
> nl802154 feature" the CONFIG_NL802154_EXPERIMENTAL, but I don't think we
> should introduce it for this patch series. Because we change the
> behaviour of max_frame_retries -1 to the ackreq bit. We will introduce
> them later then (maybe for llsec).

I think this is a small enough addition that we do not need the 
CONFIG_NL802154_EXPERIMENTAL guard for it yet.
> Should we name it ackreq_default or ackreq_fallback?

I would go with ackreq_default.

regards
Stefan Schmidt

Re: [RFCv3 bluetooth-next 4/6] mac802154: change max_frame_retries behaviour

[Alexander Aring]

On Wed, Aug 05, 2015 at 10:46:04AM +0200, Stefan Schmidt wrote:
...
> >>This confuses me. During alloc_hw we set min to 0 which makes sense for me
> >>but afterwards we set both, min and max, to 3? Should it not only be max to
> >>three here? Am I missing something?
> >>
> >Here is also meant the range. But in this case the transceiver doesn't
> >support IEEE802154_HW_FRAME_RETRIES. Then the stack assumes the 802.15.4
> >default value and the range is "3..3".
> 
> Hmm, would it not make sense to assume that a transceiver which does not
> support IEEE802154_HW_FRAME_RETRIES does not support ARET at all?
> 

We indicate right now that the frame_retries value "-1" indicates that the
transceiver running in "no ARET" support.

I am fine to introduce this with a new HW flag which indicates that the
transceiver doesn't support ARET handling. But then we can also bring
the AACK flag back.

We can't forbid from stack that we doesn't send out MAC frames where the
ack request bit is set. Some of the MLME-ops requires ack handling (or
we simple don't do what the standard says and always clear this bit
before transmit when IEEE802154_HW_NO_ARET is set). Or we can also do
just printout a WARN_ONCE then that the user should buy a "real"
transceiver.

This is the same like the AACK handling flag. The mac802154 layer Should
never see any ACK frames because we can't handle them and it looks also
that when the transceivers support AACK handling, then ack frames are
completely handled by the transceiver, they don't send them to the next
layer (except if you are in promiscuous mode).

Anyway, if the transceiver doesn't support ARET _or_ AACK you _always_
running in issues in 802.15.4 networks which use acknowledge handling.

What I definitely want is to get the "-1" value out of frame_retries
behaviour. The standard says a range of "0..7" here and if the driver
implementation too poor to report it then we think it's 802.15.4 default.
There exists no value which indicates "no frame_retries handling
supported"

(In case of mrf24j40 this is also true, because it doesn't support
setting of frame_retries. It's in this case really always 3). See [0],

"The aMaxFrameRetries value is a constant and not configurable."

"... is not received after aMaxFrameRetries (3) transmissions."

The ack handling can be set by setting some bit "TXNACKREQ" and
currently the mrf24j40 implementation do this also per frame and check
on ackreq bit [1].

In my opninion, we have the frame_retries = -1 value because historical
issues and should do the handling like mrf24j40.

btw:
I currently try to work on some things on this driver.

> For me it sounds more as if we would want to set the range to 0..0 here
> because the transceiver does not support IEEE802154_HW_FRAME_RETRIES andthus
> no ARET.
> 

no, (frame_retries = 0) != no ARET.

The reason is easy, you can set frame_retries to zero and going into
ARET mode. The important thing is "the transceiver waits for an ACK
frame after transmit", if this is not see then the transceiver logic is
-> transmit failed. (In this case you have no retransmit, because
frame_retries is 0).


The diffirent between no ARET handling is, the transceiver _doesn't_ wait
for an ACK frame and it's always successful to send something out.

- Alex

[0] http://ww1.microchip.com/downloads/en/DeviceDoc/39776C.pdf
[1] http://lxr.free-electrons.com/source/drivers/net/ieee802154/mrf24j40.c#L355