Date: Mon, 27 Jan 2025 08:14:59 -0800 From: Adrian Chadd <adrian.chadd@gmail.com> To: "Bjoern A. Zeeb" <bz@freebsd.org> Cc: FreeBSD wireless mailing list <wireless@freebsd.org> Subject: Re: ieee80211_rx_stats cleanup discussion Message-ID: <CAJ-Vmo=2Zh_JsH6fy6daNNDdxTtJgpxoe0OAYXTCE1BHMJ1D7w@mail.gmail.com> In-Reply-To: <r6974on7-5871-72q1-0p9p-0o0rsr01901n@SerrOFQ.bet>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --] On Mon, 27 Jan 2025 at 07:58, Bjoern A. Zeeb <bz@freebsd.org> wrote: > Hi, > > before more drivers are growing this (LinuxKPI is currently thus I > noticed) beyond the two rtwn chipsets: > > sys/dev/rtwn/rtl8192c/r92c_rx.c: rxs->c_phytype = > IEEE80211_RX_FP_11B; > sys/dev/rtwn/rtl8192c/r92c_rx.c: rxs->c_phytype = > IEEE80211_RX_FP_11G; > sys/dev/rtwn/rtl8192c/r92c_rx.c: rxs->c_phytype = > IEEE80211_RX_FP_11NG; > sys/dev/rtwn/rtl8812a/r12a_rx.c: rxs->c_phytype = > IEEE80211_RX_FP_11B; > sys/dev/rtwn/rtl8812a/r12a_rx.c: > rxs->c_phytype = IEEE80211_RX_FP_11A; > sys/dev/rtwn/rtl8812a/r12a_rx.c: > rxs->c_phytype = IEEE80211_RX_FP_11G; > sys/dev/rtwn/rtl8812a/r12a_rx.c: > rxs->c_phytype = IEEE80211_RX_FP_11NA; > sys/dev/rtwn/rtl8812a/r12a_rx.c: > rxs->c_phytype = IEEE80211_RX_FP_11NG; > sys/dev/rtwn/rtl8812a/r12a_rx.c: rxs->c_phytype = > IEEE80211_RX_FP_11NA; > > These flags are kind-of redundant and along with > > #define IEEE80211_RX_F_CCK 0x00001000 > #define IEEE80211_RX_F_OFDM 0x00002000 > > Then we would also have the band in c_band avail given we know > it for the current c_phytype. > The reason for the split between band and phytype was because way back when I found some NICs didn't give us all the receive frame info, especially in offload scenarios. We may only get back something like an 802.3 encapsulated frame with no rate info, but we know what channel it was received on. > > I would suggest (as it really simplifies the logic in drivers) to do > these conversions once centrally if we really need them for anything > and simply report the encoding up insetad of what currently is > c_phytype, that is legacy, HT, VHT, .. as that removes a lot of > redundancy and confusion. > > And we kind-of have that already as well given we have: > #define IEEE80211_RX_F_HT 0x00004000 > #define IEEE80211_RX_F_VHT 0x00008000 > > So there's redundancy. I want to understand why? The c_phytype also > has no notion of AC but then again we do have the VHT flag. > It likely doesn't have a VHT flag because we haven't added one yet. :-) > > (I am not even going to mention what happens if c_band, c_freq, c_ieee, > the c_pktflags and the current c_phytype would not agree). > Ah, band, freq, and ieee. If you're lucky, and most of the NICs we play with are "normal" nowdays, there's going to be a nice line-up between frequency, mode and ieee number. However in the past (and maybe future, we'll see) people made custom NICs that operated on other frequencies. They'd take a 2GHz or 5GHz NIC and they'd add a down/up converter and filter so they could operate on things like 3GHz, 1.2GHz, 900MHz, etc. If you look at our regdomain / code you'll see some 900MHz conversion support for older NICs in there. In that case, ieee, freq and band don't always line up. Then, you have the problem with half/quarter rates and the allowable frequencies. This is where I started trying to clean it up (eg by making c_freq khz instead of mhz) and life got in the way. Specifically, the older atheros NICs didn't allow you to set arbitrary frequencies, they had to be on very specific boundaries. So the half/quarter channel gaps weren't ACTUALLY what they said they were.) (I documented the details in the radio code for the AR5212/AR9280 HAL). But the AR9820 and later radios allow you to tune to a KHz boundary because they used a fractional synthesiser. Which meant until I err, "fixed" the AR9280 RF programming code to interoperate, they didn't interoperate. ifconfig and all the programming said one thing, but the spectrum analyser said something completely different. So, my goal here was to be able to capture if a NIC is operating on a frequency that was "off" from what was programmed, so we could at least diagnose what the heck was going on. It /may/ make sense to also capture information from later NICs that can report when the centre frequency is off due to doppler effects (eg if you're moving), since some (eg rtwn) can report the offsets. However that should be a different field. > It's these things which in parts make net80211 sometimes hard to figure > out because there are three ways to express something at times and it's > not clear at all which ones would be needed (especially if not yet > used). > I/we should go add some documentation around these and try to get them consistent. I do agree that for the default cases we should have err, 'sensible defaults' pop up so drivers don't have to set /everything exactly correct/. > > I would really love us to clean this up before it's being used for > anything to avoid further future work. > > There's a lot of "nice to have for reporting" but currently dead code there > which seems to need a bit more tought... > > /bz > > -- > Bjoern A. Zeeb r15:7 > > [-- Attachment #2 --] <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Mon, 27 Jan 2025 at 07:58, Bjoern A. Zeeb <<a href="mailto:bz@freebsd.org">bz@freebsd.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br> <br> before more drivers are growing this (LinuxKPI is currently thus I<br> noticed) beyond the two rtwn chipsets:<br> <br> sys/dev/rtwn/rtl8192c/r92c_rx.c: rxs->c_phytype = IEEE80211_RX_FP_11B;<br> sys/dev/rtwn/rtl8192c/r92c_rx.c: rxs->c_phytype = IEEE80211_RX_FP_11G;<br> sys/dev/rtwn/rtl8192c/r92c_rx.c: rxs->c_phytype = IEEE80211_RX_FP_11NG;<br> sys/dev/rtwn/rtl8812a/r12a_rx.c: rxs->c_phytype = IEEE80211_RX_FP_11B;<br> sys/dev/rtwn/rtl8812a/r12a_rx.c: rxs->c_phytype = IEEE80211_RX_FP_11A;<br> sys/dev/rtwn/rtl8812a/r12a_rx.c: rxs->c_phytype = IEEE80211_RX_FP_11G;<br> sys/dev/rtwn/rtl8812a/r12a_rx.c: rxs->c_phytype = IEEE80211_RX_FP_11NA;<br> sys/dev/rtwn/rtl8812a/r12a_rx.c: rxs->c_phytype = IEEE80211_RX_FP_11NG;<br> sys/dev/rtwn/rtl8812a/r12a_rx.c: rxs->c_phytype = IEEE80211_RX_FP_11NA;<br> <br> These flags are kind-of redundant and along with<br> <br> #define IEEE80211_RX_F_CCK 0x00001000<br> #define IEEE80211_RX_F_OFDM 0x00002000<br> <br> Then we would also have the band in c_band avail given we know<br> it for the current c_phytype.<br></blockquote><div><br></div><div>The reason for the split between band and phytype was because way back when I found some NICs didn't</div><div>give us all the receive frame info, especially in offload scenarios. We may only get back something like an</div><div>802.3 encapsulated frame with no rate info, but we know what channel it was received on.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> I would suggest (as it really simplifies the logic in drivers) to do<br> these conversions once centrally if we really need them for anything<br> and simply report the encoding up insetad of what currently is<br> c_phytype, that is legacy, HT, VHT, .. as that removes a lot of<br> redundancy and confusion.<br> <br> And we kind-of have that already as well given we have:<br> #define IEEE80211_RX_F_HT 0x00004000<br> #define IEEE80211_RX_F_VHT 0x00008000<br> <br> So there's redundancy. I want to understand why? The c_phytype also<br> has no notion of AC but then again we do have the VHT flag.<br></blockquote><div><br></div><div>It likely doesn't have a VHT flag because we haven't added one yet. :-)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> (I am not even going to mention what happens if c_band, c_freq, c_ieee,<br> the c_pktflags and the current c_phytype would not agree).<br></blockquote><div><br></div><div>Ah, band, freq, and ieee. If you're lucky, and most of the NICs we play with are</div><div>"normal" nowdays, there's going to be a nice line-up between frequency, mode</div><div>and ieee number.</div><div><br></div><div>However in the past (and maybe future, we'll see) people made custom NICs that operated</div><div>on other frequencies. They'd take a 2GHz or 5GHz NIC and they'd add a down/up converter</div><div>and filter so they could operate on things like 3GHz, 1.2GHz, 900MHz, etc. If you look at our</div><div>regdomain / code you'll see some 900MHz conversion support for older NICs in there.</div><div><br></div><div>In that case, ieee, freq and band don't always line up.</div><div><br></div><div>Then, you have the problem with half/quarter rates and the allowable frequencies.</div><div>This is where I started trying to clean it up (eg by making c_freq khz instead of mhz)</div><div>and life got in the way. Specifically, the older atheros NICs didn't allow you to set arbitrary</div><div>frequencies, they had to be on very specific boundaries. So the half/quarter channel</div><div>gaps weren't ACTUALLY what they said they were.)</div><div><br></div><div>(I documented the details in the radio code for the AR5212/AR9280 HAL).</div><div><br></div><div>But the AR9820 and later radios allow you to tune to a KHz boundary because they used</div><div>a fractional synthesiser.</div><div><br></div><div>Which meant until I err, "fixed" the AR9280 RF programming code to interoperate, they</div><div>didn't interoperate. ifconfig and all the programming said one thing, but the spectrum analyser</div><div>said something completely different.</div><div><br></div><div>So, my goal here was to be able to capture if a NIC is operating on a frequency that was "off"</div><div>from what was programmed, so we could at least diagnose what the heck was going on.</div><div><br></div><div>It /may/ make sense to also capture information from later NICs that can report when the</div><div>centre frequency is off due to doppler effects (eg if you're moving), since some (eg rtwn)</div><div>can report the offsets. However that should be a different field.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> It's these things which in parts make net80211 sometimes hard to figure<br> out because there are three ways to express something at times and it's<br> not clear at all which ones would be needed (especially if not yet<br> used).<br></blockquote><div><br></div><div>I/we should go add some documentation around these and try to get them consistent.</div><div>I do agree that for the default cases we should have err, 'sensible defaults' pop up so</div><div>drivers don't have to set /everything exactly correct/.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> I would really love us to clean this up before it's being used for<br> anything to avoid further future work.<br> <br> There's a lot of "nice to have for reporting" but currently dead code there<br> which seems to need a bit more tought...<br> <br> /bz<br> <br> -- <br> Bjoern A. Zeeb r15:7<br> <br> </blockquote></div></div>home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-Vmo=2Zh_JsH6fy6daNNDdxTtJgpxoe0OAYXTCE1BHMJ1D7w>