On 8/5/21 11:30 AM, Willy Tarreau wrote:
On Thu, Aug 05, 2021 at 10:29:49AM -0700, Guenter Roeck wrote:
It looks like a typical "works for me" regression. The best thing that
could possibly be done to limit such occurrences would be to wait "long
enough" before backporting them, in hope to catch breakage reports before
the backport, but here there were already 3 weeks between the patch was
submitted and it was backported.
No. The patch is wrong. It just _looks_ correct at first glance.
So that's the core of the problem. Stable maintainers cannot be tasked
to try to analyse each patch in its finest details to figure whether a
maintainer that's expected to be more knowledgeable than them on their
driver did something wrong.
Then in my opinion we should encourage *not* to use "Fixes:" on untested
patches (untested patches will always happen due to hardware availability
or lack of a reliable reproducer).
What about this to try to improve the situation in this specific case ?
Willy
From ef646bae2139ba005de78940064c464126c430e6 Mon Sep 17 00:00:00 2001
From: Willy Tarreau <w@xxxxxx>
Date: Thu, 5 Aug 2021 20:24:30 +0200
Subject: docs: process: submitting-patches.rst: recommend against 'Fixes:' if
untested
'Fixes:' usually is taken as authority and often results in a backport. If
a patch couldn't be tested although it looks perfectly valid, better not
add this tag to leave a final chance for backporters to ask about the
relevance of the backport or to check for future tests.
Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
Signed-off-by: Willy Tarreau <w@xxxxxx>
---
Documentation/process/submitting-patches.rst | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index 0852bcf73630..54782b0e2f4c 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -140,6 +140,15 @@ An example call::
$ git log -1 --pretty=fixes 54a4f0239f2e
Fixes: 54a4f0239f2e ("KVM: MMU: make kvm_mmu_zap_page() return the number of pages it actually freed")
+Please note that a 'Fixes:' tag will most often result in your patch being
+automatically backported to stable branches. If for any reason you could not
+test that it really fixes the problem (for example, because the bug is not
+reproducible, or because you did not have access to the required hardware
+at the time of writing the patch to verify it does not cause regressions),
+even if you are absolutely certain of your patch's validity, do not include
+a 'Fixes:' tag and instead explain the situation in the commit message in
+plain English.
+
I don't think that would be a good idea, First, it would discourage people
from using Fixes: tags, and second it would not really solve the problem either.
While I am sure that the patch in question wasn't really tested (after all,
it broke EC communication on all Mediatek Chromebooks using SPI to communicate
with the EC), we don't know what the author tested. It may well be (and is quite
likely) that the author _did_ test the patch, only not the affected code path.
That means the author may still have added a Fixes: tag in the wrong belief
to have tested the fix, even with the above restrictions in place.
The same is true for the Bluetooth patch: This patch was for sure tested
and works on many Bluetooth devices, except for those with a specific
Mediatek controller and driver. Again, the Fixes: tag would be there
(and it would be completely unreasonable to expect that infrastructure
patches are tested on all affected hardware).
No, I think all we can do is to improve test coverage.
Thanks,
Guenter
