linux-kernel - Re: [PATCH] wifi: mwifiex: decrease timeout waiting for host sleep from 10s to 5s

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAD=FV=XhDdBJXfC72PZAbgaSpVx4ubtKSRFptock0SMRg=+Miw@mail.gmail.com>
Date: Wed, 27 Nov 2024 07:44:00 -0800
From: Doug Anderson <dianders@...omium.org>
To: Pin-yen Lin <treapking@...omium.org>
Cc: Francesco Dolcini <francesco@...cini.it>, Kalle Valo <kvalo@...nel.org>, 
	David Lin <yu-hao.lin@....com>, linux-kernel@...r.kernel.org, 
	linux-wireless@...r.kernel.org, Brian Norris <briannorris@...omium.org>
Subject: Re: [PATCH] wifi: mwifiex: decrease timeout waiting for host sleep
 from 10s to 5s

Hi,

On Wed, Nov 27, 2024 at 2:58 AM Pin-yen Lin <treapking@...omium.org> wrote:
>
> In commit 52250cbee7f6 ("mwifiex: use timeout variant for
> wait_event_interruptible") it was noted that sometimes we seemed
> to miss the signal that our host sleep settings took effect. A
> 10 second timeout was added to the code to make sure we didn't
> hang forever waiting. It appears that this problem still exists
> and we hit the timeout sometimes for Chromebooks in the field.
>
> Recently on ChromeOS we've started setting the DPM watchdog
> to trip if full system suspend takes over 10 seconds. Given
> the timeout in the original patch, obviously we're hitting
> the DPM watchdog before mwifiex gets a chance to timeout.
>
> While we could increase the DPM watchdog in ChromeOS to avoid
> this problem, it's probably better to simply decrease the
> timeout. Any time we're waiting several seconds for the
> firmware to respond it's likely that the firmware won't ever
> respond. With that in mind, decrease the timeout in mwifiex
> from 10 seconds to 5 seconds.
>
> Suggested-by: Doug Anderson <dianders@...omium.org>
> Signed-off-by: Pin-yen Lin <treapking@...omium.org>
> ---
>
>  drivers/net/wireless/marvell/mwifiex/sta_ioctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

I believe Brian Norris is still anointed as the personally nominally
in charge of mwifiex upstream (get_maintainer labels him as "odd"
fixer, which seems awfully judgemental), so he should be CCed on
fixes. Added him.


> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
> index e06a0622973e..f79589cafe57 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_ioctl.c
> @@ -545,7 +545,7 @@ int mwifiex_enable_hs(struct mwifiex_adapter *adapter)
>
>         if (wait_event_interruptible_timeout(adapter->hs_activate_wait_q,
>                                              adapter->hs_activate_wait_q_woken,
> -                                            (10 * HZ)) <= 0) {
> +                                            (5 * HZ)) <= 0) {

Given that I suggested this fix, it should be no surprise:

Reviewed-by: Douglas Anderson <dianders@...omium.org>

Upon sleeping on the idea, the only slight concern I have here is
whether we'll trigger this timeout if we try to suspend while WiFi
scanning is in progress. I don't have any actual evidence supporting
this concern, but I remember many years ago when I used to deal with
the WiFi drivers more often there were cases where suspend could be
delayed if it happened while a scan was in progress. Maybe/hopefully
that's fixed now, but I figured I'd at least bring it up in case it
tickled anything in someone's mind...

If somehow that turns out to be a problem, hopefully we'd be able to
find a way to cancel the scan or break scans up into smaller chunks
because even delaying suspend for 5 seconds seems like it would be a
big problem.

-Doug