[PATCH v4 0/2] Support for Administrative Controllers

[PATCH v4 0/2] Support for Administrative Controllers

[Kamaljit Singh]

Patch-set summary:
------------------
[1] add capability to connect to an administrative controller
    - New function identifies an administrative controller via the 
      controller's CNTRLTYPE
    - Prevent I/O queue creation for administrative controllers by
      overriding the controller's queue_count and setting it 
      to 1, so only an admin connection is allowed to be created. 

[2] prevent admin controller from smart log fetch (LID 2)
    - Conditional around nvme_hwmon_init() now disallowes administrative 
      controllers during controller initialization.

Changelog:
----------
Changes from v3:
  - Remove inline helper nvme_override_prohibited_io_queues() and 
    move code directly into nvme_init_ctrl_finish()
  - Add more detailed comments and changelog to patch-set

Changes from v2:
  - Avoid calling nvme_override_prohibited_io_queues() from 
    RDMA and TCP (nvme_rdma_setup_ctrl()/nvme_tcp_setup_ctrl()) fabrics 
    drivers by moving it into the generic code under nvme_init_ctrl_finish()
  - Remove export of nvme_override_prohibited_io_queues()
  - Change announcing of administrative controller from dev_info to dev_dbg
  - Coding style change. Remove parenthesis in nvme_admin_ctrl() as 
    suggested by Damien

Changes from v1:
  - Remove check that disallowed zero I/O queues for an I/O controller
  - Create helper nvme_override_prohibited_io_queues() for identical code 
    in nvme_rdma_setup_ctrl() and nvme_tcp_setup_ctrl() 
  - Separate into multiple patches
  - Revert naming of nvmf_discovery_ctrl() back to nvme_discovery_ctrl() 
  - Move nvme_admin_ctrl() from nvme.h to core.c


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 | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

-- 
2.43.0

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

[Kamaljit Singh]

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

* Add helper nvme_admin_ctrl() to check if a controller's CNTRLTYPE
  indicates that it is an administrative controller.
* Override ctrl->queue_count to 1, so only one admin queue and no IO
  queues are allocated for an administrative controller.
* This override is done in nvme_init_ctrl_finish() after ctrl->cntrltype
  has been initialized in nvme_init_identify() so nvme_admin_ctrl() will
  work correctly.
* Doing this override in generic code (nvme_init_ctrl_finish) makes it
  transport agnostic and will work properly for nvme/tcp as well as for
  nvme/rdma.

Suggested-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Kamaljit Singh <kamaljit.singh1@wdc.com>
---
 drivers/nvme/host/core.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e533d791955d..a6388ed3ddca 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3149,6 +3149,11 @@ 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;
+}
+
 static bool nvme_validate_cntlid(struct nvme_subsystem *subsys,
 		struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 {
@@ -3670,6 +3675,17 @@ int nvme_init_ctrl_finish(struct nvme_ctrl *ctrl, bool was_suspended)
 	if (ret)
 		return ret;
 
+	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;
+	}
+
 	ret = nvme_configure_apst(ctrl);
 	if (ret < 0)
 		return ret;
-- 
2.43.0

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

[Christoph Hellwig]

Thanks,

added to nvme-6.17.

[PATCH v4 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 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.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
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 a6388ed3ddca..0cec1e58e845 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3700,7 +3700,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 v4 2/2] nvme: prevent admin controller from smart log fetch (LID 2)

[Christoph Hellwig]

On Mon, Jul 21, 2025 at 10:37:00AM -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.

Umm, as pointed out last time, this log page is prohibited for discovery
controllers, but optional for admin controllers.  So I don't see why
we want or need to skip it here.

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

[Kamaljit Singh]

Hi Christoph,

From: Christoph Hellwig <hch@lst.de>
Date: Monday, July 21, 2025 at 23:30
>> 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.

>Umm, as pointed out last time, this log page is prohibited for discovery
>controllers, but optional for admin controllers.  So I don't see why
>we want or need to skip it here.
Sorry, looks I may have misunderstood your last comment from v2 (pasted below).
By "let's leave this in", I thought you were referring to my patch. But now that I re-read
it, seems like by "failing a get_log page is fine" you probably meant to let the driver 
issue a get_log for LID=2 and fail.

Process wise, can you ignore this 2/2 patch and we call this patch-set done?
 
>From: Christoph Hellwig hch@lst.de
>Date: Thursday, July 3, 2025 at 02:05
>Subject: Re: [PATCH v2 2/3] nvme: prevent admin controller from smart log fetch (LID 2)
>> 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.
 
Thanks,
Kamaljit

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

[hch]

On Tue, Jul 22, 2025 at 04:34:05PM +0000, Kamaljit Singh wrote:
> By "let's leave this in", I thought you were referring to my patch. But now that I re-read
> it, seems like by "failing a get_log page is fine" you probably meant to let the driver 
> issue a get_log for LID=2 and fail.

Yes.

> Process wise, can you ignore this 2/2 patch and we call this patch-set done?

That's what I've done.  Patch 1 is queued up in the nvme-6.17 branch.

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

[Kamaljit Singh]

From: hch <hch@lst.de>
Date: Tuesday, July 22, 2025 at 23:11
>> By "let's leave this in", I thought you were referring to my patch. But now that I re-read
>> it, seems like by "failing a get_log page is fine" you probably meant to let the driver
>> issue a get_log for LID=2 and fail.
>
>Yes.

>> Process wise, can you ignore this 2/2 patch and we call this patch-set done?
>
>That's what I've done.  Patch 1 is queued up in the nvme-6.17 branch.

Yes, I already pulled it. Thank you Christoph!
 
-Kamaljit