[PATCH v2] nvme: Print capabilities changes just once

[PATCH v2] nvme: Print capabilities changes just once

[Breno Leitao]

This current dev_info() could be very verbose and being printed very
frequently depending on some userspace application sending some specific
commands.

Just print this message once and skip it until the controller resets.
Use a controller flag (NVME_CTRL_DIRTY_CAPABILITY) to track if the
capability needs a reset.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/nvme/host/core.c | 6 +++++-
 drivers/nvme/host/nvme.h | 1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 76e8f8b4098e..4b7f9edab5e8 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1125,8 +1125,11 @@ void nvme_passthru_end(struct nvme_ctrl *ctrl, u32 effects,
 		mutex_unlock(&ctrl->scan_lock);
 	}
 	if (effects & NVME_CMD_EFFECTS_CCC) {
-		dev_info(ctrl->device,
+		if (!test_and_set_bit(NVME_CTRL_DIRTY_CAPABILITY,
+				      &ctrl->flags)) {
+			dev_info(ctrl->device,
 "controller capabilities changed, reset may be required to take effect.\n");
+		}
 	}
 	if (effects & (NVME_CMD_EFFECTS_NIC | NVME_CMD_EFFECTS_NCC)) {
 		nvme_queue_scan(ctrl);
@@ -3280,6 +3283,7 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl, bool was_suspended)
 			return ret;
 	}
 
+	clear_bit(NVME_CTRL_DIRTY_CAPABILITY, &ctrl->flags);
 	ctrl->identified = true;
 
 	return 0;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 78308f15e090..f3182134487a 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -243,6 +243,7 @@ enum nvme_ctrl_flags {
 	NVME_CTRL_STARTED_ONCE		= 2,
 	NVME_CTRL_STOPPED		= 3,
 	NVME_CTRL_SKIP_ID_CNS_CS	= 4,
+	NVME_CTRL_DIRTY_CAPABILITY	= 5,
 };
 
 struct nvme_ctrl {
-- 
2.34.1

Re: [PATCH v2] nvme: Print capabilities changes just once

[Keith Busch]

On Thu, Jun 15, 2023 at 02:49:03AM -0700, Breno Leitao wrote:
> This current dev_info() could be very verbose and being printed very
> frequently depending on some userspace application sending some specific
> commands.
> 
> Just print this message once and skip it until the controller resets.
> Use a controller flag (NVME_CTRL_DIRTY_CAPABILITY) to track if the
> capability needs a reset.

Looks good. 

Reviewed-by: Keith Busch <kbusch@kernel.org>

Re: [PATCH v2] nvme: Print capabilities changes just once

[Chaitanya Kulkarni]

On 6/15/2023 2:49 AM, Breno Leitao wrote:
> This current dev_info() could be very verbose and being printed very
> frequently depending on some userspace application sending some specific
> commands.
> 
> Just print this message once and skip it until the controller resets.
> Use a controller flag (NVME_CTRL_DIRTY_CAPABILITY) to track if the
> capability needs a reset.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH v2] nvme: Print capabilities changes just once

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

Re: [PATCH v2] nvme: Print capabilities changes just once

[Keith Busch]

On Thu, Jun 15, 2023 at 02:49:03AM -0700, Breno Leitao wrote:
>  	if (effects & NVME_CMD_EFFECTS_CCC) {
> -		dev_info(ctrl->device,
> +		if (!test_and_set_bit(NVME_CTRL_DIRTY_CAPABILITY,
> +				      &ctrl->flags)) {
> +			dev_info(ctrl->device,
>  "controller capabilities changed, reset may be required to take effect.\n");
> +		}
>  	}

Thanks, patch applied to nvme-6.5.

A question for Christoph and future consideration: I *think* the
complications that had the driver stop refreshing the controller
attributes are largely mitigated by the fact we now handle effects
after the request is freed. Perhaps I'm missing something. Is there
a risk in just bringing that feature back into the driver?

Re: [PATCH v2] nvme: Print capabilities changes just once

[Christoph Hellwig]

On Wed, Jun 21, 2023 at 10:46:17AM -0600, Keith Busch wrote:
> On Thu, Jun 15, 2023 at 02:49:03AM -0700, Breno Leitao wrote:
> >  	if (effects & NVME_CMD_EFFECTS_CCC) {
> > -		dev_info(ctrl->device,
> > +		if (!test_and_set_bit(NVME_CTRL_DIRTY_CAPABILITY,
> > +				      &ctrl->flags)) {
> > +			dev_info(ctrl->device,
> >  "controller capabilities changed, reset may be required to take effect.\n");
> > +		}
> >  	}
> 
> Thanks, patch applied to nvme-6.5.
> 
> A question for Christoph and future consideration: I *think* the
> complications that had the driver stop refreshing the controller
> attributes are largely mitigated by the fact we now handle effects
> after the request is freed. Perhaps I'm missing something. Is there
> a risk in just bringing that feature back into the driver?

No, the main issue was and is that we can't just call
nvme_init_ctrl_finish on a live controller.