1. Patchew
  2. linux
  3. [PATCH v2 0/8] cleanup: Introduce ACQUIRE(), a guard() for conditional locks
  4. View patch

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

Dan Williams posted 8 patches 3 months, 3 weeks ago
There is a newer version of this series
[PATCH v2 2/8] cxl/mbox: Convert poison list mutex to ACQUIRE()
Posted by Dan Williams 3 months, 3 weeks ago 
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>
---
 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);
+	if ((rc = ACQUIRE_ERR(mutex_intr, &lock)))
 		return rc;
 
 	po = mds->poison.list_out;
@@ -1437,7 +1437,6 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
 		}
 	} while (po->flags & CXL_POISON_FLAG_MORE);
 
-	mutex_unlock(&mds->poison.lock);
 	return rc;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_mem_get_poison, "CXL");
@@ -1473,7 +1472,7 @@ int cxl_poison_state_init(struct cxl_memdev_state *mds)
 		return rc;
 	}
 
-	mutex_init(&mds->poison.lock);
+	mutex_init(&mds->poison.mutex);
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_poison_state_init, "CXL");
diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
index 551b0ba2caa1..f5b20641e57c 100644
--- a/drivers/cxl/cxlmem.h
+++ b/drivers/cxl/cxlmem.h
@@ -254,7 +254,7 @@ enum security_cmd_enabled_bits {
  * @max_errors: Maximum media error records held in device cache
  * @enabled_cmds: All poison commands enabled in the CEL
  * @list_out: The poison list payload returned by device
- * @lock: Protect reads of the poison list
+ * @mutex: Protect reads of the poison list
  *
  * Reads of the poison list are synchronized to ensure that a reader
  * does not get an incomplete list because their request overlapped
@@ -265,7 +265,7 @@ struct cxl_poison_state {
 	u32 max_errors;
 	DECLARE_BITMAP(enabled_cmds, CXL_POISON_ENABLED_MAX);
 	struct cxl_mbox_poison_out *list_out;
-	struct mutex lock;  /* Protect reads of poison list */
+	struct mutex mutex;  /* Protect reads of poison list */
 };
 
 /*
-- 
2.49.0
Re: [PATCH v2 2/8] cxl/mbox: Convert poison list mutex to ACQUIRE()
Posted by Dave Jiang 3 months, 2 weeks ago 

On 6/18/25 10:04 PM, Dan Williams 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>

Reviewed-by: Dave Jiang <dave.jiang@intel.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);
> +	if ((rc = ACQUIRE_ERR(mutex_intr, &lock)))
>  		return rc;
>  
>  	po = mds->poison.list_out;
> @@ -1437,7 +1437,6 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>  		}
>  	} while (po->flags & CXL_POISON_FLAG_MORE);
>  
> -	mutex_unlock(&mds->poison.lock);
>  	return rc;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_mem_get_poison, "CXL");
> @@ -1473,7 +1472,7 @@ int cxl_poison_state_init(struct cxl_memdev_state *mds)
>  		return rc;
>  	}
>  
> -	mutex_init(&mds->poison.lock);
> +	mutex_init(&mds->poison.mutex);
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_poison_state_init, "CXL");
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 551b0ba2caa1..f5b20641e57c 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -254,7 +254,7 @@ enum security_cmd_enabled_bits {
>   * @max_errors: Maximum media error records held in device cache
>   * @enabled_cmds: All poison commands enabled in the CEL
>   * @list_out: The poison list payload returned by device
> - * @lock: Protect reads of the poison list
> + * @mutex: Protect reads of the poison list
>   *
>   * Reads of the poison list are synchronized to ensure that a reader
>   * does not get an incomplete list because their request overlapped
> @@ -265,7 +265,7 @@ struct cxl_poison_state {
>  	u32 max_errors;
>  	DECLARE_BITMAP(enabled_cmds, CXL_POISON_ENABLED_MAX);
>  	struct cxl_mbox_poison_out *list_out;
> -	struct mutex lock;  /* Protect reads of poison list */
> +	struct mutex mutex;  /* Protect reads of poison list */
>  };
>  
>  /*
Re: [PATCH v2 2/8] cxl/mbox: Convert poison list mutex to ACQUIRE()
Posted by Jonathan Cameron 3 months, 2 weeks ago 
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;

> +	if ((rc = ACQUIRE_ERR(mutex_intr, &lock)))
>  		return rc;
>  
>  	po = mds->poison.list_out;
> @@ -1437,7 +1437,6 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>  		}
>  	} while (po->flags & CXL_POISON_FLAG_MORE);
>  
> -	mutex_unlock(&mds->poison.lock);
>  	return rc;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_mem_get_poison, "CXL");
> @@ -1473,7 +1472,7 @@ int cxl_poison_state_init(struct cxl_memdev_state *mds)
>  		return rc;
>  	}
>  
> -	mutex_init(&mds->poison.lock);
> +	mutex_init(&mds->poison.mutex);
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_poison_state_init, "CXL");
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 551b0ba2caa1..f5b20641e57c 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -254,7 +254,7 @@ enum security_cmd_enabled_bits {
>   * @max_errors: Maximum media error records held in device cache
>   * @enabled_cmds: All poison commands enabled in the CEL
>   * @list_out: The poison list payload returned by device
> - * @lock: Protect reads of the poison list
> + * @mutex: Protect reads of the poison list
>   *
>   * Reads of the poison list are synchronized to ensure that a reader
>   * does not get an incomplete list because their request overlapped
> @@ -265,7 +265,7 @@ struct cxl_poison_state {
>  	u32 max_errors;
>  	DECLARE_BITMAP(enabled_cmds, CXL_POISON_ENABLED_MAX);
>  	struct cxl_mbox_poison_out *list_out;
> -	struct mutex lock;  /* Protect reads of poison list */
> +	struct mutex mutex;  /* Protect reads of poison list */
>  };
>  
>  /*
Re: [PATCH v2 2/8] cxl/mbox: Convert poison list mutex to ACQUIRE()
Posted by dan.j.williams@intel.com 3 months ago 
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.
Re: [PATCH v2 2/8] cxl/mbox: Convert poison list mutex to ACQUIRE()
Posted by Alison Schofield 3 months, 2 weeks ago 
On Wed, Jun 18, 2025 at 10:04:10PM -0700, Dan Williams 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.

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.