Re: [PATCH v2] checkpatch: don't flag ACQUIRE_ERR() assignments in if conditions

[Guixin Liu]

Hi Joe,
  Could you please apply this patch to 7.4?

Best Regards,
Guixin Liu

在 2026/9/25 01:09, Joe Perches 写道:
> On Thu, 2026-09-24 at 11:39 +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 only when every assignment in the condition assigns the
>> result of such a call, matched by the *_ACQUIRE_ERR() /
>> *_ACQUIRE_FAILED() naming convention of its wrappers. Plain
>> assignments, mixed conditions and near-miss identifiers still get
>> flagged.
>>
>> Suggested-by: Alison Schofield <[alison.schofield@xxxxxxxxx](mailto:alison.schofield@xxxxxxxxx)>
>> Cc: [linux-cxl@xxxxxxxxxxxxxxx](mailto:linux-cxl@xxxxxxxxxxxxxxx)
>> Assisted-by: LLM
>> Signed-off-by: Guixin Liu <[kanie@xxxxxxxxxxxxxxxxx](mailto:kanie@xxxxxxxxxxxxxxxxx)>
> Acked-by: Joe Perches <joe@xxxxxxxxxxx>
>
>> ---
>> Changes since v1 [1]:
>>
>> - Check each assignment in the condition instead of the condition as a
>>   whole, so a mixed condition such as
>>   "if ((rc = regular_function()) || (ret = ACQUIRE_ERR(...)))" keeps
>>   getting flagged (Alison Schofield).
>> - Match the assigned-to expression with $Lval, as Joe Perches
>>   suggested on Alison's earlier attempt at this exception [2].
>> - Cc linux-cxl, where most users of this form live.
>>
>> Two deviations from [2]: the assigned expression capture excludes '=',
>> so an '==' comparison in a mixed condition is not mistaken for a bad
>> assignment, and the check requires an assignment the $Lval match can
>> see, so compound assignments like '+=', which it cannot see, are not
>> silently allowed.
>>
>> Testing: ran checkpatch on a test file with the allowed forms (bare
>> macro and both wrappers, member and array element lhs, multiple
>> ACQUIRE family assignments in one condition, an '==' comparison
>> alongside, no spaces around '=') and with 14 cases that must still
>> trigger (plain assignments, near-miss identifiers, arithmetic on the
>> call result, ACQUIRE_ERR in a comment, indirect assignment, mixed
>> conditions in both orders). All allowed forms pass, all 14 still
>> trigger. The 49 in-tree call sites under drivers/cxl and
>> drivers/pci/tsm.c no longer report ASSIGN_IN_IF, and checkpatch output
>> is unchanged for fs/namei.c, kernel/sched/core.c, mm/vmscan.c and
>> net/core/dev.c.
>>
>> [1] [https://lore.kernel.org/all/20260916020921.3480730-1-kanie@xxxxxxxxxxxxxxxxx/](https://lore.kernel.org/all/20260916020921.3480730-1-kanie@xxxxxxxxxxxxxxxxx/)
>> [2] [https://lore.kernel.org/linux-cxl/20250815010645.2980846-1-alison.schofield@xxxxxxxxx/](https://lore.kernel.org/linux-cxl/20250815010645.2980846-1-alison.schofield@xxxxxxxxx/)
>> ---
>>  scripts/checkpatch.pl | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 8a7787d228a6..eee06ac6737f 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -5777,7 +5777,21 @@ sub process {
>>  			my ($s, $c) = ($stat, $cond);
>>  			my $fixed_assign_in_if = 0;
>>  
>> +			# ACQUIRE_ERR() and its wrappers, e.g. PM_RUNTIME_ACQUIRE_ERR()
>> +			# and IIO_DEV_ACQUIRE_FAILED(), are meant to be evaluated in an
>> +			# if condition, with the error assigned in the condition:
>> +			#	if ((rc = ACQUIRE_ERR(name, &lock)))
>> +			# Allow that only when every assignment in the condition assigns
>> +			# the result of such a call, so that a mixed condition keeps
>> +			# getting flagged:
>> +			#	if ((rc = regular_function()) || (ret = ACQUIRE_ERR(name, &lock)))
>> +			my $assign_in_if = 0;
>>  			if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s) {
>> +				my $has_assignment = $c =~ /\b$Lval\s*=\s*[^,)&|=]+/;
>> +				my $has_other_assignment = $c =~ /\b$Lval\s*=\s*(?!\s*\w*ACQUIRE_(?:ERR|FAILED)\s*\()[^,)&|=]+/;
>> +				$assign_in_if = !$has_assignment || $has_other_assignment;
>> +			}
>> +			if ($assign_in_if) {
>>  				if (ERROR("ASSIGN_IN_IF",
>>  					  "do not use assignment in if condition\n" . $herecurr) &&
>>  				    $fix && $perl_version_ok) {
>>