MIME-Version: 1.0
References: <1548937297-14660-1-git-send-email-yhchuang@realtek.com>
 <1548937297-14660-2-git-send-email-yhchuang@realtek.com> <20190209030756.GB163159@google.com>
 <F7CD281DE3E379468C6D07993EA72F84D17A2A22@RTITMBSVM04.realtek.com.tw>
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D17A2A22@RTITMBSVM04.realtek.com.tw>
From:   Brian Norris <briannorris@chromium.org>
Date:   Mon, 11 Feb 2019 11:21:40 -0800
Message-ID: <CA+ASDXP5PCDnQoLXVnCkjyRoM31DbieusJa214_Km_BTiZMPkA@mail.gmail.com>
Subject: Re: [PATCH 01/24] rtw88: report correct tx status if mac80211
 requested one
To:     Tony Chuang <yhchuang@realtek.com>
Cc:     "kvalo@codeaurora.org" <kvalo@codeaurora.org>,
        "Larry.Finger@lwfinger.net" <Larry.Finger@lwfinger.net>,
        Andy Huang <tehuang@realtek.com>,
        "sgruszka@redhat.com" <sgruszka@redhat.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Sun, Feb 10, 2019 at 8:31 PM Tony Chuang <yhchuang@realtek.com> wrote:
> To fix `iw wlan0 station dump` display, I think I can just restore one line
> in pci.c. That is, restore IEEE80211_TX_STAT_ACK flag line:
>
> +                       continue;
> +               }
>                 ieee80211_tx_info_clear_status(info);
> -               info->flags |= IEEE80211_TX_STAT_ACK;
>                 ieee80211_tx_status_irqsafe(hw, skb)
>
> And with some modifications, such as IEEE80211_TX_CTL_NO_ACK check.
> Then we can better reporting ACK status for data frames without
> IEEE80211_TX_CTL_REQ_TX_STATUS. This way we can also ensure the
> connection monitor can work. (but it will be no loss)

This seems like a small improvement. It means you will almost never
actually report a "drop", but that's still probably better than
*always* reporting drops I think. I also see that there are handful of
older (likely poorly-maintained? or perhaps similarly-constrained)
drivers that have a similar default behavior -- they report ACK by
default, apparently without any particular notice that the packet was
actually ACKed by the receiver.

But I'm not really a mac80211 expert, so someone else may have better
ideas here.

> It's not buggy I think, if firmware is not reporting status, something must
> go wrong. And after some test I know why you feel it's unreliable.
>
> For WOW implementation, we modified a lot in fw.c functions.
> And correct some driver-firmware interface behaviors. To make sure the
> firmware is running as expected. But the patches are still holding in my hand.
> I can attach them in this patch set, and apparently I should. I will separate
> them out of WOW patch set and resend again.

Ah, well I don't know at what point we should cut things off, but I'll
stick to my stated opinion from previously: all bugfixes will help
someone like me. It's tough to differentiate firmware bugs from driver
bugs, especially when I have absolutely no firmware documentation :)

BTW, while we're at it: is this still a reasonable firmware to reference?

https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/commit/?id=338684a0c7760644031483311464c7cf5b3aac94
rtw88: Add firmware file for driver rtw88

> I think we should keep this feature. Because we actually can report status,
> despite not for every packet. The only problem is when we use `iw wlan0
> station dump` we could get *no* packet loss (like I've mentioned above,
> report TX_STAT_ACK for every other packets not have
> IEEE80211_TX_CTL_REQ_TX_STATUS). We cannot accurately report
> everything by firmware report, it takes too many tx bandwidth, and
> the performance will degrade severely. If we really cannot accept reporting

Yeah, it does seem like it would be pretty heavyweight to do this C2H
reporting for every frame.

> tx status this way, we need to find another way to solve it. That means I
> need some time to investigate and test connect monitor system and get
> a better report logic if we drop the REPORTS_TX_ACK_STATUS feature.
> Or if you have a point of view, we can discuss about it.
> Thanks!

Regards,
Brian