Re: [PATCH v2 2/8] cxl/mbox: Convert poison list mutex to ACQUIRE()

[<dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> On Wed, 18 Jun 2025 22:04:10 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > Towards removing all explicit unlock calls in the CXL subsystem, convert
> > the conditional poison list mutex to use a conditional lock guard.
> > 
> > Rename the lock to have the compiler validate that all existing call sites
> > are converted.
> > 
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Davidlohr Bueso <dave@stgolabs.net>
> > Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
> > Cc: Dave Jiang <dave.jiang@intel.com>
> > Cc: Alison Schofield <alison.schofield@intel.com>
> > Cc: Vishal Verma <vishal.l.verma@intel.com>
> > Cc: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> One trivial inline. Either way
> 
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> 
> > ---
> >  drivers/cxl/core/mbox.c | 7 +++----
> >  drivers/cxl/cxlmem.h    | 4 ++--
> >  2 files changed, 5 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> > index 2689e6453c5a..81b21effe8cf 100644
> > --- a/drivers/cxl/core/mbox.c
> > +++ b/drivers/cxl/core/mbox.c
> > @@ -1401,8 +1401,8 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
> >  	int nr_records = 0;
> >  	int rc;
> >  
> > -	rc = mutex_lock_interruptible(&mds->poison.lock);
> > -	if (rc)
> > +	ACQUIRE(mutex_intr, lock)(&mds->poison.mutex);
> 
> I'd slightly prefer the 'canonical' style from the cleanup.h docs in previous patch.
> 
> 	rc = ACQUIRE_ERR(mutex_intr, &lock);
> 	if (rc)
> 		return rc;

I took a pass at that conversion before sending this out, but came back
to the inline assignment approach because the compactness matched the
familiar pattern with other error pointer handling.

The comfortable pattern with ERR_PTR() is:

	if (IS_ERR())
		return PTR_ERR();

The same could be done here with something like:

	if (ACQUIRE_ERR(@class, @lock))
		return ACQUIRE_ERR(@class, @lock))

...but that also feels like a mouthful especially if there is already a
local return code variable that can be used. So,

	if ((ret = ACQUIRE_ERR(@class, @lock))
		return ret;

...feel like a reasonable compromise for compactness on a common
pattern. Especially if cleanup helpers are also generally accepted as a
reason to bend the "variables declared at the top of scope" convention.