[PATCH] firmware: arm_ffa: Handle compatibility with different firmware versions

[Jens Wiklander]

On Wed, Oct 13, 2021 at 6:08 PM Sudeep Holla <sudeep.holla at arm.com> wrote:
>
> On Wed, Oct 13, 2021 at 03:10:50PM +0200, Jens Wiklander wrote:
> > On Wed, Oct 13, 2021 at 11:11 AM Sudeep Holla <sudeep.holla at arm.com> wrote:
> > >
> > > The driver currently just support v1.0 of Arm FFA specification. It also
> > > expects the firmware implementation to match the same and bail out if it
> > > doesn't match. This is causing issue when running with higher version of
> > > firmware implementation(e.g. v1.1 which will released soon).
> > >
> > > In order to support compatibility with different firmware versions, let
> > > us add additional checks and find the compatible version the driver can
> > > work with.
> > >
> > > Signed-off-by: Sudeep Holla <sudeep.holla at arm.com>
> > > ---
> > >  drivers/firmware/arm_ffa/driver.c | 37 ++++++++++++++++++++++++++-----
> > >  1 file changed, 32 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > > index c9fb56afbcb4..6e0c883ab708 100644
> > > --- a/drivers/firmware/arm_ffa/driver.c
> > > +++ b/drivers/firmware/arm_ffa/driver.c
> > > @@ -167,6 +167,28 @@ struct ffa_drv_info {
> > >
> > >  static struct ffa_drv_info *drv_info;
> > >
> > > +/*
> > > + * The driver must be able to support all the versions from the earliest
> > > + * supported FFA_MIN_VERSION to the latest supported FFA_DRIVER_VERSION.
> > > + * The specification states that if firmware supports a FFA implementation
> > > + * that is incompatible with and at a greater version number than specified
> > > + * by the caller(FFA_DRIVER_VERSION passed as parameter to FFA_VERSION),
> > > + * it must return the NOT_SUPPORTED error code.
> >
> > From what I understand from the specification (v1.1 beta0) it may also
> > return its version in this case.
> >
>
> Yes we are fixing that in the spec. Basically return a version higher than the
> caller version only if it can work just fine at the caller version. It must
> return NOT_SUPPORTED otherwise.

OK

>
> > > + */
> > > +static u32 ffa_compatible_version_find(u32 version)
> > > +{
> > > +       u32 compat_version;
> > > +       u16 major = MAJOR_VERSION(version), minor = MINOR_VERSION(version);
> > > +       u16 drv_major = MAJOR_VERSION(FFA_DRIVER_VERSION);
> > > +       u16 drv_minor = MINOR_VERSION(FFA_DRIVER_VERSION);
> > > +
> > > +       if ((major < drv_major) || (major == drv_major && minor <= drv_minor))
> > > +               return version;
> >
> > A mismatch in the major version number makes the versions
> > incompatible. There's no recovery from this, an error code must be
> > returned instead.
> >
>
> The point is if the firmware can operate at reduced functionality, why just
> return error. Most other specs operate with that assumption.
> e.g. if we just add few extra functionality in say v2.0 vs v1.y, would
> you not want the v1.y driver and firmware v2.0 work together ? Especially
> if we give the flexibility for the firmware to decide that and return
> the version as v2.0 in response to v1.y by the caller.
>
> If firmware is completely incompatible, it still has option top return
> NOT_SUPPORTED.

OK, how do you anticipate that this will work in practice? I mean how
will the driver know which functions are compatible in a v2.0 api when
the driver itself only is at v1.1? Which parts are available at
reduced functionality?

Cheers,
Jens