Search Linux Wireless

RE: [PATCH 00/31] wireless: Marvell mwifiex driver for SD8787

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Thank you very much for all the detailed comments. We will work on these items and re-submit the patches.

Regards,

Bing

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



[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]
  Powered by Linux