Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <8fa12bfff1cc30b655934e303cad78ae75b0fcde.camel@gmail.com>
Subject: Re: [PATCH 1/3] mwifiex: disable ps_mode explicitly by default
 instead
From:   Tsuchiya Yuto <kitakar@gmail.com>
To:     Brian Norris <briannorris@chromium.org>
Cc:     Amitkumar Karwar <amitkarwar@gmail.com>,
        Ganapathi Bhat <ganapathi.bhat@nxp.com>,
        Xinming Hu <huxinming820@gmail.com>,
        Kalle Valo <kvalo@codeaurora.org>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        "<netdev@vger.kernel.org>" <netdev@vger.kernel.org>,
        Linux Kernel <linux-kernel@vger.kernel.org>,
        Maximilian Luz <luzmaximilian@gmail.com>,
        Andy Shevchenko <andriy.shevchenko@intel.com>, verdre@v0yd.nl
Date:   Fri, 30 Oct 2020 17:04:34 +0900
In-Reply-To: <CA+ASDXMfuqy=kCECktP_mYm9cAapXukeLhe=1i3uPbTu9wS2Qw@mail.gmail.com>
References: <20201028142433.18501-1-kitakar@gmail.com>
         <20201028142433.18501-2-kitakar@gmail.com>
         <CA+ASDXMfuqy=kCECktP_mYm9cAapXukeLhe=1i3uPbTu9wS2Qw@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.38.1 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 2020-10-29 at 11:25 -0700, Brian Norris wrote:
> On Wed, Oct 28, 2020 at 7:04 PM Tsuchiya Yuto <kitakar@gmail.com> wrote:
> > 
> > On Microsoft Surface devices (PCIe-88W8897), the ps_mode causes
> > connection unstable, especially with 5GHz APs. Then, it eventually causes
> > fw crash.
> > 
> > This commit disables ps_mode by default instead of enabling it.
> > 
> > Required code is extracted from mwifiex_drv_set_power().
> > 
> > Signed-off-by: Tsuchiya Yuto <kitakar@gmail.com>
> 
> You should read up on WIPHY_FLAG_PS_ON_BY_DEFAULT and
> CONFIG_CFG80211_DEFAULT_PS, and set/respect those appropriately (hint:
> mwifiex sets WIPHY_FLAG_PS_ON_BY_DEFAULT, and your patch makes this a
> lie). Also, this seems like a quirk that you haven't properly worked
> out -- if you're working on a quirk framework in your other series,
> you should just key into that.

Thanks for the review! I didn't know about the flag, much appreciated.
By setting the flag to false explicitly, indeed userspace doesn't try
to enable power_save now at least for this short amount of time. I wonder
if I can drop the second patch (adding module parameter) now. But I still
want to make sure that power_save won't be enabled by userspace tools by
default.

Regarding quirks, I also don't want to break existing users. So, of course
I can try to use the quirk framework if we really can't fix the firmware.

> For the record, Chrome OS supports plenty of mwifiex systems with 8897
> (SDIO only) and 8997 (PCIe), with PS enabled, and you're hurting
> those. Your problem sounds to be exclusively a problem with the PCIe
> 8897 firmware.

Actually, I already know that some Chromebooks use these mwifiex cards
(but not out PCIe-88W8897) because I personally like chromiumos. I'm
always wondering what is the difference. If the difference is firmware,
our PCIe-88W8897 firmware should really be fixed instead of this stupid
series.

Yes, I'm sorry that I know this series is just a stupid one but I have to
send this anyway because this stability issue has not been fixed for a
long time. I should have added this buglink to every commit as well:

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=109681

If the firmware can't be fixed, I'm afraid I have to go this way. It makes
no sense to keep enabling power_save for the affected devices if we know
it's broken.

> As-is, NAK.
> 
> Brian