Dmitry Antipov <[email protected]> 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
On Thu, Sep 21, 2023 at 5:12 AM Kalle Valo <[email protected]> wrote:
>
> Dmitry Antipov <[email protected]> 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