Re: [PATCH] cxl/mbox: Use guard() for mbox_mutex locking

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 28 Apr 2026 14:22:20 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> Use the new helpers and simplify the code. No change in
> semantics.
Does it simplify the code?  I'm not sure these particular
ones are that helpful.

Don't get me wrong, I love guards() and using them in new
code is fine but I'm not sure it's worth the churn if we don't
see a significant advantage.

Some specific comments inline.  I think with a few tweaks
the advantages become greater and outweight the churn aspect
- others may disagree!

> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
> ---
>  drivers/cxl/core/memdev.c | 16 ++++++++--------
>  drivers/cxl/pci.c         | 21 ++++++++-------------
>  2 files changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 3db4e91170a8..0c9067ced7b3 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -138,10 +138,10 @@ static ssize_t security_state_show(struct device *dev,
>  	int rc = 0;
>  
>  	/* sync with latest submission state */
> -	mutex_lock(&cxl_mbox->mbox_mutex);
> -	if (mds->security.sanitize_active)
> -		rc = sysfs_emit(buf, "sanitize\n");
> -	mutex_unlock(&cxl_mbox->mbox_mutex);
> +	scoped_guard(mutex, &cxl_mbox->mbox_mutex) {
> +		if (mds->security.sanitize_active)
> +			rc = sysfs_emit(buf, "sanitize\n");
> +	}
>  	if (rc)
>  		return rc;
As it stands, if anything this is worse from readability point of view
as the setting of rc is getting further away.
However... If we take a it a step further I think it would be worth doing.

	scoped_guard(mutex, &cxl_mbox->mbox_mutex) {
		if (mds->security.santize_active) {
			rc = sysfs_emit(buf, "sanitize\n");
			if (rc)
				return rc;
		}
	}
+ can drop the rc init at the top.
That way the scoped_guard() is letting us move the error check nearer
the source of the error and is a win.

>  
> @@ -1247,10 +1247,10 @@ static void sanitize_teardown_notifier(void *data)
>  	 * Prevent new irq triggered invocations of the workqueue and
>  	 * flush inflight invocations.
>  	 */
> -	mutex_lock(&cxl_mbox->mbox_mutex);
> -	state = mds->security.sanitize_node;
> -	mds->security.sanitize_node = NULL;
> -	mutex_unlock(&cxl_mbox->mbox_mutex);
> +	scoped_guard(mutex, &cxl_mbox->mbox_mutex) {
> +		state = mds->security.sanitize_node;
> +		mds->security.sanitize_node = NULL;
> +	}

This one is churn for me. But if it's the only manual
mutex handling int he file that is left, fair enough.

>  
>  	cancel_delayed_work_sync(&mds->security.poll_dwork);
>  	sysfs_put(state);
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 95bf773aab14..3462cea6e61b 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -134,10 +134,11 @@ static irqreturn_t cxl_pci_mbox_irq(int irq, void *id)
>  	reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
>  	opcode = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_OPCODE_MASK, reg);
>  	if (opcode == CXL_MBOX_OP_SANITIZE) {
> -		mutex_lock(&cxl_mbox->mbox_mutex);
> -		if (mds->security.sanitize_node)
> -			mod_delayed_work(system_percpu_wq, &mds->security.poll_dwork, 0);
> -		mutex_unlock(&cxl_mbox->mbox_mutex);
> +		scoped_guard(mutex, &cxl_mbox->mbox_mutex) {
> +			if (mds->security.sanitize_node)
> +				mod_delayed_work(system_percpu_wq,
> +						 &mds->security.poll_dwork, 0);

This one I dislike because the indent is getting larger for no
major readability advantage. GIven the scope is tightly defined anyway how about
instead doing:

	if (opcode == CXL_MBOX_OP_SANTIZE) {
		guard(mutex)(&cxl_mbox.santize_mode);
		if (mds->security.sanitize_node)
			mod_delayed_work(system_percpu_wq, &mds->security.poll_dwork, 0);
>  	} else {
>  		/* short-circuit the wait in __cxl_pci_mbox_send_cmd() */
>  		rcuwait_wake_up(&cxl_mbox->mbox_wait);
> @@ -156,7 +157,7 @@ static void cxl_mbox_sanitize_work(struct work_struct *work)
>  	struct cxl_dev_state *cxlds = &mds->cxlds;
>  	struct cxl_mailbox *cxl_mbox = &cxlds->cxl_mbox;
>  
> -	mutex_lock(&cxl_mbox->mbox_mutex);
> +	guard(mutex)(&cxl_mbox->mbox_mutex);
>  	if (cxl_mbox_background_complete(cxlds)) {
>  		mds->security.poll_tmo_secs = 0;
>  		if (mds->security.sanitize_node)
> @@ -170,7 +171,6 @@ static void cxl_mbox_sanitize_work(struct work_struct *work)
>  		mds->security.poll_tmo_secs = min(15 * 60, timeout);
>  		schedule_delayed_work(&mds->security.poll_dwork, timeout * HZ);
>  	}
> -	mutex_unlock(&cxl_mbox->mbox_mutex);
>  }
>  
>  /**
> @@ -377,13 +377,8 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
>  static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
>  			     struct cxl_mbox_cmd *cmd)
>  {
> -	int rc;
> -
> -	mutex_lock(&cxl_mbox->mbox_mutex);
> -	rc = __cxl_pci_mbox_send_cmd(cxl_mbox, cmd);
> -	mutex_unlock(&cxl_mbox->mbox_mutex);
> -
> -	return rc;
> +	guard(mutex)(&cxl_mbox->mbox_mutex);
> +	return __cxl_pci_mbox_send_cmd(cxl_mbox, cmd);
I like these really simple ones just for saving lines of code.
So this is good as far as I'm concerned.

>  }
>  
>  static int cxl_pci_setup_mailbox(struct cxl_memdev_state *mds, bool irq_avail)