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. 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 <[email protected]>
Reviewed-by: Franky (Zhenhui) Lin <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
Hi Johannes,
I decided based on last comments to submit it as a patch this
time for inclusion in the mac80211-next repository. Last changes
made are mentioned in V5 below. It applies to the master branch on
top of commit "37a0116 mac80211: fix do_stop handling while suspended"
Regards, Arend
Changelog:
----------
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 | 31 ++++++++++++++++
net/wireless/core.h | 3 ++
net/wireless/mlme.c | 3 ++
net/wireless/nl80211.c | 81 ++++++++++++++++++++++++++++++++++++++++++
net/wireless/rdev-ops.h | 28 ++++++++++++++-
net/wireless/trace.h | 35 ++++++++++++++++++
7 files changed, 203 insertions(+), 1 deletion(-)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 57870b6..f75d720 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2002,6 +2002,9 @@ 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.
+ * @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 +2234,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);
};
/*
@@ -2830,6 +2839,7 @@ struct cfg80211_cached_keys;
* @p2p_started: true if this is a P2P Device that has been started
* @cac_started: true if DFS channel availability check has been started
* @cac_start_time: timestamp (jiffies) when the dfs state was entered.
+ * @crit_proto_started: true if crit_proto_start() has been done.
*/
struct wireless_dev {
struct wiphy *wiphy;
@@ -2884,6 +2894,8 @@ struct wireless_dev {
bool cac_started;
unsigned long cac_start_time;
+ bool crit_proto_started;
+
#ifdef CONFIG_CFG80211_WEXT
/* wext data */
struct {
@@ -4126,6 +4138,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);
+
/* 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..16d3eb5 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 */
@@ -1709,6 +1719,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 +3695,22 @@ 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_BOOTP: BOOTP protocol.
+ * @NL80211_CRIT_PROTO_EAPOL: EAPOL protocol.
+ * @NL80211_CRIT_PROTO_ARP: ARP protocol for APIPA.
+ * @NL80211_CRIT_PROTO_LAST: must be kept last.
+ */
+enum nl80211_crit_proto_id {
+ NL80211_CRIT_PROTO_UNSPEC,
+ NL80211_CRIT_PROTO_BOOTP,
+ NL80211_CRIT_PROTO_EAPOL,
+ NL80211_CRIT_PROTO_ARP,
+ /* add other protocols before this one */
+ NL80211_CRIT_PROTO_LAST
+};
+
#endif /* __LINUX_NL80211_H */
diff --git a/net/wireless/core.h b/net/wireless/core.h
index d5d06fd..2db239c 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;
+ /* critical protocol operation started */
+ bool crit_proto_started;
+
/* 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..7c42b04 100644
--- a/net/wireless/mlme.c
+++ b/net/wireless/mlme.c
@@ -648,6 +648,9 @@ void cfg80211_mlme_unregister_socket(struct wireless_dev *wdev, u32 nlportid)
spin_unlock_bh(&wdev->mgmt_registrations_lock);
+ if (rdev->ops->crit_proto_stop)
+ 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..9e126e9 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -25,6 +25,8 @@
#include "reg.h"
#include "rdev-ops.h"
+#define NL80211_MAX_CRIT_PROT_DURATION 5000 /* msec */
+
static int nl80211_crypto_settings(struct cfg80211_registered_device *rdev,
struct genl_info *info,
struct cfg80211_crypto_settings *settings,
@@ -1417,6 +1419,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 +8202,55 @@ 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;
+
+ if (!rdev->ops->crit_proto_start)
+ return -EOPNOTSUPP;
+
+ if (WARN_ON(!rdev->ops->crit_proto_stop))
+ return -EINVAL;
+
+ if (rdev->crit_proto_started)
+ 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 >= NL80211_CRIT_PROTO_LAST)
+ 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]);
+
+ duration = min_t(u16, duration, NL80211_MAX_CRIT_PROT_DURATION);
+
+ return rdev_crit_proto_start(rdev, wdev, proto, duration);
+}
+
+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;
+
+ 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 +8940,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 +10701,16 @@ void cfg80211_ft_event(struct net_device *netdev,
}
EXPORT_SYMBOL(cfg80211_ft_event);
+void cfg80211_crit_proto_stopped(struct wireless_dev *wdev)
+{
+ struct cfg80211_registered_device *rdev;
+
+ rdev = wiphy_to_dev(wdev->wiphy);
+ WARN_ON(!rdev->crit_proto_started);
+ rdev->crit_proto_started = false;
+}
+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..720e0c5 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,30 @@ 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);
+ rdev->crit_proto_started = !ret;
+ 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);
+ if (rdev->crit_proto_started) {
+ rdev->ops->crit_proto_stop(&rdev->wiphy, wdev);
+ rdev->crit_proto_started = false;
+ }
+ 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 *
*************************************************************/
--
1.7.10.4
On 04/09/2013 12:06 PM, Johannes Berg wrote:
> On Mon, 2013-04-08 at 11:09 +0200, Arend van Spriel wrote:
>> +void cfg80211_crit_proto_stopped(struct wireless_dev *wdev)
>> +{
>> + struct cfg80211_registered_device *rdev;
>> +
>> + rdev = wiphy_to_dev(wdev->wiphy);
>> + WARN_ON(!rdev->crit_proto_started);
>> + rdev->crit_proto_started = false;
>> +}
>
> Oh, so you don't want to tell userspace?
Just an observation will looking for an example of netlink event
messaging to user-space. I noticed that the cfg80211_ft_event() function
used fixed GFP_KERNEL value. Should event functions always have a gfp_t
parameter?
Gr. AvS
On Tue, 2013-04-16 at 23:19 +0200, Arend van Spriel wrote:
> >> + NL80211_CMD_CRIT_PROTOCOL_START,
> >> + NL80211_CMD_CRIT_PROTOCOL_STOP,
> >> + NL80211_CMD_CRIT_PROTOCOL_STOPPED_EVENT,
> >
> > Why use a separate command ID? Usually we use the same _STOP for the
> > event as well, I think? Except maybe scan which you can't stop? Not
> > sure ... Anyway I don't mind, just wondering if there was a special
> > reason to do this.
> >
>
> This is my first nl80211 event :-) I looked at the ft_event thingy. I am
> fine using the _STOP instead.
Ah, but that didn't have a command. I'd just use _STOP in this case.
> >> + nla_put_failure:
> >> + if (hdr)
> >> + genlmsg_cancel(msg, hdr);
> >
> > There's not really a reason to cancel, but we still do most of the time.
> > I guess we can keep it, but it doesn't matter :)
>
> If it not really needed it may call for separate patch removing all
> occurrences?
Yeah, agree.
> >> trace_rdev_return_void(&rdev->wiphy);
> >> -} $
> >> +}$
> >
> > Heh, thanks.
>
> Have to thank my editor, I guess. Trailing whitespace?
Plenty (see the $ signs I just inserted)
johannes
On Thu, 2013-04-11 at 12:47 +0200, Arend van Spriel wrote:
> On 04/09/2013 12:06 PM, Johannes Berg wrote:
> > On Mon, 2013-04-08 at 11:09 +0200, Arend van Spriel wrote:
> >> +void cfg80211_crit_proto_stopped(struct wireless_dev *wdev)
> >> +{
> >> + struct cfg80211_registered_device *rdev;
> >> +
> >> + rdev = wiphy_to_dev(wdev->wiphy);
> >> + WARN_ON(!rdev->crit_proto_started);
> >> + rdev->crit_proto_started = false;
> >> +}
> >
> > Oh, so you don't want to tell userspace?
>
> Just an observation will looking for an example of netlink event
> messaging to user-space. I noticed that the cfg80211_ft_event() function
> used fixed GFP_KERNEL value. Should event functions always have a gfp_t
> parameter?
Well I guess in general I would add it (unless there are other reasons
it needs to be able to sleep). OTOH, if all drivers always call it in a
context where they can sleep, it's safe to just use GFP_KERNEL, so for
the ft_event it probably doesn't actually matter.
johannes
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. 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 <[email protected]>
Reviewed-by: Franky (Zhenhui) Lin <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
Hi Johannes,
I reworked the patch based on your comment in this V6. It
applies to the master branch on top of commit:
5253ffb mac80211: always pick a basic rate to tx RTS/CTS for pre-HT rates
Regards, Arend
Changelog:
----------
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 | 27 ++++++++++
include/uapi/linux/nl80211.h | 40 +++++++++++++++
net/wireless/core.h | 3 ++
net/wireless/mlme.c | 5 ++
net/wireless/nl80211.c | 116 ++++++++++++++++++++++++++++++++++++++++++
net/wireless/rdev-ops.h | 24 ++++++++-
net/wireless/trace.h | 35 +++++++++++++
7 files changed, 249 insertions(+), 1 deletion(-)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 57870b6..7afd1ad 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);
};
/*
@@ -2830,6 +2842,7 @@ struct cfg80211_cached_keys;
* @p2p_started: true if this is a P2P Device that has been started
* @cac_started: true if DFS channel availability check has been started
* @cac_start_time: timestamp (jiffies) when the dfs state was entered.
+ * @crit_proto_started: true if crit_proto_start() has been done.
*/
struct wireless_dev {
struct wiphy *wiphy;
@@ -2884,6 +2897,8 @@ struct wireless_dev {
bool cac_started;
unsigned long cac_start_time;
+ bool crit_proto_started;
+
#ifdef CONFIG_CFG80211_WEXT
/* wext data */
struct {
@@ -4126,6 +4141,18 @@ 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. This function must not be called
+ * in .crit_proto_stop() context.
+ */
+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..c68373b 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,10 @@ enum nl80211_commands {
NL80211_CMD_UPDATE_FT_IES,
NL80211_CMD_FT_EVENT,
+ NL80211_CMD_CRIT_PROTOCOL_START,
+ NL80211_CMD_CRIT_PROTOCOL_STOP,
+ NL80211_CMD_CRIT_PROTOCOL_STOPPED_EVENT,
+
/* add new commands above here */
/* used to define NL80211_CMD_MAX below */
@@ -1414,6 +1425,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 +1725,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 +3701,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..719c2f8 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_stop(rdev, wdev);
+ rdev->crit_proto_nlportid = 0;
+ }
+
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..533eccf 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_stop(rdev, wdev);
+ rdev->crit_proto_nlportid = 0;
+ }
+ 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,44 @@ 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);
+ WARN_ON(!rdev->crit_proto_nlportid);
+ 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_STOPPED_EVENT);
+ 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 *
*************************************************************/
--
1.7.10.4
On Mon, 2013-04-08 at 11:09 +0200, Arend van Spriel wrote:
> + * @crit_proto_start: Indicates a critical protocol needs more link reliability.
Can you elaborate here on what the protocol means? Why is it there, and
how is it supposed to be used? Why and how would a device/driver do
something different for different protocols?
Also please document that the duration units is milliseconds. (both here
and in the missing kernel-doc for the nl80211 attributes)
> +/**
> + * 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);
May need gfp_t argument to send a netlink message?
> @@ -1709,6 +1719,9 @@ enum nl80211_attrs {
> NL80211_ATTR_MDID,
> NL80211_ATTR_IE_RIC,
>
> + NL80211_ATTR_CRIT_PROT_ID,
> + NL80211_ATTR_MAX_CRIT_PROT_DURATION,
missing kernel-doc.
> +/**
> + * enum nl80211_crit_proto_id - nl80211 critical protocol identifiers
> + *
> + * @NL80211_CRIT_PROTO_UNSPEC: protocol unspecified.
> + * @NL80211_CRIT_PROTO_BOOTP: BOOTP protocol.
> + * @NL80211_CRIT_PROTO_EAPOL: EAPOL protocol.
> + * @NL80211_CRIT_PROTO_ARP: ARP protocol for APIPA.
Don't like IPv6? :-)
> + * @NL80211_CRIT_PROTO_LAST: must be kept last.
shouldn't that be NUM_NL80211_CRIT_PROTO to be more like the (most)
other enums?
> --- a/net/wireless/mlme.c
> +++ b/net/wireless/mlme.c
> @@ -648,6 +648,9 @@ void cfg80211_mlme_unregister_socket(struct wireless_dev *wdev, u32 nlportid)
>
> spin_unlock_bh(&wdev->mgmt_registrations_lock);
>
> + if (rdev->ops->crit_proto_stop)
> + rdev_crit_proto_stop(rdev, wdev);
This is broken, you're not checking that it's the correct socket.
Therefore, if you run, for example, "iw wlan0 link" while the critical
operation is ongoing it will be aborted.
> + duration = min_t(u16, duration, NL80211_MAX_CRIT_PROT_DURATION);
Why not reject it if too large (although then the max should be defined
in the header file)? Is there a reason, like maybe wanting to be able to
increase the kernel value later? If so, might want to have a comment?
> +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;
> +
> + rdev_crit_proto_stop(rdev, wdev);
What if it's not even started?
> +void cfg80211_crit_proto_stopped(struct wireless_dev *wdev)
> +{
> + struct cfg80211_registered_device *rdev;
> +
> + rdev = wiphy_to_dev(wdev->wiphy);
> + WARN_ON(!rdev->crit_proto_started);
> + rdev->crit_proto_started = false;
> +}
Oh, so you don't want to tell userspace?
Do you expect drivers to call this function even when explicitly asked
to stop? That should be documented then, I think.
johannes
On Wed, 2013-04-10 at 13:49 +0200, Arend van Spriel wrote:
> >> I am a dark-ages guy :-p I think I will rename the BOOTP one and
> >> indicate it should be used for BOOTP and DHCPv6.
> >
> > Might also be worth it to rename ARP to APIPA since ARP is ... well
> > often done :-)
>
> I chose ARP because APIPA is essentially not a protocol. As far as I
> understand it makes use of ARP to make sure the address in unique. I see
> your point and will change it. Then again it all may go away :-)
I see your point too, but I'd argue that APIPA is really more of a
higher-level protocol that uses ARP for its implementation?
> > Well you have to store the nlportid (rather than crit_proto_started) and
> > then check it.
>
> Yeah, I surmised that already. If I would drop crit_proto_started and
> use the portid as a flag as well, I need an invalid portid. Is there a
> definition for that?
I'm pretty sure 0 is invalid for userspace because it means "kernel".
johannes
On 04/16/2013 04:12 PM, Johannes Berg wrote:
>
>
>> I reworked the patch based on your comment in this V6. It
>> applies to the master branch on top of commit:
>>
>> 5253ffb mac80211: always pick a basic rate to tx RTS/CTS for pre-HT rates
>
> Yep, applies, thanks. I think you missed something, I can fix it but
> wanted to ask:
>
>
>> + * @crit_proto_started: true if crit_proto_start() has been done.
>> */
>> struct wireless_dev {
>
>> + bool crit_proto_started;
>
> This is no longer needed, right?
>
oops. That should go indeed.
>> + NL80211_CMD_CRIT_PROTOCOL_START,
>> + NL80211_CMD_CRIT_PROTOCOL_STOP,
>> + NL80211_CMD_CRIT_PROTOCOL_STOPPED_EVENT,
>
> Why use a separate command ID? Usually we use the same _STOP for the
> event as well, I think? Except maybe scan which you can't stop? Not
> sure ... Anyway I don't mind, just wondering if there was a special
> reason to do this.
>
This is my first nl80211 event :-) I looked at the ft_event thingy. I am
fine using the _STOP instead.
>> + nla_put_failure:
>> + if (hdr)
>> + genlmsg_cancel(msg, hdr);
>
> There's not really a reason to cancel, but we still do most of the time.
> I guess we can keep it, but it doesn't matter :)
If it not really needed it may call for separate patch removing all
occurrences?
>> --- 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);
>> -}
>> +}
>
> Heh, thanks.
Have to thank my editor, I guess. Trailing whitespace?
Gr. AvS
> I reworked the patch based on your comment in this V6. It
> applies to the master branch on top of commit:
>
> 5253ffb mac80211: always pick a basic rate to tx RTS/CTS for pre-HT rates
Yep, applies, thanks. I think you missed something, I can fix it but
wanted to ask:
> + * @crit_proto_started: true if crit_proto_start() has been done.
> */
> struct wireless_dev {
> + bool crit_proto_started;
This is no longer needed, right?
> + NL80211_CMD_CRIT_PROTOCOL_START,
> + NL80211_CMD_CRIT_PROTOCOL_STOP,
> + NL80211_CMD_CRIT_PROTOCOL_STOPPED_EVENT,
Why use a separate command ID? Usually we use the same _STOP for the
event as well, I think? Except maybe scan which you can't stop? Not
sure ... Anyway I don't mind, just wondering if there was a special
reason to do this.
> + nla_put_failure:
> + if (hdr)
> + genlmsg_cancel(msg, hdr);
There's not really a reason to cancel, but we still do most of the time.
I guess we can keep it, but it doesn't matter :)
> --- 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);
> -}
> +}
Heh, thanks.
johannes
On 04/09/2013 10:42 PM, Johannes Berg wrote:
> On Tue, 2013-04-09 at 21:54 +0200, Arend van Spriel wrote:
>
>> I should at least try :-) The given protocol can help the driver decide
>> what actions should be taken. As an example, for a streaming protocol
>> like Miracast [1] other wireless parameters/features may be changed as
>> for DHCP.
>
> Ok? I guess I don't really see what you'd do differently, but hey, what
> do I know :)
Understood. It was the input I received internally that they would, but
I will discuss whether it makes sense to treat all in the same way.
>>>> + * @NL80211_CRIT_PROTO_UNSPEC: protocol unspecified.
>>>> + * @NL80211_CRIT_PROTO_BOOTP: BOOTP protocol.
>>>> + * @NL80211_CRIT_PROTO_EAPOL: EAPOL protocol.
>>>> + * @NL80211_CRIT_PROTO_ARP: ARP protocol for APIPA.
>>>
>>> Don't like IPv6? :-)
>>
>> I am a dark-ages guy :-p I think I will rename the BOOTP one and
>> indicate it should be used for BOOTP and DHCPv6.
>
> Might also be worth it to rename ARP to APIPA since ARP is ... well
> often done :-)
I chose ARP because APIPA is essentially not a protocol. As far as I
understand it makes use of ARP to make sure the address in unique. I see
your point and will change it. Then again it all may go away :-)
>>>> @@ -648,6 +648,9 @@ void cfg80211_mlme_unregister_socket(struct wireless_dev *wdev, u32 nlportid)
>>>>
>>>> spin_unlock_bh(&wdev->mgmt_registrations_lock);
>>>>
>>>> + if (rdev->ops->crit_proto_stop)
>>>> + rdev_crit_proto_stop(rdev, wdev);
>>>
>>> This is broken, you're not checking that it's the correct socket.
>>> Therefore, if you run, for example, "iw wlan0 link" while the critical
>>> operation is ongoing it will be aborted.
>>
>> I was wondering about that. Will change it checking nlportid, right?
>
> Well you have to store the nlportid (rather than crit_proto_started) and
> then check it.
Yeah, I surmised that already. If I would drop crit_proto_started and
use the portid as a flag as well, I need an invalid portid. Is there a
definition for that?
>>>> + duration = min_t(u16, duration, NL80211_MAX_CRIT_PROT_DURATION);
>>>
>>> Why not reject it if too large (although then the max should be defined
>>> in the header file)? Is there a reason, like maybe wanting to be able to
>>> increase the kernel value later? If so, might want to have a comment?
>>
>> There were people in earlier discussions that considered a timeguard
>> appropriate, ie. not trusting user-space. I do not have a strong opinion
>> on this so....
>
> I'm not really arguing there shouldn't be any limit, but I guess I'm not
> sure why it should be limited rather than refusing anything above the
> limit? Maybe it'd be worthwhile to advertise the limit instead and then
> just take it?
>
> It really doesn't matter all that much though ... mostly I'm curious
> whether any design thought went into this :-)
I guess not :-( Thanks for clarifying you were talking about the
limiting mechanism. I will modify that.
>>>> + rdev_crit_proto_stop(rdev, wdev);
>>>
>>> What if it's not even started?
>>
>> That is handled in rdev_crit_proto_stop() itself.
>
> Oh, I see. In a way I guess that makes sense, but should this not return
> an error? Also, I'd like the rdev_* inlines to not actually have such
> logic, I tend to simply ignore them when reading ...
I was not aware of that coding principle although I could have seen a
pattern looking at the other rdev_* inlines. I will take the logic out
of it.
> Or maybe just give that another name / put it elsewhere? Maybe even give
> it a return value then to refuse stopping crit proto when it's not
> started?
>
>>> Do you expect drivers to call this function even when explicitly asked
>>> to stop? That should be documented then, I think.
>>
>> No, I don't and I will add that in documentation.
>
> I was going to say this is broken then ... but that's again only because
> the wrapper sets started=false. See what you did there? I totally
> ignored the rdev_*() wrapper code :)
Yeah, yeah. I got it the first time :-p
Regards,
Arend
On 04/09/2013 12:06 PM, Johannes Berg wrote:
> On Mon, 2013-04-08 at 11:09 +0200, Arend van Spriel wrote:
>
>> + * @crit_proto_start: Indicates a critical protocol needs more link reliability.
>
> Can you elaborate here on what the protocol means? Why is it there, and
> how is it supposed to be used? Why and how would a device/driver do
> something different for different protocols?
Thanks, Johannes
I should at least try :-) The given protocol can help the driver decide
what actions should be taken. As an example, for a streaming protocol
like Miracast [1] other wireless parameters/features may be changed as
for DHCP.
> Also please document that the duration units is milliseconds. (both here
> and in the missing kernel-doc for the nl80211 attributes)
>
Will do.
>> +/**
>> + * 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);
>
> May need gfp_t argument to send a netlink message?
>
So far, the is not netlink message being sent, but I guess we should.
>> @@ -1709,6 +1719,9 @@ enum nl80211_attrs {
>> NL80211_ATTR_MDID,
>> NL80211_ATTR_IE_RIC,
>>
>> + NL80211_ATTR_CRIT_PROT_ID,
>> + NL80211_ATTR_MAX_CRIT_PROT_DURATION,
>
> missing kernel-doc.
Will add that.
>> +/**
>> + * enum nl80211_crit_proto_id - nl80211 critical protocol identifiers
>> + *
>> + * @NL80211_CRIT_PROTO_UNSPEC: protocol unspecified.
>> + * @NL80211_CRIT_PROTO_BOOTP: BOOTP protocol.
>> + * @NL80211_CRIT_PROTO_EAPOL: EAPOL protocol.
>> + * @NL80211_CRIT_PROTO_ARP: ARP protocol for APIPA.
>
> Don't like IPv6? :-)
I am a dark-ages guy :-p I think I will rename the BOOTP one and
indicate it should be used for BOOTP and DHCPv6.
>> + * @NL80211_CRIT_PROTO_LAST: must be kept last.
>
> shouldn't that be NUM_NL80211_CRIT_PROTO to be more like the (most)
> other enums?
Will do.
>> --- a/net/wireless/mlme.c
>> +++ b/net/wireless/mlme.c
>> @@ -648,6 +648,9 @@ void cfg80211_mlme_unregister_socket(struct wireless_dev *wdev, u32 nlportid)
>>
>> spin_unlock_bh(&wdev->mgmt_registrations_lock);
>>
>> + if (rdev->ops->crit_proto_stop)
>> + rdev_crit_proto_stop(rdev, wdev);
>
> This is broken, you're not checking that it's the correct socket.
> Therefore, if you run, for example, "iw wlan0 link" while the critical
> operation is ongoing it will be aborted.
I was wondering about that. Will change it checking nlportid, right?
>> + duration = min_t(u16, duration, NL80211_MAX_CRIT_PROT_DURATION);
>
> Why not reject it if too large (although then the max should be defined
> in the header file)? Is there a reason, like maybe wanting to be able to
> increase the kernel value later? If so, might want to have a comment?
There were people in earlier discussions that considered a timeguard
appropriate, ie. not trusting user-space. I do not have a strong opinion
on this so....
>> +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;
>> +
>> + rdev_crit_proto_stop(rdev, wdev);
>
> What if it's not even started?
That is handled in rdev_crit_proto_stop() itself.
>
>> +void cfg80211_crit_proto_stopped(struct wireless_dev *wdev)
>> +{
>> + struct cfg80211_registered_device *rdev;
>> +
>> + rdev = wiphy_to_dev(wdev->wiphy);
>> + WARN_ON(!rdev->crit_proto_started);
>> + rdev->crit_proto_started = false;
>> +}
>
> Oh, so you don't want to tell userspace?
Better we do, I guess.
> Do you expect drivers to call this function even when explicitly asked
> to stop? That should be documented then, I think.
No, I don't and I will add that in documentation.
On Tue, 2013-04-09 at 21:54 +0200, Arend van Spriel wrote:
> I should at least try :-) The given protocol can help the driver decide
> what actions should be taken. As an example, for a streaming protocol
> like Miracast [1] other wireless parameters/features may be changed as
> for DHCP.
Ok? I guess I don't really see what you'd do differently, but hey, what
do I know :)
> >> + * @NL80211_CRIT_PROTO_UNSPEC: protocol unspecified.
> >> + * @NL80211_CRIT_PROTO_BOOTP: BOOTP protocol.
> >> + * @NL80211_CRIT_PROTO_EAPOL: EAPOL protocol.
> >> + * @NL80211_CRIT_PROTO_ARP: ARP protocol for APIPA.
> >
> > Don't like IPv6? :-)
>
> I am a dark-ages guy :-p I think I will rename the BOOTP one and
> indicate it should be used for BOOTP and DHCPv6.
Might also be worth it to rename ARP to APIPA since ARP is ... well
often done :-)
> >> @@ -648,6 +648,9 @@ void cfg80211_mlme_unregister_socket(struct wireless_dev *wdev, u32 nlportid)
> >>
> >> spin_unlock_bh(&wdev->mgmt_registrations_lock);
> >>
> >> + if (rdev->ops->crit_proto_stop)
> >> + rdev_crit_proto_stop(rdev, wdev);
> >
> > This is broken, you're not checking that it's the correct socket.
> > Therefore, if you run, for example, "iw wlan0 link" while the critical
> > operation is ongoing it will be aborted.
>
> I was wondering about that. Will change it checking nlportid, right?
Well you have to store the nlportid (rather than crit_proto_started) and
then check it.
> >> + duration = min_t(u16, duration, NL80211_MAX_CRIT_PROT_DURATION);
> >
> > Why not reject it if too large (although then the max should be defined
> > in the header file)? Is there a reason, like maybe wanting to be able to
> > increase the kernel value later? If so, might want to have a comment?
>
> There were people in earlier discussions that considered a timeguard
> appropriate, ie. not trusting user-space. I do not have a strong opinion
> on this so....
I'm not really arguing there shouldn't be any limit, but I guess I'm not
sure why it should be limited rather than refusing anything above the
limit? Maybe it'd be worthwhile to advertise the limit instead and then
just take it?
It really doesn't matter all that much though ... mostly I'm curious
whether any design thought went into this :-)
> >> + rdev_crit_proto_stop(rdev, wdev);
> >
> > What if it's not even started?
>
> That is handled in rdev_crit_proto_stop() itself.
Oh, I see. In a way I guess that makes sense, but should this not return
an error? Also, I'd like the rdev_* inlines to not actually have such
logic, I tend to simply ignore them when reading ...
Or maybe just give that another name / put it elsewhere? Maybe even give
it a return value then to refuse stopping crit proto when it's not
started?
> > Do you expect drivers to call this function even when explicitly asked
> > to stop? That should be documented then, I think.
>
> No, I don't and I will add that in documentation.
I was going to say this is broken then ... but that's again only because
the wrapper sets started=false. See what you did there? I totally
ignored the rdev_*() wrapper code :)
johannes