Re: [Intel-gfx] [bug report] drm/i915/guc: Use GuC submission API version number

[John Harrison <john.c.harrison@intel.com>]

On 3/24/2023 02:12, Dan Carpenter wrote:
> Hello John Harrison,
>
> The patch 9bbba0667f37: "drm/i915/guc: Use GuC submission API version
> number" from Nov 29, 2022, leads to the following Smatch static
> checker warning:
>
> 	drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c:821 intel_uc_fw_fetch()
> 	warn: passing zero to 'ERR_PTR'
Doh! Yes. That is a bug. It's an error path that should never be hit - 
it means a firmware file has been released with a version number that 
breaks the spec or is simply corrupted data. So it's a low risk bug but 
it should still be fixed. I'll post a patch...

Thanks,
John.

>
> drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
>      709 int intel_uc_fw_fetch(struct intel_uc_fw *uc_fw)
>      710 {
>      711         struct intel_gt *gt = __uc_fw_to_gt(uc_fw);
>      712         struct drm_i915_private *i915 = gt->i915;
>      713         struct intel_uc_fw_file file_ideal;
>      714         struct drm_i915_gem_object *obj;
>      715         const struct firmware *fw = NULL;
>      716         bool old_ver = false;
>      717         int err;
>      718
>      719         GEM_BUG_ON(!gt->wopcm.size);
>      720         GEM_BUG_ON(!intel_uc_fw_is_enabled(uc_fw));
>      721
>      722         err = i915_inject_probe_error(i915, -ENXIO);
>      723         if (err)
>      724                 goto fail;
>      725
>      726         __force_fw_fetch_failures(uc_fw, -EINVAL);
>      727         __force_fw_fetch_failures(uc_fw, -ESTALE);
>      728
>      729         err = try_firmware_load(uc_fw, &fw);
>      730         memcpy(&file_ideal, &uc_fw->file_wanted, sizeof(file_ideal));
>      731
>      732         /* Any error is terminal if overriding. Don't bother searching for older versions */
>      733         if (err && intel_uc_fw_is_overridden(uc_fw))
>      734                 goto fail;
>      735
>      736         while (err == -ENOENT) {
>      737                 old_ver = true;
>      738
>      739                 __uc_fw_auto_select(i915, uc_fw);
>      740                 if (!uc_fw->file_selected.path) {
>      741                         /*
>      742                          * No more options! But set the path back to something
>      743                          * valid just in case it gets dereferenced.
>      744                          */
>      745                         uc_fw->file_selected.path = file_ideal.path;
>      746
>      747                         /* Also, preserve the version that was really wanted */
>      748                         memcpy(&uc_fw->file_wanted, &file_ideal, sizeof(uc_fw->file_wanted));
>      749                         break;
>      750                 }
>      751
>      752                 err = try_firmware_load(uc_fw, &fw);
>      753         }
>      754
>      755         if (err)
>      756                 goto fail;
>      757
>      758         err = check_fw_header(gt, fw, uc_fw);
>      759         if (err)
>      760                 goto fail;
>      761
>      762         if (uc_fw->type == INTEL_UC_FW_TYPE_GUC && !guc_check_version_range(uc_fw))
>      763                 goto fail;
>                          ^^^^^^^^^
> Should "err" be set on this path?
>
>
>      764
>      765         if (uc_fw->file_wanted.ver.major && uc_fw->file_selected.ver.major) {
>      766                 /* Check the file's major version was as it claimed */
>      767                 if (uc_fw->file_selected.ver.major != uc_fw->file_wanted.ver.major) {
>      768                         gt_notice(gt, "%s firmware %s: unexpected version: %u.%u != %u.%u\n",
>      769                                   intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path,
>      770                                   uc_fw->file_selected.ver.major, uc_fw->file_selected.ver.minor,
>      771                                   uc_fw->file_wanted.ver.major, uc_fw->file_wanted.ver.minor);
>      772                         if (!intel_uc_fw_is_overridden(uc_fw)) {
>      773                                 err = -ENOEXEC;
>      774                                 goto fail;
>      775                         }
>      776                 } else {
>      777                         if (uc_fw->file_selected.ver.minor < uc_fw->file_wanted.ver.minor)
>      778                                 old_ver = true;
>      779                 }
>      780         }
>      781
>      782         if (old_ver && uc_fw->file_selected.ver.major) {
>      783                 /* Preserve the version that was really wanted */
>      784                 memcpy(&uc_fw->file_wanted, &file_ideal, sizeof(uc_fw->file_wanted));
>      785
>      786                 gt_notice(gt, "%s firmware %s (%d.%d) is recommended, but only %s (%d.%d) was found\n",
>      787                           intel_uc_fw_type_repr(uc_fw->type),
>      788                           uc_fw->file_wanted.path,
>      789                           uc_fw->file_wanted.ver.major, uc_fw->file_wanted.ver.minor,
>      790                           uc_fw->file_selected.path,
>      791                           uc_fw->file_selected.ver.major, uc_fw->file_selected.ver.minor);
>      792                 gt_info(gt, "Consider updating your linux-firmware pkg or downloading from %s\n",
>      793                         INTEL_UC_FIRMWARE_URL);
>      794         }
>      795
>      796         if (HAS_LMEM(i915)) {
>      797                 obj = i915_gem_object_create_lmem_from_data(i915, fw->data, fw->size);
>      798                 if (!IS_ERR(obj))
>      799                         obj->flags |= I915_BO_ALLOC_PM_EARLY;
>      800         } else {
>      801                 obj = i915_gem_object_create_shmem_from_data(i915, fw->data, fw->size);
>      802         }
>      803
>      804         if (IS_ERR(obj)) {
>      805                 err = PTR_ERR(obj);
>      806                 goto fail;
>      807         }
>      808
>      809         uc_fw->obj = obj;
>      810         uc_fw->size = fw->size;
>      811         intel_uc_fw_change_status(uc_fw, INTEL_UC_FIRMWARE_AVAILABLE);
>      812
>      813         release_firmware(fw);
>      814         return 0;
>      815
>      816 fail:
>      817         intel_uc_fw_change_status(uc_fw, err == -ENOENT ?
>      818                                   INTEL_UC_FIRMWARE_MISSING :
>      819                                   INTEL_UC_FIRMWARE_ERROR);
>      820
> --> 821         gt_probe_error(gt, "%s firmware %s: fetch failed %pe\n",
>      822                        intel_uc_fw_type_repr(uc_fw->type), uc_fw->file_selected.path, ERR_PTR(err));
>      823         gt_info(gt, "%s firmware(s) can be downloaded from %s\n",
>      824                 intel_uc_fw_type_repr(uc_fw->type), INTEL_UC_FIRMWARE_URL);
>      825
>      826         release_firmware(fw);                /* OK even if fw is NULL */
>      827         return err;
>      828 }
>
> regards,
> dan carpenter