isci: rework timer api

Prepare the timer api for the arrival of dynamic creation and
destruction events from the core.  It pretended to do this previously
but the core to date only used it in a static init-time only fashion.
This is an interim fix until a cleaner event queue can be developed.

1/ make all locking external to the api (add WARN_ONCE to verify)
2/ add a timer_destroy interface (to be used by the core)
3/ use del_timer_sync() prior to deallocating timer data
4/ delete the "timer_list" indirection, we only have timers allocated
   for the isci_host
5/ fix detection of timer list allocation errors

Signed-off-by: Dan Williams <dan.j.williams@intel.com>

drivers/scsi/isci/events.c

@@ -75,35 +75,23 @@
  *    whenever the timer expires.
  * @controller: This parameter specifies the controller with which this timer
  *    is to be associated.
- * @cookie: This parameter specifies a piece of information that the user must
- *    retain.  This cookie is to be supplied by the user anytime a timeout
- *    occurs for the created timer.
+ * @cb_param: opaque callback parameter
  *
  * This method returns a handle to a timer object created by the user.  The
  * handle will be utilized for all further interactions relating to this timer.
  */
-void *isci_event_timer_create(
-	struct scic_sds_controller *controller,
-	void (*timer_callback)(void *),
-	void *cookie)
+void *isci_event_timer_create(struct scic_sds_controller *scic,
+			      void (*timer_callback)(void *),
+			      void *cb_param)
 {
-	struct isci_host *isci_host;
-	struct isci_timer *timer = NULL;
-
-	isci_host = (struct isci_host *)sci_object_get_association(controller);
+	struct isci_host *ihost = sci_object_get_association(scic);
+	struct isci_timer *itimer;
 
-	dev_dbg(&isci_host->pdev->dev,
-		"%s: isci_host = %p",
-		__func__, isci_host);
-
-	timer = isci_timer_create(&isci_host->timer_list_struct,
-				  isci_host,
-				  cookie,
-				  timer_callback);
+	itimer = isci_timer_create(ihost, cb_param, timer_callback);
 
-	dev_dbg(&isci_host->pdev->dev, "%s: timer = %p\n", __func__, timer);
+	dev_dbg(&ihost->pdev->dev, "%s: timer = %p\n", __func__, itimer);
 
-	return (void *)timer;
+	return itimer;
 }
 
 
@@ -146,14 +134,9 @@ void isci_event_timer_start(
  * @timer: This parameter specifies the timer to be stopped.
  *
  */
-void isci_event_timer_stop(
-	struct scic_sds_controller *controller,
-	void *timer)
+void isci_event_timer_stop(struct scic_sds_controller *controller, void *timer)
 {
-	struct isci_host *isci_host;
-
-	isci_host =
-		(struct isci_host *)sci_object_get_association(controller);
+	struct isci_host *isci_host = sci_object_get_association(controller);
 
 	dev_dbg(&isci_host->pdev->dev,
 		"%s: isci_host = %p, timer = %p\n",
@@ -162,6 +145,16 @@ void isci_event_timer_stop(
 	isci_timer_stop((struct isci_timer *)timer);
 }
 
+void isci_event_timer_destroy(struct scic_sds_controller *scic, void *timer)
+{
+        struct isci_host *ihost = sci_object_get_association(scic);
+
+	dev_dbg(&ihost->pdev->dev, "%s: ihost = %p, timer = %p\n",
+			__func__, ihost, timer);
+
+	isci_del_timer(ihost, timer);
+}
+
 /**
  * isci_event_controller_start_complete() - This user callback will inform the
  *    user that the controller has finished the start process. The associated

drivers/scsi/isci/events.h

@@ -111,6 +111,9 @@ void isci_event_timer_stop(
 	struct scic_sds_controller *controller,
 	void *timer);
 
+
+void isci_event_timer_destroy(struct scic_sds_controller *scic, void *timer);
+
 /**
  * isci_event_controller_start_complete() - This user callback will inform the
  *    user that the controller has finished the start process.

drivers/scsi/isci/host.c

@@ -349,9 +349,14 @@ void isci_host_deinit(struct isci_host *ihost)
 	}
 
 	set_bit(IHOST_STOP_PENDING, &ihost->flags);
+
+	spin_lock_irq(&ihost->scic_lock);
 	scic_controller_stop(scic, SCIC_CONTROLLER_STOP_TIMEOUT);
+	spin_unlock_irq(&ihost->scic_lock);
+
 	wait_for_stop(ihost);
 	scic_controller_reset(scic);
+	isci_timer_list_destroy(ihost);
 }
 
 static void __iomem *scu_base(struct isci_host *isci_host)
@@ -370,8 +375,6 @@ static void __iomem *smu_base(struct isci_host *isci_host)
 	return pcim_iomap_table(pdev)[SCI_SMU_BAR * 2] + SCI_SMU_BAR_SIZE * id;
 }
 
-#define SCI_MAX_TIMER_COUNT 25
-
 int isci_host_init(struct isci_host *isci_host)
 {
 	int err = 0;
@@ -382,11 +385,7 @@ int isci_host_init(struct isci_host *isci_host)
 	union scic_oem_parameters scic_oem_params;
 	union scic_user_parameters scic_user_params;
 
-	INIT_LIST_HEAD(&isci_host->timer_list_struct.timers);
-	isci_timer_list_construct(
-		&isci_host->timer_list_struct,
-		SCI_MAX_TIMER_COUNT
-		);
+	isci_timer_list_construct(isci_host);
 
 	controller = scic_controller_alloc(&isci_host->pdev->dev);
 
@@ -473,7 +472,17 @@ int isci_host_init(struct isci_host *isci_host)
 		}
 	}
 
+	tasklet_init(&isci_host->completion_tasklet,
+		     isci_host_completion_routine, (unsigned long)isci_host);
+
+	INIT_LIST_HEAD(&(isci_host->mdl_struct_list));
+
+	INIT_LIST_HEAD(&isci_host->requests_to_complete);
+	INIT_LIST_HEAD(&isci_host->requests_to_abort);
+
+	spin_lock_irq(&isci_host->scic_lock);
 	status = scic_controller_initialize(isci_host->core_controller);
+	spin_unlock_irq(&isci_host->scic_lock);
 	if (status != SCI_SUCCESS) {
 		dev_warn(&isci_host->pdev->dev,
 			 "%s: scic_controller_initialize failed -"
@@ -482,17 +491,8 @@ int isci_host_init(struct isci_host *isci_host)
 		return -ENODEV;
 	}
 
-	tasklet_init(&isci_host->completion_tasklet,
-		     isci_host_completion_routine, (unsigned long)isci_host);
-
-	INIT_LIST_HEAD(&(isci_host->mdl_struct_list));
-
-	INIT_LIST_HEAD(&isci_host->requests_to_complete);
-	INIT_LIST_HEAD(&isci_host->requests_to_abort);
-
 	/* populate mdl with dma memory. scu_mdl_allocate_coherent() */
 	err = isci_host_mdl_allocate_coherent(isci_host);
-
 	if (err)
 		return err;
 

drivers/scsi/isci/host.h

@@ -87,7 +87,7 @@ struct isci_host {
 	union scic_oem_parameters oem_parameters;
 
 	int id; /* unique within a given pci device */
-	struct isci_timer_list timer_list_struct;
+	struct list_head timers;
 	void *core_ctrl_memory;
 	struct dma_pool *dma_pool;
 	unsigned int dma_pool_alloc_size;

drivers/scsi/isci/task.c

@@ -475,14 +475,8 @@ int isci_task_execute_tmf(
 	}
 
 	/* Allocate the TMF timeout timer. */
-	tmf->timeout_timer = isci_timer_create(
-		&isci_host->timer_list_struct,
-		isci_host,
-		request,
-		isci_tmf_timeout_cb
-		);
-
 	spin_lock_irqsave(&isci_host->scic_lock, flags);
+	tmf->timeout_timer = isci_timer_create(isci_host, request, isci_tmf_timeout_cb);
 
 	/* Start the timer. */
 	if (tmf->timeout_timer)
@@ -557,9 +551,7 @@ int isci_task_execute_tmf(
 
 	/* Clean up the timer if needed. */
 	if (tmf->timeout_timer) {
-		isci_timer_stop(tmf->timeout_timer);
-		isci_timer_free(&isci_host->timer_list_struct,
-				tmf->timeout_timer);
+		isci_del_timer(isci_host, tmf->timeout_timer);
 		tmf->timeout_timer = NULL;
 	}
 
@@ -1468,10 +1460,7 @@ void isci_task_request_complete(
 
 	/* Manage the timer if it is still running. */
 	if (tmf->timeout_timer) {
-
-		isci_timer_stop(tmf->timeout_timer);
-		isci_timer_free(&isci_host->timer_list_struct,
-				tmf->timeout_timer);
+		isci_del_timer(isci_host, tmf->timeout_timer);
 		tmf->timeout_timer = NULL;
 	}
 

drivers/scsi/isci/timers.c

@@ -56,96 +56,59 @@
 #include "isci.h"
 #include "timers.h"
 
-
 /**
  * isci_timer_list_construct() - This method contrucst the SCI Timer List
  *    object used by the SCI Module class. The construction process involves
  *    creating isci_timer objects and adding them to the SCI Timer List
  *    object's list member. The number of isci_timer objects is determined by
  *    the timer_list_size parameter.
- * @isci_timer_list: This parameter points to the SCI Timer List object being
- *    constructed. The calling routine is responsible for allocating the memory
- *    for isci_timer_list and initializing the timer list_head member of
- *    isci_timer_list.
- * @timer_list_size: This parameter specifies the number of isci_timer objects
- *    contained by the SCI Timer List. which this timer is to be associated.
+ * @ihost: container of the timer list
  *
  * This method returns an error code indicating sucess or failure. The user
  * should check for possible memory allocation error return otherwise, a zero
  * indicates success.
  */
-int isci_timer_list_construct(
-	struct isci_timer_list *isci_timer_list,
-	int timer_list_size)
+int isci_timer_list_construct(struct isci_host *ihost)
 {
-	struct isci_timer *isci_timer;
-	int i;
-	int err = 0;
-
+	struct isci_timer *itimer;
+	int i, err = 0;
 
-	for (i = 0; i < timer_list_size; i++) {
-
-		isci_timer = kzalloc(sizeof(*isci_timer), GFP_KERNEL);
-
-		if (!isci_timer) {
+	INIT_LIST_HEAD(&ihost->timers);
+	for (i = 0; i < SCI_MAX_TIMER_COUNT; i++) {
+		itimer = devm_kzalloc(&ihost->pdev->dev, sizeof(*itimer), GFP_KERNEL);
 
+		if (!itimer) {
 			err = -ENOMEM;
 			break;
 		}
-		isci_timer->used = 0;
-		isci_timer->stopped = 1;
-		isci_timer->parent = isci_timer_list;
-		list_add(&isci_timer->node, &isci_timer_list->timers);
+		init_timer(&itimer->timer);
+		itimer->used = 0;
+		itimer->stopped = 1;
+		list_add(&itimer->node, &ihost->timers);
 	}
 
-	return 0;
-
+	return err;
 }
 
-
 /**
  * isci_timer_list_destroy() - This method destroys the SCI Timer List object
  *    used by the SCI Module class. The destruction  process involves freeing
  *    memory allocated for isci_timer objects on the SCI Timer List object's
  *    timers list_head member. If any isci_timer objects are mark as "in use",
  *    they are not freed and the function returns an error code of -EBUSY.
- * @isci_timer_list: This parameter points to the SCI Timer List object being
- *    destroyed.
- *
- * This method returns an error code indicating sucess or failure. The user
- * should check for possible -EBUSY error return, in the event of one or more
- * isci_timers still "in use", otherwise, a zero indicates success.
+ * @ihost: container of the list to be destroyed
  */
-int isci_timer_list_destroy(
-	struct isci_timer_list *isci_timer_list)
+void isci_timer_list_destroy(struct isci_host *ihost)
 {
-	struct isci_timer *timer, *tmp;
-
-	list_for_each_entry_safe(timer, tmp, &isci_timer_list->timers, node) {
-		isci_timer_free(isci_timer_list, timer);
-		list_del(&timer->node);
-		kfree(timer);
-	}
-	return 0;
-}
+	struct isci_timer *timer;
+	LIST_HEAD(list);
 
+	spin_lock_irq(&ihost->scic_lock);
+	list_splice_init(&ihost->timers, &list);
+	spin_unlock_irq(&ihost->scic_lock);
 
-
-static void isci_timer_restart(struct isci_timer *isci_timer)
-{
-	struct timer_list *timer =
-		&isci_timer->timer;
-	unsigned long timeout;
-
-	dev_dbg(&isci_timer->isci_host->pdev->dev,
-		"%s: isci_timer = %p\n", __func__, isci_timer);
-
-	isci_timer->restart = 0;
-	isci_timer->stopped = 0;
-	timeout = isci_timer->timeout_value;
-	timeout = (timeout * HZ) / 1000;
-	timeout = timeout ? timeout : 1;
-	mod_timer(timer, jiffies + timeout);
+	list_for_each_entry(timer, &list, node)
+		del_timer_sync(&timer->timer);
 }
 
 /**
@@ -169,7 +132,7 @@ static void isci_timer_restart(struct isci_timer *isci_timer)
 static void timer_function(unsigned long data)
 {
 
-	struct isci_timer *timer     = (struct isci_timer *)data;
+	struct isci_timer *timer = (struct isci_timer *)data;
 	struct isci_host *isci_host = timer->isci_host;
 	unsigned long flags;
 
@@ -185,89 +148,66 @@ static void timer_function(unsigned long data)
 
 	if (!timer->stopped) {
 		timer->stopped = 1;
-		timer->timer_callback(timer->cookie);
-
-		if (timer->restart)
-			isci_timer_restart(timer);
+		timer->timer_callback(timer->cb_param);
 	}
 
 	spin_unlock_irqrestore(&isci_host->scic_lock, flags);
 }
 
 
-struct isci_timer *isci_timer_create(
-	struct isci_timer_list *isci_timer_list,
-	struct isci_host *isci_host,
-	void *cookie,
-	void (*timer_callback)(void *))
+struct isci_timer *isci_timer_create(struct isci_host *ihost, void *cb_param,
+				     void (*timer_callback)(void *))
 {
-
 	struct timer_list *timer;
 	struct isci_timer *isci_timer;
-	struct list_head *timer_list =
-		&isci_timer_list->timers;
-	unsigned long flags;
+	struct list_head *list = &ihost->timers;
 
-	spin_lock_irqsave(&isci_host->scic_lock, flags);
+	WARN_ONCE(!spin_is_locked(&ihost->scic_lock),
+		  "%s: unlocked!\n", __func__);
 
-	if (list_empty(timer_list)) {
-		spin_unlock_irqrestore(&isci_host->scic_lock, flags);
+	if (WARN_ONCE(list_empty(list), "%s: timer pool empty\n", __func__))
 		return NULL;
-	}
 
-	isci_timer = list_entry(timer_list->next, struct isci_timer, node);
+	isci_timer = list_entry(list->next, struct isci_timer, node);
 
-	if (isci_timer->used) {
-		spin_unlock_irqrestore(&isci_host->scic_lock, flags);
-		return NULL;
-	}
 	isci_timer->used = 1;
 	isci_timer->stopped = 1;
-	list_move_tail(&isci_timer->node, timer_list);
-
-	spin_unlock_irqrestore(&isci_host->scic_lock, flags);
+	/* FIXME: what!? we recycle the timer, rather than take it off
+	 * the free list?
+	 */
+	list_move_tail(&isci_timer->node, list);
 
 	timer = &isci_timer->timer;
 	timer->data = (unsigned long)isci_timer;
 	timer->function = timer_function;
-	isci_timer->cookie = cookie;
+	isci_timer->cb_param = cb_param;
 	isci_timer->timer_callback = timer_callback;
-	isci_timer->isci_host = isci_host;
+	isci_timer->isci_host = ihost;
 
-	dev_dbg(&isci_host->pdev->dev,
+	dev_dbg(&ihost->pdev->dev,
 		"%s: isci_timer = %p\n", __func__, isci_timer);
 
 	return isci_timer;
 }
 
-/**
- * isci_timer_free() - This method frees the isci_timer, marking it "free to
+/* isci_del_timer() - This method frees the isci_timer, marking it "free to
  *    use", then places its back at the head of the timers list for the SCI
  *    Timer List object specified.
- * @isci_timer_list: This parameter points to the SCI Timer List from which the
- *    timer is reserved.
- * @isci_timer: This parameter specifies the timer to be freed.
- *
  */
-void isci_timer_free(
-	struct isci_timer_list *isci_timer_list,
-	struct isci_timer *isci_timer)
+void isci_del_timer(struct isci_host *ihost, struct isci_timer *isci_timer)
 {
-	struct list_head *timer_list = &isci_timer_list->timers;
+	struct list_head *list = &ihost->timers;
+
+	WARN_ONCE(!spin_is_locked(&ihost->scic_lock),
+		  "%s unlocked!\n", __func__);
 
 	dev_dbg(&isci_timer->isci_host->pdev->dev,
 		"%s: isci_timer = %p\n", __func__, isci_timer);
 
-	if (list_empty(timer_list))
-		return;
-
 	isci_timer->used = 0;
-	list_move(&isci_timer->node, timer_list);
-
-	if (!isci_timer->stopped) {
-		del_timer(&isci_timer->timer);
-		isci_timer->stopped = 1;
-	}
+	list_move(&isci_timer->node, list);
+	del_timer(&isci_timer->timer);
+	isci_timer->stopped = 1;
 }
 
 /**
@@ -278,26 +218,15 @@ void isci_timer_free(
  *    the associated callback function will be called.
  *
  */
-void isci_timer_start(
-	struct isci_timer *isci_timer,
-	unsigned long timeout)
+void isci_timer_start(struct isci_timer *isci_timer, unsigned long tmo)
 {
 	struct timer_list *timer = &isci_timer->timer;
 
 	dev_dbg(&isci_timer->isci_host->pdev->dev,
 		"%s: isci_timer = %p\n", __func__, isci_timer);
 
-	isci_timer->timeout_value = timeout;
-	init_timer(timer);
-	timeout = (timeout * HZ) / 1000;
-	timeout = timeout ? timeout : 1;
-
-	timer->expires = jiffies + timeout;
-	timer->data = (unsigned long)isci_timer;
-	timer->function = timer_function;
 	isci_timer->stopped = 0;
-	isci_timer->restart = 0;
-	add_timer(timer);
+	mod_timer(timer, jiffies + msecs_to_jiffies(tmo));
 }
 
 /**
@@ -310,10 +239,6 @@ void isci_timer_stop(struct isci_timer *isci_timer)
 	dev_dbg(&isci_timer->isci_host->pdev->dev,
 		"%s: isci_timer = %p\n", __func__, isci_timer);
 
-	if (isci_timer->stopped)
-		return;
-
 	isci_timer->stopped = 1;
-
 	del_timer(&isci_timer->timer);
 }

drivers/scsi/isci/timers.h

@@ -59,68 +59,30 @@
 #include <linux/timer.h>
 #include <linux/types.h>
 
-/***************************************************
- * isci_timer
- ***************************************************
- */
-/**
- * struct isci_timer_list - This class is the container for all isci_timer
- *    objects
- *
- *
- */
-struct isci_timer_list {
-
-	struct list_head timers;
-};
+#define SCI_MAX_TIMER_COUNT 25
 
 /**
  * struct isci_timer - This class represents the timer object used by SCIC. It
- *    wraps the Linux timer_list object.
- *
+ *    wraps the Linux timer_list object, and (TODO) should be removed in favor
+ *    of a delayed-workqueue style interface with simpler locking
  *
  */
 struct isci_timer {
 	int used;
 	int stopped;
-	int restart;
-	int timeout_value;
-	void *cookie;
+	void *cb_param;
 	void (*timer_callback)(void *);
 	struct list_head node;
 	struct timer_list timer;
-	struct isci_timer_list *parent;
 	struct isci_host *isci_host;
 };
 
-#define to_isci_timer(t)	\
-	container_of(t, struct isci_timer, timer);
-
-int isci_timer_list_construct(
-	struct isci_timer_list *isci_timer_list,
-	int timer_list_size);
-
-
-int isci_timer_list_destroy(
-	struct isci_timer_list *isci_timer_list);
-
-struct isci_timer *isci_timer_create(
-	struct isci_timer_list *isci_timer_list,
-	struct isci_host *isci_host,
-	void *cookie,
-	void (*timer_callback)(void *));
-
-void isci_timer_free(
-	struct isci_timer_list *isci_timer_list,
-	struct isci_timer *isci_timer);
-
-void isci_timer_start(
-	struct isci_timer *isci_timer,
-	unsigned long timeout);
-
-void isci_timer_stop(
-	struct isci_timer *isci_timer);
-
+int isci_timer_list_construct(struct isci_host *ihost);
+void isci_timer_list_destroy(struct isci_host *ihost);
+struct isci_timer *isci_timer_create(struct isci_host *ihost, void *cb_param,
+				     void (*timer_callback)(void *));
+void isci_del_timer(struct isci_host *ihost, struct isci_timer *itimer);
+void isci_timer_start(struct isci_timer *isci_timer, unsigned long timeout);
+void isci_timer_stop(struct isci_timer *isci_timer);
 
 #endif /* !defined (_SCI_TIMER_H_) */
-