On Fri, Jul 17, 2020 at 12:50:04PM +0200, Holger Mikolon wrote: > I came across this by reading the code if_iwn.c and DPRINTFs on > a kernel with IWN_DEBUG. > > IWN_LSB() returns an index starting with 1, however the arrays used > later on (noise and gain in iwn5000_set_gains()) start with 0. The > current code accounts for this difference when setting the antenna > gain by accessing cmd.gain[i - 1]. However the noise array is accessed > with noise[i], the chainmask is as well checked against i and more > importantly the overall for() loop iterates wrongly over the antennas by > always starting with i=2 (the third antenna). One consequence is, that > gain calibration never happens in case of only two antennas. > > Secondly, the final DPRINTF in iwn5000_set_gains() assumes a two-antenna > setup. In my case three antennas are connected. I don't know if there > are iwn setups with one antenna, but the DPRINTF wouldn't make sense > there at all. Hence I propose to move this DPRINTF up where it makes > more sense (and adjust it to the new place). > > My diff below fixes the said off-by-one and DPRINTF. Additionally > it adds another DPRINTF which I felt useful while debugging and > it extends a comment - those additions may be skipped of course. > > Here is few details of my laptop (cvs updated and kernel built today): > > $ dmesg | grep iwn0 > iwn0 at pci2 dev 0 function 0 "Intel WiFi Link 5300" rev 0x00: msi, MIMO > 3T3R, MoW, address 00:21:6a:56:2b:36 > > $ sysctl hw | grep -e machine -e model -e vendor -e product > hw.machine=amd64 > hw.model=Intel(R) Core(TM)2 Duo CPU P8600 @ 2.40GHz > hw.vendor=Dell Inc. > hw.product=Studio 1555 > > Let me know if you need a full dmesg or anything else. >
Thanks! This is a pretty obvious fix. I have committed it and afterwards tweaked the multi-line comment you've added to conform to style(9). Something to keep in mind for future patches ;-) Regarding DPRINTFs: I try to avoid committing them into the tree because they add a lot of noise as they accumulate. And they are rarely useful outside of the context of the debugging session they were written for. Note that iwn(4) in particular contains so many DPRINTFs that just turning on IWN_DEBUG may cause the driver to miss the WPA handshake timeout while the console is being displayed (in particular if 'ifconfig iwn0 debug' is enabled, too). We have already reached the point where the many DPRINTFs prevent proper operation of this driver. I kept your DPRINTFs for now. But it would make sense to do a sweep over the entire iwn(4) driver and eliminate some or most of them. I did that for iwm(4) many years ago and never had any regrets.
