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

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

Posted by alison.schofield@intel.com 1 month, 3 weeks ago

From: Alison Schofield <alison.schofield@intel.com>

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

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>
---
 scripts/checkpatch.pl | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e722dd6fa8ef..150f355c632e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5696,7 +5696,14 @@ sub process {
 			my ($s, $c) = ($stat, $cond);
 			my $fixed_assign_in_if = 0;
 
-			if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s) {
+			if ($c =~ /\bif\s*\((.*[^<>!=]=[^=].*)\)/s) {
+				my $expr = $1;
+
+				# Allow ACQUIRE_ERR() macro syntax
+				if ($expr =~ /\w+\s*=\s*ACQUIRE_ERR\s*\(/) {
+					next;
+				}
+
 				if (ERROR("ASSIGN_IN_IF",
 					  "do not use assignment in if condition\n" . $herecurr) &&
 				    $fix && $perl_version_ok) {
-- 
2.37.3

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

Posted by Joe Perches 1 month, 3 weeks ago

On Tue, 2025-08-12 at 17:38 -0700, alison.schofield@intel.com wrote:
> From: Alison Schofield <alison.schofield@intel.com>
> 
> New helpers, ACQUIRE() and ACQUIRE_ERR(), were recently introduced to
> clean up conditional locking paths [1].
[]
> 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.

not stylish IMO.

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -5696,7 +5696,14 @@ sub process {
>  			my ($s, $c) = ($stat, $cond);
>  			my $fixed_assign_in_if = 0;
>  
> -			if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s) {
> +			if ($c =~ /\bif\s*\((.*[^<>!=]=[^=].*)\)/s) {
> +				my $expr = $1;
> +
> +				# Allow ACQUIRE_ERR() macro syntax
> +				if ($expr =~ /\w+\s*=\s*ACQUIRE_ERR\s*\(/) {
> +					next;
> +				}

nak.

Using next would not do any additional checks on this line.
Likely use basic block or a constuct like

			if ($c =~ /\bif\s*\((.*[^<>!=]=[^=].*)/s &&
			    $1 !~ /\bACQUIRE_ERR\b/) {


and perhaps the \w+ should be $Lval

> +
>  				if (ERROR("ASSIGN_IN_IF",
>  					  "do not use assignment in if condition\n" . $herecurr) &&
>  				    $fix && $perl_version_ok) {

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

Posted by Alison Schofield 1 month, 2 weeks ago

On Wed, Aug 13, 2025 at 12:17:51AM -0700, Joe Perches wrote:
> On Tue, 2025-08-12 at 17:38 -0700, alison.schofield@intel.com wrote:
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > New helpers, ACQUIRE() and ACQUIRE_ERR(), were recently introduced to
> > clean up conditional locking paths [1].
> []
> > 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.
> 
> not stylish IMO.

I knew when suggesting this patch, that the 'no assignment in if
condition' hasn't had any previous exceptions. I appreciate your
helping in working out the implementation details here.

more below -

> 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -5696,7 +5696,14 @@ sub process {
> >  			my ($s, $c) = ($stat, $cond);
> >  			my $fixed_assign_in_if = 0;
> >  
> > -			if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s) {
> > +			if ($c =~ /\bif\s*\((.*[^<>!=]=[^=].*)\)/s) {
> > +				my $expr = $1;
> > +
> > +				# Allow ACQUIRE_ERR() macro syntax
> > +				if ($expr =~ /\w+\s*=\s*ACQUIRE_ERR\s*\(/) {
> > +					next;
> > +				}
> 
> nak.
> 
> Using next would not do any additional checks on this line.
I see. I'll get rid of the next and use conditional blocks instead.

> Likely use basic block or a constuct like
> 
> 			if ($c =~ /\bif\s*\((.*[^<>!=]=[^=].*)/s &&
> 			    $1 !~ /\bACQUIRE_ERR\b/) {
> 
> 
> and perhaps the \w+ should be $Lval

Thanks for that. The v2 approach uses the $Lval and captures the full
if condition expression and iterates thru each assignment.

I did more diligence in v2 wrt testing and will post my test cases
along with that so you can see what the patch intends to allow and
what triggers ERRORs, including ERRORs that the v1 usage of 'next'
got ignored.

-- Alison


> 
> > +
> >  				if (ERROR("ASSIGN_IN_IF",
> >  					  "do not use assignment in if condition\n" . $herecurr) &&
> >  				    $fix && $perl_version_ok) {
>