[PATCH] nvme: requeue aen after firmware activation

[PATCH] nvme: requeue aen after firmware activation

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The driver prevents async event work while handling a processing paused
event, but someone needs to restart it after the controller returns to a
live state.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=216400
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 drivers/nvme/host/core.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index af367b22871b..66446f1e06cf 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4703,6 +4703,8 @@ static void nvme_fw_act_work(struct work_struct *work)
 	nvme_start_queues(ctrl);
 	/* read FW slot information to clear the AER */
 	nvme_get_fw_slot_info(ctrl);
+
+	queue_work(nvme_wq, &ctrl->async_event_work);
 }
 
 static u32 nvme_aer_type(u32 result)
@@ -4715,9 +4717,10 @@ static u32 nvme_aer_subtype(u32 result)
 	return (result & 0xff00) >> 8;
 }
 
-static void nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result)
+static bool nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result)
 {
 	u32 aer_notice_type = nvme_aer_subtype(result);
+	bool requeue = true;
 
 	trace_nvme_async_event(ctrl, aer_notice_type);
 
@@ -4734,6 +4737,7 @@ static void nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result)
 		 */
 		if (nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING)) {
 			nvme_auth_stop(ctrl);
+			requeue = false;
 			queue_work(nvme_wq, &ctrl->fw_act_work);
 		}
 		break;
@@ -4750,6 +4754,7 @@ static void nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result)
 	default:
 		dev_warn(ctrl->device, "async event result %08x\n", result);
 	}
+	return requeue;
 }
 
 static void nvme_handle_aer_persistent_error(struct nvme_ctrl *ctrl)
@@ -4765,13 +4770,14 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 	u32 result = le32_to_cpu(res->u32);
 	u32 aer_type = nvme_aer_type(result);
 	u32 aer_subtype = nvme_aer_subtype(result);
+	bool requeue = true;
 
 	if (le16_to_cpu(status) >> 1 != NVME_SC_SUCCESS)
 		return;
 
 	switch (aer_type) {
 	case NVME_AER_NOTICE:
-		nvme_handle_aen_notice(ctrl, result);
+		requeue = nvme_handle_aen_notice(ctrl, result);
 		break;
 	case NVME_AER_ERROR:
 		/*
@@ -4792,7 +4798,9 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 	default:
 		break;
 	}
-	queue_work(nvme_wq, &ctrl->async_event_work);
+
+	if (requeue)
+		queue_work(nvme_wq, &ctrl->async_event_work);
 }
 EXPORT_SYMBOL_GPL(nvme_complete_async_event);
 
-- 
2.30.2

Re: [PATCH] nvme: requeue aen after firmware activation

[Sagi Grimberg]

On 9/1/22 18:30, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> The driver prevents async event work while handling a processing paused
> event, but someone needs to restart it after the controller returns to a
> live state.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=216400
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/nvme/host/core.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index af367b22871b..66446f1e06cf 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4703,6 +4703,8 @@ static void nvme_fw_act_work(struct work_struct *work)
>   	nvme_start_queues(ctrl);
>   	/* read FW slot information to clear the AER */
>   	nvme_get_fw_slot_info(ctrl);
> +
> +	queue_work(nvme_wq, &ctrl->async_event_work);

Why is the rest below needed? why is this not enough? Also a code
comment would help here...

Re: [PATCH] nvme: requeue aen after firmware activation

[Keith Busch]

On Mon, Sep 05, 2022 at 12:10:24PM +0300, Sagi Grimberg wrote:
> 
> 
> On 9/1/22 18:30, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> > 
> > The driver prevents async event work while handling a processing paused
> > event, but someone needs to restart it after the controller returns to a
> > live state.
> > 
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216400
> > Signed-off-by: Keith Busch <kbusch@kernel.org>
> > ---
> >   drivers/nvme/host/core.c | 14 +++++++++++---
> >   1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index af367b22871b..66446f1e06cf 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -4703,6 +4703,8 @@ static void nvme_fw_act_work(struct work_struct *work)
> >   	nvme_start_queues(ctrl);
> >   	/* read FW slot information to clear the AER */
> >   	nvme_get_fw_slot_info(ctrl);
> > +
> > +	queue_work(nvme_wq, &ctrl->async_event_work);
> 
> Why is the rest below needed? why is this not enough? Also a code
> comment would help here...

We can only allow one thread to queue the work because no one is tracking
outstanding command id's on AEN commands. If nvme_fw_act_work needs to take
responsibility to restart the async_event_work, then we can't let the previous
handler also attempt to requeue it.

Re: [PATCH] nvme: requeue aen after firmware activation

[Christoph Hellwig]

Thanks,

applied to nvme-6.0.