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 <[email protected]> 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/[email protected]?part=4
