Re: [PATCH V7 1/3] cfg80211: introduce critical protocol indication from user-space

["Arend van Spriel" <arend@broadcom.com>]

On 05/23/2013 08:30 PM, Dan Williams wrote:
> On Thu, 2013-04-18 at 15:49 +0200, Arend van Spriel wrote:
>> Some protocols need a more reliable connection to complete
>> successful in reasonable time. This patch adds a user-space
>> API to indicate the wireless driver that a critical protocol
>> is about to commence and when it is done, using nl80211 primitives
>> NL80211_CMD_CRIT_PROTOCOL_START and NL80211_CRIT_PROTOCOL_STOP.
>>
>> There can be only on critical protocol session started per
>> registered cfg80211 device.
>
> Ok, so while implementing support for this in NetworkManager, I ran into
> a few questions some issues.

Hi Dan,

Thanks for your feedback.
> 1) Why have a new attribute?  Why not just use NL80211_ATTR_DURATION
> like all the other commands do?

I guess it was overlooked. The only difference is that this attribute is 
u32. I have not problem changing it.

> 2) Why have a restriction on a single critical protocol at a time?  Even
> if this is the case *now*, just for sake of time, we should pass the
> protocol to the _STOP command to allow for multiples in the future.
>
> Yeah, you won't have EAPOL running at the same time as DHCP, but think
> about it from userspace's perspective:
>
> a) process A starts critical protocol like EAPOL
> b) process A forgets to stop critical protocol
> c) process B starts critical protocol DHCP, oops, error!
> d) process B has to clear old critical protocol
> e) process B starts critical protocol DHCP
> f) process A realizes it forgot (b) and stops protocol
>
> I think there's a lot of opportunity for races here.  This would at
> least be reduced if the START/STOP commands were paired for a specific
> protocol, and if something requested a STOP for a protocol that's
> currently not started, it was rejected.

I guess you are right if the processes mentioned above share the same 
netlink socket. That is why the netlink portid is used as a flag that a 
critical protocol has been started. Just scrolled down and found we are 
not checking the portid in nl80211_crit_proto_stop(). That needs to be 
fixed or .... we should reconsider based on your feedback. I am not 
fully convinced there will be a need for multiple protocols.

> Better yet, why not just have an internal array of all the protocols
> with their max duration and start time, and the stack manages when each
> protocol gets stopped?  (unless you think drivers will have different
> behavior on a per-protocol basis, eg they'd do something different with
> DHCP than with EAPOL...?)

Not sure if I understand. Does 'start time' mean a fixed time after link 
being established. The whole idea was that user-space tools would know 
when a protocol would complete. I do not see how putting static numbers 
into an array would be 'better yet'.

Regards,
Arend

>> The driver can support this by implementing the cfg80211 callbacks
>> .crit_proto_start() and .crit_proto_stop(). Examples of protocols
>> that can benefit from this are DHCP, EAPOL, APIPA. Exactly how the
>> link can/should be made more reliable is up to the driver. Things
>> to consider are avoid scanning, no multi-channel operations, and
>> alter coexistence schemes.
>>
>> Reviewed-by: Pieter-Paul Giesberts <pieterpg@broadcom.com>
>> Reviewed-by: Franky (Zhenhui) Lin <frankyl@broadcom.com>
>> Signed-off-by: Arend van Spriel <arend@broadcom.com>
>> ---
>> Hi Johannes,
>>
>> Not sure whether you made the minor fixes already as you offered
>> to do so. I saw the change pop-up in your repo a couple of nights
>> ago, but it disappeared the next morning after another fetch.
>>
>> I also made related changes in our brcmfmac driver. Can you take
>> them through your tree as well.
>>
>> Regards,
>> Arend
>>
>> Changelog:
>> ----------
>> V7:
>> - remove crit_proto_started field from wireless_dev.
>> - remove CRIT_PROTO_STOPPED_EVENT definition.
>> - allow always calling cfg80211_crit_proto_stopped().
>> V6:
>> - added crit_proto_stopped event message.
>> - use nlportid as flag for critical protocol being active.
>> - return error when duration is over specified limit.
>> - remove logic from rdev_* inline wrappers.
>> - added more documentation.
>> - some renaming of identifiers.
>> V5:
>> - change return type for .crit_prot_stop() to void.
>> - correct limiting the duration.
>> V4:
>> - added cfg80211_crit_proto_stopped() for drivers to use.
>> - added back protocol identifier for drivers to use.
>> - reject starting critical protocol session when already started.
>> - critical protocol session tracked per registered device.
>> V3:
>> - remove protocol identifier.
>> - remove delayed work from cfg80211.
>> - guard maximum limit for duration.
>> - do .crit_proto_stop() upon netlink socket release.
>> V2:
>> - subject changed. Below previous subject is given for reference:
>>    [RFC] cfg80211: configuration of Bluetooth coexistence mode
>> - introduced dedicated nl80211 API.
>> V1:
>> - initial proposal.
>> ---
>>   include/net/cfg80211.h       |   23 +++++++++
>>   include/uapi/linux/nl80211.h |   39 ++++++++++++++
>>   net/wireless/core.h          |    3 ++
>>   net/wireless/mlme.c          |    5 ++
>>   net/wireless/nl80211.c       |  117 ++++++++++++++++++++++++++++++++++++++++++
>>   net/wireless/rdev-ops.h      |   24 ++++++++-
>>   net/wireless/trace.h         |   35 +++++++++++++
>>   7 files changed, 245 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
>> index dff96d8..26b5b69 100644
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
>> @@ -2002,6 +2002,12 @@ struct cfg80211_update_ft_ies_params {
>>    * @update_ft_ies: Provide updated Fast BSS Transition information to the
>>    *	driver. If the SME is in the driver/firmware, this information can be
>>    *	used in building Authentication and Reassociation Request frames.
>> + *
>> + * @crit_proto_start: Indicates a critical protocol needs more link reliability
>> + *	for a given duration (milliseconds). The protocol is provided so the
>> + *	driver can take the most appropriate actions.
>> + * @crit_proto_stop: Indicates critical protocol no longer needs increased link
>> + *	reliability. This operation can not fail.
>>    */
>>   struct cfg80211_ops {
>>   	int	(*suspend)(struct wiphy *wiphy, struct cfg80211_wowlan *wow);
>> @@ -2231,6 +2237,12 @@ struct cfg80211_ops {
>>   					 struct cfg80211_chan_def *chandef);
>>   	int	(*update_ft_ies)(struct wiphy *wiphy, struct net_device *dev,
>>   				 struct cfg80211_update_ft_ies_params *ftie);
>> +	int	(*crit_proto_start)(struct wiphy *wiphy,
>> +				    struct wireless_dev *wdev,
>> +				    enum nl80211_crit_proto_id protocol,
>> +				    u16 duration);
>> +	void	(*crit_proto_stop)(struct wiphy *wiphy,
>> +				   struct wireless_dev *wdev);
>>   };
>>
>>   /*
>> @@ -4137,6 +4149,17 @@ void cfg80211_report_wowlan_wakeup(struct wireless_dev *wdev,
>>   				   struct cfg80211_wowlan_wakeup *wakeup,
>>   				   gfp_t gfp);
>>
>> +/**
>> + * cfg80211_crit_proto_stopped() - indicate critical protocol stopped by driver.
>> + *
>> + * @wdev: the wireless device for which critical protocol is stopped.
>> + *
>> + * This function can be called by the driver to indicate it has reverted
>> + * operation back to normal. One reason could be that the duration given
>> + * by .crit_proto_start() has expired.
>> + */
>> +void cfg80211_crit_proto_stopped(struct wireless_dev *wdev, gfp_t gfp);
>> +
>>   /* Logging, debugging and troubleshooting/diagnostic helpers. */
>>
>>   /* wiphy_printk helpers, similar to dev_printk */
>> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
>> index 79da871..d1e48b5 100644
>> --- a/include/uapi/linux/nl80211.h
>> +++ b/include/uapi/linux/nl80211.h
>> @@ -639,6 +639,13 @@
>>    *	with the relevant Information Elements. This event is used to report
>>    *	received FT IEs (MDIE, FTIE, RSN IE, TIE, RICIE).
>>    *
>> + * @NL80211_CMD_CRIT_PROTOCOL_START: Indicates user-space will start running
>> + *	a critical protocol that needs more reliability in the connection to
>> + *	complete.
>> + *
>> + * @NL80211_CMD_CRIT_PROTOCOL_STOP: Indicates the connection reliability can
>> + *	return back to normal.
>> + *
>>    * @NL80211_CMD_MAX: highest used command number
>>    * @__NL80211_CMD_AFTER_LAST: internal use
>>    */
>> @@ -798,6 +805,9 @@ enum nl80211_commands {
>>   	NL80211_CMD_UPDATE_FT_IES,
>>   	NL80211_CMD_FT_EVENT,
>>
>> +	NL80211_CMD_CRIT_PROTOCOL_START,
>> +	NL80211_CMD_CRIT_PROTOCOL_STOP,
>> +
>>   	/* add new commands above here */
>>
>>   	/* used to define NL80211_CMD_MAX below */
>> @@ -1414,6 +1424,11 @@ enum nl80211_commands {
>>    * @NL80211_ATTR_IE_RIC: Resource Information Container Information
>>    *	Element
>>    *
>> + * @NL80211_ATTR_CRIT_PROT_ID: critical protocol identifier requiring increased
>> + *	reliability, see &enum nl80211_crit_proto_id (u16).
>> + * @NL80211_ATTR_MAX_CRIT_PROT_DURATION: duration in milliseconds in which
>> + *      the connection should have increased reliability (u16).
>> + *
>>    * @NL80211_ATTR_MAX: highest attribute number currently defined
>>    * @__NL80211_ATTR_AFTER_LAST: internal use
>>    */
>> @@ -1709,6 +1724,9 @@ enum nl80211_attrs {
>>   	NL80211_ATTR_MDID,
>>   	NL80211_ATTR_IE_RIC,
>>
>> +	NL80211_ATTR_CRIT_PROT_ID,
>> +	NL80211_ATTR_MAX_CRIT_PROT_DURATION,
>> +
>>   	/* add attributes here, update the policy in nl80211.c */
>>
>>   	__NL80211_ATTR_AFTER_LAST,
>> @@ -3682,4 +3700,25 @@ enum nl80211_protocol_features {
>>   	NL80211_PROTOCOL_FEATURE_SPLIT_WIPHY_DUMP =	1 << 0,
>>   };
>>
>> +/**
>> + * enum nl80211_crit_proto_id - nl80211 critical protocol identifiers
>> + *
>> + * @NL80211_CRIT_PROTO_UNSPEC: protocol unspecified.
>> + * @NL80211_CRIT_PROTO_DHCP: BOOTP or DHCPv6 protocol.
>> + * @NL80211_CRIT_PROTO_EAPOL: EAPOL protocol.
>> + * @NL80211_CRIT_PROTO_APIPA: APIPA protocol.
>> + * @NUM_NL80211_CRIT_PROTO: must be kept last.
>> + */
>> +enum nl80211_crit_proto_id {
>> +	NL80211_CRIT_PROTO_UNSPEC,
>> +	NL80211_CRIT_PROTO_DHCP,
>> +	NL80211_CRIT_PROTO_EAPOL,
>> +	NL80211_CRIT_PROTO_APIPA,
>> +	/* add other protocols before this one */
>> +	NUM_NL80211_CRIT_PROTO
>> +};
>> +
>> +/* maximum duration for critical protocol measures */
>> +#define NL80211_CRIT_PROTO_MAX_DURATION		5000 /* msec */
>> +
>>   #endif /* __LINUX_NL80211_H */
>> diff --git a/net/wireless/core.h b/net/wireless/core.h
>> index d5d06fd..eac5308 100644
>> --- a/net/wireless/core.h
>> +++ b/net/wireless/core.h
>> @@ -88,6 +88,9 @@ struct cfg80211_registered_device {
>>
>>   	struct delayed_work dfs_update_channels_wk;
>>
>> +	/* netlink port which started critical protocol (0 means not started) */
>> +	u32 crit_proto_nlportid;
>> +
>>   	/* must be last because of the way we do wiphy_priv(),
>>   	 * and it should at least be aligned to NETDEV_ALIGN */
>>   	struct wiphy wiphy __aligned(NETDEV_ALIGN);
>> diff --git a/net/wireless/mlme.c b/net/wireless/mlme.c
>> index 390198b..0c7b7dd 100644
>> --- a/net/wireless/mlme.c
>> +++ b/net/wireless/mlme.c
>> @@ -648,6 +648,11 @@ void cfg80211_mlme_unregister_socket(struct wireless_dev *wdev, u32 nlportid)
>>
>>   	spin_unlock_bh(&wdev->mgmt_registrations_lock);
>>
>> +	if (nlportid && rdev->crit_proto_nlportid == nlportid) {
>> +		rdev->crit_proto_nlportid = 0;
>> +		rdev_crit_proto_stop(rdev, wdev);
>> +	}
>> +
>>   	if (nlportid == wdev->ap_unexpected_nlportid)
>>   		wdev->ap_unexpected_nlportid = 0;
>>   }
>> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
>> index f924d45..96ba1eb 100644
>> --- a/net/wireless/nl80211.c
>> +++ b/net/wireless/nl80211.c
>> @@ -1417,6 +1417,10 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *dev,
>>   		}
>>   		CMD(start_p2p_device, START_P2P_DEVICE);
>>   		CMD(set_mcast_rate, SET_MCAST_RATE);
>> +		if (split) {
>> +			CMD(crit_proto_start, CRIT_PROTOCOL_START);
>> +			CMD(crit_proto_stop, CRIT_PROTOCOL_STOP);
>> +		}
>>
>>   #ifdef CONFIG_NL80211_TESTMODE
>>   		CMD(testmode_cmd, TESTMODE);
>> @@ -8196,6 +8200,64 @@ static int nl80211_update_ft_ies(struct sk_buff *skb, struct genl_info *info)
>>   	return rdev_update_ft_ies(rdev, dev, &ft_params);
>>   }
>>
>> +static int nl80211_crit_protocol_start(struct sk_buff *skb,
>> +				       struct genl_info *info)
>> +{
>> +	struct cfg80211_registered_device *rdev = info->user_ptr[0];
>> +	struct wireless_dev *wdev = info->user_ptr[1];
>> +	enum nl80211_crit_proto_id proto = NL80211_CRIT_PROTO_UNSPEC;
>> +	u16 duration;
>> +	int ret;
>> +
>> +	if (!rdev->ops->crit_proto_start)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (WARN_ON(!rdev->ops->crit_proto_stop))
>> +		return -EINVAL;
>> +
>> +	if (rdev->crit_proto_nlportid)
>> +		return -EBUSY;
>> +
>> +	/* determine protocol if provided */
>> +	if (info->attrs[NL80211_ATTR_CRIT_PROT_ID])
>> +		proto = nla_get_u16(info->attrs[NL80211_ATTR_CRIT_PROT_ID]);
>> +
>> +	if (proto >= NUM_NL80211_CRIT_PROTO)
>> +		return -EINVAL;
>> +
>> +	/* timeout must be provided */
>> +	if (!info->attrs[NL80211_ATTR_MAX_CRIT_PROT_DURATION])
>> +		return -EINVAL;
>> +
>> +	duration =
>> +		nla_get_u16(info->attrs[NL80211_ATTR_MAX_CRIT_PROT_DURATION]);
>> +
>> +	if (duration > NL80211_CRIT_PROTO_MAX_DURATION)
>> +		return -ERANGE;
>> +
>> +	ret = rdev_crit_proto_start(rdev, wdev, proto, duration);
>> +	if (!ret)
>> +		rdev->crit_proto_nlportid = info->snd_portid;
>> +
>> +	return ret;
>> +}
>> +
>> +static int nl80211_crit_protocol_stop(struct sk_buff *skb,
>> +				      struct genl_info *info)
>> +{
>> +	struct cfg80211_registered_device *rdev = info->user_ptr[0];
>> +	struct wireless_dev *wdev = info->user_ptr[1];
>> +
>> +	if (!rdev->ops->crit_proto_stop)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (rdev->crit_proto_nlportid) {
>> +		rdev->crit_proto_nlportid = 0;
>> +		rdev_crit_proto_stop(rdev, wdev);
>> +	}
>> +	return 0;
>> +}
>> +
>>   #define NL80211_FLAG_NEED_WIPHY		0x01
>>   #define NL80211_FLAG_NEED_NETDEV	0x02
>>   #define NL80211_FLAG_NEED_RTNL		0x04
>> @@ -8885,6 +8947,22 @@ static struct genl_ops nl80211_ops[] = {
>>   		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
>>   				  NL80211_FLAG_NEED_RTNL,
>>   	},
>> +	{
>> +		.cmd = NL80211_CMD_CRIT_PROTOCOL_START,
>> +		.doit = nl80211_crit_protocol_start,
>> +		.policy = nl80211_policy,
>> +		.flags = GENL_ADMIN_PERM,
>> +		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
>> +				  NL80211_FLAG_NEED_RTNL,
>> +	},
>> +	{
>> +		.cmd = NL80211_CMD_CRIT_PROTOCOL_STOP,
>> +		.doit = nl80211_crit_protocol_stop,
>> +		.policy = nl80211_policy,
>> +		.flags = GENL_ADMIN_PERM,
>> +		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
>> +				  NL80211_FLAG_NEED_RTNL,
>> +	}
>>   };
>>
>>   static struct genl_multicast_group nl80211_mlme_mcgrp = {
>> @@ -10630,6 +10708,45 @@ void cfg80211_ft_event(struct net_device *netdev,
>>   }
>>   EXPORT_SYMBOL(cfg80211_ft_event);
>>
>> +void cfg80211_crit_proto_stopped(struct wireless_dev *wdev, gfp_t gfp)
>> +{
>> +	struct cfg80211_registered_device *rdev;
>> +	struct sk_buff *msg;
>> +	void *hdr;
>> +	u32 nlportid;
>> +
>> +	rdev = wiphy_to_dev(wdev->wiphy);
>> +	if (!rdev->crit_proto_nlportid)
>> +		return;
>> +
>> +	nlportid = rdev->crit_proto_nlportid;
>> +	rdev->crit_proto_nlportid = 0;
>> +
>> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
>> +	if (!msg)
>> +		return;
>> +
>> +	hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_CRIT_PROTOCOL_STOP);
>> +	if (!hdr)
>> +		goto nla_put_failure;
>> +
>> +	if (nla_put_u32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx) ||
>> +	    nla_put_u64(msg, NL80211_ATTR_WDEV, wdev_id(wdev)))
>> +		goto nla_put_failure;
>> +
>> +	genlmsg_end(msg, hdr);
>> +
>> +	genlmsg_unicast(wiphy_net(&rdev->wiphy), msg, nlportid);
>> +	return;
>> +
>> + nla_put_failure:
>> +	if (hdr)
>> +		genlmsg_cancel(msg, hdr);
>> +	nlmsg_free(msg);
>> +
>> +}
>> +EXPORT_SYMBOL(cfg80211_crit_proto_stopped);
>> +
>>   /* initialisation/exit functions */
>>
>>   int nl80211_init(void)
>> diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
>> index d77e1c1..9f15f0a 100644
>> --- a/net/wireless/rdev-ops.h
>> +++ b/net/wireless/rdev-ops.h
>> @@ -875,7 +875,7 @@ static inline void rdev_stop_p2p_device(struct cfg80211_registered_device *rdev,
>>   	trace_rdev_stop_p2p_device(&rdev->wiphy, wdev);
>>   	rdev->ops->stop_p2p_device(&rdev->wiphy, wdev);
>>   	trace_rdev_return_void(&rdev->wiphy);
>> -}					
>> +}
>>
>>   static inline int rdev_set_mac_acl(struct cfg80211_registered_device *rdev,
>>   				   struct net_device *dev,
>> @@ -901,4 +901,26 @@ static inline int rdev_update_ft_ies(struct cfg80211_registered_device *rdev,
>>   	return ret;
>>   }
>>
>> +static inline int rdev_crit_proto_start(struct cfg80211_registered_device *rdev,
>> +					struct wireless_dev *wdev,
>> +					enum nl80211_crit_proto_id protocol,
>> +					u16 duration)
>> +{
>> +	int ret;
>> +
>> +	trace_rdev_crit_proto_start(&rdev->wiphy, wdev, protocol, duration);
>> +	ret = rdev->ops->crit_proto_start(&rdev->wiphy, wdev,
>> +					  protocol, duration);
>> +	trace_rdev_return_int(&rdev->wiphy, ret);
>> +	return ret;
>> +}
>> +
>> +static inline void rdev_crit_proto_stop(struct cfg80211_registered_device *rdev,
>> +				       struct wireless_dev *wdev)
>> +{
>> +	trace_rdev_crit_proto_stop(&rdev->wiphy, wdev);
>> +	rdev->ops->crit_proto_stop(&rdev->wiphy, wdev);
>> +	trace_rdev_return_void(&rdev->wiphy);
>> +}
>> +
>>   #endif /* __CFG80211_RDEV_OPS */
>> diff --git a/net/wireless/trace.h b/net/wireless/trace.h
>> index ccadef2..499c982 100644
>> --- a/net/wireless/trace.h
>> +++ b/net/wireless/trace.h
>> @@ -1805,6 +1805,41 @@ TRACE_EVENT(rdev_update_ft_ies,
>>   		  WIPHY_PR_ARG, NETDEV_PR_ARG, __entry->md)
>>   );
>>
>> +TRACE_EVENT(rdev_crit_proto_start,
>> +	TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev,
>> +		 enum nl80211_crit_proto_id protocol, u16 duration),
>> +	TP_ARGS(wiphy, wdev, protocol, duration),
>> +	TP_STRUCT__entry(
>> +		WIPHY_ENTRY
>> +		WDEV_ENTRY
>> +		__field(u16, proto)
>> +		__field(u16, duration)
>> +	),
>> +	TP_fast_assign(
>> +		WIPHY_ASSIGN;
>> +		WDEV_ASSIGN;
>> +		__entry->proto = protocol;
>> +		__entry->duration = duration;
>> +	),
>> +	TP_printk(WIPHY_PR_FMT ", " WDEV_PR_FMT ", proto=%x, duration=%u",
>> +		  WIPHY_PR_ARG, WDEV_PR_ARG, __entry->proto, __entry->duration)
>> +);
>> +
>> +TRACE_EVENT(rdev_crit_proto_stop,
>> +	TP_PROTO(struct wiphy *wiphy, struct wireless_dev *wdev),
>> +	TP_ARGS(wiphy, wdev),
>> +	TP_STRUCT__entry(
>> +		WIPHY_ENTRY
>> +		WDEV_ENTRY
>> +	),
>> +	TP_fast_assign(
>> +		WIPHY_ASSIGN;
>> +		WDEV_ASSIGN;
>> +	),
>> +	TP_printk(WIPHY_PR_FMT ", " WDEV_PR_FMT,
>> +		  WIPHY_PR_ARG, WDEV_PR_ARG)
>> +);
>> +
>>   /*************************************************************
>>    *	     cfg80211 exported functions traces		     *
>>    *************************************************************/
>
>
>