Re: [PATCH 6/6] drm/i915/uc: Make unexpected firmware versions an error in debug builds

[John Harrison <john.c.harrison@xxxxxxxxx>]

On 4/28/2023 17:19, Ceraolo Spurio, Daniele wrote:
> On 4/20/2023 6:15 PM, John.C.Harrison@xxxxxxxxx wrote:
> > From: John Harrison <John.C.Harrison@xxxxxxxxx>
> >
> > If the DEBUG_GEM config option is set then escalate the 'unexpected
> > firmware version' message from a notice to an error. This will ensure
> > that the CI system treats such occurences as a failure and logs a bug
> > about it (or fails the pre-merge testing).
> >
> > Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 34 ++++++++++++++----------
> >   1 file changed, 20 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c 
> > b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> > index bc4011d55667c..c5b21d17ca437 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> > @@ -17,6 +17,12 @@
> >   #include "i915_drv.h"
> >   #include "i915_reg.h"
> >   +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
> > +#define UNEXPECTED    gt_err
> > +#else
> > +#define UNEXPECTED    gt_notice
>
> I tried to find an alternative word that had the same number of 
> characters as "gt_notice" so that we could avoid the indent in the 
> diff, but nothing came to mind, so:
Yup, already went through that! And it didn't seem worth it to 
abbreviate by dropping a single random vowel.

John.


> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
>
> Daniele
>
> > +#endif
> > +
> >   static inline struct intel_gt *
> >   ____uc_fw_to_gt(struct intel_uc_fw *uc_fw, enum intel_uc_fw_type type)
> >   {
> > @@ -828,10 +834,10 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
> >       if (uc_fw->file_wanted.ver.major && 
> > uc_fw->file_selected.ver.major) {
> >           /* Check the file's major version was as it claimed */
> >           if (uc_fw->file_selected.ver.major != 
> > uc_fw->file_wanted.ver.major) {
> > -            gt_notice(gt, "%s firmware %s: unexpected version: %u.%u 
> > != %u.%u\n",
> > -                  intel_uc_fw_type_repr(uc_fw->type), 
> > uc_fw->file_selected.path,
> > -                  uc_fw->file_selected.ver.major, 
> > uc_fw->file_selected.ver.minor,
> > -                  uc_fw->file_wanted.ver.major, 
> > uc_fw->file_wanted.ver.minor);
> > +            UNEXPECTED(gt, "%s firmware %s: unexpected version: 
> > %u.%u != %u.%u\n",
> > +                   intel_uc_fw_type_repr(uc_fw->type), 
> > uc_fw->file_selected.path,
> > +                   uc_fw->file_selected.ver.major, 
> > uc_fw->file_selected.ver.minor,
> > +                   uc_fw->file_wanted.ver.major, 
> > uc_fw->file_wanted.ver.minor);
> >               if (!intel_uc_fw_is_overridden(uc_fw)) {
> >                   err = -ENOEXEC;
> >                   goto fail;
> > @@ -849,16 +855,16 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
> >           /* Preserve the version that was really wanted */
> >           memcpy(&uc_fw->file_wanted, &file_ideal, 
> > sizeof(uc_fw->file_wanted));
> >   -        gt_notice(gt, "%s firmware %s (%d.%d.%d) is recommended, 
> > but only %s (%d.%d.%d) was found\n",
> > -              intel_uc_fw_type_repr(uc_fw->type),
> > -              uc_fw->file_wanted.path,
> > -              uc_fw->file_wanted.ver.major,
> > -              uc_fw->file_wanted.ver.minor,
> > -              uc_fw->file_wanted.ver.patch,
> > -              uc_fw->file_selected.path,
> > -              uc_fw->file_selected.ver.major,
> > -              uc_fw->file_selected.ver.minor,
> > -              uc_fw->file_selected.ver.patch);
> > +        UNEXPECTED(gt, "%s firmware %s (%d.%d.%d) is recommended, 
> > but only %s (%d.%d.%d) was found\n",
> > +               intel_uc_fw_type_repr(uc_fw->type),
> > +               uc_fw->file_wanted.path,
> > +               uc_fw->file_wanted.ver.major,
> > +               uc_fw->file_wanted.ver.minor,
> > +               uc_fw->file_wanted.ver.patch,
> > +               uc_fw->file_selected.path,
> > +               uc_fw->file_selected.ver.major,
> > +               uc_fw->file_selected.ver.minor,
> > +               uc_fw->file_selected.ver.patch);
> >           gt_info(gt, "Consider updating your linux-firmware pkg or 
> > downloading from %s\n",
> >               INTEL_UC_FIRMWARE_URL);
> >       }