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

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

[Sudeep Holla]

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@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.
+ */
+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;
+
+	pr_info("Firmware version higher than driver version, downgrading\n");
+	return FFA_DRIVER_VERSION;
+}
+
 static int ffa_version_check(u32 *version)
 {
 	ffa_value_t ver;
@@ -180,15 +202,20 @@ static int ffa_version_check(u32 *version)
 		return -EOPNOTSUPP;
 	}
 
-	if (ver.a0 < FFA_MIN_VERSION || ver.a0 > FFA_DRIVER_VERSION) {
-		pr_err("Incompatible version %d.%d found\n",
-		       MAJOR_VERSION(ver.a0), MINOR_VERSION(ver.a0));
+	if (ver.a0 < FFA_MIN_VERSION) {
+		pr_err("Incompatible v%d.%d! Earliest supported v%d.%d\n",
+		       MAJOR_VERSION(ver.a0), MINOR_VERSION(ver.a0),
+		       MAJOR_VERSION(FFA_MIN_VERSION),
+		       MINOR_VERSION(FFA_MIN_VERSION));
 		return -EINVAL;
 	}
 
-	*version = ver.a0;
-	pr_info("Version %d.%d found\n", MAJOR_VERSION(ver.a0),
+	pr_info("Driver version %d.%d\n", MAJOR_VERSION(FFA_DRIVER_VERSION),
+		MINOR_VERSION(FFA_DRIVER_VERSION));
+	pr_info("Firmware version %d.%d found\n", MAJOR_VERSION(ver.a0),
 		MINOR_VERSION(ver.a0));
+	*version = ffa_compatible_version_find(ver.a0);
+
 	return 0;
 }
 
-- 
2.25.1


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

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

[Jens Wiklander]

On Wed, Oct 13, 2021 at 11:11 AM Sudeep Holla <sudeep.holla@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@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.

> + */
> +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.

Cheers,
Jens

> +
> +       pr_info("Firmware version higher than driver version, downgrading\n");
> +       return FFA_DRIVER_VERSION;
> +}
> +
>  static int ffa_version_check(u32 *version)
>  {
>         ffa_value_t ver;
> @@ -180,15 +202,20 @@ static int ffa_version_check(u32 *version)
>                 return -EOPNOTSUPP;
>         }
>
> -       if (ver.a0 < FFA_MIN_VERSION || ver.a0 > FFA_DRIVER_VERSION) {
> -               pr_err("Incompatible version %d.%d found\n",
> -                      MAJOR_VERSION(ver.a0), MINOR_VERSION(ver.a0));
> +       if (ver.a0 < FFA_MIN_VERSION) {
> +               pr_err("Incompatible v%d.%d! Earliest supported v%d.%d\n",
> +                      MAJOR_VERSION(ver.a0), MINOR_VERSION(ver.a0),
> +                      MAJOR_VERSION(FFA_MIN_VERSION),
> +                      MINOR_VERSION(FFA_MIN_VERSION));
>                 return -EINVAL;
>         }
>
> -       *version = ver.a0;
> -       pr_info("Version %d.%d found\n", MAJOR_VERSION(ver.a0),
> +       pr_info("Driver version %d.%d\n", MAJOR_VERSION(FFA_DRIVER_VERSION),
> +               MINOR_VERSION(FFA_DRIVER_VERSION));
> +       pr_info("Firmware version %d.%d found\n", MAJOR_VERSION(ver.a0),
>                 MINOR_VERSION(ver.a0));
> +       *version = ffa_compatible_version_find(ver.a0);
> +
>         return 0;
>  }
>
> --
> 2.25.1
>

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

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

[Sudeep Holla]

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@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@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.

> > + */
> > +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.

--
Regards,
Sudeep

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

Re: [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@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@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@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

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

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

[Sudeep Holla]

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@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@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@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.

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).

Hope I didn't add more confusions and this clarifies what is the intention
here.

--
Regards,
Sudeep

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

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

[Jens Wiklander]

On Fri, Oct 15, 2021 at 1:23 PM Sudeep Holla <sudeep.holla@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@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@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@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.

>
> 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?

>
> 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?

Cheers,
Jens

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

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

[Sudeep Holla]

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@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@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@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@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 ?
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!)

-- 
Regards,
Sudeep

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

Re: [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@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@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@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@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@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@linaro.org>

Cheers,
Jens

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

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

[Sudeep Holla]

On Wed, 13 Oct 2021 10:11:27 +0100, Sudeep Holla 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.
> 
> [...]


Applied to sudeep.holla/linux (for-next/ffa), thanks!

[1/1] firmware: arm_ffa: Handle compatibility with different firmware versions
      https://git.kernel.org/sudeep.holla/c/8e3f9da608

--
Regards,
Sudeep


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