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

[Stefan Schmidt <stefan@osg.samsung.com>]

Hello.

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

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

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

regards
Stefan Schmidt