1. Patchew
  2. linux
  3. View series

[PATCH v2] checkpatch: allow an assignment in if condition for ACQUIRE_ERR()

alison.schofield@intel.com posted 1 patch 5 months, 3 weeks ago 
scripts/checkpatch.pl | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
[PATCH v2] checkpatch: allow an assignment in if condition for ACQUIRE_ERR()
Posted by alison.schofield@intel.com 5 months, 3 weeks ago 
From: Alison Schofield <alison.schofield@intel.com>

New helpers, ACQUIRE() and ACQUIRE_ERR(), were recently introduced
and employed here [1] to clean up conditional locking paths.

That led to checkpatch ERRORS:
        ERROR: do not use assignment in if condition

on usages like this:
        ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
        if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)))
                return rc;

That compact format was a deliberate choice by the authors. By making
this a checkpatch exception, existing ERRORs are quieted, and future
users of the macro will not be dissuaded by checkpatch from using the
preferred compact format.

[1] Commit d03fcf50ba56 ("cxl: Convert to ACQUIRE() for conditional rwsem locking")

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
---

Changes in v2:
- Remove next that skipped other rules (Joe)
- Replace \w+ with $Lval for more precise pattern match (Joe)
- Only allow lines where all assignments are to ACQUIRE_ERR
- Tested many more allow and trigger conditions.
  Will share in reply to this post.
- Update commit message

 scripts/checkpatch.pl | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e722dd6fa8ef..30435967d8c4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5696,7 +5696,18 @@ sub process {
 			my ($s, $c) = ($stat, $cond);
 			my $fixed_assign_in_if = 0;
 
-			if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s) {
+			if ($c =~ /\bif\s*\((.*[^<>!=]=[^=].*)\)/s) {
+			    my $condition = $1;
+			    my $allow_assignment = 1;
+
+			    # Allow single ACQUIRE_ERR assignment, reject everything else
+			    while ($condition =~ /\b($Lval)\s*=\s*([^,)&|]+)/g) {
+				 if ($2 !~ /^\s*ACQUIRE_ERR\s*\(/) {
+					$allow_assignment = 0;
+					last;
+				}
+			    }
+			    if (!$allow_assignment) {
 				if (ERROR("ASSIGN_IN_IF",
 					  "do not use assignment in if condition\n" . $herecurr) &&
 				    $fix && $perl_version_ok) {
@@ -5721,6 +5732,7 @@ sub process {
 						fix_insert_line($fixlinenr + 1, $newline);
 						$fixed_assign_in_if = 1;
 					}
+				    }
 				}
 			}
 

base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
-- 
2.37.3
Re: [PATCH v2] checkpatch: allow an assignment in if condition for ACQUIRE_ERR()
Posted by dan.j.williams@intel.com 3 months, 3 weeks ago 
alison.schofield@ wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> New helpers, ACQUIRE() and ACQUIRE_ERR(), were recently introduced
> and employed here [1] to clean up conditional locking paths.
> 
> That led to checkpatch ERRORS:
>         ERROR: do not use assignment in if condition
> 
> on usages like this:
>         ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
>         if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem)))
>                 return rc;
> 
> That compact format was a deliberate choice by the authors. By making
> this a checkpatch exception, existing ERRORs are quieted, and future
> users of the macro will not be dissuaded by checkpatch from using the
> preferred compact format.
> 
> [1] Commit d03fcf50ba56 ("cxl: Convert to ACQUIRE() for conditional rwsem locking")

Works for me.

Tested-by: Dan Williams <dan.j.williams@intel.com>

I am going to be resubmitting the final (hopefully) version of the
PCI/TSM core implementation [1] which uses this compact style.

It takes the current version of the patch from:

    total: 4 errors, 2 warnings, 1074 lines checked

...to:

    total: 0 errors, 2 warnings, 1074 lines checked

Where one of those warnings I overlooked previously due to the other
errors. The final warning in that set is:

    WARNING: suspect code indent for conditional statements (0, 0)
    #380: FILE: drivers/pci/tsm.c:72:
    +DEFINE_FREE(tsm_remove, struct pci_tsm *, if (_T) tsm_remove(_T))
    [...]
    +static void pci_tsm_walk_fns(struct pci_dev *pdev,

Which is another cleanup.h usage enlightenment for checkpatch.pl that
would be nice to see.

[1]: http://lore.kernel.org/20250911235647.3248419-5-dan.j.williams@intel.com
Re: [PATCH v2] checkpatch: allow an assignment in if condition for ACQUIRE_ERR()
Posted by Alison Schofield 5 months, 3 weeks ago 
On Thu, Aug 14, 2025 at 06:06:43PM -0700, alison.schofield@intel.com wrote:
> From: Alison Schofield <alison.schofield@intel.com>

Appended is the file.c used in testing. It's annotated with what is
expected to pass thru vs trigger.


/*
 * Test file for checkpatch.pl ACQUIRE_ERR exception
 * This file contains various assignment-in-if patterns to verify
 * that the checkpatch modification works correctly.
 *
 * Usage: ./scripts/checkpatch.pl --file this_filename.c --strict 
 *
 * Expect: total: 16 errors, 3 warnings, 1 checks
 */

/*
 * Valid ACQUIRE_ERR() usage patterns that should be allowed
 * Should all pass,  no checkpatch errors
 */
int test_cases_that_should_pass(void)
{
	/* Basic ACQUIRE_ERR usage - should be allowed */
	if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem_ptr)))
		return rc;

	/* ACQUIRE_ERR with different variable names - should be allowed */
	if ((ret = ACQUIRE_ERR(mutex_lock, &obj->ptr)))
		goto error_exit;

	/* ACQUIRE_ERR with longer variable names - should be allowed */
	if ((return_code = ACQUIRE_ERR(spinlock_acquire, rwsem_ptr)))
		return return_code;

	/* ACQUIRE_ERR with struct member assignment - should be allowed */
	if ((obj->value = ACQUIRE_ERR(lock_type, &rwsem_ptr)))
		return -EBUSY;

	/* Different whitespace patterns - should all be allowed */
	if ((rc = ACQUIRE_ERR(lock_type, &ptr)))
		return rc;

	if ((rc = ACQUIRE_ERR(lock_type, &ptr)))
		return rc;

	if ((rc = ACQUIRE_ERR(lock_type, &ptr)))
		return rc;

	/* Multi-line ACQUIRE_ERR - should be allowed */
	if ((rc = ACQUIRE_ERR(very_long_lock_type_name,
			      &ptr)))
		return rc;

	/* Multiple ACQUIRE_ERR assignments - should be allowed */
	if ((rc = ACQUIRE_ERR(lock_type, &ptr)) || (ret = ACQUIRE_ERR(other_lock, &rwsem_ptr)))
		return rc;

	/* ACQUIRE_ERR in complex but all-ACQUIRE_ERR expression - should be allowed */
	if ((obj->value = ACQUIRE_ERR(lock1, &ptr)) && (rc = ACQUIRE_ERR(lock2, &rwsem_ptr)))
		return rc;

	/* Comparison operators - should be allowed */
	if (rc == ACQUIRE_ERR(lock_type, &rwsem_ptr))
		return rc;

	if (rc != ACQUIRE_ERR(lock_type, &rwsem_ptr))
		return rc;

	if (rc < ACQUIRE_ERR(lock_type, &rwsem_ptr))
		return rc;

	/* Function calls without assignment - should be allowed */
	if (ACQUIRE_ERR(lock_type, &rwsem_ptr))
		return -EBUSY;

	/* Regular conditionals - should be allowed */
	if (rc)
		return rc;

	if (obj && obj->value)
		return obj->value;

	return 0;
}

/*
 * Should all trigger ERROR or WARNING or CHECK
 *
 * Non-ACQUIRE_ERR assignments that should still be caught
 * And, cases that pass ASSIGN_IN_IF, but fail other rules
 */
int test_cases_that_should_fail(void)
{
	/* ERROR: do not use assignment in if condition */
	if ((rc = regular_function()))
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((value = obj->value))
		return value;

	/* ERROR: do not use assignment in if condition */
	if ((rc = value + 1))
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((rc = ACQUIRE(rwsem_write_kill, &rwsem_ptr)))
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((rc = some_ACQUIRE_ERR_helper_function()))
		return rc;

	/* ERROR: do not use assignment in if condition */
	ret = ACQUIRE_ERR(lock_type, &rwsem_ptr);
	if ((rc = ret))
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((rc = regular_function())) /* ACQUIRE_ERR pattern in comment */
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((rc = regular_function() + ACQUIRE_ERR(lock_type, &rwsem_ptr)))
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((rc = regular_function()) || (ret = ACQUIRE_ERR(lock_type, &rwsem_ptr)))
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((ret = ACQUIRE_ERR(lock_type, &rwsem_ptr)) || (rc = regular_function()))
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((rc = regular_function()) || (value = obj->value))
		return rc;

	/* ERROR: do not use assignment in if condition */
	if ((rc = regular_function()) && (value = 22))
		return rc;

	/* Next set should pass the ASSIGN_IN_IF check but fail other rules */

	/* ERROR: trailing whitespace */
	/* WARNING: please, no space before tabs */
	if ((rc = ACQUIRE_ERR(lock_type, &ptr)))   	
		return rc;

	/* ERROR: space required before the open brace '{' */
	/* WARNING: braces {} are not necessary for single statement blocks */
	if ((rc = ACQUIRE_ERR(lock_type, &ptr))){
		return rc;
	}

	/* CHECK: line length of 150 exceeds 100 columns */
	if ((rc = ACQUIRE_ERR(very_very_very_very_very_very_very_very_very_very_very_very_long_lock_type_name_that_exceeds_typical_line_limit, &ptr)))
		return rc;

	/* ERROR: space required after that ',' (ctx:VxO) */
	/* ERROR: space required before that '&' (ctx:OxV) */
	if ((rc = ACQUIRE_ERR(lock_type,&ptr)))
		return rc;
}

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.