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
--
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
