From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: accel/ivpu: Update FW Boot API to version 3.29.4 Date: Sat, 21 Feb 2026 05:55:39 +1000 Message-ID: In-Reply-To: <20260220160116.220367-1-maciej.falkowski@linux.intel.com> References: <20260220160116.220367-1-maciej.falkowski@linux.intel.com> <20260220160116.220367-1-maciej.falkowski@linux.intel.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review The commit message says "Remove unused boot parameters from the vpu_firmware_header structure," but no fields are removed from `vpu_firmware_header` -- all the removals are from `vpu_boot_params`. This is a minor inaccuracy in the commit message. The layout preservation is correct throughout. I verified each substitution: > - u32 pll[VPU_BOOT_PLL_COUNT][VPU_BOOT_PLL_OUT_COUNT]; > + u32 reserved_1[12]; 3 * 4 = 12 elements. Size preserved. > - u64 global_memory_allocator_base; > - u32 global_memory_allocator_size; > + u32 reserved_3[3]; 8 + 4 = 12 bytes = 3 * u32. Size preserved. > - u32 mmu_update_request_irq; > - u32 mmu_update_done_irq; > - u32 set_power_level_irq; > - u32 set_power_level_done_irq; > - u32 set_vpu_idle_update_irq; > - u32 metric_query_event_irq; > - u32 metric_query_event_done_irq; > - u32 preemption_done_irq; > - u32 pad3[52]; > + u32 reserved_5[60]; 8 + 52 = 60. Size preserved. > - u32 pad7[14]; > - /* Warm boot information: 0x400 - 0x43F */ > - u32 warm_boot_sections_count; > - u32 warm_boot_start_address_reference; > - u32 warm_boot_section_info_address_offset; > - u32 pad5[13]; > + u32 reserved_8[30]; 14 + 3 + 13 = 30. Size preserved. > - u32 temp_sensor_period_ms; > + u32 reserved_6; Single u32 to single u32. Size preserved. All removed fields (`pll`, `global_memory_allocator_*`, the IRQ fields, `warm_boot_*`, `temp_sensor_period_ms`) and macros (`VPU_BOOT_PLL_COUNT`, `VPU_BOOT_PLL_OUT_COUNT`) have no references anywhere in the driver code, so the removals are safe. The `vpu_warm_boot_section` struct removal is also confirmed unused. One minor point on the doxygen comment conversion: > - VPU_GOV_DEFAULT = 0, /* Default Governor for the system */ > + VPU_GOV_DEFAULT = 0, /** Default Governor for the system */ Using `/**` for inline trailing comments on enum values is non-standard doxygen usage. Doxygen trailing member comments typically use `/**<` (with the `<` marker) to associate the comment with the preceding item rather than the following one. As written, `/**` on the same line after the value may be interpreted by doxygen as documenting the *next* enum value rather than the current one. This is a cosmetic issue and does not affect compilation or correctness, but if the intent is to generate proper doxygen documentation, these should be `/**<` instead. The same issue applies to these trailing `/**` comments: > + u64 save_restore_ret_address; /** stores the address of FW's restore entry point */ > + u64 verbose_tracing_sw_component_mask; /** TO BE REMOVED */ > + u32 alignment; /** 64, 128, 256 */ The previously anonymous struct for power state timestamps now has a tag name: > - struct { > + struct power_states_timestamps { This is a harmless change -- the tag name is not referenced elsewhere and does not affect the layout. The commit message states that unused boot parameters are removed "from the vpu_firmware_header structure." Should this say `vpu_boot_params` instead? The `vpu_firmware_header` structure is not modified by this patch (other than comment style changes). No correctness issues found. The patch is a clean header update that preserves the binary layout. --- Generated by Claude Code Patch Reviewer