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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <BY5PR04MB6849B60D0ACFD724C1B8171DBC56A@BY5PR04MB6849.namprd04.prod.outlook.com>
Date: Wed, 16 Jul 2025 21:27:28 +0000
From: Kamaljit Singh <Kamaljit.Singh1@....com>
To: Hannes Reinecke <hare@...e.de>, "kbusch@...nel.org" <kbusch@...nel.org>,
	"axboe@...nel.dk" <axboe@...nel.dk>, hch <hch@....de>, "sagi@...mberg.me"
	<sagi@...mberg.me>, "linux-nvme@...ts.infradead.org"
	<linux-nvme@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
CC: "cassel@...nel.org" <cassel@...nel.org>, "dlemoal@...nel.org"
	<dlemoal@...nel.org>
Subject: Re: [PATCH v2 1/3] nvme: add capability to connect to an admin
 controller

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