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

[Kamaljit Singh]

Hi Niklas & Keith,
 
On 2025/04/01 01:04, Niklas Cassel wrote:
 
>> > +   /* 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;
>> > +   }
>> 
>> The queue_count comes from the user. If the user provides a bad value
>> for an IO controller, you're erroring. If they provide a bad value for a
>> discovery or admin controller, you override the value. Why the different
>> behavior?
>> 
> 
> Good question.
Keith,
Yeah, that was me trying to plug the driver hole when I was hacking the
nvme-cli to pass a zero I/O queue value for an admin-controller. While
doing that allowed us to connect to an admin-controller, it didn't prevent
us from connecting to an I/O controller with zero I/O queues. Per the
spec that shouldn't be allowed. Hope that clarifies the reason for that
2nd change. I'll make it into its own patch.
 
 
> 
> My initial proposal was simply to override the user provided value
> to 1 (admin queue only) in case of admin (or discovery) controller.
>
> The check for queue_count < 2 should be in a separate patch, if we
> want that check at all.
Yes, I'll make that into a separate patch.
 
> But to be honest, the code did previously
> allow an I/O controller with just the admin queue and no I/O queues.
Agree. Initially, I was able to use that hole by forcing nvme-cli to
allow zero IOQs. But based on your suggested driver change we don't
need to patch nvme-cli anymore. That's slick!
 
However, from the nvme-cli's perspective it does feel awkward to be forced
by "nvme connect" to use -i <non-zero> for an admin-controller, even though
its now being overridden with this patch. We will have to come up with a
cleaner and standardized way to connect to an admin controller.  A standard
port number for an admin controller might be the way to go but it's not in
the spec yet.
 
 
> Thus, without a commit message explaining clearly why we should start
> to disallow an I/O controller with just the admin queue, I think that
> additional check is wrong.
For the separate patch you suggested, I'll add comments to make it clearer.
 
Thanks,
Kamaljit