Re: [PATCH v1 1/1] nvme: add admin controller support. prohibit ioq creation for admin & disco ctrlrs

[Kamaljit Singh]

Hi Damien,
 
On 2025/03/28 15:09, Damien Le Moal wrote:
>> Added capability to connect to an administrative controller by
> 
>s/Added/Add
Done
 
>> preventing ioq creation for admin-controllers. Also prevent ioq creation
>> for discovery-controllers as the spec prohibits them.
 
>This second part should be a different (preparatory) patch.
I've separated it as its own patch. Sorry took a while to procure the setup
But I’ve tested it now. Will share updated patch-set.
 
You were right, it is a pain using Outlook for kernel work :(
Vim to the rescue for now.
 
>> 
>> * Added nvme_admin_ctrl() to check for an administrative controller
 
>s/Added/Add
>And this should be a different preparatory patch.
Sounds good.
 
>> 
>> * Renamed nvme_discovery_ctrl() to nvmf_discovery_ctrl() as discovery is
>>   more relevant to fabrics
 
>I do not think that is necessary since this is testing the controller type,
>which may be limited to fabrics or not.
I undid that rename and moved the function as Niklas suggested, to core.c.
I tried moving nvme_discovery_ctrl(), nvme_admin_ctrl &
nvme_update_ctrl_queue_count() to host/nvme.h but couldn't since target/
passthru.c includes that host header & fails compilation.
 
To make it work that way we would have to include host/fabrics.h in target/
passthru.c but that's causing a lot of unresolved symbols and requires a
different/larger commit. Also, it may not even be desirable as it will further
pollute the target code with host specific header.  Please comment.
 
 
>> @@ -2863,13 +2858,19 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>>       else
>>               subsys->subtype = NVME_NQN_NVME;
>> 
>> -     if (nvme_discovery_ctrl(ctrl) && subsys->subtype != NVME_NQN_DISC) {
>> +     if (nvmf_discovery_ctrl(ctrl) && subsys->subtype != NVME_NQN_DISC) {
>>               dev_err(ctrl->device,
>>                       "Subsystem %s is not a discovery controller",
>>                       subsys->subnqn);
>>               kfree(subsys);
>>               return -EINVAL;
>>       }
>> +     if (nvme_admin_ctrl(ctrl)) {
>> +             dev_info(ctrl->device,
>> +                     "Subsystem %s is an administrative controller",
>> +                     subsys->subnqn);
>> +     }
 
>Is this really necessary ? In any case, please remove the curly brackets.
I've removed it. I had started off by adding a dev_err check similar to
the one for discovery ctrl above it but since there is no unique identifier
(like the SUBNQN or unique port) for an admin controller, I turned it into
a dev_info to at least announce the presence of an admin controller. Any
suggestions for error validation at init time?
 
 
>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index 299e3c19df9d..0f3150411bd5 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -1030,6 +1030,17 @@ static int nvme_rdma_setup_ctrl(struct nvme_rdma_ctrl *ctrl, bool new)
>>               goto destroy_admin;
>>       }
>> 
>> +     /* An admin or discovery controller has one admin queue, but no I/O queues */
>> +     if (nvme_admin_ctrl(&ctrl->ctrl) || nvmf_discovery_ctrl(&ctrl->ctrl)) {
>> +             ctrl->ctrl.queue_count = 1;
>> +     } else if (ctrl->ctrl.queue_count < 2) {
>> +             /* I/O controller with no I/O queues is not allowed */
>> +             ret = -EOPNOTSUPP;
>> +             dev_err(ctrl->ctrl.device,
>> +                     "I/O controller doesn't allow zero I/O queues!\n");
>> +             goto destroy_admin;
>> +     }
 
>This is identical to the change for tcp, so maybe make this a helper function ?
I've converted it into nvme_update_ctrl_queue_count() and added it to host/nvme.h.
Will be in the next patch-set.