Hi Johannes, > -----Original Message----- > From: linux-wireless-owner@xxxxxxxxxxxxxxx [mailto:linux-wireless-owner@xxxxxxxxxxxxxxx] On Behalf Of > Johannes Berg > Sent: Friday, December 03, 2010 4:53 AM > To: Bing Zhao > Cc: linux-wireless@xxxxxxxxxxxxxxx; John W. Linville; Dan Williams; David Woodhouse; Amitkumar Karwar; > Kiran Divekar; Greg KH; David S. Miller > Subject: Re: [PATCH 00/31] wireless: Marvell mwifiex driver for SD8787 > > On Wed, 2010-12-01 at 17:31 -0800, Bing Zhao wrote: > > The mwifiex driver for Marvell 802.11n chipset SD8787 is available at > > git://git.marvell.com/mwifiex.git. Both STA mode and Mobile hotspot > > mode are supported. > > > > git pull git://git.marvell.com/mwifiex.git master > > Please post patches, not git trees. > > But since John is holding us somewhat hostage by saying he'll consider > merging this, and apparently you don't have the taste to even consider > the review comments you have gotten before, I took a brief look to > *prove* that this driver cannot be merged as is. > > > Here's a short list of things to fix: > * __packed > * ENTER/LEAVE must go > * enum mwifiex_status must go -- use errno > * private ioctls were grandfathered in, > but aren't allowed in new drivers > * do you really need your own crypto mode definitions? > * your pseudo-kerneldoc comments "/**" don't conform > to the right format > * private locking wrappers are not allowed (mwifiex_spin_lock etc) > * private debugging like PRINTM is not allowed > * there's exactly one space after "#include" > * struct eth_803_hdr? are you serious? and you even have > it twice: struct eth_II_hdr > * enum ieee_types_elementid_e must go > * similarly, almost all of ieee.h > * no bitfields and BIG_ENDIAN_BITFIELD allowed > * not all allocations are GFP_ATOMIC, use wisely and > GFP_KERNEL in most places > * a wifi driver that's doing stuff with sockets (nl_sk, sk_socket)? > explain, in MUCH detail (or preferably just get rid of it) > * there's debugfs_remove_recursive > * some functions like mwifiex_is_network_compatible need to be broken > into smaller ones to avoid the crap indentation they have > * mwifiex_uap_ap_cfg_parse_data. Must I say more? > * struct mwifiex_buffer *mbuf? Is this a BSD driver? We have SKBs, get > rid of this. (Oh and skbs have a CB) > * mwifiex_init_lock/mwifiex_free_lock -- I'd laugh in your general > direction if it wasn't so sad > * mwifiex_util_peek_list etc.: learn about list.h > * mwifiex_init_timer -- still sad, dynamically allocating timers? just > like spinlocks, they can be embedded and make your code simpler During the past 3 months, we have made 56 mwifiex patches (listed below) since the initial mwifiex driver submission. Most of these patches address the above issues; and the ioctl multiple abstraction layers and the static forward declarations as you commented in another e-mail. Some other patches are for code cleanups, mainly coding style. Any comments from you, John, or anyone else, are always welcome. Again, thank you all for your helps so that I can improve the mwifiex driver a lot! Best regards, Bing =================================================== Amitkumar Karwar (23): mwifiex: remove some element ID's definitions mwifiex: remove struct ieeetypes_sub_band_set from ieee.h mwifiex: remove bitfields and __BIG_ENDIAN_BITFIELD mwifiex: remove wrapper functions for spin_lock/spin_unlock mwifiex: modify set_default_key() prototype mwifiex: use debugfs_remove_recursive() mwifiex: solve kernel dump issue for debugfs command 'debug' mwifiex: remove set operation for debugfs command 'debug' mwifiex: remove some crypto definitions mwifiex: remove linked list implementation mwifiex: remove duplicated NULL checks mwifiex: use modified ieee80211_channel_to_frequency() API mwifiex: fix list_del corruption while unloading modules mwifiex: cleanup command ioctls and abstraction layers mwifiex: remove enum mwifiex_status mwifiex: remove ENTER and LEAVE mwifiex: remove unused/unnecessary ret variables and usage of goto mwifiex: replace pseudo-kerneldoc comments. mwifiex: break some functions into smaller ones mwifiex: code cleanup: save some extra lines mwifiex: remove redundant variable upld_len mwifiex: remove unnecessory external function declarations mwifiex: use dynamic allocation for large size structure Bing Zhao (20): mwifiex: replace struct eth_II_hdr with ethhdr mwifiex: replace struct eth_803_hdr with ethhdr mwifiex: remove struct mwifiex_drv_timer and associated mwifiex: replace struct ieee_htinfo with ieee80211_ht_info mwifiex: remove struct ieee_htcap mwifiex: remove struct ieee_types_erp_info mwifiex: remove struct ieee_types_htinfo mwifiex: remove struct ieee_types_htcap mwifiex: remove struct ieee_extcap and ieee_types_extcap mwifiex: remove static forward declarations mwifiex: make a shorter name of hotplug_device mwifiex: remove PRINTM/HEXDUMP and associated mwifiex: remove struct ieee_bssco_2040 mwifiex: remove struct ieee_types_2040bssco mwifiex: remove COUNTRY_CODE_LEN mwifiex: remove MWIFIEX_MAC_ADDR_LENGTH mwifiex: use IEEE80211_MAX_SSID_LEN mwifiex: remove unused macros mwifiex: remove unnecessary mwifiex_get_assoc_status() mwifiex: remove unnecessary type casting John W. Linville (3): mwifiex: remove uAP functionality mwifiex: remove NETLINK_MARVELL and associate mwifiex: remove questionable wireless_send_event Kiran Divekar (6): mwifiex: change incorrect GFP_ATOMIC memory allocation mwifiex: remove timer wrapper functions mwifiex: keep exactly one space after #include mwifiex: convert uses of __attribute__ ((packed)) to __packed mwifiex: remove un-needed functions mwifiex: remove redundant event handling Marc Yang (1): mwifiex: remove or add braces where applicable Yogesh Ashok Powar (3): mwifiex: remove struct mwifiex_buffer and associated mwifiex: rearrange code for better readability mwifiex: remove ieee.h drivers/net/wireless/mwifiex/11n.c | 988 ++--- drivers/net/wireless/mwifiex/11n.h | 162 +- drivers/net/wireless/mwifiex/11n_aggr.c | 397 +- drivers/net/wireless/mwifiex/11n_aggr.h | 11 +- drivers/net/wireless/mwifiex/11n_rxreorder.c | 562 +-- drivers/net/wireless/mwifiex/11n_rxreorder.h | 45 +- drivers/net/wireless/mwifiex/Makefile | 4 - drivers/net/wireless/mwifiex/README | 92 - drivers/net/wireless/mwifiex/cfg80211.c | 1413 +++---- drivers/net/wireless/mwifiex/cfg80211.h | 13 +- drivers/net/wireless/mwifiex/cfp.c | 143 +- drivers/net/wireless/mwifiex/cmdevt.c | 1347 +++---- drivers/net/wireless/mwifiex/debugfs.c | 798 +--- drivers/net/wireless/mwifiex/decl.h | 127 +- drivers/net/wireless/mwifiex/fw.h | 801 ++--- drivers/net/wireless/mwifiex/ieee.h | 372 -- drivers/net/wireless/mwifiex/init.c | 477 +-- drivers/net/wireless/mwifiex/ioctl.h | 473 +-- drivers/net/wireless/mwifiex/join.c | 1029 ++--- drivers/net/wireless/mwifiex/main.c | 853 +--- drivers/net/wireless/mwifiex/main.h | 844 ++--- drivers/net/wireless/mwifiex/scan.c | 2234 +++++------ drivers/net/wireless/mwifiex/sdio.c | 1277 +++---- drivers/net/wireless/mwifiex/sdio.h | 188 +- drivers/net/wireless/mwifiex/sta_cmd.c | 811 ++--- drivers/net/wireless/mwifiex/sta_cmdresp.c | 1114 ++---- drivers/net/wireless/mwifiex/sta_event.c | 273 +- drivers/net/wireless/mwifiex/sta_ioctl.c | 5813 +++++++------------------- drivers/net/wireless/mwifiex/sta_rx.c | 184 +- drivers/net/wireless/mwifiex/sta_tx.c | 162 +- drivers/net/wireless/mwifiex/txrx.c | 194 +- drivers/net/wireless/mwifiex/uap.c | 99 - drivers/net/wireless/mwifiex/uap.h | 160 - drivers/net/wireless/mwifiex/uap_cmdevent.c | 1544 ------- drivers/net/wireless/mwifiex/uap_ioctl.c | 397 -- drivers/net/wireless/mwifiex/uap_txrx.c | 416 -- drivers/net/wireless/mwifiex/util.c | 1408 +------ drivers/net/wireless/mwifiex/util.h | 226 +- drivers/net/wireless/mwifiex/wmm.c | 982 ++--- drivers/net/wireless/mwifiex/wmm.h | 73 +- 40 files changed, 8146 insertions(+), 20360 deletions(-) delete mode 100644 drivers/net/wireless/mwifiex/ieee.h delete mode 100644 drivers/net/wireless/mwifiex/uap.c delete mode 100644 drivers/net/wireless/mwifiex/uap.h delete mode 100644 drivers/net/wireless/mwifiex/uap_cmdevent.c delete mode 100644 drivers/net/wireless/mwifiex/uap_ioctl.c delete mode 100644 drivers/net/wireless/mwifiex/uap_txrx.c > > Ok good enough, that should keep you busy for a while... > > The above is might be a good basis for a TODO file for staging... > > 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 ÿô.nlj·Ÿ®‰†+%ŠË±é¥Šwÿº{.nlj·¥Š{±ÿ«zW¬³ø¡Ü}©ž²ÆzÚj:+v‰¨þø®w¥þŠàÞ¨è&¢)ß«a¶Úÿûz¹ÞúŽŠÝjÿŠwèf
