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

[Jens Wiklander]

On Fri, Oct 15, 2021 at 7:21 PM Sudeep Holla <sudeep.holla at arm.com> wrote:
>
> On Fri, Oct 15, 2021 at 02:49:02PM +0200, Jens Wiklander wrote:
> > On Fri, Oct 15, 2021 at 1:23 PM Sudeep Holla <sudeep.holla at arm.com> wrote:
> > >
> > > On Thu, Oct 14, 2021 at 10:55:01AM +0200, Jens Wiklander wrote:
> > > > 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(-)
> > > > > > >
> > >
> > > [...]
> > >
> > > > > > > +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?
> > > >
> > >
> > > Sorry I wasn't clear earlier. The driver is just aware of v1.1 and nothing
> > > more. The firmware implementing FF-A has been upgraded to v2.0 for example
> > > and must have knowledge that it implements everything mandatory(not
> > > discoverable) functionality as per v1.1. If it does, it can advertise it
> > > supports v2.0 in response to the caller specifying v1.1 which for the
> > > caller means the firmware can operate well with v1.1 even though from
> > > firmware perspective it is reduced functionality.
> >
> > In this case it seems that v2.0 is backwards compatible. Why was the
> > major version number increased then? It seems more natural to only
> > increase the major version number when it's actually needed.
> >
>
> Do you mean to say if the caller(driver in this case) passes v1.x and
> even if firmware implements v2.0, it must return what caller asked ?

No, I just found it curious that the major version number might be
increased even though it's still backwards compatible.

> Sounds good enough but at-least my opinion here is more to let know that
> driver can be upgraded if available and required.
>
> E.g. vendor may be stuck with some old version of the kernel but
> the firmware is latest. This way we can advertise the version of firmware
> so that one can upgrade if the driver changes are available in future
> kernel versions for example.
>
> > >
> > > On the other hand, if v2.0 spec changes removed some of the mandatory
> > > functionality in v1.1 or v1.x, then firmware can return NOT_SUPPORTED
> > > to the caller with v1.x as argument or it may implement all the necessary
> > > APIs and return its version to indicate that it is fine to interact and
> > > provide functionality(though all functionality may not be used by the
> > > caller as caller is unware in this case).
> >
> > I get the incompatible case, but not the latter when it's fine to
> > interact. Is the latter the same case as described further up with a
> > backwards compatible change?
> >
>
> Not sure if we are aligned with the configurations we are talking here.
> I see 3 possibility(not considering major and minor version details for
> simplicity)
> 1. Firmware <= Driver
>         Driver must match and operate at firmware version
> 2. Firmware > Driver and Firmware OK with Driver version
>         Returns implemented version but driver uses its higher supported
>         version which was used in FFA_VERSION call
> 3. Firmware > Driver and Firmware not OK(KO) with Driver version
>         Returns NOT_SUPPORTED and driver halts there
>
> Let me know if this improves the confusion.
>
> > >
> > > Hope I didn't add more confusions and this clarifies what is the intention
> > > here.
> >
> > Is this about allowing to increase the major number even when it's
> > backwards compatible with the previous version?
> >
>
> Yes, to let the caller know the actual firmware version(which may not be
> of much use!)

Got it now, thanks for your patience!

Reviewed-by: Jens Wiklander <jens.wiklander at linaro.org>

Cheers,
Jens