* Re: [RFC PATCH 1/3] cxl: Modify background cmd handling for the others
2023-09-22 13:05 ` [RFC PATCH 1/3] cxl: Modify background cmd handling for the others Jeongtae Park
@ 2023-09-25 23:44 ` Dave Jiang
0 siblings, 0 replies; 2+ messages in thread
From: Dave Jiang @ 2023-09-25 23:44 UTC (permalink / raw)
To: Jeongtae Park, linux-cxl
Cc: Dan Williams, Alison Schofield, Vishal Verma, Ben Widawsky,
Jonathan Cameron, Davidlohr Bueso, Fan Ni, Kyungsan Kim,
Wonjae Lee, Hojin Nam, Junhyeok Im, Jehoon Park, Jeongtae Park
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;
^ permalink raw reply [flat|nested] 2+ messages in thread