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

[Hongyu Xie]

Hi Samuel,

Thanks for reviewing the series.

You asked whether the blocking this patch removes is really a problem,
since an operator who loads/unloads/switches a synth knows speech is
being played and can stop it first.  The report that motivated this
series is a case where that cannot happen: it comes from automated
module load/unload testing on arm64 boards that have a serial port but
no synthesizer attached.  The sequence is:

    modprobe speakup
    modprobe speakup_acntsa
    modprobe -r speakup_acntsa

speakup_acntsa attaches to the serial port and becomes the active
synth, but nothing on the other end of the port consumes the output,
so the tty write never makes progress and catch_up() keeps looping
while holding spk_mutex.  modprobe -r never returns - the original
report is a hung task warning on a stock 6.6 distro kernel.  The trace
below is from a qemu reproduction of the same sequence with the serial
port backpressured:

    INFO: task rmmod:84 blocked for more than 10 seconds.
    task:rmmod   state:D
    Call Trace:
     __mutex_lock
     synth_remove+0x16/0x60 [speakup]
     __x64_sys_delete_module

    INFO: task rmmod:84 is blocked on a mutex likely owned by task speakup:76.
    task:speakup   state:S
    Call Trace:
     schedule_timeout
     _spk_do_catch_up+0x1d4/0x2d0 [speakup]
     speakup_thread+0xd6/0x240 [speakup]
    locks held by speakup/76:
     #0: (spk_mutex){+.+.}, at: speakup_thread+0x1b8/0x240 [speakup]

There is nobody at these machines to stop speech first: they are
headless test boxes, and no speech is being produced at all - there is
no synthesizer connected, the port simply never drains.  The same
mutex hold also blocks loading another synth module (synth_add) and
any synth switch, which is how the load-side variant of the test hangs
as well.

The "stop it manually first" route itself hangs the same way, because
the synth sysfs store handler takes spk_mutex too:

    echo none > /sys/accessibility/speakup/synth

reproduces the identical D state on the same mutex:

    INFO: task sh:79 is blocked on a mutex likely owned by task speakup:73.
    locks held by sh/79:
     #3: (spk_mutex){+.+.}, at: synth_init+0xab/0xe0 [speakup]

and a task in D state ignores SIGTERM, so even timeout(1) cannot get
the shell back.

Patch 1 alone does not unblock this sequence: it adds the loop exit on
!synth->alive, but in this scenario nothing ever clears synth->alive -
the port stays open, the synth stays alive, its output just never
drains.  It is synth_release() in patch 2 that clears ->alive and
waits for the in-flight catch_up() to leave before releasing the
resources, so the unload terminates the loop instead of waiting for
output that will never complete.  As the cover letter says, patch 1
provides the loop exit, patch 2 provides the deactivation and the
drain.

For real hardware the same holds with a finite per-utterance but
unbounded aggregate wait: /dev/synth accepts arbitrary amounts of text
and with the current code a switch or unload first waits for all
queued text to be spoken.  Patch 2 does not add locking on the read
paths - it drops the mutex from the longest-held path and replaces it
with a catch-up counter plus a drain wait in synth_release(), so the
synth structures stay valid for the whole call without holding
spk_mutex.

Hongyu