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>
Subject: Re: [RFC v2 5/6] mac80211_hwsim: add TPC per packet support
Date: Tue, 28 Feb 2023 18:44:03 +0100	[thread overview]
Message-ID: <12fce295c582793e662ca322b0d3ae3b461ddb24.camel@sipsolutions.net> (raw)
In-Reply-To: <03B7A24C-D6E2-4DBB-B52D-6174539BB781@gmail.com>

On Thu, 2023-01-26 at 17:53 +0100, Jonas Jelonek wrote:
> > 
> > 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,
> > > 
> > > tx_attempts = (struct hwsim_tx_rate *)nla_data(
> > >       info->attrs[HWSIM_ATTR_TX_INFO]);
> > > + tx_attempts_flags = (struct hwsim_tx_rate_flag *)nla_data(
> > > +     info->attrs[HWSIM_ATTR_TX_INFO_FLAGS]);
> > > + sta = (struct ieee80211_sta *)txi->rate_driver_data[1];
> > 
> > That seems dangerous - what if the STA was freed already? You don't
> > walk
> > the pending list or something if the STA goes away.
> 
> 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 

> 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