Re: [PATCH v2] fusion-mptbase: handle failed allocation for workqueue

[Ewan Milne <emilne@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1480 bytes --]

On Thu, 2016-02-18 at 11:40 -0500, Ewan Milne wrote:
> It also appears to me upon further inspection that the existing code has
> other problems.  In particular, once mpt_mapresources() has returned
> with a nonzero error code, it looks like iounmap() should be called, but
> it is not in the case of a failed allocation of reset_work_q.  I'm also
> not sure why pci_release_selected_regions() is only called for the case
> of mpt_do_ioc_recovery() returning != -5 when it is called whenever
> there is a failed allocation of reset_work_q.
> 
> Consider the attached patch (untested, because I don't have hardware):
> It shows what I would do for labels & error handling.  If the rc != -5
> case of return from mpt_do_ioc_recovery() could be eliminated, then
> another label "out_free_fw_event_q:" could be added prior to the other
> error cases at the end, and all the code after the printk() in that path
> could be replaced by "goto out_free_fw_event_q;"
> 
> 	if ((r = mpt_do_ioc_recovery(ioc, MPT_HOSTEVENT_IOC_BRINGUP,
> 	    CAN_SLEEP)) != 0){
> 		printk(MYIOC_s_ERR_FMT "didn't initialize properly! (%d)\n",
> 		    ioc->name, r);
> 		goto out_free_fw_event_q;
> 	}
> ...
> 
> out_free_fw_event_q:
> 	destroy_workqueue(ioc->fw_event_q);
> 	ioc->fw_event_q = NULL;
> 
> out_remove_ioc:
> ...
> 
> However I do not know if that change is legitimate.
> 
> -Ewan

There is an error in the patch attached in my previous mail.
Please refer to the attached PATCH v2 RFC.

-Ewan


[-- Attachment #2: 0001-mptbase-fixup-error-handling-paths-in-mpt_attach.patch --]
[-- Type: text/x-patch, Size: 3018 bytes --]

>From 79fd59c66c3afb339cf9fdd1fda65e78e32831a4 Mon Sep 17 00:00:00 2001
From: "Ewan D. Milne" <emilne@redhat.com>
Date: Thu, 18 Feb 2016 11:49:37 -0500
Subject: [PATCH v2 RFC] mptbase: fixup error handling paths in mpt_attach()

mpt_attach() was not checking for the failure to create fw_event_q.
Also, iounmap() was not being called in all error cases after ioremap()
had been called by mpt_mapresources().
---
 drivers/message/fusion/mptbase.c | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
index 5dcc031..a4f362b 100644
--- a/drivers/message/fusion/mptbase.c
+++ b/drivers/message/fusion/mptbase.c
@@ -1801,8 +1801,7 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	ioc->pcidev = pdev;
 	if (mpt_mapresources(ioc)) {
-		kfree(ioc);
-		return r;
+		goto out_free_ioc;
 	}
 
 	/*
@@ -1871,9 +1870,8 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (!ioc->reset_work_q) {
 		printk(MYIOC_s_ERR_FMT "Insufficient memory to add adapter!\n",
 		    ioc->name);
-		pci_release_selected_regions(pdev, ioc->bars);
-		kfree(ioc);
-		return -ENOMEM;
+		r = -ENOMEM;
+		goto out_unmap_resources;
 	}
 
 	dinitprintk(ioc, printk(MYIOC_s_INFO_FMT "facts @ %p, pfacts[0] @ %p\n",
@@ -1995,12 +1993,21 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
 	spin_lock_init(&ioc->fw_event_lock);
 	snprintf(ioc->fw_event_q_name, MPT_KOBJ_NAME_LEN, "mpt/%d", ioc->id);
 	ioc->fw_event_q = create_singlethread_workqueue(ioc->fw_event_q_name);
+	if (!ioc->fw_event_q) {
+		printk(MYIOC_s_ERR_FMT "Insufficient memory to add adapter!\n",
+		    ioc->name);
+		r = -ENOMEM;
+		goto out_remove_ioc;
+	}
 
 	if ((r = mpt_do_ioc_recovery(ioc, MPT_HOSTEVENT_IOC_BRINGUP,
 	    CAN_SLEEP)) != 0){
 		printk(MYIOC_s_ERR_FMT "didn't initialize properly! (%d)\n",
 		    ioc->name, r);
 
+		destroy_workqueue(ioc->fw_event_q);
+		ioc->fw_event_q = NULL;
+
 		list_del(&ioc->list);
 		if (ioc->alt_ioc)
 			ioc->alt_ioc->alt_ioc = NULL;
@@ -2040,6 +2047,24 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
 			msecs_to_jiffies(MPT_POLLING_INTERVAL));
 
 	return 0;
+
+out_remove_ioc:
+	list_del(&ioc->list);
+	if (ioc->alt_ioc)
+		ioc->alt_ioc->alt_ioc = NULL;
+	pci_set_drvdata(ioc->pcidev, NULL);
+
+	destroy_workqueue(ioc->reset_work_q);
+	ioc->reset_work_q = NULL;
+
+out_unmap_resources:
+	iounmap(ioc->memmap);
+	pci_release_selected_regions(pdev, ioc->bars);
+
+out_free_ioc:
+	kfree(ioc);
+
+	return r;
 }
 
 /*=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=*/
@@ -6229,7 +6254,7 @@ mpt_get_manufacturing_pg_0(MPT_ADAPTER *ioc)
 	memcpy(ioc->board_assembly, pbuf->BoardAssembly, sizeof(ioc->board_assembly));
 	memcpy(ioc->board_tracer, pbuf->BoardTracerNumber, sizeof(ioc->board_tracer));
 
-	out:
+out:
 
 	if (pbuf)
 		pci_free_consistent(ioc->pcidev, hdr.PageLength * 4, pbuf, buf_dma);
-- 
2.1.0