docs: driver-api: firmware: add driver firmware guidelines. (v2)

From: Dave Airlie <airlied@redhat.com>

A recent snafu where Intel ignored upstream feedback on a firmware
change, led to a late rc6 fix being required. In order to avoid this
in the future we should document some expectations around
linux-firmware.

I was originally going to write this for drm, but it seems quite generic
advice.

v2: rewritten with suggestions from Thorsten Leemhuis.

Acked-by: Luis Chamberlain <mcgrof@kernel.org>
Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
 Documentation/driver-api/firmware/core.rst    |  1 +
 .../firmware/firmware-usage-guidelines.rst    | 34 +++++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100644 Documentation/driver-api/firmware/firmware-usage-guidelines.rst

Em Tue, 19 Jul 2022 16:53:57 +1000
Dave Airlie <airlied@gmail.com> escreveu:

> From: Dave Airlie <airlied@redhat.com>
> 
> A recent snafu where Intel ignored upstream feedback on a firmware
> change, led to a late rc6 fix being required. In order to avoid this
> in the future we should document some expectations around
> linux-firmware.
> 
> I was originally going to write this for drm, but it seems quite generic
> advice.

Indeed it makes sense to document firmware API compatibility in a generic way.

Some suggestions below.

> v2: rewritten with suggestions from Thorsten Leemhuis.
>
> Acked-by: Luis Chamberlain <mcgrof@kernel.org>
> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  Documentation/driver-api/firmware/core.rst    |  1 +
>  .../firmware/firmware-usage-guidelines.rst    | 34 +++++++++++++++++++
>  2 files changed, 35 insertions(+)
>  create mode 100644 Documentation/driver-api/firmware/firmware-usage-guidelines.rst
> 
> diff --git a/Documentation/driver-api/firmware/core.rst b/Documentation/driver-api/firmware/core.rst
> index 1d1688cbc078..803cd574bbd7 100644
> --- a/Documentation/driver-api/firmware/core.rst
> +++ b/Documentation/driver-api/firmware/core.rst
> @@ -13,4 +13,5 @@ documents these features.
>     direct-fs-lookup
>     fallback-mechanisms
>     lookup-order
> +   firmware-usage-guidelines
>  
> diff --git a/Documentation/driver-api/firmware/firmware-usage-guidelines.rst b/Documentation/driver-api/firmware/firmware-usage-guidelines.rst
> new file mode 100644
> index 000000000000..34d2412e78c6
> --- /dev/null
> +++ b/Documentation/driver-api/firmware/firmware-usage-guidelines.rst
> @@ -0,0 +1,34 @@
> +===================
> +Firmware Guidelines
> +===================
> +
> +Drivers that use firmware from linux-firmware should attempt to follow
> +the rules in this guide.
> +
> +* Firmware should be versioned with at least a major/minor version.

It is hard to enforce how vendors will version their firmware. On media,
we have some drivers whose major means different hardware versions. For
instance, on xc3028, v3.x means low voltage chips, while v2.x means
"normal" voltage. We end changing the file name on Linux to avoid the risk
of damaging the hardware, as using v27 firmware on low power chips damage
them. So, we have:

	drivers/media/tuners/xc2028.h:#define XC2028_DEFAULT_FIRMWARE "xc3028-v27.fw"
	drivers/media/tuners/xc2028.h:#define XC3028L_DEFAULT_FIRMWARE "xc3028L-v36.fw"

As their main market is not Linux - nor PC - as their main sales are on 
TV sets, and them don't officially support Linux, there's nothing we can
do to enforce it.

IMO we need a more generic text here to indicate that Linux firmware
files should be defined in a way that it should be possible to detect
when there are incompatibilities with past versions. 
So, I would say, instead:

	Firmware files shall be designed in a way that it allows
	checking for firmware ABI version changes. It is recommended
	that firmware files to be versioned with at least major/minor
	version.

> It
> +  is suggested that the firmware files in linux-firmware be named with
> +  some device specific name, and just the major version. 

> The
> +  major/minor/patch versions should be stored in a header in the
> +  firmware file for the driver to detect any non-ABI fixes/issues. 

I would also make this more generic. On media, we ended adding the firmware
version indicated at the file name. For instance, xc4000 driver checks for
two firmware files:

drivers/media/tuners/xc4000.c:#define XC4000_DEFAULT_FIRMWARE "dvb-fe-xc4000-1.4.fw"
drivers/media/tuners/xc4000.c:#define XC4000_DEFAULT_FIRMWARE_NEW "dvb-fe-xc4000-1.4.1.fw"

On such cases, the driver can take decisions based on the firmware name.

I would change the text to be more generic covering both cases:

	The firmware version shall either be stored at the firmware
	header or as part of the firmware file name, in order to let the
	driver to detect any non-ABI fixes/changes.

> The
> +  firmware files in linux-firmware should be overwritten with the newest
> +  compatible major version.

For me "shall" is mandatory, while "should" is optional.

In this specific case, I'm not so sure if overriding it is the best thing 
to do on all subsystems. I mean, even with the same ABI, older firmware 
usually means that some bugs and/or limitations will be present there.

That's specially true on codecs: even having the same ABI, older versions
won't support decoding newer protocols. We have one case with some
digital TV decoders that only support some Cable-TV protocols with
newer firmware versions. We have also one case were remote controller
decoding is buggy with older firmwares. On both situations, the ABI
didn't change.

> Newer major version firmware should remain
> +  compatible with all kernels that load that major number.

	should -> shall

> +
> +* Users should *not* have to install newer firmware to use existing
> +  hardware when they install a newer kernel. 

> If the hardware isn't
> +  enabled by default or under development,

Hmm.. someone might understand that not having a "default Y" at Kconfig
would mean that this is not enabled by default ;-)

IMO you can just tell, instead:

	"This can be ignored until the first kernel release that enables support
	 for such hardware."

> this can be ignored, until
> +  the first kernel release that enables that hardware. 

> This means no
> +  major version bumps without the kernel retaining backwards
> +  compatibility for the older major versions.  Minor version bumps
> +  should not introduce new features that newer kernels depend on
> +  non-optionally.
> +
> +* If a security fix needs lockstep firmware and kernel fixes in order to
> +  be successful, then all supported major versions in the linux-firmware
> +  repo should be updated with the security fix, and the kernel patches
> +  should detect if the firmware is new enough to declare if the security
> +  issue is fixed.  All communications around security fixes should point
> +  at both the firmware and kernel fixes. If a security fix requires
> +  deprecating old major versions, then this should only be done as a
> +  last option, and be stated clearly in all communications.
> +

Perfect!

Regards,
Mauro

On 2022-07-19 02:53, Dave Airlie wrote:
> From: Dave Airlie <airlied@redhat.com>
> 
> A recent snafu where Intel ignored upstream feedback on a firmware
> change, led to a late rc6 fix being required. In order to avoid this
> in the future we should document some expectations around
> linux-firmware.
> 
> I was originally going to write this for drm, but it seems quite generic
> advice.
> 
> v2: rewritten with suggestions from Thorsten Leemhuis.
> 
> Acked-by: Luis Chamberlain <mcgrof@kernel.org>
> Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Acked-by: Harry Wentland <harry.wentland@amd.com>

Harry

> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
>  Documentation/driver-api/firmware/core.rst    |  1 +
>  .../firmware/firmware-usage-guidelines.rst    | 34 +++++++++++++++++++
>  2 files changed, 35 insertions(+)
>  create mode 100644 Documentation/driver-api/firmware/firmware-usage-guidelines.rst
> 
> diff --git a/Documentation/driver-api/firmware/core.rst b/Documentation/driver-api/firmware/core.rst
> index 1d1688cbc078..803cd574bbd7 100644
> --- a/Documentation/driver-api/firmware/core.rst
> +++ b/Documentation/driver-api/firmware/core.rst
> @@ -13,4 +13,5 @@ documents these features.
>     direct-fs-lookup
>     fallback-mechanisms
>     lookup-order
> +   firmware-usage-guidelines
>  
> diff --git a/Documentation/driver-api/firmware/firmware-usage-guidelines.rst b/Documentation/driver-api/firmware/firmware-usage-guidelines.rst
> new file mode 100644
> index 000000000000..34d2412e78c6
> --- /dev/null
> +++ b/Documentation/driver-api/firmware/firmware-usage-guidelines.rst
> @@ -0,0 +1,34 @@
> +===================
> +Firmware Guidelines
> +===================
> +
> +Drivers that use firmware from linux-firmware should attempt to follow
> +the rules in this guide.
> +
> +* Firmware should be versioned with at least a major/minor version. It
> +  is suggested that the firmware files in linux-firmware be named with
> +  some device specific name, and just the major version. The
> +  major/minor/patch versions should be stored in a header in the
> +  firmware file for the driver to detect any non-ABI fixes/issues. The
> +  firmware files in linux-firmware should be overwritten with the newest
> +  compatible major version. Newer major version firmware should remain
> +  compatible with all kernels that load that major number.
> +
> +* Users should *not* have to install newer firmware to use existing
> +  hardware when they install a newer kernel.  If the hardware isn't
> +  enabled by default or under development, this can be ignored, until
> +  the first kernel release that enables that hardware.  This means no
> +  major version bumps without the kernel retaining backwards
> +  compatibility for the older major versions.  Minor version bumps
> +  should not introduce new features that newer kernels depend on
> +  non-optionally.
> +
> +* If a security fix needs lockstep firmware and kernel fixes in order to
> +  be successful, then all supported major versions in the linux-firmware
> +  repo should be updated with the security fix, and the kernel patches
> +  should detect if the firmware is new enough to declare if the security
> +  issue is fixed.  All communications around security fixes should point
> +  at both the firmware and kernel fixes. If a security fix requires
> +  deprecating old major versions, then this should only be done as a
> +  last option, and be stated clearly in all communications.
> +

> It is hard to enforce how vendors will version their firmware. On media,
> we have some drivers whose major means different hardware versions. For
> instance, on xc3028, v3.x means low voltage chips, while v2.x means
> "normal" voltage. We end changing the file name on Linux to avoid the risk
> of damaging the hardware, as using v27 firmware on low power chips damage
> them. So, we have:
>
>         drivers/media/tuners/xc2028.h:#define XC2028_DEFAULT_FIRMWARE "xc3028-v27.fw"
>         drivers/media/tuners/xc2028.h:#define XC3028L_DEFAULT_FIRMWARE "xc3028L-v36.fw"
>
> As their main market is not Linux - nor PC - as their main sales are on
> TV sets, and them don't officially support Linux, there's nothing we can
> do to enforce it.
>
> IMO we need a more generic text here to indicate that Linux firmware
> files should be defined in a way that it should be possible to detect
> when there are incompatibilities with past versions.
> So, I would say, instead:
>
>         Firmware files shall be designed in a way that it allows
>         checking for firmware ABI version changes. It is recommended
>         that firmware files to be versioned with at least major/minor
>         version.

This sounds good, will update with this.

>
> > It
> > +  is suggested that the firmware files in linux-firmware be named with
> > +  some device specific name, and just the major version.
>
> > The
> > +  major/minor/patch versions should be stored in a header in the
> > +  firmware file for the driver to detect any non-ABI fixes/issues.
>
> I would also make this more generic. On media, we ended adding the firmware
> version indicated at the file name. For instance, xc4000 driver checks for
> two firmware files:
>
> drivers/media/tuners/xc4000.c:#define XC4000_DEFAULT_FIRMWARE "dvb-fe-xc4000-1.4.fw"
> drivers/media/tuners/xc4000.c:#define XC4000_DEFAULT_FIRMWARE_NEW "dvb-fe-xc4000-1.4.1.fw"

This is probably fine for products where development never produces
much firmwares, but it quickly becomes unmanageable when you end up
with _NEW_NEW_NEW etc.

I'd rather not encourage this sort of thing unless it is totally
outside our control. So I'd like to keep the guidelines for when we
have some control what we'd recommend.

In this case I'd have recommended you put the 1.4.1 in the header of
the fw, and just have it called dvb-fe-xc4000-1.fw and overwrite the
NEW with the OLD, I understand we likely don't have the control here.

> > +  firmware files in linux-firmware should be overwritten with the newest
> > +  compatible major version.
>
> For me "shall" is mandatory, while "should" is optional.
>
> In this specific case, I'm not so sure if overriding it is the best thing
> to do on all subsystems. I mean, even with the same ABI, older firmware
> usually means that some bugs and/or limitations will be present there.

As long as you can detect the minor/patch versions from the firmware
file after loading it you should be able to do sufficient workarounds.
>
> That's specially true on codecs: even having the same ABI, older versions
> won't support decoding newer protocols. We have one case with some
> digital TV decoders that only support some Cable-TV protocols with
> newer firmware versions. We have also one case were remote controller
> decoding is buggy with older firmwares. On both situations, the ABI
> didn't change.

If the only way to figure that out is by the filename or minor
version, then so be it, but where people have some control I'd rather
provide some harder guidelines.

Dave.