Re: [PATCHv3 1/6] arm/arm64: smccc/psci: add arm_smccc_1_1_get_conduit()

[Dave Martin <Dave.Martin@arm.com>]

On Mon, Aug 12, 2019 at 04:06:35PM +0100, Mark Rutland wrote:
> On Mon, Aug 12, 2019 at 04:03:29PM +0100, Dave Martin wrote:
> > On Fri, Aug 09, 2019 at 02:22:40PM +0100, Mark Rutland wrote:
> > > SMCCC callers are currently amassing a collection of enums for the SMCCC
> > > conduit, and are having to dig into the PSCI driver's internals in order
> > > to figure out what to do.
> > > 
> > > Let's clean this up, with common SMCCC_CONDUIT_* definitions, and an
> > > arm_smccc_1_1_get_conduit() helper that abstracts the PSCI driver's
> > > internal state.
> > > 
> > > We can kill off the PSCI_CONDUIT_* definitions once we've migrated users
> > > over to the new interface.
> > > 
> > > Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> > > Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > Acked-by: Will Deacon <will.deacon@arm.com>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > ---
> > >  drivers/firmware/psci/psci.c | 15 +++++++++++++++
> > >  include/linux/arm-smccc.h    | 16 ++++++++++++++++
> > >  2 files changed, 31 insertions(+)
> > > 
> > > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > > index f82ccd39a913..5f31f1bea1af 100644
> > > --- a/drivers/firmware/psci/psci.c
> > > +++ b/drivers/firmware/psci/psci.c
> > > @@ -57,6 +57,21 @@ struct psci_operations psci_ops = {
> > >  	.smccc_version = SMCCC_VERSION_1_0,
> > >  };
> > >  
> > > +enum arm_smccc_conduit arm_smccc_1_1_get_conduit(void)
> > 
> > Do we expect this to be specific to SMCCC v1.1?
> 
> I intend it to be 1.1+

It seems overspecific, but I guess we can address this later if it
becomes an issue.  This is an internal API for now (at worst I might
envisage it being EXPORT_SYMBOL_GPL()).

> > > +{
> > > +	if (psci_ops.smccc_version < SMCCC_VERSION_1_1)
> > > +		return SMCCC_CONDUIT_NONE;
> > > +
> > > +	switch (psci_ops.conduit) {
> > > +	case PSCI_CONDUIT_SMC:
> > > +		return SMCCC_CONDUIT_SMC;
> > > +	case PSCI_CONDUIT_HVC:
> > > +		return SMCCC_CONDUIT_HVC;
> > > +	default:
> > > +		return SMCCC_CONDUIT_NONE;
> > > +	}
> > > +}
> > > +
> > >  typedef unsigned long (psci_fn)(unsigned long, unsigned long,
> > >  				unsigned long, unsigned long);
> > >  static psci_fn *invoke_psci_fn;
> > > diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> > > index 080012a6f025..df01a8579034 100644
> > > --- a/include/linux/arm-smccc.h
> > > +++ b/include/linux/arm-smccc.h
> > > @@ -80,6 +80,22 @@
> > >  
> > >  #include <linux/linkage.h>
> > >  #include <linux/types.h>
> > > +
> > > +enum arm_smccc_conduit {
> > > +	SMCCC_CONDUIT_NONE,
> > 
> > If this is intended to have the value 0, is it worth making that
> > explicit?  I can never remember whether enums start at 1 or 0 by
> > default...
> 
> They start at 0. I intend that checks are done explicitly against an
> enum value, so I'm not sure that matters.

Not really.

It depends whether code like if (!arm_smccc_1_1_get_conduit()) { ... }
is considered sane or not.

If we don't think people should be doing this, omitting the explicit
value specifier seems fine.

Cheers
---Dave

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