[PATCH v4] nvme: reduce firmware activation poll interval

[PATCH v4] nvme: reduce firmware activation poll interval

[guzebing]

From: Guzebing <guzebing@bytedance.com>

nvme_fw_act_work() polls the controller processing-paused status every
100 ms while firmware activation is pending. Some devices can complete
online activation in only a few hundred milliseconds, so the fixed
100 ms interval can add up to 100 ms of latency before the driver
observes completion.

nvme_wait_ready() already uses a 1 to 2 ms delay between CSTS reads.
Use the same interval in nvme_fw_act_work() and factor the CSTS read,
status check, and sleep loop into a common helper for both paths. For
firmware activation, preserve the existing CC.EN check before testing
CSTS.PP.

Signed-off-by: Guzebing <guzebing@bytedance.com>
---
Changes in v4:
- Move the CSTS read, status check, and sleep loop into a common helper,
  preserving the firmware activation path's CC.EN check before CSTS.PP.

Changes in v3:
- Add a common polling delay helper for nvme_fw_act_work() and
  nvme_wait_ready().

Changes in v2:
- Drop the module parameter and use a fixed 1 to 2 ms poll interval.

v3: https://lore.kernel.org/linux-nvme/20260715114401.1084214-1-guzebing1612@gmail.com/
v2: https://lore.kernel.org/linux-nvme/20260714092846.3381169-1-guzebing1612@gmail.com/
v1: https://lore.kernel.org/linux-nvme/20260627010610.47768-1-guzebing1612@gmail.com/

Note:
The common helper combines the CSTS condition with a
firmware-activation-specific software state condition: the cached CC.EN
value in ctrl_config. This follows the suggestion to share the complete
polling loop, but makes the helper slightly less focused. Would it be
clearer and simpler to keep the loops separate, replace msleep(100) in
nvme_fw_act_work() with usleep_range(1000, 2000), and add a comment that
the interval matches nvme_wait_ready()?

 drivers/nvme/host/core.c | 78 +++++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 36 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 453c1f0b2dd09..d6a7e1cbd0e6f 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2688,33 +2688,52 @@ const struct block_device_operations nvme_bdev_ops = {
 	.pr_ops		= &nvme_pr_ops,
 };
 
-static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 mask, u32 val,
-		u32 timeout, const char *op)
+/*
+ * Poll CSTS until the requested status is reached.
+ * If exit_if_ctrl_disabled is set, check the cached CC.EN value first and
+ * stop polling when it is cleared.
+ */
+static int nvme_wait_csts(struct nvme_ctrl *ctrl, u32 mask, u32 val,
+			  unsigned long timeout, bool exit_if_ctrl_disabled,
+			  u32 *csts)
 {
-	unsigned long timeout_jiffies = jiffies + timeout * HZ;
-	u32 csts;
 	int ret;
 
-	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts)) == 0) {
-		if (csts == ~0)
+	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, csts)) == 0) {
+		if (*csts == ~0)
 			return -ENODEV;
-		if ((csts & mask) == val)
-			break;
+		if (exit_if_ctrl_disabled &&
+		    !(ctrl->ctrl_config & NVME_CC_ENABLE))
+			return 0;
+		if ((*csts & mask) == val)
+			return 0;
 
 		usleep_range(1000, 2000);
 		if (fatal_signal_pending(current))
 			return -EINTR;
-		if (time_after(jiffies, timeout_jiffies)) {
-			dev_err(ctrl->device,
-				"Device not ready; aborting %s, CSTS=0x%x\n",
-				op, csts);
-			return -ENODEV;
-		}
+		if (time_after(jiffies, timeout))
+			return -ETIMEDOUT;
 	}
 
 	return ret;
 }
 
+static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 mask, u32 val,
+			   u32 timeout, const char *op)
+{
+	unsigned long timeout_jiffies = jiffies + timeout * HZ;
+	u32 csts;
+	int ret;
+
+	ret = nvme_wait_csts(ctrl, mask, val, timeout_jiffies, false, &csts);
+	if (ret != -ETIMEDOUT)
+		return ret;
+
+	dev_err(ctrl->device,
+		"Device not ready; aborting %s, CSTS=0x%x\n", op, csts);
+	return -ENODEV;
+}
+
 int nvme_disable_ctrl(struct nvme_ctrl *ctrl, bool shutdown)
 {
 	int ret;
@@ -4748,20 +4767,6 @@ static void nvme_async_event_work(struct work_struct *work)
 		ctrl->ops->submit_async_event(ctrl);
 }
 
-static bool nvme_ctrl_pp_status(struct nvme_ctrl *ctrl)
-{
-
-	u32 csts;
-
-	if (ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts))
-		return false;
-
-	if (csts == ~0)
-		return false;
-
-	return ((ctrl->ctrl_config & NVME_CC_ENABLE) && (csts & NVME_CSTS_PP));
-}
-
 static void nvme_get_fw_slot_info(struct nvme_ctrl *ctrl)
 {
 	struct nvme_fw_slot_info_log *log;
@@ -4797,6 +4802,8 @@ static void nvme_fw_act_work(struct work_struct *work)
 	struct nvme_ctrl *ctrl = container_of(work,
 				struct nvme_ctrl, fw_act_work);
 	unsigned long fw_act_timeout;
+	u32 csts;
+	int ret;
 
 	nvme_auth_stop(ctrl);
 
@@ -4806,14 +4813,13 @@ static void nvme_fw_act_work(struct work_struct *work)
 		fw_act_timeout = jiffies + secs_to_jiffies(admin_timeout);
 
 	nvme_quiesce_io_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");
-			nvme_try_sched_reset(ctrl);
-			return;
-		}
-		msleep(100);
+	ret = nvme_wait_csts(ctrl, NVME_CSTS_PP, 0, fw_act_timeout, true,
+			     &csts);
+	if (ret == -ETIMEDOUT) {
+		dev_warn(ctrl->device,
+			 "Fw activation timeout, reset controller\n");
+		nvme_try_sched_reset(ctrl);
+		return;
 	}
 
 	if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_CONNECTING) ||
-- 
2.20.1

Re: [PATCH v4] nvme: reduce firmware activation poll interval

[Keith Busch]

On Tue, Jul 28, 2026 at 08:17:35PM +0800, guzebing wrote:
> +static int nvme_wait_csts(struct nvme_ctrl *ctrl, u32 mask, u32 val,
> +			  unsigned long timeout, bool exit_if_ctrl_disabled,
> +			  u32 *csts)
>  {
> -	unsigned long timeout_jiffies = jiffies + timeout * HZ;
> -	u32 csts;
>  	int ret;
>  
> -	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, &csts)) == 0) {
> -		if (csts == ~0)
> +	while ((ret = ctrl->ops->reg_read32(ctrl, NVME_REG_CSTS, csts)) == 0) {
> +		if (*csts == ~0)
>  			return -ENODEV;
> -		if ((csts & mask) == val)
> -			break;
> +		if (exit_if_ctrl_disabled &&
> +		    !(ctrl->ctrl_config & NVME_CC_ENABLE))
> +			return 0;
> +		if ((*csts & mask) == val)
> +			return 0;
>  
>  		usleep_range(1000, 2000);
>  		if (fatal_signal_pending(current))
>  			return -EINTR;
> -		if (time_after(jiffies, timeout_jiffies)) {
> -			dev_err(ctrl->device,
> -				"Device not ready; aborting %s, CSTS=0x%x\n",
> -				op, csts);
> -			return -ENODEV;
> -		}
> +		if (time_after(jiffies, timeout))
> +			return -ETIMEDOUT;

Mostly looks fine, though I was hoping to converge a little more
commonality among these functions. I folded some minor changes in when
applying, so please have a look at the current git tree to see if the
result is okay.

Re: [PATCH v4] nvme: reduce firmware activation poll interval

[Keith Busch]

On Tue, Jul 28, 2026 at 08:17:35PM +0800, guzebing wrote:
> +static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 mask, u32 val,
> +			   u32 timeout, const char *op)
> +{
> +	unsigned long timeout_jiffies = jiffies + timeout * HZ;
> +	u32 csts;
> +	int ret;
> +
> +	ret = nvme_wait_csts(ctrl, mask, val, timeout_jiffies, false, &csts);
> +	if (ret != -ETIMEDOUT)
> +		return ret;
> +
> +	dev_err(ctrl->device,
> +		"Device not ready; aborting %s, CSTS=0x%x\n", op, csts);
> +	return -ENODEV;

Actually, I'm to back this out for a moment to think about this. I like
that you've changed the error to ETIMEDOUT for that condition, but I'm
not sure we need to convert it to ENODEV. None of the callers seem to
care about the specific error, and the user visible side effect of the
condition is more informative than ENODEV.

So if we can get rid of that legacy behavior, then this simplifies even
more. Thoughts?

Re: [PATCH v4] nvme: reduce firmware activation poll interval

[guzebing]

On 8/12/26 12:46 AM, Keith Busch wrote:
> On Tue, Jul 28, 2026 at 08:17:35PM +0800, guzebing wrote:
>> +static int nvme_wait_ready(struct nvme_ctrl *ctrl, u32 mask, u32 val,
>> +			   u32 timeout, const char *op)
>> +{
>> +	unsigned long timeout_jiffies = jiffies + timeout * HZ;
>> +	u32 csts;
>> +	int ret;
>> +
>> +	ret = nvme_wait_csts(ctrl, mask, val, timeout_jiffies, false, &csts);
>> +	if (ret != -ETIMEDOUT)
>> +		return ret;
>> +
>> +	dev_err(ctrl->device,
>> +		"Device not ready; aborting %s, CSTS=0x%x\n", op, csts);
>> +	return -ENODEV;
> 
> Actually, I'm to back this out for a moment to think about this. I like
> that you've changed the error to ETIMEDOUT for that condition, but I'm
> not sure we need to convert it to ENODEV. None of the callers seem to
> care about the specific error, and the user visible side effect of the
> condition is more informative than ENODEV.
> 
> So if we can get rid of that legacy behavior, then this simplifies even
> more. Thoughts?

When restructuring the polling loop, I tried to preserve the existing
behavior and return values as much as possible. Converting -ETIMEDOUT
to -ENODEV was part of that effort.

However, I agree that -ETIMEDOUT more accurately describes the actual
failure. I checked the callers and confirmed that they do not
distinguish between -ENODEV and -ETIMEDOUT, so propagating -ETIMEDOUT
directly makes sense to me.

Please feel free to fold this change in, or let me know if you would
prefer a v5.

Thanks,
Guzebing