Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/imagination: under-allocation of page tables due to incorrect start_addr computation --
commit 2406b8d6a0a1f7f5c8f27c8165d80a69f8753892 Author: Brajesh Gupta <[email protected]> drm/imagination: Fix page count for page table for map() interface This patch updates pvr_mmu_op_context_create() to accept a device address and includes it in the page table count calculations for the map() interface. This aims to fix mapping failures that occurred when the virtual start address was omitted. > diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c > b/drivers/gpu/drm/imagination/pvr_mmu.c > --- a/drivers/gpu/drm/imagination/pvr_mmu.c > +++ b/drivers/gpu/drm/imagination/pvr_mmu.c [ ... ] > @@ -2347,8 +2348,9 @@ void pvr_mmu_op_context_destroy(struct > pvr_mmu_op_context *op_ctx) > */ > struct pvr_mmu_op_context * > pvr_mmu_op_context_create(struct pvr_mmu_context *ctx, struct sg_table *sgt, > - u64 sgt_offset, u64 size) > + u64 device_addr, u64 sgt_offset, u64 size) > { > + u64 start_addr = device_addr + sgt_offset; [Severity: High] Since device_addr is the virtual address where the mapping starts, and sgt_offset is the physical offset within the backing buffer object, does adding the physical offset to the virtual address incorrectly shift the preallocation start address here? > int err; > > struct pvr_mmu_op_context *op_ctx = kzalloc_obj(*op_ctx); > @@ -2364,16 +2366,16 @@ pvr_mmu_op_context_create(struct pvr_mmu_context > *ctx, struct sg_table *sgt, > if (size) { > /* > * The number of page table objects we need to prealloc is > - * indicated by the mapping size, start offset and the sizes > + * indicated by the mapping size, start address and the sizes > * of the areas mapped per PT or PD. The range calculation is > * identical to that for the index into a table for a device > * address, so we reuse those functions here. > */ > - const u32 l1_start_idx = pvr_page_table_l2_idx(sgt_offset); > - const u32 l1_end_idx = pvr_page_table_l2_idx(sgt_offset + size); > + const u32 l1_start_idx = pvr_page_table_l2_idx(start_addr); > + const u32 l1_end_idx = pvr_page_table_l2_idx(start_addr + size); > const u32 l1_count = l1_end_idx - l1_start_idx + 1; > - const u32 l0_start_idx = pvr_page_table_l1_idx(sgt_offset); > - const u32 l0_end_idx = pvr_page_table_l1_idx(sgt_offset + size); > + const u32 l0_start_idx = pvr_page_table_l1_idx(start_addr); > + const u32 l0_end_idx = pvr_page_table_l1_idx(start_addr + size); [Severity: High] By using this shifted start_addr, could we under-allocate page tables if the actual virtual mapping from device_addr to device_addr + size crosses a page table boundary (like a 2MB L0 block), but the incorrectly shifted start_addr range does not? If we preallocate fewer tables than needed, pvr_page_table_l0_get_or_insert() might fail to pop from the empty preallocation list during pvr_mmu_map() and return -ENOMEM, causing valid user mapping requests to unpredictably fail. > const u32 l0_count = l0_end_idx - l0_start_idx + 1; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
