core: change some internals for triggers

This is the first step of a series of changes aimed at a cleaner and
more flexible management of triggers, as well as a really transparent
"user" trigger.

This commit does the following:

- zio_fire_trigger renamed to zio_arm_trigger: the trigger is
actually armed by software, and then it fires by hardware. This
distinction is especially important for the transparent trigger: when
devices have internal timing, the software trigger must arm it
immediately (as opposed to the dumb devices, where the software trigger
really causes I/O to happen).

- ZIO_TI_BUSY renamed to ZIO_TI_ARMED. Also, ZIO_TI_COMPLETING is
removed. The new, simplified policy is like this: the trigger is armed
by software (the trigger module), but completion is driven by hardware
(the device module).  There is no need for a COMPLETING flag, because
the ARMED flag is only cleared after data_done is over for all channels
in the cset.

- the cset spinlock is used to protect all changes of the ti flags.

- change_current_trigger completely revised and fixed to match new
conventions.

- trigger->abort now takes "ti" instead of "cset" as argument; it is
more natural do do so, and no current trigger implements abort so no
harm is done.

- the trigger->abort and trigger->change_status methods are now
always called while holding the cset spinlock. ZIO core calls abort
when changing the current trigger type, to ensure no pending blocks
are there.

- zio_trigger_abort is renamed to zio_trigger_abort_disable, with
an additional argument to state whether the trigger must be atomically
disabled after the abort is over. This avoids a race condition on
trigger removal. It returns the previous "disabled" bit, to be used
when changing a buffer type while preserving trigger status.

- minor unrelated improvements in error management in objects.c.

- documentation update to match the new locking, and a few typos fixed.

Signed-off-by: Alessandro Rubini <rubini@gnudd.com>
Acked-by: Federico Vaga <federico.vaga@gmail.com>

doc/zio-manual.in

@@ -34,7 +34,7 @@
 @comment %**end of header
 
 @setchapternewpage off
-@set update-month November 2012
+@set update-month January 2013
 @finalout
 @c #################################################################
 @titlepage
@@ -481,7 +481,8 @@ knows the trigger has fired, in the latter case ZIO knows the trigger
 is still armed for this cset.
 
 @item At some time in the future, the device that reported @t{-EAGAIN}
-is done with the input operation. It thus calls @i{ti->data_done}.
+is done with the input operation. It thus calls @t{zio_data_done()}
+(that may rely on @i{ti->data_done}).
 When this happens (or if @t{raw_io} completed synchronously), the
 respective trigger changes its status to @i{idle}; the active block
 for each channel in the cset is stored to the respective buffer
@@ -537,9 +538,10 @@ or ti can perform I/O asynchronously and return @code{-EAGAIN}.
 In the latter case the trigger remains in @i{armed} state.
 
 @item At some time in the future, the device that reported @t{-EAGAIN}
-is done with hardware output, and calls @i{ti->data_done}.
+is done with hardware output, and calls  @t{zio_data_done()}
+(that may rely on @i{ti->data_done}).
 When this happens (or if @t{raw_io} completed synchronously), the
-trigger instance bcomes @i{idle} again, and it frees the blocks
+trigger instance becomes @i{idle} again, and it frees the blocks
 for the cset by calling @t{bi->free_block}.  In general,
 the trigger will then call @t{bi->retr_block} in order to be
 ready to re-arm the trigger.
@@ -1727,7 +1729,7 @@ are:
 @end table
 
 @c --------------------------------------------------------------------------
-@node The Cset Struvture
+@node The Cset Structure
 @subsection The Cset Structure
 
 @cindex cset
@@ -1737,7 +1739,7 @@ characteristics.  This object is the most important in the ZIO device
 hierarchy because all data transfers are cset-wide. Each cset includes
 a pointer to the current trigger and buffer types, because the current
 types are two cset-wide attributes. (We chose to set the preferred type
-at device level because this fits most use cases, whle avoiding
+at device level because this fits most use cases, while avoiding
 the need to repeat the default for each cset).
 
 @tindex zio_cset
@@ -1753,7 +1755,7 @@ or used by the developer are:
         to be performed when the internal trigger fires. If the
         function returns 0, the input or output operation is already
         completed. A return value of @code{-EAGAIN} means that
-        cset code will call @code{trigger->data_done} at a later time.
+        cset code will call @code{zio_data_done()} at a later time.
         Other return values are used to report real errors.
 
 @cindex sample size
@@ -2126,13 +2128,17 @@ The detailed meaning of the operations is as follows:
 @cindex data_done
 @item data_done
 
-	This method is called by the device. I/O in the device is
+	This method, if defined, is called by the device by means of the helper
+        @t{zio_data_done()}. I/O in the device is
         almost always asynchronous, so when asked to transfer a cset,
-        the device will prepare to do it, and will call @code{data_done}
-        later in the current trigger instance. For output csets,
-        @code{data_done} frees the blocks and prepares new blocks
-        for the next trigger event; for input, @code{data_done} pushes
-        material to the buffers.
+        the device will prepare to do it, and will call @code{zio_data_done}
+        later. For output csets,
+        @code{zio_data_done} frees the blocks and prepares new blocks
+        for the next trigger event; for input, @code{zio_data_done} pushes
+        material to the buffers.  This method, if present, is called
+        while holding the @i{cset} spin lock, and the trigger still
+        in @t{ARMED} state (@t{zio_data_done} clears the flag only
+        when everything is over with the current trigger event).
 
 @cindex config for triggers
 @item config
@@ -2142,11 +2148,24 @@ The detailed meaning of the operations is as follows:
         structure, this callback allows the trigger to reconfigure itself.
 @c FIXME: use the config trigger method
 
-@item change_status
-@itemx abort
+@item abort
+	Abort, if defined, is called when an already-armed trigger event
+        must be aborted. This happens, for example, because event
+        parameters changed (e.g., the block size).  The method must
+        dispose the @i{active_block} for each channel, and set the
+        pointer to NULL. The trigger may perform @i{data_done} for
+        partially-filled block. The method
+        is called while holding the cset lock and cannot fail nor sleep.
+
+@c  The trigger will usually call the @i{stop_io}
+@c        device method and optionally may perform @i{data_done} for
+@c        partially-filled  blocks. The method
+@c        is called while holding the cset lock and cannot fail nor sleep
 
-	To be documented.
-@c FIXME: document change_status and abort for triggers
+@item change_status
+        The method, if defined, is called when the trigger is enabled or
+        disabled. On disable, ZIO calls @i{abort} beforehand.
+        The method is called while hoding the cset lock.
 
 @end table
 
@@ -2366,9 +2385,12 @@ The following locks are defined used in ZIO core and data structures:
 @c FIXME: trigger-related  flags and locks
 	The cset structure includes a lock that is used to
         serialize access to the @code{ti->flags} bits (e.g.:
-        @code{ZTI_BUSY}, that signals that a trigger is pending).
-        I/O itself is serialized by trigger code: only one trigger event
-        can be pending for each cset, using the BUSY flag.
+        @code{ZIO_TI_ARMED}, that signals that a trigger is pending).
+        I/O itself is serialized by trigger code in the ZIO core:
+        only one trigger event
+        can be pending for each cset, using the ARMED flag. The cset
+        itself, then,  can use this same lock to serialize other activities
+        and prevent a trigger to be armed during such activities.
 
 @item zio_buffer_type->lock
 @itemx zio_trigger_type->lock
@@ -2389,7 +2411,7 @@ The following locks are defined used in ZIO core and data structures:
 @end table
 
 From the table above, it's clear how the device lock, used for
-configuration, is the most used ZIO spinlock.  Even though
+configuration, is the ZIO spinlock with the widest scope.  Even though
 device, buffer and trigger are registered as different objects, they
 live on the same peripheral device. Thus, you most often need to
 serialize configuration on the device as a whole, because

helpers.c

@@ -16,6 +16,7 @@
 #include <linux/zio-trigger.h>
 #include "zio-internal.h"
 
+/* The trigger is armed and the cset spin lock is taken */
 static void __zio_internal_data_done(struct zio_cset *cset)
 {
 	struct zio_buffer_type *zbuf;
@@ -63,25 +64,21 @@ static void __zio_internal_data_done(struct zio_cset *cset)
 /*
  * zio_trigger_data_done
  * This is a ZIO helper to invoke the data_done trigger operation when a data
- * transfer is over and we need to complete the operation.
- * It is useless check for pending ZIO_TI_COMPLETING because only one fire at
- * time is allowed so they cannot exist concurrent completation.
+ * transfer is over and we need to complete the operation. The trigger
+ * is in "ARMED" state when this is called, and is not any more when
+ * the function returns. Please note that  we keep the cset lock
+ * for the duration of the whole function, which must be atomic
  */
 void zio_trigger_data_done(struct zio_cset *cset)
 {
 	spin_lock(&cset->lock);
-	cset->ti->flags |= ZIO_TI_COMPLETING; /* transfer is completing*/
-	spin_unlock(&cset->lock);
 
-	/* Call the data_done function */
 	if (cset->ti->t_op->data_done)
 		cset->ti->t_op->data_done(cset);
 	else
 		__zio_internal_data_done(cset);
 
-	/* transfer is over, resetting completing and busy flags */
-	spin_lock(&cset->lock);
-	cset->ti->flags &= (~(ZIO_TI_COMPLETING | ZIO_TI_BUSY));
+	cset->ti->flags &= ~ZIO_TI_ARMED;
 	spin_unlock(&cset->lock);
 }
 EXPORT_SYMBOL(zio_trigger_data_done);
@@ -102,31 +99,37 @@ static void __zio_internal_abort_free(struct zio_cset *cset)
 /*
  * zio_trigger_abort
  * This is a ZIO helper to invoke the abort function. This must be used when
- * something is going wrong during the acquisition.
+ * something is going wrong during the acquisition or an armed trigger
+ * must be modified. If so requested, the trigger is disabled too.
+ * The function returns the previous value of the disabled flags.
  */
-void zio_trigger_abort(struct zio_cset *cset)
+int zio_trigger_abort_disable(struct zio_cset *cset, int disable)
 {
 	struct zio_ti *ti = cset->ti;
+	int ret;
 
 	/*
-	 * If trigger is running (ZIO_TI_BUSY) but it is not
-	 * completing the transfer (ZIO_TI_COMPLETING), then abort it.
-	 * If the trigger is completing its run, don't abort it because
-	 * it finished and the blocks are full of data.
+	 * If the trigger is running (ZIO_TI_ARMED), then abort it.
+	 * Since the whole data_done procedure happens in locked context,
+	 * there is no concurrency with an already-completing trigger event.
 	 */
 	spin_lock(&cset->lock);
-	if ((ti->flags & ZIO_TI_BUSY) && !(ti->flags & ZIO_TI_COMPLETING)) {
+	if (ti->flags & ZIO_TI_ARMED) {
 		if (ti->t_op->abort)
-			ti->t_op->abort(cset);
+			ti->t_op->abort(ti);
 		else
 			__zio_internal_abort_free(cset);
-		ti->flags &= (~ZIO_TI_BUSY); /* when disabled is not busy */
+		ti->flags &= (~ZIO_TI_ARMED);
 	}
+	ret = ti->flags &= ZIO_STATUS;
+	if (disable)
+		ti->flags |= ZIO_DISABLED;
 	spin_unlock(&cset->lock);
+	return ret;
 }
-EXPORT_SYMBOL(zio_trigger_abort);
+EXPORT_SYMBOL(zio_trigger_abort_disable);
 
-static void __zio_fire_input_trigger(struct zio_ti *ti)
+static void __zio_arm_input_trigger(struct zio_ti *ti)
 {
 	struct zio_buffer_type *zbuf;
 	struct zio_block *block;
@@ -160,7 +163,7 @@ static void __zio_fire_input_trigger(struct zio_ti *ti)
 	}
 }
 
-static void __zio_fire_output_trigger(struct zio_ti *ti)
+static void __zio_arm_output_trigger(struct zio_ti *ti)
 {
 	struct zio_cset *cset = ti->cset;
 
@@ -174,25 +177,29 @@ static void __zio_fire_output_trigger(struct zio_ti *ti)
 }
 
 /*
- * When a software trigger fires, it should call this function. Hw ones don't
+ * When a software trigger fires, it should call this function. It
+ * used to be called zio_fire_trigger, but actually it only arms the trigger.
+ * When hardware is self-timed, the actual trigger fires later.
  */
-void zio_fire_trigger(struct zio_ti *ti)
+void zio_arm_trigger(struct zio_ti *ti)
 {
 	/* If the trigger runs too early, ti->cset is still NULL */
 	if (!ti->cset)
 		return;
 
-	/* check if trigger is disabled or previous fire is still running */
+	/* check if trigger is disabled or previous instance is pending */
+	spin_lock(&ti->cset->lock);
 	if (unlikely((ti->flags & ZIO_STATUS) == ZIO_DISABLED ||
-			(ti->flags & ZIO_TI_BUSY)))
+		     (ti->flags & ZIO_TI_ARMED))) {
+		spin_unlock(&ti->cset->lock);
 		return;
-	spin_lock(&ti->cset->lock);
-	ti->flags |= ZIO_TI_BUSY;
+	}
+	ti->flags |= ZIO_TI_ARMED;
 	spin_unlock(&ti->cset->lock);
 
 	if (likely((ti->flags & ZIO_DIR) == ZIO_DIR_INPUT))
-		__zio_fire_input_trigger(ti);
+		__zio_arm_input_trigger(ti);
 	else
-		__zio_fire_output_trigger(ti);
+		__zio_arm_output_trigger(ti);
 }
-EXPORT_SYMBOL(zio_fire_trigger);
+EXPORT_SYMBOL(zio_arm_trigger);

include/linux/zio-trigger.h

@@ -46,12 +46,11 @@ struct zio_ti {
 
 /* first 4bit are reserved for zio object universal flags */
 enum zio_ti_flag_mask {
-	ZIO_TI_BUSY = 0x10,	/* trigger fire and transfer occurs */
-	ZIO_TI_COMPLETING = 0x20	/* trigger is clompleting transfert */
+	ZIO_TI_ARMED = 0x10,		/* trigger is armed, device rules */
 };
 
 #define to_zio_ti(obj) container_of(obj, struct zio_ti, head.dev)
-void zio_fire_trigger(struct zio_ti *ti);
+void zio_arm_trigger(struct zio_ti *ti);
 
 /*
  * When a buffer has a complete block of data, it can send it to the trigger
@@ -103,9 +102,10 @@ struct zio_trigger_operations {
 	void			(*destroy)(struct zio_ti *ti);
 	void			(*change_status)(struct zio_ti *ti,
 						 unsigned int status);
-	void			(*abort)(struct zio_cset *cset);
+	void			(*abort)(struct zio_ti *ti);
 };
 
-void zio_trigger_abort(struct zio_cset *cset);
+void zio_trigger_data_done(struct zio_cset *cset);
+int zio_trigger_abort_disable(struct zio_cset *cset, int disable);
 
 #endif /* __ZIO_TRIGGER_H__ */

include/linux/zio.h

@@ -285,9 +285,6 @@ static inline unsigned int zio_get_n_chan_enabled(struct zio_cset *cset) {
 	char *_name; \
 	module_param_named(buffer, _name, charp, 0444)
 
-void zio_trigger_data_done(struct zio_cset *cset);
-void zio_trigger_abort(struct zio_cset *cset);
-
 /*
  * Misc library-like code, from zio-misc.c
  */

objects.c

@@ -60,6 +60,8 @@ static struct zio_buffer_type *zio_buffer_get(struct zio_cset *cset,
 
 	if (!name)
 		return ERR_PTR(-EINVAL);
+	if (unlikely(strlen(name) > ZIO_OBJ_NAME_LEN))
+		return ERR_PTR(-EINVAL); /* name too long */
 
 	list_item = __zio_object_get(cset, &zstat->all_buffer_types, name);
 	if (!list_item)
@@ -78,6 +80,8 @@ static struct zio_trigger_type *zio_trigger_get(struct zio_cset *cset,
 
 	if (!name)
 		return ERR_PTR(-EINVAL);
+	if (unlikely(strlen(name) > ZIO_OBJ_NAME_LEN))
+		return ERR_PTR(-EINVAL); /* name too long */
 
 	list_item = __zio_object_get(cset, &zstat->all_trigger_types, name);
 	if (!list_item)
@@ -254,9 +258,12 @@ out_reg:
 out:
 	return err;
 }
+
+/* The trigger must not be armed when calling this helper */
 static void __ti_unregister(struct zio_trigger_type *trig, struct zio_ti *ti)
 {
 	pr_debug("%s\n", __func__);
+
 	/* Remove from trigger instance list */
 	spin_lock(&trig->lock);
 	list_del(&ti->list);
@@ -266,65 +273,57 @@ static void __ti_unregister(struct zio_trigger_type *trig, struct zio_ti *ti)
 	zattr_set_remove(&ti->head);
 }
 
-
+/* This is only called in process context (through a sysfs operation) */
 int zio_change_current_trigger(struct zio_cset *cset, char *name)
 {
 	struct zio_trigger_type *trig, *trig_old = cset->trig;
-	struct zio_channel *chan;
 	struct zio_ti *ti, *ti_old = cset->ti;
 	int err, i;
 
 	pr_debug("%s\n", __func__);
-	spin_lock(&cset->lock);
-	if (ti_old->flags & ZIO_TI_BUSY) {
-		spin_unlock(&cset->lock);
-		return -EBUSY;
-	}
-	/* Set ti BUSY, so it cannot fire */
-	ti_old->flags |= ZIO_TI_BUSY;
-	spin_unlock(&cset->lock);
 
-	if (strlen(name) > ZIO_OBJ_NAME_LEN)
-		return -EINVAL; /* name too long */
 	if (unlikely(strcmp(name, trig_old->head.name) == 0))
-		return 0; /* is the current trigger */
+		return 0; /* it is the current trigger */
 
-	/* get the new trigger */
 	trig = zio_trigger_get(cset, name);
 	if (IS_ERR(trig))
 		return PTR_ERR(trig);
+
 	/* Create and register the new trigger instance */
 	ti = __ti_create_and_init(trig, cset);
 	if (IS_ERR(ti)) {
 		err = PTR_ERR(ti);
-		goto out;
+		goto out_put;
 	}
 	err = __ti_register(trig, cset, ti, "trigger-tmp");
 	if (err)
-		goto out_reg;
-	/* New ti successful created, remove the old ti */
+		goto out_destroy;
+
+	/* Ok, we are done. Kill the current trigger to replace it*/
+	zio_trigger_abort_disable(cset, 1);
+	ti_old->cset = NULL;
 	__ti_unregister(trig_old, ti_old);
 	__ti_destroy(trig_old, ti_old);
 	zio_trigger_put(trig_old, cset->zdev->owner);
-	/* Set new trigger*/
-	mb();
+
+	/* Set new trigger and rename "trigger-tmp" to "trigger" */
+	spin_lock(&cset->lock);
 	cset->trig = trig;
 	cset->ti = ti;
-	/* Rename trigger-tmp to trigger */
 	err = device_rename(&ti->head.dev, "trigger");
-	if (err)
-		WARN(1, "%s: cannot rename trigger folder for"
-			" cset%d\n", __func__, cset->index);
-	/* Update channel current controls */
-	for (i = 0; i < cset->n_chan; ++i) {
-		chan = &cset->chan[i];
-		__zattr_trig_init_ctrl(ti, chan->current_ctrl);
-	}
+	spin_unlock(&cset->lock);
+
+	WARN(err, "%s: cannot rename trigger folder for"
+	     " cset%d\n", __func__, cset->index);
+
+	/* Update current control for each channel */
+	for (i = 0; i < cset->n_chan; ++i)
+		__zattr_trig_init_ctrl(ti, cset->chan[i].current_ctrl);
 	return 0;
 
-out_reg:
+out_destroy:
 	__ti_destroy(trig, ti);
-out:
+out_put:
 	zio_trigger_put(trig, cset->zdev->owner);
 	return err;
 }
@@ -336,11 +335,8 @@ int zio_change_current_buffer(struct zio_cset *cset, char *name)
 	int i, j, err;
 
 	pr_debug("%s\n", __func__);
-	if (strlen(name) > ZIO_OBJ_NAME_LEN)
-		return -EINVAL; /* name too long */
 	if (unlikely(strcmp(name, cset->zbuf->head.name) == 0))
-		return 0; /* is the current buffer */
-
+		return 0; /* it is the current buffer */
 	zbuf = zio_buffer_get(cset, name);
 	if (IS_ERR(zbuf))
 		return PTR_ERR(zbuf);
@@ -758,6 +754,8 @@ static void cset_unregister(struct zio_cset *cset)
 	spin_lock(&zstat->lock);
 	list_del(&cset->list_cset);
 	spin_unlock(&zstat->lock);
+	/* Make it idle */
+	zio_trigger_abort_disable(cset, 1);
 	/* Private exit function */
 	if (cset->exit)
 		cset->exit(cset);

sysfs.c

@@ -364,10 +364,12 @@ static void __zobj_enable(struct device *dev, unsigned int enable)
 		pr_debug("%s: zti\n", __func__);
 
 		ti = to_zio_ti(dev);
-		zio_trigger_abort(ti->cset);
+		spin_lock(&ti->cset->lock);
+		zio_trigger_abort_disable(ti->cset, 0);
 		/* trigger instance callback */
 		if (ti->t_op->change_status)
 			ti->t_op->change_status(ti, status);
+		spin_unlock(&ti->cset->lock);
 		break;
 	/* following objects can't be enabled/disabled */
 	case ZIO_BUF:

triggers/zio-trig-hrt.c

@@ -150,7 +150,7 @@ static enum hrtimer_restart ztt_fn(struct hrtimer *timer)
 	/* FIXME: fill the trigger attributes too */
 
 	ztt = to_ztt_instance(ti);
-	zio_fire_trigger(ti);
+	zio_arm_trigger(ti);
 
 	if (ztt->period) {
 		hrtimer_add_expires_ns(&ztt->timer, ztt->period);

triggers/zio-trig-irq.c

@@ -56,7 +56,7 @@ static irqreturn_t zti_handler(int irq, void *dev_id)
 
 	/* When a trigger fires, we must prepare our control and timestamp */
 	getnstimeofday(&ti->tstamp);
-	zio_fire_trigger(ti);
+	zio_arm_trigger(ti);
 	return IRQ_HANDLED;
 }
 

triggers/zio-trig-timer.c

@@ -78,7 +78,7 @@ static void ztt_fn(unsigned long arg)
 	getnstimeofday(&ti->tstamp);
 
 	ztt = to_ztt_instance(ti);
-	zio_fire_trigger(ti);
+	zio_arm_trigger(ti);
 
 	if (!ztt->period)
 		return; /* one-shot */
@@ -156,10 +156,10 @@ static void ztt_change_status(struct zio_ti *ti, unsigned int status)
 	if (!status) {	/* enable */
 		ztt_start_timer(ztt, ztt->period);
 	} else {	/* disable */
-		/* FIXME kernel/timer.c don't use this is lock*/
-		del_timer_sync(&ztt->timer);
+		del_timer(&ztt->timer);
 	}
 }
+
 static const struct zio_trigger_operations ztt_trigger_ops = {
 	.push_block = ztt_push_block,
 	.pull_block = NULL,

triggers/zio-trig-user.c

@@ -46,7 +46,7 @@ static int ztu_push_block(struct zio_ti *ti, struct zio_channel *chan,
 		return -EBUSY;
 	chan->active_block = block;
 	getnstimeofday(&ti->tstamp);
-	zio_fire_trigger(ti);
+	zio_arm_trigger(ti);
 	return 0;
 }
 
@@ -55,7 +55,7 @@ static void ztu_pull_block(struct zio_ti *ti, struct zio_channel *chan)
 {
 	pr_debug("%s:%d\n", __func__, __LINE__);
 	getnstimeofday(&ti->tstamp);
-	zio_fire_trigger(ti);
+	zio_arm_trigger(ti);
 }
 
 static int ztu_config(struct zio_ti *ti, struct zio_control *ctrl)