linux-kernel - Re: [PATCH v2] Revert "clocksource/drivers/riscv: Events are stopped during CPU suspend"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 22 Nov 2022 23:49:49 -0600
From:   Samuel Holland <samuel@...lland.org>
To:     Conor Dooley <conor.dooley@...rochip.com>,
        daniel.lezcano@...aro.org, tglx@...utronix.de
Cc:     Anup Patel <anup@...infault.org>,
        Palmer Dabbelt <palmer@...belt.com>,
        Palmer Dabbelt <palmer@...osinc.com>, aou@...s.berkeley.edu,
        atishp@...shpatra.org, dmitriy@...-tech.org,
        paul.walmsley@...ive.com, linux-kernel@...r.kernel.org,
        linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v2] Revert "clocksource/drivers/riscv: Events are stopped
 during CPU suspend"

On 11/22/22 06:16, Conor Dooley wrote:
> This reverts commit 232ccac1bd9b5bfe73895f527c08623e7fa0752d.
> 
> On the subject of suspend, the RISC-V SBI spec states:
>> 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.
> 
> This does not cover whether any given events actually reach the hart or
> not, just what the hart will do if it receives an event. On PolarFire
> SoC, and potentially other SiFive based implementations, events from the
> RISC-V timer do reach a hart during suspend. This is not the case for
> the implementation on the Allwinner D1 - there timer events are not
> received during suspend.
> 
> To fix this, the x86 C3STOP feature was enabled for the timer driver -

C3STOP isn't inherently x86-specific.

> but this has broken both RCU stall detection and timers generally on
> PolarFire SoC (and potentially other SiFive based implementations).
> 
> If an AXI read to the PCIe controller on PolarFire SoC times out, the
> system will stall, however, with this patch applied, the system just
> locks up without RCU stalling:
> 	io scheduler mq-deadline registered
> 	io scheduler kyber registered
> 	microchip-pcie 2000000000.pcie: host bridge /soc/pcie@...0000000 ranges:
> 	microchip-pcie 2000000000.pcie:      MEM 0x2008000000..0x2087ffffff -> 0x0008000000
> 	microchip-pcie 2000000000.pcie: sec error in pcie2axi buffer
> 	microchip-pcie 2000000000.pcie: ded error in pcie2axi buffer
> 	microchip-pcie 2000000000.pcie: axi read request error
> 	microchip-pcie 2000000000.pcie: axi read timeout
> 	microchip-pcie 2000000000.pcie: sec error in pcie2axi buffer
> 	microchip-pcie 2000000000.pcie: ded error in pcie2axi buffer
> 	microchip-pcie 2000000000.pcie: sec error in pcie2axi buffer
> 	microchip-pcie 2000000000.pcie: ded error in pcie2axi buffer
> 	microchip-pcie 2000000000.pcie: sec error in pcie2axi buffer
> 	microchip-pcie 2000000000.pcie: ded error in pcie2axi buffer
> 	Freeing initrd memory: 7332K
> 
> Similarly issues were reported with clock_nanosleep() - with a test app
> that sleeps each cpu for 6, 5, 4, 3 ms respectively, HZ=250 & the blamed
> commit in place, the sleep times are rounded up to the next jiffy:
> 
> == CPU: 1 ==      == CPU: 2 ==      == CPU: 3 ==      == CPU: 4 ==
> Mean: 7.974992    Mean: 7.976534    Mean: 7.962591    Mean: 3.952179
> Std Dev: 0.154374 Std Dev: 0.156082 Std Dev: 0.171018 Std Dev: 0.076193
> Hi: 9.472000      Hi: 10.495000     Hi: 8.864000      Hi: 4.736000
> Lo: 6.087000      Lo: 6.380000      Lo: 4.872000      Lo: 3.403000
> Samples: 521      Samples: 521      Samples: 521      Samples: 521
> 
> Fortunately, the D1 has a second timer, which is "currently used in
> preference to the RISC-V/SBI timer driver" so a revert here does not
> hurt operation of D1 in it's current form.

typo: its

> Ultimately, a DeviceTree property (or node) will be added to encode the
> behaviour of the timers, but until then revert the addition of
> CLOCK_EVT_FEAT_C3STOP.
> 
> Link: https://lore.kernel.org/linux-riscv/YzYTNQRxLr7Q9JR0@spud/
> Link: https://github.com/riscv-non-isa/riscv-sbi-doc/issues/98/
> Link: https://lore.kernel.org/linux-riscv/bf6d3b1f-f703-4a25-833e-972a44a04114@sholland.org/
> Fixes: 232ccac1bd9b ("clocksource/drivers/riscv: Events are stopped during CPU suspend")
> CC: Samuel Holland <samuel@...lland.org>
> CC: Anup Patel <anup@...infault.org>
> CC: Palmer Dabbelt <palmer@...belt.com>
> Reviewed-by: Palmer Dabbelt <palmer@...osinc.com>
> Acked-by: Palmer Dabbelt <palmer@...osinc.com>
> Signed-off-by: Conor Dooley <conor.dooley@...rochip.com>
> ---
> 
> For v2, I have re-worked the commit message to (hopefully) add improved
> context.
> 
> CC: aou@...s.berkeley.edu
> CC: atishp@...shpatra.org
> CC: daniel.lezcano@...aro.org
> CC: dmitriy@...-tech.org
> CC: paul.walmsley@...ive.com
> CC: tglx@...utronix.de
> CC: linux-kernel@...r.kernel.org
> CC: linux-riscv@...ts.infradead.org
> ---
>  drivers/clocksource/timer-riscv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
> index 969a552da8d2..a0d66fabf073 100644
> --- a/drivers/clocksource/timer-riscv.c
> +++ b/drivers/clocksource/timer-riscv.c
> @@ -51,7 +51,7 @@ static int riscv_clock_next_event(unsigned long delta,
>  static unsigned int riscv_clock_event_irq;
>  static DEFINE_PER_CPU(struct clock_event_device, riscv_clock_event) = {
>  	.name			= "riscv_timer_clockevent",
> -	.features		= CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_FEAT_C3STOP,
> +	.features		= CLOCK_EVT_FEAT_ONESHOT,
>  	.rating			= 100,
>  	.set_next_event		= riscv_clock_next_event,
>  };

Acked-by: Samuel Holland <samuel@...lland.org>