Re: [PATCH 4/4] drm/imagination: Simplify MIPS PTE definitions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mm: PTE state consistency violation in get_mips_pte_flags() for write-only mappings
- [Low] drm/imagination: Broken macro reference in PVR_MIPSFW_TLB_GET_PA
--

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

drm/imagination: Simplify MIPS PTE definitions

This commit simplifies the MIPS PTE format definitions by using BIT() and
GENMASK() macros. It also removes some unused definitions that add no value.

> diff --git a/drivers/gpu/drm/imagination/pvr_mips_internal.h b/drivers/gpu/drm/imagination/pvr_mips_internal.h
> index 732730d64bf54..2b25a69003b31 100644
> --- a/drivers/gpu/drm/imagination/pvr_mips_internal.h
> +++ b/drivers/gpu/drm/imagination/pvr_mips_internal.h
[ ... ]
> -/* Mask used for the MIPS Page Table in case of physical bus on more than 32 bit. */
> -#define PVR_MIPSFW_ENTRYLO_PFN_MASK_ABOVE_32BIT (0x3FFFFFC0)
[ ... ]
> +#define PVR_MIPSFW_ENTRYLO_PFN_ABOVE_32BIT GENMASK_U32(29, 6)

[Severity: Low]
Could this deletion break compilation for PVR_MIPSFW_TLB_GET_PA()?

While the macro PVR_MIPSFW_TLB_GET_PA() isn't used in this patch, it still
references the deleted PVR_MIPSFW_ENTRYLO_PFN_MASK_ABOVE_32BIT constant in
drivers/gpu/drm/imagination/pvr_mips_internal.h:

#define PVR_MIPSFW_TLB_GET_PA(entry_lo) (((u64)(entry_lo) & \
					  PVR_MIPSFW_ENTRYLO_PFN_MASK_ABOVE_32BIT) << 6)

Leaving a broken macro with an undeclared identifier could cause an immediate
compilation failure if any future code attempts to use it.

> diff --git a/drivers/gpu/drm/imagination/pvr_vm_mips.c b/drivers/gpu/drm/imagination/pvr_vm_mips.c
> index 40495b1486a26..7f49314bc6be4 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm_mips.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm_mips.c
[ ... ]
> @@ -145,15 +147,15 @@ get_mips_pte_flags(bool read, bool write, u32 cache_policy)
>  	u32 flags = 0;
>  
>  	if (read && write) /* Read/write. */
> -		flags |= PVR_MIPSFW_ENTRYLO_DIRTY_EN;
> +		flags |= PVR_MIPSFW_ENTRYLO_DIRTY;
>  	else if (write)    /* Write only. */
> -		flags |= PVR_MIPSFW_ENTRYLO_READ_INHIBIT_EN;
> +		flags |= PVR_MIPSFW_ENTRYLO_READ_INHIBIT;

[Severity: Medium]
Will this construct a non-writable mapping for write-only pages?

For MIPS hardware, the DIRTY bit functions as the hardware write-enable bit,
and a writable PTE must have this bit set; otherwise, any store operation
triggers a TLB Modified exception.

In this write-only branch in get_mips_pte_flags(), the code sets
PVR_MIPSFW_ENTRYLO_READ_INHIBIT but fails to set PVR_MIPSFW_ENTRYLO_DIRTY.
This will cause firmware writes to these pages to trap and crash the
firmware. Should this branch also set the DIRTY bit?

>  	else
>  		WARN_ON(!read);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-b4-simplify-mips-pte-defs-v1-0-b4ad7bb68e22@imgtec.com?part=4