On Mon, Jan 09, 2017 at 11:29:50AM +0000, Bryan O'Donoghue wrote:
On January 9, 2017 11:19:09 AM GMT+00:00, Johan Hovold johan@kernel.org wrote:
On Sun, Jan 08, 2017 at 03:27:18PM +0000, Bryan O'Donoghue wrote:
commit e854ff58ed70 ("greybus: loopback: add runtime pm support") introduces pm runtime support to the loopback code. If a gb_pm_runtime_get_sync() fails, currently we immediately return from
the
loopback thread.
Alexandre reports that later on, subsequent to the afore mentioned
failure,
doing an rmmod will trigger a kthread_stop() which will will generate
a
fault. The fault results from dereferencing gb->task in gb_loopback_disconnect().
One way to fix that is to have the loopback thread do_exit() instead
of
simply returning on gb_pm_runtime_get_sync() failure - however this
will
leave dangling sysfs entries with no loopback thread to take action
based
on the sysfs inputs.
This patch fixes the fault by ignoring the gb_pm_runtime_get_sync()
result
code, this will allow only gb_loopback_disconnect() to terminate the loopback thread. Fix validated in gbsim.
No, you must check the return for resume errors, and not attempt any further I/O in case of failure.
The greybus operations themselves do report an error subsequent to the user-space tool.
Then you must already have a mechanism for reporting errors. Just set a flag and don't execute any further operations after resume failure.
Kill the thread, or somehow report a test failure and wait for user space to retry.
Killing the thread would be messy, however yes I take your general point.
Setting a flag and doing a clean exit should do right? Considering a resume-failure as a fatal error is reasonable.
It should be possible to use the gb_pm_runtime_get_sync as a determinant and cease further operations.
But cancelling the current test and allow user-space to restart it would be more well-behaved of course. Not sure it's worth it given the state of things though.
Johan