Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
MIME-Version: 1.0
References: <20230920112259.18656-1-dmantipov@yandex.ru> <ZQt89BPMN6Fg3H6z@google.com>
 <cc57fb2f-1eca-f366-bec3-d4cdbb2c39ec@yandex.ru> <87o7hvafo8.fsf@kernel.org>
In-Reply-To: <87o7hvafo8.fsf@kernel.org>
From:   Brian Norris <briannorris@chromium.org>
Date:   Fri, 22 Sep 2023 18:39:46 -0700
Message-ID: <CA+ASDXNu-CWa9V5Oc-01pLp72iMVRw7m46+bDdNUxVmfrR9PSQ@mail.gmail.com>
Subject: Re: [PATCH] wifi: mwifiex: fix SDIO firmware dump wait
To:     Kalle Valo <kvalo@kernel.org>
Cc:     Dmitry Antipov <dmantipov@yandex.ru>,
        linux-wireless@vger.kernel.org, lvc-project@linuxtesting.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Sep 21, 2023 at 5:12=E2=80=AFAM 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