Message-ID: <2309d3e5-0e37-c77b-1c0b-610abf0af62d@sholland.org>
Date:   Tue, 22 Nov 2022 21:42:24 -0600
From:   Samuel Holland <samuel@...lland.org>
To:     Conor Dooley <conor.dooley@...rochip.com>
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

Hi Conor,

On 11/22/22 05:06, Conor Dooley wrote:
> 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?

The availability and latency properties of idle states depend on the SBI
implementation, so yes, they need to be added dynamically.

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

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

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

Thanks for following up on this.

Regards,
Samuel

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