core and triggers: a trigger should start disabled

If the trigger is enabled on creation, it may fire before the cset is
ready.  This happened to me with "modprobe zio-zero trigger=irq".
For this reason it must start disabled. Also, the create
method MUST set ti->cset before activating the trigger, because the
cset lock is needed to access the enable/disable flag.

The commit adds a WARN() if the field is not set after creation, then
it leaves it zeroed to ensure the system will crash, instead of having
a subtle race.

Also, add a pair of FIXME notes about the need to allow changing
a trigger type while leaving the new one disabled.

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

doc/zio-manual.in

@@ -2118,9 +2118,10 @@ The detailed meaning of the operations is as follows:
 
 	The operations are called when this trigger type is associated to
         (resp. de-associated from) a new cset. @code{create} returns
-        a trigger instance structure, which is usually part of a larger
+        a disabled trigger instance structure, which is usually part of a larger
         structure that the instance itself will recover with the macro
         @code{container_of}. Please look at existing triggers for details
+        (for example, @t{ti->cset} must be set at creation time.
 
 @cindex push_block
 @item push_block
@@ -2402,8 +2403,9 @@ The following locks are defined used in ZIO core and data structures:
 	The cset structure includes a lock that is used to
         serialize access the I/O operations, as well as trigger-related
         events (i.e. the @code{ti->flags} bits like
-        @code{ZIO_TI_ARMED}).
-        I/O itself is serialized by trigger code in the ZIO core:
+        @code{ZIO_TI_ARMED}). For this reason, the field @t{ti->cset}
+        must be immediateky assigned when the trigger instance is created.
+        I/O 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

helpers.c

@@ -121,10 +121,6 @@ void zio_arm_trigger(struct zio_ti *ti)
 {
 	unsigned long flags;
 
-	/* If the trigger runs too early, ti->cset is still NULL */
-	if (!ti->cset)
-		return;
-
 	/* check if trigger is disabled or previous instance is pending */
 	spin_lock_irqsave(&ti->cset->lock, flags);
 	if (unlikely((ti->flags & ZIO_STATUS) == ZIO_DISABLED ||

objects.c

@@ -183,7 +183,7 @@ static void __bi_unregister(struct zio_buffer_type *zbuf, struct zio_bi *bi)
 	zattr_set_remove(&bi->head);
 }
 
-/* create and initialize a new trigger instance */
+/* create and initialize a new disabled trigger instance */
 static struct zio_ti *__ti_create_and_init(struct zio_trigger_type *trig,
 					   struct zio_cset *cset)
 {
@@ -193,13 +193,16 @@ static struct zio_ti *__ti_create_and_init(struct zio_trigger_type *trig,
 	pr_debug("%s\n", __func__);
 	/* Create trigger, ensuring it's not reentrant */
 	spin_lock(&trig->lock);
-	ti = trig->t_op->create(trig, cset, NULL, 0/*FIXME*/);
+	ti = trig->t_op->create(trig, cset, NULL, 0 /* FIXME: fmode_t */);
 	spin_unlock(&trig->lock);
 	if (IS_ERR(ti)) {
 		pr_err("ZIO %s: can't create trigger, error %ld\n",
 		       __func__, PTR_ERR(ti));
 		goto out;
 	}
+	/* This is a new requirement: warn our users */
+	WARN(ti->cset != cset, "Trigger creation should set \"cset\" field\n");
+
 	/* Initialize trigger */
 	spin_lock_init(&ti->lock);
 	ti->t_op = trig->t_op;
@@ -248,8 +251,6 @@ static int __ti_register(struct zio_trigger_type *trig, struct zio_cset *cset,
 	spin_lock(&trig->lock);
 	list_add(&ti->list, &trig->list);
 	spin_unlock(&trig->lock);
-	ti->cset = cset;
-	/* Done. This ti->cset marks everything is running */
 
 	return 0;
 
@@ -283,6 +284,8 @@ int zio_change_current_trigger(struct zio_cset *cset, char *name)
 
 	pr_debug("%s\n", __func__);
 
+	/* FIXME: parse a leading "-" to mean we want it disabled */
+
 	if (unlikely(strcmp(name, trig_old->head.name) == 0))
 		return 0; /* it is the current trigger */
 
@@ -321,6 +324,11 @@ int zio_change_current_trigger(struct zio_cset *cset, char *name)
 	for (i = 0; i < cset->n_chan; ++i)
 		__zattr_trig_init_ctrl(ti, cset->chan[i].current_ctrl);
 
+	/* Enable this new trigger (FIXME: unless the user doesn't want it) */
+	spin_lock_irqsave(&cset->lock, flags);
+	ti->flags &= ~ZIO_DISABLED;
+	spin_unlock_irqrestore(&cset->lock, flags);
+
 	/* Finally, arm it if so needed */
 	if (zio_cset_is_self_timed(cset))
 		zio_arm_trigger(ti);
@@ -624,6 +632,7 @@ static void chan_unregister(struct zio_channel *chan)
 static int cset_register(struct zio_cset *cset, struct zio_cset *cset_t)
 {
 	int i, j, err = 0, size;
+	unsigned long flags;
 	struct zio_channel *chan_tmp;
 	struct zio_ti *ti = NULL;
 
@@ -719,7 +728,11 @@ static int cset_register(struct zio_cset *cset, struct zio_cset *cset_t)
 	list_add(&cset->list_cset, &zstat->list_cset);
 	spin_unlock(&zstat->lock);
 
-	/* All went well. Not auto-arm self-timed peripherals */
+	/* Finally, enable the trigger and arm it if needed */
+	spin_lock_irqsave(&cset->lock, flags);
+	ti->flags &= ~ZIO_DISABLED;
+	spin_unlock_irqrestore(&cset->lock, flags);
+
 	if (zio_cset_is_self_timed(cset))
 		zio_arm_trigger(ti);
 

triggers/zio-trig-hrt.c

@@ -218,6 +218,8 @@ static struct zio_ti *ztt_create(struct zio_trigger_type *trig,
 	if (!ztt)
 		return ERR_PTR(-ENOMEM);
 	ti = &ztt->ti;
+	ti->flags = ZIO_DISABLED;
+	ti->cset = cset;
 
 	/* Fill own fields */
 	hrtimer_init(&ztt->timer, CLOCK_REALTIME, HRTIMER_MODE_ABS);

triggers/zio-trig-irq.c

@@ -101,6 +101,8 @@ static struct zio_ti *zti_create(struct zio_trigger_type *trig,
 	ti = kzalloc(sizeof(*ti), GFP_KERNEL);
 	if (!ti)
 		return ERR_PTR(-ENOMEM);
+	ti->flags = ZIO_DISABLED;
+	ti->cset = cset;
 
 	/* Try all edge settings (gpio stuff prefers edges, but pci wants 0) */
 	for (i = 0; i < ARRAY_SIZE(edges); i++) {

triggers/zio-trig-timer.c

@@ -127,6 +127,8 @@ static struct zio_ti *ztt_create(struct zio_trigger_type *trig,
 	if (!ztt)
 		return ERR_PTR(-ENOMEM);
 	ti = &ztt->ti;
+	ti->flags = ZIO_DISABLED;
+	ti->cset = cset;
 
 	/* Fill own fields */
 	setup_timer(&ztt->timer, ztt_fn,

triggers/zio-trig-user.c

@@ -81,6 +81,8 @@ static struct zio_ti *ztu_create(struct zio_trigger_type *trig,
 	ti = kzalloc(sizeof(*ti), GFP_KERNEL);
 	if (!ti)
 		return ERR_PTR(-ENOMEM);
+	ti->flags = ZIO_DISABLED;
+	ti->cset = cset;
 
 	return ti;
 }