Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
Subject: Re: [PATCH v2] mac80211: fix regression in sta connection monitor
From:   Georgi Valkov <gvalkov@abv.bg>
In-Reply-To: <3762d906-31ff-8386-b9ea-d7b06ea07397@nbd.name>
Date:   Sat, 26 Sep 2020 10:31:42 +0300
Cc:     linux-wireless@vger.kernel.org, johannes@sipsolutions.net,
        openwrt-devel@lists.openwrt.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <E4065653-11EF-4E3C-8767-2F9789609026@abv.bg>
References: <20200922102434.42727-1-nbd@nbd.name>
 <FD88F09D-5FEA-4A15-B586-95843830F737@abv.bg>
 <3762d906-31ff-8386-b9ea-d7b06ea07397@nbd.name>
To:     Felix Fietkau <nbd@nbd.name>
Precedence: bulk


> thanks for testing and for your insight into this issue.
I=E2=80=99m happy to help!

> On 2020-09-26 06:41, Georgi Valkov wrote:
>> Hi Felix!
>>=20
>> With your latest suggestion, it takes between 10 and 17 hours for the =
connection to drop, then long five minutes to reconnect.
>> Notice the order of code execution in the original code of =
ieee80211_sta_tx_notify():
>> probe_send_count is always cleared when ack is true. But before =
clearing probe_send_count, we need to check if =
ieee80211_is_any_nullfunc() returns true and probe_send_count > 0: if =
yes, we must call ieee80211_queue_work(). You cleared probe_send_count =
ahead of time, so the condition to run ieee80211_queue_work() will never =
be met.
> The condition in ieee80211_sta_tx_notify may not be met, but
> ieee80211_queue_work is called from ieee80211_tx_status_ext in that
> case. Any idea why that is not enough?

Whenever that condition is met and probe_send_count is reset there, =
ieee80211_queue_work() is called for sure. Could that be the problem? =
Because in the original code we also check the result from =
ieee80211_is_any_nullfunc(). We can also add a trace e.g. printk() to =
shine some light on the order of events, hopefully without flooding the =
kernel log too much and without impact on the performance?
You can send me patches directly without committing to master and I will =
test them.

>> To spare your time, I did spend one week to find the cause, then =
another learning every detail about the code and testing various =
solutions, including those you proposed. While I do not have experience =
with mac80211=E2=80=99s design, I=E2=80=99m quite good at preserving the =
exact behaviour during large scale refactoring. And in my fix I tried =
changing as little as possible to keep the patch small, preserving both =
your changes and the original design behaviour.
> And the reason I'm still proposing changes to it is because your patch
> does not take into account no-skb or 802.3 tx status.
> I'll try to make a v3 patch that leaves more of the original code =
intact
> at the cost of a little more duplication.
Good point. You are familiar with how it should work, and I only know =
the changes. You can add the common code into a new function to make it =
easier to follow.

I=E2=80=99m curious: the original RX code used to call =
ieee80211_sta_tx_notify(). Do we need to clear probe_send_count or do =
anything else there? I suppose you removed that because it will be =
performed later. Either way we should not touch that before the issue is =
resolved.


Georgi=