Re: [PATCH bluetooth-next 1/3] ieee802154: add set transmit power support

[Alexander Aring <alex.aring@gmail.com>]

Hi,

there exist _maybe_ some general issues with the current interface and the
tx_power handling:

1.
   The at86rf2xx has also settings for a more fine granularity tx power
   setting. For example the at86rf233 has these values:
     - 4 dBm
     - 3.7 dBm
     - 3.4 dBm
     - 3 dBm
     - 2.5 dBm
     - 2 dBm
     ...
   I don't know if we should simple "round-down" and use the nearest
   value in this case. For the moment I am fine to handle the nearest value.

2.
   It seems that, for example the at86rf212 [1] which operates in
   700, 800 or 900 Mhz, the tx_power setting depends on the current setting of
   page and channel. I believe we can completely handle this inside the
   driver layer, but I am not 100% sure.

btw:
   What I am know is that the set_txpower driver callback in at86rf230
   need to decide if at86rf233, at86rf231 or at86rf212 and making some
   special handling. Especially the at86rf212 needs to check the current
   page and channel setting, because the register values differs here.
   Current behaviour is broken.


On Fri, Mar 20, 2015 at 12:52:18PM +0530, Varka Bhadram wrote:
> This patch adds transmission power setting support to nl802154.
> 
> Signed-off-by: Varka Bhadram <varkab@cdac.in>
> ---
>  include/net/cfg802154.h   |    1 +
>  net/ieee802154/nl802154.c |   20 ++++++++++++++++++++
>  net/ieee802154/rdev-ops.h |    7 +++++++
>  net/mac802154/cfg.c       |   19 +++++++++++++++++++
>  4 files changed, 47 insertions(+)
> 
> diff --git a/include/net/cfg802154.h b/include/net/cfg802154.h
> index eeda676..b163d4e 100644
> --- a/include/net/cfg802154.h
> +++ b/include/net/cfg802154.h
> @@ -57,6 +57,7 @@ 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_tx_power)(struct wpan_phy *wpan_phy, s8 power);

put this into the sequence of the others phy settings like channel,
page. cca. etc...

>  };
>  
>  struct wpan_phy_cca {
> diff --git a/net/ieee802154/nl802154.c b/net/ieee802154/nl802154.c
> index a4daf91..8288fcb 100644
> --- a/net/ieee802154/nl802154.c
> +++ b/net/ieee802154/nl802154.c
> @@ -794,6 +794,18 @@ 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_tx_power(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct cfg802154_registered_device *rdev = info->user_ptr[0];
> +	s8 power;
> +
> +	if (!info->attrs[NL802154_ATTR_TX_POWER])
> +		return -EINVAL;
> +
> +	power = nla_get_s8(info->attrs[NL802154_ATTR_TX_POWER]);
> +	return rdev_set_tx_power(rdev, power);
> +}
> +
>  #define NL802154_FLAG_NEED_WPAN_PHY	0x01
>  #define NL802154_FLAG_NEED_NETDEV	0x02
>  #define NL802154_FLAG_NEED_RTNL		0x04
> @@ -984,6 +996,14 @@ static const struct genl_ops nl802154_ops[] = {
>  		.internal_flags = NL802154_FLAG_NEED_NETDEV |
>  				  NL802154_FLAG_NEED_RTNL,
>  	},
> +	{
> +		.cmd = NL802154_CMD_SET_TX_POWER,
> +		.doit = nl802154_set_tx_power,
> +		.policy = nl802154_policy,
> +		.flags = GENL_ADMIN_PERM,
> +		.internal_flags = NL802154_FLAG_NEED_WPAN_PHY |
> +				  NL802154_FLAG_NEED_RTNL,
> +	},
>  };
>  

I posted ~4 days ago [0] a series which makes this kind of set cmd
obsolete. We should not introduce new commands if we decide to set phy
settings with only one cmd only. I will create a new thread about which
are the advantages and disadvantage about the new setting commands, then
we can decide there if we still use the old interface or switch to the
new behaviour. If we decide to use the new interface then you can rebase
your work on it.

>  /* initialisation/exit functions */
> diff --git a/net/ieee802154/rdev-ops.h b/net/ieee802154/rdev-ops.h
> index 7c46732..3de05a8 100644
> --- a/net/ieee802154/rdev-ops.h
> +++ b/net/ieee802154/rdev-ops.h
> @@ -91,6 +91,13 @@ rdev_set_lbt_mode(struct cfg802154_registered_device *rdev,
>  		  struct wpan_dev *wpan_dev, bool mode)
>  {
>  	return rdev->ops->set_lbt_mode(&rdev->wpan_phy, wpan_dev, mode);
> +

remove this whitespace.

>  }
>  
> +static inline int
> +rdev_set_tx_power(struct cfg802154_registered_device *rdev,
> +		  u8 power)

s/u8/s8/

> +{
> +	return rdev->ops->set_tx_power(&rdev->wpan_phy, power);
> +}

also move this to the other phy rdev-ops wrappers.

>  #endif /* __CFG802154_RDEV_OPS */
> diff --git a/net/mac802154/cfg.c b/net/mac802154/cfg.c
> index 5d9f68c..dde26f1 100644
> --- a/net/mac802154/cfg.c
> +++ b/net/mac802154/cfg.c
> @@ -212,6 +212,24 @@ ieee802154_set_lbt_mode(struct wpan_phy *wpan_phy, struct wpan_dev *wpan_dev,
>  	return 0;
>  }
>  
> +static int
> +ieee802154_set_tx_power(struct wpan_phy *wpan_phy, s8 power)
> +{
> +	struct ieee802154_local *local = wpan_phy_priv(wpan_phy);
> +	int ret;
> +
> +	ASSERT_RTNL();
> +
> +	if (!(local->hw.flags & IEEE802154_HW_TXPOWER))
> +		return -EOPNOTSUPP;
> +
> +	ret = drv_set_tx_power(local, power);
> +	if (!ret)
> +		wpan_phy->transmit_power = power;
> +
> +	return ret;
> +}

also move this to the other phy handlers.

> +
>  const struct cfg802154_ops mac802154_config_ops = {
>  	.add_virtual_intf_deprecated = ieee802154_add_iface_deprecated,
>  	.del_virtual_intf_deprecated = ieee802154_del_iface_deprecated,
> @@ -225,4 +243,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_tx_power = ieee802154_set_tx_power,

same here.

- Alex

[0] http://www.spinics.net/lists/linux-wpan/msg01550.html
[1] http://www.atmel.com/images/doc8168.pdf