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;
Message-ID: <7db5b6cba1548308a63855ec1dda836b6d6d9757.camel@gmail.com>
Subject: Re: [PATCH] mwifiex: pcie: add enable_device_dump module parameter
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 19:30:15 +0900
In-Reply-To: <CA+ASDXPX+fadTKLnxNVZQ0CehsHNwvWHXEdLqZVDoQ6hf6Wp8Q@mail.gmail.com>
References: <20201028142625.18642-1-kitakar@gmail.com>
         <CA+ASDXPX+fadTKLnxNVZQ0CehsHNwvWHXEdLqZVDoQ6hf6Wp8Q@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 Wed, 2020-10-28 at 17:12 -0700, Brian Norris wrote:
> On Wed, Oct 28, 2020 at 3:58 PM Tsuchiya Yuto <kitakar@gmail.com> wrote:
> > 
> > The devicve_dump may take a little bit long time and users may want to
> > disable the dump for daily usage.
> > 
> > This commit adds a new module parameter enable_device_dump and disables
> > the device_dump by default.
> 
> As with one of your other patches, please don't change the defaults
> and hide them under a module parameter. If you're adding a module
> parameter, leave the default behavior alone.

Thanks for the review!

I mentioned about power_save stability on the other patches. But I should
have added this fact into the commit message of this patch that even if
we disable that power_save, the firmware crashes a lot on some specific
devices. Really a lot.

For example, as far as I know Surface Pro 5 needs ASPM L1.2 substate
disabled to avoid the firmware crash. Disabling it is still acceptable.
On the other hand, Surface 3 needs L1 ASPM state disabled. This is not
acceptable because this breaks S0ix. Anyway, handling ASPM should be done
in firmware I think.

So, the context of why I sent this patch is the next. We can't fix the
fw crash itself, so, we decided to just let it crash and reset by itself
(with the other fw reset quirks I sent). In this way, the time it does
device_dump is really annoying if fw crashes so often.

Let me know if splitting this patch like this works. 1) The first patch
is to add this module parameter but don't change the default behavior.
2) The second patch is to change the parameter value depending on the
DMI matching or something so that it doesn't break the existing users.

But what I want to say here as well is that, if the firmware can be fixed,
we don't need a patch like this.

> This also seems like something that might be nicer as a user-space
> knob in generic form (similar to "/sys/class/devcoredump/disabled",
> except on a per-device basis, and fed back to the driver so it doesn't
> waste time generating such dumps), but I suppose I can see why a
> module parameter (so you can just stick your configuration in
> /etc/modprobe.d/) might be easier to deal with in some cases.

Agreed.

> Brian