Re: [PATCH v1 1/1] riscv: sbi: Introduce system suspend support

From: Andrew Jones <ajones@ventanamicro.com>
To: Conor Dooley <conor@kernel.org>
Cc: linux-riscv@lists.infradead.org, paul.walmsley@sifive.com,
	 palmer@dabbelt.com, aou@eecs.berkeley.edu,
	leyfoon.tan@starfivetech.com,  jeeheng.sia@starfivetech.com,
	conor.dooley@microchip.com, apatel@ventanamicro.com
Subject: Re: [PATCH v1 1/1] riscv: sbi: Introduce system suspend support
Date: Thu, 12 Oct 2023 18:01:38 +0200	[thread overview]
Message-ID: <20231012-0cb15beef3128da9f3bd3299@orel> (raw)
In-Reply-To: <20231012-pulverize-founding-f459336028a2@spud>

On Thu, Oct 12, 2023 at 02:32:46PM +0100, Conor Dooley wrote:
> On Thu, Oct 12, 2023 at 02:30:02PM +0100, Conor Dooley wrote:
> > Yo,
> > 
> > On Thu, Oct 12, 2023 at 09:21:50AM +0200, Andrew Jones wrote:
> > > When the SUSP SBI extension is present it implies that the standard
> > > "suspend to RAM" type is available. Wire it up to the generic
> > > platform suspend support, also applying the already present support
> > > for non-retentive CPU suspend. When the kernel is built with
> > > CONFIG_SUSPEND, one can do 'echo mem > /sys/power/state' to suspend.
> > > Resumption will occur when a platform-specific wake-up event arrives.
> > > 
> > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > 
> > > +static int __init sbi_system_suspend_init(void)
> > > +{
> > > +	if (!sbi_spec_is_0_1() && sbi_probe_extension(SBI_EXT_SUSP) > 0) {
> > 
> > Random thought I had reading this, was that it'll be possible to have a
> > firmware that implements SBI < 2.0 that provides the SUSP extension.
> > FWIW, I don't think that that is problematic, but maybe I am missing
> > something that would make it so.
> 
> Hmm, next patch I look at is from Anup's debug console series, and he
> does check that the SBI implementation is at least version 2.0 before
> probing for the extension. We should probably have the same policy
> everywhere.

Yeah, the main reason I wrote in the other response that I'd prefer not
to always check version when probing is because in most (I think all
except PMU) cases it would only reduce the platforms where the extension
can be used, but without any reason to do so. For example, right now QEMU
bundles an OpenSBI v1.3.1 binary and that version supports both SUSP and
DBCN, but, if we were to add SBI 2.0 checks in Linux for those extensions,
then users will need to update their SBI implementations in order to use
them. While requiring users to update their SBI implementations makes
sense for new functionality or fixes, it seems like a lot to ask for them
to just get a bigger number in their version check.

(And then there's downstream SBI implementations which may end up cherry
picking extensions, so their version numbers would be hard to define.)

So my vote for Linux policy would be to only do version checks when
necessary. And my preference for SBI would be to try and avoid specifying
extensions which require clients to check versions.

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv