Date: Fri, 27 Dec 2024 18:26:04 -0800 From: Adrian Chadd <adrian@freebsd.org> To: "Bjoern A. Zeeb" <bz@freebsd.org> Cc: freebsd-wireless <freebsd-wireless@freebsd.org> Subject: Re: net80211 channel cleanup - IEEE80211_IS_CHAN_DEFINED() / IEEE80211_IS_CHAN_ANYC() Message-ID: <CAJ-Vmo=j3tyquMmZxw62zmj9%2BPVdtk%2BROGYE=MCKCEB-E=ux1Q@mail.gmail.com> In-Reply-To: <322ss8n1-1334-8s6q-85s9-80nsns73724q@SerrOFQ.bet> References: <CAJ-Vmom1vKu-S%2BpNeVme=WyE5GmGDs_=aduCTimXPYPMKfTDwQ@mail.gmail.com> <322ss8n1-1334-8s6q-85s9-80nsns73724q@SerrOFQ.bet>
next in thread | previous in thread | raw e-mail | index | archive | help
--000000000000d62745062a4b4d22 Content-Type: text/plain; charset="UTF-8" On Thu, 26 Dec 2024 at 22:58, Bjoern A. Zeeb <bz@freebsd.org> wrote: > On Sat, 21 Dec 2024, Adrian Chadd wrote: > > > hi! > > > > i've started on the bare minimum to start cleaning up the way we handle > > channels in net80211. The first part - no matter what direction we > > eventually DO go - is to try and remove all of the direct pointer > > comparisons and dereferences that are going on. > > > > So along this topic, i've created a diff to demonstrate a couple of those > > cases: > > > > https://reviews.freebsd.org/D48172 > > > > Specifically: > > > > * IEEE80211_IS_CHAN_DEFINED() returns true if the channel isn't null; and > > I am sceptical about this one; see further below. > > > > * IEEE80211_IS_CHAN_ANYC() returns true if the channel is the "any > channel" > > channel (ie IEEE80211_CHAN_ANYC). > > I would suggest to keep the IEEE80211_CHAN_ prefix and insert the IS > after that: IEEE80211_CHAN_IS_ANYC() > ok, why's that? All of the other mcaros are IEEE80211_IS_CHAN_xxx(). (I don't mind renaming it, I'm just curious why!) > > There are a bunch of places in net80211 and sys/dev that manually check > if > > the channel is NULL / the channel is/isn't ANYC. My proposal is we go > > through, find them all and convert them to these macros. > > From looking through the code I would hypothesize that if NULL checks > are not for local state most of them are either a result of copy&paste > in multi-OS-ported drivers or there as a result of crashes due to race > conditions/insufficient locking in net80211 and most should simply never > occur. Plasting them behind a IEEE80211_CHAN_IS_DEFINED (or IS_VALID > whatever you'd name it) is likely not helpful to fix the underlying > issues upfront. I would wonder if most could go away or should me made > sure to have a valid state (often coming along with the ANYC check > anyway?). > My goal here is to enumerate them in a way that's easily searchable via grep and remove all of the direct pointer comparisons/arithmetic. I do agree with you that 99% of "channel pointer is NULL" cases shouldn't exist, and those should be looked at and fixed. I'm hoping though with some future channel representation work we just move away entirely from channel pointers and we use something like the channel contexts that mac80211 uses, and then the "channel is defined" macro will go away. I'm not looking for it to be perfect, just a step in trying to make things more manageable and searchable. > There are also plain "ANY" (no C) cases which should also be covered but > they fit even less into this scheme. > Which ones? I want to make sure those get covered too. -adrian > > -- > Bjoern A. Zeeb r15:7 > --000000000000d62745062a4b4d22 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote g= mail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">On Thu, 26 Dec = 2024 at 22:58, Bjoern A. Zeeb <<a href=3D"mailto:bz@freebsd.org">bz@free= bsd.org</a>> wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"= margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-lef= t:1ex">On Sat, 21 Dec 2024, Adrian Chadd wrote:<br> <br> > hi!<br> ><br> > i've started on the bare minimum to start cleaning up the way we h= andle<br> > channels in net80211. The first part - no matter what direction we<br> > eventually DO go - is to try and remove all of the direct pointer<br> > comparisons and dereferences that are going on.<br> ><br> > So along this topic, i've created a diff to demonstrate a couple o= f those<br> > cases:<br> ><br> > <a href=3D"https://reviews.freebsd.org/D48172" rel=3D"noreferrer" targ= et=3D"_blank">https://reviews.freebsd.org/D48172</a><br>; ><br> > Specifically:<br> ><br> > * IEEE80211_IS_CHAN_DEFINED() returns true if the channel isn't nu= ll; and<br> <br> I am sceptical about this one; see further below.<br> <br> <br> > * IEEE80211_IS_CHAN_ANYC() returns true if the channel is the "an= y channel"<br> > channel (ie IEEE80211_CHAN_ANYC).<br> <br> I would suggest to keep the IEEE80211_CHAN_ prefix and insert the IS<br> after that: IEEE80211_CHAN_IS_ANYC()<br></blockquote><div><br></div><div>ok= , why's that? All of the other mcaros are IEEE80211_IS_CHAN_xxx().</div= ><div>(I don't mind renaming it, I'm just curious why!)=C2=A0</div>= <div><br></div><div>=C2=A0</div><blockquote class=3D"gmail_quote" style=3D"= margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-lef= t:1ex"> > There are a bunch of places in net80211 and sys/dev that manually chec= k if<br> > the channel is NULL / the channel is/isn't ANYC. My proposal is we= go<br> > through, find them all and convert them to these macros.<br> <br> >From looking through the code I would hypothesize that if NULL checks<br> are not for local state most of them are either a result of copy&paste<= br> in multi-OS-ported drivers or there as a result of crashes due to race<br> conditions/insufficient locking in net80211 and most should simply never<br= > occur.=C2=A0 Plasting them behind a IEEE80211_CHAN_IS_DEFINED (or IS_VALID<= br> whatever you'd name it) is likely not helpful to fix the underlying<br> issues upfront.=C2=A0 I would wonder if most could go away or should me mad= e<br> sure to have a valid state (often coming along with the ANYC check<br> anyway?).<br></blockquote><div><br></div><div>My goal here is to enumerate = them in a way that's easily searchable via grep and remove all of the d= irect pointer comparisons/arithmetic.</div><div><br></div><div>I do agree w= ith you that 99% of "channel pointer is NULL" cases shouldn't= exist, and those should be looked at and fixed.</div><div><br></div><div>I= 'm hoping though with some future channel representation work we just m= ove away entirely from channel pointers and</div><div>we use something like= the channel contexts that mac80211 uses, and then the "channel is def= ined" macro will go away.</div><div><br></div><div>I'm not looking= for it to be perfect, just a step in trying to make things more manageable= and searchable.</div><div><br></div><blockquote class=3D"gmail_quote" styl= e=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);paddin= g-left:1ex"> <br> There are also plain "ANY" (no C) cases which should also be cove= red but<br> they fit even less into this scheme.<br></blockquote><div><br></div><div>Wh= ich ones? I want to make sure those get covered too.</div><div><br></div><d= iv><br></div><div><br></div><div>-adrian</div><div>=C2=A0</div><div>=C2=A0<= /div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;bo= rder-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> -- <br> Bjoern A. Zeeb=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0r15:7<br> </blockquote></div></div> --000000000000d62745062a4b4d22--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-Vmo=j3tyquMmZxw62zmj9%2BPVdtk%2BROGYE=MCKCEB-E=ux1Q>