[PATCH v2 0/3] Support for Administrative Controllers

[PATCH v2 0/3] Support for Administrative Controllers

[Kamaljit Singh]

[1] Add capability to connect to an admin controller
    - prevents ioq creation for admin-controllers.

[2] Prevent admin controller from smart log fetch (LID 2)
    - LID 2 is optional for admin controllers.

[3] Prevent ioq creation for discovery controllers
    - prohibited by spec



Kamaljit Singh (3):
  nvme: add capability to connect to an admin controller
  nvme: prevent admin controller from smart log fetch (LID 2)
  nvme: prevent ioq creation for discovery controllers

 drivers/nvme/host/core.c | 23 ++++++++++++++++++++++-
 drivers/nvme/host/nvme.h |  1 +
 drivers/nvme/host/rdma.c |  2 ++
 drivers/nvme/host/tcp.c  |  2 ++
 4 files changed, 27 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH v2 1/3] nvme: add capability to connect to an admin 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 in nvme_ctrl

Reference: NVMe Base rev 2.2, sec 3.1.3.4, fig 28.

Signed-off-by: Kamaljit Singh <kamaljit.singh1@wdc.com>
---
 drivers/nvme/host/core.c | 21 +++++++++++++++++++++
 drivers/nvme/host/nvme.h |  1 +
 drivers/nvme/host/rdma.c |  2 ++
 drivers/nvme/host/tcp.c  |  2 ++
 4 files changed, 26 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e533d791955d..a1155fb8d5be 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3149,6 +3149,22 @@ 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.
+ */
+void nvme_override_prohibited_io_queues(struct nvme_ctrl *ctrl)
+{
+	if (nvme_admin_ctrl(ctrl))
+		ctrl->queue_count = 1;
+}
+EXPORT_SYMBOL_GPL(nvme_override_prohibited_io_queues);
+
 static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
 		struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 {
@@ -3215,6 +3231,11 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 		kfree(subsys);
 		return -EINVAL;
 	}
+	if (nvme_admin_ctrl(ctrl))
+		dev_info(ctrl->device,
+			"Subsystem %s is an administrative controller",
+			subsys->subnqn);
+
 	nvme_mpath_default_iopolicy(subsys);
 
 	subsys->dev.class = &nvme_subsys_class;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 7df2ea21851f..5e07ba634e97 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -1212,6 +1212,7 @@ struct nvme_ctrl *nvme_ctrl_from_file(struct file *file);
 struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid);
 bool nvme_get_ns(struct nvme_ns *ns);
 void nvme_put_ns(struct nvme_ns *ns);
+void nvme_override_prohibited_io_queues(struct nvme_ctrl *ctrl);
 
 static inline bool nvme_multi_css(struct nvme_ctrl *ctrl)
 {
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 9bd3646568d0..7d50a2f31c3a 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1026,6 +1026,8 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
 		goto destroy_admin;
 	}
 
+	nvme_override_prohibited_io_queues(&ctrl->ctrl);
+
 	if (ctrl->ctrl.opts->queue_size > ctrl->ctrl.sqsize + 1) {
 		dev_warn(ctrl->ctrl.device,
 			"queue_size %zu > ctrl sqsize %u, clamping down\n",
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index d924008c3949..bfb52a487c45 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2381,6 +2381,8 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
 		goto destroy_admin;
 	}
 
+	nvme_override_prohibited_io_queues(ctrl);
+
 	if (opts->queue_size > ctrl->sqsize + 1)
 		dev_warn(ctrl->device,
 			"queue_size %zu > ctrl sqsize %u, clamping down\n",
-- 
2.43.0

Re: [PATCH v2 1/3] nvme: add capability to connect to an admin controller

[Damien Le Moal]

On 7/2/25 09:58, 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 in nvme_ctrl
> 
> Reference: NVMe Base rev 2.2, sec 3.1.3.4, fig 28.
> 
> Signed-off-by: Kamaljit Singh <kamaljit.singh1@wdc.com>
> ---
>  drivers/nvme/host/core.c | 21 +++++++++++++++++++++
>  drivers/nvme/host/nvme.h |  1 +
>  drivers/nvme/host/rdma.c |  2 ++
>  drivers/nvme/host/tcp.c  |  2 ++
>  4 files changed, 26 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index e533d791955d..a1155fb8d5be 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3149,6 +3149,22 @@ 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);

No need for the parenthesis.

> +}
> +
> +/*
> + * An admin controller has one admin queue, but no I/O queues.
> + * Override queue_count so it only creates an admin queue.
> + */
> +void nvme_override_prohibited_io_queues(struct nvme_ctrl *ctrl)
> +{
> +	if (nvme_admin_ctrl(ctrl))
> +		ctrl->queue_count = 1;
> +}
> +EXPORT_SYMBOL_GPL(nvme_override_prohibited_io_queues);

Why not have this done in nvme_init_subsystem() ? That would avoid needing to
call this in all fabrics drivers.

> +
>  static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
>  		struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>  {
> @@ -3215,6 +3231,11 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>  		kfree(subsys);
>  		return -EINVAL;
>  	}
> +	if (nvme_admin_ctrl(ctrl))
> +		dev_info(ctrl->device,
> +			"Subsystem %s is an administrative controller",
> +			subsys->subnqn);

We do not print an equivalent message for other subsystem controller types. So
drop this.

> +
>  	nvme_mpath_default_iopolicy(subsys);
>  
>  	subsys->dev.class = &nvme_subsys_class;


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 1/3] nvme: add capability to connect to an admin controller

[Kamaljit Singh]

Hi Damien,

On 7/1/25 19:11, Damien Le Moal wrote:
>> +static inline bool nvme_admin_ctrl(struct nvme_ctrl *ctrl)
>> +{
>> +     return (ctrl->cntrltype == NVME_CTRL_ADMIN);
>
>No need for the parenthesis.
Updated for v3.


>> +/*
>> + * An admin controller has one admin queue, but no I/O queues.
>> + * Override queue_count so it only creates an admin queue.
>> + */
>> +void nvme_override_prohibited_io_queues(struct nvme_ctrl *ctrl)
>> +{
>> +     if (nvme_admin_ctrl(ctrl))
>> +             ctrl->queue_count = 1;
>> +}
>> +EXPORT_SYMBOL_GPL(nvme_override_prohibited_io_queues);
>
>Why not have this done in nvme_init_subsystem() ? That would avoid needing to
>call this in all fabrics drivers.
Like I mentioned in response for 3/3, this can affect other drivers like apple/fc
and I don't have any way of testing them. If you're okay with that, I can move
nvme_override_prohibited_io_queues() into nvme_init_subsystem(). I did some
analysis and nvme_init_subsystem() seems to be covering reconnects as well.


>>  static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
>>               struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>>  {
>> @@ -3215,6 +3231,11 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>>               kfree(subsys);
>>               return -EINVAL;
>>       }
>> +     if (nvme_admin_ctrl(ctrl))
>> +             dev_info(ctrl->device,
>> +                     "Subsystem %s is an administrative controller",
>> +                     subsys->subnqn);
>
>We do not print an equivalent message for other subsystem controller types. So
>drop this.
I left that msg in there for debugging purposes. I can either change it to
dev_dbg() or if that's still not likeable/cluttering, I can remove it.
Please let me know.

Thanks,
Kamaljit

Re: [PATCH v2 1/3] nvme: add capability to connect to an admin controller

[hch]

On Wed, Jul 02, 2025 at 06:39:09PM +0000, Kamaljit Singh wrote:
> >call this in all fabrics drivers.
> Like I mentioned in response for 3/3, this can affect other drivers like apple/fc
> and I don't have any way of testing them. If you're okay with that, I can move
> nvme_override_prohibited_io_queues() into nvme_init_subsystem(). I did some
> analysis and nvme_init_subsystem() seems to be covering reconnects as well.

This should go into generic code.  Apple does not support admin controllers.
FC should, and this does the right thing for them.

> >> @@ -3215,6 +3231,11 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
> >>               kfree(subsys);
> >>               return -EINVAL;
> >>       }
> >> +     if (nvme_admin_ctrl(ctrl))
> >> +             dev_info(ctrl->device,
> >> +                     "Subsystem %s is an administrative controller",
> >> +                     subsys->subnqn);
> >
> >We do not print an equivalent message for other subsystem controller types. So
> >drop this.
> I left that msg in there for debugging purposes. I can either change it to
> dev_dbg() or if that's still not likeable/cluttering, I can remove it.
> Please let me know.

I'd leave this as dev_dbg only.

Re: [PATCH v2 1/3] nvme: add capability to connect to an admin controller

[Hannes Reinecke]

On 7/2/25 02:58, 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 in nvme_ctrl
> 
> Reference: NVMe Base rev 2.2, sec 3.1.3.4, fig 28.
> 
> Signed-off-by: Kamaljit Singh <kamaljit.singh1@wdc.com>
> ---
>   drivers/nvme/host/core.c | 21 +++++++++++++++++++++
>   drivers/nvme/host/nvme.h |  1 +
>   drivers/nvme/host/rdma.c |  2 ++
>   drivers/nvme/host/tcp.c  |  2 ++
>   4 files changed, 26 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index e533d791955d..a1155fb8d5be 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3149,6 +3149,22 @@ 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.
> + */
> +void nvme_override_prohibited_io_queues(struct nvme_ctrl *ctrl)
> +{
> +	if (nvme_admin_ctrl(ctrl))
> +		ctrl->queue_count = 1;
> +}
> +EXPORT_SYMBOL_GPL(nvme_override_prohibited_io_queues);
> +
>   static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
>   		struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   {
> @@ -3215,6 +3231,11 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>   		kfree(subsys);
>   		return -EINVAL;
>   	}
> +	if (nvme_admin_ctrl(ctrl))
> +		dev_info(ctrl->device,
> +			"Subsystem %s is an administrative controller",
> +			subsys->subnqn);
> +

Bzzt. A subsystem is a subsystem, a controller is a controller.
Better issue a message when connecting the controller.

>   	nvme_mpath_default_iopolicy(subsys);
>   
>   	subsys->dev.class = &nvme_subsys_class;
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 7df2ea21851f..5e07ba634e97 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -1212,6 +1212,7 @@ struct nvme_ctrl *nvme_ctrl_from_file(struct file *file);
>   struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid);
>   bool nvme_get_ns(struct nvme_ns *ns);
>   void nvme_put_ns(struct nvme_ns *ns);
> +void nvme_override_prohibited_io_queues(struct nvme_ctrl *ctrl);
>   
>   static inline bool nvme_multi_css(struct nvme_ctrl *ctrl)
>   {
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 9bd3646568d0..7d50a2f31c3a 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1026,6 +1026,8 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
>   		goto destroy_admin;
>   	}
>   
> +	nvme_override_prohibited_io_queues(&ctrl->ctrl);
> +
>   	if (ctrl->ctrl.opts->queue_size > ctrl->ctrl.sqsize + 1) {
>   		dev_warn(ctrl->ctrl.device,
>   			"queue_size %zu > ctrl sqsize %u, clamping down\n",
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index d924008c3949..bfb52a487c45 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2381,6 +2381,8 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
>   		goto destroy_admin;
>   	}
>   
> +	nvme_override_prohibited_io_queues(ctrl);
> +
>   	if (opts->queue_size > ctrl->sqsize + 1)
>   		dev_warn(ctrl->device,
>   			"queue_size %zu > ctrl sqsize %u, clamping down\n",

And that is a bit convoluted.

Why not add a check in 'nvme_set_queue_count' and reduce the number of
queues to '1' there?

(Then you also have a place to put your message about the admin
controller ...)

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 v2 1/3] nvme: add capability to connect to an admin controller

[Kamaljit Singh]

Hi Hannes,
Sorry for delayed reply due to PTO. Responses are inline below. Working on v3.

On 7/3/25 01:55, Hannes Reinecke wrote:
>> @@ -3215,6 +3231,11 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>>               kfree(subsys);
>>               return -EINVAL;
>>       }
>> +     if (nvme_admin_ctrl(ctrl))
>> +             dev_info(ctrl->device,
>> +                     "Subsystem %s is an administrative controller",
>> +                     subsys->subnqn);
>> +
>
>Bzzt. A subsystem is a subsystem, a controller is a controller.
>Better issue a message when connecting the controller.
Yeah, changing it to dev_dbg() as Christoph suggested.


>> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
>> index d924008c3949..bfb52a487c45 100644
>> --- a/drivers/nvme/host/tcp.c
>> +++ b/drivers/nvme/host/tcp.c
>> @@ -2381,6 +2381,8 @@ static int nvme_tcp_setup_ctrl(struct nvme_ctrl *ctrl, bool new)
>>               goto destroy_admin;
>>       }
>>
>> +     nvme_override_prohibited_io_queues(ctrl);
>> +
>>       if (opts->queue_size > ctrl->sqsize + 1)
>>               dev_warn(ctrl->device,
>>                       "queue_size %zu > ctrl sqsize %u, clamping down\n",

>And that is a bit convoluted.
>
>Why not add a check in 'nvme_set_queue_count' and reduce the number of
>queues to '1' there?
>
>(Then you also have a place to put your message about the admin
>controller ...)
nvme_set_queue_count() won't be called in this case as its only called to
configure IO queues. Besides, this function is only called to do set-features
(FID=7) on the target.

I've moved nvme_override_prohibited_io_queues() to nvme_init_subsystem() as
Damien suggested. I've checked it and that should work fine in the generic code.

Thanks,
Kamaljit

[PATCH v2 2/3] 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 the newly added 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 a1155fb8d5be..c310634e75f3 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 v2 2/3] nvme: prevent admin controller from smart log fetch (LID 2)

[Damien Le Moal]

On 7/2/25 09:58, 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.

If it is optional, when the admin controller support it, why prevent it ?
This is what your code does... Or is it that at this stage of the
initialization, you do not know yet if the admin controller supports LTD 2 ?

> 
> In the future when support for the newly added 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 a1155fb8d5be..c310634e75f3 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.


-- 
Damien Le Moal
Western Digital Research

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

[Kamaljit Singh]

Hi Damien,

On 7/1/25 19:14, Damien Le Moal wrote:
>> Similar to a discovery ctrl, prevent an admin-ctrl from getting a smart
>> log. LID 2 is optional for admin controllers to support.

>If it is optional, when the admin controller support it, why prevent it ?
>This is what your code does... Or is it that at this stage of the
>initialization, you do not know yet if the admin controller supports LTD 2 ?
Yes, the latter. If the admin controller does not support LID=2 then GLP
will fail. My suggestion is to revisit this later and add smarts based on
LID=0 (supported log pages) response from the controller.

>
> In the future when support for the newly added 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 a1155fb8d5be..c310634e75f3 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.


Thanks,
Kamaljit 

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

[Hannes Reinecke]

On 7/2/25 02:58, 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 the newly added 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 a1155fb8d5be..c310634e75f3 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.

Nope. You said yourself, that log page is optional.
But that also means that there _might_ be controller who support it.
If you want to avoid an error here we would need to check if that log
page is supported, not disable it upfront.

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 v2 2/3] nvme: prevent admin controller from smart log fetch (LID 2)

[Christoph Hellwig]

On Tue, Jul 01, 2025 at 05:58:28PM -0700, 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 the newly added LID=0 (supported log
> pages) is added, GLP accesses can be made smarter by basing such calls
> on response from LID=0 reads.

Let's leave this in.  Failing a get_log page is fine.  The difference
for discovery controllers is that implementing it is prohibited.

[PATCH v2 3/3] nvme: prevent ioq creation for discovery controllers

[Kamaljit Singh]

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.

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

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c310634e75f3..3ad3b1da8b34 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3155,12 +3155,12 @@ static inline bool nvme_admin_ctrl(struct nvme_ctrl *ctrl)
 }
 
 /*
- * An admin controller has one admin queue, but no I/O queues.
+ * An admin or discovery controller has one admin queue, but no I/O queues.
  * Override queue_count so it only creates an admin queue.
  */
 void nvme_override_prohibited_io_queues(struct nvme_ctrl *ctrl)
 {
-	if (nvme_admin_ctrl(ctrl))
+	if (nvme_admin_ctrl(ctrl) || nvme_discovery_ctrl(ctrl))
 		ctrl->queue_count = 1;
 }
 EXPORT_SYMBOL_GPL(nvme_override_prohibited_io_queues);
-- 
2.43.0

Re: [PATCH v2 3/3] nvme: prevent ioq creation for discovery controllers

[Damien Le Moal]

On 7/2/25 09:58, Kamaljit Singh wrote:
> 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.
> 
> Signed-off-by: Kamaljit Singh <kamaljit.singh1@wdc.com>
> ---
>  drivers/nvme/host/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index c310634e75f3..3ad3b1da8b34 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3155,12 +3155,12 @@ static inline bool nvme_admin_ctrl(struct nvme_ctrl *ctrl)
>  }
>  
>  /*
> - * An admin controller has one admin queue, but no I/O queues.
> + * An admin or discovery controller has one admin queue, but no I/O queues.
>   * Override queue_count so it only creates an admin queue.
>   */
>  void nvme_override_prohibited_io_queues(struct nvme_ctrl *ctrl)
>  {
> -	if (nvme_admin_ctrl(ctrl))
> +	if (nvme_admin_ctrl(ctrl) || nvme_discovery_ctrl(ctrl))
>  		ctrl->queue_count = 1;
>  }
>  EXPORT_SYMBOL_GPL(nvme_override_prohibited_io_queues);

Repeating comment on patch 1. Can't we do this in nvme_init_subsystem() or may
be better, in nvme_set_queue_count() or nvme_init_ctrl_finish() ?


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 3/3] nvme: prevent ioq creation for discovery controllers

[Kamaljit Singh]

Hi Damien,
 
On 7/1/25 19:18, Damien Le Moal wrote:
>>  /*
>> - * An admin controller has one admin queue, but no I/O queues.
>> + * An admin or discovery controller has one admin queue, but no I/O queues.
>>   * Override queue_count so it only creates an admin queue.
>>   */
>>  void nvme_override_prohibited_io_queues(struct nvme_ctrl *ctrl)
>>  {
>> -     if (nvme_admin_ctrl(ctrl))
>> +     if (nvme_admin_ctrl(ctrl) || nvme_discovery_ctrl(ctrl))
>>               ctrl->queue_count = 1;
>>  }
>>  EXPORT_SYMBOL_GPL(nvme_override_prohibited_io_queues);
>
>Repeating comment on patch 1. Can't we do this in nvme_init_subsystem() or may
>be better, in nvme_set_queue_count() or nvme_init_ctrl_finish() ?
nvme_set_queue_count() won’t even be called in this case, as its only used to
configure IO queues, which are not being configured for an admin controller.
 
If we move nvme_override_prohibited_io_queues() up the stack into either of your
suggested core.c functions, it will affect apple & fc drivers, which I don’t have any
means of testing. Any suggestions?

Thanks,
Kamaljit 

Re: [PATCH v2 3/3] nvme: prevent ioq creation for discovery controllers

[Damien Le Moal]

On 7/3/25 3:09 AM, Kamaljit Singh wrote:
> Hi Damien,
>  
> On 7/1/25 19:18, Damien Le Moal wrote:
>>>   /*
>>> - * An admin controller has one admin queue, but no I/O queues.
>>> + * An admin or discovery controller has one admin queue, but no I/O queues.
>>>    * Override queue_count so it only creates an admin queue.
>>>    */
>>>   void nvme_override_prohibited_io_queues(struct nvme_ctrl *ctrl)
>>>   {
>>> -     if (nvme_admin_ctrl(ctrl))
>>> +     if (nvme_admin_ctrl(ctrl) || nvme_discovery_ctrl(ctrl))
>>>                ctrl->queue_count = 1;
>>>   }
>>>   EXPORT_SYMBOL_GPL(nvme_override_prohibited_io_queues);
>>
>> Repeating comment on patch 1. Can't we do this in nvme_init_subsystem() or may
>> be better, in nvme_set_queue_count() or nvme_init_ctrl_finish() ?
> nvme_set_queue_count() won’t even be called in this case, as its only used to
> configure IO queues, which are not being configured for an admin controller.
>  
> If we move nvme_override_prohibited_io_queues() up the stack into either of your
> suggested core.c functions, it will affect apple & fc drivers, which I don’t have any
> means of testing. Any suggestions?

Why would the controller vendor or its fabric matter ?
If the controller type is admin or discovery, you want queue_count set to 1. So
I do not think there is any problem.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 3/3] nvme: prevent ioq creation for discovery controllers

[hch]

On Thu, Jul 03, 2025 at 08:59:47AM +0900, Damien Le Moal wrote:
> Why would the controller vendor or its fabric matter ?
> If the controller type is admin or discovery, you want queue_count set to 1. So
> I do not think there is any problem.

Exactly.

Re: [PATCH v2 3/3] nvme: prevent ioq creation for discovery controllers

[Hannes Reinecke]

On 7/2/25 02:58, Kamaljit Singh wrote:
> 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.

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.

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