[PATCH] nvme: prevent id ctrl csi for specs below 2.0

[PATCH] nvme: prevent id ctrl csi for specs below 2.0

[Ameer Hamza]

The 'id ctrl csi' command was introduced in version 2.0, as specified
in Section 5.17.2.6 of the NVME Base Specification 2.0. Executing this
command on previous NVMe versions returns an "Invalid Field" error,
and the error entry is saved in the log page. Although, Commit
c917dd96fe41 ("nvme: skip optional id ctrl csi if it failed") reduced
the error occurrences, but the error persisted during the initial
module load. This patch ensures the command isn't executed on versions
older than 2.0, and it also eliminates the skip implementation because
NVME_ID_CNS_CS_CTRL is expected to succeed with version 2.0.

Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
---
 drivers/nvme/host/core.c | 6 ++----
 drivers/nvme/host/nvme.h | 3 +--
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f3a01b79148c..6c92aa5704f9 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2894,9 +2894,9 @@ static int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl)
 	else
 		ctrl->max_zeroes_sectors = 0;
 
+	/* NVME_ID_CNS_CS_CTRL is supported from v2.0 onwards. */
 	if (ctrl->subsys->subtype != NVME_NQN_NVME ||
-	    nvme_ctrl_limited_cns(ctrl) ||
-	    test_bit(NVME_CTRL_SKIP_ID_CNS_CS, &ctrl->flags))
+	    ctrl->vs < NVME_VS(2, 0, 0))
 		return 0;
 
 	id = kzalloc(sizeof(*id), GFP_KERNEL);
@@ -2918,8 +2918,6 @@ static int nvme_init_non_mdts_limits(struct nvme_ctrl *ctrl)
 		ctrl->max_zeroes_sectors = nvme_mps_to_sectors(ctrl, id->wzsl);
 
 free_data:
-	if (ret > 0)
-		set_bit(NVME_CTRL_SKIP_ID_CNS_CS, &ctrl->flags);
 	kfree(id);
 	return ret;
 }
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index f35647c470af..f3cf10710bce 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -249,8 +249,7 @@ enum nvme_ctrl_flags {
 	NVME_CTRL_ADMIN_Q_STOPPED	= 1,
 	NVME_CTRL_STARTED_ONCE		= 2,
 	NVME_CTRL_STOPPED		= 3,
-	NVME_CTRL_SKIP_ID_CNS_CS	= 4,
-	NVME_CTRL_DIRTY_CAPABILITY	= 5,
+	NVME_CTRL_DIRTY_CAPABILITY	= 4,
 };
 
 struct nvme_ctrl {
-- 
2.39.2

Re: [PATCH] nvme: prevent id ctrl csi for specs below 2.0

[Keith Busch]

On Mon, Sep 11, 2023 at 02:26:41AM +0500, Ameer Hamza wrote:
> The 'id ctrl csi' command was introduced in version 2.0, as specified
> in Section 5.17.2.6 of the NVME Base Specification 2.0. Executing this
> command on previous NVMe versions returns an "Invalid Field" error,
> and the error entry is saved in the log page. Although, Commit
> c917dd96fe41 ("nvme: skip optional id ctrl csi if it failed") reduced
> the error occurrences, but the error persisted during the initial
> module load. This patch ensures the command isn't executed on versions
> older than 2.0, and it also eliminates the skip implementation because
> NVME_ID_CNS_CS_CTRL is expected to succeed with version 2.0.

NVMe TP's are allowed to be implemented by versions lower than the
release that first included it. I recall the first nvme controller I'd
seen that implemented this identification reported itself as 1.4.

Re: [PATCH] nvme: prevent id ctrl csi for specs below 2.0

[Alexander Motin]

On 11.09.2023 14:40, Keith Busch wrote:
> On Mon, Sep 11, 2023 at 02:26:41AM +0500, Ameer Hamza wrote:
>> The 'id ctrl csi' command was introduced in version 2.0, as specified
>> in Section 5.17.2.6 of the NVME Base Specification 2.0. Executing this
>> command on previous NVMe versions returns an "Invalid Field" error,
>> and the error entry is saved in the log page. Although, Commit
>> c917dd96fe41 ("nvme: skip optional id ctrl csi if it failed") reduced
>> the error occurrences, but the error persisted during the initial
>> module load. This patch ensures the command isn't executed on versions
>> older than 2.0, and it also eliminates the skip implementation because
>> NVME_ID_CNS_CS_CTRL is expected to succeed with version 2.0.
> 
> NVMe TP's are allowed to be implemented by versions lower than the
> release that first included it. I recall the first nvme controller I'd
> seen that implemented this identification reported itself as 1.4.

Then there must be a way to detect it.  How otherwise it is not a 
standard violation to send arbitrary effectively vendor-specific 
commands to a drive?

-- 
Alexander Motin

Re: [PATCH] nvme: prevent id ctrl csi for specs below 2.0

[Keith Busch]

On Mon, Sep 11, 2023 at 03:29:34PM -0400, Alexander Motin wrote:
> On 11.09.2023 14:40, Keith Busch wrote:
> > On Mon, Sep 11, 2023 at 02:26:41AM +0500, Ameer Hamza wrote:
> > > The 'id ctrl csi' command was introduced in version 2.0, as specified
> > > in Section 5.17.2.6 of the NVME Base Specification 2.0. Executing this
> > > command on previous NVMe versions returns an "Invalid Field" error,
> > > and the error entry is saved in the log page. Although, Commit
> > > c917dd96fe41 ("nvme: skip optional id ctrl csi if it failed") reduced
> > > the error occurrences, but the error persisted during the initial
> > > module load. This patch ensures the command isn't executed on versions
> > > older than 2.0, and it also eliminates the skip implementation because
> > > NVME_ID_CNS_CS_CTRL is expected to succeed with version 2.0.
> > 
> > NVMe TP's are allowed to be implemented by versions lower than the
> > release that first included it. I recall the first nvme controller I'd
> > seen that implemented this identification reported itself as 1.4.
> 
> Then there must be a way to detect it.  How otherwise it is not a standard
> violation to send arbitrary effectively vendor-specific commands to a drive?

For identification structures, you detect support for these by seeing if
the device responds with success or not.

Not sure what you mean by "standard violation". The standard defines
ways for the controller to respond when it receives an unrecognized
command.