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