Re: [PATCH] cpuidle: riscv-sbi: Stop using non-retentive suspend

[Conor Dooley]

Hey Samuel,

>On Tue, Nov 22, 2022 at 09:42:24PM -0600, Samuel Holland wrote:
> On 11/22/22 05:06, Conor Dooley wrote:
> > On Mon, Nov 21, 2022 at 06:45:25PM -0600, Samuel Holland wrote:
> > > On 11/21/22 14:56, Palmer Dabbelt wrote:

> > > > 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?
> 
> The availability and latency properties of idle states depend on the SBI
> implementation, so yes, they need to be added dynamically.

Right, thanks for clarifying.

> > > 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.
> 
> Are you suggesting that we need some property to declare the delivery of
> each kind of interrupt (software, timer, external, PMU)? 

I'm possibly taking things to an extreme, since if we're having a
discussion that covers what the spec does and does not allow I see no
harm in going down the rabbit hole!

Obviously, some sort of event must get the CPU out of suspend - what I
meant was more like "The correct (software) behaviour, to me, is to
assume that, when looking at an individual source, its events may not
arrive during suspend."

I've not looked at the relevant specs to see if they specify whether
their interrupts *must* arrive, just the SBI one that the issue was
created against.

> I assumed that
> external interrupt delivery would be required to consider an idle state
> "viable", but I suppose it would be _possible_ to have a state where
> only timer interrupts are deliverable.

Who knows what some hardware folks will come up with! Maybe I am being
pretty <whatever the modern version of black & white> is here, but I
fear for a repeat whenever someone does something "creative".

I know I've not answered your question about other kinds of properties
but I am well outside my comfort zone here.

Thanks,
Conor.