Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <af05af0f8f88c42af486bbc6cd5412e3f97e1369.camel@sipsolutions.net>
Subject: Re: [PATCHv4] wifi: mac80211: Mesh Fast xmit support
From:   Johannes Berg <johannes@sipsolutions.net>
To:     "Sriram R (QUIC)" <quic_srirrama@quicinc.com>,
        "nbd@nbd.name" <nbd@nbd.name>
Cc:     "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Date:   Mon, 05 Sep 2022 11:32:46 +0200
In-Reply-To: <SN6PR02MB4334A54FCCA1F46A0FF4CC5FF77C9@SN6PR02MB4334.namprd02.prod.outlook.com>
References: <20220818070542.15870-1-quic_srirrama@quicinc.com>
         <5e0b07409a8c30f61ebe514d31d77e4564b86258.camel@sipsolutions.net>
         <SN6PR02MB4334A54FCCA1F46A0FF4CC5FF77C9@SN6PR02MB4334.namprd02.prod.outlook.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Sun, 2022-09-04 at 03:14 +0000, Sriram R (QUIC) wrote:
> >=20
> > Though I've actually thought for a while now that we have so many capab=
ility
> > checks, we might benefit from making those static keys for systems that=
 only
> > have a single wifi NIC (or multiple similar ones), which is quite many =
I guess.
> Sure, we could check for fast-xmit cap. But, I added 'enabled' so as to a=
void
> accessing local->hw in all these functions and also in case if we need a
> selective enable/disable of this cache in future.

Right, that's what I meant wrt. cache locality - I guess we can keep it
this way. Though we probably check local so often I'm not sure it
matters, and if we ever do revive the static keys idea (I did have a
patch at some point), it'd be better with the flag check.

> > > +     if (key) {
> > > +             hdr =3D (struct ieee80211_hdr *)mhdr->hdr;
> > > +             hdr->frame_control |=3D cpu_to_le16(IEEE80211_FCTL_PROT=
ECTED);
> > > +     }
> >=20
> > Isn't "if (key)" equivalent to "if (pn_offs)" at this point, so you can=
 move it into
> > the same if?
> The pn_offs will be set only on IEEE80211_KEY_FLAG_GENERATE_IV is set in =
key->conf.flags
> So had separate checks.

Ack.

> > > +     /* availability of hdr entry ensures the mpath and sta are vali=
d
> > > +      * hence validation can be avoided.
> >=20
> > What about key? :)

> Hmm.. I was relying on the availability of cache entry to ensure sta and =
key are valid.

And I thought you're right - but you didn't mention it in the comment,
so I wanted to double-check.

> But I see your point and realize the entry gets removed later in __sta_in=
fo_destroy_part2()
> whereas the key can get cleared before the entry is flushed.
> May be I'll see if I could flush the cache entry in __sta_info_destroy_pa=
rt1() so that
> the entry itself is not visible. Hope that solves if such race occurs.
>=20

Alright, let's see. But you can also remove a key without removing a
STA, so maybe there's some flushing needed there too?

johannes