Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 31 May 2021 11:21:53 -0600
From:   Mathieu Poirier <mathieu.poirier@linaro.org>
To:     Bjorn Andersson <bjorn.andersson@linaro.org>
Cc:     Alex Elder <elder@linaro.org>, ohad@wizery.com,
        linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] remoteproc: use freezable workqueue for crash
 notifications
Message-ID: <20210531172153.GA1718330@xps15>
References: <20210519234418.1196387-1-elder@linaro.org>
 <20210519234418.1196387-2-elder@linaro.org>
 <YLBpmdZoGDXNz64y@builder.lan>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YLBpmdZoGDXNz64y@builder.lan>
Precedence: bulk

On Thu, May 27, 2021 at 10:55:05PM -0500, 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 is a very tight race condition - I agree with you that it is next to
impossible to test.

> > 
> > 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).
> > 

I think the real fix for this problem should be in the platform driver where
the remoteproc interrupt would be masked while suspending and re-enabled again
when resuming.  The runtime PM API would work just fine for that...  But doing
so wouldn't guarantee that other drivers, i.e IPA, would be operational.  Unless
of one is a child of the other or using a bus like mechanic, and getting
to that point will introduce a lot more churn than what this patch does. 

The proposed solution will work since user space is expected to resume when
the kernel and drivers are fully operational.  As you pointed out the only
concern is with other users, which may not be noticeable given the very small
delay that is introduced.  As such:

Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro>

But be mindful the patch will have to be reverted if someone complains, whether
that happens in two weeks or 10 months.

Thanks,
Mathieu

> 
> 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@linaro.org>
> 
> But it would be nice to get some input from other users of the
> framework.
> 
> Regards,
> Bjorn
> 
> > Signed-off-by: Alex Elder <elder@linaro.org>
> > ---
> >  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
> >