Re: [PATCH v5 3/4] drm/imagination: Improve some logging in error paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/imagination: wrong page count check in pvr_vm_get_pt_phys_addrs()
--

commit b112075b03dc02481bd49eeee6ccd64222b769b3
Author: Matt Coster <opensource@mtcoster.net>

drm/imagination: Improve some logging in error paths

This commit adds error logging when attempting to map an already mapped
firmware object, unmap an already unmapped firmware object, or provide too
few DMA address slots for the MIPS firmware page table.

> diff --git a/drivers/gpu/drm/imagination/pvr_vm_mips.c b/drivers/gpu/drm/imagination/pvr_vm_mips.c
> index 726f900d35827..b74386032e6ab 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm_mips.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm_mips.c
[ ... ]
> @@ -278,8 +279,12 @@ pvr_vm_get_pt_phys_addrs(struct pvr_device *pvr_dev, u64 *phys_addrs, int max_nu
>  	struct pvr_fw_device *fw_dev = &pvr_dev->fw_dev;
>  	struct pvr_vm_mips_data *vm_data = fw_dev->processor_data.mips_data.vm_data;
>  
> -	if (ARRAY_SIZE(vm_data->pt_pages) > max_num_addrs)
> +	if (ARRAY_SIZE(vm_data->pt_pages) > max_num_addrs) {
> +		drm_err(from_pvr_device(pvr_dev),
> +			"Minimum %zu pages required for FW PT, but only %d DMA address slots were provided\n",
> +			ARRAY_SIZE(vm_data->pt_pages), max_num_addrs);
>  		return -EINVAL;
> +	}
>  
>  	for (u32 page_nr = 0; page_nr < max_num_addrs; page_nr++) {
>  		/* Firmware expects 4k pages, but host page size might be different. */

[Severity: Medium]
As reported by Sashiko on v4 [1], in pvr_vm_get_pt_phys_addrs() in
drivers/gpu/drm/imagination/pvr_vm_mips.c, vm_data->pt_pages and
vm_data->pt_dma_addr are sized by PVR_MIPS_PT_PAGE_COUNT, which is the
number of host PAGE_SIZE pages rather than 4K firmware pages:

#define PVR_MIPS_PT_PAGE_COUNT \
	DIV_ROUND_UP(PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES * PVR_MIPSFW_PAGE_SIZE_4K, PAGE_SIZE)

However, phys_addrs and max_num_addrs are in units of 4K firmware pages
(requiring PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES slots), as shown by the loop
body translating each 4K page_nr into a host page index:

	for (u32 page_nr = 0; page_nr < max_num_addrs; page_nr++) {
		/* Firmware expects 4k pages, but host page size might be different. */
		u32 src_page_nr = (page_nr * PVR_MIPSFW_PAGE_SIZE_4K) >> PAGE_SHIFT;
		u32 page_offset = (page_nr * PVR_MIPSFW_PAGE_SIZE_4K) & ~PAGE_MASK;

		phys_addrs[page_nr] = vm_data->pt_dma_addr[src_page_nr] + page_offset;
	}

On hosts with PAGE_SIZE > 4K (such as 16K or 64K page kernels),
ARRAY_SIZE(vm_data->pt_pages) is 1 while 4 DMA address slots are still
required for the firmware page table. Checking and logging
ARRAY_SIZE(vm_data->pt_pages) allows an undersized buffer of 1 to 3 slots
to pass validation on those kernels and logs the host page count instead of
the required 4K page table slot count.

Additionally, because max_num_addrs is a signed int compared against
unsigned ARRAY_SIZE() and u32 page_nr, and the loop iterates up to
max_num_addrs rather than PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES, passing
max_num_addrs > PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES or a negative
max_num_addrs will cause src_page_nr to read out of bounds from
vm_data->pt_dma_addr[].

Should the check, error message, and loop bound use
PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES instead of ARRAY_SIZE(vm_data->pt_pages)
and max_num_addrs?

[1] https://lore.kernel.org/r/20261029-b4-simplify-mips-pte-defs-v4-3-9656041e040b@imgtec.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-b4-simplify-mips-pte-defs-v5-0-d5bcf7d95a1a@imgtec.com?part=3