[bug report] drm/imagination: Implement MIPS firmware processor and MMU support

[Dan Carpenter]

Hello Sarah Walker,

The patch 927f3e0253c1: "drm/imagination: Implement MIPS firmware
processor and MMU support" from Nov 22, 2023 (linux-next), leads to
the following Smatch static checker warning:

	drivers/gpu/drm/imagination/pvr_vm_mips.c:204 pvr_vm_mips_map()
	warn: 'pfn' can be negative (type promoted to high)

drivers/gpu/drm/imagination/pvr_vm_mips.c
    144 int
    145 pvr_vm_mips_map(struct pvr_device *pvr_dev, struct pvr_fw_object *fw_obj)
    146 {
    147         struct pvr_fw_device *fw_dev = &pvr_dev->fw_dev;
    148         struct pvr_fw_mips_data *mips_data = fw_dev->processor_data.mips_data;
    149         struct pvr_gem_object *pvr_obj = fw_obj->gem;
    150         const u64 start = fw_obj->fw_mm_node.start;
    151         const u64 size = fw_obj->fw_mm_node.size;
    152         u64 end;
    153         u32 cache_policy;
    154         u32 pte_flags;
    155         u32 start_pfn;
    156         u32 end_pfn;
    157         s32 pfn;

pfn is s32 but start_pfn is u32.

    158         int err;
    159 
    160         if (check_add_overflow(start, size - 1, &end))
    161                 return -EINVAL;
    162 
    163         if (start < ROGUE_FW_HEAP_BASE ||
    164             start >= ROGUE_FW_HEAP_BASE + fw_dev->fw_heap_info.raw_size ||
    165             end < ROGUE_FW_HEAP_BASE ||
    166             end >= ROGUE_FW_HEAP_BASE + fw_dev->fw_heap_info.raw_size ||
    167             (start & ROGUE_MIPSFW_PAGE_MASK_4K) ||
    168             ((end + 1) & ROGUE_MIPSFW_PAGE_MASK_4K))
    169                 return -EINVAL;
    170 
    171         start_pfn = (start & fw_dev->fw_heap_info.offset_mask) >> ROGUE_MIPSFW_LOG2_PAGE_SIZE_4K;

Can start_pfn be zero?

    172         end_pfn = (end & fw_dev->fw_heap_info.offset_mask) >> ROGUE_MIPSFW_LOG2_PAGE_SIZE_4K;
    173 
    174         if (pvr_obj->flags & PVR_BO_FW_FLAGS_DEVICE_UNCACHED)
    175                 cache_policy = ROGUE_MIPSFW_UNCACHED_CACHE_POLICY;
    176         else
    177                 cache_policy = mips_data->cache_policy;
    178 
    179         pte_flags = get_mips_pte_flags(true, true, cache_policy);
    180 
    181         for (pfn = start_pfn; pfn <= end_pfn; pfn++) {
    182                 dma_addr_t dma_addr;
    183                 u32 pte;
    184 
    185                 err = pvr_fw_object_get_dma_addr(fw_obj,
    186                                                  (pfn - start_pfn) <<
    187                                                  ROGUE_MIPSFW_LOG2_PAGE_SIZE_4K,
    188                                                  &dma_addr);
    189                 if (err)
    190                         goto err_unmap_pages;
    191 
    192                 pte = ((dma_addr >> ROGUE_MIPSFW_LOG2_PAGE_SIZE_4K)
    193                        << ROGUE_MIPSFW_ENTRYLO_PFN_SHIFT) & mips_data->pfn_mask;
    194                 pte |= pte_flags;
    195 
    196                 WRITE_ONCE(mips_data->pt[pfn], pte);
    197         }
    198 
    199         pvr_mmu_flush_request_all(pvr_dev);
    200 
    201         return 0;
    202 
    203 err_unmap_pages:
--> 204         for (; pfn >= start_pfn; pfn--)
                       ^^^^^^^^^^^^^^^^
If start_pfn can be zero then this is an endless loop.  I've seen this
code in other places as well.  This loop is slightly off as well.  It
should decrement pfn on the first iteration.

There are hack arounds for using unsigned iterators:

	while (pfn-- > start_pfn)
		WRITE_ONCE(mips_data->pt[pfn], 0);

Personally I would be tempted to make things signed...  Here are some
links to various rants I have wrote:
https://staticthinking.wordpress.com/2022/06/01/unsigned-int-i-is-stupid/
https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/
You didn't ask for rants...  No one ever asks for rants...

    205                 WRITE_ONCE(mips_data->pt[pfn], 0);
    206 
    207         pvr_mmu_flush_request_all(pvr_dev);
    208         WARN_ON(pvr_mmu_flush_exec(pvr_dev, true));
    209 
    210         return err;
    211 }

regards,
dan carpenter