2024-06-11 13:44:51

by Dan Carpenter

[permalink] [raw]
Subject: [PATCH v5] checkpatch: check for missing Fixes tags

This check looks for common words that probably indicate a patch
is a fix. For now the regex is:

(?:(?:BUG: K.|UB)SAN: |Call Trace:|stable\@|syzkaller)/)

Why are stable patches encouraged to have a fixes tag? Some people mark
their stable patches as "# 5.10" etc. This is useful but a Fixes tag is
still a good idea. For example, the Fixes tag helps in review. It
helps people to not cherry-pick buggy patches without also
cherry-picking the fix.

Also if a bug affects the 5.7 kernel some people will round it up to
5.10+ because 5.7 is not supported on kernel.org. It's possible the Bad
Binder bug was caused by this sort of gap where companies outside of
kernel.org are supporting different kernels from kernel.org.

Should it be counted as a Fix when a patch just silences harmless
WARN_ON() stack trace. Yes. Definitely.

Is silencing compiler warnings a fix? It seems unfair to the original
authors, but we use -Werror now, and warnings break the build so let's
just add Fixes tags. I tell people that silencing static checker
warnings is not a fix but the rules on this vary by subsystem.

Is fixing a minor LTP issue (Linux Test Project) a fix? Probably? It's
hard to know what to do if the LTP test has technically always been
broken.

One clear false positive from this check is when someone updated their
debug output and included before and after Call Traces. Or when crashes
are introduced deliberately for testing. In those cases, you should
just ignore checkpatch.

Signed-off-by: Dan Carpenter <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Reviewed-by: Kees Cook <[email protected]>
---
v5: Change the output:
OLD: This looks like a fix but there is no Fixes: tag
NEW: The commit message has 'stable@', perhaps it also needs a 'Fixes:' tag?
NEW: The commit message has 'syzkaller', perhaps it also needs a 'Fixes:' tag?

v4: Fix another formatting issue.
v3: Add UBSAN to the regex as Kees suggested.
v2: I fixed the formatting issues Joe pointed out. I also silenced the
warning if the commit was a Revert because revert patches already
include the hash.

scripts/checkpatch.pl | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2b812210b412..df99f48d824f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -28,6 +28,7 @@ my %verbose_messages = ();
my %verbose_emitted = ();
my $tree = 1;
my $chk_signoff = 1;
+my $chk_fixes_tag = 1;
my $chk_patch = 1;
my $tst_only;
my $emacs = 0;
@@ -88,6 +89,7 @@ Options:
-v, --verbose verbose mode
--no-tree run without a kernel tree
--no-signoff do not check for 'Signed-off-by' line
+ --no-fixes-tag do not check for 'Fixes:' tag
--patch treat FILE as patchfile (default)
--emacs emacs compile window format
--terse one line per report
@@ -295,6 +297,7 @@ GetOptions(
'v|verbose!' => \$verbose,
'tree!' => \$tree,
'signoff!' => \$chk_signoff,
+ 'fixes-tag!' => \$chk_fixes_tag,
'patch!' => \$chk_patch,
'emacs!' => \$emacs,
'terse!' => \$terse,
@@ -1257,6 +1260,7 @@ sub git_commit_info {
}

$chk_signoff = 0 if ($file);
+$chk_fixes_tag = 0 if ($file);

my @rawlines = ();
my @lines = ();
@@ -2636,6 +2640,9 @@ sub process {

our $clean = 1;
my $signoff = 0;
+ my $fixes_tag = 0;
+ my $is_revert = 0;
+ my $needs_fixes_tag = "";
my $author = '';
my $authorsignoff = 0;
my $author_sob = '';
@@ -3189,6 +3196,16 @@ sub process {
}
}

+# These indicate a bug fix
+ if (!$in_header_lines && !$is_patch &&
+ $line =~ /^This reverts commit/) {
+ $is_revert = 1;
+ }
+
+ if (!$in_header_lines && !$is_patch &&
+ $line =~ /((?:(?:BUG: K.|UB)SAN: |Call Trace:|stable\@|syzkaller))/) {
+ $needs_fixes_tag = $1;
+ }

# Check Fixes: styles is correct
if (!$in_header_lines &&
@@ -3201,6 +3218,7 @@ sub process {
my $id_length = 1;
my $id_case = 1;
my $title_has_quotes = 0;
+ $fixes_tag = 1;

if ($line =~ /(\s*fixes:?)\s+([0-9a-f]{5,})\s+($balanced_parens)/i) {
my $tag = $1;
@@ -7697,6 +7715,12 @@ sub process {
ERROR("NOT_UNIFIED_DIFF",
"Does not appear to be a unified-diff format patch\n");
}
+ if ($is_patch && $has_commit_log && $chk_fixes_tag) {
+ if ($needs_fixes_tag ne "" && !$is_revert && !$fixes_tag) {
+ WARN("MISSING_FIXES_TAG",
+ "The commit message has '$needs_fixes_tag', perhaps it also needs a 'Fixes:' tag?\n");
+ }
+ }
if ($is_patch && $has_commit_log && $chk_signoff) {
if ($signoff == 0) {
ERROR("MISSING_SIGN_OFF",


2024-06-11 13:55:48

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH v5] checkpatch: check for missing Fixes tags

Btw, I re-checked this script against patches from v6.9-rc2..v6.9 and
here are the patches it warned about. Just looking at it and thinking
about how to determine where all these patches are supposed to be
applied? What a nightmare.

I didn't find anything I would consider a "false positive" but I think
people have started tagging patch dependencies as stable patches. For
example:
3b80cff5a4d1 ("io_uring/kbuf: get rid of bl->is_ready")
fb9f8125ed9d ("ASoC: SOF: Add dsp_max_burst_size_in_ms member to snd_sof_pcm_stream")

I don't think this is required. I believe that if the stable patch is
part of a patchset then the stable scripts will pull in the whole
patchset? Or if it depends on a patch which has already been committed
then you have to add:
Cc: <[email protected]> # 6.8.x: fb9f8125ed9d "ASoC: SOF: Add dsp_max_burst_size_in_ms member to snd_sof_pcm_stream"

regards,
dan carpenter

b436f1cbed9c ("drm/amd/display: Fix idle optimization checks for multi-display and dual eDP")
d5887dc6b6c0 ("nvme-pci: Add quirk for broken MSIs")
c66b8356273c ("drm/i915/audio: Fix audio time stamp programming for DP")
71dac2482ad3 ("bcachefs: BCH_SB_LAYOUT_SIZE_BITS_MAX")
8060bf1d83f7 ("bcachefs: Fix snapshot_t() usage in bch2_fs_quota_read_inode()")
0ec5b3b7ccfc ("bcachefs: Fix shift-by-64 in bformat_needs_redo()")
2bb9600d5d47 ("bcachefs: Guard against unknown k.k->type in __bkey_invalid()")
f39055220f6f ("bcachefs: Add missing validation for superblock section clean")
6b8cbfc3db75 ("bcachefs: Fix assert in bch2_alloc_v4_invalid()")
db42549d402c ("bcachefs: Add a better limit for maximum number of buckets")
1267df40acb2 ("bcachefs: Initialize bch_write_op->failed in inline data path")
4a8521b6bb81 ("bcachefs: Inodes need extra padding for varint_decode_fast()")
b30b70ad8bff ("bcachefs: Fix early error path in bch2_fs_btree_key_cache_exit()")
a2ddaf965f6a ("bcachefs: bucket_pos_to_bp_noerror()")
7ffec9ccdc6a ("bcachefs: don't free error pointers")
4efaa5acf0a1 ("epoll: be better about file lifetimes")
691aae4f36f9 ("ksmbd: do not grant v2 lease if parent lease key and epoch are not set")
d1c189c6cb8b ("ksmbd: use rwsem instead of rwlock for lease break")
97c2ec64667b ("ksmbd: avoid to send duplicate lease break notifications")
cc00bc83f26e ("ksmbd: off ipv6only for both ipv4/ipv6 binding")
e03418abde87 ("btrfs: make sure that WRITTEN is set on all metadata blocks")
02b670c1f88e ("x86/mm: Remove broken vsyscall emulation code from the page fault code")
892b41b16f61 ("drm/amd/display: Fix incorrect DSC instance for MST")
719564737a9a ("drm/amd/display: Handle Y carry-over in VCP X.Y calculation")
6f572a805457 ("drm/nouveau/gsp: Use the sg allocator for level 2 of radix3")
52a6947bf576 ("drm/nouveau/firmware: Fix SG_DEBUG error with nvkm_firmware_ctor()")
00e7d3bea2ce ("dyndbg: fix old BUG_ON in >control parser")
2d5af3ab9e6f ("ALSA: hda/realtek: Fix mute led of HP Laptop 15-da3001TU")
1dd1eff161bd ("softirq: Fix suspicious RCU usage in __do_softirq()")
7af2ae1b1531 ("erofs: reliably distinguish block based and fscache mode")
8861fd518047 ("smb3: fix lock ordering potential deadlock in cifs_sync_mid_result")
8094a600245e ("smb3: missing lock when picking channel")
6e159fd653d7 ("ethernet: Add helper for assigning packet type when dest address does not match device address")
0f2b8098d72a ("btrfs: take the cleaner_mutex earlier in qgroup disable")
d1a5a7eede29 ("Bluetooth: btusb: Add Realtek RTL8852BE support ID 0x0bda:0x4853")
9bf4e919ccad ("Bluetooth: Fix type of len in {l2cap,sco}_sock_getsockopt_old()")
4108a30f1097 ("mei: me: add lunar lake point M DID")
9792b7cc18aa ("drm/amdgpu/sdma5.2: use legacy HDP flush for SDMA2/3")
661d71ee5a01 ("drm/amdgpu/umsch: don't execute umsch test when GPU is in reset/suspend")
aebd3eb9d3ae ("drm/amdgpu: Assign correct bits for SDMA HDP flush")
0e95ed6452cb ("drm/amdgpu/pm: Remove gpu_od if it's an empty directory")
25e9227c6afd ("drm/amdgpu: Fix leak when GPU memory allocation fails")
4a237d55446f ("usb: xhci-plat: Don't include xhci.h")
4973b04d3ea5 ("ksmbd: clear RENAME_NOREPLACE before calling vfs_rename")
17cf0c2794bd ("ksmbd: validate request buffer size in smb2_allocate_rsp_buf()")
c119f4ede3fa ("ksmbd: fix slab-out-of-bounds in smb2_allocate_rsp_buf")
7ee5faad0f8c ("ALSA: hda/realtek - Enable audio jacks of Haier Boyue G42 with ALC269VC")
2f7ef5bb4a2f ("btrfs: fix information leak in btrfs_ioctl_logical_to_ino()")
582ee2f9d268 ("USB: serial: option: add Telit FN920C04 rmnet compositions")
7caf3daaaf04 ("ALSA: hda/realtek: Add quirks for Huawei Matebook D14 NBLB-WAX9N")
311f97a4c7c2 ("USB: serial: option: add Rolling RW101-GL and RW135-GL support")
cf16ffa17c39 ("USB: serial: option: add Lonsung U8300/U9300 product")
f74ab0c5e594 ("ALSA: hda/tas2781: Add new vendor_id and subsystem_id to support ThinkPad ICE-1")
fb1f4584b121 ("USB: serial: option: add support for Fibocom FM650/FG650")
1f737846aa3c ("mm/shmem: inline shmem_is_huge() for disabled transparent hugepages")
9253c54e01b6 ("Squashfs: check the inode number is not the invalid value of zero")
b914ec33b391 ("clk: sunxi-ng: common: Support minimum and maximum rate")
c840244aba7a ("USB: serial: option: support Quectel EM060K sub-models")
356952b13af5 ("USB: serial: option: add Fibocom FM135-GL variants")
3b0daecfeac0 ("amdkfd: use calloc instead of kzalloc to avoid integer overflow")
40e0ee6338f0 ("KVM: selftests: Add coverage of EPT-disabled to vmx_dirty_log_test")
fd706c9b1674 ("KVM: x86: Snapshot if a vCPU's vendor model is AMD vs. Intel compatible")
1a629fe4cca0 ("LoongArch: Make virt_addr_valid()/__virt_addr_valid() work with KFENCE")
f87cbcb345d0 ("timekeeping: Use READ/WRITE_ONCE() for tick_do_timer_cpu")
dcd12acaf384 ("thunderbolt: Avoid notify PM core about runtime PM resume")
c38fa07dc69f ("thunderbolt: Fix wake configurations after device unplug")
6dba20d23e85 ("drm/amdgpu: differentiate external rev id for gfx 11.5.0")
e047dd448d2b ("drm/amd/display: Adjust dprefclk by down spread percentage.")
c3e2a5f2da90 ("drm/amd/display: Set VSC SDP Colorimetry same way for MST and SST")
9e61ef8d2198 ("drm/amd/display: Program VSC SDP colorimetry for all DP sinks >= 1.4")
cf79814cb0bf ("drm/amd/display: fix disable otg wa logic in DCN316")
953927587f37 ("drm/amd/display: Do not recursively call manual trigger programming")
81901d8d0472 ("drm/amd/display: always reset ODM mode in context when adding first plane")
bbca7f414ae9 ("drm/amdgpu: fix incorrect number of active RBs for gfx11")
2cc69a10d831 ("drm/amd/display: Return max resolution supported by DWB")
ecedd99a9369 ("drm/amd/display: Skip on writeback when it's not applicable")
8b2be55f4d6c ("drm/amdgpu: Reset dGPU if suspend got aborted")
0f1bbcc2bab2 ("drm/amdgpu/umsch: reinitialize write pointer in hw init")
65ff8092e480 ("drm/amdgpu: always force full reset for SOC21")
8bdfb4ea95ca ("drm/amdkfd: Reset GPU on queue preemption failure")
7a87441c9651 ("nfc: llcp: fix nfc_llcp_setsockopt() unsafe copies")
21c9fb611c25 ("mtd: diskonchip: work around ubsan link failure")
de120e1d692d ("KVM: x86/pmu: Set enable bits for GP counters in PERF_GLOBAL_CTRL at "RESET"")
ab9177d83c04 ("wifi: mac80211: don't use rate mask for scanning")
dcd8992e47f1 ("drm/i915/vrr: Disable VRR when using bigjoiner")
4a36e46df7aa ("drm/i915: Disable live M/N updates when using bigjoiner")
0653d501409e ("drm/i915: Disable port sync when bigjoiner is used")
e3d4ead4d48c ("drm/i915/psr: Disable PSR when bigjoiner is used")
6154cc9177cc ("drm/i915/cdclk: Fix voltage_level programming edge case")
3eadd887dbac ("drm/client: Fully protect modes[] with dev->mode_config.mutex")
5ce344beaca6 ("x86/apic: Force native_apic_mem_read() to use the MOV instruction")
beaa51b36012 ("blk-iocost: avoid out of bounds shift")
3ddf944b32f8 ("x86/mce: Make sure to grab mce_sysfs_mutex in set_bank()")
d920a2ed8620 ("usb: Disable USB3 LPM at shutdown")
6334b8e4553c ("usb: gadget: f_ncm: Fix UAF ncm object at re-bind after usb ep transport error")
99485c4c026f ("x86/coco: Require seeding RNG with RDRAND on CoCo systems")
e0e50401cc39 ("smb: client: fix potential UAF in cifs_signal_cifsd_for_reconnect()")
63981561ffd2 ("smb: client: fix potential UAF in smb2_is_network_name_deleted()")
69ccf040acdd ("smb: client: fix potential UAF in is_valid_oplock_break()")
22863485a462 ("smb: client: fix potential UAF in smb2_is_valid_oplock_break()")
705c76fbf726 ("smb: client: fix potential UAF in smb2_is_valid_lease_break()")
0865ffefea19 ("smb: client: fix potential UAF in cifs_stats_proc_show()")
d3da25c5ac84 ("smb: client: fix potential UAF in cifs_stats_proc_write()")
58acd1f49716 ("smb: client: fix potential UAF in cifs_dump_full_key()")
ca545b7f0823 ("smb: client: fix potential UAF in cifs_debug_files_proc_show()")
173217bd7336 ("smb3: retrying on failed server close")
378ca2d2ad41 ("s390/entry: align system call table on 8 bytes")
0e60f0b75884 ("xtensa: fix MAKE_PC_FROM_RA second argument")
6b69c4ab4f68 ("io_uring/kbuf: protect io_buffer_list teardown with a reference")
3b80cff5a4d1 ("io_uring/kbuf: get rid of bl->is_ready")
09ab7eff3820 ("io_uring/kbuf: get rid of lower BGID lists")
93cee45ccfeb ("smb: client: serialise cifs_construct_tcon() with cifs_mount_mutex")
4a5ba0e0bfe5 ("smb: client: handle DFS tcons in cifs_construct_tcon()")
0a05ad21d77a ("smb: client: refresh referral without acquiring refpath_lock")
062a7f0ff46e ("smb: client: guarantee refcounted children from parent session")
5ed11af19e56 ("ksmbd: do not set SMB2_GLOBAL_CAP_ENCRYPTION for SMB 3.1.1")
a677ebd8ca2f ("ksmbd: validate payload size in ipc response")
c1832f67035d ("ksmbd: don't send oplock break if rename fails")
73eaa2b58349 ("io_uring: use private workqueue for exit work")
c4e51e424e2c ("ALSA: line6: Zero-initialize message buffers")
0bfe105018bd ("ALSA: hda/realtek: cs35l41: Support ASUS ROG G634JYR")
b67a7dc418aa ("ALSA: hda/realtek: Add sound quirks for Lenovo Legion slim 7 16ARHA7 models")
24a9799aa8ef ("smb: client: fix UAF in smb2_reconnect_server()")
bee1d5becdf5 ("io_uring: disable io-wq execution of multishot NOWAIT requests")
c569242cd492 ("Bluetooth: hci_event: set the conn encrypted before conn establishes")
daf6c4681a74 ("ALSA: hda/realtek - Fix inactive headset mic jack")
a1aa5390cc91 ("of: module: prevent NULL pointer dereference in vsnprintf()")
0462c56c290a ("driver core: Introduce device_link_wait_removal()")
1abc2642588e ("ASoC: SOF: Intel: hda: Compensate LLP in case it is not reset")
f9eeb6bb13fb ("ALSA: hda: Add pplcllpl/u members to hdac_ext_stream")
0ea06680dfcb ("ASoC: SOF: ipc4-pcm: Correct the delay calculation")
77165bd955d5 ("ASoC: SOF: sof-pcm: Add pointer callback to sof_ipc_pcm_ops")
3ce3bc36d915 ("ASoC: SOF: ipc4-pcm: Invalidate the stream_start_offset in PAUSED state")
55ca6ca227bf ("ASoC: SOF: ipc4-pcm: Combine the SOF_IPC4_PIPE_PAUSED cases in pcm_trigger")
31d2874d083b ("ASoC: SOF: ipc4-pcm: Move struct sof_ipc4_timestamp_info definition locally")
07007b8ac42c ("ASoC: SOF: Remove the get_stream_position callback")
4ab6c38c6644 ("ASoC: SOF: Intel: hda-common-ops: Do not set the get_stream_position callback")
37679a1bd372 ("ASoC: SOF: ipc4-pcm: Use the snd_sof_pcm_get_dai_frame_counter() for pcm_delay")
fd6f6a0632bc ("ASoC: SOF: Intel: Set the dai/host get frame/byte counter callbacks")
ce2faa9a180c ("ASoC: SOF: Introduce a new callback pair to be used for PCM delay reporting")
4374f698d7d9 ("ASoC: SOF: Intel: mtl/lnl: Use the generic get_stream_position callback")
67b182bea08a ("ASoC: SOF: Intel: hda: Implement get_stream_position (Linear Link Position)")
fe76d2e75a6d ("ASoC: SOF: Intel: hda-pcm: Use dsp_max_burst_size_in_ms to place constraint")
842bb8b62cc6 ("ASoC: SOF: ipc4-topology: Save the DMA maximum burst size for PCMs")
fb9f8125ed9d ("ASoC: SOF: Add dsp_max_burst_size_in_ms member to snd_sof_pcm_stream")

2024-06-11 18:39:08

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH v5] checkpatch: check for missing Fixes tags

On Tue, 11 Jun 2024 16:43:29 +0300 Dan Carpenter <[email protected]> wrote:

> This check looks for common words that probably indicate a patch
> is a fix. For now the regex is:
>
> (?:(?:BUG: K.|UB)SAN: |Call Trace:|stable\@|syzkaller)/)
>
> Why are stable patches encouraged to have a fixes tag? Some people mark
> their stable patches as "# 5.10" etc. This is useful but a Fixes tag is
> still a good idea.

I'd say that "# 5.10" is lame and it would be good if checkpatch could
detect this and warn "hey, use a proper Fixes:". Because

> It helps people to not cherry-pick buggy patches without also
> cherry-picking the fix.

seems pretty important.



2024-06-12 06:48:14

by Thorsten Leemhuis

[permalink] [raw]
Subject: Re: [PATCH v5] checkpatch: check for missing Fixes tags

On 11.06.24 20:38, Andrew Morton wrote:
> On Tue, 11 Jun 2024 16:43:29 +0300 Dan Carpenter <[email protected]> wrote:
>
>> This check looks for common words that probably indicate a patch
>> is a fix. For now the regex is:
>>
>> (?:(?:BUG: K.|UB)SAN: |Call Trace:|stable\@|syzkaller)/)
>>
>> Why are stable patches encouraged to have a fixes tag? Some people mark
>> their stable patches as "# 5.10" etc. This is useful but a Fixes tag is
>> still a good idea.
>
> I'd say that "# 5.10" is lame

Documentation/process/stable-kernel-rules.rst documents this use to
"Point out kernel version prerequisites".

> and it would be good if checkpatch could
> detect this and warn "hey, use a proper Fixes:". Because
>
>> It helps people to not cherry-pick buggy patches without also
>> cherry-picking the fix.
>
> seems pretty important.

Hmmm. That would lead to false positive when it comes to changes that
for example just add a device ID (and thus do not "Fix" anything) while
having prerequisites that are only available in a specific version.

Ciao, Thorsten

2024-06-12 08:50:12

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH v5] checkpatch: check for missing Fixes tags

On Wed, Jun 12, 2024 at 08:46:24AM +0200, Thorsten Leemhuis wrote:
> On 11.06.24 20:38, Andrew Morton wrote:
> > On Tue, 11 Jun 2024 16:43:29 +0300 Dan Carpenter <[email protected]> wrote:
> >
> >> This check looks for common words that probably indicate a patch
> >> is a fix. For now the regex is:
> >>
> >> (?:(?:BUG: K.|UB)SAN: |Call Trace:|stable\@|syzkaller)/)
> >>
> >> Why are stable patches encouraged to have a fixes tag? Some people mark
> >> their stable patches as "# 5.10" etc. This is useful but a Fixes tag is
> >> still a good idea.
> >
> > I'd say that "# 5.10" is lame
>
> Documentation/process/stable-kernel-rules.rst documents this use to
> "Point out kernel version prerequisites".
>

No, the 5.10 means that the fix is required for everything after 5.10.
Here is how you reference pre-requisites.

Cc: <[email protected]> # 3.3.x: a1f84a3: sched: Check for idle
Cc: <[email protected]> # 3.3.x: 1b9508f: sched: Rate-limit newidle
Cc: <[email protected]> # 3.3.x: fd21073: sched: Fix affinity logic

The documentation was written before we went to 12 character hashes and
also these days we normally put ("") around the subject. I've made a
copy of all the uses of this format from 2023 at the bottom of this
email to see how people use it in real life.

> > and it would be good if checkpatch could
> > detect this and warn "hey, use a proper Fixes:". Because
> >
> >> It helps people to not cherry-pick buggy patches without also
> >> cherry-picking the fix.
> >
> > seems pretty important.
>
> Hmmm. That would lead to false positive when it comes to changes that
> for example just add a device ID (and thus do not "Fix" anything) while
> having prerequisites that are only available in a specific version.

What I'm saying is, imagine you are maintaining a distro kernel for
10 years. In this scenario you're pulling in whole new wifi drivers
so the kernel still runs on modern hardware. The stable tag says
"apply this to 6.8+" because that's when the driver was merged. But as
a distro maintainer it's much nicer to have a Fixes: 123412341234 ("Add
new wifi driver").

regards,
dan carpenter

Dependencies listed in 2023:

Cc: <[email protected]> # 6.1.x: 3837a03 serial: sc16is7xx: improve regmap debugfs by using one regmap per port
Cc: <[email protected]> # 6.1.x: 3837a03 serial: sc16is7xx: improve regmap debugfs by using one regmap per port
Cc: <[email protected]> # 6.6+: f8ff234: kernel/Kconfig.kexec: drop select of KEXEC for CRASH_DUMP
Cc: <[email protected]> # v6.0+: 1da5c9b x86: Introduce ia32_enabled()
Cc: [email protected] # 6.6.x: c5dbf0416000: platform/x86: hp-bioscfg: Simplify return check in hp_add_other_attributes()
Cc: [email protected] # 6.6.x: 5736aa9537c9: platform/x86: hp-bioscfg: move mutex_lock() down in hp_add_other_attributes()
Cc: <[email protected]> # selftests/resctrl: Refactor feature check to use resource and feature name
Cc: <[email protected]> # selftests/resctrl: Remove duplicate feature check from CMT test
Cc: <[email protected]> # selftests/resctrl: Move _GNU_SOURCE define into Makefile
Cc: [email protected] # 5.9.x: 09252177d5f9: SUNRPC: Handle major timeout in xprt_adjust_timeout()
Cc: [email protected] # 5.9.x: 7de62bc09fe6: SUNRPC dont update timeout value on connection reset
Cc: [email protected] # 0b035401c570: rbd: move rbd_dev_refresh() definition
Cc: [email protected] # 510a7330c82a: rbd: decouple header read-in from updating rbd_dev->header
Cc: [email protected] # c10311776f0a: rbd: decouple parent info read-in from updating rbd_dev
Cc: <[email protected]> # 6.1.y: bf0207e172703 ("drm/amdgpu: add S/G display parameter")
Cc: <[email protected]> # 6.1.y: bf0207e172703 ("drm/amdgpu: add S/G display parameter")
Cc: <[email protected]> # 5.15.x: 60a0aab7463ee69 arm64: module-plts: inline linux/moduleloader.h
Cc: [email protected] # 588159009d5b: rbd: retrieve and check lock owner twice before blocklisting
Cc: [email protected] # f38cb9d9c204: rbd: make get_lock_owner_info() return a single locker or NULL
Cc: [email protected] # 8ff2c64c9765: rbd: harden get_lock_owner_info() a bit
Cc: 6.4+ <[email protected]> # 6.4+: 8bcbb18c61d6: thermal: core: constify params in thermal_zone_device_register
Cc: [email protected] # please backport to all LTSes but not before v6.6-rc2 is tagged
Cc: [email protected] # 3.18: a872ab303d5d: "usb: dwc3: qcom: fix use-after-free on runtime-PM wakeup"
Cc: [email protected] # v6.0+ 2f38e84 net/ncsi: make one oem_gma function for all mfr id
Cc: <[email protected]> # 6.0: 5365cea199c7 ("soc: qcom: llcc: Rename reg_offset structs to reflect LLCC version")
Cc: <[email protected]> # 6.0: c13d7d261e36 ("soc: qcom: llcc: Pass LLCC version based register offsets to EDAC driver")
Cc: [email protected] # 6.1.y: 5591a051b86b: drm/amdgpu: refine get gpu clock counter method
Cc: [email protected] # 6.2.y: 5591a051b86b: drm/amdgpu: refine get gpu clock counter method
Cc: [email protected] # 6.3.y: 5591a051b86b: drm/amdgpu: refine get gpu clock counter method
Cc: [email protected] #3.2: 30332eeefec8: debugfs: regset32: Add Runtime PM support
Cc: <[email protected]> # dependency for "drm/rockchip: vop: Leave
Cc: [email protected] # 4.15: 30332eeefec8: debugfs: regset32: Add Runtime PM support
Cc: <[email protected]> # v5.19+ (if someone else does the backport)
CC: [email protected] # 5.4.x: c8a5f8ca9a9c: btrfs: print checksum type and implementation at mount time
Cc: <[email protected]> # d6fd48eff750 ("virt/coco/sev-guest: Check SEV_SNP attribute at probe time")
Cc: <[email protected]> # 970ab823743f (" virt/coco/sev-guest: Simplify extended guest request handling")
Cc: <[email protected]> # c5a338274bdb ("virt/coco/sev-guest: Remove the disable_vmpck label in handle_guest_request()")
Cc: <[email protected]> # 0fdb6cc7c89c ("virt/coco/sev-guest: Carve out the request issuing logic into a helper")
Cc: <[email protected]> # d25bae7dc7b0 ("virt/coco/sev-guest: Do some code style cleanups")
Cc: <[email protected]> # fa4ae42cc60a ("virt/coco/sev-guest: Convert the sw_exit_info_2 checking to a switch-case")
Cc: [email protected] # 5.1: 680f8666baf6: interconnect: Make icc_provider_del() return void
Cc: <[email protected]> # 2355370cd941 ("x86/microcode/amd: Remove load_microcode_amd()'s bsp parameter")
Cc: <[email protected]> # a5ad92134bd1 ("x86/microcode/AMD: Add a @cpu parameter to the reloading functions")


2024-06-12 09:16:06

by Thorsten Leemhuis

[permalink] [raw]
Subject: Re: [PATCH v5] checkpatch: check for missing Fixes tags

On 12.06.24 10:49, Dan Carpenter wrote:
> On Wed, Jun 12, 2024 at 08:46:24AM +0200, Thorsten Leemhuis wrote:
>> On 11.06.24 20:38, Andrew Morton wrote:
>>> On Tue, 11 Jun 2024 16:43:29 +0300 Dan Carpenter <[email protected]> wrote:
>>>
>>>> This check looks for common words that probably indicate a patch
>>>> is a fix. For now the regex is:
>>>>
>>>> (?:(?:BUG: K.|UB)SAN: |Call Trace:|stable\@|syzkaller)/)
>>>>
>>>> Why are stable patches encouraged to have a fixes tag? Some people mark
>>>> their stable patches as "# 5.10" etc. This is useful but a Fixes tag is
>>>> still a good idea.
>>>
>>> I'd say that "# 5.10" is lame
>>
>> Documentation/process/stable-kernel-rules.rst documents this use to
>> "Point out kernel version prerequisites".
>
> No, the 5.10 means that the fix is required for everything after 5.10.
> Here is how you reference pre-requisites.
>
> Cc: <[email protected]> # 3.3.x: a1f84a3: sched: Check for idle
> Cc: <[email protected]> # 3.3.x: 1b9508f: sched: Rate-limit newidle
> Cc: <[email protected]> # 3.3.x: fd21073: sched: Fix affinity logic

That format according to the docs is to "Specify any additional patch
prerequisites for cherry picking", but cherry picking might not be what
the maintainer wants.

Anyway, I won't commit on this further and from here will leave this to
Greg, that's best at this point, it's his domain.

> But as a distro maintainer it's much nicer to have a Fixes:
> 123412341234 ("Add new wifi driver").

I see your point and agree that it would be nice to have. At the same
time I've seen people on the lists that don't like to use the Fixes: tag
when nothing is "fixed". And it would be an additional burden for
developers to look the commit-id up. So it could contribute to the
"checkpatch is asking too much here and not worth the trouble" stance
I've seen a few times (to which I contributed myself... :-/ ).

Ciao, Thorsten

2024-06-12 10:52:43

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH v5] checkpatch: check for missing Fixes tags

On Wed, Jun 12, 2024 at 11:15:45AM +0200, Thorsten Leemhuis wrote:
> I see your point and agree that it would be nice to have. At the same
> time I've seen people on the lists that don't like to use the Fixes: tag
> when nothing is "fixed".

That's correct. Checkpatch stuff, clean ups, and patches which silence
*harmless* static checker warnings shouldn't get a Fixes tag. This
checkpatch warning doesn't affect that. If you look at the patches
which were flagged it's mostly because of CCing stable or syzbot.

> And it would be an additional burden for
> developers to look the commit-id up. So it could contribute to the
> "checkpatch is asking too much here and not worth the trouble" stance
> I've seen a few times (to which I contributed myself... :-/ ).

Someone's got to do it. It might as well be the person who writes the
patch.

There are times where you're working across function boundaries or even
subsystem boundaries and in those cases finding the correct Fixes tag is
difficult. The other case where it's annoying is when the code has
moved between files. But it's generally a worthwhile exercise. It
helps to look at what the original author was trying to do when they
introduced the bug. And when you add a Fixes tag then checkpatch will
CC the original author so the review is better as well.

regards,
dan carpenter