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.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
