PERFORCE change 110660 for review

[Scott Long]

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

Change 110660 by scottl at scottl-x64 on 2006/11/29 07:05:25

	Calling xpt_action(XPT_SASYNC_CB) can wind up calling into other
	SIMs.  This quickly becomes a locking nightmare, so decouple it
	from the caller via a taskqueue.

Affected files ...

.. //depot/projects/scottl-camlock/src/sys/cam/cam_ccb.h#10 edit
.. //depot/projects/scottl-camlock/src/sys/cam/cam_xpt.c#44 edit

Differences ...

==== //depot/projects/scottl-camlock/src/sys/cam/cam_ccb.h#10 (text+ko) ====

@@ -246,8 +246,7 @@
 typedef union {
 	void		*ptr;
 	u_long		field;
-	u_int8_t	bytes[sizeof(void *) > sizeof(u_long)
-			      ? sizeof(void *) : sizeof(u_long)];
+	u_int8_t	bytes[sizeof(uintptr_t)];
 } ccb_priv_entry;
 
 typedef union {
@@ -277,6 +276,7 @@
 	u_int32_t	flags;		/* ccb_flags */
 	ccb_ppriv_area	periph_priv;
 	ccb_spriv_area	sim_priv;
+	void		*xptpriv;	/* Holds a task object if needed */
 	u_int32_t	timeout;	/* Timeout value */
 
 	/*

==== //depot/projects/scottl-camlock/src/sys/cam/cam_xpt.c#44 (text+ko) ====

@@ -42,6 +42,7 @@
 #include <sys/md5.h>
 #include <sys/interrupt.h>
 #include <sys/sbuf.h>
+#include <sys/taskqueue.h>
 
 #include <sys/lock.h>
 #include <sys/mutex.h>
@@ -3006,6 +3007,86 @@
 	return(1);
 }
 
+static void
+xpt_action_sasync_cb(void *context, int pending)
+{
+	union ccb *start_ccb;
+	struct ccb_setasync *csa;
+	struct async_node *cur_entry;
+	struct async_list *async_head;
+	u_int32_t added;
+	int s;
+
+	start_ccb = (union ccb *)context;
+	csa = &start_ccb->csa;
+	added = csa->event_enable;
+	async_head = &csa->ccb_h.path->device->asyncs;
+
+	/*
+	 * If there is already an entry for us, simply
+	 * update it.
+	 */
+	s = splcam();
+	mtx_lock(csa->ccb_h.path->bus->sim->mtx);
+	cur_entry = SLIST_FIRST(async_head);
+	while (cur_entry != NULL) {
+		if ((cur_entry->callback_arg == csa->callback_arg)
+		 && (cur_entry->callback == csa->callback))
+			break;
+		cur_entry = SLIST_NEXT(cur_entry, links);
+	}
+
+	if (cur_entry != NULL) {
+	 	/*
+		 * If the request has no flags set,
+		 * remove the entry.
+		 */
+		added &= ~cur_entry->event_enable;
+		if (csa->event_enable == 0) {
+			SLIST_REMOVE(async_head, cur_entry,
+				     async_node, links);
+			csa->ccb_h.path->device->refcount--;
+			free(cur_entry, M_CAMXPT);
+		} else {
+			cur_entry->event_enable = csa->event_enable;
+		}
+	} else {
+		cur_entry = malloc(sizeof(*cur_entry), M_CAMXPT,
+				   M_NOWAIT);
+		if (cur_entry == NULL) {
+			splx(s);
+			goto out;
+		}
+		cur_entry->event_enable = csa->event_enable;
+		cur_entry->callback_arg = csa->callback_arg;
+		cur_entry->callback = csa->callback;
+		SLIST_INSERT_HEAD(async_head, cur_entry, links);
+		csa->ccb_h.path->device->refcount++;
+	}
+	mtx_unlock(csa->ccb_h.path->bus->sim->mtx);
+
+	if ((added & AC_FOUND_DEVICE) != 0) {
+		/*
+		 * Get this peripheral up to date with all
+		 * the currently existing devices.
+		 */
+		xpt_for_all_devices(xptsetasyncfunc, cur_entry);
+	}
+	if ((added & AC_PATH_REGISTERED) != 0) {
+		/*
+		 * Get this peripheral up to date with all
+		 * the currently existing busses.
+		 */
+		xpt_for_all_busses(xptsetasyncbusfunc, cur_entry);
+		}
+	splx(s);
+
+out:
+	free(start_ccb->ccb_h.xptpriv, M_CAMXPT);
+	xpt_free_path(start_ccb->ccb_h.path);
+	xpt_free_ccb(start_ccb);
+}
+
 void
 xpt_action(union ccb *start_ccb)
 {
@@ -3414,73 +3495,42 @@
 	}
 	case XPT_SASYNC_CB:
 	{
-		struct ccb_setasync *csa;
-		struct async_node *cur_entry;
-		struct async_list *async_head;
-		u_int32_t added;
-		int s;
+		union ccb *task_ccb;
+		struct task *task;
 
-		csa = &start_ccb->csa;
-		added = csa->event_enable;
-		async_head = &csa->ccb_h.path->device->asyncs;
-
 		/*
-		 * If there is already an entry for us, simply
-		 * update it.
+		 * Need to decouple this operation via a taqskqueue so that
+		 * the locking doesn't become a mess.  Clone the ccb so that
+		 * we own the memory and can free it later.
 		 */
-		s = splcam();
-		cur_entry = SLIST_FIRST(async_head);
-		while (cur_entry != NULL) {
-			if ((cur_entry->callback_arg == csa->callback_arg)
-			 && (cur_entry->callback == csa->callback))
-				break;
-			cur_entry = SLIST_NEXT(cur_entry, links);
+		task_ccb = malloc(sizeof(union ccb), M_CAMXPT, M_NOWAIT);
+		if (task_ccb == NULL) {
+			start_ccb->ccb_h.status = CAM_RESRC_UNAVAIL;
+			break;
+		}
+		bcopy(start_ccb, task_ccb, sizeof(union ccb));
+		if (xpt_create_path(&task_ccb->ccb_h.path, NULL,
+				    start_ccb->ccb_h.path_id,
+				    start_ccb->ccb_h.target_id,
+				    start_ccb->ccb_h.target_lun) !=
+				    CAM_REQ_CMP) {
+			start_ccb->ccb_h.status = CAM_RESRC_UNAVAIL;
+			xpt_free_ccb(task_ccb);
+			break;
 		}
 
-		if (cur_entry != NULL) {
-		 	/*
-			 * If the request has no flags set,
-			 * remove the entry.
-			 */
-			added &= ~cur_entry->event_enable;
-			if (csa->event_enable == 0) {
-				SLIST_REMOVE(async_head, cur_entry,
-					     async_node, links);
-				csa->ccb_h.path->device->refcount--;
-				free(cur_entry, M_CAMXPT);
-			} else {
-				cur_entry->event_enable = csa->event_enable;
-			}
-		} else {
-			cur_entry = malloc(sizeof(*cur_entry), M_CAMXPT,
-					   M_NOWAIT);
-			if (cur_entry == NULL) {
-				splx(s);
-				csa->ccb_h.status = CAM_RESRC_UNAVAIL;
-				break;
-			}
-			cur_entry->event_enable = csa->event_enable;
-			cur_entry->callback_arg = csa->callback_arg;
-			cur_entry->callback = csa->callback;
-			SLIST_INSERT_HEAD(async_head, cur_entry, links);
-			csa->ccb_h.path->device->refcount++;
+		task = malloc(sizeof(struct task), M_CAMXPT, M_NOWAIT);
+		if (task == NULL) {
+			start_ccb->ccb_h.status = CAM_RESRC_UNAVAIL;
+			xpt_free_path(task_ccb->ccb_h.path);
+			xpt_free_ccb(task_ccb);
+			break;
 		}
 
-		if ((added & AC_FOUND_DEVICE) != 0) {
-			/*
-			 * Get this peripheral up to date with all
-			 * the currently existing devices.
-			 */
-			xpt_for_all_devices(xptsetasyncfunc, cur_entry);
-		}
-		if ((added & AC_PATH_REGISTERED) != 0) {
-			/*
-			 * Get this peripheral up to date with all
-			 * the currently existing busses.
-			 */
-			xpt_for_all_busses(xptsetasyncbusfunc, cur_entry);
-		}
-		splx(s);
+		TASK_INIT(task, 0, xpt_action_sasync_cb, task_ccb);
+		task_ccb->ccb_h.xptpriv = task;
+		taskqueue_enqueue(taskqueue_thread, task);
+
 		start_ccb->ccb_h.status = CAM_REQ_CMP;
 		break;
 	}