| Message ID | 20201028142433.18501-1-kitakar@gmail.com |
|---|---|
| Headers | show |
| Series | mwifiex: disable ps_mode by default for stability | expand |
On Wed, Oct 28, 2020 at 2:56 PM Tsuchiya Yuto <kitakar@gmail.com> 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. 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. 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 <kitakar@gmail.com>
On Wed, Oct 28, 2020 at 9:13 PM Brian Norris <briannorris@chromium.org> wrote: > > On Wed, Oct 28, 2020 at 2:56 PM Tsuchiya Yuto <kitakar@gmail.com> 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 some form of access control, not something that makes power control "easier"? What exactly is the use case. Also, module params in networking devices are discouraged.
On Thu, Oct 29, 2020 at 11:37 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > And this feeling (that it's a FW issue) what I have. But the problem > here, that Marvell didn't fix and probably won't fix their FW... Sure, I wouldn't hold your breath. So some of these tactics (disabling PS, etc.) may be valid, but you have to do them smartly, acknowledging that there are other (more stable) firmwares and chips in use for this same driver. > Just wondering if Google (and MS in their turn) use different > firmwares to what we have available in Linux. No clue about MS. But Chrom{e,ium} OS generally publishes all this stuff where possible. You can see what we use here: https://chromium.googlesource.com/chromiumos/third_party/linux-firmware/+/HEAD/mrvl/ https://chromium.googlesource.com/chromiumos/third_party/marvell/+/HEAD/ We try to stay somewhat in sync / parallel with "upstream" linux-firmware, and strongly encourage vendors to send the same binaries upstream when they hand them to us, but there are exceptions and oversights (e.g., old products might have used a different firmware branch). Notably, I'll repeat: we (Chrome OS) don't actually support the PCIe variant of 8897, so the report in question ("PCIe-88W8897") has no equivalent in a supported Chrome OS system (even if there are binaries in the links above, we don't use them). I would not be surprised if there are an enormous number of firmware bugs there, as there were initially for PCIe-88W8997 (which we do support). Brian
On Wed, 2020-10-28 at 15:04 -0700, Brian Norris wrote: > On Wed, Oct 28, 2020 at 2:56 PM Tsuchiya Yuto <kitakar@gmail.com> 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 <kitakar@gmail.com>
On Thu, 2020-10-29 at 13:04 -0400, Willem de Bruijn wrote: > On Wed, Oct 28, 2020 at 9:13 PM Brian Norris <briannorris@chromium.org> wrote: > > > > On Wed, Oct 28, 2020 at 2:56 PM Tsuchiya Yuto <kitakar@gmail.com> 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 some form of access control, not something that makes > power control "easier"? What exactly is the use case. Thanks for the review! As I replied to Brian, 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. So, the commit message is misleading. What will be "easier" is letting userspace tools know power_save should be off, not the procedure of toggling ps_mode state in the driver. > Also, module params in networking devices are discouraged. Even though it should be avoided, some upstream drivers provide a module parameter like this to let users enable it if needed (since they disable power_save by default because of stability on some devices) likw this commit 0172b0292649 ("iwlagn: add power_save module parameter").
On Wed, 2020-10-28 at 17:21 +0200, Andy Shevchenko wrote: > On Wed, Oct 28, 2020 at 11:24:30PM +0900, Tsuchiya Yuto wrote: > > Hello all, > > > > On Microsoft Surface devices (PCIe-88W8897), we are observing stability > > issues when ps_mode (IEEE power_save) is enabled, then eventually causes > > firmware crash. Especially on 5GHz APs, the connection is completely > > unstable and almost unusable. > > > > I think the most desirable change is to fix the ps_mode itself. But is > > seems to be hard work [1], I'm afraid we have to go this way. > > > > Therefore, the first patch of this series disables the ps_mode by default > > instead of enabling it on driver init. I'm not sure if explicitly > > disabling it is really required or not. I don't have access to the details > > of this chip. Let me know if it's enough to just remove the code that > > enables ps_mode. > > > > The Second patch adds a new module parameter named "allow_ps_mode". Since > > other wifi drivers just disable power_save by default by module parameter > > like this, I also added this. > > > > The third patch adds a message when ps_mode will be changed. Useful when > > diagnosing connection issues. > > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=109681 > > Can you attach this to the actual patch as BugLink: tag? > Thanks! Indeed I should have added this... I wrote it in the replies. If I send the v2 version of this series, I'll add it to them.
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. -- With Best Regards, Andy Shevchenko
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.
Hello! Please CC me in future for mwifiex discussion :-) On Wednesday 28 October 2020 23:24:30 Tsuchiya Yuto wrote: > Hello all, > > On Microsoft Surface devices (PCIe-88W8897), we are observing stability > issues when ps_mode (IEEE power_save) is enabled, then eventually causes > firmware crash. Especially on 5GHz APs, the connection is completely > unstable and almost unusable. > > I think the most desirable change is to fix the ps_mode itself. But is > seems to be hard work [1], I'm afraid we have to go this way. > > Therefore, the first patch of this series disables the ps_mode by default > instead of enabling it on driver init. I'm not sure if explicitly > disabling it is really required or not. I don't have access to the details > of this chip. Let me know if it's enough to just remove the code that > enables ps_mode. > > The Second patch adds a new module parameter named "allow_ps_mode". Since > other wifi drivers just disable power_save by default by module parameter > like this, I also added this. > > The third patch adds a message when ps_mode will be changed. Useful when > diagnosing connection issues. There are more issues with power save API and implementation in mwifiex. See my email for more details: https://lore.kernel.org/linux-wireless/20200609111544.v7u5ort3yk4s7coy@pali/T/#u These patches would just break power save API and reporting status to userspace even more due to WIPHY_FLAG_PS_ON_BY_DEFAULT and CONFIG_CFG80211_DEFAULT_PS options. I would suggest to first fix issues mentioned in my email and then start providing a way how to blacklist or whitelist power save feature depending on firmware or card/chip version. From my experience I know that e.g. 88W8997 cards have lot of bugs in their firmware and I'm not aware that bugs are going to be fixed... So we really need workarounds, like disabling power save mode to have cards usable. > Thanks, > Tsuchiya Yuto > > [1] https://bugzilla.kernel.org/show_bug.cgi?id=109681 > > Tsuchiya Yuto (3): > mwifiex: disable ps_mode explicitly by default instead > mwifiex: add allow_ps_mode module parameter > mwifiex: print message when changing ps_mode > > .../net/wireless/marvell/mwifiex/cfg80211.c | 23 +++++++++++++++++++ > .../net/wireless/marvell/mwifiex/sta_cmd.c | 11 ++++++--- > 2 files changed, 31 insertions(+), 3 deletions(-) > > -- > 2.29.1 >