Message-ID: <12fce295c582793e662ca322b0d3ae3b461ddb24.camel@sipsolutions.net>
Subject: Re: [RFC v2 5/6] mac80211_hwsim: add TPC per packet support
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Jonas Jelonek <jelonek.jonas@gmail.com>
Cc:     linux-wireless@vger.kernel.org, Felix Fietkau <nbd@nbd.name>
Date:   Tue, 28 Feb 2023 18:44:03 +0100
In-Reply-To: <03B7A24C-D6E2-4DBB-B52D-6174539BB781@gmail.com>
References: <20220920104032.496697-1-jelonek.jonas@gmail.com>
         <20220920104032.496697-6-jelonek.jonas@gmail.com>
         <5d15d193c76dcc2cbd59b26912973e3bce34c776.camel@sipsolutions.net>
         <03B7A24C-D6E2-4DBB-B52D-6174539BB781@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.3 (3.46.3-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Thu, 2023-01-26 at 17:53 +0100, Jonas Jelonek wrote:
> >=20
> > On Tue, 2022-09-20 at 12:40 +0200, Jonas Jelonek wrote:
> > > @@ -4846,16 +4989,32 @@ static int
> > > hwsim_tx_info_frame_received_nl(struct sk_buff *skb_2,
> > >=20
> > > tx_attempts =3D (struct hwsim_tx_rate *)nla_data(
> > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0info->attrs[HWSIM_ATTR_TX_INFO]);
> > > + tx_attempts_flags =3D (struct hwsim_tx_rate_flag *)nla_data(
> > > +     info->attrs[HWSIM_ATTR_TX_INFO_FLAGS]);
> > > + sta =3D (struct ieee80211_sta *)txi->rate_driver_data[1];
> >=20
> > That seems dangerous - what if the STA was freed already? You don't
> > walk
> > the pending list or something if the STA goes away.
>=20
> Yes, I see. Is it in general a bad idea to take the sta reference from
> ieee80211_control, put
> it in rate_driver_data and use it for tx-status? I guess I should pass
> sta to tx_status_ext whenever
> possible because it is used for several statistics.

Well you have to think about the lifetime. In most cases you do a lookup
of the STA (under RCU) etc. but=20

> I could think of two ways:
> - add NULL checks for the case that the sta pointer might be freed as
> you said

How would that pointer even go NULL though? The pointer would remain,
but the STA can be freed, no?

> - get sta by using, e.g., sta_info_get_by_addrs to get the sta if it
> is available. However, this always
> loops through the sta list. Might be a performance issue?

It should use the hashtable?

> Or do you suggest something different?

Well you could keep it here and walk the list of queued skbs (?) when a
STA is removed, and kill them all at that point, or something. Not sure
it's worth it vs. the hash table lookup, this is just hwsim after all.

johannes