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

[Kamaljit Singh]

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