* [PATCH RESEND] checkpatch: don't flag ACQUIRE_ERR() assignments in if conditions @ 2026-09-16 2:09 Guixin Liu 2026-09-22 23:32 ` Alison Schofield 0 siblings, 1 reply; 3+ messages in thread From: Guixin Liu @ 2026-09-16 2:09 UTC (permalink / raw) To: Andy Whitcroft, Joe Perches, Jonathan Cameron, Alison Schofield Cc: linux-kernel ACQUIRE_ERR() and its wrappers, PM_RUNTIME_ACQUIRE_ERR() and IIO_DEV_ACQUIRE_FAILED(), report whether a conditional cleanup.h guard was acquired, and drivers consume the result directly in an if condition: if ((rc = ACQUIRE_ERR(mutex_intr, &lock))) return rc; That combined form is the established style at the 49 in-tree call sites under drivers/cxl and drivers/pci/tsm.c, so ASSIGN_IN_IF fires there only as a false positive, and every patch touching those lines carries noise that reviewers have to wave off manually. Skip the check when the assigned expression is a call to a macro from that family, matched by the *_ACQUIRE_ERR() / *_ACQUIRE_FAILED() naming convention of its wrappers. Plain assignments and near-miss identifiers still get flagged. Signed-off-by: Guixin Liu <kanie@linux.alibaba.com> --- Discussion leading to this patch, where the false positive showed up on a cxl patch and covering the wrapper macros was requested as well: https://lore.kernel.org/linux-cxl/20260910094017.4032170-3-kanie@linux.alibaba.com/ scripts/checkpatch.pl | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 8a7787d228a6..fbc500bcfaa1 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5777,7 +5777,12 @@ sub process { my ($s, $c) = ($stat, $cond); my $fixed_assign_in_if = 0; - if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s) { + # ACQUIRE_ERR() and its wrappers, e.g. PM_RUNTIME_ACQUIRE_ERR() + # and IIO_DEV_ACQUIRE_FAILED(), are intended to be evaluated in + # an if condition, with the error assigned in the condition: + # if ((rc = ACQUIRE_ERR(name, &lock))) + if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s && + $c !~ /=\s*\w*ACQUIRE_(?:ERR|FAILED)\s*\(/) { if (ERROR("ASSIGN_IN_IF", "do not use assignment in if condition\n" . $herecurr) && $fix && $perl_version_ok) { base-commit: 50d05c7c76c96b90462f24debacca971d2e86713 -- 2.43.7 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH RESEND] checkpatch: don't flag ACQUIRE_ERR() assignments in if conditions 2026-09-16 2:09 [PATCH RESEND] checkpatch: don't flag ACQUIRE_ERR() assignments in if conditions Guixin Liu @ 2026-09-22 23:32 ` Alison Schofield 2026-09-24 3:35 ` Guixin Liu 0 siblings, 1 reply; 3+ messages in thread From: Alison Schofield @ 2026-09-22 23:32 UTC (permalink / raw) To: Guixin Liu; +Cc: Andy Whitcroft, Joe Perches, Jonathan Cameron, linux-kernel On Wed, Sep 16, 2026 at 10:09:21AM +0800, Guixin Liu wrote: > ACQUIRE_ERR() and its wrappers, PM_RUNTIME_ACQUIRE_ERR() and > IIO_DEV_ACQUIRE_FAILED(), report whether a conditional cleanup.h guard > was acquired, and drivers consume the result directly in an if > condition: > > if ((rc = ACQUIRE_ERR(mutex_intr, &lock))) > return rc; > > That combined form is the established style at the 49 in-tree call > sites under drivers/cxl and drivers/pci/tsm.c, so ASSIGN_IN_IF fires > there only as a false positive, and every patch touching those lines > carries noise that reviewers have to wave off manually. > > Skip the check when the assigned expression is a call to a macro from > that family, matched by the *_ACQUIRE_ERR() / *_ACQUIRE_FAILED() naming > convention of its wrappers. Plain assignments and near-miss identifiers > still get flagged. Hi Guixin, Glad to see you picking this up! I tried to address the same checkpatch false positive last year. Joe Perches gave some useful feedback on my v1, including a suggestion to use checkpatch's $Lval, and I incorporated that into v2 along with some test cases. That may be useful for your next version. More inline - > > Signed-off-by: Guixin Liu <kanie@linux.alibaba.com> > --- > Discussion leading to this patch, where the false positive showed up on > a cxl patch and covering the wrapper macros was requested as well: > > https://lore.kernel.org/linux-cxl/20260910094017.4032170-3-kanie@linux.alibaba.com/ > > scripts/checkpatch.pl | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 8a7787d228a6..fbc500bcfaa1 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -5777,7 +5777,12 @@ sub process { > my ($s, $c) = ($stat, $cond); > my $fixed_assign_in_if = 0; > > - if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s) { > + # ACQUIRE_ERR() and its wrappers, e.g. PM_RUNTIME_ACQUIRE_ERR() > + # and IIO_DEV_ACQUIRE_FAILED(), are intended to be evaluated in > + # an if condition, with the error assigned in the condition: > + # if ((rc = ACQUIRE_ERR(name, &lock))) > + if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s && > + $c !~ /=\s*\w*ACQUIRE_(?:ERR|FAILED)\s*\(/) { What happens if there is also an unrelated assignment in the condition? For example: if ((rc = regular_function()) || (ret = ACQUIRE_ERR(lock_type, &lock))) It looks like the ACQUIRE_ERR() match suppresses ASSIGN_IN_IF for the whole condition, including the unrelated assignment. Could you use $Lval, as Joe suggested on my v1, to make this more precise? I also suggest running this against a small test program with the expected-pass cases, ordinary assignments, and mixed cases like the one above. Feel free to reuse or extend the test cases from my v2 [1]. Could you also Cc linux-cxl on the next version? There are several users of this form there, so it may help get some review and testing tags on the patch. -- Alison [1] https://lore.kernel.org/linux-cxl/20250815010645.2980846-1-alison.schofield@intel.com/ > if (ERROR("ASSIGN_IN_IF", > "do not use assignment in if condition\n" . $herecurr) && > $fix && $perl_version_ok) { > > base-commit: 50d05c7c76c96b90462f24debacca971d2e86713 > -- > 2.43.7 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH RESEND] checkpatch: don't flag ACQUIRE_ERR() assignments in if conditions 2026-09-22 23:32 ` Alison Schofield @ 2026-09-24 3:35 ` Guixin Liu 0 siblings, 0 replies; 3+ messages in thread From: Guixin Liu @ 2026-09-24 3:35 UTC (permalink / raw) To: Alison Schofield Cc: Andy Whitcroft, Joe Perches, Jonathan Cameron, linux-kernel 在 2026/9/23 07:32, Alison Schofield 写道: > On Wed, Sep 16, 2026 at 10:09:21AM +0800, Guixin Liu wrote: >> ACQUIRE_ERR() and its wrappers, PM_RUNTIME_ACQUIRE_ERR() and >> IIO_DEV_ACQUIRE_FAILED(), report whether a conditional cleanup.h guard >> was acquired, and drivers consume the result directly in an if >> condition: >> >> if ((rc = ACQUIRE_ERR(mutex_intr, &lock))) >> return rc; >> >> That combined form is the established style at the 49 in-tree call >> sites under drivers/cxl and drivers/pci/tsm.c, so ASSIGN_IN_IF fires >> there only as a false positive, and every patch touching those lines >> carries noise that reviewers have to wave off manually. >> >> Skip the check when the assigned expression is a call to a macro from >> that family, matched by the *_ACQUIRE_ERR() / *_ACQUIRE_FAILED() naming >> convention of its wrappers. Plain assignments and near-miss identifiers >> still get flagged. > Hi Guixin, > > Glad to see you picking this up! > > I tried to address the same checkpatch false positive last year. Joe Perches > gave some useful feedback on my v1, including a suggestion to use checkpatch's > $Lval, and I incorporated that into v2 along with some test cases. That may > be useful for your next version. > > More inline - Yes, got that, thanks. > >> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com> >> --- >> Discussion leading to this patch, where the false positive showed up on >> a cxl patch and covering the wrapper macros was requested as well: >> >> https://lore.kernel.org/linux-cxl/20260910094017.4032170-3-kanie@linux.alibaba.com/ >> >> scripts/checkpatch.pl | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >> index 8a7787d228a6..fbc500bcfaa1 100755 >> --- a/scripts/checkpatch.pl >> +++ b/scripts/checkpatch.pl >> @@ -5777,7 +5777,12 @@ sub process { >> my ($s, $c) = ($stat, $cond); >> my $fixed_assign_in_if = 0; >> >> - if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s) { >> + # ACQUIRE_ERR() and its wrappers, e.g. PM_RUNTIME_ACQUIRE_ERR() >> + # and IIO_DEV_ACQUIRE_FAILED(), are intended to be evaluated in >> + # an if condition, with the error assigned in the condition: >> + # if ((rc = ACQUIRE_ERR(name, &lock))) >> + if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s && >> + $c !~ /=\s*\w*ACQUIRE_(?:ERR|FAILED)\s*\(/) { > What happens if there is also an unrelated assignment in the condition? For example: > if ((rc = regular_function()) || (ret = ACQUIRE_ERR(lock_type, &lock))) > > It looks like the ACQUIRE_ERR() match suppresses ASSIGN_IN_IF for the whole condition, > including the unrelated assignment. > > Could you use $Lval, as Joe suggested on my v1, to make this more precise? OK, changed in v2, thanks. > I also suggest running this against a small test program with the expected-pass cases, > ordinary assignments, and mixed cases like the one above. Feel free to reuse or extend > the test cases from my v2 [1]. Sure, I will try to test as much as possible. > > Could you also Cc linux-cxl on the next version? There are several users of this form > there, so it may help get some review and testing tags on the patch. OK, will cc linux-cxl in v2, thanks. Best Regards, Guixin Liu > > -- Alison > > > [1] https://lore.kernel.org/linux-cxl/20250815010645.2980846-1-alison.schofield@intel.com/ > > > > >> if (ERROR("ASSIGN_IN_IF", >> "do not use assignment in if condition\n" . $herecurr) && >> $fix && $perl_version_ok) { >> >> base-commit: 50d05c7c76c96b90462f24debacca971d2e86713 >> -- >> 2.43.7 >> ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-09-24 3:36 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-09-16 2:09 [PATCH RESEND] checkpatch: don't flag ACQUIRE_ERR() assignments in if conditions Guixin Liu 2026-09-22 23:32 ` Alison Schofield 2026-09-24 3:35 ` Guixin Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®