[PATCHv3 bluetooth-next 0/4] ieee802154: nl802154 SET commands and pib defaults

[PATCHv3 bluetooth-next 0/4] ieee802154: nl802154 SET commands and pib defaults

[Alexander Aring]

Hi,

this patch-series implement the already existed SET commands of an interface
and a wpan-phy. Currently we have one CMD for each setting which was a mistake
by me to implement it in this way. So we aren't currently a "real" UAPI header
for nl802154 and it's still under development. I decide now to change to
implement these setting in this way now. Currently I let these commands in
coexists with the old commands but _NOTE_ I will remove the other commands
from the nl802154 later.

So please use the new commands. When these patches are mainline I also want to
release a new wpan-tools release which use this new interface. Then everybody
has some time to change to the new wpan-tools release. After this "time" I will
remove the obsolete nl802154 commands from the header. This means older
wpan-tools releases doesn't work any more with new kernels. Also this means
newer wpan-tools releases doesn't work with older kernels. That will end in
some kind of chaos and I will note this on the website.

This behaviour to do it in only one command is much easier to handle in
userspace, like clicking an "apply button".


Also this patch series contains two patches for move the MAC PIB defaults
setting out of SoftMAC layer. Additional we set the max frame retries
parameter to 3 which is a real 802.15.4 default. Some transceivers doesn't
support ARET settings. If these transceivers doesn't support it, we assume
the 802.15.4 default.

- Alex

changes since v2:
 - remove ".done = nl802154_dump_wpan_phy_done" of patch 1/2.
 - add other movements for mac pib defaults which is needed for cfg802154
   crypto layer.

changes since v3:
 - some cleanups in the patch commit msgs, mostly I want to resend this series
   only.

Alexander Aring (4):
  nl802154: add set wpan phy cmd
  nl802154: add set interface cmd
  ieee802154: move mac pib defaults
  ieee802154: set aret handling according to 802.15.4

 net/ieee802154/core.c     |  29 +++++++++
 net/ieee802154/nl802154.c | 161 ++++++++++++++++++++++++++++++++++++++++++++++
 net/mac802154/iface.c     |  15 -----
 3 files changed, 190 insertions(+), 15 deletions(-)

-- 
2.3.5

[PATCHv3 bluetooth-next 1/4] nl802154: add set wpan phy cmd

[Alexander Aring]

This patch adds NL802154_CMD_SET_WPAN_PHY command to set all wpan phy
attributes instead of doing separate commands. This will introduce an
easilier wpan phy settings handling in userspace application with nl802154.

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

diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
index a4daf91..c12c07f 100644
--- a/net/ieee802154/nl802154.c
+++ b/net/ieee802154/nl802154.c
@@ -437,6 +437,49 @@ static int nl802154_get_wpan_phy(struct sk_buff *skb, struct genl_info *info)
 	return genlmsg_reply(msg, info);
 }
 
+static int nl802154_set_wpan_phy(struct sk_buff *skb, struct genl_info *info)
+{
+	struct cfg802154_registered_device *rdev = info->user_ptr[0];
+	int ret;
+
+	if (info->attrs[NL802154_ATTR_PAGE] &&
+	    info->attrs[NL802154_ATTR_CHANNEL]) {
+		u8 channel, page;
+
+		page = nla_get_u8(info->attrs[NL802154_ATTR_PAGE]);
+		channel = nla_get_u8(info->attrs[NL802154_ATTR_CHANNEL]);
+		if (page > IEEE802154_MAX_PAGE ||
+		    channel > IEEE802154_MAX_CHANNEL)
+			return -EINVAL;
+
+		ret = rdev_set_channel(rdev, page, channel);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (info->attrs[NL802154_ATTR_CCA_MODE]) {
+		struct wpan_phy_cca cca;
+
+		cca.mode = nla_get_u32(info->attrs[NL802154_ATTR_CCA_MODE]);
+		if (cca.mode < NL802154_CCA_ENERGY ||
+		    cca.mode > NL802154_CCA_ATTR_MAX)
+			return -EINVAL;
+
+		if (cca.mode == NL802154_CCA_ENERGY_CARRIER) {
+			if (!info->attrs[NL802154_ATTR_CCA_OPT])
+				return -EINVAL;
+
+			cca.opt = nla_get_u32(info->attrs[NL802154_ATTR_CCA_OPT]);
+			if (cca.opt > NL802154_CCA_OPT_ATTR_MAX)
+				return -EINVAL;
+		}
+
+		return rdev_set_cca_mode(rdev, &cca);
+	}
+
+	return 0;
+}
+
 static inline u64 wpan_dev_id(struct wpan_dev *wpan_dev)
 {
 	return (u64)wpan_dev->identifier |
@@ -896,6 +939,14 @@ static const struct genl_ops nl802154_ops[] = {
 				  NL802154_FLAG_NEED_RTNL,
 	},
 	{
+		.cmd = NL802154_CMD_SET_WPAN_PHY,
+		.doit = nl802154_set_wpan_phy,
+		.policy = nl802154_policy,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL802154_FLAG_NEED_WPAN_PHY |
+				  NL802154_FLAG_NEED_RTNL,
+	},
+	{
 		.cmd = NL802154_CMD_GET_INTERFACE,
 		.doit = nl802154_get_interface,
 		.dumpit = nl802154_dump_interface,
-- 
2.3.5

[PATCHv3 bluetooth-next 2/4] nl802154: add set interface cmd

[Alexander Aring]

This patch introduce the NL802154_CMD_SET_INTERFACE command which
handles setting of all wpan interface mac attributes. his will introduce
an easilier wpan mac settings handling in userspace application with
nl802154.

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

diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
index c12c07f..e2f50ba 100644
--- a/net/ieee802154/nl802154.c
+++ b/net/ieee802154/nl802154.c
@@ -603,6 +603,108 @@ static int nl802154_get_interface(struct sk_buff *skb, struct genl_info *info)
 	return genlmsg_reply(msg, info);
 }
 
+static int nl802154_set_interface(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;
+	int ret;
+
+	if (info->attrs[NL802154_ATTR_PAN_ID]) {
+		__le16 pan_id;
+
+		if (wpan_dev->iftype == NL802154_IFTYPE_MONITOR)
+			return -EINVAL;
+
+		if (netif_running(dev))
+			return -EBUSY;
+
+		pan_id = nla_get_le16(info->attrs[NL802154_ATTR_PAN_ID]);
+		ret = rdev_set_pan_id(rdev, wpan_dev, pan_id);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (info->attrs[NL802154_ATTR_SHORT_ADDR]) {
+		__le16 short_addr;
+
+		if (wpan_dev->iftype == NL802154_IFTYPE_MONITOR)
+			return -EINVAL;
+
+		if (netif_running(dev))
+			return -EBUSY;
+
+		short_addr = nla_get_le16(info->attrs[NL802154_ATTR_SHORT_ADDR]);
+
+		ret = rdev_set_short_addr(rdev, wpan_dev, short_addr);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (info->attrs[NL802154_ATTR_MAX_FRAME_RETRIES]) {
+		s8 max_frame_retries;
+
+		if (netif_running(dev))
+			return -EBUSY;
+
+		max_frame_retries = nla_get_s8(
+				info->attrs[NL802154_ATTR_MAX_FRAME_RETRIES]);
+		if (max_frame_retries < -1 || max_frame_retries > 7)
+			return -EINVAL;
+
+		ret = rdev_set_max_frame_retries(rdev, wpan_dev,
+						 max_frame_retries);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (info->attrs[NL802154_ATTR_MIN_BE] &&
+	    info->attrs[NL802154_ATTR_MAX_BE]) {
+		u8 min_be, max_be;
+
+		if (netif_running(dev))
+			return -EBUSY;
+
+		min_be = nla_get_u8(info->attrs[NL802154_ATTR_MIN_BE]);
+		max_be = nla_get_u8(info->attrs[NL802154_ATTR_MAX_BE]);
+		if (max_be < 3 || max_be > 8 || min_be > max_be)
+			return -EINVAL;
+
+		ret = rdev_set_backoff_exponent(rdev, wpan_dev, min_be, max_be);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (info->attrs[NL802154_ATTR_MAX_CSMA_BACKOFFS]) {
+		u8 max_csma_backoffs;
+
+		if (netif_running(dev))
+			return -EBUSY;
+
+		max_csma_backoffs = nla_get_u8(
+				info->attrs[NL802154_ATTR_MAX_CSMA_BACKOFFS]);
+		if (max_csma_backoffs > 5)
+			return -EINVAL;
+
+		ret = rdev_set_max_csma_backoffs(rdev, wpan_dev,
+						 max_csma_backoffs);
+		if (ret < 0)
+			return ret;
+	}
+
+	if (info->attrs[NL802154_ATTR_LBT_MODE]) {
+		bool mode;
+
+		if (netif_running(dev))
+			return -EBUSY;
+
+		mode = !!nla_get_u8(info->attrs[NL802154_ATTR_LBT_MODE]);
+		return rdev_set_lbt_mode(rdev, wpan_dev, mode);
+	}
+
+	return 0;
+}
+
 static int nl802154_new_interface(struct sk_buff *skb, struct genl_info *info)
 {
 	struct cfg802154_registered_device *rdev = info->user_ptr[0];
@@ -964,6 +1066,14 @@ static const struct genl_ops nl802154_ops[] = {
 				  NL802154_FLAG_NEED_RTNL,
 	},
 	{
+		.cmd = NL802154_CMD_SET_INTERFACE,
+		.doit = nl802154_set_interface,
+		.policy = nl802154_policy,
+		.flags = GENL_ADMIN_PERM,
+		.internal_flags = NL802154_FLAG_NEED_NETDEV |
+				  NL802154_FLAG_NEED_RTNL,
+	},
+	{
 		.cmd = NL802154_CMD_DEL_INTERFACE,
 		.doit = nl802154_del_interface,
 		.policy = nl802154_policy,
-- 
2.3.5

[PATCHv3 bluetooth-next 3/4] ieee802154: move mac pib defaults

[Alexander Aring]

This patch moves the mac pib defaults setting out of SoftMAC layer. This
will prepare for possible HardMAC drivers that these values are 802.15.4
defaults while netdev registration. Currently the SoftMAC layer is the wrong
place to do the settings of pib defaults.

Signed-off-by: Alexander Aring <alex.aring@gmail.com>
---
 net/ieee802154/core.c | 30 ++++++++++++++++++++++++++++++
 net/mac802154/iface.c | 15 ---------------
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c
index 2ee00e8..7a107b0 100644
--- a/net/ieee802154/core.c
+++ b/net/ieee802154/core.c
@@ -211,6 +211,35 @@ cfg802154_update_iface_num(struct cfg802154_registered_device *rdev,
 	rdev->num_running_ifaces += num;
 }
 
+static void
+cfg802154_reset_mac_pib(struct wpan_dev *wpan_dev)
+{
+	get_random_bytes(&wpan_dev->bsn, 1);
+	get_random_bytes(&wpan_dev->dsn, 1);
+
+	/* defaults per 802.15.4-2011 */
+	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->pan_id = cpu_to_le16(IEEE802154_PAN_ID_BROADCAST);
+	wpan_dev->short_addr = cpu_to_le16(IEEE802154_ADDR_SHORT_BROADCAST);
+
+	/* normally false, but monitors are always in promiscuous_mode */
+	switch (wpan_dev->iftype) {
+	case NL802154_IFTYPE_NODE:
+		wpan_dev->promiscuous_mode = false;
+		break;
+	case NL802154_IFTYPE_MONITOR:
+		wpan_dev->promiscuous_mode = true;
+		break;
+	default:
+		BUG();
+	}
+}
+
 static int cfg802154_netdev_notifier_call(struct notifier_block *nb,
 					  unsigned long state, void *ptr)
 {
@@ -234,6 +263,7 @@ static int cfg802154_netdev_notifier_call(struct notifier_block *nb,
 		rdev->devlist_generation++;
 
 		wpan_dev->netdev = dev;
+		cfg802154_reset_mac_pib(wpan_dev);
 		break;
 	case NETDEV_DOWN:
 		cfg802154_update_iface_num(rdev, wpan_dev->iftype, -1);
diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
index 38b56f9..3f6a3fe 100644
--- a/net/mac802154/iface.c
+++ b/net/mac802154/iface.c
@@ -479,19 +479,6 @@ ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata,
 	sdata->vif.type = type;
 	sdata->wpan_dev.iftype = type;
 
-	get_random_bytes(&wpan_dev->bsn, 1);
-	get_random_bytes(&wpan_dev->dsn, 1);
-
-	/* defaults per 802.15.4-2011 */
-	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->pan_id = cpu_to_le16(IEEE802154_PANID_BROADCAST);
-	wpan_dev->short_addr = cpu_to_le16(IEEE802154_ADDR_BROADCAST);
-
 	switch (type) {
 	case NL802154_IFTYPE_NODE:
 		ieee802154_be64_to_le64(&wpan_dev->extended_addr,
@@ -501,7 +488,6 @@ ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata,
 		sdata->dev->destructor = mac802154_wpan_free;
 		sdata->dev->netdev_ops = &mac802154_wpan_ops;
 		sdata->dev->ml_priv = &mac802154_mlme_wpan;
-		wpan_dev->promiscuous_mode = false;
 
 		spin_lock_init(&sdata->mib_lock);
 		mutex_init(&sdata->sec_mtx);
@@ -511,7 +497,6 @@ ieee802154_setup_sdata(struct ieee802154_sub_if_data *sdata,
 	case NL802154_IFTYPE_MONITOR:
 		sdata->dev->destructor = free_netdev;
 		sdata->dev->netdev_ops = &mac802154_monitor_ops;
-		wpan_dev->promiscuous_mode = true;
 		break;
 	default:
 		BUG();
-- 
2.3.5

[PATCHv3 bluetooth-next 4/4] ieee802154: set aret handling according to 802.15.4

[Alexander Aring]

The 802.15.4 standard describes a max frame retries parameter 3 as
default. Currently this parameter affects only the transceivers which
supports automatic retransmission. This at the moment the at86rf230
driver, all other drivers which don't support setting of max frame
retries parameter we assume the 802.15.4 default now.

This change will introduce that the at86rf230 driver will go into ARET
mode (max frame retries parameter above or equal zero) per default.

If somebody experience that other nodes receives the same frame
three times repeatly, then your receiving node doesn't support AACK
handling for receiving. For all users they should set the max frame
retries parameter to "-1" which turns off the ARET mode and restore the
old behaviour.

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

diff --git a/net/ieee802154/core.c b/net/ieee802154/core.c
index 7a107b0..619a220 100644
--- a/net/ieee802154/core.c
+++ b/net/ieee802154/core.c
@@ -221,8 +221,7 @@ cfg802154_reset_mac_pib(struct wpan_dev *wpan_dev)
 	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_PAN_ID_BROADCAST);
 	wpan_dev->short_addr = cpu_to_le16(IEEE802154_ADDR_SHORT_BROADCAST);
-- 
2.3.5

Re: [PATCHv3 bluetooth-next 2/4] nl802154: add set interface cmd

[Phoebe Buckheister]

On Tue,  7 Apr 2015 13:49:51 +0200
Alexander Aring <alex.aring@gmail.com> wrote:

> This patch introduce the NL802154_CMD_SET_INTERFACE command which
> handles setting of all wpan interface mac attributes. his will
> introduce an easilier wpan mac settings handling in userspace
> application with nl802154.
> 
> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> ---
>  net/ieee802154/nl802154.c | 110
> ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110
> insertions(+)
> 
> diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
> index c12c07f..e2f50ba 100644
> --- a/net/ieee802154/nl802154.c
> +++ b/net/ieee802154/nl802154.c
> @@ -603,6 +603,108 @@ static int nl802154_get_interface(struct
> sk_buff *skb, struct genl_info *info) return genlmsg_reply(msg, info);
>  }
>  
> +static int nl802154_set_interface(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;
> +	int ret;
> +
> +	if (info->attrs[NL802154_ATTR_PAN_ID]) {
> +		__le16 pan_id;
> +
> +		if (wpan_dev->iftype == NL802154_IFTYPE_MONITOR)
> +			return -EINVAL;
> +
> +		if (netif_running(dev))
> +			return -EBUSY;
> +
> +		pan_id =
> nla_get_le16(info->attrs[NL802154_ATTR_PAN_ID]);
> +		ret = rdev_set_pan_id(rdev, wpan_dev, pan_id);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	if (info->attrs[NL802154_ATTR_SHORT_ADDR]) {
> +		__le16 short_addr;
> +
> +		if (wpan_dev->iftype == NL802154_IFTYPE_MONITOR)
> +			return -EINVAL;

This is not good. You may have partially applied the requested settings
when you return with an error, leaving the device in some intermediate
state that is most likely not useful. It'd be better to first check
whether all settings can be applied, and then apply them all at once.
But even then we have a problem with rolling back some changes if a
command fails :/

> +
> +		if (netif_running(dev))
> +			return -EBUSY;
> +
> +		short_addr =
> nla_get_le16(info->attrs[NL802154_ATTR_SHORT_ADDR]); +
> +		ret = rdev_set_short_addr(rdev, wpan_dev,
> short_addr);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	if (info->attrs[NL802154_ATTR_MAX_FRAME_RETRIES]) {
> +		s8 max_frame_retries;
> +
> +		if (netif_running(dev))
> +			return -EBUSY;
> +
> +		max_frame_retries = nla_get_s8(
> +
> info->attrs[NL802154_ATTR_MAX_FRAME_RETRIES]);
> +		if (max_frame_retries < -1 || max_frame_retries > 7)
> +			return -EINVAL;
> +
> +		ret = rdev_set_max_frame_retries(rdev, wpan_dev,
> +						 max_frame_retries);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	if (info->attrs[NL802154_ATTR_MIN_BE] &&
> +	    info->attrs[NL802154_ATTR_MAX_BE]) {
> +		u8 min_be, max_be;
> +
> +		if (netif_running(dev))
> +			return -EBUSY;
> +
> +		min_be =
> nla_get_u8(info->attrs[NL802154_ATTR_MIN_BE]);
> +		max_be =
> nla_get_u8(info->attrs[NL802154_ATTR_MAX_BE]);
> +		if (max_be < 3 || max_be > 8 || min_be > max_be)
> +			return -EINVAL;
> +
> +		ret = rdev_set_backoff_exponent(rdev, wpan_dev,
> min_be, max_be);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	if (info->attrs[NL802154_ATTR_MAX_CSMA_BACKOFFS]) {
> +		u8 max_csma_backoffs;
> +
> +		if (netif_running(dev))
> +			return -EBUSY;
> +
> +		max_csma_backoffs = nla_get_u8(
> +
> info->attrs[NL802154_ATTR_MAX_CSMA_BACKOFFS]);
> +		if (max_csma_backoffs > 5)
> +			return -EINVAL;
> +
> +		ret = rdev_set_max_csma_backoffs(rdev, wpan_dev,
> +						 max_csma_backoffs);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	if (info->attrs[NL802154_ATTR_LBT_MODE]) {
> +		bool mode;
> +
> +		if (netif_running(dev))
> +			return -EBUSY;
> +
> +		mode
> = !!nla_get_u8(info->attrs[NL802154_ATTR_LBT_MODE]);
> +		return rdev_set_lbt_mode(rdev, wpan_dev, mode);
> +	}
> +
> +	return 0;
> +}
> +
>  static int nl802154_new_interface(struct sk_buff *skb, struct
> genl_info *info) {
>  	struct cfg802154_registered_device *rdev = info->user_ptr[0];
> @@ -964,6 +1066,14 @@ static const struct genl_ops nl802154_ops[] = {
>  				  NL802154_FLAG_NEED_RTNL,
>  	},
>  	{
> +		.cmd = NL802154_CMD_SET_INTERFACE,
> +		.doit = nl802154_set_interface,
> +		.policy = nl802154_policy,
> +		.flags = GENL_ADMIN_PERM,
> +		.internal_flags = NL802154_FLAG_NEED_NETDEV |
> +				  NL802154_FLAG_NEED_RTNL,
> +	},
> +	{
>  		.cmd = NL802154_CMD_DEL_INTERFACE,
>  		.doit = nl802154_del_interface,
>  		.policy = nl802154_policy,

Re: [PATCHv3 bluetooth-next 2/4] nl802154: add set interface cmd

[Alexander Aring]

Hi Phoebe,

On Tue, Apr 07, 2015 at 01:59:02PM +0200, Phoebe Buckheister wrote:
> On Tue,  7 Apr 2015 13:49:51 +0200
> Alexander Aring <alex.aring@gmail.com> wrote:
> 
> > This patch introduce the NL802154_CMD_SET_INTERFACE command which
> > handles setting of all wpan interface mac attributes. his will
> > introduce an easilier wpan mac settings handling in userspace
> > application with nl802154.
> > 
> > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > ---
> >  net/ieee802154/nl802154.c | 110
> > ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110
> > insertions(+)
> > 
> > diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
> > index c12c07f..e2f50ba 100644
> > --- a/net/ieee802154/nl802154.c
> > +++ b/net/ieee802154/nl802154.c
> > @@ -603,6 +603,108 @@ static int nl802154_get_interface(struct
> > sk_buff *skb, struct genl_info *info) return genlmsg_reply(msg, info);
> >  }
> >  
> > +static int nl802154_set_interface(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;
> > +	int ret;
> > +
> > +	if (info->attrs[NL802154_ATTR_PAN_ID]) {
> > +		__le16 pan_id;
> > +
> > +		if (wpan_dev->iftype == NL802154_IFTYPE_MONITOR)
> > +			return -EINVAL;
> > +
> > +		if (netif_running(dev))
> > +			return -EBUSY;
> > +
> > +		pan_id =
> > nla_get_le16(info->attrs[NL802154_ATTR_PAN_ID]);
> > +		ret = rdev_set_pan_id(rdev, wpan_dev, pan_id);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> > +	if (info->attrs[NL802154_ATTR_SHORT_ADDR]) {
> > +		__le16 short_addr;
> > +
> > +		if (wpan_dev->iftype == NL802154_IFTYPE_MONITOR)
> > +			return -EINVAL;
> 
> This is not good. You may have partially applied the requested settings
> when you return with an error, leaving the device in some intermediate
> state that is most likely not useful. It'd be better to first check
> whether all settings can be applied, and then apply them all at once.
> But even then we have a problem with rolling back some changes if a
> command fails :/
>

Yes, we need kind of rollback here if the command failed. I also stumble
over this issue. [0] Mhh, we don't have it when I do each command in their
own CMD SET call. So then I will leave the current behaviour as it is.

Some idea:

Solution would be on MAC settings that we really check if everything is
valid. Then we can set everything in the pib values, after an interface
up this values will be "really" set, like address filter settings in phy
registers.

This works for interface settings (MAC PIB values). But for phy values
this is more difficult, because it's directly driver-layer calls. So we
can't predigt if an driver layer return some error then.


If nobody complaint then I will send a v4 and remove the nl802154
settings, or maybe somebody has a better idea.

- Alex

[0] http://www.spinics.net/lists/linux-wpan/msg01568.html

Re: [PATCHv3 bluetooth-next 2/4] nl802154: add set interface cmd

[Varka Bhadram]

On 04/07/2015 05:51 PM, Alexander Aring wrote:
> Hi Phoebe,
>
> On Tue, Apr 07, 2015 at 01:59:02PM +0200, Phoebe Buckheister wrote:
>> On Tue,  7 Apr 2015 13:49:51 +0200
>> Alexander Aring <alex.aring@gmail.com> wrote:
>>
>>> This patch introduce the NL802154_CMD_SET_INTERFACE command which
>>> handles setting of all wpan interface mac attributes. his will
>>> introduce an easilier wpan mac settings handling in userspace
>>> application with nl802154.
>>>
>>> Signed-off-by: Alexander Aring <alex.aring@gmail.com>
>>> ---
>>>  net/ieee802154/nl802154.c | 110
>>> ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110
>>> insertions(+)
>>>
>>> diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
>>> index c12c07f..e2f50ba 100644
>>> --- a/net/ieee802154/nl802154.c
>>> +++ b/net/ieee802154/nl802154.c
>>> @@ -603,6 +603,108 @@ static int nl802154_get_interface(struct
>>> sk_buff *skb, struct genl_info *info) return genlmsg_reply(msg, info);
>>>  }
>>>  
>>> +static int nl802154_set_interface(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;
>>> +	int ret;
>>> +
>>> +	if (info->attrs[NL802154_ATTR_PAN_ID]) {
>>> +		__le16 pan_id;
>>> +
>>> +		if (wpan_dev->iftype == NL802154_IFTYPE_MONITOR)
>>> +			return -EINVAL;
>>> +
>>> +		if (netif_running(dev))
>>> +			return -EBUSY;
>>> +
>>> +		pan_id =
>>> nla_get_le16(info->attrs[NL802154_ATTR_PAN_ID]);
>>> +		ret = rdev_set_pan_id(rdev, wpan_dev, pan_id);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +	}
>>> +
>>> +	if (info->attrs[NL802154_ATTR_SHORT_ADDR]) {
>>> +		__le16 short_addr;
>>> +
>>> +		if (wpan_dev->iftype == NL802154_IFTYPE_MONITOR)
>>> +			return -EINVAL;
>> This is not good. You may have partially applied the requested settings
>> when you return with an error, leaving the device in some intermediate
>> state that is most likely not useful. It'd be better to first check
>> whether all settings can be applied, and then apply them all at once.
>> But even then we have a problem with rolling back some changes if a
>> command fails :/
>>
> Yes, we need kind of rollback here if the command failed. I also stumble
> over this issue. [0] Mhh, we don't have it when I do each command in their
> own CMD SET call. So then I will leave the current behaviour as it is.
>
> Some idea:
>
> Solution would be on MAC settings that we really check if everything is
> valid. Then we can set everything in the pib values, after an interface
> up this values will be "really" set, like address filter settings in phy
> registers.
>
> This works for interface settings (MAC PIB values). But for phy values
> this is more difficult, because it's directly driver-layer calls. So we
> can't predigt if an driver layer return some error then.
>
>
> If nobody complaint then I will send a v4 and remove the nl802154
> settings, or maybe somebody has a better idea.
>
> - Alex
>
> [0] http://www.spinics.net/lists/linux-wpan/msg01568.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Instead of this its better to leave the current behavior as it is.

Why do we need to complicate the process...?

-- 
Varka Bhadram

Re: [PATCHv3 bluetooth-next 2/4] nl802154: add set interface cmd

[Phoebe Buckheister]

On Tue, 7 Apr 2015 14:21:52 +0200
Alexander Aring <alex.aring@gmail.com> wrote:

> Hi Phoebe,
> 
> On Tue, Apr 07, 2015 at 01:59:02PM +0200, Phoebe Buckheister wrote:
> > On Tue,  7 Apr 2015 13:49:51 +0200
> > Alexander Aring <alex.aring@gmail.com> wrote:
> > 
> > > This patch introduce the NL802154_CMD_SET_INTERFACE command which
> > > handles setting of all wpan interface mac attributes. his will
> > > introduce an easilier wpan mac settings handling in userspace
> > > application with nl802154.
> > > 
> > > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > > ---
> > >  net/ieee802154/nl802154.c | 110
> > > ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110
> > > insertions(+)
> > > 
> > > diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
> > > index c12c07f..e2f50ba 100644
> > > --- a/net/ieee802154/nl802154.c
> > > +++ b/net/ieee802154/nl802154.c
> > > @@ -603,6 +603,108 @@ static int nl802154_get_interface(struct
> > > sk_buff *skb, struct genl_info *info) return genlmsg_reply(msg,
> > > info); }
> > >  
> > > +static int nl802154_set_interface(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;
> > > +	int ret;
> > > +
> > > +	if (info->attrs[NL802154_ATTR_PAN_ID]) {
> > > +		__le16 pan_id;
> > > +
> > > +		if (wpan_dev->iftype == NL802154_IFTYPE_MONITOR)
> > > +			return -EINVAL;
> > > +
> > > +		if (netif_running(dev))
> > > +			return -EBUSY;
> > > +
> > > +		pan_id =
> > > nla_get_le16(info->attrs[NL802154_ATTR_PAN_ID]);
> > > +		ret = rdev_set_pan_id(rdev, wpan_dev, pan_id);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +	}
> > > +
> > > +	if (info->attrs[NL802154_ATTR_SHORT_ADDR]) {
> > > +		__le16 short_addr;
> > > +
> > > +		if (wpan_dev->iftype == NL802154_IFTYPE_MONITOR)
> > > +			return -EINVAL;
> > 
> > This is not good. You may have partially applied the requested
> > settings when you return with an error, leaving the device in some
> > intermediate state that is most likely not useful. It'd be better
> > to first check whether all settings can be applied, and then apply
> > them all at once. But even then we have a problem with rolling back
> > some changes if a command fails :/
> >
> 
> Yes, we need kind of rollback here if the command failed. I also
> stumble over this issue. [0] Mhh, we don't have it when I do each
> command in their own CMD SET call. So then I will leave the current
> behaviour as it is.
> 
> Some idea:
> 
> Solution would be on MAC settings that we really check if everything
> is valid. Then we can set everything in the pib values, after an
> interface up this values will be "really" set, like address filter
> settings in phy registers.
> 
> This works for interface settings (MAC PIB values). But for phy values
> this is more difficult, because it's directly driver-layer calls. So
> we can't predigt if an driver layer return some error then.

That sounds good. We could add another driver function that checks a
PIB/MIB and returns "valid" or "i'm sorry, dave", where "valid" may only
happen when the driver *knows* that it can load the *IB into the
hardware. Any possible hardware errors are excluded by these checks,
but we'll only see those on the actual load anyway. Then we can load
the *IB in bulk and be sure that only the hardware can fail.

> 
> If nobody complaint then I will send a v4 and remove the nl802154
> settings, or maybe somebody has a better idea.
> 
> - Alex
> 
> [0] http://www.spinics.net/lists/linux-wpan/msg01568.html

Re: [PATCHv3 bluetooth-next 2/4] nl802154: add set interface cmd

[Alexander Aring]

On Tue, Apr 07, 2015 at 02:29:30PM +0200, Phoebe Buckheister wrote:
> On Tue, 7 Apr 2015 14:21:52 +0200
> Alexander Aring <alex.aring@gmail.com> wrote:
> 
> > Hi Phoebe,
> > 
> > On Tue, Apr 07, 2015 at 01:59:02PM +0200, Phoebe Buckheister wrote:
> > > On Tue,  7 Apr 2015 13:49:51 +0200
> > > Alexander Aring <alex.aring@gmail.com> wrote:
> > > 
> > > > This patch introduce the NL802154_CMD_SET_INTERFACE command which
> > > > handles setting of all wpan interface mac attributes. his will
> > > > introduce an easilier wpan mac settings handling in userspace
> > > > application with nl802154.
> > > > 
> > > > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > > > ---
> > > >  net/ieee802154/nl802154.c | 110
> > > > ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 110
> > > > insertions(+)
> > > > 
> > > > diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
> > > > index c12c07f..e2f50ba 100644
> > > > --- a/net/ieee802154/nl802154.c
> > > > +++ b/net/ieee802154/nl802154.c
> > > > @@ -603,6 +603,108 @@ static int nl802154_get_interface(struct
> > > > sk_buff *skb, struct genl_info *info) return genlmsg_reply(msg,
> > > > info); }
> > > >  
> > > > +static int nl802154_set_interface(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;
> > > > +	int ret;
> > > > +
> > > > +	if (info->attrs[NL802154_ATTR_PAN_ID]) {
> > > > +		__le16 pan_id;
> > > > +
> > > > +		if (wpan_dev->iftype == NL802154_IFTYPE_MONITOR)
> > > > +			return -EINVAL;
> > > > +
> > > > +		if (netif_running(dev))
> > > > +			return -EBUSY;
> > > > +
> > > > +		pan_id =
> > > > nla_get_le16(info->attrs[NL802154_ATTR_PAN_ID]);
> > > > +		ret = rdev_set_pan_id(rdev, wpan_dev, pan_id);
> > > > +		if (ret < 0)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	if (info->attrs[NL802154_ATTR_SHORT_ADDR]) {
> > > > +		__le16 short_addr;
> > > > +
> > > > +		if (wpan_dev->iftype == NL802154_IFTYPE_MONITOR)
> > > > +			return -EINVAL;
> > > 
> > > This is not good. You may have partially applied the requested
> > > settings when you return with an error, leaving the device in some
> > > intermediate state that is most likely not useful. It'd be better
> > > to first check whether all settings can be applied, and then apply
> > > them all at once. But even then we have a problem with rolling back
> > > some changes if a command fails :/
> > >
> > 
> > Yes, we need kind of rollback here if the command failed. I also
> > stumble over this issue. [0] Mhh, we don't have it when I do each
> > command in their own CMD SET call. So then I will leave the current
> > behaviour as it is.
> > 
> > Some idea:
> > 
> > Solution would be on MAC settings that we really check if everything
> > is valid. Then we can set everything in the pib values, after an
> > interface up this values will be "really" set, like address filter
> > settings in phy registers.
> > 
> > This works for interface settings (MAC PIB values). But for phy values
> > this is more difficult, because it's directly driver-layer calls. So
> > we can't predigt if an driver layer return some error then.
> 
> That sounds good. We could add another driver function that checks a
> PIB/MIB and returns "valid" or "i'm sorry, dave", where "valid" may only
> happen when the driver *knows* that it can load the *IB into the
> hardware. Any possible hardware errors are excluded by these checks,

I think we need such functionality anyway, I wrote in my previous mail
that the not _all_ valid 802.15.4 mac pib values are supported by real
transceivers. Like the mrf24j40 [0] which can set the MIN_BE value
only (I suppose MAX_BE is always 802.15.4 default). But nl802154 checks
only on 802.15.4 constraints only, so you can set everything which is
allowed by 802.15.4. On a "doing interface up" it will fail because the
driver layer will told -EINVAL; then and you don't know why (okay,
experts knows why).

We need to check this value on the SoftMAC layer in "net/mac802154/cfg.c".

Now the design question is "a function" or "a array like
channels_supported which extends the phy pib by us". I would prefer the
array solution and put simple a u32 backoff_exponents_supported in
phy_pib [1] and checking over flags if the transceiver supports it's or not.

Then we can add more supported arrays for csma retries, frame retries,
tx_pwr (also design question if all bands or current band only), etc...

(For lbt mode, we don't need such functionality... it's only a bool and
the HW flag is enough).

Currently we don't have this kind of problem, because most settings can
set on an at86rf230 transceiver only which is fully (and beyond) 802.15.4
complaint.

Now the question is again:

It's okay to extend the "supported arrays" in phy pib or we should introduce
a driver_ops function? I prefer the array solution, because we have
already such thing which is described by 802.15.4 pib.

> but we'll only see those on the actual load anyway. Then we can load
> the *IB in bulk and be sure that only the hardware can fail.
> 

Then I would add such functionality to check if valid at first which I
describe above.

- Alex

[0] http://ww1.microchip.com/downloads/en/DeviceDoc/39776C.pdf
[1] http://lxr.free-electrons.com/source/include/net/cfg802154.h#L77

Re: [PATCHv3 bluetooth-next 2/4] nl802154: add set interface cmd

[Phoebe Buckheister]

On Tue, 7 Apr 2015 14:59:52 +0200
Alexander Aring <alex.aring@gmail.com> wrote:

> On Tue, Apr 07, 2015 at 02:29:30PM +0200, Phoebe Buckheister wrote:
> > On Tue, 7 Apr 2015 14:21:52 +0200
> > Alexander Aring <alex.aring@gmail.com> wrote:
> > 
> > > Hi Phoebe,
> > > 
> > > On Tue, Apr 07, 2015 at 01:59:02PM +0200, Phoebe Buckheister
> > > wrote:
> > > > On Tue,  7 Apr 2015 13:49:51 +0200
> > > > Alexander Aring <alex.aring@gmail.com> wrote:
> > > > 
> > > > > This patch introduce the NL802154_CMD_SET_INTERFACE command
> > > > > which handles setting of all wpan interface mac attributes.
> > > > > his will introduce an easilier wpan mac settings handling in
> > > > > userspace application with nl802154.
> > > > > 
> > > > > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > > > > ---
> > > > >  net/ieee802154/nl802154.c | 110
> > > > > ++++++++++++++++++++++++++++++++++++++++++++++ 1 file
> > > > > changed, 110 insertions(+)
> > > > > 
> > > > > diff --git a/net/ieee802154/nl802154.c
> > > > > b/net/ieee802154/nl802154.c index c12c07f..e2f50ba 100644
> > > > > --- a/net/ieee802154/nl802154.c
> > > > > +++ b/net/ieee802154/nl802154.c
> > > > > @@ -603,6 +603,108 @@ static int nl802154_get_interface(struct
> > > > > sk_buff *skb, struct genl_info *info) return
> > > > > genlmsg_reply(msg, info); }
> > > > >  
> > > > > +static int nl802154_set_interface(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;
> > > > > +	int ret;
> > > > > +
> > > > > +	if (info->attrs[NL802154_ATTR_PAN_ID]) {
> > > > > +		__le16 pan_id;
> > > > > +
> > > > > +		if (wpan_dev->iftype ==
> > > > > NL802154_IFTYPE_MONITOR)
> > > > > +			return -EINVAL;
> > > > > +
> > > > > +		if (netif_running(dev))
> > > > > +			return -EBUSY;
> > > > > +
> > > > > +		pan_id =
> > > > > nla_get_le16(info->attrs[NL802154_ATTR_PAN_ID]);
> > > > > +		ret = rdev_set_pan_id(rdev, wpan_dev,
> > > > > pan_id);
> > > > > +		if (ret < 0)
> > > > > +			return ret;
> > > > > +	}
> > > > > +
> > > > > +	if (info->attrs[NL802154_ATTR_SHORT_ADDR]) {
> > > > > +		__le16 short_addr;
> > > > > +
> > > > > +		if (wpan_dev->iftype ==
> > > > > NL802154_IFTYPE_MONITOR)
> > > > > +			return -EINVAL;
> > > > 
> > > > This is not good. You may have partially applied the requested
> > > > settings when you return with an error, leaving the device in
> > > > some intermediate state that is most likely not useful. It'd be
> > > > better to first check whether all settings can be applied, and
> > > > then apply them all at once. But even then we have a problem
> > > > with rolling back some changes if a command fails :/
> > > >
> > > 
> > > Yes, we need kind of rollback here if the command failed. I also
> > > stumble over this issue. [0] Mhh, we don't have it when I do each
> > > command in their own CMD SET call. So then I will leave the
> > > current behaviour as it is.
> > > 
> > > Some idea:
> > > 
> > > Solution would be on MAC settings that we really check if
> > > everything is valid. Then we can set everything in the pib
> > > values, after an interface up this values will be "really" set,
> > > like address filter settings in phy registers.
> > > 
> > > This works for interface settings (MAC PIB values). But for phy
> > > values this is more difficult, because it's directly driver-layer
> > > calls. So we can't predigt if an driver layer return some error
> > > then.
> > 
> > That sounds good. We could add another driver function that checks a
> > PIB/MIB and returns "valid" or "i'm sorry, dave", where "valid" may
> > only happen when the driver *knows* that it can load the *IB into
> > the hardware. Any possible hardware errors are excluded by these
> > checks,
> 
> I think we need such functionality anyway, I wrote in my previous mail
> that the not _all_ valid 802.15.4 mac pib values are supported by real
> transceivers. Like the mrf24j40 [0] which can set the MIN_BE value
> only (I suppose MAX_BE is always 802.15.4 default). But nl802154
> checks only on 802.15.4 constraints only, so you can set everything
> which is allowed by 802.15.4. On a "doing interface up" it will fail
> because the driver layer will told -EINVAL; then and you don't know
> why (okay, experts knows why).
> 
> We need to check this value on the SoftMAC layer in
> "net/mac802154/cfg.c".
> 
> Now the design question is "a function" or "a array like
> channels_supported which extends the phy pib by us". I would prefer
> the array solution and put simple a u32 backoff_exponents_supported in
> phy_pib [1] and checking over flags if the transceiver supports it's
> or not.
> 
> Then we can add more supported arrays for csma retries, frame retries,
> tx_pwr (also design question if all bands or current band only),
> etc...
> 
> (For lbt mode, we don't need such functionality... it's only a bool
> and the HW flag is enough).
> 
> Currently we don't have this kind of problem, because most settings
> can set on an at86rf230 transceiver only which is fully (and beyond)
> 802.15.4 complaint.
> 
> Now the question is again:
> 
> It's okay to extend the "supported arrays" in phy pib or we should
> introduce a driver_ops function? I prefer the array solution, because
> we have already such thing which is described by 802.15.4 pib.

Either is fine. The values permitted by the standard are mostly
bitmasks and min/max-pairs, so that actually makes more sense than a
check() function in each driver (until some driver doesn't fit that
model, but we should worry about that then).

> 
> > but we'll only see those on the actual load anyway. Then we can load
> > the *IB in bulk and be sure that only the hardware can fail.
> > 
> 
> Then I would add such functionality to check if valid at first which I
> describe above.
> 
> - Alex
> 
> [0] http://ww1.microchip.com/downloads/en/DeviceDoc/39776C.pdf
> [1] http://lxr.free-electrons.com/source/include/net/cfg802154.h#L77

Re: [PATCHv3 bluetooth-next 2/4] nl802154: add set interface cmd

[Alexander Aring]

Hi Varka,

On Tue, Apr 07, 2015 at 05:59:07PM +0530, Varka Bhadram wrote:
> On 04/07/2015 05:51 PM, Alexander Aring wrote:
...
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> Instead of this its better to leave the current behavior as it is.
> 
> Why do we need to complicate the process...?

I think more this is easier for userspace application which need to call
one CMD_SET for dev or phy only, but yes... it's much complicated to
handle error handling then. The most error codes would be -EINVAL and
-ENOTSUPP and we should forget some error codes which comming from
spi_sync/spi_async calls, these are very unlikely.

To handle the -EINVAL and -ENOTSUPP error we need more stuff to check if
the phy supports that value. I will try to introduce at first such
behaviour, then we can reconsider if we need something like that or not.

- Alex

Re: [PATCHv3 bluetooth-next 2/4] nl802154: add set interface cmd

[Alexander Aring]

On Tue, Apr 07, 2015 at 03:02:32PM +0200, Phoebe Buckheister wrote:
> On Tue, 7 Apr 2015 14:59:52 +0200
> Alexander Aring <alex.aring@gmail.com> wrote:
> 
> > On Tue, Apr 07, 2015 at 02:29:30PM +0200, Phoebe Buckheister wrote:
> > > On Tue, 7 Apr 2015 14:21:52 +0200
> > > Alexander Aring <alex.aring@gmail.com> wrote:
> > > 
> > > > Hi Phoebe,
> > > > 
> > > > On Tue, Apr 07, 2015 at 01:59:02PM +0200, Phoebe Buckheister
> > > > wrote:
> > > > > On Tue,  7 Apr 2015 13:49:51 +0200
> > > > > Alexander Aring <alex.aring@gmail.com> wrote:
> > > > > 
> > > > > > This patch introduce the NL802154_CMD_SET_INTERFACE command
> > > > > > which handles setting of all wpan interface mac attributes.
> > > > > > his will introduce an easilier wpan mac settings handling in
> > > > > > userspace application with nl802154.
> > > > > > 
> > > > > > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > > > > > ---
> > > > > >  net/ieee802154/nl802154.c | 110
> > > > > > ++++++++++++++++++++++++++++++++++++++++++++++ 1 file
> > > > > > changed, 110 insertions(+)
> > > > > > 
> > > > > > diff --git a/net/ieee802154/nl802154.c
> > > > > > b/net/ieee802154/nl802154.c index c12c07f..e2f50ba 100644
> > > > > > --- a/net/ieee802154/nl802154.c
> > > > > > +++ b/net/ieee802154/nl802154.c
> > > > > > @@ -603,6 +603,108 @@ static int nl802154_get_interface(struct
> > > > > > sk_buff *skb, struct genl_info *info) return
> > > > > > genlmsg_reply(msg, info); }
> > > > > >  
> > > > > > +static int nl802154_set_interface(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;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	if (info->attrs[NL802154_ATTR_PAN_ID]) {
> > > > > > +		__le16 pan_id;
> > > > > > +
> > > > > > +		if (wpan_dev->iftype ==
> > > > > > NL802154_IFTYPE_MONITOR)
> > > > > > +			return -EINVAL;
> > > > > > +
> > > > > > +		if (netif_running(dev))
> > > > > > +			return -EBUSY;
> > > > > > +
> > > > > > +		pan_id =
> > > > > > nla_get_le16(info->attrs[NL802154_ATTR_PAN_ID]);
> > > > > > +		ret = rdev_set_pan_id(rdev, wpan_dev,
> > > > > > pan_id);
> > > > > > +		if (ret < 0)
> > > > > > +			return ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	if (info->attrs[NL802154_ATTR_SHORT_ADDR]) {
> > > > > > +		__le16 short_addr;
> > > > > > +
> > > > > > +		if (wpan_dev->iftype ==
> > > > > > NL802154_IFTYPE_MONITOR)
> > > > > > +			return -EINVAL;
> > > > > 
> > > > > This is not good. You may have partially applied the requested
> > > > > settings when you return with an error, leaving the device in
> > > > > some intermediate state that is most likely not useful. It'd be
> > > > > better to first check whether all settings can be applied, and
> > > > > then apply them all at once. But even then we have a problem
> > > > > with rolling back some changes if a command fails :/
> > > > >
> > > > 
> > > > Yes, we need kind of rollback here if the command failed. I also
> > > > stumble over this issue. [0] Mhh, we don't have it when I do each
> > > > command in their own CMD SET call. So then I will leave the
> > > > current behaviour as it is.
> > > > 
> > > > Some idea:
> > > > 
> > > > Solution would be on MAC settings that we really check if
> > > > everything is valid. Then we can set everything in the pib
> > > > values, after an interface up this values will be "really" set,
> > > > like address filter settings in phy registers.
> > > > 
> > > > This works for interface settings (MAC PIB values). But for phy
> > > > values this is more difficult, because it's directly driver-layer
> > > > calls. So we can't predigt if an driver layer return some error
> > > > then.
> > > 
> > > That sounds good. We could add another driver function that checks a
> > > PIB/MIB and returns "valid" or "i'm sorry, dave", where "valid" may
> > > only happen when the driver *knows* that it can load the *IB into
> > > the hardware. Any possible hardware errors are excluded by these
> > > checks,
> > 
> > I think we need such functionality anyway, I wrote in my previous mail
> > that the not _all_ valid 802.15.4 mac pib values are supported by real
> > transceivers. Like the mrf24j40 [0] which can set the MIN_BE value
> > only (I suppose MAX_BE is always 802.15.4 default). But nl802154
> > checks only on 802.15.4 constraints only, so you can set everything
> > which is allowed by 802.15.4. On a "doing interface up" it will fail
> > because the driver layer will told -EINVAL; then and you don't know
> > why (okay, experts knows why).
> > 
> > We need to check this value on the SoftMAC layer in
> > "net/mac802154/cfg.c".
> > 
> > Now the design question is "a function" or "a array like
> > channels_supported which extends the phy pib by us". I would prefer
> > the array solution and put simple a u32 backoff_exponents_supported in
> > phy_pib [1] and checking over flags if the transceiver supports it's
> > or not.
> > 
> > Then we can add more supported arrays for csma retries, frame retries,
> > tx_pwr (also design question if all bands or current band only),
> > etc...
> > 
> > (For lbt mode, we don't need such functionality... it's only a bool
> > and the HW flag is enough).
> > 
> > Currently we don't have this kind of problem, because most settings
> > can set on an at86rf230 transceiver only which is fully (and beyond)
> > 802.15.4 complaint.
> > 
> > Now the question is again:
> > 
> > It's okay to extend the "supported arrays" in phy pib or we should
> > introduce a driver_ops function? I prefer the array solution, because
> > we have already such thing which is described by 802.15.4 pib.
> 
> Either is fine. The values permitted by the standard are mostly
> bitmasks and min/max-pairs, so that actually makes more sense than a

So we could some union type for it like:

union wpan_phy_support {
	u32	mask;
	union {
		u16	min:16;
		u16	max:16;
	};
};

Don't know if this is good idea.

> check() function in each driver (until some driver doesn't fit that
> model, but we should worry about that then).
> 

ack.

- Alex

Re: [PATCHv3 bluetooth-next 2/4] nl802154: add set interface cmd

[Phoebe Buckheister]

On Tue, 7 Apr 2015 15:25:02 +0200
Alexander Aring <alex.aring@gmail.com> wrote:

> On Tue, Apr 07, 2015 at 03:02:32PM +0200, Phoebe Buckheister wrote:
> > On Tue, 7 Apr 2015 14:59:52 +0200
> > Alexander Aring <alex.aring@gmail.com> wrote:
> > 
> > > On Tue, Apr 07, 2015 at 02:29:30PM +0200, Phoebe Buckheister
> > > wrote:
> > > > On Tue, 7 Apr 2015 14:21:52 +0200
> > > > Alexander Aring <alex.aring@gmail.com> wrote:
> > > > 
> > > > > Hi Phoebe,
> > > > > 
> > > > > On Tue, Apr 07, 2015 at 01:59:02PM +0200, Phoebe Buckheister
> > > > > wrote:
> > > > > > On Tue,  7 Apr 2015 13:49:51 +0200
> > > > > > Alexander Aring <alex.aring@gmail.com> wrote:
> > > > > > 
> > > > > > > This patch introduce the NL802154_CMD_SET_INTERFACE
> > > > > > > command which handles setting of all wpan interface mac
> > > > > > > attributes. his will introduce an easilier wpan mac
> > > > > > > settings handling in userspace application with nl802154.
> > > > > > > 
> > > > > > > Signed-off-by: Alexander Aring <alex.aring@gmail.com>
> > > > > > > ---
> > > > > > >  net/ieee802154/nl802154.c | 110
> > > > > > > ++++++++++++++++++++++++++++++++++++++++++++++ 1 file
> > > > > > > changed, 110 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/net/ieee802154/nl802154.c
> > > > > > > b/net/ieee802154/nl802154.c index c12c07f..e2f50ba 100644
> > > > > > > --- a/net/ieee802154/nl802154.c
> > > > > > > +++ b/net/ieee802154/nl802154.c
> > > > > > > @@ -603,6 +603,108 @@ static int
> > > > > > > nl802154_get_interface(struct sk_buff *skb, struct
> > > > > > > genl_info *info) return genlmsg_reply(msg, info); }
> > > > > > >  
> > > > > > > +static int nl802154_set_interface(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;
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	if (info->attrs[NL802154_ATTR_PAN_ID]) {
> > > > > > > +		__le16 pan_id;
> > > > > > > +
> > > > > > > +		if (wpan_dev->iftype ==
> > > > > > > NL802154_IFTYPE_MONITOR)
> > > > > > > +			return -EINVAL;
> > > > > > > +
> > > > > > > +		if (netif_running(dev))
> > > > > > > +			return -EBUSY;
> > > > > > > +
> > > > > > > +		pan_id =
> > > > > > > nla_get_le16(info->attrs[NL802154_ATTR_PAN_ID]);
> > > > > > > +		ret = rdev_set_pan_id(rdev, wpan_dev,
> > > > > > > pan_id);
> > > > > > > +		if (ret < 0)
> > > > > > > +			return ret;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	if (info->attrs[NL802154_ATTR_SHORT_ADDR]) {
> > > > > > > +		__le16 short_addr;
> > > > > > > +
> > > > > > > +		if (wpan_dev->iftype ==
> > > > > > > NL802154_IFTYPE_MONITOR)
> > > > > > > +			return -EINVAL;
> > > > > > 
> > > > > > This is not good. You may have partially applied the
> > > > > > requested settings when you return with an error, leaving
> > > > > > the device in some intermediate state that is most likely
> > > > > > not useful. It'd be better to first check whether all
> > > > > > settings can be applied, and then apply them all at once.
> > > > > > But even then we have a problem with rolling back some
> > > > > > changes if a command fails :/
> > > > > >
> > > > > 
> > > > > Yes, we need kind of rollback here if the command failed. I
> > > > > also stumble over this issue. [0] Mhh, we don't have it when
> > > > > I do each command in their own CMD SET call. So then I will
> > > > > leave the current behaviour as it is.
> > > > > 
> > > > > Some idea:
> > > > > 
> > > > > Solution would be on MAC settings that we really check if
> > > > > everything is valid. Then we can set everything in the pib
> > > > > values, after an interface up this values will be "really"
> > > > > set, like address filter settings in phy registers.
> > > > > 
> > > > > This works for interface settings (MAC PIB values). But for
> > > > > phy values this is more difficult, because it's directly
> > > > > driver-layer calls. So we can't predigt if an driver layer
> > > > > return some error then.
> > > > 
> > > > That sounds good. We could add another driver function that
> > > > checks a PIB/MIB and returns "valid" or "i'm sorry, dave",
> > > > where "valid" may only happen when the driver *knows* that it
> > > > can load the *IB into the hardware. Any possible hardware
> > > > errors are excluded by these checks,
> > > 
> > > I think we need such functionality anyway, I wrote in my previous
> > > mail that the not _all_ valid 802.15.4 mac pib values are
> > > supported by real transceivers. Like the mrf24j40 [0] which can
> > > set the MIN_BE value only (I suppose MAX_BE is always 802.15.4
> > > default). But nl802154 checks only on 802.15.4 constraints only,
> > > so you can set everything which is allowed by 802.15.4. On a
> > > "doing interface up" it will fail because the driver layer will
> > > told -EINVAL; then and you don't know why (okay, experts knows
> > > why).
> > > 
> > > We need to check this value on the SoftMAC layer in
> > > "net/mac802154/cfg.c".
> > > 
> > > Now the design question is "a function" or "a array like
> > > channels_supported which extends the phy pib by us". I would
> > > prefer the array solution and put simple a u32
> > > backoff_exponents_supported in phy_pib [1] and checking over
> > > flags if the transceiver supports it's or not.
> > > 
> > > Then we can add more supported arrays for csma retries, frame
> > > retries, tx_pwr (also design question if all bands or current
> > > band only), etc...
> > > 
> > > (For lbt mode, we don't need such functionality... it's only a
> > > bool and the HW flag is enough).
> > > 
> > > Currently we don't have this kind of problem, because most
> > > settings can set on an at86rf230 transceiver only which is fully
> > > (and beyond) 802.15.4 complaint.
> > > 
> > > Now the question is again:
> > > 
> > > It's okay to extend the "supported arrays" in phy pib or we should
> > > introduce a driver_ops function? I prefer the array solution,
> > > because we have already such thing which is described by 802.15.4
> > > pib.
> > 
> > Either is fine. The values permitted by the standard are mostly
> > bitmasks and min/max-pairs, so that actually makes more sense than a
> 
> So we could some union type for it like:
> 
> union wpan_phy_support {
> 	u32	mask;
> 	union {
> 		u16	min:16;
> 		u16	max:16;
> 	};
> };
> 
> Don't know if this is good idea.

Eh? No.

struct wpan_phy_support {
	bool lbt, aack;
	u16 min_be, max_be;
	u32 pages;
	u32 channels[32];
};

or something like that. A complete description of PHY capabilities, we
might even export that to userspace somehow?

> 
> > check() function in each driver (until some driver doesn't fit that
> > model, but we should worry about that then).
> > 
> 
> ack.
> 
> - Alex

Re: [PATCHv3 bluetooth-next 2/4] nl802154: add set interface cmd

[Alexander Aring]

On Tue, Apr 07, 2015 at 03:32:13PM +0200, Phoebe Buckheister wrote:
> 
> Eh? No.
> 

Okay. :-)

> struct wpan_phy_support {
> 	bool lbt, aack;

for aack I would use max and min for frame_retries (need to be signed
integer) and the same logik like max_frame_retries parameter in driver
ops. So if s8 max_frame_retries and min_frame_retries -1 then we don't
support aack handling. 

> 	u16 min_be, max_be;

I think for the backoff exponents we need something like:

min_minbe and max_minbe

and

min_maxbe and max_maxbe

sounds a little bit ugly, but some transceivers can't change the maxbe
value.

> 	u32 pages;
> 	u32 channels[32];

This is already combined in channels_supported array for pages and channels.

> };
> 
> or something like that. A complete description of PHY capabilities, we
> might even export that to userspace somehow?
> 

yes, of course. I think we should implement then a nested nl attribute for
wpan_phy_supported things while wpan_phy dump.

- Alex