Re: [PATCHv2 6/6] nvme: Wait for reset state when required

Sagi Grimberg
2019-09-20T18:13:17Z

On 9/19/19 2:34 PM, kbusch@kernel.org wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> Provide a method to block until we've successfully entered the
> RESETTING state so that we can ensure disabling the controller can
> not be interrupted by another one of the various controller reset
> paths. Otherwise, these simultaneous controller disabling/enabling tasks
> may interfere with each other and leave the controller in the wrong state.
>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
> drivers/nvme/host/core.c | 36 ++++++++++++++++++++++++++++++-
> drivers/nvme/host/nvme.h | 4 ++++
> drivers/nvme/host/pci.c | 46 +++++++++++++++++++++++++++-------------
> 3 files changed, 70 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 33b5729763c2..3c75459c28bb 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -374,8 +374,10 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
> break;
> }
>
> - if (changed)
> + if (changed) {
> ctrl->state = new_state;
> + wake_up_all(&ctrl->state_wq);
> + }
>
> spin_unlock_irqrestore(&ctrl->lock, flags);
> if (changed && ctrl->state == NVME_CTRL_LIVE)
> @@ -384,6 +386,37 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
> }
> EXPORT_SYMBOL_GPL(nvme_change_ctrl_state);
>
> +/*
> + * Returns false for sink states that can't ever transition back to live.
> + */
> +static bool nvme_state_transient(struct nvme_ctrl *ctrl)
> +{
> + switch (ctrl->state) {
> + case NVME_CTRL_NEW:
> + case NVME_CTRL_LIVE:
> + case NVME_CTRL_RESETTING:
> + case NVME_CTRL_CONNECTING:
> + return true;
> + case NVME_CTRL_DELETING:
> + case NVME_CTRL_DEAD:
> + default:
> + return false;
> + }
> +}
> +
> +/*
> + * Waits for the controller state to be resetting, or returns false if it is
> + * not possible to ever transition to that state.
> + */
> +bool nvme_wait_reset(struct nvme_ctrl *ctrl)
> +{
> + wait_event(ctrl->state_wq,
> + nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING) ||
> + !nvme_state_transient(ctrl));
> + return ctrl->state == NVME_CTRL_RESETTING;
> +}
> +EXPORT_SYMBOL_GPL(nvme_wait_reset);
> +
> static void nvme_free_ns_head(struct kref *ref)
> {
> struct nvme_ns_head *head =
> @@ -3891,6 +3924,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
> INIT_WORK(&ctrl->async_event_work, nvme_async_event_work);
> INIT_WORK(&ctrl->fw_act_work, nvme_fw_act_work);
> INIT_WORK(&ctrl->delete_work, nvme_delete_ctrl_work);
> + init_waitqueue_head(&ctrl->state_wq);
>
> INIT_DELAYED_WORK(&ctrl->ka_work, nvme_keep_alive_work);
> memset(&ctrl->ka_cmd, 0, sizeof(ctrl->ka_cmd));
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 3ad16a81370b..43ac1244c4f7 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -15,6 +15,7 @@
> #include <linux/sed-opal.h>
> #include <linux/fault-inject.h>
> #include <linux/rcupdate.h>
> +#include <linux/wait.h>
>
> #include <trace/events/block.h>
>
> @@ -198,6 +199,7 @@ struct nvme_ctrl {
> struct cdev cdev;
> struct work_struct reset_work;
> struct work_struct delete_work;
> + wait_queue_head_t state_wq;
>
> struct nvme_subsystem *subsys;
> struct list_head subsys_entry;
> @@ -448,6 +450,7 @@ void nvme_complete_rq(struct request *req);
> bool nvme_cancel_request(struct request *req, void *data, bool reserved);
> bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
> enum nvme_ctrl_state new_state);
> +bool nvme_wait_reset(struct nvme_ctrl *ctrl);
> int nvme_disable_ctrl(struct nvme_ctrl *ctrl);
> int nvme_enable_ctrl(struct nvme_ctrl *ctrl);
> int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl);
> @@ -499,6 +502,7 @@ void nvme_stop_keep_alive(struct nvme_ctrl *ctrl);
> int nvme_reset_ctrl(struct nvme_ctrl *ctrl);
> int nvme_reset_continue(struct nvme_ctrl *ctrl);
> int nvme_reset_ctrl_sync(struct nvme_ctrl *ctrl);
> +int nvme_reset_continue(struct nvme_ctrl *ctrl);
> int nvme_delete_ctrl(struct nvme_ctrl *ctrl);
>
> int nvme_get_log(struct nvme_ctrl *ctrl, u32 nsid, u8 log_page, u8 lsp,
> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
> index 9a9e90efcc95..41eb89bf0be3 100644
> --- a/drivers/nvme/host/pci.c
> +++ b/drivers/nvme/host/pci.c
> @@ -2461,6 +2461,14 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
> mutex_unlock(&dev->shutdown_lock);
> }
>
> +static int nvme_disable_prepare_reset(struct nvme_dev *dev, bool shutdown)
> +{
> + if (!nvme_wait_reset(&dev->ctrl))
> + return -EBUSY;
> + nvme_dev_disable(dev, shutdown);
> + return 0;
> +}
> +
> static int nvme_setup_prp_pools(struct nvme_dev *dev)
> {
> dev->prp_page_pool = dma_pool_create("prp list page", dev->dev,
> @@ -2508,6 +2516,11 @@ static void nvme_pci_free_ctrl(struct nvme_ctrl *ctrl)
>
> static void nvme_remove_dead_ctrl(struct nvme_dev *dev)
> {
> + /*
> + * Set state to deleting now to avoid blocking nvme_wait_reset(), which
> + * may be holding this pci_dev's device lock.
> + */
> + nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_DELETING);
> nvme_get_ctrl(&dev->ctrl);
> nvme_dev_disable(dev, false);
> nvme_kill_queues(&dev->ctrl);
> @@ -2833,19 +2846,28 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> static void nvme_reset_prepare(struct pci_dev *pdev)
> {
> struct nvme_dev *dev = pci_get_drvdata(pdev);
> - nvme_dev_disable(dev, false);
> +
> + /*
> + * We don't need to check the return value from waiting for the reset
> + * state as pci_dev device lock is held, making it impossible to race
> + * with ->remove().
> + */
> + nvme_disable_prepare_reset(dev, false);
> + nvme_sync_queues(&dev->ctrl);
> }
>
> static void nvme_reset_done(struct pci_dev *pdev)
> {
> struct nvme_dev *dev = pci_get_drvdata(pdev);
> - nvme_reset_ctrl_sync(&dev->ctrl);
> +
> + if (!nvme_reset_continue(&dev->ctrl))
> + flush_work(&dev->ctrl.reset_work);

Is nvme_reset_continue allowed to fail here?
As I see it, the flush must complete regardless doesn't it?

> }
>
> static void nvme_shutdown(struct pci_dev *pdev)
> {
> struct nvme_dev *dev = pci_get_drvdata(pdev);
> - nvme_dev_disable(dev, true);
> + nvme_disable_prepare_reset(dev, true);
> }
>
> /*
> @@ -2898,7 +2920,7 @@ static int nvme_resume(struct device *dev)
>
> if (ndev->last_ps == U32_MAX ||
> nvme_set_power_state(ctrl, ndev->last_ps) != 0)
> - nvme_reset_ctrl(ctrl);
> + return nvme_reset_continue(&ndev->ctrl);

Is the controller allowed not to be in RESETTING in this stage?
do we need a WARN_ON if it is?

> return 0;
> }
>
> @@ -2926,10 +2948,8 @@ static int nvme_suspend(struct device *dev)
> */
> if (pm_suspend_via_firmware() || !ctrl->npss ||
> !pcie_aspm_enabled(pdev) ||
> - (ndev->ctrl.quirks & NVME_QUIRK_SIMPLE_SUSPEND)) {
> - nvme_dev_disable(ndev, true);
> - return 0;
> - }
> + (ndev->ctrl.quirks & NVME_QUIRK_SIMPLE_SUSPEND))
> + return nvme_disable_prepare_reset(ndev, true);
>
> nvme_start_freeze(ctrl);
> nvme_wait_freeze(ctrl);
> @@ -2951,9 +2971,8 @@ static int nvme_suspend(struct device *dev)
> * Clearing npss forces a controller reset on resume. The
> * correct value will be resdicovered then.
> */
> - nvme_dev_disable(ndev, true);
> + ret = nvme_disable_prepare_reset(ndev, true);
> ctrl->npss = 0;
> - ret = 0;
> goto unfreeze;
> }
> /*
> @@ -2970,9 +2989,7 @@ static int nvme_suspend(struct device *dev)
> static int nvme_simple_suspend(struct device *dev)
> {
> struct nvme_dev *ndev = pci_get_drvdata(to_pci_dev(dev));
> -
> - nvme_dev_disable(ndev, true);
> - return 0;
> + return nvme_disable_prepare_reset(ndev, true);
> }
>
> static int nvme_simple_resume(struct device *dev)
> @@ -2980,8 +2997,7 @@ static int nvme_simple_resume(struct device *dev)
> struct pci_dev *pdev = to_pci_dev(dev);
> struct nvme_dev *ndev = pci_get_drvdata(pdev);
>
> - nvme_reset_ctrl(&ndev->ctrl);
> - return 0;
> + return nvme_reset_continue(&ndev->ctrl);

Same here...