Re: [PATCH 2/6] cxl/mbox: Add sanitation handling machinery

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

On 5/25/23 20:33, Davidlohr Bueso wrote:
> Sanitation is by definition a device-monopolizing operation, and thus
> the timeslicing rules for other background commands do not apply.
> As such handle this special case asynchronously and return immediately.
> Subsequent changes will allow completion to be pollable from userspace
> via a sysfs file interface.
>
> For devices that don't support interrupts for notifying background
> command completion, self-poll with the caveat that the poller can
> be out of sync with the ready hardware, and therefore care must be
> taken to not allow any new commands to go through until the poller
> sees the hw completion. The poller takes the mbox_mutex to stabilize
> the flagging, minimizing any runtime overhead in the send path to
> check for 'sanitize_tmo' for uncommon poll scenarios. This flag
> also serves for sanitation (the only user of async polling) to know
> when to queue work or simply rely on irqs.
>
> The irq case is much simpler as hardware will serialize/error
> appropriately.
>
> Signed-off-by: Davidlohr Bueso <dave@stgolabs.net>

Just a minor nit below, otherwise

Reviewed-by: Dave Jiang <dave.jiang@intel.com>


> ---
>   drivers/cxl/core/memdev.c | 10 +++++
>   drivers/cxl/cxlmem.h      | 10 +++++
>   drivers/cxl/pci.c         | 83 +++++++++++++++++++++++++++++++++++++--
>   3 files changed, 99 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 6e1d7d3610a2..02763e83545c 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -460,11 +460,21 @@ void clear_exclusive_cxl_commands(struct cxl_dev_state *cxlds, unsigned long *cm
>   }
>   EXPORT_SYMBOL_NS_GPL(clear_exclusive_cxl_commands, CXL);
>   
> +static void cxl_memdev_security_shutdown(struct device *dev)
> +{
> +	struct cxl_memdev *cxlmd = to_cxl_memdev(dev);
> +	struct cxl_dev_state *cxlds = cxlmd->cxlds;
> +
> +	if (cxlds->security.poll_tmo_secs != -1)
> +		cancel_delayed_work_sync(&cxlds->security.poll_dwork);
> +}
> +
>   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);
>   	cxlmd->cxlds = NULL;
>   	up_write(&cxl_memdev_rwsem);
>   }
> diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> index 5329274b0076..02ec68f97de2 100644
> --- a/drivers/cxl/cxlmem.h
> +++ b/drivers/cxl/cxlmem.h
> @@ -264,9 +264,18 @@ struct cxl_poison_state {
>    * struct cxl_security_state - Device security state
>    *
>    * @state: state of last security operation
> + * @poll_tmo_secs: polling timeout
> + * @poll_dwork: polling work item
> + *
> + * Polling (sanitation) is only used when device mbox irqs are not
> + * supported. As such, @poll_tmo_secs == -1 indicates that polling
> + * is disabled. Otherwise, when enabled, @poll_tmo_secs is maxed
> + * at 15 minutes and serialized by the mbox_mutex.
>    */
>   struct cxl_security_state {
>   	unsigned long state;
> +	int poll_tmo_secs;
> +	struct delayed_work poll_dwork;
>   };
>   
>   /**
> @@ -380,6 +389,7 @@ enum cxl_opcode {
>   	CXL_MBOX_OP_GET_SCAN_MEDIA_CAPS	= 0x4303,
>   	CXL_MBOX_OP_SCAN_MEDIA		= 0x4304,
>   	CXL_MBOX_OP_GET_SCAN_MEDIA	= 0x4305,
> +	CXL_MBOX_OP_SANITIZE		= 0x4400,
>   	CXL_MBOX_OP_GET_SECURITY_STATE	= 0x4500,
>   	CXL_MBOX_OP_SET_PASSPHRASE	= 0x4501,
>   	CXL_MBOX_OP_DISABLE_PASSPHRASE	= 0x4502,
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index a78e40e6d0e0..a0d93719ab18 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -115,16 +115,52 @@ 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;
>   
> -	/* short-circuit the wait in __cxl_pci_mbox_send_cmd() */
> -	if (cxl_mbox_background_complete(cxlds))
> -		rcuwait_wake_up(&cxlds->mbox_wait);
> +	if (!cxl_mbox_background_complete(cxlds))
> +		goto done;
>   
> +	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) {
> +		dev_dbg(cxlds->dev, "Sanitation operation ended\n");
> +	} else {
> +		/* short-circuit the wait in __cxl_pci_mbox_send_cmd() */
> +		rcuwait_wake_up(&cxlds->mbox_wait);
> +	}
> +done:
>   	return IRQ_HANDLED;
>   }
>   
> +/*
> + * Sanitation operation polling mode.
> + */
> +static void cxl_mbox_sanitize_work(struct work_struct *work)
> +{
> +	struct cxl_dev_state *cxlds;
> +
> +	cxlds = container_of(work,
> +			     struct cxl_dev_state, security.poll_dwork.work);
> +
> +	mutex_lock(&cxlds->mbox_mutex);
> +	if (cxl_mbox_background_complete(cxlds)) {
> +		cxlds->security.poll_tmo_secs = 0;
> +		put_device(cxlds->dev);
> +
> +		dev_dbg(cxlds->dev, "Sanitation operation ended\n");
> +	} else {
> +		int timeout = cxlds->security.poll_tmo_secs + 10;
> +
> +		cxlds->security.poll_tmo_secs = min(15 * 60, timeout);
> +		queue_delayed_work(system_wq, &cxlds->security.poll_dwork,
> +				   timeout * HZ);
> +	}
> +	mutex_unlock(&cxlds->mbox_mutex);
> +}
> +
>   /**
>    * __cxl_pci_mbox_send_cmd() - Execute a mailbox command
>    * @cxlds: The device state to communicate with.
> @@ -185,6 +221,16 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
>   		return -EBUSY;
>   	}
>   
> +	/*
> +	 * With sanitize polling, hardware might be done and the poller still
> +	 * not be in sync. Ensure no new command comes in until so. Keep the
> +	 * hardware semantics and only allow device health status.
> +	 */
> +	if (unlikely(cxlds->security.poll_tmo_secs > 0)) {
> +		if (mbox_cmd->opcode != CXL_MBOX_OP_GET_HEALTH_INFO)
> +			return -EBUSY;
> +	}
> +
>   	cmd_reg = FIELD_PREP(CXLDEV_MBOX_CMD_COMMAND_OPCODE_MASK,
>   			     mbox_cmd->opcode);
>   	if (mbox_cmd->size_in) {
> @@ -233,11 +279,34 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
>   	 */
>   	if (mbox_cmd->return_code == CXL_MBOX_CMD_RC_BACKGROUND) {
>   		u64 bg_status_reg;
> -		int i, timeout = mbox_cmd->poll_interval_ms;
> +		int i, timeout;
> +
> +		/*
> +		 * Sanitation 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 (cxlds->security.poll_tmo_secs != -1) {
> +				/* hold the device throughout */
> +				get_device(cxlds->dev);
> +
> +				/* give first timeout a second */
> +				timeout = 1;
> +				cxlds->security.poll_tmo_secs = timeout;
> +				queue_delayed_work(system_wq,
> +						   &cxlds->security.poll_dwork,
> +						   timeout * HZ);
> +			}
> +
> +			dev_dbg(dev, "Sanitation 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(&cxlds->mbox_wait,
>   				       cxl_mbox_background_complete(cxlds),
> @@ -268,6 +337,7 @@ static int __cxl_pci_mbox_send_cmd(struct cxl_dev_state *cxlds,
>   		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);
> @@ -376,10 +446,15 @@ static int cxl_pci_setup_mailbox(struct cxl_dev_state *cxlds)
>   		ctrl |= CXLDEV_MBOX_CTRL_BG_CMD_IRQ;
>   		writel(ctrl, cxlds->regs.mbox + CXLDEV_MBOX_CTRL_OFFSET);
>   
> +		/* flag that irqs are enabled */
> +		cxlds->security.poll_tmo_secs = -1;

Use a #define instead of -1 magic number? CXL_CMD_TIMEOUT_INVALID 
perhaps? Would also apply to all the checking of poll_tmo_secs in this 
patch.


>   		return 0;
>   	}
>   
>   mbox_poll:
> +	cxlds->security.poll_tmo_secs = 0;
> +	INIT_DELAYED_WORK(&cxlds->security.poll_dwork,
> +			  cxl_mbox_sanitize_work);
>   	dev_dbg(cxlds->dev, "Mailbox interrupts are unsupported");
>   	return 0;
>   }