From nobody Sat Dec 28 02:26:04 2024 X-Original-To: freebsd-wireless@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4YKmSr6hYYz5jldj for ; Sat, 28 Dec 2024 02:26:20 +0000 (UTC) (envelope-from adrian.chadd@gmail.com) Received: from mail-lf1-f43.google.com (mail-lf1-f43.google.com [209.85.167.43]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "WR4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4YKmSr4Z6nz4Lkn; Sat, 28 Dec 2024 02:26:20 +0000 (UTC) (envelope-from adrian.chadd@gmail.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-lf1-f43.google.com with SMTP id 2adb3069b0e04-53df6322ea7so10390087e87.0; Fri, 27 Dec 2024 18:26:20 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1735352777; x=1735957577; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=kGe3VqkZIqTfW5SPqwNz4MAtg5kkP91R6NA6VAzC/2A=; b=b3F/YcCWS0jnBF8r6v7mRoaNvczhBIYICo6ur9fq/OcuqTUsqF21MbprpdNrwbgdgm S+Q0cXTK5kHwCBy2tIyhBRy+ZFHSIjv+HzoRFAcOM3gXFJ8LAV2oWnAI/HNC435dcuYp Mb4SDI0tnWslN751cZ4Ts3gGEjcyb94jWhls8PzdAcuYotoLe8/swVmoPXgO6bhG7UW+ ugzDIJCGnOaQlzH2XLfXGvR1FIb0xeW/arquS0zFt+e7kV/mnDlppZc0EWvNe/Vq8bQz YobUjsBOxWx7EK6v8CpBoZLw+uIL5c9jHNqnWmHNyEXCZUvJidA1APUXqUNwOlBQOwFv sX2A== X-Gm-Message-State: AOJu0Yx0UQZkEOxrFt1M6iiihvtW5FuW1Rir73X4hOEpdq3cNA5Aug9k qMNFUndPp8JRkWVD626EhA6Ac2/OYaC4wLmY9aT04yaYKH1h0pP3nWoZoiTVkfcQnBLylCCbY6h PfB+dNlyUy4z7be+GZnDUkIPg/+rmkA== X-Gm-Gg: ASbGncv2vJ0eErbcj1NuFAmO1FqxgbypaGZ0QfcTbk+61c7BjmkPz6K437sc6RguYnO dO0WvsXGEcNJRrUlrDVeH1iFx4BZN62mJxMGpP7A= X-Google-Smtp-Source: AGHT+IFj5hWqOlIQ86zOJuPqML9XEhNMDvxghGrYd7/bU3Tnx2clPc+QWPWAjhA+hMKIsGN9zFKGFLtQeYrqQr02O+E= X-Received: by 2002:a05:6512:1055:b0:542:232a:7b2c with SMTP id 2adb3069b0e04-54229548e22mr8320566e87.29.1735352777055; Fri, 27 Dec 2024 18:26:17 -0800 (PST) List-Id: Discussions List-Archive: https://lists.freebsd.org/archives/freebsd-wireless List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: freebsd-wireless@freebsd.org Sender: owner-freebsd-wireless@FreeBSD.org MIME-Version: 1.0 References: <322ss8n1-1334-8s6q-85s9-80nsns73724q@SerrOFQ.bet> In-Reply-To: <322ss8n1-1334-8s6q-85s9-80nsns73724q@SerrOFQ.bet> From: Adrian Chadd Date: Fri, 27 Dec 2024 18:26:04 -0800 Message-ID: Subject: Re: net80211 channel cleanup - IEEE80211_IS_CHAN_DEFINED() / IEEE80211_IS_CHAN_ANYC() To: "Bjoern A. Zeeb" Cc: freebsd-wireless Content-Type: multipart/alternative; boundary="000000000000d62745062a4b4d22" X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; TAGGED_FROM(0.00)[]; ASN(0.00)[asn:15169, ipnet:209.85.128.0/17, country:US] X-Rspamd-Queue-Id: 4YKmSr4Z6nz4Lkn X-Spamd-Bar: ---- --000000000000d62745062a4b4d22 Content-Type: text/plain; charset="UTF-8" On Thu, 26 Dec 2024 at 22:58, Bjoern A. Zeeb 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


On Thu, 26 Dec = 2024 at 22:58, Bjoern A. Zeeb <bz@free= bsd.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 h= andle
> 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 o= f those
> cases:
>
> https://reviews.freebsd.org/D48172
>
> Specifically:
>
> * IEEE80211_IS_CHAN_DEFINED() returns true if the channel isn't nu= ll; and

I am sceptical about this one; see further below.


> * IEEE80211_IS_CHAN_ANYC() returns true if the channel is the "an= y 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!)=C2=A0
=

=C2=A0
> There are a bunch of places in net80211 and sys/dev that manually chec= k 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<= br> 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.=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
issues upfront.=C2=A0 I would wonder if most could go away or should me mad= e
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 d= irect pointer comparisons/arithmetic.

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.

I= 'm hoping though with some future channel representation work we just m= ove away entirely from channel pointers and
we use something like= the channel contexts that mac80211 uses, and then the "channel is def= ined" 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 cove= red but
they fit even less into this scheme.

Wh= ich ones? I want to make sure those get covered too.



-adrian
=C2=A0
=C2=A0<= /div>

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