Re: [PATCH 1/2] staging: greybus: loopback: fix gb_pm_runtime_get_sync error handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx> 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
_______________________________________________
greybus-dev mailing list
greybus-dev@xxxxxxxxxxxxxxxx
https://lists.linaro.org/mailman/listinfo/greybus-dev




[Index of Archives]     [Asterisk App Development]     [PJ SIP]     [Gnu Gatekeeper]     [IETF Sipping]     [Info Cyrus]     [ALSA User]     [Fedora Linux Users]     [Linux SCTP]     [DCCP]     [Gimp]     [Yosemite News]     [Deep Creek Hot Springs]     [Yosemite Campsites]     [ISDN Cause Codes]     [Asterisk Books]

  Powered by Linux