Re: [PATCH] Revert "nvme: verify MNAN value if ANA is enabled"

[Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>]

On 6/10/21 00:45, Daniel Wagner wrote:
> Hi Chaitanya,
>
> On Thu, Jun 10, 2021 at 02:55:08AM +0000, Chaitanya Kulkarni wrote:
>> On 6/9/21 19:45, Chaitanya Kulkarni wrote:
>>> This reverts commit 8872c159c7a83daf633768cee7a7ef7154010341. This is
>>> needed to move forward with the blktests for now, without this patch
>>> all the testcases result in the error :-
>>>
>>> [ 3502.072798] nvme nvme1: Invalid MNAN value 1024
>> Thinking about the code again I think following should work :-
>>
>> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
>> index 23573fe3fc7d..4277f1554bd5 100644
>> --- a/drivers/nvme/host/multipath.c
>> +++ b/drivers/nvme/host/multipath.c
>> @@ -813,7 +813,7 @@ int nvme_mpath_init_identify(struct nvme_ctrl *ctrl,
>> struct nvme_id_ctrl *id)
>>             !(ctrl->subsys->cmic & NVME_CTRL_CMIC_ANA))
>>                 return 0;
>>  
>> -       if (!ctrl->max_namespaces ||
>> +       if (ctrl->max_namespaces &&
>>             ctrl->max_namespaces > le32_to_cpu(id->nn)) {
>>                 dev_err(ctrl->device,
>>                         "Invalid MNAN value %u\n", ctrl->max_namespaces);
> '!ctrl->max_namespace' could also be written as
> 'ctrl->max_namespace != 0' which makes it more obvious what the intend
> is here:
>
>   If the controller supports Asymmetric Namespace Access Reporting, then
>   this field shall be set to a non-zero value that is less than or equal
>   to the NN value.
>
>

Let us look into the host side issue first then we can move to target side.

Consider a scenario where ANA enabled subsys with 0 namespaces on the target
side. When host issues connect command to such a controller ctrl->mnan
should be 0 and ctrl->nn should be 0 which should be valid.

With original check in the code :-

if (!ctrl->max_namespaces ||
    ctrl->max_namespaces > le32_to_cpu(id->nn))

!ctrl->max_namespaces will return in 1 and due to || condition will be
true and code will report the error. The change proposed in this patch
with above mentioned scenario :- if (ctrl->max_namespaces &&
ctrl->max_namespaces > le32_to_cpu(id->nn)) { ctrl->max_namespaces will
return 0 and due to && condition will be false and new code will not
report zero. So I think above suggested patch is needed irrespective of
the target fix.


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme