[PATCH] wifi: mwifiex: fix SDIO firmware dump wait

[PATCH] wifi: mwifiex: fix SDIO firmware dump wait

[Dmitry Antipov]

In 'mwifiex_sdio_generic_fw_dump()', move (presumably placed
by mistake) 'if (tries == MAX_POLL_TRIES)' check to an outer
waiting loop.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/net/wireless/marvell/mwifiex/sdio.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 774858cfe86f..98f16eb8f298 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -2899,12 +2899,12 @@ static void mwifiex_sdio_generic_fw_dump(struct mwifiex_adapter *adapter)
 			}
 			if (start_flag == 0)
 				break;
-			if (tries == MAX_POLL_TRIES) {
-				mwifiex_dbg(adapter, ERROR,
-					    "FW not ready to dump\n");
-				ret = -1;
-				goto done;
-			}
+		}
+		if (tries == MAX_POLL_TRIES) {
+			mwifiex_dbg(adapter, ERROR,
+				    "FW not ready to dump\n");
+			ret = -1;
+			goto done;
 		}
 		usleep_range(100, 200);
 	}
-- 
2.41.0

Re: [PATCH] wifi: mwifiex: fix SDIO firmware dump wait

[Brian Norris]

On Wed, Sep 20, 2023 at 02:22:32PM +0300, Dmitry Antipov wrote:
> In 'mwifiex_sdio_generic_fw_dump()', move (presumably placed
> by mistake) 'if (tries == MAX_POLL_TRIES)' check to an outer
> waiting loop.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>

Have you tested this patch? You've certainly caught a logic bug, but
that doesn't mean the seemingly obvious solution actually works.

(Disclaimer: I haven't tested it.)

Re: [PATCH] wifi: mwifiex: fix SDIO firmware dump wait

[Dmitry Antipov]

On 9/21/23 02:15, Brian Norris wrote:

> Have you tested this patch? You've certainly caught a logic bug, but
> that doesn't mean the seemingly obvious solution actually works.

Unfortunately by eyes only :-(. IIUC there should be a weird hardware
stall to trigger an execution of the branch in subject, so I'm not sure
how to actually test it even if I would have an access to the hardware.

Dmitry

Re: [PATCH] wifi: mwifiex: fix SDIO firmware dump wait

[Kalle Valo]

Dmitry Antipov <dmantipov@yandex.ru> writes:

> On 9/21/23 02:15, Brian Norris wrote:
>
>> Have you tested this patch? You've certainly caught a logic bug, but
>> that doesn't mean the seemingly obvious solution actually works.
>
> Unfortunately by eyes only :-(. IIUC there should be a weird hardware
> stall to trigger an execution of the branch in subject, so I'm not sure
> how to actually test it even if I would have an access to the hardware.

I don't know about Brian but for me testing for regressions is the most
important part. If the patch is only compile tested it could break the
whole driver without anyone noticing. And then it's in a release and too
late.

That's why I have been asking you to add "Compile tested only" to the
commit log so that it's obvious to everyone that your patches have
received zero testing but you don't seem to care.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wifi: mwifiex: fix SDIO firmware dump wait

[Brian Norris]

On Thu, Sep 21, 2023 at 5:12 AM Kalle Valo <kvalo@kernel.org> wrote:
>
> Dmitry Antipov <dmantipov@yandex.ru> writes:
>
> > On 9/21/23 02:15, Brian Norris wrote:
> >
> >> Have you tested this patch? You've certainly caught a logic bug, but
> >> that doesn't mean the seemingly obvious solution actually works.
> >
> > Unfortunately by eyes only :-(. IIUC there should be a weird hardware
> > stall to trigger an execution of the branch in subject, so I'm not sure
> > how to actually test it even if I would have an access to the hardware.

If you had the hardware, you could at least test the positive case --
that the "normally operating" case at least succeeds without hitting
the retry/timeout loop failure. What if it's common for this "start
flag" to be stuck non-zero, but we were ignoring it due to the bug
(after a timeout), and things still worked fine? You rarely can trust
that driver authors got their hardware/firmware bits correct as
written, just because the driver logic suggests it should be so...

Or if you're just interested in testing the firmware dump: these
drivers have a debugfs mechanism for triggering firmware dumps on
demand. You don't need to actually crash the WiFi firmware.

> I don't know about Brian but for me testing for regressions is the most
> important part. If the patch is only compile tested it could break the
> whole driver without anyone noticing. And then it's in a release and too
> late.

I might make a similar claim, but context-dependent. Certain kinds of
patches are clear refactorings, and can reasonably be verified with
static analysis. But many patches have a moderate or substantial
runtime impact, and those are very important to test. In any case, it
can even be difficult to judge the difference between the two types,
so it's pretty fair to err on the side of "if it isn't run tested, it
isn't worth merging" if you'd like.

> That's why I have been asking you to add "Compile tested only" to the
> commit log so that it's obvious to everyone that your patches have
> received zero testing but you don't seem to care.

Yeah, that'd be nice. By now, I've pattern-matched the author though,
so my question was more rhetorical ("because I'm sure you haven't
tested the patch, I don't feel inclined to Ack it").

Brian