To make the ps_mode (power_save) control easier, this commit adds a new
module parameter allow_ps_mode and set it false (disallowed) by default.
When this parameter is set to false, changing the power_save mode will
be disallowed like the following:
$ sudo iw dev mlan0 set power_save on
command failed: Operation not permitted (-1)
Signed-off-by: Tsuchiya Yuto <[email protected]>
---
drivers/net/wireless/marvell/mwifiex/cfg80211.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index a6b9dc6700b14..943bc1e8ceaee 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -25,6 +25,11 @@
static char *reg_alpha2;
module_param(reg_alpha2, charp, 0);
+static bool allow_ps_mode;
+module_param(allow_ps_mode, bool, 0644);
+MODULE_PARM_DESC(allow_ps_mode,
+ "allow WiFi power management to be enabled. (default: disallowed)");
+
static const struct ieee80211_iface_limit mwifiex_ap_sta_limits[] = {
{
.max = MWIFIEX_MAX_BSS_NUM,
@@ -435,6 +440,17 @@ mwifiex_cfg80211_set_power_mgmt(struct wiphy *wiphy,
ps_mode = enabled;
+ /* Allow ps_mode to be enabled only when allow_ps_mode is true */
+ if (ps_mode && !allow_ps_mode) {
+ mwifiex_dbg(priv->adapter, MSG,
+ "Enabling ps_mode disallowed by modparam\n");
+
+ /* Return -EPERM to inform userspace tools that setting
+ * power_save to be enabled is not permitted.
+ */
+ return -EPERM;
+ }
+
return mwifiex_drv_set_power(priv, &ps_mode);
}
--
2.29.1
On Wed, 2020-10-28 at 15:04 -0700, Brian Norris wrote:
> On Wed, Oct 28, 2020 at 2:56 PM Tsuchiya Yuto <[email protected]> wrote:
> >
> > To make the ps_mode (power_save) control easier, this commit adds a new
> > module parameter allow_ps_mode and set it false (disallowed) by default.
>
> This sounds like a bad idea, as it breaks all the existing users who
> expect this feature to be allowed. Seems like you should flip the
> defaults. Without some better justification, NACK.
Thanks for the review! I wanted to open a discussion widely and wanted
to ask from the upstream developers the direction of how this stability
issue should be resolved.
I added the link to the Bugzilla in the cover-letter (that should have
arrived on the mailing list now), but I should have added this to every
commit as well:
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=109681
This stability issue exists for a long time. I also submitted there the
required kernel log and device_dump more than three months ago. However,
unfortunately, it's not fixed yet. So, I have to send a series like this.
If we know that the power_save feature is broken (on some devices), I
think it should be fixed in either firmware or driver for the affected
devices. It makes no sense to keep enabling the broken features by
default.
Because userspace tools sometimes try to enable power_save anyway
regardless of default driver settings (expecting it's not broken, but
in fact it's broken), the module parameter like this is required in
addition to the first patch of this series. The commit 8298383c2cd5
("ath9k: Do not support PowerSave by default") also does the same thing
for this purpose.
On the other hand, I agree that I don't want to break the existing users.
As you mentioned in the reply to the first patch, I can set the default
value of this parameter depending on the chip id (88W8897) or DMI matching.
> Also, I can't find the other 2 patches in this alleged series. Maybe
> they're still making it through the mailing lists and archives.
Yes, there seems to be a problem with the mailing list at the time.
All the other patches I sent have arrived by now.
> Brian
>
> > When this parameter is set to false, changing the power_save mode will
> > be disallowed like the following:
> >
> > $ sudo iw dev mlan0 set power_save on
> > command failed: Operation not permitted (-1)
> >
> > Signed-off-by: Tsuchiya Yuto <[email protected]>
On Fri, 2020-10-30 at 13:02 +0200, Andy Shevchenko wrote:
> On Fri, Oct 30, 2020 at 04:58:33PM +0900, Tsuchiya Yuto wrote:
> > On Wed, 2020-10-28 at 15:04 -0700, Brian Norris wrote:
>
> ...
>
> > On the other hand, I agree that I don't want to break the existing users.
> > As you mentioned in the reply to the first patch, I can set the default
> > value of this parameter depending on the chip id (88W8897) or DMI matching.
>
> Since it's a PCIe device you already have ID table where you may add a
> driver_data with what ever quirks are needed.
Sorry that my comment was misleading. I meant using the quirk framework
(that is based on DMI matching) I sent in another series. This applies
to the other replies from me.
However, thanks to your comment, I remembered that currently, the quirk
framework can be used only within pcie.c file. For example, the quirk
initialization is currently done in pcie.c file. The mwifiex driver is
divided into interface-specific modules (PCIe, SDIO, USB) (e.g.,
mwifiex_pcie module for PCIe interface) + common module (mwifiex module).
So, I need to extend the quirk framework so that it can be used by the
mwifiex module globally.
I'll make a v2 version of this series with using the updated quirk
framework so that it won't change behaviors for existing users.