Re: [PATCH 7/7] drm/i915/uc: Hardening firmware fetch

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Michal Wajdeczko (2019-08-07 18:00:34)
> Insert few more failure points into firmware fetch procedure to check
> use of the wrong blob name or use of the mismatched firmware versions.
> 
> Also update some messages (remove ptr, duplicated infos) and stop
> treating all fetch errors as missing firmware case.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 137 ++++++++++++++++-------
>  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h |   3 +
>  2 files changed, 97 insertions(+), 43 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 3a3803bfa5a8..4faff1010398 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> @@ -153,20 +153,23 @@ static const char *__override_huc_firmware_path(void)
>         return "";
>  }
>  
> -static bool
> -__uc_fw_override(struct intel_uc_fw *uc_fw)
> +static void __uc_fw_user_override(struct intel_uc_fw *uc_fw)
>  {
> +       const char *path = NULL;
> +
>         switch (uc_fw->type) {
>         case INTEL_UC_FW_TYPE_GUC:
> -               uc_fw->path = __override_guc_firmware_path();
> +               path = __override_guc_firmware_path();
>                 break;
>         case INTEL_UC_FW_TYPE_HUC:
> -               uc_fw->path = __override_huc_firmware_path();
> +               path = __override_huc_firmware_path();
>                 break;
>         }
>  
> -       uc_fw->user_overridden = uc_fw->path;
> -       return uc_fw->user_overridden;
> +       if (unlikely(path)) {
> +               uc_fw->path = path;
> +               uc_fw->user_overridden = true;
> +       }

The code got longer for no real improvement in clarity?
Oh, because now uc_fw->path may be non-NULL on entry due to call
reordering.

>  }
>  
>  /**
> @@ -194,8 +197,10 @@ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
>  
>         uc_fw->type = type;
>  
> -       if (supported && likely(!__uc_fw_override(uc_fw)))
> +       if (supported) {
>                 __uc_fw_auto_select(uc_fw, platform, rev);
> +               __uc_fw_user_override(uc_fw);
> +       }
>  
>         if (uc_fw->path && *uc_fw->path)
>                 uc_fw->status = INTEL_UC_FIRMWARE_SELECTED;
> @@ -203,6 +208,41 @@ void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
>                 uc_fw->status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
>  }
>  
> +static void __force_fw_fetch_failures(struct intel_uc_fw *uc_fw,
> +                                     struct drm_i915_private *i915, bool user)
> +{
> +       int e = user ? -EINVAL : -ESTALE;
> +
> +       if (i915_inject_load_error(i915, e)) {
> +               /* non-existing blob */
> +               uc_fw->path = "<invalid>";
> +               uc_fw->user_overridden = user;
> +       } else if (i915_inject_load_error(i915, e)) {
> +               /* require next major version */
> +               uc_fw->major_ver_wanted += 1;
> +               uc_fw->minor_ver_wanted = 0;
> +               uc_fw->user_overridden = user;
> +       } else if (i915_inject_load_error(i915, e)) {
> +               /* require next minor version */
> +               uc_fw->minor_ver_wanted += 1;
> +               uc_fw->user_overridden = user;
> +       } else if (uc_fw->major_ver_wanted && i915_inject_load_error(i915, e)) {
> +               /* require prev major version */
> +               uc_fw->major_ver_wanted -= 1;
> +               uc_fw->minor_ver_wanted = 0;
> +               uc_fw->user_overridden = user;
> +       } else if (uc_fw->minor_ver_wanted && i915_inject_load_error(i915, e)) {
> +               /* require prev minor version - hey, this should work! */
> +               uc_fw->minor_ver_wanted -= 1;
> +               uc_fw->user_overridden = user;
> +       } else if (user && i915_inject_load_error(i915, e)) {
> +               /* officially unsupported platform */
> +               uc_fw->major_ver_wanted = 0;
> +               uc_fw->minor_ver_wanted = 0;
> +               uc_fw->user_overridden = true;
> +       }

Taking self-abuse to a whole new level :)

> +}
> +
>  /**
>   * intel_uc_fw_fetch - fetch uC firmware
>   * @uc_fw: uC firmware
> @@ -222,17 +262,22 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw, struct drm_i915_private *i915)
>  
>         GEM_BUG_ON(!intel_uc_fw_supported(uc_fw));
>  
> +       err = i915_inject_load_error(i915, -ENXIO);
> +       if (err)
> +               return err;
> +
> +       __force_fw_fetch_failures(uc_fw, i915, true);
> +       __force_fw_fetch_failures(uc_fw, i915, false);

Ok. The set of major/minor/user_override make some sort of sense to me,
but I couldn't say if that's a complete set or not.

> @@ -292,29 +343,22 @@ int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw, struct drm_i915_private *i915)
>                 break;
>         }
>  
> -       DRM_DEBUG_DRIVER("%s fw version %u.%u (wanted %u.%u)\n",
> -                        intel_uc_fw_type_repr(uc_fw->type),
> -                        uc_fw->major_ver_found, uc_fw->minor_ver_found,
> -                        uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
> -
> -       if (uc_fw->major_ver_wanted == 0 && uc_fw->minor_ver_wanted == 0) {
> -               DRM_NOTE("%s: Skipping firmware version check\n",
> -                        intel_uc_fw_type_repr(uc_fw->type));
> -       } else if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
> -                  uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
> -               DRM_NOTE("%s: Wrong firmware version (%u.%u, required %u.%u)\n",
> -                        intel_uc_fw_type_repr(uc_fw->type),
> +       if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
> +           uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
> +               dev_warn(i915->drm.dev,

Still a dev_notice(), especially as this is still user controllable.

> +                        "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
> +                        intel_uc_fw_type_repr(uc_fw->type), uc_fw->path,
>                          uc_fw->major_ver_found, uc_fw->minor_ver_found,
>                          uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
> -               err = -ENOEXEC;
> -               goto fail;
> +               if (!intel_uc_fw_is_overridden(uc_fw)) {
> +                       err = -ENOEXEC;
> +                       goto fail;
> +               }
>         }

> -       DRM_WARN("%s: Failed to fetch firmware %s (error %d)\n",
> +       dev_info(i915->drm.dev, "%s firmware %s: fetch failed with error %d\n",
>                  intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);

WARN -> info ? Not at least a notice?

> -       DRM_INFO("%s: Firmware can be downloaded from %s\n",
> +       dev_info(i915->drm.dev, "%s firmware(s) can be downloaded from %s\n",
>                  intel_uc_fw_type_repr(uc_fw->type), INTEL_UC_FIRMWARE_URL);
>  
>         release_firmware(fw);           /* OK even if fw is NULL */

Just a question of log levels.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux