Re: [PATCH] nvme: determine the number of IO queues

[Aaron Ma]

On 4/18/19 5:30 AM, Edmund Nadolski (Microsoft) wrote:
> On 4/17/19 7:12 AM, Aaron Ma wrote:
>> Some controllers support limited IO queues, when over set
>> the number, it will return invalid field error.
>> Then NVME will be removed by driver.
>>
>> Find the max number of IO queues that controller supports.
>> When it still got invalid result, set 1 IO queue at least to
>> bring NVME online.
>>
>> Signed-off-by: Aaron Ma <aaron.ma@xxxxxxxxxxxxx>
>> ---
>> Â drivers/nvme/host/core.c | 29 ++++++++++++++++++-----------
>> Â 1 file changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 2c43e12b70af..fb7f05c310c8 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -1134,14 +1134,24 @@ static int nvme_set_features(struct nvme_ctrl
>> *dev, unsigned fid, unsigned dword
>> Â Â int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count)
>> Â {
>> -ÂÂÂ u32 q_count = (*count - 1) | ((*count - 1) << 16);
>> +ÂÂÂ u32 q_count;
>> ÂÂÂÂÂ u32 result;
>> -ÂÂÂ int status, nr_io_queues;
>> -
>> -ÂÂÂ status = nvme_set_features(ctrl, NVME_FEAT_NUM_QUEUES, q_count,
>> NULL, 0,
>> -ÂÂÂÂÂÂÂÂÂÂÂ &result);
>> -ÂÂÂ if (status < 0)
>> -ÂÂÂÂÂÂÂ return status;
>> +ÂÂÂ int status = -1;
>> +ÂÂÂ int nr_io_queues;
>> +ÂÂÂ int try_count;
>> +
>> +ÂÂÂ for (try_count = *count; try_count > 0; try_count--) {
>> +ÂÂÂÂÂÂÂ q_count = (try_count - 1) | ((try_count - 1) << 16);
> 
> A macro here might help readability.

Will add in V2.

> 
>> +ÂÂÂÂÂÂÂ status = nvme_set_features(ctrl, NVME_FEAT_NUM_QUEUES,
>> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ q_count, NULL, 0, &result);
>> +ÂÂÂÂÂÂÂ if (status < 0)
>> +ÂÂÂÂÂÂÂÂÂÂÂ return status;
>> +ÂÂÂÂÂÂÂ else if (status == 0) {
> 
> else following return is not needed.

 *      0: successful read
 *      > 0: NVMe error status code
 *      < 0: Linux errno error code

3 conditions should be taken care.
status > 0 will be handled after loop.

else is needed.

> 
> 
>> +ÂÂÂÂÂÂÂÂÂÂÂ nr_io_queues = min(result & 0xffff, result >> 16) + 1;
> 
> Likewise, a macro as above.

Will add in V2.

> 
> 
> Ed
> 
>> +ÂÂÂÂÂÂÂÂÂÂÂ *count = min(try_count, nr_io_queues);
>> +ÂÂÂÂÂÂÂÂÂÂÂ break;
>> +ÂÂÂÂÂÂÂ }
>> +ÂÂÂ }
>> Â ÂÂÂÂÂ /*
>> ÂÂÂÂÂÂ * Degraded controllers might return an error when setting the
>> queue
>> @@ -1150,10 +1160,7 @@ int nvme_set_queue_count(struct nvme_ctrl
>> *ctrl, int *count)
>> ÂÂÂÂÂÂ */
>> ÂÂÂÂÂ if (status > 0) {
>> ÂÂÂÂÂÂÂÂÂ dev_err(ctrl->device, "Could not set queue count (%d)\n",
>> status);
>> -ÂÂÂÂÂÂÂ *count = 0;
>> -ÂÂÂ } else {
>> -ÂÂÂÂÂÂÂ nr_io_queues = min(result & 0xffff, result >> 16) + 1;
>> -ÂÂÂÂÂÂÂ *count = min(*count, nr_io_queues);
>> +ÂÂÂÂÂÂÂ *count = 1;
>> ÂÂÂÂÂ }
>> Â ÂÂÂÂÂ return 0;
>>
>