Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1568617425-28062-1-git-send-email-yhchuang@realtek.com>
 <1568617425-28062-6-git-send-email-yhchuang@realtek.com> <CA+ASDXOMao9uucGxa6C0Q99pFvAtKTptreeshbfKqiELDKUH0Q@mail.gmail.com>
 <F7CD281DE3E379468C6D07993EA72F84D18DC62E@RTITMBSVM04.realtek.com.tw>
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D18DC62E@RTITMBSVM04.realtek.com.tw>
From:   Brian Norris <briannorris@chromium.org>
Date:   Thu, 19 Sep 2019 17:35:56 -0700
Message-ID: <CA+ASDXP_7FVEaQrE1V=rX0Vieu8GGWj+it7p4F_8XeRNniEWkw@mail.gmail.com>
Subject: Re: [PATCH 05/15] rtw88: pci: release tx skbs DMAed when stop
To:     Tony Chuang <yhchuang@realtek.com>
Cc:     Kalle Valo <kvalo@codeaurora.org>,
        linux-wireless <linux-wireless@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Tue, Sep 17, 2019 at 7:10 PM Tony Chuang <yhchuang@realtek.com> wrote:
> > On Mon, Sep 16, 2019 at 12:03 AM <yhchuang@realtek.com> wrote:
> > >
> > > From: Yan-Hsuan Chuang <yhchuang@realtek.com>
> > >
> > > Interrupt is disabled to stop PCI, which means the skbs
> > > queued for each TX ring will not be released via DMA
> > > interrupt.
> >
> > In what cases do you hit this? I think you do this when entering PS
> > mode, no? But then, see below.
>
> I'll hit this when ieee80211_ops::stop, or rtw_power_off.
> Both are to turn off the device, so there's no more DMA activities.
> If we don't release the SKBs that are not released by DMA interrupt
> when powering off, these could be leaked.

Ah, I was a bit confused. So it does get called from "PS" routines:
rtw_enter_ips() -> rtw_core_stop()
but that "IPS" mode means "Inactive" Power Save, and it's only used
when transitioning into idle states (IEEE80211_CONF_IDLE).

Incidentally, I think this also may explain many of the leaks I've
been seeing elsewhere, when I leave a device sitting and scanning for
a very long time -- each scan attempt is making a single transition
out-and-back to IPS mode, which meant it may be leaking any
outstanding TX DMA. And testing confirms this: if I just bring up the
interface, run a scan, then bring it down, I see many fewer unmaps
than maps. Doing this enough times, I run out of contiguous DMA memory
and the device stops working. This fixes that problem for me. So:

Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>

I wonder if, given the problems I've seen (the driver can become
totally ineroperable), this patch and the previous patch (its only
real dependency) should be fast-tracked to the current release.

Brian