[PATCH] nvme: check for dynamic controller model before verifying cntlid

[PATCH] nvme: check for dynamic controller model before verifying cntlid

[David Milburn]

If controller supports dynamic controller model the cntlid returned
from the identify command maybe FFFFh causing check in nvme_init_identify()
to fail which results in "nvme discover" failure.

nvme discover -t rdma -a <target_IP>
Failed to write to /dev/nvme-fabrics: Invalid argument

Signed-off-by: David Milburn <dmilburn@redhat.com>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index fd7dea36c3b6..235aa1811446 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2812,7 +2812,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 		 * In fabrics we need to verify the cntlid matches the
 		 * admin connect
 		 */
-		if (ctrl->cntlid != le16_to_cpu(id->cntlid)) {
+		if (id->ctrattr && (ctrl->cntlid != le16_to_cpu(id->cntlid))) {
 			ret = -EINVAL;
 			goto out_free;
 		}
-- 
2.18.1


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

Re: [PATCH] nvme: check for dynamic controller model before verifying cntlid

[Christoph Hellwig]

On Wed, Oct 16, 2019 at 03:23:22PM -0500, David Milburn wrote:
> -		if (ctrl->cntlid != le16_to_cpu(id->cntlid)) {
> +		if (id->ctrattr && (ctrl->cntlid != le16_to_cpu(id->cntlid))) {

Where do get the all-Fs cntrlid?  Identify Controller should return
the actual controller id, and the fabrics setup code also sets it up
to the return value of the connect command, which shouldn't be all-Fs
either.

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

Re: [PATCH] nvme: check for dynamic controller model before verifying cntlid

[David Milburn]

Hi Christoph,

On 10/17/2019 09:50 AM, Christoph Hellwig wrote:
> On Wed, Oct 16, 2019 at 03:23:22PM -0500, David Milburn wrote:
>> -		if (ctrl->cntlid != le16_to_cpu(id->cntlid)) {
>> +		if (id->ctrattr && (ctrl->cntlid != le16_to_cpu(id->cntlid))) {
> 
> Where do get the all-Fs cntrlid?  Identify Controller should return
> the actual controller id, and the fabrics setup code also sets it up
> to the return value of the connect command, which shouldn't be all-Fs
> either.

I see it in nvme_init_identify() after doing the nvme_identify_ctrl(),
id->cntlid is FFFF causing the check to fail. There is a discussion
in the NVMe over Fabrics spec, not specifically about identify, but
with discover log page concerning dynamic vs static model and the
value of cntlid, the target is a storage array.

Thanks,
David

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

Re: [PATCH] nvme: check for dynamic controller model before verifying cntlid

[Christoph Hellwig]

On Thu, Oct 17, 2019 at 10:08:30AM -0500, David Milburn wrote:
> Hi Christoph,
>
> On 10/17/2019 09:50 AM, Christoph Hellwig wrote:
>> On Wed, Oct 16, 2019 at 03:23:22PM -0500, David Milburn wrote:
>>> -		if (ctrl->cntlid != le16_to_cpu(id->cntlid)) {
>>> +		if (id->ctrattr && (ctrl->cntlid != le16_to_cpu(id->cntlid))) {
>>
>> Where do get the all-Fs cntrlid?  Identify Controller should return
>> the actual controller id, and the fabrics setup code also sets it up
>> to the return value of the connect command, which shouldn't be all-Fs
>> either.
>
> I see it in nvme_init_identify() after doing the nvme_identify_ctrl(),
> id->cntlid is FFFF causing the check to fail. There is a discussion
> in the NVMe over Fabrics spec, not specifically about identify, but
> with discover log page concerning dynamic vs static model and the
> value of cntlid, the target is a storage array.

Please trace down where it comes from, because I suspect your array is
buggy.  I guess it returns 0xffff from the connect command response,
which is wrong as that should contain the actual controller id, and not
0xffff which is passed in the connect command to request the dynamic
controller model.

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