[Intel-gfx] [bug report] drm/i915/guc: Use GuC submission API version number
John Harrison
john.c.harrison at intel.com
Fri Apr 21 17:49:10 UTC 2023
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
More information about the Intel-gfx
mailing list