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

[Kamaljit Singh <Kamaljit.Singh1@wdc.com>]

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