* [Intel-gfx] [bug report] drm/i915/guc: Use GuC submission API version number
@ 2023-03-24 9:12 Dan Carpenter
2023-04-21 17:49 ` John Harrison
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2023-03-24 9:12 UTC (permalink / raw)
To: John.C.Harrison; +Cc: intel-gfx
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'
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [Intel-gfx] [bug report] drm/i915/guc: Use GuC submission API version number
2023-03-24 9:12 [Intel-gfx] [bug report] drm/i915/guc: Use GuC submission API version number Dan Carpenter
@ 2023-04-21 17:49 ` John Harrison
0 siblings, 0 replies; 2+ messages in thread
From: John Harrison @ 2023-04-21 17:49 UTC (permalink / raw)
To: Dan Carpenter; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-04-21 17:49 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-24 9:12 [Intel-gfx] [bug report] drm/i915/guc: Use GuC submission API version number Dan Carpenter
2023-04-21 17:49 ` John Harrison
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox