> Johannes Berg [mailto:johannes@xxxxxxxxxxxxxxxx] wrote: > > On Wed, 2016-12-21 at 04:11 +0000, David Lin wrote: > > This patch provides the mwlwifi driver for Marvell 8863, 8864 and > > 8897 > > chipsets. > > This driver was developed as part of the openwrt.org project to > > support Linksys WRT1900AC and is maintained on > > https://github.com/kaloz/mwlwi fi. > > I found some minor things: > > * using kmalloc/memset instead of kzalloc I will change it. > * mwl_fwcmd_get_80m_pri_chnl() could use some arithmetic or at least > switch case consolidation - if it shouldn't even go away entirely > since mac80211 doesn't really limit you to the primary channel as > the spec requires it - so the whole purpose of the function seems a > bit questionable This function is used for the host command of firmware. > * wiphy_err(hw->wiphy, "failed execution\n"); seems like a bit too > generic (you have that many times - how are you going to tell which > happened?) I will modify it. > * rates = sta->supp_rates[NL80211_BAND_5GHZ] << 5; > is a bit questionable - that 5 should probably be defined (also, > does that mean you support 1,2, 5.5, 11 and *22MBps*? curious, > almost nobody does that afaict) This is used to comply with the parameters of host command of firmware. > * a lot of initializations like > struct mwl_vif *mwl_vif; > [...] > mwl_vif = mwl_dev_get_vif(vif); > > could be rolled into a single line of code (which you sometimes do, > apparently always for "priv" but never for "mwl_vif" or "pcmd"?) I will modify it. > * "celcius" isn't really spelled that way - and you probably mean > "temperature" anyway since Celsius is a unit :) I will change it. > * typo: firware I will fix it. > * a bunch of your IE handling structs seem duplicate with ieee80211.h No, only pointer is defined for extracting content of beacon. This driver does not redefine IE structure if it is already defined in ieee80211.h. > * and partially wrong - something with a vendor OUI can't be RSN > (maybe you meant WPA? though it's not quite clear to me why you need > this at all!) Yes. It means WPA. This driver does not handle content of it. It only needs the IE for host command of firmware. > * +#ifdef CONFIG_OF > +#include <linux/of.h> > +#endif > I don't think you have to guard that include ... > * +#ifdef CONFIG_DEBUG_FS > +#include "debugfs.h" > +#endif > likewise > * git am also complained about a blank line at EOF > I will remove them and fix blank line. > > 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. > johannes
