Re: [RFC PATCH 1/3] cxl: Modify background cmd handling for the others

[Dave Jiang <dave.jiang@intel.com>]

On 9/22/23 06:05, Jeongtae Park wrote:
> This patch modifies sanitize command handling to support background-capable
> commands asynchronously also. Much of the implementation follows the approach
> of the sanitize command implementation, with the addition of 'bgmode' to
> cxl_mbox_cmd explicitly could choose how each command is handled. However,
> even if you want asynchronous processing via irq, it will fallback to poll
> based if the device doesn't support it. Added a new cxl_bgcmd_state by
> moving the necessary data structures from cxl_security_state.
> 
> Signed-off-by: Jeongtae Park <jtp.park@samsung.com>
> Signed-off-by: Hojin Nam <hj96.nam@samsung.com>
> ---
>  drivers/cxl/core/memdev.c |  8 ++---
>  drivers/cxl/cxlmem.h      | 33 +++++++++++++++++---
>  drivers/cxl/pci.c         | 65 +++++++++++++++++++--------------------
>  3 files changed, 64 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index f99e7ec3cc40..8d1692231767 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -537,13 +537,13 @@ void clear_exclusive_cxl_commands(struct cxl_memdev_state *mds,
>  }
>  EXPORT_SYMBOL_NS_GPL(clear_exclusive_cxl_commands, CXL);
>  
> -static void cxl_memdev_security_shutdown(struct device *dev)
> +static void cxl_memdev_bgcmd_shutdown(struct device *dev)
>  {
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlmd->cxlds);
>  
> -	if (mds->security.poll)
> -		cancel_delayed_work_sync(&mds->security.poll_dwork);
> +	if (mds->bgcmd.poll)
> +		cancel_delayed_work_sync(&mds->bgcmd.poll_dwork);
>  }
>  
>  static void cxl_memdev_shutdown(struct device *dev)
> @@ -551,7 +551,7 @@ static void cxl_memdev_shutdown(struct device *dev)
>  	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
>  
>  	down_write(&cxl_memdev_rwsem);
> -	cxl_memdev_security_shutdown(dev);
> +	cxl_memdev_bgcmd_shutdown(dev);
>  	cxlmd->cxlds = NULL;
>  	up_write(&cxl_memdev_rwsem);
>  }
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 79e99c873ca2..879903a8c822 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -100,6 +100,13 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
>  	return xa_load(&port->endpoints, (unsigned long)&cxlmd->dev);
>  }
>  
> +enum cxl_bg_cmd_mode {
> +#define CXL_BG_CMD_ASYNC	BIT_MASK(0)
> +	CXL_BG_CMD_SYNC		= 0x00,
> +	CXL_BG_CMD_ASYNC_POLL	= 0x01,
> +	CXL_BG_CMD_ASYNC_INT	= 0x03
> +};
> +
>  /**
>   * struct cxl_mbox_cmd - A command to be submitted to hardware.
>   * @opcode: (input) The command set and command submitted to hardware.
> @@ -113,6 +120,8 @@ static inline struct cxl_ep *cxl_ep_load(struct cxl_port *port,
>   *            variable sized output commands, it tells the exact number of bytes
>   *            written.
>   * @min_out: (input) internal command output payload size validation
> + * @bg_mode: (input) The processing mode of a background command. It will be
> + *	     valid only when the background command started successfully.
>   * @poll_count: (input) Number of timeouts to attempt.
>   * @poll_interval_ms: (input) Time between mailbox background command polling
>   *                    interval timeouts.
> @@ -131,6 +140,7 @@ struct cxl_mbox_cmd {
>  	size_t size_in;
>  	size_t size_out;
>  	size_t min_out;
> +	int bg_mode;
>  	int poll_count;
>  	int poll_interval_ms;
>  	u16 return_code;
> @@ -346,17 +356,31 @@ struct cxl_fw_state {
>   * struct cxl_security_state - Device security state
>   *
>   * @state: state of last security operation
> - * @poll: polling for sanitization is enabled, device has no mbox irq support
> - * @poll_tmo_secs: polling timeout
> - * @poll_dwork: polling work item
>   * @sanitize_node: sanitation sysfs file to notify
>   */
>  struct cxl_security_state {
>  	unsigned long state;
> +	struct kernfs_node *sanitize_node;

The sanitize_node is moved here from cxl_bgcmd_state, but does not appear to be used any longer. Does it need to just be removed?

DJ

> +};
> +
> +/**
> + * struct cxl_bgcmd_state - Device background command state
> + *
> + * @poll: polling for background command completion, device has no mbox
> + *        irq support
> + * @poll_tmo_secs: polling timeout
> + * @poll_dwork: polling work item
> + * @opcode: The background command submitted to hardware
> + * @mode: The background command submitted to hardware
> + * @complete_node: background command completion file to notify
> + */
> +struct cxl_bgcmd_state {
>  	bool poll;
>  	int poll_tmo_secs;
>  	struct delayed_work poll_dwork;
> -	struct kernfs_node *sanitize_node;
> +	u16 opcode;
> +	int mode;
> +	struct kernfs_node *complete_node;
>  };
>  
>  /*
> @@ -460,6 +484,7 @@ struct cxl_memdev_state {
>  	struct cxl_poison_state poison;
>  	struct cxl_security_state security;
>  	struct cxl_fw_state fw;
> +	struct cxl_bgcmd_state bgcmd;
>  
>  	struct rcuwait mbox_wait;
>  	int (*mbox_send)(struct cxl_memdev_state *mds,
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 1cb1494c28fe..570fca24ab12 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -116,8 +116,6 @@ static bool cxl_mbox_background_complete(struct cxl_dev_state *cxlds)
>  
>  static irqreturn_t cxl_pci_mbox_irq(int irq, void *id)
>  {
> -	u64 reg;
> -	u16 opcode;
>  	struct cxl_dev_id *dev_id = id;
>  	struct cxl_dev_state *cxlds = dev_id->cxlds;
>  	struct cxl_memdev_state *mds = to_cxl_memdev_state(cxlds);
> @@ -125,13 +123,12 @@ static irqreturn_t cxl_pci_mbox_irq(int irq, void *id)
>  	if (!cxl_mbox_background_complete(cxlds))
>  		return IRQ_NONE;
>  
> -	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) {
> -		if (mds->security.sanitize_node)
> -			sysfs_notify_dirent(mds->security.sanitize_node);
> +	if (mds->bgcmd.mode & CXL_BG_CMD_ASYNC) {
> +		if (mds->bgcmd.complete_node)
> +			sysfs_notify_dirent(mds->bgcmd.complete_node);
>  
> -		dev_dbg(cxlds->dev, "Sanitization operation ended\n");
> +		dev_dbg(cxlds->dev, "Mailbox background operation (0x%04x) ended\n",
> +			mds->bgcmd.opcode);
>  	} else {
>  		/* short-circuit the wait in __cxl_pci_mbox_send_cmd() */
>  		rcuwait_wake_up(&mds->mbox_wait);
> @@ -141,28 +138,29 @@ static irqreturn_t cxl_pci_mbox_irq(int irq, void *id)
>  }
>  
>  /*
> - * Sanitization operation polling mode.
> + * Background operation polling mode.
>   */
> -static void cxl_mbox_sanitize_work(struct work_struct *work)
> +static void cxl_mbox_bg_work(struct work_struct *work)
>  {
>  	struct cxl_memdev_state *mds =
> -		container_of(work, typeof(*mds), security.poll_dwork.work);
> +		container_of(work, typeof(*mds), bgcmd.poll_dwork.work);
>  	struct cxl_dev_state *cxlds = &mds->cxlds;
>  
>  	mutex_lock(&mds->mbox_mutex);
>  	if (cxl_mbox_background_complete(cxlds)) {
> -		mds->security.poll_tmo_secs = 0;
> +		mds->bgcmd.poll_tmo_secs = 0;
>  		put_device(cxlds->dev);
>  
> -		if (mds->security.sanitize_node)
> -			sysfs_notify_dirent(mds->security.sanitize_node);
> +		if (mds->bgcmd.complete_node)
> +			sysfs_notify_dirent(mds->bgcmd.complete_node);
>  
> -		dev_dbg(cxlds->dev, "Sanitization operation ended\n");
> +		dev_dbg(cxlds->dev, "Mailbox background operation (0x%04x) ended\n",
> +			mds->bgcmd.opcode);
>  	} else {
> -		int timeout = mds->security.poll_tmo_secs + 10;
> +		int timeout = mds->bgcmd.poll_tmo_secs + 10;
>  
> -		mds->security.poll_tmo_secs = min(15 * 60, timeout);
> -		queue_delayed_work(system_wq, &mds->security.poll_dwork,
> +		mds->bgcmd.poll_tmo_secs = min(15 * 60, timeout);
> +		queue_delayed_work(system_wq, &mds->bgcmd.poll_dwork,
>  				   timeout * HZ);
>  	}
>  	mutex_unlock(&mds->mbox_mutex);
> @@ -234,7 +232,8 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_memdev_state *mds,
>  	 * not be in sync. Ensure no new command comes in until so. Keep the
>  	 * hardware semantics and only allow device health status.
>  	 */
> -	if (mds->security.poll_tmo_secs > 0) {
> +	if (mds->bgcmd.poll_tmo_secs > 0 &&
> +			mds->bgcmd.opcode == CXL_MBOX_OP_SANITIZE) {
>  		if (mbox_cmd->opcode != CXL_MBOX_OP_GET_HEALTH_INFO)
>  			return -EBUSY;
>  	}
> @@ -289,31 +288,29 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_memdev_state *mds,
>  		u64 bg_status_reg;
>  		int i, timeout;
>  
> -		/*
> -		 * Sanitization is a special case which monopolizes the device
> -		 * and cannot be timesliced. Handle asynchronously instead,
> -		 * and allow userspace to poll(2) for completion.
> -		 */
> -		if (mbox_cmd->opcode == CXL_MBOX_OP_SANITIZE) {
> -			if (mds->security.poll) {
> +		dev_dbg(dev, "Mailbox background operation (0x%04x) started\n",
> +			mbox_cmd->opcode);
> +
> +		mds->bgcmd.opcode = mbox_cmd->opcode;
> +		mds->bgcmd.mode = mbox_cmd->bg_mode;
> +
> +		if (mbox_cmd->bg_mode & CXL_BG_CMD_ASYNC) {
> +			if (mbox_cmd->bg_mode == CXL_BG_CMD_ASYNC_POLL ||
> +					mds->bgcmd.poll) {
>  				/* hold the device throughout */
>  				get_device(cxlds->dev);
>  
>  				/* give first timeout a second */
>  				timeout = 1;
> -				mds->security.poll_tmo_secs = timeout;
> +				mds->bgcmd.poll_tmo_secs = timeout;
>  				queue_delayed_work(system_wq,
> -						   &mds->security.poll_dwork,
> +						   &mds->bgcmd.poll_dwork,
>  						   timeout * HZ);
>  			}
>  
> -			dev_dbg(dev, "Sanitization operation started\n");
>  			goto success;
>  		}
>  
> -		dev_dbg(dev, "Mailbox background operation (0x%04x) started\n",
> -			mbox_cmd->opcode);
> -
>  		timeout = mbox_cmd->poll_interval_ms;
>  		for (i = 0; i < mbox_cmd->poll_count; i++) {
>  			if (rcuwait_wait_event_timeout(&mds->mbox_wait,
> @@ -460,8 +457,8 @@ static int cxl_pci_setup_mailbox(struct cxl_memdev_state *mds)
>  	}
>  
>  mbox_poll:
> -	mds->security.poll = true;
> -	INIT_DELAYED_WORK(&mds->security.poll_dwork, cxl_mbox_sanitize_work);
> +	mds->bgcmd.poll = true;
> +	INIT_DELAYED_WORK(&mds->bgcmd.poll_dwork, cxl_mbox_bg_work);
>  
>  	dev_dbg(cxlds->dev, "Mailbox interrupts are unsupported");
>  	return 0;