I had a couple of good discussions with our power management expert
and even then, the way forward isn't as clear as I would have liked.
I am currently travelling and as such don't have the required time to
go into greater details, something I will be doing next week.
On Sun, 6 Nov 2022 at 18:14, Aiqun(Maria) Yu <quic_aiquny@xxxxxxxxxxx> wrote:
>
> Hi,
> On 11/4/2022 11:59 PM, Mathieu Poirier wrote:
> > On Thu, Nov 03, 2022 at 10:03:49AM +0800, Aiqun(Maria) Yu wrote:
> >> On 11/3/2022 2:03 AM, Mathieu Poirier wrote:
> >>> On Wed, Nov 02, 2022 at 06:53:49PM +0800, Aiqun(Maria) Yu wrote:
> >>>> Hi,
> >>>>
> >>>> Let me think about this carefully.
> >>>>
> >>>> When in RPROC_RECOVERY_FAIL case we want to re-do the recovery process again
> >>>> or just leave the pm_relax?
> >>>
> >>> Neither.
> >>>
> >>> When a recovery fail we don't want to call pm_relax(). The code in
> >>> rproc_crash_handler_work() becomes:
> >>>
> >>> if (rproc->state == RPROC_OFFLINE) {
> >>> /* We have raced with rproc_shutdown() */
> >>> pm_relax()
> >>> mutex_unlock(&rproc->lock);
> >>> return;
> >>> }
> >>>
> >>> if (rproc->state == RPROC_CRASHED ||
> >>> rproc->state == RPROC_RECOVERY_FAILED) {
> >>> /* handle only the first crash detected */
> >>> mutex_unlock(&rproc->lock);
> >>> return;
> >>> }
> >>>
> >>>
> >>> RPROC_RECOVERY_FAILED gets set in rproc_boot_recovery() if request_firmware() or
> >>> rproc_start() fail. Function rproc_trigger_recovery() needs to allow for the
> >>> recovery the the remote processor is in RPROC_RECOVERY_FAILED state. As such
> >>> the condition becomes:
> >>>
> >>> /* State could have changed before we got the mutex */
> >>> if (rproc->state != RPROC_CRASHED &&
> >>> rproc->state != RPROC_RECOVERY_FAILED)
> >>> goto unlock_mutex;
> >>>
> >>> Start with that and we can look at corner cases (if some exists) with a fresh
> >>> patchset. Note that I have not addressed the attach/detach() scenario in the
> >>> above.
> >>
> >> If we didn't deal with the recovery failed case with correct pm_relax call,
> >> it may left the device in a state that cannot enter to suspend state.
> >
> > That is what I am looking for. We don't want to give the impression that
> > everything is fine by allowing the device to suspend. If the remote processor
> > can't be recovered than it needs to be dealth with.
> For the normal recovery failed case, it still need to do pm_relax to not
> prevent the device goes to suspend. It is what in normal recovery failed
> case we do in rproc_crash_handler_work as well.
> rproc_crash_handler_work will not check the result of the
> rproc_trigger_recovery return value, and will always do pm_relax.
>
> For current conconrency cornor case as well, it is better to consistant
> with the current design of recovery fail senarios in normal cases.
>
> I personally agree that we shouldn't do nothing when it is a
> RPROC_RECOVERY_FAILED senario when it is in rproc_crash_handler_work
> check, because it maybe crash happened when it is trying to do the recovery.
> So I suggested to do a continue try of trigger recovery again instead of
> doing nothing and bail out if it is a RPROC_RECOVERY_FAILED state.
>
> >
> >> Because first PROC_RECOVERY_FAIL case cannot ensure it have pm_relax called
> >> before the second crash handler call pm_stay_awake or not.
> >>
> >
> > I've been thinking about that part. I don't think adding a wake_count to
> > control calls to pm_stay_awake()/pm_relax() is the best way to go. There is a
> > similar count happening in the PM runtime subsystem and that is what we should
> > be using. I have asked a power management expert at Linaro for guidance with
> > this matter. I should be able to get back to you with a way forward by the end
> > of next week.
> >
> Thx for the specific date provided as well. I will wait until your reply
> for next patchset then.
>
> >> So, What about the atomic count along with pm_relax and pm_stay_awake ?
> >>
> >> struct rproc{
> >> ...
> >> atomic_t wake_count;
> >> ...
> >> }
> >>
> >> rproc_pm_stay_awake()
> >> {
> >> atomic_inc(&wake_count);
> >> pm_stay_awake();
> >> }
> >>
> >> rproc_pm_relax()
> >> {
> >> if (atomic_dec_return(&wake_count) == 0)
> >> pm_stay_awake();
> >> }
> >>
> >> can refer code like:
> >>
> >> rproc_report_crash()
> >> {
> >> ...
> >> rproc_pm_stay_awake();
> >> queue_work();
> >> ...
> >> }
> >>
> >> rproc_crash_handler_work()
> >> {
> >> ...
> >> if (rproc->state == RPROC_OFFLINE || rproc->state == RPROC_CRASHED) {
> >> /* We have raced with rproc_shutdown() */
> >> rproc_pm_relax();
> >> mutex_unlock(&rproc->lock);
> >> return;
> >> }
> >> ...
> >> }
> >>
> >>>
> >>> Thanks,
> >>> Mathieu
> >>>
> >>>>
> >>>> recovery fail case 1:
> >>>> | |firstcrash interrupt issued
> >>>> | second crashed interrupt issued | rproc_report_crash()
> >>>> | rproc_report_crash() | pm_stay_awake()
> >>>> | pm_stay_awake() | queue_work()
> >>>> | queue_work() |rproc_crash_handler_work()
> >>>> | |mutex_lock(&rproc->lock);
> >>>> | |rproc_stop()
> >>>> |rproc_crash_handler_work() |rproc->state = RPROC_OFFLINE;
> >>>> | |RPROC_RECOVERY_FAIL //new
> >>>> | |mutex_unlock(&rproc->lock);
> >>>> |mutex_lock(&rproc->lock); |pm_relax()
> >>>> |if (rproc->state == RPROC_OFFLINE) |
> >>>> |return // shouldn't do pm_relax if RPROC_RECOVERY_FAIL? |
> >>>> |mutex_unlock(&rproc->lock); |
> >>>> | |
> >>>> | |
> >>>> | |
> >>>>
> >>>> recovery fail case 2:
> >>>> | |firstcrash interrupt issued
> >>>> | | rproc_report_crash()
> >>>> | | pm_stay_awake()
> >>>> | | queue_work()
> >>>> | |rproc_crash_handler_work()
> >>>> | |mutex_lock(&rproc->lock);
> >>>> | |rproc_stop()
> >>>> | |rproc->state = RPROC_OFFLINE;
> >>>> | |RPROC_RECOVERY_FAIL //new
> >>>> | |mutex_unlock(&rproc->lock);
> >>>> | |pm_relax()
> >>>> |
> >>>> | second crashed interrupt issued |
> >>>> | rproc_report_crash() |
> >>>> | pm_stay_awake() |
> >>>> | queue_work() |
> >>>> |pm_stay_awake()
> >>>> |mutex_lock(&rproc->lock);
> >>>> |if (rproc->state == RPROC_OFFLINE) |
> >>>> |return // still need do pm_relax if RPROC_RECOVERY_FAIL? |
> >>>> |mutex_unlock(&rproc->lock); |
> >>>> | |
> >>>> | |
> >>>> | |
> >>>>
> >>>> Maybe I can have:
> >>>> 1. the pm_stay_awake and pm_relax with count based and call with paired for
> >>>> fix current concurency issue.
> >>>> 2. RPROC_RECOVERY_FAIL can be another patch for continue try to do recovery
> >>>> work.
> >>>> 3. handle RPROC_DETACHED case.
> >>>>
> >>>> On 11/2/2022 4:11 AM, Mathieu Poirier wrote:
> >>>>> On Fri, 28 Oct 2022 at 09:31, Arnaud POULIQUEN
> >>>>> <arnaud.pouliquen@xxxxxxxxxxx> wrote:
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 10/24/22 05:17, Aiqun(Maria) Yu wrote:
> >>>>>>> On 10/22/2022 3:34 AM, Mathieu Poirier wrote:
> >>>>>>>> On Wed, 19 Oct 2022 at 23:52, Aiqun(Maria) Yu <quic_aiquny@xxxxxxxxxxx> wrote:
> >>>>>>>>>
> >>>>>>>>> On 10/14/2022 2:03 AM, Mathieu Poirier wrote:
> >>>>>>>>>> On Thu, Oct 13, 2022 at 11:34:42AM -0600, Mathieu Poirier wrote:
> >>>>>>>>>>> On Thu, Oct 13, 2022 at 09:40:09AM +0800, Aiqun(Maria) Yu wrote:
> >>>>>>>>>>>> Hi Mathieu,
> >>>>>>>>>>>>
> >>>>>>>>>>>> On 10/13/2022 4:43 AM, Mathieu Poirier wrote:
> >>>>>>>>>>>>> Please add what has changed from one version to another, either in a cover
> >>>>>>>>>>>>> letter or after the "Signed-off-by". There are many examples on how to
> >>>>>>>>>>>>> do that
> >>>>>>>>>>>>> on the mailing list.
> >>>>>>>>>>>>>
> >>>>>>>>>>>> Thx for the information, will take a note and benefit for next time.
> >>>>>>>>>>>>
> >>>>>>>>>>>>> On Fri, Sep 16, 2022 at 03:12:31PM +0800, Maria Yu wrote:
> >>>>>>>>>>>>>> RPROC_OFFLINE state indicate there is no recovery process
> >>>>>>>>>>>>>> is in progress and no chance to do the pm_relax.
> >>>>>>>>>>>>>> Because when recovering from crash, rproc->lock is held and
> >>>>>>>>>>>>>> state is RPROC_CRASHED -> RPROC_OFFLINE -> RPROC_RUNNING,
> >>>>>>>>>>>>>> and then unlock rproc->lock.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> You are correct - because the lock is held rproc->state should be set to
> >>>>>>>>>>>>> RPROC_RUNNING
> >>>>>>>>>>>>> when rproc_trigger_recovery() returns. If that is not the case then
> >>>>>>>>>>>>> something
> >>>>>>>>>>>>> went wrong.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Function rproc_stop() sets rproc->state to RPROC_OFFLINE just before
> >>>>>>>>>>>>> returning,
> >>>>>>>>>>>>> so we know the remote processor was stopped. Therefore if rproc->state
> >>>>>>>>>>>>> is set
> >>>>>>>>>>>>> to RPROC_OFFLINE something went wrong in either request_firmware() or
> >>>>>>>>>>>>> rproc_start(). Either way the remote processor is offline and the system
> >>>>>>>>>>>>> probably
> >>>>>>>>>>>>> in an unknown/unstable. As such I don't see how calling pm_relax() can help
> >>>>>>>>>>>>> things along.
> >>>>>>>>>>>>>
> >>>>>>>>>>>> PROC_OFFLINE is possible that rproc_shutdown is triggered and successfully
> >>>>>>>>>>>> finished.
> >>>>>>>>>>>> Even if it is multi crash rproc_crash_handler_work contention issue, and
> >>>>>>>>>>>> last rproc_trigger_recovery bailed out with only
> >>>>>>>>>>>> rproc->state==RPROC_OFFLINE, it is still worth to do pm_relax in pair.
> >>>>>>>>>>>> Since the subsystem may still can be recovered with customer's next trigger
> >>>>>>>>>>>> of rproc_start, and we can make each error out path clean with pm resources.
> >>>>>>>>>>>>
> >>>>>>>>>>>>> I suggest spending time understanding what leads to the failure when
> >>>>>>>>>>>>> recovering
> >>>>>>>>>>>>> from a crash and address that problem(s).
> >>>>>>>>>>>>>
> >>>>>>>>>>>> In current case, the customer's information is that the issue happened when
> >>>>>>>>>>>> rproc_shutdown is triggered at similar time. So not an issue from error out
> >>>>>>>>>>>> of rproc_trigger_recovery.
> >>>>>>>>>>>
> >>>>>>>>>>> That is a very important element to consider and should have been mentioned
> >>>>>>>>>>> from
> >>>>>>>>>>> the beginning. What I see happening is the following:
> >>>>>>>>>>>
> >>>>>>>>>>> rproc_report_crash()
> >>>>>>>>>>> pm_stay_awake()
> >>>>>>>>>>> queue_work() // current thread is suspended
> >>>>>>>>>>>
> >>>>>>>>>>> rproc_shutdown()
> >>>>>>>>>>> rproc_stop()
> >>>>>>>>>>> rproc->state = RPROC_OFFLINE;
> >>>>>>>>>>>
> >>>>>>>>>>> rproc_crash_handler_work()
> >>>>>>>>>>> if (rproc->state == RPROC_OFFLINE)
> >>>>>>>>>>> return // pm_relax() is not called
> >>>>>>>>>>>
> >>>>>>>>>>> The right way to fix this is to add a pm_relax() in rproc_shutdown() and
> >>>>>>>>>>> rproc_detach(), along with a very descriptive comment as to why it is needed.
> >>>>>>>>>>
> >>>>>>>>>> Thinking about this further there are more ramifications to consider. Please
> >>>>>>>>>> confirm the above scenario is what you are facing. I will advise on how to
> >>>>>>>>>> move
> >>>>>>>>>> forward if that is the case.
> >>>>>>>>>>
> >>>>>>>>> Not sure if the situation is clear or not. So resend the email again.
> >>>>>>>>>
> >>>>>>>>> The above senario is what customer is facing. crash hanppened while at
> >>>>>>>>> the same time shutdown is triggered.
> >>>>>>>>
> >>>>>>>> Unfortunately this is not enough details to address a problem as
> >>>>>>>> complex as this one.
> >>>>>>>>
> >>>>>>>>> And the device cannto goes to suspend state after that.
> >>>>>>>>> the subsystem can still be start normally after this.
> >>>>>>>>
> >>>>>>>> If the code flow I pasted above reflects the problem at hand, the
> >>>>>>>> current patch will not be sufficient to address the issue. If Arnaud
> >>>>>>>> confirms my suspicions we will have to think about a better solution.
> >>>>>>>>
> >>>>>>>
> >>>>>>> Hi Mathiew,
> >>>>>>>
> >>>>>>> Could you pls have more details of any side effects other then power issue of
> >>>>>>> the current senario?
> >>>>>>> Why the current patch is not sufficient pls?
> >>>>>>>
> >>>>>>>
> >>>>>>> Have the current senario in details with rproc->lock information in details:
> >>>>>>>
> >>>>>>> | subsystem crashed interrupt issued | user trigger shutdown
> >>>>>>> | rproc_report_crash() |
> >>>>>>> | pm_stay_awake() |
> >>>>>>> | queue_work() |
> >>>>>>> | |rproc_shutdown
> >>>>>>> | |mutex_lock(&rproc->lock);
> >>>>>>> | |rproc_stop()
> >>>>>>> |rproc_crash_handler_work() |rproc->state = RPROC_OFFLINE;
> >>>>>>> | |mutex_unlock(&rproc->lock);
> >>>>>>> |mutex_lock(&rproc->lock); |
> >>>>>>> |if (rproc->state == RPROC_OFFLINE) |
> >>>>>>> |return // pm_relax() is not called |rproc_boot
> >>>>>>> |mutex_unlock(&rproc->lock); |
> >>>>>>> | |mutex_lock(&rproc->lock);
> >>>>>>> | |rproc_start()
> >>>>>>> | |mutex_unlock(&rproc->lock);
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>> Agree with Mathieu, this is not so simple.
> >>>>>>
> >>>>>
> >>>>> Thanks for looking into this.
> >>>>>
> >>>>>> Here is my view hoping I haven't missed a point in your discussion or
> >>>>>> an other corner cases.
> >>>>>>
> >>>>>> I tried to analyze the issues (in what follows, the term "condition" means
> >>>>>> the "if" condition in which Aiqun proposes to add the fix) :
> >>>>>>
> >>>>>> I can see 4 use cases with race condition
> >>>>>>
> >>>>>> 1) crash report while already one is treated (rproc_boot_recovery called)
> >>>>>> => not a real use case as if the remote processor is crashed we
> >>>>>> should not have a second crash report
> >>>>>>
> >>>>>
> >>>>> That part is of great concern to me. *Theoretically* we should not
> >>>>> get a new crash report while one has already been dispatched but the
> >>>>> current code accounts for this scenario and as such the possibility
> >>>>> can't be dismissed. Therefore we need to expect rproc_report_crash()
> >>>>> to be called multiple times before a single instance of
> >>>>> rproc_boot_recovery() is scheduled.
> >>>>
> >>>>
> >>>>>
> >>>>>> 2) rproc_stop executed between the queuing of the crash work and the call of
> >>>>>> rproc_crash_handler_work
> >>>>>> => rproc->state = RPROC_OFFLINE
> >>>>>> => we enter in the "condition" and the pm_relax has to be called
> >>>>>> => This commit fix should solve this use case
> >>>>>>
> >>>>>> 3) rproc_detach executed between the queue of the crash work and the call of
> >>>>>> rproc_crash_handler_work
> >>>>>> => rproc->state = RPROC_DETACHED;
> >>>>>> => we don't go in "the condition" and issue because the recovery reattach
> >>>>>> to the remote processor
> >>>>>> => but pm_relax is called
> >>>>>> => probably need an extra fix to avoid to re-attach
> >>>>>>
> >>>>>> 4) crash report while already one is treated (rproc_attach_recovery called)
> >>>>>> this one corresponds to an auto reboot of the remote processor, with a
> >>>>>> new crash
> >>>>>> => rproc->state = RPROC_CRASHED or rproc->state = RPROC_DETACHED;
> >>>>>> 4)a) rproc->state = RPROC_CRASHED if rproc->recovery_disabled = true
> >>>>>> => should call pm_relax if rproc->recovery_disabled = true
> >>>>>> => commit does not work for this use case
> >>>>>>
> >>>>>> 4)b) rproc->state = RPROC_DETACHED if recovery fails
> >>>>>> => error case with an unstable state
> >>>>>> => how to differentiate it from the use case 3) ?
> >>>>>> => introduce a RPROC_RECOVERY_FAIL state?
> >>>>>>
> >>>>>
> >>>>> The case where a recovery fails needs to be considered and is the
> >>>>> reason the original patch doesn't work. Right now in
> >>>>> rproc_crash_handler_work(), it is not possible to differentiate
> >>>>> between a legitimate shutdown request (scenario #2 above) and a
> >>>>> recovery that went wrong. I think introducing RPROC_RECOVERY_FAIL
> >>>>> would greatly simplify things.
> >>>>>
> >>>>> My initial evaluation had not considered the attach/detach scenarios -
> >>>>> thanks for adding that in the mix.
> >>>>>
> >>>>> Aiqun, please send a new patchset that adds a new remote processor
> >>>>> state, i.e RPROC_RECOVERY_FAIL. There should also be another patch in
> >>>>> that set that takes attach/detach scenarios into account. The code
> >>>>> between the v6.0 and v6.1 cycle has changed a lot in that area so make
> >>>>> sure to properly rebase.
> >>>>>
> >>>> I will try.
> >>>>
> >>>>>>
> >>>>>> Then pm_stay_awake is called when the crash work is queued.
> >>>>>> It seems to me coherent to call the pm_relax in the work handler.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> Here is a quick and dirty patch (not tested) that should take into account the
> >>>>>> main use cases ( except 1) and 4)b) )
> >>>>>>
> >>>>>> @@ -2009,8 +2009,18 @@ static void rproc_crash_handler_work(struct work_struct *work)
> >>>>>>
> >>>>>> mutex_lock(&rproc->lock);
> >>>>>>
> >>>>>> - if (rproc->state == RPROC_CRASHED || rproc->state == RPROC_OFFLINE) {
> >>>>>> + if (rproc->state == RPROC_CRASHED || rproc->state == RPROC_OFFLINE ||
> >>>>>> + rproc->state == RPROC_DETACHED) {
> >>>>>> /* handle only the first crash detected */
> >>>>>> +
> >>>>>> + /*
> >>>>>> + * call pm-relax in following use cases:
> >>>>>> + * - the remote processor has been stopped by the user
> >>>>>> + * - the remote processor is detached
> >>>>>> + + - the remote proc has an autonomous reset but recovery_disabled is true.
> >>>>>> + */
> >>>>>> + if(rproc->state != RPROC_CRASHED || rproc->recovery_disabled)
> >>>>>> + pm_relax(rproc->dev.parent);
> >>>>>> mutex_unlock(&rproc->lock);
> >>>>>> return;
> >>>>>> }
> >>>>>>
> >>>>>> Regards,
> >>>>>> Arnaud
> >>>>>>
> >>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>> Mathieu
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>> When the state is in RPROC_OFFLINE it means separate request
> >>>>>>>>>>>>>> of rproc_stop was done and no need to hold the wakeup source
> >>>>>>>>>>>>>> in crash handler to recover any more.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> Signed-off-by: Maria Yu <quic_aiquny@xxxxxxxxxxx>
> >>>>>>>>>>>>>> ---
> >>>>>>>>>>>>>> drivers/remoteproc/remoteproc_core.c | 11 +++++++++++
> >>>>>>>>>>>>>> 1 file changed, 11 insertions(+)
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> diff --git a/drivers/remoteproc/remoteproc_core.c
> >>>>>>>>>>>>>> b/drivers/remoteproc/remoteproc_core.c
> >>>>>>>>>>>>>> index e5279ed9a8d7..6bc7b8b7d01e 100644
> >>>>>>>>>>>>>> --- a/drivers/remoteproc/remoteproc_core.c
> >>>>>>>>>>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
> >>>>>>>>>>>>>> @@ -1956,6 +1956,17 @@ static void rproc_crash_handler_work(struct
> >>>>>>>>>>>>>> work_struct *work)
> >>>>>>>>>>>>>> if (rproc->state == RPROC_CRASHED || rproc->state ==
> >>>>>>>>>>>>>> RPROC_OFFLINE) {
> >>>>>>>>>>>>>> /* handle only the first crash detected */
> >>>>>>>>>>>>>> mutex_unlock(&rproc->lock);
> >>>>>>>>>>>>>> + /*
> >>>>>>>>>>>>>> + * RPROC_OFFLINE state indicate there is no recovery process
> >>>>>>>>>>>>>> + * is in progress and no chance to have pm_relax in place.
> >>>>>>>>>>>>>> + * Because when recovering from crash, rproc->lock is held and
> >>>>>>>>>>>>>> + * state is RPROC_CRASHED -> RPROC_OFFLINE -> RPROC_RUNNING,
> >>>>>>>>>>>>>> + * and then unlock rproc->lock.
> >>>>>>>>>>>>>> + * RPROC_OFFLINE is only an intermediate state in recovery
> >>>>>>>>>>>>>> + * process.
> >>>>>>>>>>>>>> + */
> >>>>>>>>>>>>>> + if (rproc->state == RPROC_OFFLINE)
> >>>>>>>>>>>>>> + pm_relax(rproc->dev.parent);
> >>>>>>>>>>>>>> return;
> >>>>>>>>>>>>>> }
> >>>>>>>>>>>>>> --
> >>>>>>>>>>>>>> 2.7.4
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> --
> >>>>>>>>>>>> Thx and BRs,
> >>>>>>>>>>>> Aiqun(Maria) Yu
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> --
> >>>>>>>>> Thx and BRs,
> >>>>>>>>> Aiqun(Maria) Yu
> >>>>>>>
> >>>>>>>
> >>>>
> >>>>
> >>>> --
> >>>> Thx and BRs,
> >>>> Aiqun(Maria) Yu
> >>
> >>
> >> --
> >> Thx and BRs,
> >> Aiqun(Maria) Yu
>
>
> --
> Thx and BRs,
> Aiqun(Maria) Yu
