NVMe: Do not save result for range type feature

The LBA range type feature being optional, we do not want to save the result or propagate it up the call stack when a device does not support it.

Signed-off-by Keith Busch <keith.busch at intel.com>

---
 drivers/block/nvme-core.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

drivers/block/nvme-core.c +2 -3

index a89f7db..00819d7 100644
@@ -1544,9 +1544,8 @@ static int nvme_dev_add(struct nvme_dev *dev)
 		if (id_ns->ncap == 0)
 			continue;
 
-		res = nvme_get_features(dev, NVME_FEAT_LBA_RANGE, i,
-							dma_addr + 4096, NULL);
-		if (res)
+		if (nvme_get_features(dev, NVME_FEAT_LBA_RANGE, i,
+							dma_addr + 4096, NULL))
 			memset(mem + 4096, 0, 4096);
 
 		ns = nvme_alloc_ns(dev, i, mem, mem + 4096);

Signature · 7 lines

1.7.1


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

Quoted 3 lines · Keith Busch

The LBA range type feature being optional, we do not want to save the result or propagate it up the call stack when a device does not support it.

Looking at the whole function, the description of what it returns is rather odd. "If nvme_setup_io_queues fails, returns that error. If nvme_identify for the controller fails, returns -EIO. Otherwise, return an error if nvme_identify for the last namespace fails or if nvme_get_features for the last namespace fails."

So we always discard errors for not-the-last namespace, but return an error if identify or get_features for the last namespace gets an error. Worse, on returning an error, the caller will free the queues and all the controller structures, even though the namespaces have been registered as devices! I really didn't think through the error path here, did I? :-)

So, I would propose that we _always_ discard any errors we hit going through the namespace addition loop. As long as we've been able to set up queues and identify the controller, this function should return success.

And I think *that* patch is as simple as adding:

                       list_add_tail(&ns->list, &dev->namespaces);
       }

(quoted diff) +1 -0

+	res = 0;
       list_for_each_entry(ns, &dev->namespaces, list)
               add_disk(ns->disk);

What do you think?

Quoted 3 lines · Keith Busch

Quoted 1 line

The LBA range type feature being optional, we do not want to save

the

Quoted 11 lines

Quoted 2 lines

result or propagate it up the call stack when a device does not support it.

Looking at the whole function, the description of what it returns is rather odd. "If nvme_setup_io_queues fails, returns that error. If nvme_identify for the controller fails, returns -EIO. Otherwise, return an error if nvme_identify for the last namespace fails or if nvme_get_features for the last namespace fails."

So we always discard errors for not-the-last namespace, but return an error if identify or get_features for the last namespace gets an

error.

Quoted 8 lines

Worse, on returning an error, the caller will free the queues and all the controller structures, even though the namespaces have been registered as devices! I really didn't think through the error path here, did I? :-)

So, I would propose that we _always_ discard any errors we hit going through the namespace addition loop. As long as we've been able to set up queues and identify the controller, this function should

return success.

Quoted 3 lines

And I think *that* patch is as simple as adding:

Quoted 7 lines

                       list_add_tail(&ns->list, &dev->namespaces);
       }

(quoted diff) +1 -0

+	res = 0;
       list_for_each_entry(ns, &dev->namespaces, list)
               add_disk(ns->disk);

What do you think?