| Message ID | 20230920112259.18656-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: mwifiex: fix SDIO firmware dump wait | expand |
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.)
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
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.
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
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); }
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(-)