On Tue, 2017-01-10 at 01:32 +0000, David Lin wrote: > > The only thing that really seems questionable to me is the whole > > beacon parsing (and apparent rebuilding in firmware?). It's very > > odd that you could do that, with a softmac device where all the > > authentication and association is handled by hostapd anyway, and > > you can't possibly pretend to handle everything hostapd might throw > > at you - this will mean that you'll have features hostapd and every > > other mac80211 supports that you will silently drop afaict - which > > is rather unexpected. > > > > First, you're parsing the data obtained from hostapd, in > > mwl_fwcmd_parse_beacon(), and then you send them all to the > > firmware in mwl_fwcmd_set_ies(), but only the things you actually > > understood. New higher-level features you don't understand, or > > vendor beacon IEs that aren't WMM, would be completely dropped. > > > > I'm not very happy with that behaviour. > > > > Why does the firmware require this? Why not just pack all IEs into > > the pcmd->ie_list_len_proprietary, and leave everything else 0? Or > > - if perhaps firmware for some reason requires HT/VHT to be treated > > specially, only parse out the ones that are really needed specially > > and leave everything else in the ie_list_len_proprietary? > > > > Also, this is dropping all the encryption stuff - even those are > > extensible btw, and hostapd might do so. Having the firmware > > rebuild those out of out-of-band information is very unexpected for > > a mac80211 driver. > > > > This driver just extracts needed IEs which is used for the host > command of firmware. I think we will not support other vendor IEs. > And if needed, we can add them to pcmd->ie_list_proprietary. Sure, I see that you're doing this. It still makes no sense though, since all management frames are handled by hostapd anyway. It would make some sense if you actually were going to handle association in the firmware, but it makes no sense here, as far as I can tell. I'm not sure how hard a line I should draw here, but I'll warn you now that I won't take this into consideration when adding new features to mac80211, and certainly Jouni won't take it into account when adding new features to hostapd, so that such new features will then SILENTLY and UNEXPECTEDLY (due to mac80211) not work with your driver. I strongly advise you to reconsider this and try to pass through all the IEs so that newly added features that shouldn't require firmware interaction (since hostapd is handling all the association handshake and IEs to advertise the feature) will automatically and cleanly work. If you really can't do that, for some reason, then for my benefit as the mac80211 maintainer and future maintainers of your driver, I'd like to have documentation in the driver as to why the firmware needs the driver to split up all those IEs and what it does with them. After all, a common-sense analysis would suggest that the firmware has no need for it, since all configuration should come through other places and all IEs are just for going out on the air. johannes
