Message-ID: <Y3ytSj6NEeU54z7m@wendy>
Date:   Tue, 22 Nov 2022 11:06:50 +0000
From:   Conor Dooley <conor.dooley@...rochip.com>
To:     Samuel Holland <samuel@...lland.org>
CC:     Palmer Dabbelt <palmer@...osinc.com>,
        Conor Dooley <conor@...nel.org>, <anup@...infault.org>,
        <rafael@...nel.org>, <daniel.lezcano@...aro.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>, <aou@...s.berkeley.edu>,
        <linux-pm@...r.kernel.org>, <linux-riscv@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>, <linux@...osinc.com>
Subject: Re: [PATCH] cpuidle: riscv-sbi: Stop using non-retentive suspend

Hey Samuel,

Thanks a lot for the extra context.

On Mon, Nov 21, 2022 at 06:45:25PM -0600, Samuel Holland wrote:
> Hi Palmer,
> 
> On 11/21/22 14:56, Palmer Dabbelt wrote:
> > From: Palmer Dabbelt <palmer@...osinc.com>
> > 
> > As per [1], whether or not the core can wake up from non-retentive
> > suspend is a platform-specific detail.  We don't have any way to encode
> > that, so just stop using them until we've sorted that out.
> 
> We do have *exactly* a way to encode that. Specifically, the existence
> or non-existence of a non-retentive CPU suspend state in the DT.
> 
> If a hart has no way of resuming from non-retentive suspend (i.e. a
> complete lack of interrupt delivery in non-retentive suspend), then it
> makes zero sense to advertise such a suspend state in the DT.

I would have to agree with that. I think the sprawling conversation has
confused us all at this point, I (prior to reading this mail) thought
that suspend was borked on the D1. I don't think anyone is advertising
specific states in the DT at the moment though, I had a check in the D1
patchset and didn't see anything there - unless you're adding it
dynamically from the bootloader?

> Therefore,
> if the state exists in the DT, you can assume there is *some* interrupt
> that can wake up the hart. And I would extend that to assume at least
> one of those wakeup-capable interrupts is a timer interrupt, although
> not necessarily the architectural timer interrupt.
> 
> > Link: https://github.com/riscv-non-isa/riscv-sbi-doc/issues/98#issuecomment-1288564687
> 
> This comment refers specifically to the architectural timer interrupt,
> not interrupts more generally.
> 
> > Fixes: 6abf32f1d9c5 ("cpuidle: Add RISC-V SBI CPU idle driver")
> > Signed-off-by: Palmer Dabbelt <palmer@...osinc.com>
> > 
> > ---
> > 
> > This should allow us to revert 232ccac1bd9b ("clocksource/drivers/riscv:
> > Events are stopped during CPU suspend"), which fixes suspend on the D1
> > but breaks timers everywhere.
> 
> I understand that reverting 232ccac1bd9b is the easiest way to fix the
> issues you and others are seeing.

I am going to submit another respin of that revert, hopefully with the
extra context from here and elsewhere mixed in.

> I do not have any functioning RISC-V
> hardware with SMP, so it is hard for me to help debug the root issue in
> the Linux timer code. I do not know why turning on the C3STOP flag
> breaks RCU stall detection or clock_nanosleep(), but I agree that
> breakage should not happen.
> 
> So while I still think 232ccac1bd9b is the right change to make from a
> "following the spec" standpoint

Right, so the spec says:
Request the SBI implementation to put the calling hart in a platform
specific suspend (or low power) state specified by the suspend_type
parameter. The hart will automatically come out of suspended state and
resume normal execution when it receives an interrupt or platform
specific hardware event.

That, as we have discussed a bunch of times, does not say whether a
given interrupt should actually arrive during suspend. The correct
behaviour, to me, is to assume that no events arrive during suspend.

We've got a regular old SiFive implementation so I assume (and will go
investigate at some point this week) that the other SiFive {,based}
implementations also have timer events during suspend.

> I am okay with reverting it for
> pragmatic reasons. Since the D1 has another timer driver that is
> currently used in preference to the RISC-V/SBI timer triver,

Once we have got something in place to actually make the determination
we can revert the revert. I'll go give some feedback on Anup's stuff,
I've been meaning to but unfortunately not had the chance to do so yet.

> reverting
> 232ccac1bd9b does not break non-retentive suspend for the D1.

Ah, I did not know this. Probably should have gone looking before I
acked this patch - sorry!
Since that's the case this patch seems un-needed.

> But please do not make the change below. It is unnecessarily broad, and
> will break something that works fine right now. If the DT advertises a
> CPU suspend state that cannot be woken up from at all, then that is a
> bug in the DT. Linux should not try to work around that.

Thanks again Samuel :)

> > ---
> >  drivers/cpuidle/cpuidle-riscv-sbi.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c
> > index 05fe2902df9a..9d1063a54495 100644
> > --- a/drivers/cpuidle/cpuidle-riscv-sbi.c
> > +++ b/drivers/cpuidle/cpuidle-riscv-sbi.c
> > @@ -214,6 +214,17 @@ static bool sbi_suspend_state_is_valid(u32 state)
> >  	if (state > SBI_HSM_SUSPEND_NON_RET_DEFAULT &&
> >  	    state < SBI_HSM_SUSPEND_NON_RET_PLATFORM)
> >  		return false;
> > +
> > +	/*
> > +	 * Whether or not RISC-V systems deliver interrupts to harts in a
> > +	 * non-retentive suspend state is a platform-specific detail.  This can
> > +	 * leave the hart unable to wake up, so just mark these states as
> > +	 * unsupported until we have a mechanism to expose these
> > +	 * platform-specific details to Linux.
> > +	 */
> > +	if (state & SBI_HSM_SUSP_NON_RET_BIT)
> > +		return false;
> > +
> >  	return true;
> >  }
> >  
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists