This patchset adds a few simple cleanups to the nvme code. Edmund Nadolski (2): nvme: remove unused return code from nvme_alloc_ns nvme: else following return is not needed drivers/nvme/host/core.c | 21 ++++++--------------- drivers/nvme/host/pci.c | 2 +- 2 files changed, 7 insertions(+), 16 deletions(-) -- 2.20.1 _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
The return code of nvme_alloc_ns is never used, so change it to void. Signed-off-by: Edmund Nadolski <edmund.nadolski@intel.com> --- drivers/nvme/host/core.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 9696404a6182..c22d986d053a 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -3446,7 +3446,7 @@ static int nvme_setup_streams_ns(struct nvme_ctrl *ctrl, struct nvme_ns *ns) return 0; } -static int nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) +static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) { struct nvme_ns *ns; struct gendisk *disk; @@ -3456,13 +3456,11 @@ static int nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node); if (!ns) - return -ENOMEM; + return; ns->queue = blk_mq_init_queue(ctrl->tagset); - if (IS_ERR(ns->queue)) { - ret = PTR_ERR(ns->queue); + if (IS_ERR(ns->queue)) goto out_free_ns; - } if (ctrl->opts && ctrl->opts->data_digest) ns->queue->backing_dev_info->capabilities @@ -3485,10 +3483,8 @@ static int nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) if (ret) goto out_free_queue; - if (id->ncap == 0) { - ret = -EINVAL; + if (id->ncap == 0) goto out_free_id; - } ret = nvme_init_ns_head(ns, nsid, id); if (ret) @@ -3497,10 +3493,8 @@ static int nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) nvme_set_disk_name(disk_name, ns, ctrl, &flags); disk = alloc_disk_node(0, node); - if (!disk) { - ret = -ENOMEM; + if (!disk) goto out_unlink_ns; - } disk->fops = &nvme_fops; disk->private_data = ns; @@ -3531,7 +3525,7 @@ static int nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) nvme_fault_inject_init(&ns->fault_inject, ns->disk->disk_name); kfree(id); - return 0; + return; out_put_disk: put_disk(ns->disk); out_unlink_ns: @@ -3545,9 +3539,6 @@ static int nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) blk_cleanup_queue(ns->queue); out_free_ns: kfree(ns); - if (ret > 0) - ret = blk_status_to_errno(nvme_error_status(ret)); - return ret; } static void nvme_ns_remove(struct nvme_ns *ns) -- 2.20.1 _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
Remove unnecessary keyword in nvme_create_queue(). Signed-off-by: Edmund Nadolski <edmund.nadolski@intel.com> --- drivers/nvme/host/pci.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c index c40a672e5047..76aa4bef27b8 100644 --- a/drivers/nvme/host/pci.c +++ b/drivers/nvme/host/pci.c @@ -1548,7 +1548,7 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid, bool polled) result = adapter_alloc_sq(dev, qid, nvmeq); if (result < 0) return result; - else if (result) + if (result) goto release_cq; nvmeq->cq_vector = vector; -- 2.20.1 _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
On 25/11/2019 17:06, Edmund Nadolski wrote: > The return code of nvme_alloc_ns is never used, so change it > to void. Oh no, please do it the other way round, check the return value of nvme_alloc_ns() and handle error properly. nvme_alloc_ns() is doing memory allocations and these can fail (although less likely because of the GFP_KERNEL allocations). -- Johannes Thumshirn SUSE Labs Filesystems jthumshirn@suse.de +49 911 74053 689 SUSE Software Solutions Germany GmbH Maxfeldstr. 5 90409 Nürnberg Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
On 11/25/2019 9:09 AM, Johannes Thumshirn wrote: > On 25/11/2019 17:06, Edmund Nadolski wrote: >> The return code of nvme_alloc_ns is never used, so change it >> to void. > > Oh no, please do it the other way round, check the return value of > nvme_alloc_ns() and handle error properly. > > nvme_alloc_ns() is doing memory allocations and these can fail (although > less likely because of the GFP_KERNEL allocations). I considered that, tho it looked to me like the caller (nvme_validate_ns) and its callers (nvme_scan_ns_list, nvme_scan_ns_sequential) don't really care, and intend simply to continue on regardless of memory alloc failure(s). I can either add the -ENOMEM handling, or clarify the caller's intent w/some comments. Which is the way to go? Thanks, Ed _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
On Mon, Nov 25, 2019 at 09:46:08AM -0700, Nadolski, Edmund wrote: > On 11/25/2019 9:09 AM, Johannes Thumshirn wrote: > > On 25/11/2019 17:06, Edmund Nadolski wrote: > > > The return code of nvme_alloc_ns is never used, so change it > > > to void. > > > > Oh no, please do it the other way round, check the return value of > > nvme_alloc_ns() and handle error properly. > > > > nvme_alloc_ns() is doing memory allocations and these can fail (although > > less likely because of the GFP_KERNEL allocations). > > I considered that, tho it looked to me like the caller (nvme_validate_ns) > and its callers (nvme_scan_ns_list, nvme_scan_ns_sequential) don't really > care, and intend simply to continue on regardless of memory alloc > failure(s). > > I can either add the -ENOMEM handling, or clarify the caller's intent w/some > comments. Which is the way to go? It would be informative to just log the error that occurred so we have something indicating why an expected namespace wasn't created. Otherwise, I can't think of anything else the caller should do in response to an error in this path. _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
On 11/25/2019 09:07 AM, Keith Busch wrote: > It would be informative to just log the error that occurred so we have > something indicating why an expected namespace wasn't created. Otherwise, > I can't think of anything else the caller should do in response to an > error in this path. Please do this, shouldn't be ignoring return value completely at first place. _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
On Tue, Nov 26, 2019 at 04:01:11AM +0000, Chaitanya Kulkarni wrote: > On 11/25/2019 09:07 AM, Keith Busch wrote: > > It would be informative to just log the error that occurred so we have > > something indicating why an expected namespace wasn't created. Otherwise, > > I can't think of anything else the caller should do in response to an > > error in this path. > > Please do this, shouldn't be ignoring return value completely at first > place. Well, what would you do instead? We call nvme_alloc_ns during the initial probe and every reset / recan. If we fail to allocate a controller for whatever reason we'd much rather still have a working controller with at least the admin queue and maybe other namespaces rather than shutting everything down. So just logging a message inside nvme_alloc_ns as suggested by Keith sounds like a good idea to me. _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
On Mon, Nov 25, 2019 at 09:06:12AM -0700, Edmund Nadolski wrote: > Remove unnecessary keyword in nvme_create_queue(). > > Signed-off-by: Edmund Nadolski <edmund.nadolski@intel.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme
Applied to nvme/for-5.5 _______________________________________________ linux-nvme mailing list linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme