Re: [PATCH 3/7] cxl/pci: Lockless background synchronous polling

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 17 Jun 2025 12:36:07 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> For timesliced background commands we rely on holding the mbox_mutex
> throughout the duration of the operation. This causes other incoming
> commands to queue up behind, and interleaving executions while the
> background command is timesliced by the user.
> 
> However, in order to support the mbox request cancel background
> operation command, the lock will need to be available to actually
> perform the request. Semantically this allows other commands to
> many times be at the mercy of hardware returning the respective error.
> Potentially users would be exposed to changes in the form of errors
> instead of commands taking longer to run - but this is not foreseen
> as a problem.
> 
> In order to not loose sync with the hardware, introduce a mailbox
> atomic that blocks any other incoming bg operations while the driver
> is still polling (synchronously) on the current one.
> 
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>
Minor comments inline.

Jonathan


> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 0d8f3605dc3f..3e6c231e9a8b 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -312,23 +312,7 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
>  	mbox_cmd->return_code =
>  		FIELD_GET(CXLDEV_MBOX_STATUS_RET_CODE_MASK, status_reg);
>  
> -	/*
> -	 * Handle the background command in a synchronous manner.
> -	 *
> -	 * All other mailbox commands will serialize/queue on the mbox_mutex,
> -	 * which we currently hold. Furthermore this also guarantees that
> -	 * cxl_mbox_background_complete() checks are safe amongst each other,
> -	 * in that no new bg operation can occur in between.
> -	 *
> -	 * Background operations are timesliced in accordance with the nature
> -	 * of the command. In the event of timeout, the mailbox state is
> -	 * indeterminate until the next successful command submission and the
> -	 * driver can get back in sync with the hardware state.
> -	 */
>  	if (mbox_cmd->return_code == CXL_MBOX_CMD_RC_BACKGROUND) {
> -		u64 bg_status_reg;
> -		int i, timeout;
> -
>  		dev_dbg(dev, "Mailbox background operation (0x%04x) started\n",
>  			mbox_cmd->opcode);
>  
> @@ -338,6 +322,8 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
>  		 */
>  		if (mbox_cmd->opcode == CXL_MBOX_OP_SANITIZE ||
>  		    mbox_cmd->opcode == CXL_MBOX_OP_ACTIVATE_FW) {
> +			int timeout;
> +
>  			if (mds->bg.opcode)
>  				return -EBUSY;
>  
> @@ -347,41 +333,17 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_mailbox *cxl_mbox,
>  			mds->bg.opcode = mbox_cmd->opcode;
>  			schedule_delayed_work(&mds->bg.poll_dwork,
>  					      timeout * HZ);
> -			goto success;
> +		} else {
> +			/* pairs with release/acquire semantics */
> +			WARN_ON_ONCE(atomic_xchg(&cxl_mbox->poll_bgop,
> +						 mbox_cmd->opcode));
>  		}
> -
> -		timeout = mbox_cmd->poll_interval_ms;
> -		for (i = 0; i < mbox_cmd->poll_count; i++) {
> -			if (rcuwait_wait_event_timeout(&cxl_mbox->mbox_wait,
> -						       cxl_mbox_background_complete(cxlds),
> -						       TASK_UNINTERRUPTIBLE,
> -						       msecs_to_jiffies(timeout)) > 0)
> -				break;
> -		}
> -
> -		if (!cxl_mbox_background_complete(cxlds)) {
> -			dev_err(dev, "timeout waiting for background (%d ms)\n",
> -				timeout * mbox_cmd->poll_count);
> -			return -ETIMEDOUT;
> -		}
> -
> -		bg_status_reg = readq(cxlds->regs.mbox +
> -				      CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
> -		mbox_cmd->return_code =
> -			FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK,
> -				  bg_status_reg);
> -		dev_dbg(dev,
> -			"Mailbox background operation (0x%04x) completed\n",
> -			mbox_cmd->opcode);
> -	}
> -
> -	if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS) {
> +	} else if (mbox_cmd->return_code != CXL_MBOX_CMD_RC_SUCCESS) {
>  		dev_dbg(dev, "Mailbox operation had an error: %s\n",
>  			cxl_mbox_cmd_rc2str(mbox_cmd));
>  		return 0; /* completed but caller must check return_code */
>  	}
>  
> -success:
>  	/* #7 */
>  	cmd_reg = readq(cxlds->regs.mbox + CXLDEV_MBOX_CMD_OFFSET);
>  	out_len = FIELD_GET(CXLDEV_MBOX_CMD_PAYLOAD_LENGTH_MASK, cmd_reg);
> @@ -411,11 +373,71 @@ static int cxl_pci_mbox_send(struct cxl_mailbox *cxl_mbox,
>  			     struct cxl_mbox_cmd *cmd)
>  {
>  	int rc;
> +	struct cxl_dev_state *cxlds = mbox_to_cxlds(cxl_mbox);
> +	struct device *dev = cxlds->dev;
> +	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
>  
>  	mutex_lock(&cxl_mbox->mbox_mutex);
Perhaps scoped_guard() appropriate here to avoid need to unlock explicitly
in the error path.

> +	/*
> +	 * Ensure cxl_mbox_background_complete() checks are safe amongst
> +	 * each other: no new bg operation can occur in between while polling.
> +	 */
> +	if (cxl_is_background_cmd(mds, cmd->opcode)) {
> +		if (atomic_read_acquire(&cxl_mbox->poll_bgop)) {
If you use scoped_guard() the indent will be getting a bit high, so maybe
combine the previous two conditions.
> +			mutex_unlock(&cxl_mbox->mbox_mutex);
> +			return -EBUSY;
> +		}
> +	}
> +
>  	rc = __cxl_pci_mbox_send_cmd(cxl_mbox, cmd);
>  	mutex_unlock(&cxl_mbox->mbox_mutex);
>  
> +	if (cmd->return_code != CXL_MBOX_CMD_RC_BACKGROUND)
> +		return rc;
> +
> +	/*
> +	 * Handle the background command in a synchronous manner. Background
> +	 * operations are timesliced in accordance with the nature of the
> +	 * command.
> +	 */
> +	if (cmd->opcode != CXL_MBOX_OP_SANITIZE &&
> +	    cmd->opcode != CXL_MBOX_OP_ACTIVATE_FW) {
> +		int i, timeout;
> +		u64 bg_status_reg;
> +
> +		timeout = cmd->poll_interval_ms;
> +		for (i = 0; i < cmd->poll_count; i++) {
> +			if (rcuwait_wait_event_timeout(&cxl_mbox->mbox_wait,
> +				       cxl_mbox_background_complete(cxlds),
> +				       TASK_UNINTERRUPTIBLE,
> +				       msecs_to_jiffies(timeout)) > 0)
> +				break;
> +		}
> +
> +		/*
> +		 * In the event of timeout, the mailbox state is indeterminate
> +		 * until the next successful command submission and the driver
> +		 * can get back in sync with the hardware state.
> +		 */
> +		if (!cxl_mbox_background_complete(cxlds)) {
> +			dev_err(dev, "timeout waiting for background (%d ms)\n",
> +				timeout * cmd->poll_count);
> +			rc = -ETIMEDOUT;
> +			goto done;
> +		}
> +
> +		bg_status_reg = readq(cxlds->regs.mbox +
> +				      CXLDEV_MBOX_BG_CMD_STATUS_OFFSET);
> +		cmd->return_code = FIELD_GET(CXLDEV_MBOX_BG_CMD_COMMAND_RC_MASK,
> +					     bg_status_reg);
> +		dev_dbg(dev,
> +			"Mailbox background operation (0x%04x) completed\n",
> +			cmd->opcode);
> +done:
> +		/* ensure clearing poll_bop is the last operation */
> +		atomic_set_release(&cxl_mbox->poll_bgop, 0);
> +	}
> +
>  	return rc;
>  }