Re: [PATCH v4 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 d2b300f194de238da66cb86085f9b887ba2e3307
Author: Matt Coster <opensource@mtcoster.net>

drm/imagination: Improve some logging in error paths

This commit adds drm_err() messages 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;
> +	}

[Severity: Medium]
Should pvr_vm_get_pt_phys_addrs() check and log
PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES instead of ARRAY_SIZE(vm_data->pt_pages)?

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:

#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
(PVR_MIPSFW_PAGE_SIZE_4K), as seen in the loop immediately following this
check:

	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 a host with 16K or 64K pages, ARRAY_SIZE(vm_data->pt_pages) is 1 while
PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES is 4, so a max_num_addrs of 1 to 3 would
pass the check and receive an incomplete page table, or log that a minimum of
1 page is required when 4 DMA address slots are needed.

Also, because max_num_addrs is a signed int compared against unsigned types
and used as the loop bound instead of PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES,
could a negative max_num_addrs or a value greater than
PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES cause src_page_nr to read out of bounds
from vm_data->pt_dma_addr[]?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-b4-simplify-mips-pte-defs-v4-0-9656041e040b@imgtec.com?part=3