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.
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.
It should be possible to use the gb_pm_runtime_get_sync as a determinant and cease further operations.
I spin that into a v2 this evening.
This is not directly related to the sysfs entries, they belong to the bundle and should stay while the device is present. As mentioned before, the sysfs-interface is not the right interface for this type of device, but that's a different story.
Next steps on this driver entail changing the relationship between
/sysfs
and the loopback thread - probably making the loopback thread
dynamically
started/stopped - as opposed to the pre-allocation model currently
used
but, those changes are for a future series to address.
You might get away with using a work struct scheduled from the current sysfs-interface, but long term this should probably be shifted over to using a character-device interface.
Thanks, Johan