[PATCH v3 0/2] Support for Administrative Controllers

[PATCH v3 0/2] Support for Administrative Controllers

[Kamaljit Singh]

[1] Add capability to connect to an administrative controller by preventing
      ioq creation.
[2] Prevent an admin-ctrl from getting a smart log (LID=2)
[3] Prevent ioq creation for discovery-controllers as the spec prohibits
      them.


Kamaljit Singh (2):
  nvme: add capability to connect to an administrative controller
  nvme: prevent admin controller from smart log fetch (LID 2)

 drivers/nvme/host/core.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH v3 1/2] nvme: add capability to connect to an administrative controller

[Kamaljit Singh]

Suggested-by: Niklas Cassel <cassel@kernel.org>

Add capability to connect to an administrative controller by
preventing ioq creation for admin-controllers.

* Add helper nvme_admin_ctrl() to check for an administrative controller
* Add helper nvme_override_prohibited_io_queues() to override queue_count
* Call nvme_override_prohibited_io_queues() from nvme_init_ctrl_finish()
  so it applies to nvme/tcp and nvme/rdma

Signed-off-by: Kamaljit Singh <kamaljit.singh1@wdc.com>
---
 drivers/nvme/host/core.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e533d791955d..105127638c31 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3149,6 +3149,21 @@ static inline bool nvme_discovery_ctrl(struct nvme_ctrl *ctrl)
 	return ctrl->opts && ctrl->opts->discovery_nqn;
 }
 
+static inline bool nvme_admin_ctrl(struct nvme_ctrl *ctrl)
+{
+	return ctrl->cntrltype == NVME_CTRL_ADMIN;
+}
+
+/*
+ * An admin controller has one admin queue, but no I/O queues.
+ * Override queue_count so it only creates an admin queue.
+ */
+static inline void nvme_override_prohibited_io_queues(struct nvme_ctrl *ctrl)
+{
+	if (nvme_admin_ctrl(ctrl))
+		ctrl->queue_count = 1;
+}
+
 static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
 		struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 {
@@ -3670,6 +3685,12 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl, bool was_suspended)
 	if (ret)
 		return ret;
 
+	if (nvme_admin_ctrl(ctrl))
+		dev_dbg(ctrl->device,
+			"Subsystem %s is an administrative controller",
+			ctrl->subsys->subnqn);
+	nvme_override_prohibited_io_queues(ctrl);
+
 	ret = nvme_configure_apst(ctrl);
 	if (ret < 0)
 		return ret;
-- 
2.43.0

[PATCH v3 2/2] nvme: prevent admin controller from smart log fetch (LID 2)

[Kamaljit Singh]

Similar to a discovery ctrl, prevent an admin-ctrl from getting a smart
log. LID 2 is optional for admin controllers to support.

In the future when support for LID=0 (supported log pages) is added,
GLP accesses can be made smarter by basing such calls on response
from LID=0 reads.

Reference: NVMe Base rev 2.2, sec 3.1.3.5, fig 31.
Signed-off-by: Kamaljit Singh <kamaljit.singh1@wdc.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 105127638c31..6e5c9871b76d 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3705,7 +3705,7 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl, bool was_suspended)
 
 	nvme_configure_opal(ctrl, was_suspended);
 
-	if (!ctrl->identified && !nvme_discovery_ctrl(ctrl)) {
+	if (!ctrl->identified && !nvme_discovery_ctrl(ctrl) && !nvme_admin_ctrl(ctrl)) {
 		/*
 		 * Do not return errors unless we are in a controller reset,
 		 * the controller works perfectly fine without hwmon.
-- 
2.43.0

Re: [PATCH v3 1/2] nvme: add capability to connect to an administrative controller

[Damien Le Moal]

On 2025/07/18 9:14, Kamaljit Singh wrote:
> Suggested-by: Niklas Cassel <cassel@kernel.org>
> 
> Add capability to connect to an administrative controller by
> preventing ioq creation for admin-controllers.
> 
> * Add helper nvme_admin_ctrl() to check for an administrative controller
> * Add helper nvme_override_prohibited_io_queues() to override queue_count
> * Call nvme_override_prohibited_io_queues() from nvme_init_ctrl_finish()
>   so it applies to nvme/tcp and nvme/rdma
> 
> Signed-off-by: Kamaljit Singh <kamaljit.singh1@wdc.com>
> ---
>  drivers/nvme/host/core.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index e533d791955d..105127638c31 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3149,6 +3149,21 @@ static inline bool nvme_discovery_ctrl(struct nvme_ctrl *ctrl)
>  	return ctrl->opts && ctrl->opts->discovery_nqn;
>  }
>  
> +static inline bool nvme_admin_ctrl(struct nvme_ctrl *ctrl)
> +{
> +	return ctrl->cntrltype == NVME_CTRL_ADMIN;
> +}
> +
> +/*
> + * An admin controller has one admin queue, but no I/O queues.
> + * Override queue_count so it only creates an admin queue.
> + */
> +static inline void nvme_override_prohibited_io_queues(struct nvme_ctrl *ctrl)
> +{
> +	if (nvme_admin_ctrl(ctrl))
> +		ctrl->queue_count = 1;
> +}
> +
>  static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
>  		struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>  {
> @@ -3670,6 +3685,12 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl, bool was_suspended)
>  	if (ret)
>  		return ret;
>  
> +	if (nvme_admin_ctrl(ctrl))
> +		dev_dbg(ctrl->device,
> +			"Subsystem %s is an administrative controller",
> +			ctrl->subsys->subnqn);
> +	nvme_override_prohibited_io_queues(ctrl);

I do not think that this inline function is useful. Simply open-code it inside
the if above:

	if (nvme_admin_ctrl(ctrl)) {
		/*
		 * An admin controller has one admin queue, but no I/O queues.
		 * Override queue_count so it only creates an admin queue.
		 */
		dev_dbg(ctrl->device,
			"Subsystem %s is an administrative controller",
			ctrl->subsys->subnqn);
		ctrl->queue_count = 1;
	}

With this, feel free to add:

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 2/2] nvme: prevent admin controller from smart log fetch (LID 2)

[Damien Le Moal]

On 2025/07/18 9:14, Kamaljit Singh wrote:
> Similar to a discovery ctrl, prevent an admin-ctrl from getting a smart
> log. LID 2 is optional for admin controllers to support.
> 
> In the future when support for LID=0 (supported log pages) is added,
> GLP accesses can be made smarter by basing such calls on response
> from LID=0 reads.
> 
> Reference: NVMe Base rev 2.2, sec 3.1.3.5, fig 31.
> Signed-off-by: Kamaljit Singh <kamaljit.singh1@wdc.com>

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v3 1/2] nvme: add capability to connect to an administrative controller

[Niklas Cassel]

Hello Kamaljit,

I think that the patch looks good.

That said, the Suggested-by trailer should go together with all other
trailers at the end of the commit log, see e.g.:
https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/commit/?id=a4daf088a77323154514eb1f8626bbdf9329cfd4


Kind regards,
Niklas

Re: [PATCH v3 1/2] nvme: add capability to connect to an administrative controller

[Hannes Reinecke]

On 7/18/25 02:14, Kamaljit Singh wrote:
> Suggested-by: Niklas Cassel <cassel@kernel.org>
> 
> Add capability to connect to an administrative controller by
> preventing ioq creation for admin-controllers.
> 
> * Add helper nvme_admin_ctrl() to check for an administrative controller
> * Add helper nvme_override_prohibited_io_queues() to override queue_count
> * Call nvme_override_prohibited_io_queues() from nvme_init_ctrl_finish()
>    so it applies to nvme/tcp and nvme/rdma
> 
> Signed-off-by: Kamaljit Singh <kamaljit.singh1@wdc.com>
> ---
>   drivers/nvme/host/core.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index e533d791955d..105127638c31 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3149,6 +3149,21 @@ static inline bool nvme_discovery_ctrl(struct nvme_ctrl *ctrl)
>   	return ctrl->opts && ctrl->opts->discovery_nqn;
>   }
>   
> +static inline bool nvme_admin_ctrl(struct nvme_ctrl *ctrl)
> +{
> +	return ctrl->cntrltype == NVME_CTRL_ADMIN;
> +}
> +
> +/*
> + * An admin controller has one admin queue, but no I/O queues.
> + * Override queue_count so it only creates an admin queue.
> + */
> +static inline void nvme_override_prohibited_io_queues(struct nvme_ctrl *ctrl)
> +{
> +	if (nvme_admin_ctrl(ctrl))
> +		ctrl->queue_count = 1;
> +}
> +
>   static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
>   		struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   {
> @@ -3670,6 +3685,12 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl, bool was_suspended)
>   	if (ret)
>   		return ret;
>   
> +	if (nvme_admin_ctrl(ctrl))
> +		dev_dbg(ctrl->device,
> +			"Subsystem %s is an administrative controller",
> +			ctrl->subsys->subnqn);
> +	nvme_override_prohibited_io_queues(ctrl);
> +
>   	ret = nvme_configure_apst(ctrl);
>   	if (ret < 0)
>   		return ret;

One wonders why this is done for admin controllers only; surely 
discovery controllers also don't support I/O queues, and should
therefore have the same setting?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v3 2/2] nvme: prevent admin controller from smart log fetch (LID 2)

[Hannes Reinecke]

On 7/18/25 02:14, Kamaljit Singh wrote:
> Similar to a discovery ctrl, prevent an admin-ctrl from getting a smart
> log. LID 2 is optional for admin controllers to support.
> 
> In the future when support for LID=0 (supported log pages) is added,
> GLP accesses can be made smarter by basing such calls on response
> from LID=0 reads.
> 
> Reference: NVMe Base rev 2.2, sec 3.1.3.5, fig 31.
> Signed-off-by: Kamaljit Singh <kamaljit.singh1@wdc.com>
> ---
>   drivers/nvme/host/core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 105127638c31..6e5c9871b76d 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3705,7 +3705,7 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl, bool was_suspended)
>   
>   	nvme_configure_opal(ctrl, was_suspended);
>   
> -	if (!ctrl->identified && !nvme_discovery_ctrl(ctrl)) {
> +	if (!ctrl->identified && !nvme_discovery_ctrl(ctrl) && !nvme_admin_ctrl(ctrl)) {
>   		/*
>   		 * Do not return errors unless we are in a controller reset,
>   		 * the controller works perfectly fine without hwmon.

And to go with my suggestion for the previous patch, maybe it's
an idea to have a helper 'nvme_io_queues_supported()' which could be
used here (and in the previous patch)?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH v3 1/2] nvme: add capability to connect to an administrative controller

[Kamaljit Singh]

Hi Hannes,
 
From: Hannes Reinecke <hare@suse.de>
Date: Tuesday, July 22, 2025 at 00:06
>> Suggested-by: Niklas Cassel <cassel@kernel.org>
>>
>> Add capability to connect to an administrative controller by
>> preventing ioq creation for admin-controllers.
>>
>> * Add helper nvme_admin_ctrl() to check for an administrative controller
>> * Add helper nvme_override_prohibited_io_queues() to override queue_count
>> * Call nvme_override_prohibited_io_queues() from nvme_init_ctrl_finish()
>>    so it applies to nvme/tcp and nvme/rdma
>>
>> Signed-off-by: Kamaljit Singh <kamaljit.singh1@wdc.com>
>> ---
>>   drivers/nvme/host/core.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index e533d791955d..105127638c31 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -3149,6 +3149,21 @@ static inline bool nvme_discovery_ctrl(struct nvme_ctrl *ctrl)
>>       return ctrl->opts && ctrl->opts->discovery_nqn;
>>   }
>>
>> +static inline bool nvme_admin_ctrl(struct nvme_ctrl *ctrl)
>> +{
>> +     return ctrl->cntrltype == NVME_CTRL_ADMIN;
>> +}
>> +
>> +/*
>> + * An admin controller has one admin queue, but no I/O queues.
>> + * Override queue_count so it only creates an admin queue.
>> + */
>> +static inline void nvme_override_prohibited_io_queues(struct nvme_ctrl *ctrl)
>> +{
>> +     if (nvme_admin_ctrl(ctrl))
>> +             ctrl->queue_count = 1;
>> +}
>> +
>>   static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
>>               struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>>   {
>> @@ -3670,6 +3685,12 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl, bool was_suspended)
>>       if (ret)
>>               return ret;
>>
>> +     if (nvme_admin_ctrl(ctrl))
>> +             dev_dbg(ctrl->device,
>> +                     "Subsystem %s is an administrative controller",
>> +                     ctrl->subsys->subnqn);
>> +     nvme_override_prohibited_io_queues(ctrl);
>> +
>>       ret = nvme_configure_apst(ctrl);
>>       if (ret < 0)
>>               return ret;
>
>One wonders why this is done for admin controllers only; surely
>discovery controllers also don't support I/O queues, and should
>therefore have the same setting?
I agree. That was my patch-3 in v2, which I retracted in v3 based on
your comment below. Is someone taking that action item to discuss at FMDS?
I haven't attended it in a while.

If everyone agrees, I can add patch-3 back in and create v5 or since v4
patch-1 was accepted, I can issue a standalone patch just for this change.
Please let me know.



>From: Hannes Reinecke <hare@suse.de>
>Date: Thursday, July 3, 2025 at 02:42
>> Prevent ioq creation for discovery-controllers as the spec prohibits
>> them, similarly to the administrative controllers.
>>
>> Reference: NVMe Base rev 2.2, sec 3.1.3.4, fig 28.
>>
>While this might be true, we already deal with discovery controllers
>just fine, and never had issues with I/O queues being created.
>Presumably because discovery controllers never exposed I/O queues
>in the first place.
>
>Which also means that admin controllers should work already if they
>would not expose I/O queues.
BTW, the reason admin controllers didn't work without my patch is that
nvme-cli "nvme connect" doesn't allow "--nr-io-queues=0" and the default
=1 forces the kernel to create an I/O queue, which my patch overrides.

>
>We _might_ reduce the number of queues to '1' in nvme_set_queue_count(),
>but then we should issue a warning here as this would be a configuration
>error on the target side.
>
>'Prohibited' cuts both ways; the host shouldn't ask for it and the
>controller shouldn't advertise it...
>
>Or that's at least my reading. Might be worthwhile clarifying this
>at FMDS.

Regards,
Kamaljit

Re: [PATCH v3 2/2] nvme: prevent admin controller from smart log fetch (LID 2)

[Kamaljit Singh]

Hi Hannes,

From: Hannes Reinecke <hare@suse.de>
Date: Tuesday, July 22, 2025 at 00:07
>> Similar to a discovery ctrl, prevent an admin-ctrl from getting a smart
>> log. LID 2 is optional for admin controllers to support.
>>
>> In the future when support for LID=0 (supported log pages) is added,
>> GLP accesses can be made smarter by basing such calls on response
>> from LID=0 reads.
>>
>> Reference: NVMe Base rev 2.2, sec 3.1.3.5, fig 31.
>> Signed-off-by: Kamaljit Singh <kamaljit.singh1@wdc.com>
>> ---
>>   drivers/nvme/host/core.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 105127638c31..6e5c9871b76d 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -3705,7 +3705,7 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl, bool was_suspended)
>>
>>       nvme_configure_opal(ctrl, was_suspended);
>>
>> -     if (!ctrl->identified && !nvme_discovery_ctrl(ctrl)) {
>> +     if (!ctrl->identified && !nvme_discovery_ctrl(ctrl) && !nvme_admin_ctrl(ctrl)) {
>>               /*
>>                * Do not return errors unless we are in a controller reset,
>>                * the controller works perfectly fine without hwmon.
>
>And to go with my suggestion for the previous patch, maybe it's
>an idea to have a helper 'nvme_io_queues_supported()' which could be
>used here (and in the previous patch)?

That's kinda where I was going with nvme_override_prohibited_io_queues().
But now that its flattened into direct code, that function doesn't exist
in v4 of this patch-set anymore.

In general, I do like the idea of a helper like nvme_io_queues_supported()
which can be applied to any controller type. But I'm not sure how it could
be used in this case, i.e. for a GLP LID=2 holdback.

We will also need to apply it at the right point so a user requested
parameter is also taken into account.

Regards,
Kamaljit

Re: [PATCH v3 1/2] nvme: add capability to connect to an administrative controller

[hch]

On Tue, Jul 22, 2025 at 05:34:56PM +0000, Kamaljit Singh wrote:
> >One wonders why this is done for admin controllers only; surely
> >discovery controllers also don't support I/O queues, and should
> >therefore have the same setting?
> I agree. That was my patch-3 in v2, which I retracted in v3 based on
> your comment below. Is someone taking that action item to discuss at FMDS?
> I haven't attended it in a while.
> 
> If everyone agrees, I can add patch-3 back in and create v5 or since v4
> patch-1 was accepted, I can issue a standalone patch just for this change.
> Please let me know.

While forcing it won't hurt, we had things working for almost 10 years
by relying on userspace to request zero queues for discovery controllers.
So I don't think it really matters.