Re: [PATCH v6 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 to consider:
- [Medium] drm/imagination: wrong page count check in
  pvr_vm_get_pt_phys_addrs()
--

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

drm/imagination: Improve some logging in error paths

This commit adds drm_err() messages to error paths in
pvr_fw_object_fw_map(), pvr_fw_object_fw_unmap(), and
pvr_vm_get_pt_phys_addrs().

> 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]
Does this check and log ARRAY_SIZE(vm_data->pt_pages) in host pages instead
of the 4K firmware page count PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES?

vm_data->pt_pages and vm_data->pt_dma_addr are sized in host pages:

drivers/gpu/drm/imagination/pvr_vm_mips.c:
#define PVR_MIPS_PT_PAGE_COUNT \
	DIV_ROUND_UP(PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES * PVR_MIPSFW_PAGE_SIZE_4K, PAGE_SIZE)

struct pvr_vm_mips_data {
	struct page *pt_pages[PVR_MIPS_PT_PAGE_COUNT];
	...
	dma_addr_t pt_dma_addr[PVR_MIPS_PT_PAGE_COUNT];

On a host with 16K or 64K PAGE_SIZE, ARRAY_SIZE(vm_data->pt_pages) is 1,
whereas the firmware page table requires PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES
(4) 4K DMA address slots in phys_addrs.  A max_num_addrs of 1 to 3 passes
this check on 16K/64K page kernels and leaves the firmware page table
incomplete, or logs 1 instead of 4 as the required minimum.

Also, max_num_addrs is a signed int compared against unsigned size_t
ARRAY_SIZE() and used as the upper bound of the loop, so a negative
max_num_addrs or a value larger than PVR_MIPSFW_MAX_NUM_PAGETABLE_PAGES
causes src_page_nr to read out of bounds from vm_data->pt_dma_addr[].

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-b4-simplify-mips-pte-defs-v6-0-65153bc9a71c@imgtec.com?part=3