Re: [PATCH 1/1] remoteproc: use freezable workqueue for crash notifications

[Siddharth Gupta]

On 5/27/2021 8:55 PM, Bjorn Andersson wrote:
> On Wed 19 May 18:44 CDT 2021, Alex Elder wrote:
>
> > When a remoteproc has crashed, rproc_report_crash() is called to
> > handle whatever recovery is desired.  This can happen at almost any
> > time, often triggered by an interrupt, though it can also be
> > initiated by a write to debugfs file remoteproc/remoteproc*/crash.
> >
> > When a crash is reported, the crash handler worker is scheduled to
> > run (rproc_crash_handler_work()).  One thing that worker does is
> > call rproc_trigger_recovery(), which calls rproc_stop().  That calls
> > the ->stop method for any remoteproc subdevices before making the
> > remote processor go offline.
> >
> > The Q6V5 modem remoteproc driver implements an SSR subdevice that
> > notifies registered drivers when the modem changes operational state
> > (prepare, started, stop/crash, unprepared).  The IPA driver
> > registers to receive these notifications.
> >
> > With that as context, I'll now describe the problem.
> >
> > There was a situation in which buggy modem firmware led to a modem
> > crash very soon after system (AP) resume had begun.  The crash caused
> > a remoteproc SSR crash notification to be sent to the IPA driver.
> > The problem was that, although system resume had begun, it had not
> > yet completed, and the IPA driver was still in a suspended state.
> >
> > This scenario could happen to any driver that registers for these
> > SSR notifications, because they are delivered without knowledge of
> > the (suspend) state of registered recipient drivers.
> >
> > This patch offers a simple fix for this, by having the crash
> > handling worker function run on the system freezable workqueue.
> > This workqueue does not operate if user space is frozen (for
> > suspend).  As a result, the SSR subdevice only delivers its
> > crash notification when the system is fully operational (i.e.,
> > neither suspended nor in suspend/resume transition).
> This makes sense to me; both that it ensures that we spend our resources
> on the actual system resume and that it avoids surprises from this
> happening while the system still is in a funky state...
>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
>
> But it would be nice to get some input from other users of the
> framework.
This patch sounds like a good idea for cases where the
request_firmware() APIs fallback to userspace firmware loading.

Will test out this patch and report back.

Thanks,
Sid
> Regards,
> Bjorn
>
> > Signed-off-by: Alex Elder <elder@xxxxxxxxxx>
> > ---
> >   drivers/remoteproc/remoteproc_core.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 39cf44cb08035..6bedf2d2af239 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -2724,8 +2724,8 @@ void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type)
> >   	dev_err(&rproc->dev, "crash detected in %s: type %s\n",
> >   		rproc->name, rproc_crash_to_string(type));
> >   
> > -	/* create a new task to handle the error */
> > -	schedule_work(&rproc->crash_handler);
> > +	/* Have a worker handle the error; ensure system is not suspended */
> > +	queue_work(system_freezable_wq, &rproc->crash_handler);
> >   }
> >   EXPORT_SYMBOL(rproc_report_crash);
> >   
> > --
> > 2.27.0