Support for FW activation without reset

This patch adds support for handling Fw activation without reset On completion of FW-activation-starting AER, all queues are paused till CSTS.PP is cleared or timed out (exceeds max time for fw activtion MTFA). If device fails to clear CSTS.PP within MTFA, driver issues reset controller

drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 2322

Quoted 2 lines · Arnav Dawn

drivers/nvme/host/core.c +2 -0

+				ctrl->fw_act_timeout = jiffies +
+					msecs_to_jiffies(ctrl->mtfa * 100);

Instead of adding another field to the nvme_ctrl structure, just calculate the timeout in your nvme_fw_act_work function.

drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 2327

Quoted 6 lines

drivers/nvme/host/core.c +5 -0

+			else
+				ctrl->fw_act_timeout = jiffies +
+					msecs_to_jiffies(admin_timeout * 1000);
+
+			schedule_delayed_work(&ctrl->fw_act_work, 0);

If scheduling with 0 delay, why is this delayed work?

Quoted 6 lines · Keith Busch

drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 2322

Quoted 2 lines · Arnav Dawn

drivers/nvme/host/core.c +2 -0

+				ctrl->fw_act_timeout = jiffies +
+					msecs_to_jiffies(ctrl->mtfa * 100);

Instead of adding another field to the nvme_ctrl structure, just calculate the timeout in your nvme_fw_act_work function.

intention was to set fw_act_timeout as soon as the AER is received. Since work could be scheduled after some time, setting timeout in work function would add that delay to it.

Quoted 6 lines

drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 2327

Quoted 5 lines

drivers/nvme/host/core.c +5 -0

+			else
+				ctrl->fw_act_timeout = jiffies +
+					msecs_to_jiffies(admin_timeout * 1000);
+
+			schedule_delayed_work(&ctrl->fw_act_work, 0);

If scheduling with 0 delay, why is this delayed work?

I used delayed work so i could use cancel_delayed_work, as cancel_work was not available.

drivers/nvme/host/core.c +4 -0

+    case NVME_AER_ERR_FW_IMG_LOAD:
+        dev_warn(ctrl->device, "FW image load error\n");
+        cancel_delayed_work(&ctrl->fw_act_work);
+        break;
     default:

Quoted 7 lines · Arnav Dawn

Quoted 6 lines · Keith Busch

drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 2322

Quoted 2 lines · Arnav Dawn

drivers/nvme/host/core.c +2 -0

+				ctrl->fw_act_timeout = jiffies +
+					msecs_to_jiffies(ctrl->mtfa * 100);

Instead of adding another field to the nvme_ctrl structure, just calculate the timeout in your nvme_fw_act_work function.

Quoted 3 lines

intention was to set fw_act_timeout as soon as the AER is received. Since work could be scheduled after some time, setting timeout in work function would add that delay to it.

This feature doesn't require such tight constraints. The 100ms sleep granularity in your CSTS.PP polling already exceeds the amount of time it takes for work to schedule.

Quoted 8 lines

Quoted 6 lines

drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 2327

Quoted 5 lines

drivers/nvme/host/core.c +5 -0

+			else
+				ctrl->fw_act_timeout = jiffies +
+					msecs_to_jiffies(admin_timeout * 1000);
+
+			schedule_delayed_work(&ctrl->fw_act_work, 0);

If scheduling with 0 delay, why is this delayed work?

I used delayed work so i could use cancel_delayed_work, as cancel_work was not available.

If you really have a use for cancel_work, you could send a patch to export the symbol.

In any case, that's probably not going to do what you want. The work can only be cancelled if it hasn't started, and since you start it without delay, the work will likely be running. Maybe you want to add some other criteria for the nvme_fw_act_work to end early, though I expect CSTS.PP to clear if f/w load failed.

One last thing I noticed, it looks like the nvme_fw_act_work will break if an IO timeout occurs while it's running since that may reset the controller and restart IO queues.

Quoted 4 lines · Keith Busch

One last thing I noticed, it looks like the nvme_fw_act_work will break if an IO timeout occurs while it's running since that may reset the controller and restart IO queues.

could you please clarify more?
I think ,that, if an IO Timeout resets controller and restarts queue, the nvme_ctrl_pp_status will return false, ending the nvme_fw_act_work.

Quoted 7 lines · Arnav Dawn

Quoted 3 lines · Keith Busch

One last thing I noticed, it looks like the nvme_fw_act_work will break if an IO timeout occurs while it's running since that may reset the controller and restart IO queues.

could you please clarify more?
I think ,that, if an IO Timeout resets controller and restarts queue, the nvme_ctrl_pp_status will return false, ending the nvme_fw_act_work.

Sure, the nvme_fw_act_work will observe CSTS.PP cleared, then it restarts the IO queues, but the nvme reset requires those queues to be quiesced.

Quoted 10 lines · Keith Busch

Quoted 7 lines · Arnav Dawn

Quoted 3 lines · Keith Busch

One last thing I noticed, it looks like the nvme_fw_act_work will break if an IO timeout occurs while it's running since that may reset the controller and restart IO queues.

could you please clarify more?
I think ,that, if an IO Timeout resets controller and restarts queue, the nvme_ctrl_pp_status will return false, ending the nvme_fw_act_work.

Sure, the nvme_fw_act_work will observe CSTS.PP cleared, then it restarts the IO queues, but the nvme reset requires those queues to be quiesced.

Thanks for clarifying,
i have made following changes based on comments, please review.

1. Added a delay of 100ms for scheduling fw_act_work, as work without delay could schedule almost immediately which may not give time to the controller to set CSTS.PP. Also, Moved fw_act_timeout calculation to the fw_act_work.

(quoted diff) +2 -0

+       case NVME_AER_NOTICE_FW_ACT_STARTING:
+               schedule_delayed_work(&ctrl->fw_act_work,

msecs_to_jiffies(100));

(quoted diff) +5 -0

+               break;
+       case NVME_AER_ERR_FW_IMG_LOAD:
+               dev_warn(ctrl->device, "FW image load error\n");
+               cancel_delayed_work(&ctrl->fw_act_work);
+               break;

2. Added a check, that the controller is in NVME_CTRL_LIVE state before starting queues once PP is cleared or fw activation times out. This prevents starting of queues unless the controller is in Live state.

(quoted diff) +19 -0

+static void nvme_fw_act_work(struct work_struct *work)
+{
+       struct nvme_ctrl *ctrl = container_of(to_delayed_work(work),
+                               struct nvme_ctrl, fw_act_work);
+       struct nvme_fw_slot_info_log *log;
+       unsigned long fw_act_timeout;
+
+       if (ctrl->mtfa)
+               fw_act_timeout = jiffies +
+                               msecs_to_jiffies(ctrl->mtfa * 100);
+       else
+               fw_act_timeout = jiffies +
+                               msecs_to_jiffies(admin_timeout * 1000);
+
+       nvme_stop_queues(ctrl);
+       while (nvme_ctrl_pp_status(ctrl)) {
+               if (time_after(jiffies, fw_act_timeout)) {
+                       dev_warn(ctrl->device,
+                               "Fw activation timeout, reset

controller\n");

(quoted diff) +20 -0

+                       ctrl->ops->reset_ctrl(ctrl);
+                       break;
+               }
+               msleep(100);
+       }
+
+       if(ctrl->state != NVME_CTRL_LIVE)
+               return;
+
+       nvme_start_queues(ctrl);
+       /* read FW slot informationi to clear the AER*/
+       log = kmalloc(sizeof(struct nvme_fw_slot_info_log), GFP_KERNEL);
+       if (!log)
+               return;
+
+       if (nvme_get_fw_slot_info(ctrl, log))
+               dev_warn(ctrl->device,
+                               "Get FW SLOT INFO log error\n");
+       kfree(log);
+}

if this is fine i will send out the next version of patch.

Quoted 8 lines · Keith Busch

Quoted 7 lines · Arnav Dawn

Quoted 6 lines · Keith Busch

drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 2322

Quoted 2 lines · Arnav Dawn

drivers/nvme/host/core.c +2 -0

+				ctrl->fw_act_timeout = jiffies +
+					msecs_to_jiffies(ctrl->mtfa * 100);

Instead of adding another field to the nvme_ctrl structure, just calculate the timeout in your nvme_fw_act_work function.

Quoted 6 lines

Quoted 3 lines

intention was to set fw_act_timeout as soon as the AER is received. Since work could be scheduled after some time, setting timeout in work function would add that delay to it.

This feature doesn't require such tight constraints. The 100ms sleep granularity in your CSTS.PP polling already exceeds the amount of time it takes for work to schedule.

i agree, i will update it in next version.

Quoted 11 lines

Quoted 8 lines

Quoted 6 lines

drivers/nvme/host/core.c void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status, · line 2327

Quoted 5 lines

drivers/nvme/host/core.c +5 -0

+			else
+				ctrl->fw_act_timeout = jiffies +
+					msecs_to_jiffies(admin_timeout * 1000);
+
+			schedule_delayed_work(&ctrl->fw_act_work, 0);

If scheduling with 0 delay, why is this delayed work?

I used delayed work so i could use cancel_delayed_work, as cancel_work was not available.

If you really have a use for cancel_work, you could send a patch to export the symbol.

Quoted 5 lines

In any case, that's probably not going to do what you want. The work can only be cancelled if it hasn't started, and since you start it without delay, the work will likely be running. Maybe you want to add some other criteria for the nvme_fw_act_work to end early, though I expect CSTS.PP to clear if f/w load failed.

you are right, if the device sends Firmware image load error AER, CSTS.PP is probably cleared, and the nvme_fw_act_work will end. so i think nothing needs to be done on FW Image load error AER.