* Re: [PATCH v2] checkpatch: allow an assignment in if condition for ACQUIRE_ERR()
2025-08-15 1:06 [PATCH v2] checkpatch: allow an assignment in if condition for ACQUIRE_ERR() alison.schofield
@ 2025-08-15 1:11 ` Alison Schofield
2025-10-14 0:30 ` dan.j.williams
1 sibling, 0 replies; 3+ messages in thread
From: Alison Schofield @ 2025-08-15 1:11 UTC (permalink / raw)
To: Joe Perches, Andy Whitcroft, Dwaipayan Ray, Lukas Bulwahn
Cc: Peter Zijlstra, Dan Williams, linux-cxl, linux-kernel
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;
}
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] checkpatch: allow an assignment in if condition for ACQUIRE_ERR()
2025-08-15 1:06 [PATCH v2] checkpatch: allow an assignment in if condition for ACQUIRE_ERR() alison.schofield
2025-08-15 1:11 ` Alison Schofield
@ 2025-10-14 0:30 ` dan.j.williams
1 sibling, 0 replies; 3+ messages in thread
From: dan.j.williams @ 2025-10-14 0:30 UTC (permalink / raw)
To: alison.schofield, Joe Perches, Andy Whitcroft, Dwaipayan Ray,
Lukas Bulwahn
Cc: Alison Schofield, Peter Zijlstra, Dan Williams, linux-cxl, linux-kernel
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
^ permalink raw reply [flat|nested] 3+ messages in thread