From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: platform/x86/intel/vsec: Plumb ACPI PMT discovery tables through vsec Date: Fri, 27 Feb 2026 14:12:37 +1000 Message-ID: In-Reply-To: <20260224-upstream-pmt-acpi-v5-v5-6-8dd73bcf049c@linux.intel.com> References: <20260224-upstream-pmt-acpi-v5-v5-0-8dd73bcf049c@linux.intel.com> <20260224-upstream-pmt-acpi-v5-v5-6-8dd73bcf049c@linux.intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Overall: Core feature patch. Has a resource leak issue and a minor doc ty= po.** This adds the ACPI discovery infrastructure: `enum intel_vsec_disc_source`,= `acpi_disc` arrays in both `intel_vsec_platform_info` and `intel_vsec_devi= ce`, and the `src` field. **Issue 1 =E2=80=94 Potential resource leak when `kmemdup` succeeds but dev= ice registration fails:** In `intel_vsec_add_dev()`: ```c intel_vsec_dev->acpi_disc =3D kmemdup(info->acpi_disc, bytes, GFP_KERNEL); if (!intel_vsec_dev->acpi_disc) return -ENOMEM; ``` The `intel_vsec_dev` is declared with `__free(kfree)` cleanup, but `acpi_di= sc` is only freed in `intel_vsec_dev_release()` (the device release callbac= k). If `intel_vsec_add_aux()` fails later in this function *before* the dev= ice is fully registered (before `device_add` succeeds), the `__free(kfree)`= cleanup will free `intel_vsec_dev` but not `acpi_disc`, since `intel_vsec_= dev_release` won't be called. This mirrors the existing pattern for `resour= ce` though =E2=80=94 `resource` has the same issue with `no_free_ptr(res)` = =E2=80=94 so this may be acceptable if the existing pattern is considered s= afe. But it's worth verifying. Actually, looking more carefully: `intel_vsec_dev` uses `__free(kfree)` and= is later passed via `no_free_ptr()` to `intel_vsec_add_aux()`. If `no_free= _ptr()` has already been called, ownership has been transferred. But the `a= cpi_disc` allocation happens *before* `no_free_ptr()`, so if the function r= eturns early between the `kmemdup` and `no_free_ptr`, `acpi_disc` will leak= since `kfree(intel_vsec_dev)` doesn't also free `intel_vsec_dev->acpi_disc= `. **Issue 2 =E2=80=94 `check_mul_overflow` alignment:** ```c if (check_mul_overflow(intel_vsec_dev->num_resources, sizeof(*info->acpi_disc), &bytes)) ``` The `sizeof(*info->acpi_disc)` evaluates to `sizeof(u32[4])` =3D 16 bytes, = and `num_resources` is `int`. The `check_mul_overflow` macro should handle = the type promotion correctly, but `bytes` is `size_t` while `num_resources`= is `int` =E2=80=94 if `num_resources` were somehow negative, this could be= have unexpectedly. It won't happen in practice since `num_resources` is val= idated earlier, but using `(size_t)intel_vsec_dev->num_resources` would be = more defensive. **Issue 3 =E2=80=94 Comment typo in the header:** ```c * @acpi_disc: ACPI discovery tables, each entry is two QWORDs * in little-endian format as defined by the PMT ACPI spec. * Valid only when @provider =3D=3D INTEL_VSEC_DISC_ACPI. ``` `@provider` should be `@src` to match the actual field name. **Issue 4 =E2=80=94 Breaking the resource loop early:** ```c for (i =3D 0, tmp =3D res; i < header->num_entries; i++, tmp++) { if (info->src =3D=3D INTEL_VSEC_DISC_ACPI) break; ``` This `break` on ACPI effectively leaves `res` allocated but empty (all zero= ed). The `num_resources` is still set to `header->num_entries`, so downstre= am code might expect valid resources. It might be cleaner to skip the alloc= ation and loop entirely for ACPI, or set `num_resources` to 0 for ACPI devi= ces. --- Generated by Claude Code Patch Reviewer