PERFORCE change 108876 for review

Scott Long scottl at FreeBSD.org
Wed Nov 1 00:45:28 UTC 2006


http://perforce.freebsd.org/chv.cgi?CH=108876

Change 108876 by scottl at scottl-x64 on 2006/11/01 00:44:27

	Don't hold the MPT lock over all of mpt_attach(), it causes all sorts
	of problems with memory allocations and system APIs.

Affected files ...

.. //depot/projects/scottl-camlock/src/sys/dev/mpt/mpt.c#14 edit
.. //depot/projects/scottl-camlock/src/sys/dev/mpt/mpt_cam.c#15 edit
.. //depot/projects/scottl-camlock/src/sys/dev/mpt/mpt_pci.c#16 edit
.. //depot/projects/scottl-camlock/src/sys/dev/mpt/mpt_raid.c#9 edit

Differences ...

==== //depot/projects/scottl-camlock/src/sys/dev/mpt/mpt.c#14 (text+ko) ====

@@ -2110,11 +2110,13 @@
 	TAILQ_INIT(&mpt->request_pending_list);
 	TAILQ_INIT(&mpt->request_free_list);
 	TAILQ_INIT(&mpt->request_timeout_list);
+	MPT_LOCK(mpt);
 	for (val = 0; val < MPT_MAX_REQUESTS(mpt); val++) {
 		request_t *req = &mpt->request_pool[val];
 		req->state = REQ_STATE_ALLOCATED;
 		mpt_free_request(mpt, req);
 	}
+	MPT_UNLOCK(mpt);
 
 	for (val = 0; val < MPT_MAX_LUNS; val++) {
 		STAILQ_INIT(&mpt->trt[val].atios);
@@ -2130,7 +2132,9 @@
 	mpt_lprt(mpt, MPT_PRT_DEBUG, "doorbell req = %s\n",
 	    mpt_ioc_diag(mpt_read(mpt, MPT_OFFSET_DOORBELL)));
 
+	MPT_LOCK(mpt);
 	error = mpt_configure_ioc(mpt);
+	MPT_UNLOCK(mpt);
 
 	return (error);
 }
@@ -2143,6 +2147,7 @@
 	 * not enabled, ports not enabled and interrupts
 	 * not enabled.
 	 */
+	MPT_LOCK(mpt);
 
 	/*
 	 * Enable asynchronous event reporting- all personalities
@@ -2177,8 +2182,10 @@
 	 */
 	if (mpt_send_port_enable(mpt, 0) != MPT_OK) {
 		mpt_prt(mpt, "failed to enable port 0\n");
+		MPT_UNLOCK(mpt);
 		return (ENXIO);
 	}
+	MPT_UNLOCK(mpt);
 	return (0);
 }
 

==== //depot/projects/scottl-camlock/src/sys/dev/mpt/mpt_cam.c#15 (text+ko) ====

@@ -210,6 +210,7 @@
 	int		 maxq;
 	int		 error;
 
+	MPT_LOCK(mpt);
 	TAILQ_INIT(&mpt->request_timeout_list);
 	maxq = (mpt->mpt_global_credits < MPT_MAX_REQUESTS(mpt))?
 	    mpt->mpt_global_credits : MPT_MAX_REQUESTS(mpt);
@@ -218,14 +219,16 @@
 	error = mpt_register_handler(mpt, MPT_HANDLER_REPLY, handler,
 				     &scsi_io_handler_id);
 	if (error != 0) {
-		goto cleanup0;
+		MPT_UNLOCK(mpt);
+		goto cleanup;
 	}
 
 	handler.reply_handler = mpt_scsi_tmf_reply_handler;
 	error = mpt_register_handler(mpt, MPT_HANDLER_REPLY, handler,
 				     &scsi_tmf_handler_id);
 	if (error != 0) {
-		goto cleanup0;
+		MPT_UNLOCK(mpt);
+		goto cleanup;
 	}
 
 	/*
@@ -237,11 +240,13 @@
 		error = mpt_register_handler(mpt, MPT_HANDLER_REPLY, handler,
 		    &fc_els_handler_id);
 		if (error != 0) {
-			goto cleanup0;
+			MPT_UNLOCK(mpt);
+			goto cleanup;
 		}
 		if (mpt_add_els_buffers(mpt) == FALSE) {
 			error = ENOMEM;
-			goto cleanup0;
+			MPT_UNLOCK(mpt);
+			goto cleanup;
 		}
 		maxq -= mpt->els_cmds_allocated;
 	}
@@ -256,7 +261,8 @@
 		error = mpt_register_handler(mpt, MPT_HANDLER_REPLY, handler,
 		    &mpt->scsi_tgt_handler_id);
 		if (error != 0) {
-			goto cleanup0;
+			MPT_UNLOCK(mpt);
+			goto cleanup;
 		}
 	}
 
@@ -267,7 +273,8 @@
 	if (mpt->tmf_req == NULL) {
 		mpt_prt(mpt, "Unable to allocate dedicated TMF request!\n");
 		error = ENOMEM;
-		goto cleanup0;
+		MPT_UNLOCK(mpt);
+		goto cleanup;
 	}
 
 	/*
@@ -279,18 +286,18 @@
 	mpt->tmf_req->state = REQ_STATE_FREE;
 	maxq--;
 
+	/*
+	 * The rest of this is CAM foo, for which we need to drop our lock
+	 */
+	MPT_UNLOCK(mpt);
+
 	if (mpt_spawn_recovery_thread(mpt) != 0) {
 		mpt_prt(mpt, "Unable to spawn recovery thread!\n");
 		error = ENOMEM;
-		goto cleanup0;
+		goto cleanup;
 	}
 
 	/*
-	 * The rest of this is CAM foo, for which we need to drop our lock
-	 */
-	MPTLOCK_2_CAMLOCK(mpt);
-
-	/*
 	 * Create the device queue for our SIM(s).
 	 */
 	devq = cam_simq_alloc(maxq);
@@ -315,9 +322,11 @@
 	/*
 	 * Register exactly this bus.
 	 */
+	MPT_LOCK(mpt);
 	if (xpt_bus_register(mpt->sim, 0) != CAM_SUCCESS) {
 		mpt_prt(mpt, "Bus registration Failed!\n");
 		error = ENOMEM;
+		MPT_UNLOCK(mpt);
 		goto cleanup;
 	}
 
@@ -325,15 +334,16 @@
 	    CAM_TARGET_WILDCARD, CAM_LUN_WILDCARD) != CAM_REQ_CMP) {
 		mpt_prt(mpt, "Unable to allocate Path!\n");
 		error = ENOMEM;
+		MPT_UNLOCK(mpt);
 		goto cleanup;
 	}
+	MPT_UNLOCK(mpt);
 
 	/*
 	 * Only register a second bus for RAID physical
 	 * devices if the controller supports RAID.
 	 */
 	if (mpt->ioc_page2 == NULL || mpt->ioc_page2->MaxPhysDisks == 0) {
-		CAMLOCK_2_MPTLOCK(mpt);
 		return (0);
 	}
 
@@ -351,9 +361,11 @@
 	/*
 	 * Register this bus.
 	 */
+	MPT_LOCK(mpt);
 	if (xpt_bus_register(mpt->phydisk_sim, 1) != CAM_SUCCESS) {
 		mpt_prt(mpt, "Physical Disk Bus registration Failed!\n");
 		error = ENOMEM;
+		MPT_UNLOCK(mpt);
 		goto cleanup;
 	}
 
@@ -362,15 +374,14 @@
 	    CAM_TARGET_WILDCARD, CAM_LUN_WILDCARD) != CAM_REQ_CMP) {
 		mpt_prt(mpt, "Unable to allocate Physical Disk Path!\n");
 		error = ENOMEM;
+		MPT_UNLOCK(mpt);
 		goto cleanup;
 	}
-	CAMLOCK_2_MPTLOCK(mpt);
+	MPT_UNLOCK(mpt);
 	mpt_lprt(mpt, MPT_PRT_DEBUG, "attached cam\n");
 	return (0);
 
 cleanup:
-	CAMLOCK_2_MPTLOCK(mpt);
-cleanup0:
 	mpt_cam_detach(mpt);
 	return (error);
 }
@@ -793,29 +804,38 @@
 int
 mpt_cam_enable(struct mpt_softc *mpt)
 {
+	int error;
+
+	MPT_LOCK(mpt);
+
+	error = EIO;
 	if (mpt->is_fc) {
 		if (mpt_read_config_info_fc(mpt)) {
-			return (EIO);
+			goto out;
 		}
 		if (mpt_set_initial_config_fc(mpt)) {
-			return (EIO);
+			goto out;
 		}
 	} else if (mpt->is_sas) {
 		if (mpt_read_config_info_sas(mpt)) {
-			return (EIO);
+			goto out;
 		}
 		if (mpt_set_initial_config_sas(mpt)) {
-			return (EIO);
+			goto out;
 		}
 	} else if (mpt->is_spi) {
 		if (mpt_read_config_info_spi(mpt)) {
-			return (EIO);
+			goto out;
 		}
 		if (mpt_set_initial_config_spi(mpt)) {
-			return (EIO);
+			goto out;
 		}
 	}
-	return (0);
+	error = 0;
+
+out:
+	MPT_UNLOCK(mpt);
+	return (error);
 }
 
 void
@@ -842,6 +862,7 @@
 {
 	mpt_handler_t handler;
 
+	MPT_LOCK(mpt);
 	mpt_terminate_recovery_thread(mpt); 
 
 	handler.reply_handler = mpt_scsi_reply_handler;
@@ -862,23 +883,20 @@
 		mpt_free_request(mpt, mpt->tmf_req);
 		mpt->tmf_req = NULL;
 	}
+	MPT_UNLOCK(mpt);
 
 	if (mpt->sim != NULL) {
-		MPTLOCK_2_CAMLOCK(mpt);
 		xpt_free_path(mpt->path);
 		xpt_bus_deregister(cam_sim_path(mpt->sim));
 		cam_sim_free(mpt->sim, TRUE);
 		mpt->sim = NULL;
-		CAMLOCK_2_MPTLOCK(mpt);
 	}
 
 	if (mpt->phydisk_sim != NULL) {
-		MPTLOCK_2_CAMLOCK(mpt);
 		xpt_free_path(mpt->phydisk_path);
 		xpt_bus_deregister(cam_sim_path(mpt->phydisk_sim));
 		cam_sim_free(mpt->phydisk_sim, TRUE);
 		mpt->phydisk_sim = NULL;
-		CAMLOCK_2_MPTLOCK(mpt);
 	}
 }
 

==== //depot/projects/scottl-camlock/src/sys/dev/mpt/mpt_pci.c#16 (text+ko) ====

@@ -559,12 +559,9 @@
 
 	/* Initialize the hardware */
 	if (mpt->disabled == 0) {
-		MPT_LOCK(mpt);
 		if (mpt_attach(mpt) != 0) {
-			MPT_UNLOCK(mpt);
 			goto bad;
 		}
-		MPT_UNLOCK(mpt);
 	} else {
 		mpt_prt(mpt, "device disabled at user request\n");
 		goto bad;
@@ -575,9 +572,7 @@
 
 	if (mpt->eh == NULL) {
 		mpt_prt(mpt, "shutdown event registration failed\n");
-		MPT_LOCK(mpt);
 		(void) mpt_detach(mpt);
-		MPT_UNLOCK(mpt);
 		goto bad;
 	}
 	return (0);
@@ -636,7 +631,6 @@
 	mpt  = (struct mpt_softc*)device_get_softc(dev);
 
 	if (mpt) {
-		MPT_LOCK(mpt);
 		mpt_disable_ints(mpt);
 		mpt_detach(mpt);
 		mpt_reset(mpt, /*reinit*/FALSE);
@@ -646,7 +640,6 @@
 		if (mpt->eh != NULL) {
                         EVENTHANDLER_DEREGISTER(shutdown_final, mpt->eh);
 		}
-		MPT_UNLOCK(mpt);
 	}
 	return(0);
 }
@@ -663,9 +656,7 @@
 	mpt = (struct mpt_softc *)device_get_softc(dev);
 	if (mpt) {
 		int r;
-		MPT_LOCK(mpt);
 		r = mpt_shutdown(mpt);
-		MPT_UNLOCK(mpt);
 		return (r);
 	}
 	return(0);

==== //depot/projects/scottl-camlock/src/sys/dev/mpt/mpt_raid.c#9 (text+ko) ====

@@ -270,6 +270,13 @@
 
 	mpt_callout_init(&mpt->raid_timer);
 
+	error = mpt_spawn_raid_thread(mpt);
+	if (error != 0) {
+		mpt_prt(mpt, "Unable to spawn RAID thread!\n");
+		goto cleanup;
+	}
+ 
+	MPT_LOCK(mpt);
 	handler.reply_handler = mpt_raid_reply_handler;
 	error = mpt_register_handler(mpt, MPT_HANDLER_REPLY, handler,
 				     &raid_handler_id);
@@ -278,28 +285,22 @@
 		goto cleanup;
 	}
 
-	error = mpt_spawn_raid_thread(mpt);
-	if (error != 0) {
-		mpt_prt(mpt, "Unable to spawn RAID thread!\n");
-		goto cleanup;
-	}
- 
 	xpt_setup_ccb(&csa.ccb_h, mpt->path, 5);
 	csa.ccb_h.func_code = XPT_SASYNC_CB;
 	csa.event_enable = AC_FOUND_DEVICE;
 	csa.callback = mpt_raid_async;
 	csa.callback_arg = mpt;
-	MPTLOCK_2_CAMLOCK(mpt);
 	xpt_action((union ccb *)&csa);
-	CAMLOCK_2_MPTLOCK(mpt);
 	if (csa.ccb_h.status != CAM_REQ_CMP) {
 		mpt_prt(mpt, "mpt_raid_attach: Unable to register "
 			"CAM async handler.\n");
 	}
+	MPT_UNLOCK(mpt);
 
 	mpt_raid_sysctl_attach(mpt);
 	return (0);
 cleanup:
+	MPT_UNLOCK(mpt);
 	mpt_raid_detach(mpt);
 	return (error);
 }
@@ -317,6 +318,7 @@
 	mpt_handler_t handler;
 
 	callout_stop(&mpt->raid_timer);
+	MPT_LOCK(mpt);
 	mpt_terminate_raid_thread(mpt); 
 
 	handler.reply_handler = mpt_raid_reply_handler;
@@ -327,9 +329,8 @@
 	csa.event_enable = 0;
 	csa.callback = mpt_raid_async;
 	csa.callback_arg = mpt;
-	MPTLOCK_2_CAMLOCK(mpt);
 	xpt_action((union ccb *)&csa);
-	CAMLOCK_2_MPTLOCK(mpt);
+	MPT_UNLOCK(mpt);
 }
 
 static void
@@ -620,12 +621,17 @@
 	 * reject I/O to an ID we later determine is for a
 	 * hidden physdisk.
 	 */
+	MPT_LOCK(mpt);
 	xpt_freeze_simq(mpt->phydisk_sim, 1);
+	MPT_UNLOCK(mpt);
 	error = mpt_kthread_create(mpt_raid_thread, mpt,
 	    &mpt->raid_thread, /*flags*/0, /*altstack*/0,
 	    "mpt_raid%d", mpt->unit);
-	if (error != 0)
+	if (error != 0) {
+		MPT_LOCK(mpt);
 		xpt_release_simq(mpt->phydisk_sim, /*run_queue*/FALSE);
+		MPT_UNLOCK(mpt);
+	}
 	return (error);
 }
 


More information about the p4-projects mailing list