[PATCH 2/4] speakup: Do not hold spk_mutex across catch_up()

[Hongyu Xie]

speakup_thread() holds spk_mutex while calling synth->catch_up().
Apart from the indefinite retry loops fixed by the previous patch, a
synth that digests its buffer slowly (e.g. a real speech synthesizer
receiving a large chunk of text) keeps the mutex held for the whole
drain, so loading, unloading or switching any synth module blocks
until all pending speech has been output, which may take arbitrarily
long.

Take spk_mutex off the catch_up() call path and keep the synth alive
for the duration of the call instead:

- speakup_thread() raises a catch-up count and releases spk_mutex
  before calling catch_up(), re-taking it afterwards.

- synth_release() first clears synth->alive (which makes catch_up()
  return) and wakes the thread out of its sleep, then waits for the
  catch-up count to drop to zero before releasing the synth resources
  (sysfs attributes, variables, tty).  The synth buffer is cleared
  there as well, so a switch discards the text that was still pending
  for the old synth; previously a switch would first wait for that
  text to be output by the old synth, which is the unbounded wait
  this patch removes.  As synth_remove() only returns after the
  drain, a synth module cannot be unloaded while catch_up() still
  runs, so the synth structure and its tty stay valid for the whole
  call without any module reference counting.

The count/wait handoff between the thread and the release looks
like this (without it, the resources could be freed while
catch_up() still runs):

    CPU0 (speakup_thread)          CPU1 (synth_release)
    -------------------            -------------------
    atomic_inc(&synth_catch_up_count)
    mutex_unlock(&spk_mutex)
    synth->catch_up(synth)
      /* reads synth->alive,       spin_lock(&speakup_info.spinlock)
         synth->io_ops, the tty */  synth->alive = 0
                                   spin_unlock(...)
                                   wake the thread out of its sleep
                                   wait_event(!synth_catch_up_count)
                                   /* resources stay valid: sysfs,
                                      vars, tty are torn down only
                                      after the count hits zero */
      /* returns once ->alive
         is 0 (loop exit of the
         previous patch) */
    atomic_dec_and_test(...)
      wake_up(&synth_catch_up_waitq)
                                   sysfs_remove_group(...)
                                   speakup_unregister_var(...)
                                   synth->release(synth)
                                   /* patch 4 clears synth itself
                                      under the spinlock */

The SPEECH_KILL key handler can revive the synth at any time: it
runs under speakup_info.spinlock (speakup_key() holds it across
do_spkup()), and the is_alive callbacks set synth->alive back to 1
and queue the init string again.  If that lands after
synth_release() cleared synth->alive, an in-flight catch_up()
draining to a stuck synth never sees ->alive cleared, and
synth_release() blocks forever on the catch-up count:

    CPU0 (SPEECH_KILL key)         CPU1 (synth_release)
    ----------------------         -------------------
                                   spin_lock(&speakup_info.spinlock)
                                   synth->alive = 0
                                   spin_unlock(...)
    spk_synth_is_alive_restart():
      /* under the same spinlock
         via speakup_key() */
      synth->alive = 1  /* revival */
      /* catch_up() rechecks         wait_event(!synth_catch_up_count)
         ->alive, sees 1 again */    /* never: the revival keeps the
                                         loop running */

Mark the synth with ->releasing under speakup_info.spinlock while it
is being released, and have both spk_synth_is_alive_restart() and
spk_synth_is_alive_nop() skip the revival then, so the drain always
terminates:

    CPU0 (SPEECH_KILL key)         CPU1 (synth_release)
    ----------------------         -------------------
                                   spin_lock(&speakup_info.spinlock)
                                   synth->alive = 0
                                   synth->releasing = true
                                   spin_unlock(...)
    spk_synth_is_alive_restart():
      if (synth->releasing)
        return 0  /* no revival */  wait_event(!synth_catch_up_count)
      /* the catch_up() loop          /* terminates: the loop
         exits on ->alive == 0           observes ->alive == 0 */

The other wake_up_process(speakup_task) sites (spk_do_flush(),
do_synth_init()) get the same guard: the speakup keyboard notifier is
registered before the kthread is created, so a speakup key press or a
sysfs write in that window would otherwise pass NULL (or the
kthread_create() error) to wake_up_process():

    CPU0 (speakup key / sysfs)     CPU1 (module exit)
    --------------------------     -----------------
    spk_do_flush():
      wake_up_process(
        speakup_task)              speakup_exit():
      /* speakup_task is NULL         ... kthread_stop(speakup_task)
         before kthread_create(),        /* task_struct freed */
         ERR_PTR after a failure */   speakup_task = NULL;
      /* wakeup on NULL / ERR_PTR,
         or on freed memory */

The wakeup sites take the task pointer under rcu_read_lock(), so
the task_struct cannot be freed underneath them by a concurrent
module exit either:

    spk_do_flush():
      rcu_read_lock();
      task = READ_ONCE(speakup_task);
      if (!IS_ERR_OR_NULL(task))
        wake_up_process(task);
      rcu_read_unlock();
      /* the exit path unregisters the notifiers first, and
         atomic_notifier_chain_unregister() synchronizes with
         the RCU readers, so by the time speakup_task is
         cleared no wakeup site is still running */

After this change spk_mutex no longer serializes synth registration,
unregistration and switching against speech output.

This depends on the previous patch: the catch-up drain introduced
here assumes that catch_up() eventually returns once synth->alive has
been cleared, including in the local catch_up() copies of the serial
synth drivers.

Fixes: 2067fd92d75b6 ("staging/speakup: Move out of staging")
Cc: stable@xxxxxxxxxxxxxxx
Cc: Samuel Thibault <samuel.thibault@xxxxxxxxxxxx>
Signed-off-by: Hongyu Xie <xiehongyu1@xxxxxxxxxx>
---
 drivers/accessibility/speakup/spk_priv.h  |  3 ++
 drivers/accessibility/speakup/spk_types.h |  1 +
 drivers/accessibility/speakup/synth.c     | 59 +++++++++++++++++++++--
 drivers/accessibility/speakup/thread.c    | 11 ++++-
 4 files changed, 70 insertions(+), 4 deletions(-)

diff --git a/drivers/accessibility/speakup/spk_priv.h b/drivers/accessibility/speakup/spk_priv.h
index 9da57ead17cbe..967862cd04403 100644
--- a/drivers/accessibility/speakup/spk_priv.h
+++ b/drivers/accessibility/speakup/spk_priv.h
@@ -75,6 +75,9 @@ struct spk_synth *synth_current(void);
 
 extern struct speakup_info_t speakup_info;
 
+extern atomic_t synth_catch_up_count;
+extern wait_queue_head_t synth_catch_up_waitq;
+
 extern struct var_t synth_time_vars[];
 
 extern struct spk_io_ops spk_serial_io_ops;
diff --git a/drivers/accessibility/speakup/spk_types.h b/drivers/accessibility/speakup/spk_types.h
index a5762330e2495..efb931db18506 100644
--- a/drivers/accessibility/speakup/spk_types.h
+++ b/drivers/accessibility/speakup/spk_types.h
@@ -200,6 +200,7 @@ struct spk_synth {
 	unsigned char (*get_index)(struct spk_synth *synth);
 	struct synth_indexing indexing;
 	int alive;
+	bool releasing;	/* set while synth_release() drains this synth */
 	struct attribute_group attributes;
 
 	void *dev;
diff --git a/drivers/accessibility/speakup/synth.c b/drivers/accessibility/speakup/synth.c
index 6a49ff475c16a..40870949b3c6b 100644
--- a/drivers/accessibility/speakup/synth.c
+++ b/drivers/accessibility/speakup/synth.c
@@ -24,6 +24,14 @@ char spk_pitch_buff[32] = "";
 static int module_status;
 bool spk_quiet_boot;
 
+/*
+ * Number of catch_up() calls currently in flight, and waitqueue used by
+ * synth_release() to wait for them to finish before tearing down the
+ * synth resources they may still be using.
+ */
+atomic_t synth_catch_up_count = ATOMIC_INIT(0);
+DECLARE_WAIT_QUEUE_HEAD(synth_catch_up_waitq);
+
 struct speakup_info_t speakup_info = {
 	/*
 	 * This spinlock is used to protect the entire speakup machinery, and
@@ -154,6 +162,8 @@ EXPORT_SYMBOL_GPL(spk_synth_get_index);
 
 int spk_synth_is_alive_nop(struct spk_synth *synth)
 {
+	if (synth->releasing)
+		return 0;
 	synth->alive = 1;
 	return 1;
 }
@@ -163,6 +173,18 @@ int spk_synth_is_alive_restart(struct spk_synth *synth)
 {
 	if (synth->alive)
 		return 1;
+	/*
+	 * The callers run under speakup_info.spinlock (speakup_key()
+	 * holds it across do_spkup()), which is also the lock
+	 * synth_release() takes to clear ->alive, so checking
+	 * ->releasing here cannot race it: a synth being released is
+	 * not revived, or an in-flight catch_up() draining to a stuck
+	 * synth would never see ->alive cleared and synth_release()
+	 * would block forever on the catch-up count while holding
+	 * spk_mutex.
+	 */
+	if (synth->releasing)
+		return 0;
 	if (synth->io_ops->wait_for_xmitr(synth) > 0) {
 		/* restart */
 		synth->alive = 1;
@@ -197,6 +219,8 @@ void synth_start(void)
 
 void spk_do_flush(void)
 {
+	struct task_struct *task;
+
 	if (!synth)
 		return;
 
@@ -209,7 +233,11 @@ void spk_do_flush(void)
 		}
 	}
 	wake_up_interruptible_all(&speakup_event);
-	wake_up_process(speakup_task);
+	rcu_read_lock();
+	task = READ_ONCE(speakup_task);
+	if (!IS_ERR_OR_NULL(task))
+		wake_up_process(task);
+	rcu_read_unlock();
 }
 
 void synth_write(const char *_buf, size_t count)
@@ -476,13 +504,18 @@ int synth_init(char *synth_name)
 /* called by: synth_add() */
 static int do_synth_init(struct spk_synth *in_synth)
 {
+	struct task_struct *task;
 	struct var_t *var;
+	unsigned long flags;
 
 	synth_release();
 	if (in_synth->checkval != SYNTH_CHECK)
 		return -EINVAL;
 	synth = in_synth;
 	synth->alive = 0;
+	spin_lock_irqsave(&speakup_info.spinlock, flags);
+	synth->releasing = false;
+	spin_unlock_irqrestore(&speakup_info.spinlock, flags);
 	pr_warn("synth probe\n");
 	if (synth->probe(synth) < 0) {
 		pr_warn("%s: device probe failed\n", in_synth->name);
@@ -510,13 +543,17 @@ static int do_synth_init(struct spk_synth *in_synth)
 		return -ENOMEM;
 	synth_flags = synth->flags;
 	wake_up_interruptible_all(&speakup_event);
-	if (speakup_task)
-		wake_up_process(speakup_task);
+	rcu_read_lock();
+	task = READ_ONCE(speakup_task);
+	if (!IS_ERR_OR_NULL(task))
+		wake_up_process(task);
+	rcu_read_unlock();
 	return 0;
 }
 
 void synth_release(void)
 {
+	struct task_struct *task;
 	struct var_t *var;
 	unsigned long flags;
 
@@ -525,8 +562,24 @@ void synth_release(void)
 	spin_lock_irqsave(&speakup_info.spinlock, flags);
 	pr_info("releasing synth %s\n", synth->name);
 	synth->alive = 0;
+	synth->releasing = true;
 	timer_delete(&thread_timer);
+	synth_buffer_clear();
 	spin_unlock_irqrestore(&speakup_info.spinlock, flags);
+	/*
+	 * catch_up() may still be running without spk_mutex held (it gets
+	 * taken off the mutex by speakup_thread()).  Wake it out of its
+	 * sleep and wait until it is done before releasing the resources
+	 * (sysfs, variables, tty) it may be accessing.  speakup_task can
+	 * be an ERR_PTR here, when speakup_init() failed after the
+	 * command-line synth_init().
+	 */
+	rcu_read_lock();
+	task = READ_ONCE(speakup_task);
+	if (!IS_ERR_OR_NULL(task))
+		wake_up_process(task);
+	rcu_read_unlock();
+	wait_event(synth_catch_up_waitq, !atomic_read(&synth_catch_up_count));
 	if (synth->attributes.name)
 		sysfs_remove_group(speakup_kobj, &synth->attributes);
 	for (var = synth->vars; var->var_id != MAXVARS; var++)
diff --git a/drivers/accessibility/speakup/thread.c b/drivers/accessibility/speakup/thread.c
index 2fc75e60fbac2..5279e112248b4 100644
--- a/drivers/accessibility/speakup/thread.c
+++ b/drivers/accessibility/speakup/thread.c
@@ -50,9 +50,18 @@ int speakup_thread(void *data)
 		if (synth && synth->catch_up && synth->alive) {
 			/*
 			 * It is up to the callee to take the lock, so that it
-			 * can sleep whenever it likes
+			 * can sleep whenever it likes.  It may indeed sleep for
+			 * a long while, e.g. while draining to a slow synth, so
+			 * run it without spk_mutex: synth_release() waits for
+			 * the catch-up count to drop before freeing the synth,
+			 * hence the synth stays alive for the whole call.
 			 */
+			atomic_inc(&synth_catch_up_count);
+			mutex_unlock(&spk_mutex);
 			synth->catch_up(synth);
+			if (atomic_dec_and_test(&synth_catch_up_count))
+				wake_up(&synth_catch_up_waitq);
+			mutex_lock(&spk_mutex);
 		}
 
 		speakup_start_ttys();
-- 
2.25.1