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 * 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 * 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?) * 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) * 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"?) * "celcius" isn't really spelled that way - and you probably mean "temperature" anyway since Celsius is a unit :) * typo: firware * a bunch of your IE handling structs seem duplicate with 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!) * +#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 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. johannes
