> From: Johannes Berg [mailto:johannes@xxxxxxxxxxxxxxxx] > Sent: woensdag 5 oktober 2011 16:57 > > Some comments. > > I sort of understand your fascination with "generic utils" but they're > wasteful if they're in a single driver only. They should either be made > more generic (for all drivers) or dissolved, so: > > * You can get rid of BRCMS_BITSCNT and brcmu_bitcount, they're no > longer used at all. > * brcmu_mhz2channel isn't used anywhere either > * neither is brcmu_chspec_ctlchan > * brcmu_mw_to_qdbm is used only in a single file, > it can move there to save the export etc. > * same for brcmu_qdbm_to_mw, brcmu_chipname, brcmu_parse_tlvs, > brcmu_chspec_malformed > * brcmu_mkiovar is used only in fmac, so you can move it there > to save the export > * brcmu_format_flags is used only in smac, so same > * same for brcmu_format_flags > > After all the above, bcmutils are left with only the pktq and pktbuf > stuff, which hopefully will be either more generic or dissolved at some > point in the future since it should really just be a few skb queues. Agree. There is opportunity to cleanup on the brcmutil module or even get rid of it. > Now the drivers :-) > > FMAC: > * A whole bunch of things in dhd.h still seem to lack endian > annotations, which I wouldn't be too worried about, if it didn't > also seem to lack conversions. For example brcmf_pkt_filter_enable, > brcmf_pkt_filter, brcmf_pkt_filter_pattern. > * These, along with others like brcmf_bss_info, also seem to lack > __packed annotations. I thought you fixed all this, what am I > missing? > * bss_info[1]; and similar should just be [] I think? > * some very odd indentation on line 1172 of wl_cfg80211.c > > SMAC: > * do you really want to be 40MHz intolerant all the time? not > supporting 40 MHz is one thing, but being intolerant? > * brcms_ops_stop doesn't seem right -- this should really power down > the device, this shouldn't be done per interface since the monitor > case will want to RX/TX even when no interface is there; also, > you'll > want multi-vif support soon :-) Agree. We noticed reading mac80211.h with statement about add_interface callback not being used in monitor mode. > * locking tx() is inefficient, you should be able to go with a lock > per > HW queue > * brcms_ops_sta_add is strange -- it shouldn't be modifying the > station > parameters, why would it do that? seems like a bug to me, especially > always assuming the peer can do greenfield etc. This data should > already be set by mac80211, if not that's a bug, not something to > "fix" in the driver > * I still think things like brcms_msleep() are guaranteed to create > subtle locking bugs that are almost impossible to find since it > drops > the spinlock and if somebody calls the function somewhere they won't > necessarily keep that in mind. There are a whole bunch of functions > behaving like that. Agree. The locking strategy in the driver works and has been proven to work for the current code base in many test cycles, but it poses a risk for upcoming feature development. > > Anyway, the code is now readable to me, I see no glaring mac80211 or > cfg80211 interfacing errors, so I guess you can work the rest in > mainline. > > johannes > Thanks for the feedback. These are all valid points and as such deserve our attention. Arend _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
