Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.400.51.1.1\))
Subject: Re: [RFC v2 5/6] mac80211_hwsim: add TPC per packet support
From:   Jonas Jelonek <jelonek.jonas@gmail.com>
In-Reply-To: <12fce295c582793e662ca322b0d3ae3b461ddb24.camel@sipsolutions.net>
Date:   Fri, 3 Mar 2023 08:46:49 +0100
Cc:     linux-wireless@vger.kernel.org, Felix Fietkau <nbd@nbd.name>
Content-Transfer-Encoding: quoted-printable
Message-Id: <BE583386-CD66-4A60-B0AC-6E88968288D7@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>
 <12fce295c582793e662ca322b0d3ae3b461ddb24.camel@sipsolutions.net>
To:     Johannes Berg <johannes@sipsolutions.net>
Precedence: bulk


> On 28. Feb 2023, at 18:44, Johannes Berg <johannes@sipsolutions.net> =
wrote:
>=20
> 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(
>>>>     info->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.
>=20
> Well you have to think about the lifetime. In most cases you do a =
lookup
> of the STA (under RCU) etc. but=20
>=20
>> I could think of two ways:
>> - add NULL checks for the case that the sta pointer might be freed as
>> you said
>=20
> How would that pointer even go NULL though? The pointer would remain,
> but the STA can be freed, no?

Yes, you=E2=80=99re right. Sorry, I mixed that up somehow.

>=20
>> - 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?
>=20
> It should use the hashtable?
>=20
>> Or do you suggest something different?
>=20
> 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.
>=20
> johannes

That=E2=80=99s also correct. Sorry, I didn=E2=80=99t have a deeper look =
into this function at the
time of writing. So I guess I will stick to this for now.

Jonas